Float Like A Butterfly: Introduce PresenceBox, TryBox, and ParamTryBox. #1876

Shadowfiend · 2017-07-07T02:50:12Z

These three intermediary types allow APIs and consumers of boxes to indicate
that they expect a particular subset of Box types. This is useful to restrict
types to what is expected, while still being able to interoperate with
APIs designed to interact with many all Box types.

For example, you can declare that a function expectes a PresenceBox to ensure
that interactors guarantee either a Full or Empty is passed. Or, you can
specify that your function returns a TryBox so callers only need to worry about
handling a Full or a Failure. Lastly, ParamTryBox allows indicating that a
given situation will only get a Full or a ParamFailure with a particular error
type.

A lot of tests and additional API conversions are required to fully bring this
vision to fruition, but this is the foundation for that work.

Required before this is ready to merge:

Proper typing of all methods in Box.scala
Initial battery of specs (with appropriate type annotations)

Of note, the compiler will already properly infer, say, PresenceBox if your
code only produces Full or Empty. It will also infer ParamTryBox[String, Int],
for example, if your code only produces Full or ParamFailure[Int].

Hoping I can get this wrapped up in time for 3.2.0-M2, but we'll see ;)

These three intermediary types allow APIs and consumers of boxes to indicate that they expect a particular subset of Box types. This is useful to restrict types to what is expected, while still being able to interoperate with APIs designed to interact with many all Box types. For example, you can declare that a function expectes a PresenceBox to ensure that interactors guarantee either a Full or Empty is passed. Or, you can specify that your function returns a TryBox so callers only need to worry about handling a Full or a Failure. Lastly, ParamTryBox allows indicating that a given situation will only get a Full or a ParamFailure with a particular error type. A lot of tests and additional API conversions are required to fully bring this vision to fruition, but this is the foundation for that work.

This allows us to make sure that we don't lose the specific type of the Box when doing a pass operation. This was less important when the only supertype was `Box`, but now that we can have the more specific `PresenceBox`, `TryBox`, and `ParamTryBox`, preserving those supertypes is more important.

Bhashit · 2017-07-09T08:51:48Z

Ah, looks like I was a bit late in pushing these changes. I did have most of it, was stuck with a serializer and didn't get enough time for that one. Looking great. 🎉

Shadowfiend · 2017-07-09T23:45:59Z

Ugh, sorry @Bhashit… I kind of did this all in one day, and in my head you were working on the combinators but not the typing changes. Re-reading the original issue, I had no reason to think that, and should have let you know I was going to put pedal to the metal on cranking something out.

In particular, collect and collectFirst will always return a PresenceBox when invoked on a PresenceBox. This is not the case for the other subtypes.

In particular, cases where each of these can guarantee a subtype of Box have reimplementations that do so. PresenceBox also adds a specialization of withFilter, which is used in for comprehensions, that ensures that typing carries through.

These can now have more specific types.

Also added a blurb to the Box scaladocs regarding the Box subtypes.

Shadowfiend · 2017-07-09T23:47:41Z

Pushed a bunch more changes here… One note for you @Bhashit particularly for collectFailure in #1864---I had to un-final the collect methods to provide more specific types in PresenceBox for them.

Bhashit · 2017-07-10T03:09:49Z

in my head you were working on the combinators but not the typing changes

No problem at all. I am pretty sure you'd do it better.

I had to un-final the collect methods

Sounds good.

farmdawgnation · 2017-07-10T16:53:32Z

Holding off on review here until the tests are passing, but this looks exciting. :)

farmdawgnation · 2017-07-23T17:22:01Z

