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

Extract GADT constraints from wildcard type arguments #14132

Merged
merged 1 commit into from
Jan 24, 2022

Conversation

dwijnand
Copy link
Member

No description provided.

@dwijnand
Copy link
Member Author

This regressed with #12506 (which shipped with 3.0.2).

@dwijnand dwijnand marked this pull request as ready for review December 18, 2021 09:52
@dwijnand
Copy link
Member Author

@abgruszecki could you review this please, or delegate the review to someone else?

@abgruszecki
Copy link
Contributor

@dwijnand I was going to prepare a different fix for the problem, but it looks like it's not going to happen any time soon. Let me just suggest a comment that I think should be there and we can merge this PR.

@dwijnand dwijnand merged commit 1595cae into scala:master Jan 24, 2022
@dwijnand dwijnand deleted the constrain-wildcards branch January 24, 2022 14:05
@smarter
Copy link
Member

smarter commented Jan 31, 2022

@abgruszecki so it's not clear to me if this change is actually sound? The previous comment mentioned that this was unsound but didn't provide a test case, and the new comment just says this fix isn't "proper" but it's not clear to me what that implies.

@abgruszecki
Copy link
Contributor

abgruszecki commented Jan 31, 2022

@smarter we don't pass TypeBounds themselves to isSubType here, we just "give up" on what I did in the deskolemization PR (referenced in the new comment) and instead we call isSubType on the entire types. It's not proper in the sense that using SkolemType in this way results in problems in some edge cases, but at least it fixes the regression.

@smarter
Copy link
Member

smarter commented Jan 31, 2022

Do you have an example of these edge cases?

@abgruszecki
Copy link
Contributor

@abgruszecki
Copy link
Contributor

So after a discussion with Guillaume, there's one more thing that should be said: this PR may have introduced unsoundness into the compiler. I paid too much attention to us regressing after the deskolemization PR, but that PR was actually necessary to fix some soundness issues, like #11103. With that in mind, we should either have a very good & convincing case why this fix is certainly sound, or we should go ahead and rewrite the isSubType check added in this PR to compare the "elements" of TypeBounds instead of comparing the entire types.

@dwijnand
Copy link
Member Author

dwijnand commented Feb 1, 2022

If you can come up with an unsound example, I'm happy to redo this PR. But without more information I wouldn't know which parts of isSubType are the unsound bits and which are the sound ones that should be inlined.

Are you free next Tuesday to pair on this in the Scala Spree?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wildcard types are not properly inferred in pattern matching
4 participants