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

Fix regression in provablyDisjoint #12786

Conversation

OlivierBlanvillain
Copy link
Contributor

#12549 introduced a regression in provablyDisjoint. The problem comes from the following case:

case (tp1: TermRef, tp2: TypeRef) if isEnumValueOrModule(tp1)
  && !tp1.classSymbols.exists(_.derivesFrom(tp2.classSymbol)) =>
    true

The !tp1.classSymbols.exists(...) is not sufficient, for instance if tp2 is unbounded type parameter that code can wrongly conclude that a TermRef doesn't belong to that type.

@liufengyun
Copy link
Contributor

#12549 introduced a regression in provablyDisjoint. The problem comes from the following case:

case (tp1: TermRef, tp2: TypeRef) if isEnumValueOrModule(tp1)
  && !tp1.classSymbols.exists(_.derivesFrom(tp2.classSymbol)) =>
    true

The !tp1.classSymbols.exists(...) is not sufficient, for instance if tp2 is unbounded type parameter that code can wrongly conclude that a TermRef doesn't belong to that type.

Good catch, I think the following will work (use tp2.symbol instead of tp2.classSymbol):

case (tp1: TermRef, tp2: TypeRef) if isEnumValueOrModule(tp1) && !tp1.classSymbols.exists(_.derivesFrom(tp2.symbol)) =>
 true

OlivierBlanvillain added a commit to dotty-staging/dotty that referenced this pull request Jun 11, 2021
@OlivierBlanvillain
Copy link
Contributor Author

That also doesn't work (Not[B] still reduces to "unreachable" in the added test case).

In general I don't think a test of the form !exists can be used to conclude that two types are disjoints given that one of those types could be abstract in the current context but become concrete in another context...

@OlivierBlanvillain
Copy link
Contributor Author

It should be possible to conclude disjointness between an enum case and a class, that should be analogous to disjointness between a final class and a class which we handle in case (tp1: TypeRef, tp2: TypeRef) if tp1.symbol.isClass && tp2.symbol.isClass =>.

@anatoliykmetyuk
Copy link
Contributor

@OlivierBlanvillain is this one good for a review? Or is the solution subject for discussion?

OlivierBlanvillain added a commit to dotty-staging/dotty that referenced this pull request Jul 30, 2021
@OlivierBlanvillain OlivierBlanvillain force-pushed the fix-regression-in-provably-disjoint branch from e184e75 to d52b365 Compare July 30, 2021 07:17
@OlivierBlanvillain OlivierBlanvillain marked this pull request as ready for review July 30, 2021 07:17
@OlivierBlanvillain OlivierBlanvillain requested review from liufengyun and odersky and removed request for liufengyun July 30, 2021 07:18
@OlivierBlanvillain OlivierBlanvillain removed the request for review from odersky July 30, 2021 09:12
@OlivierBlanvillain OlivierBlanvillain self-assigned this Jul 30, 2021
@OlivierBlanvillain OlivierBlanvillain force-pushed the fix-regression-in-provably-disjoint branch from d0f6397 to e9271f3 Compare July 30, 2021 09:35
Copy link
Contributor

@liufengyun liufengyun left a comment

Choose a reason for hiding this comment

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

LGTM

true
case (tp1: TypeRef, tp2: TermRef) if isEnumValueOrModule(tp2) && !tp2.classSymbols.exists(_.derivesFrom(tp1.classSymbol)) =>
true
case (tp1: TermRef, tp2: TypeRef) if isEnumValue(tp1) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the consideration of using isEnumValue instead of isEnumValueOrModule here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So far I haven't found a need for it, the existing infrastructure doesn't need special treatment to handle modules...

@OlivierBlanvillain OlivierBlanvillain merged commit 4e740c5 into scala:master Aug 2, 2021
@OlivierBlanvillain OlivierBlanvillain deleted the fix-regression-in-provably-disjoint branch August 2, 2021 13:14
@Kordyjan Kordyjan added this to the 3.1.0 milestone Aug 2, 2023
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.

5 participants