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

generalize variables bound by <- #1501

Merged
merged 5 commits into from
Sep 9, 2023
Merged

generalize variables bound by <- #1501

merged 5 commits into from
Sep 9, 2023

Conversation

byorgey
Copy link
Member

@byorgey byorgey commented Sep 9, 2023

Closes #351.

I sat down with @ccasin today to look at #351. He convinced me that it is actually fine to return forall a. a -> a as the type of r in r <- return (\i.i), even though return (\i.i) has type forall a. cmd (a -> a) and we can never have a type cmd (forall a. a -> a). Basically, in a Hindley-Milner-style system it is always safe to generalize at any time.

The real problem --- which I was very close to discovering in #351 (comment) --- is simply that we need to generalize the type of variables bound by <-. However, we need to make sure to do so before putting that variable in the context, which I apparently failed to do when I tried fixing this before.

Copy link
Member

@xsebek xsebek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the bug for me! 👍

While the code is already heavily annotated, this probably warrants a note. 🙂
It would also be nice to have a unit test for this.

@byorgey byorgey added the merge me Trigger the merge process of the Pull request. label Sep 9, 2023
@byorgey
Copy link
Member Author

byorgey commented Sep 9, 2023

As a note for the future, we may possibly want to reconsider this at some point; see "Let Should Not Be Generalized" by Vytiniotis, Peyton Jones, and Schrijvers ( https://www.microsoft.com/en-us/research/wp-content/uploads/2016/02/tldi10-vytiniotis.pdf ). But I think this will be fine for now.

@mergify mergify bot merged commit 096251d into main Sep 9, 2023
9 checks passed
@mergify mergify bot deleted the fix/issue-351 branch September 9, 2023 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Trigger the merge process of the Pull request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generalize properly when inferring the type of a bind
2 participants