Skip to content

Fix #9994: Handle subtyping between two ThisType #10008

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 27, 2020
Merged

Fix #9994: Handle subtyping between two ThisType #10008

merged 1 commit into from
Oct 27, 2020

Conversation

smarter
Copy link
Member

@smarter smarter commented Oct 15, 2020

Previously, i9994.scala failed when compiled with -Ythrough-tasty
with:

7 |  def foo: this.type = this
  |      ^
  | error overriding method foo in trait Foo of type => (Bar.this : pkg.Bar);
  |   method foo of type => (Bar.this : pkg.Bar) has incompatible type

The two types were pretty-printed the same but were actually:

ExprType(ThisType(TypeRef(ThisType(TypeRef(ThisType(TypeRef(NoPrefix,module class <root>)),module class <empty>)),class Bar)))

and

ExprType(ThisType(TypeRef(ThisType(TypeRef(NoPrefix,module class <empty>)),class Bar)))

Fixed by explicitly handling subtyping between two
ThisType (previously we felt through to fourthTry were the type of
the lhs was widened).

@smarter smarter force-pushed the thistype-subtyping branch from 30db3fb to 9aa5fef Compare October 15, 2020 08:18
@smarter smarter marked this pull request as ready for review October 15, 2020 11:39
smarter
smarter commented Oct 15, 2020
View reviewed changes
compiler/src/dotty/tools/dotc/core/TypeComparer.scala Outdated
@@ -310,6 +310,8 @@ class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling
def compareThis = {
val cls2 = tp2.cls
tp1 match {
case tp1: ThisType =>
(tp1.cls eq cls2) && recur(tp1.tref, tp2.tref)
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure about the condition here, isSubPrefix does something different:
https://github.com/lampepfl/dotty/blob/0525ba010061c060d759d869c63ea5b54415f729/compiler/src/dotty/tools/dotc/core/TypeComparer.scala#L859-L863
Should we do the same here and get rid of isSubPrefix, or is there a good reason for treating ThisType in prefix positions specially?

Copy link
Contributor

Choose a reason for hiding this comment

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

The isSubPrefix logic is needed to deal with double vision with self types.

I think the test here can be simplified to

case tp1: ThisType => tp1.cls eq cls2

The tref is just used to carry the class.

@smarter smarter requested a review from odersky October 15, 2020 11:41
@nicolasstucki
Copy link
Contributor

Now we can rebase it on top of #9925. Then we can remove the related tests from the blacklist.

@smarter smarter force-pushed the thistype-subtyping branch from 9aa5fef to 78c3925 Compare October 21, 2020 20:24
odersky
odersky requested changes Oct 26, 2020
View reviewed changes
compiler/src/dotty/tools/dotc/core/TypeComparer.scala Outdated
@@ -310,6 +310,8 @@ class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling
def compareThis = {
val cls2 = tp2.cls
tp1 match {
case tp1: ThisType =>
(tp1.cls eq cls2) && recur(tp1.tref, tp2.tref)
Copy link
Contributor

Choose a reason for hiding this comment

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

The isSubPrefix logic is needed to deal with double vision with self types.

I think the test here can be simplified to

case tp1: ThisType => tp1.cls eq cls2

The tref is just used to carry the class.

@smarter
Copy link
Member Author

smarter commented Oct 26, 2020

The tref is just used to carry the class.

Doesn't the tref also determine the path?

odersky
odersky approved these changes Oct 27, 2020
View reviewed changes
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

compiler/src/dotty/tools/dotc/core/TypeComparer.scala Outdated
@@ -310,6 +310,8 @@ class TypeComparer(@constructorOnly initctx: Context) extends ConstraintHandling
def compareThis = {
val cls2 = tp2.cls
tp1 match {
case tp1: ThisType =>
(tp1.cls eq cls2) && recur(tp1.tref, tp2.tref)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
(tp1.cls eq cls2) && recur(tp1.tref, tp2.tref)
tp1.cls eq cls2

@odersky odersky assigned smarter and unassigned odersky Oct 27, 2020
@smarter
Fix scala#9994: Handle subtyping between two ThisType
212211d 
Previously, i9994.scala failed when compiled with `-Ythrough-tasty`
with:

7 |  def foo: this.type = this
  |      ^
  | error overriding method foo in trait Foo of type => (Bar.this : pkg.Bar);
  |   method foo of type => (Bar.this : pkg.Bar) has incompatible type

The two types were pretty-printed the same but were actually:

ExprType(ThisType(TypeRef(ThisType(TypeRef(ThisType(TypeRef(NoPrefix,module class <root>)),module class <empty>)),class Bar)))

and

ExprType(ThisType(TypeRef(ThisType(TypeRef(NoPrefix,module class <empty>)),class Bar)))

Fixed by explicitly handling subtyping between two
`ThisType` (previously we felt through to `fourthTry` were the type of
the lhs was widened), this is a rare case because `ThisType` are
interned, so usually two `ThisType` to the same class are `eq` and
therefore `isSubType` returns true.
@smarter smarter force-pushed the thistype-subtyping branch from 78c3925 to 212211d Compare October 27, 2020 12:10
@bishabosha bishabosha merged commit b5a1715 into scala:master Oct 27, 2020
@bishabosha bishabosha deleted the thistype-subtyping branch October 27, 2020 13:59
@Kordyjan Kordyjan added this to the 3.0.0 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@odersky odersky odersky approved these changes

Assignees

@smarter smarter

Labels
None yet
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@smarter @nicolasstucki @odersky @Kordyjan @bishabosha