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.

Sign up for GitHub

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

Lift arguments of explicitly constructed annotations #22553

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+132 −14
Open

Lift arguments of explicitly constructed annotations #22553

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5f8ae12
Lift arguments of explicitly constructed annotations
mbovel Feb 7, 2025
337856c
Handle nested explicitly constructed annotations
mbovel Feb 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions compiler/src/dotty/tools/dotc/core/Mode.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,9 @@ object Mode {
*/
val ForceInline: Mode = newMode(29, "ForceInline")

/** Are we typing an annotation? */
val InAnnotation: Mode = newMode(30, "InAnnotation")

/** Skip inlining of methods. */
val NoInline: Mode = newMode(31, "NoInline")
}
13 changes: 7 additions & 6 deletions compiler/src/dotty/tools/dotc/typer/Applications.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -694,9 +694,12 @@ trait Applications extends Compatibility {
sym.is(JavaDefined) && sym.isConstructor && sym.owner.is(JavaAnnotation)


/** Is `sym` a constructor of an annotation? */
def isAnnotConstr(sym: Symbol): Boolean =
sym.isConstructor && sym.owner.isAnnotation
/** Is `sym` a constructor of an annotation class, and are we in an
* annotation? If so, we don't lift arguments.
* See #22035, #22526 and `dependent-annot-default-args.scala`.
*/
protected final def isAnnotConstr(sym: Symbol): Boolean =
ctx.mode.is(Mode.InAnnotation) && sym.isConstructor && sym.owner.isAnnotation

/** Match re-ordered arguments against formal parameters
* @param n The position of the first parameter in formals in `methType`.
Expand Down Expand Up @@ -994,9 +997,7 @@ trait Applications extends Compatibility {
case (arg: NamedArg, _) => arg
case (arg, name) => NamedArg(name, arg)
}
else if isAnnotConstr(methRef.symbol) then
typedArgs
else if !sameSeq(args, orderedArgs) && !typedArgs.forall(isSafeArg) then
else if !isAnnotConstr(methRef.symbol) && !sameSeq(args, orderedArgs) && !typedArgs.forall(isSafeArg) then
// need to lift arguments to maintain evaluation order in the
// presence of argument reorderings.
// (never do this for Java annotation constructors, hence the 'else if')
Expand Down
9 changes: 6 additions & 3 deletions compiler/src/dotty/tools/dotc/typer/Typer.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2779,7 +2779,7 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
def isInner(owner: Symbol) = owner == sym || sym.is(Param) && owner == sym.owner
val outer = ctx.outersIterator.dropWhile(c => isInner(c.owner)).next()
def local: FreshContext = outer.fresh.setOwner(newLocalDummy(sym.owner))
sym.owner.infoOrCompleter match
val ctx0 = sym.owner.infoOrCompleter match
case completer: Namer#Completer
if sym.is(Param) && completer.completerTypeParams(sym).nonEmpty =>
// Create a new local context with a dummy owner and a scope containing the
Expand All @@ -2788,6 +2788,7 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
local.setScope(newScopeWith(completer.completerTypeParams(sym)*))
case _ =>
if outer.owner.isClass then local else outer
ctx0.addMode(Mode.InAnnotation)

def completeAnnotations(mdef: untpd.MemberDef, sym: Symbol)(using Context): Unit = {
// necessary to force annotation trees to be computed.
Expand All @@ -2802,7 +2803,8 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
}

def typedAnnotation(annot: untpd.Tree)(using Context): Tree =
checkAnnotClass(checkAnnotArgs(typed(annot)))
val typedAnnot = withMode(Mode.InAnnotation)(typed(annot))
checkAnnotClass(checkAnnotArgs(typedAnnot))

def registerNowarn(tree: Tree, mdef: untpd.Tree)(using Context): Unit =
val annot = Annotations.Annotation(tree)
Expand Down Expand Up @@ -3335,7 +3337,8 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
end typedPackageDef

def typedAnnotated(tree: untpd.Annotated, pt: Type)(using Context): Tree = {
val annot1 = checkAnnotClass(typedExpr(tree.annot))
val annot0 = withMode(Mode.InAnnotation)(typedExpr(tree.annot))
val annot1 = checkAnnotClass(annot0)
val annotCls = Annotations.annotClass(annot1)
if annotCls == defn.NowarnAnnot then
registerNowarn(annot1, tree)
Expand Down
6 changes: 6 additions & 0 deletions tests/printing/dependent-annot-default-args.check
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,12 @@ package <empty> {
@annot2(
y = Array.apply[Any](["Hello",y : Any]*)(scala.reflect.ClassTag.Any))
val z4: Int = 45
val z5: annot2 =
{
val y$1: Array[Any] =
Array.apply[Any](["World" : Any]*)(scala.reflect.ClassTag.Any)
new annot2(x = 1, y = y$1)
}
()
}
}
Expand Down
3 changes: 3 additions & 0 deletions tests/printing/dependent-annot-default-args.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,6 @@ def test =
@annot(44) val z3 = 45
@annot2(y = Array("Hello", y)) val z4 = 45

// Arguments are still lifted if the annotation class is instantiated
// explicitly. See #22526.
val z5 = new annot2(y = Array("World"), x = 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens on @annot(y = new A, x = new annot(y = new B, x = new C))?

I think in principle, the AST should be

new annot(
  x = { val y$1 = new B; val x$1 = new C; new annot(x = x$1, y = y$1) }
  y = new A)

Copy link
Member Author

@mbovel mbovel Feb 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have just pushed a commit that addresses that.

I can agree with the principle in this case. But in general, it should be noted that trees in annotation arguments are currently not guaranteed to be valid by any definition. We should spec it.