Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tuple related compiler segfaults #3723

Merged
merged 2 commits into from
Feb 27, 2021

Conversation

Trundle
Copy link
Contributor

@Trundle Trundle commented Feb 27, 2021

It's possible that the method body returns a subtype of the return type.
For example, a return value with type ((U32, U32) | (U32, None)) is
legal for a method with return type (U32, (U32 | None)), but it
requires a cast.

071db10 changed that methods returning a tuple always use a cast as if the method body's value already has the return type. Unfortunately, it doesn't go into detail why. This already caused an issue in #849 and an exception was added if return type and body type are both tuples. I think that assumption doesn't hold: for the above mentioned case, the return type is a tuple, but the body type is an union, and yet the current code continues as if the body produces a tuple.

Fixes #2609, #2808

It's possible that the method body returns a subtype of the return type.
For example, a return value with type `((U32, U32) | (U32, None))` is
legal for a method with return type `(U32, (U32 | None))`, but it
requires a cast.

Fixes ponylang#2609, ponylang#2808
@SeanTAllen SeanTAllen added the changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge label Feb 27, 2021
@SeanTAllen
Copy link
Member

@Trundle on a roll!

@ponylang-main
Copy link
Contributor

Hi @Trundle,

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 3723.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
Copy link
Member

Confirmed that #2808 is fixed by this.

@SeanTAllen
Copy link
Member

Confirmed #2609 is fixed.

@SeanTAllen
Copy link
Member

I'll hold tomorrow's ponyc release until this has release notes and is merged.

Thanks @Trundle.

@SeanTAllen SeanTAllen changed the title Don't assume that method bodies produce the correct tuple type already Fix tuple related compiler segfaults Feb 27, 2021
@SeanTAllen
Copy link
Member

@Trundle I think "Fix tuple related compiler segfaults" would be a good release notes title.

@SeanTAllen
Copy link
Member

I've added release notes.

@SeanTAllen SeanTAllen merged commit 338bc8a into ponylang:main Feb 27, 2021
github-actions bot pushed a commit that referenced this pull request Feb 27, 2021
github-actions bot pushed a commit that referenced this pull request Feb 27, 2021
@Trundle
Copy link
Contributor Author

Trundle commented Feb 28, 2021

Thank you for adding the release notes, @SeanTAllen. They contain a slight mistake though: the crash happens in the compiled code, not the compiler itself. Is it worth fixing and if so, how does it work? Should I open a new PR with changes to 0.39.0.md?

@Trundle Trundle deleted the cast-tuple-returns branch February 28, 2021 00:52
@SeanTAllen
Copy link
Member

@Trundle yeah go ahead and then I'll update the release page as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SIGSEGV when accessing tuple element from partial function
3 participants