Skip to content

Fix i11545 #12087

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

Closed
wants to merge 2 commits into from
Closed

Fix i11545 #12087

wants to merge 2 commits into from

Conversation

abgruszecki
Copy link
Contributor

This is more of a makeshift fix than anything else. If we care about plugging the soundness hole for 3.0.0, then we should merge it. Otherwise, I'll have to spend a bit more time fiddling with TypeComparer in order to properly fix the issue.

@abgruszecki abgruszecki requested a review from odersky April 13, 2021 19:05
@abgruszecki abgruszecki linked an issue Apr 13, 2021 that may be closed by this pull request
@abgruszecki
Copy link
Contributor Author

If we do decide to merge this, I'll open a new issue for investigating in detail what is the exact problem here, and how to fix it well.

// if base is a disjunction, this might have come from a tp1 type that
// expands to a match type. In this case, we should try to reduce the type
// and compare the redux. This is done in fourthTry
def tryBaseType(cls2: Symbol): Boolean = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it will overshoot. tryBaseType is an essential element for comparisons. Simply ignoring this step under GADT constraint inference is too drastic. I would expect instead a necessaryEither somewhere in the logic.

Copy link
Contributor Author

@abgruszecki abgruszecki Apr 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only avoid tryBaseType if the LHS is an AndType. In that case, using the base type can end up performing a comparison that is too approximate to be useful for GADTs (see neg/i11545a.scala, added in this PR). I have ideas for other fixes, we can discuss them during today's lab meeting.

@odersky odersky assigned abgruszecki and unassigned odersky Apr 14, 2021
@dwijnand
Copy link
Member

dwijnand commented Jun 9, 2022

Duplicate of #15175

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.

Overconstrained GADT bounds in presence of bad bounds
3 participants