Tests still are not passing. :(

Shadowfiend · 2017-07-23T18:50:47Z

Well no changes have been pushed so that seems perfectly reasonable :p

farmdawgnation · 2017-07-23T18:57:48Z

BUT I WANT THE FEATURES NOWWWWWWWWWWWWW 👶

Due to the way Scala resolves method overloads with implicit parameters, I haven't been able to find a good way of defining that a TryBox that contains a TryBox should flatten to a TryBox, and a PresenceBox that contains a PresenceBox should flatten to a PresenceBox. Unfortunately Scala seems to resolve the overload of flatten based solely on class hierarchy and number of parameters, and then fails the compile if it can't plug the proper implicits in, rather than using the available implicit evidence to choose between overloads. Since we rely entirely on implicit evidence to know what we can do with the contained type of the box, there's no way (that I could find) to express diverging return types based on the contained type :( Instead, TryBox and PresenceBox each have their own flattenTry and flattenPresence methods, respectively, which will only compile if their contents match the container type. This means the caller has to choose the appropriate one to call, but at least it ensures you *can* flatten that way if you really want to.

Shadowfiend · 2017-08-05T16:13:28Z

Sadly I had to jettison a properly-typed flatten implementation, at least for the time being. Details in the commit, but net-net I couldn't figure out how to get the compiler to play nice on making TryBox[TryBox].flatten=>TryBox and PresenceBox[PresenceBox].flatten=>PresenceBox. For now, that means we have flattenTry on TryBox and flattenPresence on PresenceBox that only compile if both types match and produce the right return type.

I don't like this gotcha one bit :(

farmdawgnation

Left some initial remarks throughout.

I'm a bit concerned about the added conceptual complexity here, but that could just be me not having enough coffee yet. Broadly, I think that Box in its current form is quite easy to understand today. After seeing some of the hoops this code has to jump through to work, I'm wondering whether or not we've made Box harder to understand conceptually and therefore increased the barrier to entry for using it and contributing to it.

This is one of those moments where I sincerely wish scala.util.Try supported arbitrary error messages in its constructor, because if it did I would say "Hey, let's not re-invent the wheel here." :/

In any case, the code itself looks like it's moving in the right direction for a workable implementation. :)

farmdawgnation · 2017-08-06T13:50:55Z

core/common/src/main/scala/net/liftweb/common/Box.scala

+  def or[B >: A](alternative: => Full[B]): Full[B]
+  def or[B >: A](alternative: => PresenceBox[B])(implicit a: DummyImplicit): PresenceBox[B]
+  def or[B >: A](alternative: => TryBox[B])(implicit a: DummyImplicit, b: DummyImplicit): TryBox[B]
+  def or[B >: A, E](alternative: => ParamTryBox[B, E])(implicit a: DummyImplicit, b: DummyImplicit, c: DummyImplicit): ParamTryBox[B, E]


I'm idly wondering if the presence of these DummyImplicits might be a sign we've tried to be a bit too clever by incorporating this into the overall box structure instead of having them as a parallel data type with implicit conversions.

Parallel data types with implicit conversions are only an option for a small subset of these methods, because of the fact that some of the methods overlap with Iterable and such, which means if someone imports Box._ (which is not an unreasonable thing to do in a few situations), they'll be unable to get our implicits at a higher specificity than the iterable conversions.

It ain't fun.

That said, these dummy implicits don't have anything to do with the way in which we attach these methods; instead, they are here because we're trying to overload on an parameter type with a call-by-name parameter. Overloading on parameter type is directly supported in the JVM, but by making these by-name we turn them into parameterized Function1s, or something roughly equivalent. That means we're suddenly overloading by the parameter's generic type, which isn't directly supported. Dummy implicits step in to fill this gap.

farmdawgnation · 2017-08-06T13:51:28Z

core/common/src/main/scala/net/liftweb/common/Box.scala

+
+  override def map[B](f: A => B): PresenceBox[B] = Empty
+  def flatMap[B](f: A => PresenceBox[B]): PresenceBox[B]
+  override def collect[B](pf: PartialFunction[A, B]): PresenceBox[B] = ???


We should remove the presence of ??? from anything that'll be production code.

I'll double-check this. I introduced it for a reason, but it may have been an earlier iteration.

Worth noting though---neither of the ??? implementations are used in practice, because they are overridden further in all of their subclasses, and dynamic binding ensures it's those implementations, not these, that are executed.

Yeah, I'm not concerned about that. More about accident proofing the code. If there is a ??? implementation here, the compiler would say, for example, "Hey this interface is satisfied!" If someone were to somehow break the override of a child implementation, this would become the used implementation.

Whereas, if we don't provide ??? here, breaking the override implementation should break the compile.

farmdawgnation · 2017-08-06T13:52:23Z

core/common/src/main/scala/net/liftweb/common/Box.scala

+ */
+sealed trait TryBox[+A] extends Box[A] {
+  def or[B >: A](alternative: => Full[B]): Full[B]
+  def or[B >: A](alternative: => PresenceBox[B])(implicit a: DummyImplicit): PresenceBox[B] // *


Rouge comment on this line?

farmdawgnation · 2017-08-06T13:53:06Z

core/common/src/main/scala/net/liftweb/common/Box.scala

+  def or[B >: A](alternative: => TryBox[B])(implicit a: DummyImplicit, b: DummyImplicit): TryBox[B]
+  def or[B >: A, E2](alternative: => ParamTryBox[B, E2])(implicit a: DummyImplicit, b: DummyImplicit, c: DummyImplicit): ParamTryBox[B, E2]
+
+  override def map[B](f: A => B): ParamTryBox[B, E] = ???


Ditto here with ???

Shadowfiend · 2017-08-14T20:35:48Z

I'm wondering whether or not we've made Box harder to understand conceptually and therefore increased the barrier to entry for using it and contributing to it.

We've definitely increased complexity here, but a lot of the hoops we jump through here are to make the intuition of this added complexity work as expected.

Specifically, we added PresenceBox, TryBox, and ParamTryBox. These let you specify at a type level that you expect certain subsets of the box hierarchy---a situation we find ourselves in quite frequently, more often than not because we're trying to communicate failure-or-full and there's no way to do that without also having to handle the empty state. From a user perspective, this should be the only complication. The rest of the code in this PR (and there is a good bit of added code) is to ensure that the various Box combinators produce the correct subsets in their type signatures, so that if , say, I want to get a TryBox and I have to do some flatMaps to get there, I don't immediately have to resort to type-casting (because flatMap is aware of what's being flatMapped and can maintain proper typing throughout the operation).

With that said, it definitely increases the barrier to entry for contributing to Box. Personally I can't think of anything else that we would need to add, but of course you can't predict the future. I think from continuous experience that this gets us:

A solution to a common problem.
A setup to prevent potentially dangerous type conversions in the future. This one specifically is about the implicit box2Option conversion, which today is always potentially lossy. In a world where PresenceBox is a thing, we can imagine giving PresenceBox an implicit conversion to Option but Box would not (obviously we wouldn't do this until a breaking release cycle, though).

I definitely understand the complexity concern here though, so if we collectively think this is too much, I'm happy to leave it on the table.

farmdawgnation · 2017-08-15T14:17:47Z

I think the complexity concern here is probably worth it given what we're getting. And I can't think of any way to simplify things significantly.

Bhashit · 2017-09-11T03:40:30Z

FWIW, I'd love having this new hieararchy. The pattern matches with Empty and Failure, even when they are not possible outcomes, always feels impure.

Shadowfiend · 2017-09-25T01:57:54Z

Ok, so slight complication here which we'll need to decide if we're okay with… In a sense, this branch is too effective heh. ?~ now returns TryBox, as it should. This means that code that has a for comprehension where each step uses ?~ to ensure a Failure pops out even in case of Empty, and then pattern-matches on the result, will fail compilation if the developer pattern-matched against Empty (if they didn't, they would have gotten a compiler warning prior to this).

We can change ?~ to return Box instead of TryBox, but we start winking at “what even is the point” territory. The compilation error would also be very easy to fix: the Empty case just needs to be nuked.

An example is this chunk of WithParam:

framework/web/webkit/src/main/scala/net/liftweb/builtin/snippet/WithParam.scala

Lines 44 to 58 in aa24c2d

    
           def render(kids: NodeSeq) : NodeSeq = { 
        
             (for { 
        
               ctx <- S.session ?~ ("FIX"+"ME: Invalid session") 
        
               req <- S.request ?~ ("FIX"+"ME: Invalid request") 
        
             } yield { 
        
               val name: String = S.attr("name") openOr "main" 
        
               val body = ctx.processSurroundAndInclude(PageName.get, kids) 
        
                WithParamVar.atomicUpdate(_ + (name -> body)) 
        
                NodeSeq.Empty 
        
             }) match { 
        
               case Full(x) => x 
        
               case Empty => Comment("FIX"+ "ME: session or request are invalid") 
        
               case Failure(msg, _, _) => Comment(msg) 
        
             } 
        
           }

EDIT to say: I think this is still a big step up, and that given the relatively benign nature of the compilation errors and that they happen at compile time (rather than runtime), this is acceptable.

The existing definition wasn't actually catching for all ParamTryBoxes.

Also add some scaladocs to those two methods.

We do this by dropping the concrete definitions of these two methods in Box, preferring instead to lean 100% on late binding to resolve the correct concrete definition of the method based on the concrete type it is being invoked on.

Shadowfiend · 2017-09-25T03:21:37Z

Ok, wrapped up the specs, made a fix or two, nuked the = ??? definitions, and merged in master with @Bhashit's changes...

I think I consider this PR good to go, provided travis doesn't slap me repeatedly in the face with red ❌es.

Shadowfiend · 2017-09-25T03:32:53Z

Ohai Scala 2.11. Looks like still some cleanup to be done. Will circle back!

Shadowfiend · 2017-09-25T03:55:46Z

this is acceptable

Don’t know what I was thinking. Minor releases are API-compatible, by definition. So... that’s awkward heh. Will noodle.

Shadowfiend · 2017-12-07T16:22:33Z

Moved this to hypothetical 4.0 for now… Really bummed about it though :(

Shadowfiend · 2017-12-07T16:24:08Z

(May be worth discussing if this is worth doing an earlier-than-expected 4.0 release with the community… Perhaps after another couple of minor releases.)

farmdawgnation · 2020-06-09T14:43:47Z

Hey there, I'm closing PR as orphaned since it's been more than a year since it's had any updates. Feel free to reopen when you have time to update it!

Shadowfiend · 2020-06-25T19:57:25Z

Not much to update here, tbh. This PR was left open because it can't land before a 4.0 release; it may have some merge conflicts but I suspect I'd be able to dispense with them when the time comes.

farmdawgnation · 2020-06-29T19:15:37Z

Hm, gotcha. I can reopen it.

farmdawgnation · 2020-06-29T19:16:43Z

Also converted it to a Draft so we know it's not an "actually actionable thing" right now

Shadowfiend added this to the 3.2.0-M1 milestone Jul 7, 2017

Shadowfiend added 5 commits July 9, 2017 16:46

Tweak collect and collectFirst typing for PresenceBox.

671a958

In particular, collect and collectFirst will always return a PresenceBox when invoked on a PresenceBox. This is not the case for the other subtypes.

Adjust typing on a few Box singleton functions.

f22afbe

These can now have more specific types.

Scaladoc updates for a few places in Box.

9b906eb

Also added a blurb to the Box scaladocs regarding the Box subtypes.

Add usage docs for CombinableBox.

90ad32f

farmdawgnation self-requested a review July 10, 2017 16:53

farmdawgnation modified the milestones: 3.2.0-M1, 3.2.0-M2 Jul 22, 2017

Shadowfiend mentioned this pull request Aug 5, 2017

Early Tutors: Initial pass at a new tutorial living in the repo #1846

Merged

farmdawgnation requested changes Aug 6, 2017

View reviewed changes

farmdawgnation modified the milestones: 3.2.0-M2, 3.2.0-M3 Sep 15, 2017

Shadowfiend added 6 commits September 24, 2017 22:59

Fix Full flatMap for ParamTryBox.

69bfa97

The existing definition wasn't actually catching for all ParamTryBoxes.

Fix asA to return Failure for Failure.

86f3503

Fill in remaining typing specs.

8679ca2

Move flattenPresence/flattenTry specs into their own examples.

28d5d57

Also add some scaladocs to those two methods.

Drop ??? definitions of collect and map.

4b8fb17

We do this by dropping the concrete definitions of these two methods in Box, preferring instead to lean 100% on late binding to resolve the correct concrete definition of the method based on the concrete type it is being invoked on.

Merge remote-tracking branch 'origin/master' into float-like-a-butterfly

f58523f

Shadowfiend mentioned this pull request Nov 5, 2017

Removing CometActors from page #1655

Open

farmdawgnation modified the milestones: 3.2.0-M3, 3.2.0-RC1 Nov 17, 2017

Shadowfiend modified the milestones: 3.2.0-RC1, Hypothetical 4.0 Dec 7, 2017

farmdawgnation closed this Jun 9, 2020

farmdawgnation reopened this Jun 29, 2020

farmdawgnation marked this pull request as draft June 29, 2020 19:15

farmdawgnation added the Lift5 Features/Changes for Lift 5 label Jul 13, 2024

farmdawgnation removed this from the Hypothetical 4.0 milestone Jul 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Float Like A Butterfly: Introduce PresenceBox, TryBox, and ParamTryBox. #1876

Float Like A Butterfly: Introduce PresenceBox, TryBox, and ParamTryBox. #1876

Shadowfiend commented Jul 7, 2017 •

edited

Loading

Bhashit commented Jul 9, 2017 •

edited

Loading

Shadowfiend commented Jul 9, 2017

Shadowfiend commented Jul 9, 2017

Bhashit commented Jul 10, 2017 •

edited

Loading

farmdawgnation commented Jul 10, 2017

farmdawgnation commented Jul 23, 2017

Shadowfiend commented Jul 23, 2017

farmdawgnation commented Jul 23, 2017

Shadowfiend commented Aug 5, 2017

farmdawgnation left a comment

farmdawgnation Aug 6, 2017

Shadowfiend Aug 14, 2017

farmdawgnation Aug 6, 2017

Shadowfiend Aug 14, 2017

Shadowfiend Aug 14, 2017

farmdawgnation Aug 15, 2017

farmdawgnation Aug 6, 2017

Shadowfiend Aug 14, 2017

farmdawgnation Aug 6, 2017

Shadowfiend commented Aug 14, 2017

farmdawgnation commented Aug 15, 2017

Bhashit commented Sep 11, 2017

Shadowfiend commented Sep 25, 2017 •

edited

Loading

Shadowfiend commented Sep 25, 2017

Shadowfiend commented Sep 25, 2017

Shadowfiend commented Sep 25, 2017

Shadowfiend commented Dec 7, 2017

Shadowfiend commented Dec 7, 2017

farmdawgnation commented Jun 9, 2020

Shadowfiend commented Jun 25, 2020

farmdawgnation commented Jun 29, 2020

farmdawgnation commented Jun 29, 2020 •

edited

Loading

Float Like A Butterfly: Introduce PresenceBox, TryBox, and ParamTryBox. #1876

Are you sure you want to change the base?

Float Like A Butterfly: Introduce PresenceBox, TryBox, and ParamTryBox. #1876

Conversation

Shadowfiend commented Jul 7, 2017 • edited Loading

Bhashit commented Jul 9, 2017 • edited Loading

Shadowfiend commented Jul 9, 2017

Shadowfiend commented Jul 9, 2017

Bhashit commented Jul 10, 2017 • edited Loading

farmdawgnation commented Jul 10, 2017

farmdawgnation commented Jul 23, 2017

Shadowfiend commented Jul 23, 2017

farmdawgnation commented Jul 23, 2017

Shadowfiend commented Aug 5, 2017

farmdawgnation left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shadowfiend commented Aug 14, 2017

farmdawgnation commented Aug 15, 2017

Bhashit commented Sep 11, 2017

Shadowfiend commented Sep 25, 2017 • edited Loading

Shadowfiend commented Sep 25, 2017

Shadowfiend commented Sep 25, 2017

Shadowfiend commented Sep 25, 2017

Shadowfiend commented Dec 7, 2017

Shadowfiend commented Dec 7, 2017

farmdawgnation commented Jun 9, 2020

Shadowfiend commented Jun 25, 2020

farmdawgnation commented Jun 29, 2020

farmdawgnation commented Jun 29, 2020 • edited Loading

Shadowfiend commented Jul 7, 2017 •

edited

Loading

Bhashit commented Jul 9, 2017 •

edited

Loading

Bhashit commented Jul 10, 2017 •

edited

Loading

Shadowfiend commented Sep 25, 2017 •

edited

Loading

farmdawgnation commented Jun 29, 2020 •

edited

Loading