Restore a call to instance that was dropped in #12236#12542
Merged
gasche merged 3 commits intoocaml:trunkfrom Sep 15, 2023
Merged
Restore a call to instance that was dropped in #12236#12542gasche merged 3 commits intoocaml:trunkfrom
instance that was dropped in #12236#12542gasche merged 3 commits intoocaml:trunkfrom
Conversation
garrigue
approved these changes
Sep 15, 2023
Contributor
garrigue
left a comment
There was a problem hiding this comment.
Trivially correct.
The reason you cannot detect the difference is that rue unifies with an instance of the expected type, so the levels will be lowered anyway. And since the polymorphism of the structure is only used locally, it doesn't matter if those levels are lowered afterwards.
This said the current code is morally wrong (plain wrong if we prohibit lowering generic_level through unification, as I plan to do) and this fixes it.
Contributor
Author
|
Thanks, the explanation makes sense. I think this PR is ready to merge then. |
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
3 participants
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.
#12236 dropped a possibly-load-bearing call to
instancefrom the type-checking ofPexp_constraint: see the second component of the tuple on this removed line. This PR restores it.I am unable to find a way that this change matters. I happened to notice the discrepancy while reading through #12236 for a related change internal to Jane Street. The main difference between pre-#12236 and post-#12236 code that I can think of is:
exp_typefield of the result of type-checkingPexp_constraintwas an instance of a possibly-generic type. (In particular, it was this result of this now-restored call toinstance.)exp_typefield of the result of type-checkingPexp_constraintcan possibly be a type with generic level (and not an instance thereof).I've been unable to find a way that this discrepancy results in a change in principality warnings/errors/etc. It's possible this current PR is just not needed! Either this call to
instanceis needed and this PR fixes an unintentional change in behavior, or it's not needed and I'll have learned something new about the type-checker.