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 type inference for recursive let bindings #2187

Merged
merged 2 commits into from
Oct 19, 2024

Conversation

byorgey
Copy link
Member

@byorgey byorgey commented Oct 19, 2024

Fixes #2186.

When inferring the type of a recursive let binding with no provided type signature, we generate a unification variable to stand for its type, then infer the type of the body with the unification variable in the context. Finally, we must unify the generated unification variable with the inferred type. However, previously, we were doing this final unification with a direct call to =:= (which simply returns an Either) and discarding the result, which meant that we would simply ignore it when they failed to unify.

This PR replaces the bad call to =:= with a call to unify, and also makes the =:= import qualified to make it less likely that we will ever call =:= directly like this again.

This would only have ever caused a problem with (1) a recursive function definition (2) with no type annotation (3) with a type error (4) specifically in one of the recursive applications. Apparently no one had ever done all of 1-4 at the same time before; @xsebek was the lucky winner. 😄

@byorgey byorgey requested a review from xsebek October 19, 2024 16:37
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.

Thanks for the quick fix. 👍

@byorgey byorgey added the merge me Trigger the merge process of the Pull request. label Oct 19, 2024
@mergify mergify bot merged commit 694e063 into main Oct 19, 2024
12 checks passed
@mergify mergify bot deleted the fix/2186-infer-recursive-let branch October 19, 2024 17:07
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.

Bad application of execConst - Fst [VInt 4]
2 participants