Support Maven Plugin annotations in ExcludedFieldAnnotations

[ascopes]

Apache Maven's plugin API runs on top of Plexus and Google Guice via Eclipse Sisu to provide dependency injection.

The org.apache.maven.plugins.annotations.Parameter annotation is a special annotation that shares some semantics with javax.inject.Inject, in that it will be populated by dependency injection.

See https://maven.apache.org/plugin-tools/maven-plugin-annotations/apidocs/org/apache/maven/plugins/annotations/Parameter.html.

Whilst more complex configuration could be provided, at a bare minimum, it is safe to assume that anything annotated with this should be treated as being dynamically initialized if and only if the outer class implements Mojo (or extends a class implementing that interface, such as AbstractMojo).

A rough workaround right now is to specify the class in -XepOpt:NullAway:ExcludedFieldAnnotations=org.apache.maven.plugins.annotations.Parameter, but it would be nice for potential first-class support as described above, as it will allow encouraging the use of null-safety annotations within Maven plugins correctly, which in turn will encourage the development of more robust build tooling in the Java community.

[msridhar]

Thanks for the report @ascopes. I'm guessing there are a lot of conventions like these in different libraries, and I'm not sure how much support we should bake in to NullAway. Making org.apache.maven.plugins.annotations.Parameter an excluded field annotation by default is doable, but might lead to missed bugs. We don't have a built-in notion of "only exclude fields with this annotation if inside some other class"; that would require a Handler to implement. That adds a bit more overhead, and I'm concerned that everyone would be paying that overhead even if they are not working on Maven plugins. We have ways for others to add their own library models to NullAway, and I guess we could extend that to arbitrary Handlers. It's a bit of a worse developer experience (as users would need another dep in their processor path), but maybe a good compromise as we may get more requests like these in the future.

@ascopes do you think making org.apache.maven.plugins.annotations.Parameter an excluded field annotation by default would mask a lot of potential bugs in practice, or is it a reasonable compromise?

[ascopes]

I can't really see it masking many bugs unless a codebase already is putting in measures to avoid handling the annotations sensibly. In concept, this has the same rough effect as @Inject does, just that it gets sourced from Plexus.

It is possibly worth noting this only applies if it is a field though, as it will be late initialised.

[msridhar]

So do you think then that making org.apache.maven.plugins.annotations.Parameter an excluded field annotation by default would be a reasonable solution? I'd be happy to accept a PR for that.

[ascopes]

Yeah, I think it is a reasonable default assumption. I did a brief search for where this was configured but wasn't quite sure where it needed to go. When I have some time, I can have another look.

thanks!

[msridhar]

Here's where you'd need to make a change:

https://github.com/uber/NullAway/blob/844e7a3fb84db722f94396b1842999118002c2d3/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java#L177-L177

Also would be good to have a test. Thanks!