Skip to content

Fix #3938: adapt prefix to ease prefix inference #3939

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

Merged
merged 2 commits into from
Jan 30, 2018

Conversation

liufengyun
Copy link
Contributor

Fix #3938: adapt prefix to ease prefix inference

@liufengyun
Copy link
Contributor Author

@odersky : we are doing prefix inference in exhaustivity check, this PR does some magic in order for inference to work smoothly. Could you have a look and see if it makes sense?

@allanrenucci allanrenucci requested a review from odersky January 29, 2018 17:39
@allanrenucci allanrenucci assigned odersky and unassigned odersky and allanrenucci Jan 29, 2018
@@ -638,15 +640,31 @@ class SpaceEngine(implicit ctx: Context) extends SpaceLogic {
}
}

// Fix subtype checking for child instantiation,
// such that `Foo(Test.this.foo) <:< Foo(Foo.this)`
Copy link
Contributor

@odersky odersky Jan 30, 2018

Choose a reason for hiding this comment

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

I could not correlate the claim Foo(Test.this.foo) <:< Foo(Foo.this) in i3939.scala with the code here. In i3938, foo is not a module, so how does the removeThisType affect it?

Also, note that if we do have a

 object Foo

then we have already Foo.type <:< Foo.this and Foo.this <:< Foo.type by the rules for ThisType in TypeComparer's firstTry and secondTry methods. So I am not sure why the code would be necessesary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the clarification. The correlation is that in the else branch, it throws away of the ThisType. For the module case, I'll try if I can throw it away.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just checked, the patch for module cannot be removed, as the subtyping for Foo.type <:< Foo.this only works if it's static owner:

          case tp1: NamedType if cls2.is(Module) && cls2.eq(tp1.widen.typeSymbol) =>
            cls2.isStaticOwner ||
            isSubType(tp1.prefix, cls2.owner.thisType) ||
            secondTry(tp1, tp2)

The test case tests/patmat/3455.scala shows that we need the patch:

trait AxisCompanion {
   sealed trait Format
   object Format {
      case object Decimal extends Format
      case object Integer extends Format
   }
}
object Axis extends AxisCompanion
class Axis {
   import Axis._
   def test( f: Format ) = f match {
      case Format.Integer => "Int"
   }
}

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.

Thanks for explaining @liufengyun.

@liufengyun liufengyun merged commit 7058f69 into scala:master Jan 30, 2018
@liufengyun liufengyun deleted the fix-3938 branch January 30, 2018 19:38
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.

Erroneous unreachable warning with path dependant type
3 participants