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 return checking in behaviours and constructors #3971

Merged
merged 7 commits into from
Jan 28, 2022
Merged

Conversation

SeanTAllen
Copy link
Member

@SeanTAllen SeanTAllen commented Jan 28, 2022

Our checks to make sure that the usage of return in behaviours and constructors was overly restrictive. It was checking for any usage of return that had a value including when the return wasn't from the method itself.

For example:

actor Main
  new create(env: Env) =>
    let f = {() => if true then return None end}

Would return an error despite the return being in a lambda and therefore not returning a value from the constructor.

Fixes #3682

@SeanTAllen
Copy link
Member Author

To be a full PR, this needs a regression test added as well as release notes and the PR comments etc cleaned up.

@SeanTAllen SeanTAllen changed the title Issue 3682 Fix return checking in constructors and behaviors Jan 28, 2022
@SeanTAllen SeanTAllen added the changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge label Jan 28, 2022
@ponylang-main
Copy link
Contributor

Hi @SeanTAllen,

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 3971.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 SeanTAllen changed the title Fix return checking in constructors and behaviors Fix return checking in constructors and behaviours Jan 28, 2022
@SeanTAllen SeanTAllen changed the title Fix return checking in constructors and behaviours Fix return checking in behaviours and constructors Jan 28, 2022
@SeanTAllen SeanTAllen marked this pull request as ready for review January 28, 2022 13:50
@SeanTAllen SeanTAllen requested a review from a team January 28, 2022 13:50
@SeanTAllen
Copy link
Member Author

When this is squashed for merging, the comment should be:


Fix return checking in behaviours and constructors

Our checks to make sure that the usage of return in behaviours and constructors was overly restrictive. It was checking for any usage of return that had a value including when the return wasn't from the method itself.

For example:

actor Main
  new create(env: Env) =>
    let f = {() => if true then return None end}

Would return an error despite the return being in a lambda and therefore not returning a value from the constructor.

@SeanTAllen SeanTAllen changed the title Fix return checking in behaviours and constructors Fix return checking in behaviours and constructors Jan 28, 2022
@jemc
Copy link
Member

jemc commented Jan 28, 2022

Looks good, but can you also add a test showing that this works for object literals (and if it doesn't, fix the code so that it does)?

@SeanTAllen
Copy link
Member Author

@jemc 2 additional tests were added. Let me know if it looks good. This always worked for object literals. The problem was the check came before the lambda's got desugared. The additional tests verify that it working for object literals (an axiom upon which this is based) continues to remain true.

@SeanTAllen SeanTAllen merged commit a6d14ca into main Jan 28, 2022
@SeanTAllen SeanTAllen deleted the issue-3682 branch January 28, 2022 20:00
github-actions bot pushed a commit that referenced this pull request Jan 28, 2022
github-actions bot pushed a commit that referenced this pull request Jan 28, 2022
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.

Syntax pass: "return on constructor or behaviour must not have an expression" before lambda desugar
4 participants