Skip to content

Fix #2104: Instantiate unapply result type variables #3905

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

Merged
merged 2 commits into from
Jan 25, 2018
Merged

Fix #2104: Instantiate unapply result type variables #3905

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5b7c0d1
Fix #2104: Instantiate unapply result type variables
odersky Jan 24, 2018
540b011
Fix ErrorMessages test
odersky Jan 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/typer/Applications.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -920,6 +920,7 @@ trait Applications extends Compatibility { self: Typer with Dynamic =>
val ownType =
if (selType <:< unapplyArgType) {
unapp.println(i"case 1 $unapplyArgType ${ctx.typerState.constraint}")
fullyDefinedType(unapplyArgType, "pattern selector", tree.pos)
Copy link
Contributor

@liufengyun liufengyun Jan 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

From the diagnosis, the type parameter could be further constrained by patterns, thus I'm not sure if this change is the right fix. However, given that there is no test case invalidated this change, maybe we can accept the fix for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think it is analogous to fully instantiating the match scrutinee. In pattern matching all information flows from the scrutinee via the expected type to the pattern, never the other way round.

selType
} else if (isSubTypeOfParent(unapplyArgType, selType)(ctx.addMode(Mode.GADTflexible))) {
maximizeType(unapplyArgType) match {
Expand Down Expand Up @@ -952,7 +953,6 @@ trait Applications extends Compatibility { self: Typer with Dynamic =>
ex"Pattern type $unapplyArgType is neither a subtype nor a supertype of selector type $selType",
tree.pos)
}

val dummyArg = dummyTreeOfType(ownType)
val unapplyApp = typedExpr(untpd.TypedSplice(Apply(unapplyFn, dummyArg :: Nil)))
val unapplyImplicits = unapplyApp match {
Expand Down
3 changes: 1 addition & 2 deletions compiler/test/dotty/tools/dotc/reporting/ErrorMessagesTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -811,8 +811,7 @@ class ErrorMessagesTests extends ErrorMessagesTest {
.expect { (ictx, messages) =>
implicit val ctx: Context = ictx

assertMessageCount(1, messages)
val AnonymousFunctionMissingParamType(param, args, _, pt) :: Nil = messages
val AnonymousFunctionMissingParamType(param, args, _, pt) = messages.last
Copy link
Contributor Author

@odersky odersky Jan 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This is very mysterious: If I copy out the code of the test I always see 4 messages - one "missing param type", followed by 3 "cannot resolve overloading". that happens no matter whether I am on master or this PR.

On master the ErrorMessageTests succeeded, i.e. they report only one failure instead of the 4 we see in the terminal. On this PR this is no longer the case. Instead, it reports all 4 tests in messages, but in reverse order.

I have no idea what could be the cause of these effects.

Copy link
Contributor

Choose a reason for hiding this comment

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

The reporter for the error message tests is mixed in with dotty.tools.dotc.reporting.UniqueMessagePositions which suppresses multiple error messages per position. This is why only one error message is reported. This PR must have changed the position of the reported error messages, so that now the test sees 4 messages as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is that in master, there is no overlap in positions of the 4 errors, and this PR doesn't change positions of the errors, and the order doesn't change either.

Copy link
Contributor

@liufengyun liufengyun Jan 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

The root cause is the following line in Inferencing.scala (https://github.com/lampepfl/dotty/blob/master/compiler/src/dotty/tools/dotc/typer/Inferencing.scala#L339-L342):

      val hasUnreportedErrors = ctx.typerState.reporter match {
        case r: StoreReporter if r.hasErrors => true
        case _ => false
      }

      // ....

      if (!hasUnreportedErrors)
        vs foreachBinding { (tvar, v) =>
          if (v != 0 && ctx.typerState.constraint.contains(tvar)) {
            // previous interpolations could have already instantiated `tvar`
            // through unification, that's why we have to check again whether `tvar`
            // is contained in the current constraint.
            typr.println(s"interpolate ${if (v == 1) "co" else "contra"}variant ${tvar.show} in ${tp.show}")
            ensureConstrained()
            tvar.instantiate(fromBelow = v == 1)
          }
        }

In normal command line compilation, we are using ConsoleReporter, hasUnreportedErrors will be false, despite there's error in ConsoleReporter ( unspecified type of anonymous function), thus resulting instantiating of List.unapplySeq[A](null) with A = Nothing. The instantiation in turn leads to 3 overloading errors.

In the testing, we are using StoreReporter, thus hasUnreportedErrors will be true due to the unspecified type of the anonymous function. Now, instantiation of A in List.unapplySeq[A](null) will be delayed, after type checking (1, 2, 3). So there will not be overloading errors and A = Int eventually.

After the change in this PR, we are triggering early instantiation even for StoreReporter, thus now in test we get 4 errors.

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 clearing this up, Fengyung!

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to change or document hasUnreportedErrors somehow? The fact that different reporters will influence type inference doesn't sound correct.

assertEquals("x$1", param.show)
assertEquals(s"List(ValDef(${param.show},TypeTree,EmptyTree))", args.toString)
assertEquals("?", pt.show)
Expand Down
20 changes: 20 additions & 0 deletions tests/pos/i2104.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
case class Pair[A, B](_1: A, _2: B)

trait Cons[+H, +T]

object Cons {
def apply[H, T](h: H, t: T): Cons[H, T] = ???
def unapply[H, T](t: Cons[H, T]): Option[Pair[H, T]] = ???
}



object Test {
def main(args: Array[String]): Unit = {
Cons(Option(1), None) match {
case Cons(Some(i), None) =>
i: Int // error: found: Any(i), requires: Int
assert(i == 1)
}
}
}