Skip to content

Commit

Permalink
Only accept singletons, only check arguments
Browse files Browse the repository at this point in the history
  • Loading branch information
mbovel committed Oct 24, 2024
1 parent 9fcfbaf commit 7f5eaa8
Show file tree
Hide file tree
Showing 19 changed files with 95 additions and 110 deletions.
1 change: 1 addition & 0 deletions compiler/src/dotty/tools/dotc/ast/TreeInfo.scala
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ trait TreeInfo[T <: Untyped] { self: Trees.Instance[T] =>
def allTermArguments(tree: Tree): List[Tree] = unsplice(tree) match {
case Apply(fn, args) => allTermArguments(fn) ::: args
case TypeApply(fn, args) => allTermArguments(fn)
// TOOD(mbovel): is it really safe to skip all blocks here and in `allArguments`?
case Block(_, expr) => allTermArguments(expr)
case _ => Nil
}
Expand Down
61 changes: 29 additions & 32 deletions compiler/src/dotty/tools/dotc/typer/Checking.scala
Original file line number Diff line number Diff line change
Expand Up @@ -937,16 +937,15 @@ object Checking {
case Literal(_) => // ok
case _ =>
report.error(em"@${cls.name} needs a string literal as argument", arg.srcPos)
case Apply(tycon, args) => args.foreach(checkAnnotArg)
case _ =>
// `FirstTransform.toTypeTree` creates `Annotated` nodes where the
// annotation tree is not an `Apply` node. For example, in
// tests/neg/i13044.scala, `FirstTransform.toTypeTree` creates two
// annotations with trees `t` and `ts`. In tests/run/t2755.scala,
// `FirstTransform.toTypeTree` creates an annotated types with tree `_`.
//
// report.error(em"unexpected annotation tree: $tree", tree.srcPos)
()
if cls.isRetainsLike then () // Do not check @retain annotations
else if cls == defn.ThrowsAnnot then
// Do not check @throws annotations.
// TODO(mbovel): in tests/run/t6380.scala, an annotation tree is
// `new throws[Exception](throws.<init>[Exception])`. What is this?
()
else
tpd.allTermArguments(tree).foreach(checkAnnotArg)
tree

private def checkAnnotArg(tree: Tree)(using Context): Unit =
Expand All @@ -955,40 +954,38 @@ object Checking {

def isFunctionAllowed(t: Tree): Boolean =
t match
case Select(qual, nme.apply) => qual.symbol == defn.ArrayModule || isTupleModule(qual.symbol)
case Select(qual, nme.apply) =>
qual.symbol == defn.ArrayModule
|| qual.symbol == defn.ClassTagModule // class tags are used as arguments to Array.apply
|| isTupleModule(qual.symbol)
case Select(New(clazz), nme.CONSTRUCTOR) => clazz.symbol.isAnnotation
case Apply(fun, _) => isFunctionAllowed(fun)
case TypeApply(fun, _) => isFunctionAllowed(fun)
case _ => false

def valid(t: Tree): Boolean =
t match
case Literal(_) => true
case Apply(fun, args) => isFunctionAllowed(fun) && args.forall(valid)
case SeqLiteral(elems, _) => elems.forall(valid)
case Typed(expr, _) => valid(expr)
case NamedArg(_, arg) => valid(arg)
case _ =>
t.tpe.stripped match
case _: SingletonType => true
// We need to handle type refs for these test cases:
// - tests/pos/dependent-annot.scala
// - tests/pos/i16208.scala
// - tests/run/java-ann-super-class
// - tests/run/java-ann-super-class-separate
// - tests/neg/i19470.scala (@retains)
// Why do we get type refs in these cases?
case _: TypeRef => true
case _: TypeParamRef => true
case tp => false
t.tpe.isEffectivelySingleton
|| (
t match
case Literal(_) => true
case Ident(nme.WILDCARD) => true // example: tests/run/java-ann-super-class
case Apply(fun, args) => isFunctionAllowed(fun) && args.forall(valid)
case TypeApply(fun, args) => isFunctionAllowed(fun)
//case TypeApply(meth @ Select(arg, _), _) if meth.symbol == defn.Any_asInstanceOf => valid(arg)
case SeqLiteral(elems, _) => elems.forall(valid)
case Typed(expr, _) => valid(expr)
case NamedArg(_, arg) => valid(arg)
case _ =>
false
)

if !valid(tree) then
report.error(
s"""Implementation restriction: not a valid annotation argument.
i"""Implementation restriction: not a valid annotation argument.
| Argument: $tree
| Type: ${tree.tpe}""".stripMargin,
| Type: ${tree.tpe}""",
tree.srcPos
)

}

trait Checking {
Expand Down
15 changes: 15 additions & 0 deletions tests/neg/i15054.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import scala.annotation.Annotation

class AnAnnotation(function: Int => String) extends Annotation

@AnAnnotation(_.toString) // error: not a valid annotation
val a = 1
@AnAnnotation(_.toString.length.toString) // error: not a valid annotation
val b = 2

def test =
@AnAnnotation(_.toString) // error: not a valid annotation
val a = 1
@AnAnnotation(_.toString.length.toString) // error: not a valid annotation
val b = 2
a + b
2 changes: 2 additions & 0 deletions tests/neg/i7740a.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
class A(a: Any) extends annotation.StaticAnnotation
@A({val x = 0}) trait B // error: not a valid annotation
2 changes: 2 additions & 0 deletions tests/neg/i7740b.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
class A(a: Any) extends annotation.StaticAnnotation
@A({def x = 0}) trait B // error: not a valid annotation
2 changes: 1 addition & 1 deletion tests/pos/i9314.scala → tests/neg/i9314.scala
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
final class fooAnnot[T](member: T) extends scala.annotation.StaticAnnotation // must have type parameter

@fooAnnot(new RecAnnotated {}) // must pass instance of anonymous subclass
@fooAnnot(new RecAnnotated {}) // error: not a valid annotation
trait RecAnnotated
78 changes: 34 additions & 44 deletions tests/neg/nowarn.check
Original file line number Diff line number Diff line change
Expand Up @@ -5,22 +5,22 @@
| its body in a block; no exceptions are handled.
|
| longer explanation available when compiling with `-explain`
-- [E002] Syntax Warning: tests/neg/nowarn.scala:25:25 -----------------------------------------------------------------
25 |@nowarn(o.inl) def t2d = try 1 // two warnings (`inl` is not a compile-time constant)
-- [E002] Syntax Warning: tests/neg/nowarn.scala:22:25 -----------------------------------------------------------------
22 |@nowarn(o.inl) def t2d = try 1 // two warnings (`inl` is not a compile-time constant)
| ^^^^^
| A try without catch or finally is equivalent to putting
| its body in a block; no exceptions are handled.
|
| longer explanation available when compiling with `-explain`
-- [E002] Syntax Warning: tests/neg/nowarn.scala:33:26 -----------------------------------------------------------------
33 |@nowarn("id=1") def t4d = try 1 // error and warning (unused nowarn, wrong id)
-- [E002] Syntax Warning: tests/neg/nowarn.scala:30:26 -----------------------------------------------------------------
30 |@nowarn("id=1") def t4d = try 1 // error and warning (unused nowarn, wrong id)
| ^^^^^
| A try without catch or finally is equivalent to putting
| its body in a block; no exceptions are handled.
|
| longer explanation available when compiling with `-explain`
-- [E002] Syntax Warning: tests/neg/nowarn.scala:35:28 -----------------------------------------------------------------
35 |@nowarn("verbose") def t5 = try 1 // warning with details
-- [E002] Syntax Warning: tests/neg/nowarn.scala:32:28 -----------------------------------------------------------------
32 |@nowarn("verbose") def t5 = try 1 // warning with details
| ^^^^^
| A try without catch or finally is equivalent to putting
| its body in a block; no exceptions are handled.
Expand All @@ -40,71 +40,61 @@ Matching filters for @nowarn or -Wconf:
| ^^^^^^
| Invalid message filter
| unknown filter: wat?
-- [E129] Potential Issue Warning: tests/neg/nowarn.scala:18:12 --------------------------------------------------------
18 |def t2a = { 1; 2 } // warning (invalid nowarn doesn't silence)
| ^
| A pure expression does nothing in statement position
|
| longer explanation available when compiling with `-explain`
-- Warning: tests/neg/nowarn.scala:17:8 --------------------------------------------------------------------------------
17 |@nowarn(t1a.toString) // warning (typer, argument not a compile-time constant)
| ^^^^^^^^^^^^
| filter needs to be a compile-time constant string
-- Warning: tests/neg/nowarn.scala:25:10 -------------------------------------------------------------------------------
25 |@nowarn(o.inl) def t2d = try 1 // two warnings (`inl` is not a compile-time constant)
-- Warning: tests/neg/nowarn.scala:22:10 -------------------------------------------------------------------------------
22 |@nowarn(o.inl) def t2d = try 1 // two warnings (`inl` is not a compile-time constant)
| ^^^^^
| filter needs to be a compile-time constant string
-- Deprecation Warning: tests/neg/nowarn.scala:39:10 -------------------------------------------------------------------
39 |def t6a = f // warning (refchecks, deprecation)
-- Deprecation Warning: tests/neg/nowarn.scala:36:10 -------------------------------------------------------------------
36 |def t6a = f // warning (refchecks, deprecation)
| ^
| method f is deprecated
-- Deprecation Warning: tests/neg/nowarn.scala:42:30 -------------------------------------------------------------------
42 |@nowarn("msg=fish") def t6d = f // error (unused nowarn), warning (deprecation)
-- Deprecation Warning: tests/neg/nowarn.scala:39:30 -------------------------------------------------------------------
39 |@nowarn("msg=fish") def t6d = f // error (unused nowarn), warning (deprecation)
| ^
| method f is deprecated
-- Deprecation Warning: tests/neg/nowarn.scala:49:10 -------------------------------------------------------------------
49 |def t7c = f // warning (deprecation)
-- Deprecation Warning: tests/neg/nowarn.scala:46:10 -------------------------------------------------------------------
46 |def t7c = f // warning (deprecation)
| ^
| method f is deprecated
-- [E092] Pattern Match Unchecked Warning: tests/neg/nowarn.scala:55:7 -------------------------------------------------
55 | case _: List[Int] => 0 // warning (patmat, unchecked)
-- [E092] Pattern Match Unchecked Warning: tests/neg/nowarn.scala:52:7 -------------------------------------------------
52 | case _: List[Int] => 0 // warning (patmat, unchecked)
| ^
|the type test for List[Int] cannot be checked at runtime because its type arguments can't be determined from Any
|
| longer explanation available when compiling with `-explain`
-- Error: tests/neg/nowarn.scala:33:1 ----------------------------------------------------------------------------------
33 |@nowarn("id=1") def t4d = try 1 // error and warning (unused nowarn, wrong id)
-- Error: tests/neg/nowarn.scala:30:1 ----------------------------------------------------------------------------------
30 |@nowarn("id=1") def t4d = try 1 // error and warning (unused nowarn, wrong id)
|^^^^^^^^^^^^^^^
|@nowarn annotation does not suppress any warnings
-- Error: tests/neg/nowarn.scala:42:1 ----------------------------------------------------------------------------------
42 |@nowarn("msg=fish") def t6d = f // error (unused nowarn), warning (deprecation)
-- Error: tests/neg/nowarn.scala:39:1 ----------------------------------------------------------------------------------
39 |@nowarn("msg=fish") def t6d = f // error (unused nowarn), warning (deprecation)
|^^^^^^^^^^^^^^^^^^^
|@nowarn annotation does not suppress any warnings
-- Error: tests/neg/nowarn.scala:50:5 ----------------------------------------------------------------------------------
50 | : @nowarn("msg=fish") // error (unused nowarn)
-- Error: tests/neg/nowarn.scala:47:5 ----------------------------------------------------------------------------------
47 | : @nowarn("msg=fish") // error (unused nowarn)
| ^^^^^^^^^^^^^^^^^^^
| @nowarn annotation does not suppress any warnings
-- Error: tests/neg/nowarn.scala:62:0 ----------------------------------------------------------------------------------
62 |@nowarn def t9a = { 1: @nowarn; 2 } // error (outer @nowarn is unused)
-- Error: tests/neg/nowarn.scala:59:0 ----------------------------------------------------------------------------------
59 |@nowarn def t9a = { 1: @nowarn; 2 } // error (outer @nowarn is unused)
|^^^^^^^
|@nowarn annotation does not suppress any warnings
-- Error: tests/neg/nowarn.scala:63:27 ---------------------------------------------------------------------------------
63 |@nowarn def t9b = { 1: Int @nowarn; 2 } // error (inner @nowarn is unused, it covers the type, not the expression)
-- Error: tests/neg/nowarn.scala:60:27 ---------------------------------------------------------------------------------
60 |@nowarn def t9b = { 1: Int @nowarn; 2 } // error (inner @nowarn is unused, it covers the type, not the expression)
| ^^^^^^^
| @nowarn annotation does not suppress any warnings
-- Error: tests/neg/nowarn.scala:68:0 ----------------------------------------------------------------------------------
68 |@nowarn @ann(f) def t10b = 0 // error (unused nowarn)
-- Error: tests/neg/nowarn.scala:65:0 ----------------------------------------------------------------------------------
65 |@nowarn @ann(f) def t10b = 0 // error (unused nowarn)
|^^^^^^^
|@nowarn annotation does not suppress any warnings
-- Error: tests/neg/nowarn.scala:69:8 ----------------------------------------------------------------------------------
69 |@ann(f: @nowarn) def t10c = 0 // error (unused nowarn), should be silent
-- Error: tests/neg/nowarn.scala:66:8 ----------------------------------------------------------------------------------
66 |@ann(f: @nowarn) def t10c = 0 // error (unused nowarn), should be silent
| ^^^^^^^
| @nowarn annotation does not suppress any warnings
-- Error: tests/neg/nowarn.scala:72:0 ----------------------------------------------------------------------------------
72 |@nowarn class I1a { // error (unused nowarn)
-- Error: tests/neg/nowarn.scala:69:0 ----------------------------------------------------------------------------------
69 |@nowarn class I1a { // error (unused nowarn)
|^^^^^^^
|@nowarn annotation does not suppress any warnings
-- Error: tests/neg/nowarn.scala:77:0 ----------------------------------------------------------------------------------
77 |@nowarn class I1b { // error (unused nowarn)
-- Error: tests/neg/nowarn.scala:74:0 ----------------------------------------------------------------------------------
74 |@nowarn class I1b { // error (unused nowarn)
|^^^^^^^
|@nowarn annotation does not suppress any warnings
3 changes: 0 additions & 3 deletions tests/neg/nowarn.scala
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,6 @@ def t1a = try 1 // warning (parser)
@nowarn("wat?") // warning (typer, invalid filter)
def t2 = { 1; 2 } // warning (the invalid nowarn doesn't silence anything)

@nowarn(t1a.toString) // warning (typer, argument not a compile-time constant)
def t2a = { 1; 2 } // warning (invalid nowarn doesn't silence)

object o:
final val const = "msg=try"
inline def inl = "msg=try"
Expand Down
5 changes: 4 additions & 1 deletion tests/neg/serialversionuid-not-const.scala
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
@SerialVersionUID(13l.toLong) class C1 extends Serializable // OK because toLong is constant-folded
@SerialVersionUID(13l) class C2 extends Serializable // OK
@SerialVersionUID(13.asInstanceOf[Long]) class C3 extends Serializable // error

//@SerialVersionUID(13.asInstanceOf[Long]) class C3 extends Serializable
//now catched in typer already: not a valid annotation argument

@SerialVersionUID(Test.bippy) class C4 extends Serializable // error

object Test {
Expand Down
2 changes: 1 addition & 1 deletion tests/pos/t1942/A_1.scala → tests/neg/t1942.scala
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,5 @@ class ann(x: Int) extends annotation.StaticAnnotation

class t {
val a = new A
@ann(a.foo(1)) def bar = 1
@ann(a.foo(1)) def bar = 1 // error: not a valid annotation
}
2 changes: 1 addition & 1 deletion tests/pos/t5892.scala → tests/neg/t5892.scala
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
class foo(a: String) extends annotation.StaticAnnotation
object o {
implicit def i2s(i: Int): String = ""
@foo(1: String) def blerg: Unit = { }
@foo(1: String) def blerg: Unit = { } // error: not a valid annotation
}
3 changes: 3 additions & 0 deletions tests/neg/t7426.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
class foo(x: Any) extends annotation.StaticAnnotation

@foo(new AnyRef { }) trait A // error: not a valid annotation
15 changes: 0 additions & 15 deletions tests/pos/i15054.scala

This file was deleted.

2 changes: 0 additions & 2 deletions tests/pos/i7740a.scala

This file was deleted.

2 changes: 0 additions & 2 deletions tests/pos/i7740b.scala

This file was deleted.

3 changes: 0 additions & 3 deletions tests/pos/t1942/Test_2.scala

This file was deleted.

3 changes: 0 additions & 3 deletions tests/pos/t7426.scala

This file was deleted.

2 changes: 1 addition & 1 deletion tests/run/i12656.scala
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ transparent inline def expectTypeCheck(
inline code: String,
) : Boolean = compiletime.testing.typeChecks(code)

@main def Test =
@main def Test: Unit =
assert(!expectTypeCheck("""compiletime.error("some error")"""))
assert(expectTypeCheck("""1 + 1"""))
expectCompileError("""compiletime.error("some error")""", "some error")
Expand Down
2 changes: 1 addition & 1 deletion tests/warn/i15503i.scala
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ package foo.test.i16877:

class ExampleAnnotation(val a: Object) extends StaticAnnotation // OK

@ExampleAnnotation(new HashMap()) // OK
//@ExampleAnnotation(new HashMap()) // Invalid annotation argument
class Test //OK

package foo.test.i16926:
Expand Down

0 comments on commit 7f5eaa8

Please sign in to comment.