From 8610d7ec063407f62b11df848dd588e4594b3b40 Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Tue, 29 Jan 2013 16:11:58 -0800 Subject: Add Bytecode test (ASM-based) to partest. This commit introduces a new kind of test `Bytecode` that allows one to inspect bytecode generated for given piece of Scala code. The bytecode inspection is achieved by inspection of ASM trees. See the included example for details. NOTE: This commit does not introduce a new category of pratest tests. Bytecode tests should be run in `jvm` category of partest tests. Specific list of changes: * Add BytecodeTest that contains common utilities to partest * Add asm to classpath when compiling partest. That's not a new dependency as it's being already done for javac task we were running while compiling partest. * Add an example test that shows how to count null checks in given method. --- test/files/jvm/bytecode-test-example.check | 1 + test/files/jvm/bytecode-test-example/Foo_1.scala | 9 +++++++ test/files/jvm/bytecode-test-example/Test.scala | 32 ++++++++++++++++++++++++ 3 files changed, 42 insertions(+) create mode 100644 test/files/jvm/bytecode-test-example.check create mode 100644 test/files/jvm/bytecode-test-example/Foo_1.scala create mode 100644 test/files/jvm/bytecode-test-example/Test.scala (limited to 'test/files/jvm') diff --git a/test/files/jvm/bytecode-test-example.check b/test/files/jvm/bytecode-test-example.check new file mode 100644 index 0000000000..0cfbf08886 --- /dev/null +++ b/test/files/jvm/bytecode-test-example.check @@ -0,0 +1 @@ +2 diff --git a/test/files/jvm/bytecode-test-example/Foo_1.scala b/test/files/jvm/bytecode-test-example/Foo_1.scala new file mode 100644 index 0000000000..4f679d156f --- /dev/null +++ b/test/files/jvm/bytecode-test-example/Foo_1.scala @@ -0,0 +1,9 @@ +class Foo_1 { + def foo(x: AnyRef): Int = { + val bool = x == null + if (x != null) + 1 + else + 0 + } +} diff --git a/test/files/jvm/bytecode-test-example/Test.scala b/test/files/jvm/bytecode-test-example/Test.scala new file mode 100644 index 0000000000..d668059cb7 --- /dev/null +++ b/test/files/jvm/bytecode-test-example/Test.scala @@ -0,0 +1,32 @@ +import scala.tools.partest.BytecodeTest + +import scala.tools.nsc.util.JavaClassPath +import java.io.InputStream +import scala.tools.asm +import asm.ClassReader +import asm.tree.{ClassNode, InsnList} +import scala.collection.JavaConverters._ + +object Test extends BytecodeTest { + def show: Unit = { + val classNode = loadClassNode("Foo_1") + val methodNode = getMethod(classNode, "foo") + println(countNullChecks(methodNode.instructions)) + } + + def countNullChecks(insnList: InsnList): Int = { + /** Is given instruction a null check? + * NOTE + * This will detect direct null compparsion as in + * if (x == null) ... + * and not indirect as in + * val foo = null + * if (x == foo) ... + */ + def isNullCheck(node: asm.tree.AbstractInsnNode): Boolean = { + val opcode = node.getOpcode + (opcode == asm.Opcodes.IFNULL) || (opcode == asm.Opcodes.IFNONNULL) + } + insnList.iterator.asScala.count(isNullCheck) + } +} -- cgit v1.2.3 From e22d801a539f951422e91519844c7cb9ce81b413 Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Thu, 24 Jan 2013 16:36:23 -0800 Subject: Test case for SI-7009. The next commit fixes the problem itself and it's easier to see in diff what's being fixed exactly. --- test/files/jvm/throws-annot-from-java.check | 47 ++++++++++++++++++++++ .../PolymorphicException_1.scala | 3 ++ test/files/jvm/throws-annot-from-java/Test_3.scala | 29 +++++++++++++ .../ThrowsDeclaration_2.java | 6 +++ 4 files changed, 85 insertions(+) create mode 100644 test/files/jvm/throws-annot-from-java.check create mode 100644 test/files/jvm/throws-annot-from-java/PolymorphicException_1.scala create mode 100644 test/files/jvm/throws-annot-from-java/Test_3.scala create mode 100644 test/files/jvm/throws-annot-from-java/ThrowsDeclaration_2.java (limited to 'test/files/jvm') diff --git a/test/files/jvm/throws-annot-from-java.check b/test/files/jvm/throws-annot-from-java.check new file mode 100644 index 0000000000..376061be2e --- /dev/null +++ b/test/files/jvm/throws-annot-from-java.check @@ -0,0 +1,47 @@ +Type in expressions to have them evaluated. +Type :help for more information. + +scala> :power +** Power User mode enabled - BEEP WHIR GYVE ** +** :phase has been set to 'typer'. ** +** scala.tools.nsc._ has been imported ** +** global._, definitions._ also imported ** +** Try :help, :vals, power. ** + +scala> :paste +// Entering paste mode (ctrl-D to finish) + +{ + val clazz = rootMirror.getClassByName(newTermName("test.ThrowsDeclaration_2")); + { + val method = clazz.info.member(newTermName("foo")) + val throwsAnn = method.annotations.head + val atp = throwsAnn.atp + println("foo") + println("atp.typeParams.isEmpty: " + atp.typeParams.isEmpty) + println(throwsAnn) + } + println + + { + val method = clazz.info.member(newTermName("bar")) + val throwsAnn = method.annotations.head + val Literal(const) = throwsAnn.args.head + val tp = const.typeValue + println("bar") + println("tp.typeParams.isEmpty: " + tp.typeParams.isEmpty) + println(throwsAnn) + } +} + +// Exiting paste mode, now interpreting. + +foo +atp.typeParams.isEmpty: false +throws(classOf[java.lang.IllegalStateException]) + +bar +tp.typeParams.isEmpty: false +throws(classOf[test.PolymorphicException]) + +scala> diff --git a/test/files/jvm/throws-annot-from-java/PolymorphicException_1.scala b/test/files/jvm/throws-annot-from-java/PolymorphicException_1.scala new file mode 100644 index 0000000000..58fa536f0b --- /dev/null +++ b/test/files/jvm/throws-annot-from-java/PolymorphicException_1.scala @@ -0,0 +1,3 @@ +package test + +class PolymorphicException[T] extends Exception diff --git a/test/files/jvm/throws-annot-from-java/Test_3.scala b/test/files/jvm/throws-annot-from-java/Test_3.scala new file mode 100644 index 0000000000..de1d984573 --- /dev/null +++ b/test/files/jvm/throws-annot-from-java/Test_3.scala @@ -0,0 +1,29 @@ +import scala.tools.partest.ReplTest + +object Test extends ReplTest { + def code = """:power +:paste +{ + val clazz = rootMirror.getClassByName(newTermName("test.ThrowsDeclaration_2")); + { + val method = clazz.info.member(newTermName("foo")) + val throwsAnn = method.annotations.head + val atp = throwsAnn.atp + println("foo") + println("atp.typeParams.isEmpty: " + atp.typeParams.isEmpty) + println(throwsAnn) + } + println + + { + val method = clazz.info.member(newTermName("bar")) + val throwsAnn = method.annotations.head + val Literal(const) = throwsAnn.args.head + val tp = const.typeValue + println("bar") + println("tp.typeParams.isEmpty: " + tp.typeParams.isEmpty) + println(throwsAnn) + } +} +""" +} diff --git a/test/files/jvm/throws-annot-from-java/ThrowsDeclaration_2.java b/test/files/jvm/throws-annot-from-java/ThrowsDeclaration_2.java new file mode 100644 index 0000000000..3708fe626b --- /dev/null +++ b/test/files/jvm/throws-annot-from-java/ThrowsDeclaration_2.java @@ -0,0 +1,6 @@ +package test; + +public class ThrowsDeclaration_2 { + public void foo() throws IllegalStateException {}; + public void bar() throws PolymorphicException {}; +} -- cgit v1.2.3 From fefe6ccc0c47d202156e5e1fc3385b92cbb589a5 Mon Sep 17 00:00:00 2001 From: Grzegorz Kossakowski Date: Wed, 30 Jan 2013 11:06:55 -0800 Subject: SI-7009: `@throws` annotation synthesized incorrectly The 990b3c7 made `scala.throws` annotation polymorphic but forgot to adapt compiler code that synthesizes it, e.g. when parsing class files. The consequence was that we would get non-deterministically either `scala.throws` or `scala.throws[T]` as a type for synthesized annotation. The reason is that `Symbol.addAnnotation` would call `tpe` method which does not initialization of symbol so type parameters list would not be determined correctly. Only if info of that symbol was forced for other reason we would get `scala.throws[T]`. That non-deterministic behavior was observed in sbt's incremental compiler. Another problem we have is that Scala allows polymorphic exceptions so in ClassfileParser we could synthesize `@throws` annotation with wrong (polymorphic) type applied. In such case the best we can do is to convert such type to monomorphic one by introducing existentials. Here's list of changes this commit introduces: * The `Symbol.addAnnotation` that takes symbol as argument asserts that the type represented by that symbol is monomorphic (disabled due to cycles; see comments in the code) * Introduce `Symbol.addAnnotation` overload that allows us to pass an applied type * Change all places where polymorphic annotations are synthesized to pass an applied type * Handle polymorphic exception types in `ClassfileParser.parseExceptions` Fixes SI-7009. --- src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala | 2 +- src/compiler/scala/tools/nsc/backend/jvm/GenJVM.scala | 2 +- .../tools/nsc/symtab/classfile/ClassfileParser.scala | 8 +++++++- src/reflect/scala/reflect/internal/Symbols.scala | 15 ++++++++++++++- test/files/jvm/throws-annot-from-java.check | 8 ++++---- 5 files changed, 27 insertions(+), 8 deletions(-) (limited to 'test/files/jvm') diff --git a/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala b/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala index d185ed0c34..0abbe44b02 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala @@ -1018,7 +1018,7 @@ abstract class GenASM extends SubComponent with BytecodeWriters with GenJVMASM { if (needsAnnotation) { val c = Constant(RemoteExceptionClass.tpe) val arg = Literal(c) setType c.tpe - meth.addAnnotation(ThrowsClass, arg) + meth.addAnnotation(appliedType(ThrowsClass, c.tpe), arg) } } diff --git a/src/compiler/scala/tools/nsc/backend/jvm/GenJVM.scala b/src/compiler/scala/tools/nsc/backend/jvm/GenJVM.scala index fe0020e074..598965b982 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/GenJVM.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/GenJVM.scala @@ -888,7 +888,7 @@ abstract class GenJVM extends SubComponent with GenJVMUtil with GenAndroid with if (needsAnnotation) { val c = Constant(RemoteExceptionClass.tpe) val arg = Literal(c) setType c.tpe - meth.addAnnotation(ThrowsClass, arg) + meth.addAnnotation(appliedType(ThrowsClass, c.tpe), arg) } } diff --git a/src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala b/src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala index a708a262e7..4b1d3c34f3 100644 --- a/src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala +++ b/src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala @@ -1043,7 +1043,13 @@ abstract class ClassfileParser { val nClasses = in.nextChar for (n <- 0 until nClasses) { val cls = pool.getClassSymbol(in.nextChar.toInt) - sym.addAnnotation(definitions.ThrowsClass, Literal(Constant(cls.tpe))) + val tp = if (cls.isMonomorphicType) cls.tpe else { + debuglog(s"Encountered polymorphic exception `${cls.fullName}` while parsing class file.") + // in case we encounter polymorphic exception the best we can do is to convert that type to + // monomorphic one by introducing existientals, see SI-7009 for details + typer.packSymbols(cls.typeParams, cls.tpe) + } + sym.addAnnotation(appliedType(definitions.ThrowsClass, tp), Literal(Constant(tp))) } } diff --git a/src/reflect/scala/reflect/internal/Symbols.scala b/src/reflect/scala/reflect/internal/Symbols.scala index c274a9e3af..5fe02331ef 100644 --- a/src/reflect/scala/reflect/internal/Symbols.scala +++ b/src/reflect/scala/reflect/internal/Symbols.scala @@ -1583,8 +1583,21 @@ trait Symbols extends api.Symbols { self: SymbolTable => setAnnotations(annot :: annotations) // Convenience for the overwhelmingly common case - def addAnnotation(sym: Symbol, args: Tree*): this.type = + def addAnnotation(sym: Symbol, args: Tree*): this.type = { + // The assertion below is meant to prevent from issues like SI-7009 but it's disabled + // due to problems with cycles while compiling Scala library. It's rather shocking that + // just checking if sym is monomorphic type introduces nasty cycles. We are definitively + // forcing too much because monomorphism is a local property of a type that can be checked + // syntactically + // assert(sym.initialize.isMonomorphicType, sym) addAnnotation(AnnotationInfo(sym.tpe, args.toList, Nil)) + } + + /** Use that variant if you want to pass (for example) an applied type */ + def addAnnotation(tp: Type, args: Tree*): this.type = { + assert(tp.typeParams.isEmpty, tp) + addAnnotation(AnnotationInfo(tp, args.toList, Nil)) + } // ------ comparisons ---------------------------------------------------------------- diff --git a/test/files/jvm/throws-annot-from-java.check b/test/files/jvm/throws-annot-from-java.check index 376061be2e..be3ba412f8 100644 --- a/test/files/jvm/throws-annot-from-java.check +++ b/test/files/jvm/throws-annot-from-java.check @@ -37,11 +37,11 @@ scala> :paste // Exiting paste mode, now interpreting. foo -atp.typeParams.isEmpty: false -throws(classOf[java.lang.IllegalStateException]) +atp.typeParams.isEmpty: true +throws[IllegalStateException](classOf[java.lang.IllegalStateException]) bar -tp.typeParams.isEmpty: false -throws(classOf[test.PolymorphicException]) +tp.typeParams.isEmpty: true +throws[test.PolymorphicException[_]](classOf[test.PolymorphicException]) scala> -- cgit v1.2.3