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

spurious unchecked warning when an abstract type is accompanied by a same-named extractor #5503

Closed
scabug opened this issue Feb 19, 2012 · 8 comments

Comments

@scabug
Copy link

scabug commented Feb 19, 2012

This pattern is used in scala.reflect._. I haven't managed to produce a standalone reproduction.

$ ./build/quick/bin/scala -unchecked
Welcome to Scala version 2.10.0-M1-0362-g2fe570291a-2012-02-18 (Java HotSpot(TM) 64-Bit Server VM, Java 1.6.0_29).

scala> import reflect.mirror._
import reflect.mirror._

scala> object A { def foo(t: Tree) = t.tpe match { case MethodType(_, _) => 0 }} 
<console>:13: warning: abstract type reflect.mirror.MethodType in type pattern reflect.mirror.MethodType is unchecked since it is eliminated by erasure
       object A { def foo(t: Tree) = t.tpe match { case MethodType(_, _) => 0 }}

Inspection of the generated code shows that the extractor is being used.

                                                                  ^
defined module A

scala> :javap -v ACompiled from "<console>"
...
public int foo(scala.reflect.api.Trees$Tree);
  Code:
   Stack=3, Locals=3, Args_size=2
   0:	aload_1
   1:	invokevirtual	#21; //Method scala/reflect/api/Trees$Tree.tpe:()Lscala/reflect/api/Types$AbsType;
   4:	astore_2
   5:	aload_2
   6:	instanceof	#23; //class scala/reflect/api/Types$AbsType
   9:	ifeq	38
   12:	getstatic	#28; //Field scala/reflect/package$.MODULE$:Lscala/reflect/package$;
   15:	invokevirtual	#32; //Method scala/reflect/package$.mirror:()Lscala/reflect/api/Mirror;
   18:	checkcast	#34; //class scala/reflect/api/Types
   21:	invokeinterface	#38,  1; //InterfaceMethod scala/reflect/api/Types.MethodType:()Lscala/reflect/api/Types$MethodTypeExtractor;
   26:	aload_2
   27:	invokevirtual	#44; //Method scala/reflect/api/Types$MethodTypeExtractor.unapply:(Lscala/reflect/api/Types$AbsType;)Lscala/Option;
   30:	invokevirtual	#50; //Method scala/Option.isEmpty:()Z
   33:	ifne	38
   36:	iconst_0
   37:	ireturn
   38:	new	#52; //class scala/MatchError
   41:	dup
   42:	aload_2
   43:	invokespecial	#55; //Method scala/MatchError."<init>":(Ljava/lang/Object;)V
   46:	athrow
  LineNumberTable: 
   line 13: 0
...
@scabug
Copy link
Author

scabug commented Feb 19, 2012

Imported From: https://issues.scala-lang.org/browse/SI-5503?orig=1
Reporter: @retronym
Affected Versions: 2.10.0-M1
Duplicates #5143

@scabug
Copy link
Author

scabug commented Feb 19, 2012

@retronym said:
Actually, the warning might make sense.

This gives the same warning (although I'm not certain it's a faithful minimization of the one above).

trait A {  
  type MethodType
  
  val MethodType: MethodTypeExtractor = null

  abstract class MethodTypeExtractor {    
    def unapply(tpe: MethodType): Option[(Any, Any)]
  }
}

object Test {  
  val a: A = null

  def foo(tpe: A#MethodType) = tpe match {
    case a.MethodType(_, _) =>
  }
}

Switching to def unapply(tpe: MethodType), or to def foo(tpe: a.MethodType) silences the warning.

@scabug
Copy link
Author

scabug commented Feb 19, 2012

@retronym said (edited on Feb 19, 2012 4:32:10 PM UTC):
Okay, I think that this is a more accurate minimization:

trait A {  
  type Type
  type MethodType <: Type
  
  val MethodType: MethodTypeExtractor = null

  abstract class MethodTypeExtractor {    
    def unapply(tpe: MethodType): Option[(Any, Any)]
  }
}

object Test {  
  val a: A = null

  def foo(tpe: a.Type) = tpe match {
    case a.MethodType(_, _) =>
  }
}

Who is at fault: compiler or library?

@scabug
Copy link
Author

scabug commented Feb 19, 2012

@adriaanm said:
Blame the old pattern matcher. I know, I know, it's getting old, but virtpatmat will fix this.
See scala/scala@5cc3dad991, #1697, and #2337.

@scabug
Copy link
Author

scabug commented Feb 19, 2012

@retronym said:
So old, in fact, that is was among my first thoughts. Alas:

./build/quick/bin/scala -Yvirtpatmat -unchecked
Welcome to Scala version 2.10.0-M1-0374-g526c92848a-2012-02-19 (Java HotSpot(TM) 64-Bit Server VM, Java 1.6.0_29).
Type in expressions to have them evaluated.
Type :help for more information.

scala> import reflect.mirror._import reflect.mirror._

scala> object A { def foo(t: Tree) = t.tpe match { case MethodType(_, _) => 0 }} 
<console>:10: warning: abstract type reflect.mirror.MethodType in type pattern reflect.mirror.MethodType is unchecked since it is eliminated by erasure
       object A { def foo(t: Tree) = t.tpe match { case MethodType(_, _) => 0 }} 
                                                                  ^
defined module A

scala> :paste
// Entering paste mode (ctrl-D to finish)

trait A {  
  type Type
  type MethodType <: Type
  
  val MethodType: MethodTypeExtractor = null

  abstract class MethodTypeExtractor {    
    def unapply(tpe: MethodType): Option[(Any, Any)]
  }
}

object Test {  
  val a: A = null

  def foo(tpe: a.Type) = tpe match {
    case a.MethodType(_, _) =>
  }
}

// Exiting paste mode, now interpreting.

<console>:25: warning: abstract type Test.a.MethodType in type pattern Test.a.MethodType is unchecked since it is eliminated by erasure
           case a.MethodType(_, _) =>
                            ^

@scabug
Copy link
Author

scabug commented May 1, 2012

@SethTisue said:
related to SI-5542?

@scabug
Copy link
Author

scabug commented May 2, 2012

@adriaanm said:
no, this is a different problem
one aspect was that bridges needed to become more defensive, which has been done
the other problem, I think, is that the extractor's argument is an abstract type, which cannot be checked at all
there will be a new feature in the pattern matcher that allows real type test when a Typeable/TypeTag/Manifest is available

@scabug
Copy link
Author

scabug commented May 2, 2012

@retronym said:
Yeah, I agree that the extractors should be changed to take an argument of type Any, rather than the abstract type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants