-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix #5494: Spurious unchecked warning when type testing an alias #8808
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
Changes from 16 commits
aeeb14b
3991198
b1a9fd7
7e30d84
d61eb85
77bd0cc
891db3c
2531e18
58f506b
10169b1
31c751f
77e96cd
b5ae277
1cb965a
2e028af
33094bf
fb672e5
4a5ab5a
841d191
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,7 +48,7 @@ object TypeTestsCasts { | |
* 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` | ||
* (c) maximize `pre.F[Xs]` and check `pre.F[Xs] <:< P` | ||
* 6. if `P = T1 | T2` or `P = T1 & T2`, checkable(X, T1) && checkable(X, T2). | ||
* 7. if `P` is a refinement type, FALSE | ||
* 8. otherwise, TRUE | ||
|
@@ -105,10 +105,18 @@ object TypeTestsCasts { | |
debug.println("P1 : " + P1.show) | ||
debug.println("X : " + X.show) | ||
|
||
P1 <:< X // constraint P1 | ||
// It does not matter if P1 is not a subtype of X. | ||
// It just tries to infer type arguments of P1 from X if the value x | ||
// conforms to the type skeleton pre.F[_]. Then it goes on to check | ||
// if P1 <: P, which means the type arguments in P are trivial, | ||
// thus no runtime checks are needed for them. | ||
P1 <:< X | ||
smarter marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// use fromScala2x to avoid generating pattern bound symbols | ||
maximizeType(P1, span, fromScala2x = true) | ||
// Maximization of the type means we try to cover all possible values | ||
// which conform to the skeleton pre.F[_] and X. Then we have to make | ||
// sure all of them are actually of the type P, which implies that the | ||
// type arguments in P are trivial (no runtime check needed). | ||
maximizeType(P1, span, fromScala2x = false) | ||
smarter marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
val res = P1 <:< P | ||
debug.println("P1 : " + P1.show) | ||
|
@@ -117,7 +125,7 @@ object TypeTestsCasts { | |
res | ||
} | ||
|
||
def recur(X: Type, P: Type): Boolean = (X <:< P) || (P match { | ||
def recur(X: Type, P: Type): Boolean = (X <:< P) || (P.dealias match { | ||
case _: SingletonType => true | ||
case _: TypeProxy | ||
if isAbstract(P) => false | ||
|
@@ -136,10 +144,11 @@ object TypeTestsCasts { | |
// See TypeComparer#either | ||
recur(tp1, P) && recur(tp2, P) | ||
case _ => | ||
// first try withou striping type parameters for performance | ||
// always false test warnings are emitted elsewhere | ||
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()) | ||
// first try without striping type parameters for performance | ||
isClassDetermined(X, tpe)(ctx.fresh.setNewTyperState().setFreshGADTBounds) || | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method is called There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The case for non-class type constructors is tested in the case There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 _ =>
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
liufengyun marked this conversation as resolved.
Show resolved
Hide resolved
|
||
isClassDetermined(stripTypeParam(X), tpe)(ctx.fresh.setNewTyperState().setFreshGADTBounds) | ||
} | ||
case AndType(tp1, tp2) => recur(X, tp1) && recur(X, tp2) | ||
case OrType(tp1, tp2) => recur(X, tp1) && recur(X, tp2) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
object Test1 { | ||
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) | ||
} | ||
|
||
object Test2 { | ||
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]] | ||
|
||
foo1(new DummyTree) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
class Test { | ||
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]) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
class A | ||
class B | ||
|
||
type AorB = A | B | ||
|
||
def foo(any: Any) = any match { | ||
case aorb: AorB => | ||
case _ => | ||
} | ||
|
||
def bar[T](x: List[T]) = x.isInstanceof[List[Int]] // error | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
object Test1 { | ||
trait Tree | ||
trait Context | ||
|
||
def foo1(myTree: Tree | (Context => Tree)) = | ||
println(myTree.isInstanceOf[Tree]) | ||
|
||
def foo2(myTree: Tree | (Context => Tree)) = | ||
myTree match | ||
case treeFn: (Context => Tree) => // error | ||
case _ => | ||
|
||
def foo3(myTree: Tree | (Context => Tree)) = | ||
myTree match | ||
case treeFn: (_ => _) => // ok | ||
case _ => | ||
} | ||
|
||
object Test2 { | ||
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]) */ | ||
|
||
def foo2(myTree: Tree[Type] | (Context => Tree[Type])) = | ||
myTree match | ||
case treeFn: (Context => Tree[Type]) => // error | ||
case _ => | ||
|
||
def foo3(myTree: Tree[Type] | (Context => Tree[Type])) = | ||
myTree match | ||
case treeFn: (_ => _) => // ok | ||
case _ => | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
class A[-T] | ||
class B[T] extends A[T] | ||
|
||
object Test { | ||
def foo(x: A[Null]) = x match { | ||
case x: B[Null] => | ||
case _ => | ||
} | ||
} | ||
|
||
def bar[T](x: List[T]) = x.isInstanceof[List[Int]] // error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good for uniformity but why would this make any difference for type test safety warnings ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following code shows why it matters: