Make working with promises of promises easier #3813

SeanTAllen · 2021-07-31T17:03:38Z

Adds next_unwrap method to Promise. next_unwrap makes working
with types like Promise[Promise[Foo]] much easier to work with
compared to using next.

The idea for next_unwrap was my idea while working on a Pony client
for the GitHub REST API. The implementation itself was written by
Joe McIlvain.

Adds `next_unwrap` method to `Promise`. `next_unwrap` makes working with types like `Promise[Promise[Foo]]` much easier to work with compared to using `next`. The idea for `next_unwrap` was my idea while working on a Pony client for the GitHub REST API. The implementation itself was written by Joe McIlvain.

SeanTAllen · 2021-07-31T17:05:01Z

This needs to be squashed once approved. I'll take care of that.

I have a GitHub REST API library that an early version of will be released after this is merged and I do a ponyc release.

jasoncarr0 · 2021-07-31T19:36:45Z

Are we settled on the naming here? In most contexts I see a method like this is almost always named as flat_map or rarely and_then/bind, do we have a chance to lean into one of these namings? Would flat_next or next_flatten be more suitable to reduce context switching overhead between Pony and non-Pony? I suppose this would be discussable by a tiny RFC

SeanTAllen · 2021-07-31T21:02:34Z

I think Joe's next_unwrap is a good name in the context.

I think that flat_next is probably in the same ballpark as next_unwrap for "i might guess what this does" if you don't have the FP background.

Of the two that Jason mentioned, i find next_flatten to be preferable.

Of the existing and Jason's two names, I'm pretty apathetic. Any of the three is about the same to me.

@jemc you did the naming from my original "when", do you have a preference?

I have a library to release that is waiting on this, so i'd like to make any decision quickly.

rhagenson

No problems with this addition. Few changes suggested for formatting and wording. I also am wondering why the test does not have an assertion for both parts of the chain both "start" and "end" values?

.release-notes/next-unwrap.md

rhagenson · 2021-08-01T17:33:22Z

packages/promises/_test.pony

+    start("start")
+    inter("end")
+
+primitive _NextUnwrapHelper
+  fun promise_string(s: String): Promise[String] =>
+    // fake do something with string, return promise of another
+    Promise[String]
+
+  fun complete(h: TestHelper, s: String) =>
+    h.assert_eq[String]("end", s)


I see an assertion for "end" here, but not one for "start" -- should a "start" assertion be added?

Start exists entirely so we can have something to call next_unwrap on. Think testing its value in promise_string makes the case that we are testing less clear.

Fair enough. My rationale was from a perspective of "tests as documentation" to show where in the process "start" is accessible. For the same reason of "tests as documentation" I agree that as it is makes it clear where "end" is accessible -- which is the additional functionality here.

packages/promises/promise.pony

Co-authored-by: Ryan A. Hagenson <[email protected]>

jemc · 2021-08-01T20:12:16Z

I approve of using flat or flatten as a word instead of unwrap. It's definitely no worse than unwrap for describing what's going on. And since Jason sees it as better, I'm good with that. 👍

So I'd be good with next_flat, next_flatten, flat_next, or flatten_next (only the 2nd and 3rd of which were actually suggested by Jason above).

SeanTAllen · 2021-08-01T21:04:58Z

@jasoncarr0 what do you think of flatten_next? I like it the most.

SeanTAllen · 2021-08-01T21:05:48Z

at the moment however i have renamed to next_flatten and I'm good with that as well. I'll leave next_flatten and flatten_next choice to @jasoncarr0.

packages/promises/_test.pony

jasoncarr0 · 2021-08-01T22:15:13Z

packages/promises/promise.pony

+          inner.next[None](
+              {(fulfilled: B) =>
+                p(fulfilled)
+              })


Shouldn't we handle rejection from inner here and reject p?

I'm not sure what you mean. we handle in the else below.

If we reach line 216 then there are no more sources of errors, so we'll never reach the else block.

But inner is still a future we need to respect. If inner rejects, then so should this method, but it never will, since this next call isn't passing a reject callback and so is using the default that does nothing on a reject and hence the returned lambda will never accept nor reject if inner is produced but fails.

