Skip to content
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

Fix #3324: add isInstanceOf check #4045

Merged
merged 30 commits into from
Apr 5, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
b96b5cc
Fix #3324: introduce IsInstanceOfChecker
liufengyun Feb 22, 2018
94d8313
handle Array in isInstanceOf check
liufengyun Feb 27, 2018
0b9f8a8
fix check for Array
liufengyun Feb 27, 2018
4c2436b
fix runtime check warning for canEqual & equals of case class
liufengyun Feb 27, 2018
ce22485
fix test group
liufengyun Feb 27, 2018
825dbdf
fix typo in numbering of rules
liufengyun Feb 27, 2018
183be33
add more tests
liufengyun Feb 27, 2018
974a7c1
fix nested @unchecked
liufengyun Feb 27, 2018
daf2260
add test case
liufengyun Feb 27, 2018
9ee4518
adapt specification
liufengyun Feb 27, 2018
545930d
tune specification
liufengyun Feb 27, 2018
8719a88
Fix #1828: add test
liufengyun Feb 28, 2018
5fc5db0
Allow test for abstract types if it is always true
liufengyun Feb 28, 2018
d263372
code refactoring
liufengyun Feb 28, 2018
3f12d28
fix test for abstract types
liufengyun Feb 28, 2018
ed13d25
adapt specification for unrelated types
liufengyun Mar 1, 2018
a90f060
Fix GADT test failures
liufengyun Mar 1, 2018
37f4e32
fix fromTasty test
liufengyun Mar 1, 2018
ac696da
Adapt specification to handle narrowed GADT bounds
liufengyun Mar 2, 2018
e836b71
WIP - address review
liufengyun Mar 6, 2018
7adc838
add more tests
liufengyun Mar 7, 2018
734afb3
instantiate if current class is final
liufengyun Mar 7, 2018
5348b31
fix test
liufengyun Mar 7, 2018
0d84bcc
ignore GADT related issues for now
liufengyun Mar 8, 2018
f3d1d90
add classTag test
liufengyun Mar 8, 2018
ac0ec6d
address review
liufengyun Mar 9, 2018
2b23c69
address review
liufengyun Mar 23, 2018
c31c917
use isPatternTypeSymbol to avoid duplicate
liufengyun Mar 23, 2018
a05074d
move pos test to pos-special/isInstanceOf
liufengyun Mar 26, 2018
2b00b7a
fix tests
liufengyun Mar 27, 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
1 change: 1 addition & 0 deletions compiler/src/dotty/tools/dotc/Compiler.scala
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ class Compiler {
new CrossCastAnd, // Normalize selections involving intersection types.
new Splitter) :: // Expand selections involving union types into conditionals
List(new ErasedDecls, // Removes all erased defs and vals decls (except for parameters)
new IsInstanceOfChecker, // check runtime realisability for `isInstanceOf`
new VCInlineMethods, // Inlines calls to value class methods
new SeqLiterals, // Express vararg arguments as arrays
new InterceptedMethods, // Special handling of `==`, `|=`, `getClass` methods
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/ast/Trees.scala
Original file line number Diff line number Diff line change
Expand Up @@ -805,7 +805,7 @@ object Trees {
def unforced: AnyRef
protected def force(x: AnyRef): Unit
def forceIfLazy(implicit ctx: Context): T = unforced match {
case lzy: Lazy[T] =>
case lzy: Lazy[T @unchecked] =>
val x = lzy.complete
force(x)
x
Expand Down
133 changes: 133 additions & 0 deletions compiler/src/dotty/tools/dotc/transform/IsInstanceOfChecker.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
package dotty.tools.dotc
package transform

import util.Positions._
import MegaPhase.MiniPhase
import core._
import Contexts.Context, Types._, Decorators._, Symbols._, typer._, ast._, NameKinds._
import TypeUtils._, Flags._
import config.Printers.{ transforms => debug }

/** Check runtime realizability of type test, see the documentation for `Checkable`.
*/
class IsInstanceOfChecker extends MiniPhase {

import ast.tpd._

val phaseName = "isInstanceOfChecker"

override def transformTypeApply(tree: TypeApply)(implicit ctx: Context): Tree = {
def ensureCheckable(qual: Tree, pt: Tree): Tree = {
if (!Checkable.checkable(qual.tpe, pt.tpe, tree.pos))
ctx.warning(
s"the type test for ${pt.show} cannot be checked at runtime",
tree.pos
)

tree
}

tree.fun match {
case fn: Select
if fn.symbol == defn.Any_typeTest || fn.symbol == defn.Any_isInstanceOf =>
ensureCheckable(fn.qualifier, tree.args.head)
case _ => tree
}
}
}

object Checkable {
import Inferencing._
import ProtoTypes._

/** Whether `(x:X).isInstanceOf[P]` can be checked at runtime?
*
* First do the following substitution:
* (a) replace `T @unchecked` and pattern binder types (e.g., `_$1`) in P with WildcardType
* (b) replace pattern binder types (e.g., `_$1`) in X:
* - variance = 1 : hiBound
* - variance = -1 : loBound
* - variance = 0 : OrType(Any, Nothing) // TODO: use original type param bounds
*
* Then check:
*
* 1. if `X <:< P`, TRUE
* 2. if `P` is a singleton type, TRUE
* 3. if `P` refers to an abstract type member or type parameter, FALSE
* 4. if `P = Array[T]`, checkable(E, T) where `E` is the element type of `X`, defaults to `Any`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why there is a special treatment for Array?

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 @allanrenucci for the helpful feedback, I've addressed them in the latest commit.

For Array, we need a special treatment, because JVM carries type information of array elements, it's possible to test the element type at runtime [1]:

scala> Array(1, 2, 3).getClass
res2: Class[_ <: Array[Int]] = class [I
// where the string "[I" is the run-time type signature for the class 
// object "array with component type int".

[1]: https://docs.oracle.com/javase/specs/jls/se7/html/jls-10.html

* 5. if `P` is `pre.F[Ts]` and `pre.F` refers to a class which is not `Array`:
* (a) replace `Ts` with fresh type variables `Xs`
* (b) constrain `Xs` with `pre.F[Xs] <:< X`
* (c) instantiate Xs and check `pre.F[Xs] <:< P`
* 6. if `P = T1 | T2` or `P = T1 & T2`, checkable(X, T1) && checkable(X, T2).
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a test for & types. For example:

trait Marker
def foo[T](x: T) = x match {
  case _: T & Marker => // no warning
  // case _: T with Marker => // scalac emits a warning
  case _ =>
}

Scalac emits a warning but I believe it is erroneous.
Use case:

def foo[T](xs: List[T]): List[T] = xs.collect { case x: T with Marker => x }

* 7. if `P` is a refinement type, FALSE
* 8. otherwise, TRUE
*/
def checkable(X: Type, P: Type, pos: Position)(implicit ctx: Context): Boolean = {
def isAbstract(P: Type) = !P.dealias.typeSymbol.isClass
def isPatternTypeSymbol(sym: Symbol) = !sym.isClass && sym.is(Case)

def replaceP(implicit ctx: Context) = new TypeMap {
def apply(tp: Type) = tp match {
case tref: TypeRef
if isPatternTypeSymbol(tref.typeSymbol) => WildcardType
case AnnotatedType(_, annot)
if annot.symbol == defn.UncheckedAnnot => WildcardType
case _ => mapOver(tp)
}
}

def replaceX(implicit ctx: Context) = new TypeMap {
def apply(tp: Type) = tp match {
case tref: TypeRef
if isPatternTypeSymbol(tref.typeSymbol) =>
if (variance == 1) tref.info.hiBound
else if (variance == -1) tref.info.loBound
else OrType(defn.AnyType, defn.NothingType)
case _ => mapOver(tp)
}
}

def isClassDetermined(X: Type, P: AppliedType)(implicit ctx: Context) = {
val AppliedType(tycon, _) = P
val typeLambda = tycon.ensureLambdaSub.asInstanceOf[TypeLambda]
val tvars = constrained(typeLambda, untpd.EmptyTree, alwaysAddTypeVars = true)._2.map(_.tpe)
val P1 = tycon.appliedTo(tvars)

debug.println("P : " + P.show)
debug.println("P1 : " + P1.show)
debug.println("X : " + X.show)

P1 <:< X // may fail, ignore

val res = isFullyDefined(P1, ForceDegree.noBottom) && P1 <:< P
debug.println("P1 : " + P1)
debug.println("P1 <:< P = " + res)

res
}

def recur(X: Type, P: Type): Boolean = (X <:< P) || (P match {
case _: SingletonType => true
case _: TypeProxy
if isAbstract(P) => false
case defn.ArrayOf(tpT) =>
X match {
case defn.ArrayOf(tpE) => recur(tpE, tpT)
case _ => recur(defn.AnyType, tpT)
}
case tpe: AppliedType => isClassDetermined(X, tpe)(ctx.fresh.setNewTyperState())
case AndType(tp1, tp2) => recur(X, tp1) && recur(X, tp2)
case OrType(tp1, tp2) => recur(X, tp1) && recur(X, tp2)
case AnnotatedType(t, _) => recur(X, t)
case _: RefinedType => false
case _ => true
})

val res = recur(replaceX.apply(X.widen), replaceP.apply(P))

debug.println(i"checking ${X.show} isInstanceOf ${P} = $res")

res
}
}
14 changes: 10 additions & 4 deletions compiler/src/dotty/tools/dotc/transform/SyntheticMethods.scala
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import ast.Trees._
import ast.untpd
import Decorators._
import NameOps._
import Annotations.Annotation
import ValueClasses.isDerivedValueClass
import scala.collection.mutable.ListBuffer
import scala.language.postfixOps
Expand Down Expand Up @@ -152,17 +153,20 @@ class SyntheticMethods(thisPhase: DenotTransformer) {
* def equals(that: Any): Boolean =
* (this eq that) || {
* that match {
* case x$0 @ (_: C) => this.x == this$0.x && this.y == x$0.y
* case x$0 @ (_: C @unchecked) => this.x == this$0.x && this.y == x$0.y
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add comment warning maintainers this code is fragile, since unchecked might hide type errors...

* case _ => false
* }
* ```
*
* If `C` is a value class the initial `eq` test is omitted.
*
* `@unchecked` is needed for parametric case classes.
*
*/
def equalsBody(that: Tree)(implicit ctx: Context): Tree = {
val thatAsClazz = ctx.newSymbol(ctx.owner, nme.x_0, Synthetic, clazzType, coord = ctx.owner.pos) // x$0
def wildcardAscription(tp: Type) = Typed(Underscore(tp), TypeTree(tp))
val pattern = Bind(thatAsClazz, wildcardAscription(clazzType)) // x$0 @ (_: C)
val pattern = Bind(thatAsClazz, wildcardAscription(AnnotatedType(clazzType, Annotation(defn.UncheckedAnnot)))) // x$0 @ (_: C @unchecked)
val comparisons = accessors map { accessor =>
This(clazz).select(accessor).equal(ref(thatAsClazz).select(accessor)) }
val rhs = // this.x == this$0.x && this.y == x$0.y
Expand Down Expand Up @@ -250,10 +254,12 @@ class SyntheticMethods(thisPhase: DenotTransformer) {
* gets the `canEqual` method
*
* ```
* def canEqual(that: Any) = that.isInstanceOf[C]
* def canEqual(that: Any) = that.isInstanceOf[C @unchecked]
* ```
*
* `@unchecked` is needed for parametric case classes.
*/
def canEqualBody(that: Tree): Tree = that.isInstance(clazzType)
def canEqualBody(that: Tree): Tree = that.isInstance(AnnotatedType(clazzType, Annotation(defn.UncheckedAnnot)))

symbolsToSynthesize flatMap syntheticDefIfMissing
}
Expand Down
2 changes: 2 additions & 0 deletions compiler/test/dotty/tools/dotc/CompilationTests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ class CompilationTests extends ParallelTesting {
) +
compileFilesInDir("tests/pos-special/spec-t5545", defaultOptions) +
compileFilesInDir("tests/pos-special/strawman-collections", defaultOptions) +
compileFilesInDir("tests/pos-special/isInstanceOf", allowDeepSubtypes.and("-Xfatal-warnings")) +
compileFile("scala2-library/src/library/scala/collection/immutable/IndexedSeq.scala", defaultOptions) +
compileFile("scala2-library/src/library/scala/collection/parallel/mutable/ParSetLike.scala", defaultOptions) +
compileList(
Expand Down Expand Up @@ -190,6 +191,7 @@ class CompilationTests extends ParallelTesting {
compileFile("tests/neg-custom-args/noimports2.scala", defaultOptions.and("-Yno-imports")) +
compileFile("tests/neg-custom-args/i3882.scala", allowDeepSubtypes) +
compileFile("tests/neg-custom-args/i1754.scala", allowDeepSubtypes) +
compileFilesInDir("tests/neg-custom-args/isInstanceOf", allowDeepSubtypes and "-Xfatal-warnings") +
compileFile("tests/neg-custom-args/i3627.scala", allowDeepSubtypes)
}.checkExpectedErrors()

Expand Down
9 changes: 9 additions & 0 deletions tests/neg-custom-args/isInstanceOf/1828.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
class Test {
def remove[S](a: S | Int, f: Int => S):S = a match {
case a: S => a // error
case a: Int => f(a)
}

val t: Int | String = 5
val t1 = remove[String](t, _.toString)
}
9 changes: 9 additions & 0 deletions tests/neg-custom-args/isInstanceOf/3324b.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
class C[T] {
val x: Any = ???
if (x.isInstanceOf[List[String]]) // error: unchecked
if (x.isInstanceOf[T]) // error: unchecked
x match {
case x: List[String] => // error: unchecked
case x: T => // error: unchecked
}
}
8 changes: 8 additions & 0 deletions tests/neg-custom-args/isInstanceOf/3324f.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
trait C[T]
class D[T]

class Test {
def foo[T](x: C[T]) = x match {
case _: D[T] => // error
}
}
19 changes: 19 additions & 0 deletions tests/neg-custom-args/isInstanceOf/3324g.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
class Test {
trait A[+T]
class B[T] extends A[T]
class C[T] extends B[Any] with A[T]

def foo[T](c: C[T]): Unit = c match {
case _: B[T] => // error
}

def bar[T](b: B[T]): Unit = b match {
case _: A[T] =>
}

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

quux(new C[Int])
}
22 changes: 22 additions & 0 deletions tests/neg-custom-args/isInstanceOf/4075.scala.ignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
object Test {
trait Foo
case class One[+T](fst: T)

def bad[T <: Foo](e: One[T])(x: T) = e match {
case foo: One[a] =>
x.isInstanceOf[a] // error
val y: Any = ???
y.isInstanceOf[a] // error
}
}

object Test2 {
case class One[T](fst: T)

def bad[T](e: One[T])(x: T) = e match {
case foo: One[a] =>
x.isInstanceOf[a] // error
val y: Any = ???
y.isInstanceOf[a] // error
}
}
9 changes: 9 additions & 0 deletions tests/neg-custom-args/isInstanceOf/enum-approx2.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
sealed trait Exp[T]
case class Fun[A, B](f: Exp[A => B]) extends Exp[A => B]

class Test {
def eval[T](e: Exp[T]) = e match {
case Fun(x: Fun[Int, Double]) => ??? // error
case Fun(x: Exp[Int => String]) => ??? // error
}
Copy link
Contributor

@Blaisorblade Blaisorblade Mar 6, 2018

Choose a reason for hiding this comment

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

Looking at this with @liufengyun, we realize this complication has to do with compiling a match like:

def eval[T](e: Exp[T]) = e match {
  case f @ Fun(x: Fun[Int, Double]) => ???
}

The correct desugaring is a bit surprising to me, because we want to ensure f: Fun[Int, Double], but this is not justified where f is bound but only after the inner match.

def eval[T](e: Exp[T]) = e match {
  case f1: Fun[a1, b1] =>
    f1.f match {
      case f2: Fun[Int, Double] =>
        //***Here*** we learn that a1 = Int and b1 = Double
        //So only ***here*** we can bind `f` with the correct type.
        val f: Fun[Int, Double] = f1
    }
}

We suspect the typechecker currently refines type variables a1 and b1 too soon, which has been fine until now, but strictly speaking means (I think) the current typechecker output is not well-typed in a sound typesystem.

EDIT: as usual, up to misunderstandings.

}
4 changes: 4 additions & 0 deletions tests/neg-custom-args/isInstanceOf/i3324.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
class Foo {
def foo(x: Any): Boolean =
x.isInstanceOf[List[String]] // error
}
8 changes: 8 additions & 0 deletions tests/pos-special/isInstanceOf/3324c.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
sealed trait A[T]
class B[T] extends A[T]

class Test {
def f(x: B[Int]) = x match { case _: A[Int] if true => }

def g(x: A[Int]) = x match { case _: B[Int] => }
}
8 changes: 8 additions & 0 deletions tests/pos-special/isInstanceOf/3324d.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
class Test {
val x: Any = ???

x match {
case _: List[Int @unchecked] => 5
case _: List[Int] @unchecked => 5
}
}
17 changes: 17 additions & 0 deletions tests/pos-special/isInstanceOf/3324e.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
class C[T] {
val x: T = ???
x.isInstanceOf[T]

val y: Array[T] = ???

y match {
case x: Array[T] =>
}

type F[X]

val z: F[T] = ???
z match {
case x: F[T] =>
}
}
8 changes: 8 additions & 0 deletions tests/pos-special/isInstanceOf/3324h.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
object Test {
trait Marker
def foo[T](x: T) = x match {
case _: (T & Marker) => // no warning
case _: T with Marker => // scalac emits a warning
case _ =>
}
}
8 changes: 8 additions & 0 deletions tests/pos-special/isInstanceOf/Result.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
object p {

// test parametric case classes, which synthesis `canEqual` and `equals`
enum Result[+T, +E] {
case OK [T](x: T) extends Result[T, Nothing]
case Err[E](e: E) extends Result[Nothing, E]
}
}
10 changes: 10 additions & 0 deletions tests/pos-special/isInstanceOf/classTag.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import scala.reflect.ClassTag

object IsInstanceOfClassTag {
def safeCast[T: ClassTag](x: Any): Option[T] = {
x match {
case x: T => Some(x)
case _ => None
}
}
}
Loading