Fix #5494: Spurious unchecked warning when type testing an alias #8808

smarter · 2020-04-27T10:07:30Z

No description provided.

Previously, the check relied used `maximizeType`, which means that depending on which bound got picked by type inference, the check might fail. Instead, we only need to check if there is a possible constraint solution by making sure both subtype tests return true. I'm not sure if this is always a safe type test, but it at least avoids spurious warnings. This is necessary to bootstrap dotty after the previous commit which added a `dealias` to `recur`, this probably means that there are other situations where `recur` is missing checks.

smarter · 2020-04-27T11:10:51Z

There's still some bootstrapping errors indicating more issues with the checking logic:

[error] -- Error: /home/smarter/opt/dotty/compiler/src/dotty/tools/dotc/ast/NavigateAST.scala:95:15 
[error] 95 |          case p: WithLazyField[?] =>
[error]    |               ^^^^^^^^^^^^^^^^^^^
[error]    |the type test for dotty.tools.dotc.ast.Trees.WithLazyField[_] @_ cannot be checked at runtime
[error] -- Error: /home/smarter/opt/dotty/compiler/src/dotty/tools/dotc/core/Annotations.scala:89:59 
[error] 89 |    override def isEvaluated: Boolean = myTree.isInstanceOf[Tree]
[error]    |                                        ^^^^^^^^^^^^^^^^^^^^^^^^^
[error]    |the type test for dotty.tools.dotc.ast.tpd.Tree cannot be checked at runtime
[error] -- Error: /home/smarter/opt/dotty/compiler/src/dotty/tools/dotc/core/Annotations.scala:122:59 
[error] 122 |    override def isEvaluated: Boolean = myTree.isInstanceOf[Tree]
[error]     |                                        ^^^^^^^^^^^^^^^^^^^^^^^^^
[error]     |the type test for dotty.tools.dotc.ast.tpd.Tree cannot be checked at runtime
[error] -- Error: /home/smarter/opt/dotty/compiler/src/dotty/tools/dotc/core/tasty/PositionPickler.scala:58:10 
[error] 58 |          _: WithLazyField[?]
[error]    |          ^^^^^^^^^^^^^^^^^^^
[error]    |the type test for dotty.tools.dotc.ast.Trees.WithLazyField[_] @_ cannot be checked at runtime

Doing a type test against a class type applied to wildcards like WithLazyField[?] is never unchecked
Doing a type test against a class type like Tree is never unchecked

@liufengyun do you think you could have a look at those ?

smarter · 2020-04-27T11:14:07Z

Doing a type test against a class type like Tree is never unchecked

Hmm actually maybe this one is correct: Tree here is tpd.Tree and we're checking against tpd.Tree | (Context => tpd.Tree). Because Function1 is a trait, we cannot rule out that there exists some subclass of Tree which extends it and is not a tpd.Tree.

`Tree` here is `tpd.Tree` and we're checking against `tpd.Tree | (Context => tpd.Tree)`. Because `Function1` is a trait, we cannot rule out that there exists some subclass of `Tree` which extends it and is not a `tpd.Tree`. The unchecked warning started appearing after the first commit in this PR.

liufengyun · 2020-04-27T11:31:44Z

Sure, I'll take a look at it. Thanks for the diagnosis.

smarter · 2020-04-27T11:38:02Z

OK, so the real issue with WithLazyField is that it's an unrelated trait to the type we're testing (Positioned), this shouldn't lead to unchecked warnings since any subclass may extend the trait. It didn't lead to errors before this PR because the logic in isClassDetermined used to be:

      P1 <:< X       // constraint P1

      // use fromScala2x to avoid generating pattern bound symbols
      maximizeType(P1, span, fromScala2x = true)

      val res = P1 <:< P

which succeeds even if P1 <:< X returns false.

By the way, the logic in recur is weird because it does:

X.classSymbol.exists && P.classSymbol.exists && !X.classSymbol.asClass.mayHaveCommonChild(P.classSymbol.asClass) || ...

Meaning that if X and P are known to never have a common child, the test returns true and no warning is emitted! Apparently this check was added to deal with union types (9452437) but I think it's done at the wrong level, it leads to the following code emitting no warning:

class A
class B[T]

object Test {
  def foo(x: A) = x match {
    case x: B[Int] =>
    case _ =>
  }
}

smarter · 2020-04-27T11:52:49Z

Maybe we should consider redoing TypeTestsCasts based on the scala 2 implementation which is much more battle-tested: https://github.com/scala/scala/blob/2.13.x/src/compiler/scala/tools/nsc/typechecker/Checkable.scala

liufengyun · 2020-04-27T12:14:15Z

@smarter The warning for always false test and runtime checkable are different.

The always false test warning is actually issued:

scala> class A
     | class B[T]
     |
     | object Test {
     |   def foo(x: A) = x match {
     |     case x: B[Int] =>
     |     case _ =>
     |   }
     | }
6 |    case x: B[Int] =>
  |         ^
  |         this case is unreachable since type A and class B are unrelated

liufengyun · 2020-04-28T13:09:36Z

Maybe we should consider redoing TypeTestsCasts based on the scala 2 implementation which is much more battle-tested: https://github.com/scala/scala/blob/2.13.x/src/compiler/scala/tools/nsc/typechecker/Checkable.scala

I had a look at our implementation, I don't think it's a good idea to go to Scala 2 implementation, I believe our approach is more principled. We have a spec, and the bugs so far found can be easily fixed without changing the spec nor complicating the implementation.

The isInstanceOf check is, in essence, an inference problem. Thanks to the powerful GADT support in Dotty (thanks to @AleksanderBG ), we can now handle the check properly.

As far as I know, the Scala 2 implementation does not resort to type inference, thus it cannot cover the cases that the Dotty implementation can handle.

For example, Scala 2 does not issue any warning for the following code, while our check do:

  trait A[+T]
  class B[T] extends A[T]

  class C
  class D extends C

  def quux(a: A[C]): Unit = a match {
    case _: B[C] => // error!!
  }

  quux(new B[D])

Another evidence for the correctness of the new implementation is that it found many true positives in the compiler itself, catching mistakes from missing variance annotation on tree definitions (58f506b), missing prefix such as untpd in patterns, or missing @unchecked (31c751f).

It also catches the subtle errors demonstrated in the following code:

  trait Tree[-T]
  trait Context

  trait Type

  def foo1(myTree: Tree[Type] | (Context => Tree[Type])) =
    println(myTree.isInstanceOf[Tree[Type]])   // error
    /* class DummyTree extends Tree[Nothing] with (Context => Tree[Type]) */

So I'd suggest we stay with the current scheme until a better solution emerges.

liufengyun · 2020-04-28T13:15:36Z

test performance please

dottybot · 2020-04-28T13:31:40Z

performance test scheduled: 3 job(s) in queue, 1 running.

smarter · 2020-04-28T16:00:59Z

compiler/src/dotty/tools/dotc/ast/Trees.scala

@@ -579,7 +579,7 @@ object Trees {
  }

  /** Array(elems) */
-  class JavaSeqLiteral[T >: Untyped] private[ast] (elems: List[Tree[T]], elemtpt: Tree[T])(implicit @constructorOnly src: SourceFile)
+  class JavaSeqLiteral[-T >: Untyped] private[ast] (elems: List[Tree[T]], elemtpt: Tree[T])(implicit @constructorOnly src: SourceFile)


This is good for uniformity but why would this make any difference for type test safety warnings ?

The following code shows why it matters:

trait Tree[-T] class JavaSeqLiteral[T] extends Tree[T] trait Type class DummyTree extends JavaSeqLiteral[Any] def foo1(tree: Tree[Type]) = tree.isInstanceOf[JavaSeqLiteral[Type]] // error foo1(new DummyTree)

compiler/src/dotty/tools/dotc/transform/TypeTestsCasts.scala

smarter · 2020-04-28T16:05:50Z

compiler/src/dotty/tools/dotc/tastyreflect/ReflectionCompilerInterface.scala

@@ -117,7 +117,7 @@ class ReflectionCompilerInterface(val rootContext: core.Contexts.Context) extend
  def isInstanceOfPackageClause(using ctx: Context): IsInstanceOf[PackageClause] = new {
    def runtimeClass: Class[?] = classOf[PackageClause]
    override def unapply(x: Any): Option[PackageClause] = x match


Can we avoid all these unchecked warnings by replacing x: Any by x: tpd.Tree here and below? /cc @nicolasstucki

dottybot · 2020-04-28T16:39:04Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/8808/ to see the changes.

Benchmarks is based on merging with master (2466d97)

smarter

Thanks a lot for the detailed comments! I have a couple of suggestions but otherwise LGTM.

compiler/src/dotty/tools/dotc/transform/TypeTestsCasts.scala

smarter · 2020-04-29T10:44:00Z

compiler/src/dotty/tools/dotc/transform/TypeTestsCasts.scala

            X.classSymbol.exists && P.classSymbol.exists && !X.classSymbol.asClass.mayHaveCommonChild(P.classSymbol.asClass) ||
-            isClassDetermined(X, tpe)(ctx.fresh.setNewTyperState()) ||
-            isClassDetermined(stripTypeParam(X), tpe)(ctx.fresh.setNewTyperState())
+            isClassDetermined(X, tpe)(ctx.fresh.setNewTyperState().setFreshGADTBounds) ||


This method is called isClassDetermined but actually I don't think it's specific to classes: P could be an application of a type lambda for example.

The case for non-class type constructors is tested in the case TypeProxy, where we assume it cannot be checked at runtime. It seems the assumption is practical enough to cover use cases we know of. When the need emerges, we may relax the check a little bit to make it more expressive.

The case for non-class type constructors is tested in the case TypeProxy, where we assume it cannot be checked at runtime.

Ah I see, but this is incomplete: it doesn't handle situation where a type lambda is directly applied (or appears behind an alias):

trait A[T] trait B[T] extends A[T] object Test { def foo(x: ([X] =>> A[X])[Any]) = x match { case x: ([X] =>> B[X])[Any] => case _ => } }

Yes, we haven't seen use cases like this (though the example above type checks). When valid use cases emerge we can easily support them using the method typeArgsTrivial.

compiler/src/dotty/tools/dotc/transform/TypeTestsCasts.scala

smarter requested a review from liufengyun April 27, 2020 10:07

smarter assigned liufengyun Apr 27, 2020

Fix scala#5494: Spurious unchecked warning when type testing an alias

aeeb14b

smarter force-pushed the dealias-type-test branch from 555bce3 to 7722fa9 Compare April 27, 2020 10:38

smarter force-pushed the dealias-type-test branch from 7722fa9 to 3991198 Compare April 27, 2020 10:47

This comment has been minimized.

Sign in to view

smarter mentioned this pull request Apr 27, 2020

Fix #8647: Remove TypeParamRef from instantiated test #8704

Merged

liufengyun added 9 commits April 28, 2020 12:43

Move tests

7e30d84

Handle GADT bounds in isInstanceOf type test

d61eb85

Add test

77bd0cc

Restore original spec

891db3c

Add test for Tree | Context => Tree

2531e18

Add missing variance annotation

58f506b

Add test for Tree | (Context => Tree)

10169b1

Fix unchecked warnings in compiler

31c751f

Fix typo in comment

77e96cd

Fix unchecked warning in language server

b5ae277

smarter commented Apr 28, 2020

View reviewed changes

Add more test case

1cb965a

Add documentation for code

2e028af

liufengyun assigned smarter and unassigned liufengyun Apr 29, 2020

smarter commented Apr 29, 2020

View reviewed changes

smarter assigned liufengyun and unassigned smarter Apr 29, 2020

Address review: add comment

33094bf

smarter commented Apr 29, 2020

View reviewed changes

compiler/src/dotty/tools/dotc/transform/TypeTestsCasts.scala Outdated Show resolved Hide resolved

liufengyun added 3 commits April 29, 2020 13:57

Address review: rename isClassDetermined to typeArgsTrivial

fb672e5

Address review: code refactoring

4a5ab5a

Add test case for type lambda

841d191

liufengyun merged commit c5a76f0 into scala:master Apr 29, 2020

liufengyun deleted the dealias-type-test branch April 29, 2020 13:15

Fix #5494: Spurious unchecked warning when type testing an alias #8808

Fix #5494: Spurious unchecked warning when type testing an alias #8808

Uh oh!

Conversation

smarter commented Apr 27, 2020

Uh oh!

smarter commented Apr 27, 2020

Uh oh!

smarter commented Apr 27, 2020

Uh oh!

This comment has been minimized.

This comment has been minimized.

liufengyun commented Apr 27, 2020

Uh oh!

smarter commented Apr 27, 2020

Uh oh!

smarter commented Apr 27, 2020

Uh oh!

liufengyun commented Apr 27, 2020

Uh oh!

liufengyun commented Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liufengyun commented Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dottybot commented Apr 28, 2020

Uh oh!

smarter Apr 28, 2020

Choose a reason for hiding this comment

Uh oh!

liufengyun Apr 28, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

smarter Apr 28, 2020

Choose a reason for hiding this comment

Uh oh!

dottybot commented Apr 28, 2020

Uh oh!

smarter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

smarter Apr 29, 2020

Choose a reason for hiding this comment

Uh oh!

liufengyun Apr 29, 2020

Choose a reason for hiding this comment

Uh oh!

smarter Apr 29, 2020

Choose a reason for hiding this comment

Uh oh!

liufengyun Apr 29, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

liufengyun commented Apr 28, 2020 •

edited

Loading

liufengyun commented Apr 28, 2020 •

edited

Loading