Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

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.
  • Loading branch information...
commit fefe6ccc0c47d202156e5e1fc3385b92cbb589a5 1 parent e22d801
@gkossakowski gkossakowski authored
View
2  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)
}
}
View
2  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)
}
}
View
8 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)))
}
}
View
15 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 ----------------------------------------------------------------
View
8 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>
Please sign in to comment.
Something went wrong with that request. Please try again.