Make MultiTargetAnnotation type-parameterized and fix some comments

[jwright6323]

This PR adds a type parameter to MultiTargetAnnotation (#1109) to allow concrete subclasses to have stricter type checking of targets, if desired. I also cleaned up a few comment nits.

Checklist

• Did you specify the type of improvement?
• Did you state the API impact?
• Did you specify the code generation impact?
• Did you request a desired merge strategy?

Type of Improvement

• code refactoring
• new feature/API

API Impact

API modification, but you can use the old behavior by using Target as type parameter T. This allows stricter type checking of subclasses of MultiTargetAnnotation if desired.

Backend Code Generation Impact

No change

Desired Merge Strategy

• Squash: The PR will be squashed and merged (choose this if you have no preference.

[CLAassistant]

All committers have signed the CLA.

[albert-magyar]

One thing to point out is that this does have an API impact, since people would have to change code that extends MultiTargetAnnotation. That should be fine for 1.3 (master), though, especially since it's new and doesn't appear in the repo.

[albert-magyar]

This might give a cryptic error if a rename violates the type constraints.

[jwright6323]

Any suggested change?

[albert-magyar]

I can think about non-intrusive ways to hide it from the user, but the TLDR is that casts to erased types can lead to class case exceptions in the caller's code. The normal Scala way to solve this is with ClassTags, but this is complicated for a trait and can clutter downstream code.

If someone uses a RenameMap that has a record that renames a ReferenceTarget to an InstanceTarget or something, it would produce some crazy results.

[jwright6323]

One thing to point out is that this does have an API impact, since people would have to change code that extends MultiTargetAnnotation. That should be fine for 1.3 (master), though, especially since it's new and doesn't appear in the repo.

@albert-magyar wouldn't it infer the type parameter to be Target?

Edit: never mind, I get what you are saying now.

[albert-magyar]

@albert-magyar wouldn't it infer the type parameter to be Target?

Unfortunately, it won't generally infer the type parameter for the trait, since Scala only has local type inference. Not saying that it's a problem (I personally don't think it is), but just clarifying for the sake of the issue template classification text.

[jwright6323]

@albert-magyar Cool, I edited the classification in the first comment.

[albert-magyar]

I'll preface this by saying that I don't think this is a showstopper by any means, but more of a heads-up about the potential holes in the type safety of this parameter.

As a concrete example of renames that do happen that could lead to type errors, Inline adds a rename record mapping from an InstanceTarget to a ModuleTarget. This would be unchecked within the update method of a MultiTargetAnnoation[InstanceTarget] that previously included an inlined instance.

The annoying part is that, type erasure can cause the ClassCastException to appear at some distant, indefinite point in the compiler, since the cast actually occurs outside the generic! The default solution is to use a ClassTag in the scope of the method, which allows type-casing code to actually be checked at runtime.

renames(target).map {
  case t: T => t.asInstanceOf[T]  // needs ClassTag to actually be checked!
  case _ => // handle error
}

Unfortunately, using ClassTags generally requires peppering your code with implicit values. A lot of the ways to reduce the associated boilerplate of adding these implicits are hampered by the fact that the API is already inherited from Annotation, which lacks type parameters, and by the fact that MultiTargetAnnotation is a trait.

[jwright6323]

@albert-magyar What would happen if you used a SingleTargetAnnotation[InstanceTarget] on an Inline module? Is there a reason that would work and this would not, or is this problem endemic to both annotations?

[albert-magyar]

SingleTargetAnnotation has a specific check for illegal renames.

It's generally pretty ugly, but the TLDR is that it does a duplicate call with the actual returned target inside update to provoke the cast to happen at that point in time, allowing the exception to be contained within the scope of update. Also, it more-or-less depends on the user extending SingleTargetAnnotation with a non-generic subtype (i.e. X extends SingleTargetAnnotation[ModuleTarget]) to work effectively.

The problem is that the contents of a MultiTargetAnnotation are not T, and not even a generic on T, but a generic on a generic on T, which means that similar code would be much more complex, and would not be able to rely on a simple duplicate being implemented by inheriting classes, even if they themselves are not generic.

Needs more thought, see Albert's comments