Validate annotations that use @RequiredModifiers or @IncompatibleModifiers

[cushon]

Original issue created by kevinb@google.com on 2013-02-25 at 06:59 PM

---

Some annotations are misused when applied to the wrong "kinds" of program elements; this is why annotations can be restricted to only classes, or only methods, etc. But some annotations only make sense for certain kinds of methods, for example based on modifiers.

We use two experimental annotations called @RequiredModifiers and @IncompatibleModifiers. For example, if a particular annotation only makes sense to be applied to a static method, we annotate that annotation with @RequiredModifiers(STATIC). If it only makes sense for an overrideable method, we use @IncompatibleModifiers({PRIVATE, STATIC, FINAL}), etc.

If error-prone could enforce these attributes, it can help the user annotations in question to be used correctly more often and with less head-scratching later.

I am not necessarily claiming this should be high priority or huge value, but I wanted to capture the request. We certainly wouldn't want to proceed with open-sourcing our custom home-brewed checker.

[cushon]

Original comment posted by kevinb@google.com on 2013-02-25 at 07:00 PM

---

(If you can cc: ben yu to this please...)

[cushon]

Original comment posted by eaftan@google.com on 2013-02-25 at 07:03 PM

---

(No comment entered for this change.)

---

CC: benyu@google.com

[cushon]

Original comment posted by eaftan@google.com on 2013-02-26 at 05:09 AM

---

+plumpy

Michael was also interested in writing this check for error-prone. I looked at it briefly and send him some advice, which would also be good to attach to this bug:

---

Hi Michael,

I think it is doable with some javac trickery.

For #1, now you can use Matchers.hasAnnotation("com.google.common.annotations.IncompatibleModifiers"). I think you need to start from the guice_refactoring_2 branch for that matcher to work on annotations themselves.

For #2, I believe there's are two ways to do it. One way is the way we did in our error-prone AnnotationHasArgumentWithValue matcher, by basically looking through the parse tree. The other way is to get the symbol from the annotation, and call getAnnotationMirrors() on it, which will give you a List<javax.lang.model.element.AnnotationMirror>. You can access the elements of an AnnotationMirror with .getElementValues(). (this is how a normal annotation processor would work)

For #3, I think calling .getModifiers() on the tree would give you that information.

Hopefully this is enough for you to experiment with. Feel free to ping me for more help. :)

-Eddie

On Fri, Feb 22, 2013 at 2:05 PM, Michael Plump wrote:
Hi!,

I'm finally getting back to this, but I'm worried that what I want to do is impossible. Here's what I need to do:

1. Given some AnnotationTree (say @Transactional) in the parse tree, see if that annotation is itself annotated with @IncompatibleModifiers
2. get the list of incompatible modifiers (as arguments to @IncompatibleModifiers) for @Transactional
3. see if the Tree to which @Transactional is being applied contains any of these modifiers

But I suspect it's not possible to do this with just a parse tree because information about Transactional.class isn't available at this phase of compilation. Is that correct?

---

CC: plumpy@google.com

[cushon]

Original comment posted by sgoldfed on 2013-06-25 at 07:22 PM

---

Hi everyone,

I spoke with Kevin about this check. Was there any progress with this? Michael, are you working on it? If not, I'd be happy to take this over.

Thanks!

Steven

[cushon]

Original comment posted by plumpy@google.com on 2013-06-25 at 07:40 PM

---

Honestly, I'm not proud to admit it, but I got sort of confused and gave up. I kept meaning to come back to it, but never did and I definitely don't have any time for a few weeks. So yeah, feel free to take it up. Thanks!

[cushon]

Original comment posted by eaftan@google.com on 2014-01-30 at 12:15 AM

---

This has been implemented for Google-internal error-prone, since the @RequiredModifiers and @IncompatibleModifiers annotations are Google internal. Kevin, would you guys be interested in open sourcing those annotations since we have a check now? In that case we could move the checker to external error-prone as well.

---

Status: Started

[cushon]

Original comment posted by kevinb@google.com on 2014-01-30 at 12:41 AM

---

In theory yes, but we feel like continuing to add more of these annotations to Guava is the wrong idea. There needs to be a better place for them, project-wise, maybe even package-wise? Any ideas?

[cushon]

Original comment posted by eaftan@google.com on 2014-01-30 at 07:05 PM

---

This is a tough question. I can think of a few options:

1. We add the annotations to error-prone, in a separate Maven project. It would be clear they are checked by error-prone, and we can define clear semantics for them. Other static analysis tools like FindBugs, the Checker Framework, and IntelliJ do this, but they also typically check similar annotations from other packages (e.g. the Checker Framework understands javax.annotation.Nullable as well as checkers.quals.Nullable).

Note that, at least inside Google, the non-tool-specific annotations are much more widely used than the tool-specific ones. My guess is that people don't want to tie their code to one specific checker implementation, especially when many of these annotations have value as documentation.

2. We could create a new project with a set of useful Java annotations. The annotations would not be tightly specified, but it would be clear they can be used without error-prone. I'm thinking of something roughly like the JSR305 annotations, but including other useful annotations that we don't have checkers for yet.

In this case, turning on a check for an existing annotation is hard, since people will have been using them without any mechanized checking.

We can chat about this in person, but I wanted to get my thoughts down here while they are fresh in my mind.

[cushon]

Original comment posted by kevinb@google.com on 2014-01-30 at 08:51 PM

---

First, no matter what, I feel strongly that these annotations should have clear specifications for what exactly their usage is meant to indicate.

I also would suggest that error-prone recognize only this specific set by default, but have an option to provide a mapping somewhere? Handwave.

The goal is that anyone coming across these annotations in code is one click away from having their questions answered about what that annotation is doing.

(We did just make the opposite decision for AutoValue's recognition of @Nullable, because we feel the meaning of how the presence of "@Nullable" should be interpreted is sufficiently clear and unambiguous, plus we know it has an especially large number of different incarnations already, unlike most every other annotation we're talking about here.)

[cushon]

Original comment posted by kevinb@google.com on 2014-01-30 at 08:56 PM

---

As for project location: I think part of this project in a separate artifact seems fine.

As for package location: the simple choice is com.google.errorprone.annotations. Some users might feel hesitant to explicitly tie their code to errorprone or even Google, but (a) they could use the option mentioned above, and (b) maybe in the future there will be enough interest/motivation in rebuilding a new community effort to standardize and that's the time when a non-Google package location would be called for.

(Incidentally if it's possible to move the three annotations in com.google.errorprone somewhere else, such as to the same tree and package where the other classes that implementors need to import and use are, that would be nice.)

[cushon]

We're in the process of creating a separate error-prone package for annotations, so we should be able to open-source the checks soon.

[Stephan202]

@cushon, is there a specific ticket that tracks the work on this annotation package? I assume such a package will contain the annotations mentioned in this issue and in #276 / #279.

If possible, I'd like for you to consider another annotation, namely @EverythingIsNonnullByDefault as defined in this SO answer or variations thereof, such as @ReturnTypesAreNonnullByDefault and @FieldsAreNonnullByDefault, as defined in this SO answer. It would do away with the need for everybody to re-implement/redefine these annotations themselves. (I used these in several internal projects, and based on the SO answers it's not uncommon for other to do, too.)

If you think another project would be more suited to provide such annotations (the Checker Framework, perhaps?), let me know. If desired I can open a separate issue for this.

[cushon]

@Stephan202 - initially the package is only going to contain annotations that error-prone can enforce the semantics of. Error-prone doesn't currently check nullness annotations, so we probably won't add the annotations you suggest until we can actually analyze them. I created #280 to track your suggestion.

The Checker Framework might be a better home, but their analysis already assumes fields, parameters, and return types are non-null.

[cushon]

Fixed in 792f43f