Fix #14200 (without breaking pr7321_ok.ml) - #14202
Merged
Merged
Conversation
garrigue
commented
Aug 22, 2025
| (* Only check the lower bound for abstract types. | ||
| For private types, the lower bound can be inferred, and | ||
| the internal one may be wrong in the result of functors. *) | ||
| imp abstr (imp p2 p1 && imp n2 n1 && imp j2 j1)) |
Contributor
Author
There was a problem hiding this comment.
There is no specific need to change this line, but for an abstract type p2 and n2 are always false, so that checking j2 would be sufficient.
stedolan
approved these changes
Aug 27, 2025
stedolan
left a comment
stedolan
left a comment
Contributor
There was a problem hiding this comment.
Looks good to me, thanks for the quick fix!
Contributor
Author
|
Thank you for the review. |
garrigue
added a commit
that referenced
this pull request
Aug 27, 2025
Contributor
Author
|
Cherry-picked to 5.4 branch. |
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
2 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.
This fixes #14200 by checking that types exported as private keep the same variance as their internal definition.
This was supposed to be as simple as switching two lines, handling such types the same way as abstract types, but this actually breaks
typing-module-bugs/pr7321_ok.ml.The problem is that the lower bound side of variance is inferred for private types, and it may not match the one obtained as the result of a functor application.
So we should only check the upper-bound side of the variance, and just accept the inferred variance for the lower-bound side.
Note: this should at least go in 5.4, so I put the change log there.
This is not a strict unsoundness, as it only allows to change the variance given by the user, not the one inferred from the definition. But this is still serious, as phantom parameters used to encode some safety property may be ignored.