-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Better comparisons for type projections #17092
Conversation
We already implemented in essence the rule suggested in lampepfl/dotty-feature-requests#14: ``` Γ ⊨ p : T ------------------------ (Sel-<:-Proj) Γ ⊨ p.A <: T#A ``` This rule is implemented in `isSubPrefix`. But we did not get to check that since we concluded prematurely that an alias type would never match. The alias type in question in i17064.scala was ```scala Outer#Inner ``` Since `Outer` is not a path, the asSeenFrom to recover the info of `Outer#Inner this got approximated with a `Nothing` argument and therefore the alias failed. It is important in this case that we could still succeed with a `isSubPrefix` test, which comes later. So we should not return `false` when the prefix is not a singleton. Fixes scala#17064
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise LGTM.
def hasPrecisePrefix(tp: NamedType) = | ||
tp.prefix.isSingleton || tp.prefix == NoPrefix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems fine, but since asSeenFrom itself uses TypeOps.isLegalPrefix for this purpose, wouldn't it make sense to also use it here to ensure the checks stay aligned?
`NoPrefix.isStable` already returns true.
* See pos/i17064.scala for a test case | ||
*/ | ||
def hasStablePrefix(tp: NamedType) = | ||
tp.prefix.isStable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dropped the NoPrefix check since it's already implied by isStable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, I overlooked that.
We already implemented in essence the rule suggested in lampepfl/dotty-feature-requests#14:
This rule is implemented in
isSubPrefix
. But we did not get to check that since we concluded prematurely that an alias type would never match. The alias type in question in i17064.scala wasSince
Outer
is not a path, the asSeenFrom to recover the info ofOuter#Inner
got approximated with aNothing
argument and therefore the alias failed. It is important in this case that we could still succeed with aisSubPrefix
test, which comes later. So we should not returnfalse
when the prefix is not a singleton.Fixes #17064