Yeah, I think Jason is correct about the missing case.

Sorry @jasoncarr0 I'm still not getting it.

Do you have code that you think fixes the issue? Perhaps seeing the proposed fix will help me understand what you are talking about.

@jemc given that I didnt write the code, if you have time to try and figure out how to cover the two holes, I can test it. But I'm going to take a shot at the missing case I see although I'm not sure how to cover it.

Suggested change

})

}, {() => p.reject()})

So lesson learned make a suggestion first as that's a lot faster than trying to convey it otherwise

Ah I see Joe made the suggestion down below and you got it working so we're good here

jasoncarr0

Looks like a case might be missing?

Co-authored-by: Ryan A. Hagenson <[email protected]>

SeanTAllen · 2021-08-02T00:53:33Z

I realized that my test case is totally bogus and I'm trying to figure out how to actually test this. Its non-trivial.

SeanTAllen · 2021-08-02T01:08:04Z

ok figured it out. god that test did NOTHING. dur.

SeanTAllen · 2021-08-02T01:48:13Z

correction, i figured some out.

i have other tests that are failing and... i can try out with real world stuff from my library that this started in and they work, so... arg.

SeanTAllen · 2021-08-02T02:20:01Z

@jemc we definitely missed a case.

There outer promise is always using RejectAlways which doesnt send down the chain to the inner.

jemc · 2021-08-02T02:32:22Z

packages/promises/promise.pony

+              {(fulfilled: B) =>
+                p(fulfilled)
+              })
+        else


@SeanTAllen - I think something like this is the right fix that @jasoncarr0 was pointing out.

I didn't try compiling it so there might be something slightly wrong but it looks correct to me.

Suggested change

{(fulfilled: B) =>

p(fulfilled)

})

else

{(fulfilled: B) => p(fulfilled) },

{() : B? => p.reject(); error })

else

that sadly doesnt compile

/home/sean/code/ponylang/ponyc/packages/promises/promise.pony:226:13: argument not a subtype of parameter
{() : B? => p.reject(); error})
^
Info:
/home/sean/code/ponylang/ponyc/packages/promises/promise.pony:226:13: argument type is {(): B ?}[A #share, B #share] iso^
{() : B? => p.reject(); error})
^
/home/sean/code/ponylang/ponyc/packages/promises/promise.pony:48:5: parameter type is Reject[None val] iso^
rejected: Reject[B] = RejectAlways[B])
^

This compiles:

{(fulfilled: B) => p(fulfilled)}, {()? => p.reject(); error})

SeanTAllen · 2021-08-02T03:11:18Z

@jasoncarr0 @jemc what is the test case for the issue that Jason pointed out?

SeanTAllen · 2021-08-02T03:15:04Z

@jasoncarr0 can you review the new tests and tell me if you think anything is missing?

SeanTAllen · 2021-08-02T03:31:11Z

This test also fails:

class iso _TestFlattenNextStartFulfillErrors is UnitTest
  fun name(): String => "promises/Promise.flatten_next/start_fulfill_errors"
  fun apply(h: TestHelper) =>
    let initial_string = "foo"
    let expected_reject_string = "rejected"

    h.long_test(2_000_000_000)
    h.expect_action(initial_string)
    h.expect_action(expected_reject_string)

    let start = Promise[String]
    let inter = start.flatten_next[String](
      _FlattenNextStartHelper~error_fulfill(h, initial_string),
      _FlattenNextStartHelper~reject_fail_if_called(h)
    )
    inter.next[None](
      _FlattenNextInterHelper~fulfill_fail_if_called(h),
      _FlattenNextInterHelper~reject_expected(h, expected_reject_string)
    )

    start(initial_string)

The issue as can be seen with https://playground.ponylang.io/?gist=4b958c4198f8986b2b95d554b8476dcc is that an error in fulfill apparently shouldnt cause that same reject to occur only the ones later down the chain. That's an existing bug that we didn't replicate.

Nope. Its a bug on our side. I fixed it.

