Add workaround for compiler assertion failure in Promise.flatten_next

[mfelsche]

This fixes #3856 with only a workaround and explicitly without actually solving the underlying issue, but hey, it's something.

Somehow the replaced lambda, or to be exact, the hygienic type created for it, wasn't properly reified and remained as a generic type. The hygienic type generates was:

promises_$2$7_String_val_String_val

which seems correct, but this is the type that failed the reachability, namely the part where the methods from the interface Fulfill have been applied to the type:

type: {(A): Promise[B #share] tag}[String val, String val] ref

[SeanTAllen]

I'm not a big fan of this workaround. I'd rather find the root cause of this. Who knows what weirdness might come from this workaround. Will existing working flatten code of which there is a lot in the not yet published GitHub Rest API stop working? No idea. The workaround worries me on the correctness front. It works for this case, but does it break others? To accept the workaround, I would want us to have more working test cases, how to do that and have it model the real world code we came up with this feature to address, I'm not sure.

[SeanTAllen]

@mfelsche can you rebase this against main so we don't have the spurious windows network failures?

[mfelsche]

The workaround applied here is doing the exact same thing as it did before this PR. It is just that the compiler doesn't need to create a hygienic type for the lambda and thus the reification works as it should and we avoid the error case.

I do agree that this workaround shouldn't be the last action to take solving this bug. But it is definitely an improvement over the current situation imho

[SeanTAllen]

How is it the exact same thing it did before this PR? It's a change. That sounds different to me.

[mfelsche]

Could you please look at what the lambda is returning and what the apply method of _PendingFulfill is returning?
It is the same thing. Lambdas get desugared to a type like _PendingFulfill during compilation. All I did is avoided that step that introduced the compiler assertion reported in #3856 . It was caused by that generic lambda implementing Fulfill and defined inside an object literal. I removed that generic lambda and exchanged it for a type that is reified correctly.

[SeanTAllen]

This file is redundant. You can remove it.

[SeanTAllen]

This comment doesn't make sense to me.

[SeanTAllen]

In particular

"Fulfill returning a promise" <-- ok we are fulfilling
"that is never fulfilled" <-- that we aren't fulfilling

which sounds like an internal contradiction.

[mfelsche]

yeah, the comment stems from a misunderstanding.

[SeanTAllen]

So we agree the error in question shouldn't be happening. If this is exactly equivalent then this would fix nothing. It does however fix something and there's no evidence at this time that it doesn't break anything in an odd fashion. I have a bunch of code that used flatten and there's no way from purely this PR to know it doesn't break that code.

If this was exactly equivalent, it wouldn't fix the bug. I'm not comfortable with this change without verifying that the existing usage doesn't break.

Can you build ponyc with this and run all the github_rest_api examples and verify they work? That would give me assurance that the existing code that I am using on a regular basis to works and isn't broken by this change.

Without that I have no idea that with this change that given by working inputs, that they won't stop working because of this change. It's a different code path.

If compiler was equivalent before and after this change then the sample from the issue would continue to fail. It doesn't so, at some level, this change is a change and not equivalent. It's not referentially transparent and if it isn't referentially transparent that means that existing code which we believe is correct could break given this takes existing code that we believe is correct and fixes it. The reverse is possible.

Running all the existing github_rest_api example code through this change would give me the assurance that I am currently lacking.

[SeanTAllen]

@mfelsche can you take your build of this on your machine and use it to do make test for the github_rest_api and make sure it all works?

[mfelsche]

The github_rest_api tests all worked. Also checked the examples on main.

[SeanTAllen]

@mfelsche that's good to hear. can you open a new issue for the reification problem with how to reproduce (changes to stdlib etc) so we can have that for something that we can look into in the future then this can be merged and we can close the original issue.

[ponylang-main]

Hi @mfelsche,

The changelog - fixed label was added to this pull request; all PRs with a changelog label need to have release notes included as part of the PR. If you haven't added release notes already, please do.

Release notes are added by creating a uniquely named file in the .release-notes directory. We suggest you call the file 3991.md to match the number of this pull request.

The basic format of the release notes (using markdown) should be:

## Title

End user description of changes, why it's important,
problems it solves etc.

If a breaking change, make sure to include 1 or more
examples what code would look like prior to this change
and how to update it to work after this change.

Thanks.

[SeanTAllen]

Issue #4014 has been created. This can be merged once release notes are added.