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

Do not lift annotation arguments #22035

Merged
merged 1 commit into from
Nov 27, 2024
Merged

Do not lift annotation arguments #22035

merged 1 commit into from
Nov 27, 2024

Conversation

mbovel
Copy link
Member

@mbovel mbovel commented Nov 26, 2024

When typing

class Fork(value: Int = -1, jvmArgs: Array[String] = Array("nope")) extends annotation.Annotation
@Fork(jvmArgs = Array("I'm", "hot")) class Test() extends Object() {}

the annotation arguments are currently lifted, so that the annotation tree of @Fork(jvmArgs = Array("I'm", "hot")) becomes:

{
  val jvmArgs$1: Array[String] = Array.apply[String](["I\'m","hot" : String]*)(scala.reflect.ClassTag.apply[String](classOf[String]))
  val value$1: Int @uncheckedVariance = Fork.$lessinit$greater$default$1
  new Fork(value$1, jvmArgs = jvmArgs$1)
}

This breaks the assumption that annotation trees are always of the form new annot(…).

It's currently worked around in allTermArguments where we ignore block statements:

def allTermArguments(tree: Tree): List[Tree] = unsplice(tree) match {
case Apply(fn, args) => allTermArguments(fn) ::: args
case TypeApply(fn, args) => allTermArguments(fn)
case Block(_, expr) => allTermArguments(expr)
case _ => Nil

This however falls short in different situations; as this completely ignore argument trees.


To fix this, I propose to not lift argument that are annotation arguments.

There is precedent for this: we actually already do it for Java annotations:

protected def needLiftFun: Boolean = {
def requiredArgNum(tp: Type): Int = tp.widen match {
case funType: MethodType =>
val paramInfos = funType.paramInfos
val argsNum = paramInfos.size
if (argsNum >= 1 && paramInfos.last.isRepeatedParam)
// Repeated arguments do not count as required arguments
argsNum - 1
else
argsNum
case funType: PolyType => requiredArgNum(funType.resultType)
case tp => args.size
}
!isJavaAnnotConstr(methRef.symbol) &&
args.size < requiredArgNum(funType)
}

@mbovel
Copy link
Member Author

mbovel commented Nov 27, 2024

Oops, I created the branch mb/annots/default-args on this repo, hence the duplicate CI jobs, my mistake, I meant to push it on my fork. At least we'll be doubly-sure that it works 😄

@mbovel mbovel requested a review from smarter November 27, 2024 16:10
@mbovel mbovel marked this pull request as ready for review November 27, 2024 16:10
@mbovel mbovel merged commit bcacaee into main Nov 27, 2024
52 checks passed
@mbovel mbovel deleted the mb/annots/default-args branch November 27, 2024 17:50
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

Successfully merging this pull request may close these issues.

2 participants