SeanTAllen · 2021-08-02T03:48:02Z

@jemc one of the bug fixes though means that we dont need rejected in the fulfill block (that makes sense) so it should be easier to set up the reject handler wrapper although i am struggling with it at the moment.

SeanTAllen · 2021-08-02T04:00:16Z

Ok I have all my tests passing.

@jasoncarr0 please review my tests and see what cases you think are missing and ill add.

jasoncarr0

Test contents look good but the completes can be eager for some of the checks

jasoncarr0 · 2021-08-02T04:24:52Z

packages/promises/_test.pony

+
+    let start = Promise[String]
+    let inter = start.flatten_next[String](
+      _FlattenNextStartHelper~error_fulfill(h, initial_string),


The error fulfill method up here seems to have a slight problem because this isn't the end of the test,
but error_fulfill marks complete(true)

jasoncarr0 · 2021-08-02T04:25:57Z

packages/promises/_test.pony

+  fun error_fulfill(h: TestHelper, expected: String, actual: String) ? =>
+    h.assert_eq[String](expected, actual)
+    h.complete_action(expected)
+    h.complete(true)


The completion down here could end the test early for line 189

Suggested change

h.complete(true)

No, this code is correct. This isn't called as part of the test in 189. There are two different primitives, one for Start, one for Inter. If this completion was on Start and not Inter that would be true.

jasoncarr0 · 2021-08-02T04:28:32Z

packages/promises/_test.pony

+
+  fun reject_expected(h: TestHelper, action: String) =>
+    h.complete_action(action)
+    h.complete(true)


Similarly this can complete too early e.g.at line 238

Suggested change

h.complete(true)

same as above, there are two reject_expected. One for Start and one for Inter. This is the one for Inter and the test is complete when reject_expected is called. There is also a reject_expected on the Start primitive that doesn't complete.

Note there are no complete calls on _FlattenNextStartHelper

SeanTAllen · 2021-08-02T11:06:17Z

@jasoncarr0 I believe this is ready to be squashed and merged. See my replies re: tests, those complete calls are correct as they are on _FlattenNextInterHelper not _FlattenNextStartHelper. If they were on _FlattenNextStartHelper, there would be a problem with complete being called early, but they aren't.

SeanTAllen · 2021-08-02T11:43:11Z

I did some renaming of things in the test and because this is fairly complicated, added english lanuage descriptions of what is and is not expected for each test.

SeanTAllen · 2021-08-02T11:43:50Z

I think this is ready for a squash and merge. Once approved, I'll do the squash so I can update the commit comment to match the changes this underwent since the PR was opened.

.release-notes/next-unwrap.md

Co-authored-by: Joe Eli McIlvain <[email protected]>

.release-notes/next-unwrap.md

packages/promises/promise.pony

SeanTAllen added 2 commits July 30, 2021 16:18

WIP

e2a138a

SeanTAllen requested a review from a team July 31, 2021 17:03

SeanTAllen added the changelog - added Automatically add "Added" CHANGELOG entry on merge label Jul 31, 2021

SeanTAllen mentioned this pull request Jul 31, 2021

Release 0.43.1 #3805

Closed

rhagenson approved these changes Aug 1, 2021

View reviewed changes

SeanTAllen and others added 6 commits August 1, 2021 14:16

Update .release-notes/next-unwrap.md

b358ccd

Co-authored-by: Ryan A. Hagenson <[email protected]>

Update .release-notes/next-unwrap.md

bed27d6

Co-authored-by: Ryan A. Hagenson <[email protected]>

Update .release-notes/next-unwrap.md

922bb68

Co-authored-by: Ryan A. Hagenson <[email protected]>

Update .release-notes/next-unwrap.md

abc1397

Co-authored-by: Ryan A. Hagenson <[email protected]>

Update packages/promises/promise.pony

0bbcf5d

Co-authored-by: Ryan A. Hagenson <[email protected]>

Update packages/promises/promise.pony

f4d7afb

Co-authored-by: Ryan A. Hagenson <[email protected]>

change name

cef4362

2nd name change

e8b4cc2

jasoncarr0 reviewed Aug 1, 2021

View reviewed changes

packages/promises/_test.pony Show resolved Hide resolved

jasoncarr0 reviewed Aug 1, 2021

View reviewed changes

jasoncarr0 suggested changes Aug 1, 2021

View reviewed changes

Update packages/promises/promise.pony

c20e9ec

Co-authored-by: Ryan A. Hagenson <[email protected]>

jemc reviewed Aug 2, 2021

View reviewed changes

Add tests

2361f6d

update from jason and joe

0edd697

Fix test

e1799aa

SeanTAllen added 3 commits August 1, 2021 23:24

Test update

f5666f6

Another test

3efacfc

Another error

4749869

Fix incorrect 'error in fulfill handling'

2909b7e

Fix outer reject

990f845

jasoncarr0 reviewed Aug 2, 2021

View reviewed changes

SeanTAllen added 3 commits August 2, 2021 07:31

Clean up test naming

38a90b5

Additional test cleanup

f03eff6

Reorganize flatten next helper method order

9602012

Minor formatting fix

fc88104

jemc reviewed Aug 2, 2021

View reviewed changes

.release-notes/next-unwrap.md Outdated Show resolved Hide resolved

SeanTAllen and others added 2 commits August 2, 2021 11:21

Update .release-notes/next-unwrap.md

19c4960

Co-authored-by: Joe Eli McIlvain <[email protected]>

Documentation wording update

537b3da

jemc approved these changes Aug 2, 2021

View reviewed changes

jasoncarr0 approved these changes Aug 2, 2021

View reviewed changes

SeanTAllen merged commit 88782c5 into main Aug 2, 2021

SeanTAllen deleted the next-unwrap branch August 2, 2021 17:11

github-actions bot pushed a commit that referenced this pull request Aug 2, 2021

Updates release notes for PR #3813

913ec4c

github-actions bot pushed a commit that referenced this pull request Aug 2, 2021

Update CHANGELOG for PR #3813

1e17d11

ergl reviewed Aug 2, 2021

View reviewed changes

.release-notes/next-unwrap.md Show resolved Hide resolved

packages/promises/promise.pony Show resolved Hide resolved

Make working with promises of promises easier #3813

Make working with promises of promises easier #3813

Conversation

SeanTAllen commented Jul 31, 2021

SeanTAllen commented Jul 31, 2021

jasoncarr0 commented Jul 31, 2021 • edited Loading

SeanTAllen commented Jul 31, 2021

rhagenson 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

jemc commented Aug 1, 2021

SeanTAllen commented Aug 1, 2021

SeanTAllen commented Aug 1, 2021

jasoncarr0 Aug 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasoncarr0 Aug 2, 2021 • edited Loading

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

jasoncarr0 Aug 2, 2021 • edited Loading

Choose a reason for hiding this comment

jasoncarr0 left a comment

Choose a reason for hiding this comment

SeanTAllen commented Aug 2, 2021

SeanTAllen commented Aug 2, 2021

SeanTAllen commented Aug 2, 2021

SeanTAllen commented Aug 2, 2021

jemc Aug 2, 2021 • edited Loading

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

SeanTAllen commented Aug 2, 2021

SeanTAllen commented Aug 2, 2021

SeanTAllen commented Aug 2, 2021 • edited Loading

SeanTAllen commented Aug 2, 2021

SeanTAllen commented Aug 2, 2021

jasoncarr0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasoncarr0 Aug 2, 2021 • edited Loading

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

SeanTAllen commented Aug 2, 2021

SeanTAllen commented Aug 2, 2021

SeanTAllen commented Aug 2, 2021

jasoncarr0 commented Jul 31, 2021 •

edited

Loading

jasoncarr0 Aug 1, 2021 •

edited

Loading

jasoncarr0 Aug 2, 2021 •

edited

Loading

jasoncarr0 Aug 2, 2021 •

edited

Loading

jemc Aug 2, 2021 •

edited

Loading

SeanTAllen commented Aug 2, 2021 •

edited

Loading

jasoncarr0 Aug 2, 2021 •

edited

Loading