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

Fix AnnotationInfo when using defaults #884

Open
lrytz opened this issue Nov 22, 2024 · 6 comments
Open

Fix AnnotationInfo when using defaults #884

lrytz opened this issue Nov 22, 2024 · 6 comments
Assignees

Comments

@lrytz
Copy link
Member

lrytz commented Nov 22, 2024

scala> class ann(x: Int = 1, y: Int = 2) extends annotation.StaticAnnotation
class ann

scala> @ann(y = Nil.size) class K
        ^
       warning: Usage of named or default arguments transformed this annotation
       constructor call into a block. The corresponding AnnotationInfo
       will contain references to local values and default getters instead
       of the actual argument trees
class K

scala> typeOf[K].typeSymbol.annotations.head
val res0: $r.intp.global.AnnotationInfo = ann(x$2, x$1)

scala> typeOf[K].typeSymbol.annotations.head.original
val res1: $r.intp.global.Tree = <empty>

Before typing, the annotation is represented as new ann(y = Nil.size). To construct the AnnotationInfo, that expression is type checked like ordinary code, resulting in

{
  val x$1 = Nil.size
  val x$2 = ann.<init>$default$1
  new ann(x$2, x$1)
}

The AnnotationInfo only has ann(x$2, x$1), the block is thrown away it seems, there's now way get to the actual arguments.

Ideas

  • Minimum: attach the typed Block AST to the AnnotationInfo. But IMO this is not a good solution, it leaves processing annotations too difficult.
  • Never create blocks in NamesDefaults when typing an annotation. Evaluation order is not a concern, so new ann(y = Nil.size) can be typed as new ann(<init>$default$1, Nil.size)
  • Improve handling of defaults. It would be great if default values defined in the annotation declaration ended up in the AnnotationInfo, i.e., new ann(1, Nil.size).

For the last one, the question is how to get to the default value AST. One option is to make the compiler attach it to to a symbol, so the class ann above would become

class ann(@defaultArg(1) x: Int, @defaultArg(2) y: Int) extends StaticAnnotation
object ann {
  <synthetic> def <init>$default$1 : Int = 1
  <synthetic> def <init>$default$2 : Int = 2
}

When constructing the AnnotationInfo from new ann(<init>$default$1, Nil.size) we can get the AST for the default from the annotation parameter symbol.

Earlier tickets: scala/bug#7656, scala/bug#9612

I did some prototyping when discussing @apiStatus (scala/scala#8820 / scala/scala@2.13.x...lrytz:constAnnDefaults). The goal here was to allow subclasses of annotations to define certain defaults, which would be great for @unused: scala/bug#12367.

Fixing that at the same time would be good.

@som-snytt
Copy link

There was a comment on dotty that they might re-engineer defaults to be inline. Your bullet 3 is very appealing, or to coin a phrase, "it would be great if".

@SethTisue SethTisue self-assigned this Nov 25, 2024
@lrytz
Copy link
Member Author

lrytz commented Nov 26, 2024

A workaround, at least for the case when the annotation parameters have distinct types, is defining constructor overloads:

class ann(x: Int, y: String) extends annotation.StaticAnnotation {
  def this(x: Int) = this(x, null)
  def this(s: String) = this(null, s)
}
scala> @ann(s = "ka") class K

scala> typeOf[K].typeSymbol.annotations.head
val res0: $r.intp.global.AnnotationInfo = ann("ka")

@lrytz
Copy link
Member Author

lrytz commented Dec 6, 2024

Another item to fix - though it will be less relevant after the other fixes. For an annotation with auxiliary constructors, the AnnotationInfo doesn't tell which overload was selected. Probably just add a constructorSymbol to the AnnotationInfo.

@lrytz
Copy link
Member Author

lrytz commented Dec 6, 2024

... sometimes the simple ideas are not that simple.

https://github.com/scala/scala/compare/2.13.x...lrytz:scala:annot-overload?expand=1 adds the constructor symbol to AnnotationInfo, but

  • we cannot pickle / unpickle a reference to a method / constructor symbol, at least not easily (would need overload resolution in unpickling)
  • we cannot change the pickle format

What the user really needs is to match up the args: List[Tree] with the corresponding parameter names

  • we could include the parameter names, we could even re-use the assocs: List[(Name, ClassfileAnnotArg)] for that which we could pickle. Though assert(args.isEmpty || assocs.isEmpty, atp) in CompleteAnnotationInfo would fail when an older Scala unpickles...
  • add a method that type-checks a new Ann(args: _*) AST and extracts the resolved constructor symbol from there
  • attach the parameter names to the args ASTs as annotations somehow

@SethTisue
Copy link
Member

chatted with Lukas about this today. he clarified that he doesn't think we should continue down the path of attempting to pickle a method/constructor symbol

and he pointed out that if we fix this, it would make it possible for users to e.g. have a custom subclass of @nowarn that fixes some of the parameters. this would be useful if people are doing the same @nowarn invocations over and over again

@lrytz
Copy link
Member Author

lrytz commented Dec 10, 2024

One option would be to add this method to AnnotationInfo:

    def constructorSymbol(typer: Tree => Tree): Symbol = {
      typer(New(atp, args: _*)) match {
        case Apply(constr @ Select(New(_), nme.CONSTRUCTOR), _) => constr.symbol
        case _ => atp.typeSymbol.primaryConstructor
      }
    }

The users have to provide a type checker, so basically call it as annot.constructorSymbol(exitingTyper(typer.typed(_))). The reason is that AnnotationInfo lives in the SymbolTable where we don't have access to Typer. Compiler plugins that work with AnnotationInfo have a typer available.

I tested it, it works well. That seems to be the most straightforward solution.

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

No branches or pull requests

3 participants