Repository navigation
Track type of variables bound by as - #13763
Merged
Merged
Conversation
gasche
approved these changes
Feb 11, 2025
gasche
left a comment
Member
There was a problem hiding this comment.
This is an annoying patch to read because of the amount of boilerplate, but I think that it is correct and I understand why it helps -- the problem is that the types of pattern variables returned by Typedtree.pat_bound_idents_full may be incorrect. I wish we had move Tpat_alias to an inline record in the process, but oh well, this is okay.
Keep the types of variables bound by `as` because they may not equal the types of the `as` patterns themselves. This fixes some incorrect value kinds that can lead to miscompilation.
lthls
force-pushed
the
lpw25-fix-bad-poly-variant-type
branch
from
February 12, 2025 09:10
6df7974 to
9a67d5a
Compare
tmcgilchrist
pushed a commit
to tmcgilchrist/ocaml
that referenced
this pull request
Feb 17, 2025
Keep the types of variables bound by `as` because they may not equal the types of the `as` patterns themselves. This fixes some incorrect value kinds that can lead to miscompilation. --------- Co-authored-by: Leo White <leo@lpw25.net>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Port of oxcaml/oxcaml#3507.
Short description: when typing
aspatterns, we separate the type of the pattern from the type of the variable.This can be seen on this example from the original PR:
In the first pattern,
`A as errorhas pattern type[< `A ], but sinceerroralso appears in other patterns it will end up having type[> `A | `B of unit]on the right-hand side.The code in
matching.mlgenerates a static handler for the shared right-hand side, which haserroras parameter, and generates a value kind annotation on this parameter from the type oferrorin the pattern.Before this PR, this would look up the type of the variable in the first case below the or-pattern, and would find the type of the pattern,
[< `A ], which is always an integer soPintvalis generated.After this PR, we still look only in the first case, but since we get the type of the variable instead of the pattern, we get the correct type (which produces
Pgenvalin this case).Compared to the original PR, the only relevant difference is in the
Ppat_constraintcase: the flambda-backend version has removed the code for translating(var : t)as(_ as var : t), so I had to find what would be the correct type in this case.I've chosen to add as a regression test for the native compiler the failing example from @lpw25's original PR, which fails both with Closure (since #8735) and Flambda (since #13758).