Var/LazyInit/etc annotations can cause problems for Android builds due to reliance on javax.lang.model.element.Modifier

[lazaroclapp]

Description of the problem / feature request:

Multiple annotations inside com.google.errorprone:error_prone_annotations use the meta-annotation @IncompatibleModifiers(FINAL).

Here, FINAL references javax.lang.model.element.Modifier.FINAL. However, the android.jar file used to build Android projects does not include javax.lang.model.element.Modifier.

This results in the following error whenever code using @Var, @LazyInit, @ForOverride, etc. is compiled for Android with a -Werror flag (our default setup):

warning: unknown enum constant Modifier.FINAL
  reason: class file for javax.lang.model.element.Modifier not found
error: warnings found and -Werror specified
1 error
1 warning

For an example, here is a simple repro as a standalone Android project using gradle (see below for instructions).

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

git clone https://github.com/lazaroclapp/ep_modifiers_android_repro.git
gradle build

Observe:

warning: unknown enum constant Modifier.FINAL
  reason: class file for javax.lang.model.element.Modifier not found
error: warnings found and -Werror specified
1 error
1 warning

What version of Error Prone are you using?

Can repro on 2.5.1, 2.4, and 2.3.3

Have you found anything relevant by searching the web?

I posted on the mailing list and got prompt help from Liam figuring out the root cause of the issue. Thanks!

My recommended solution

The real problem here is @IncompatibleModifiers and its use of javax.lang.model.element.Modifier's enum values as annotation parameters.

Could com.google.errorprone:error_prone_annotations perhaps include a separate Modifier enum that doesn't depend on javax.lang.model.element.Modifier, and use that for the public API of the @IncompatibleModifiers meta-annotation and for annotating the relevant Error Prone annotations?

This will likely complicate IncompatibleModifiersChecker a bit, or require a mapping from the new custom Modifier enum to javax.lang.model.element.Modifier, but it will make the annotation fully compatible with Android builds.

FYI, if you agree that supporting Android with -Werror is important, and that the above is a reasonable solution, I'd be more than happy to give it a first try at a PR that replaces javax.lang.model.element.Modifier with e.g. com.google.errorprone.annotations.Modifier.

[cushon]

Thanks for the write-up and the suggestions. I agree that, in hindsight, using a different representation of the modifiers would have been a good choice, since javax.lang.model.element.Modifier isn't available on Android.

At this point, my concern is that would be a breaking change to the API of error_prone_annotations. Even if we support both versions of the annotation in the check, users of error_prone_annotations (i.e. who are using @IncompatibleModifiers in their own code) would be broken the next time they upgrade that dependency. It's not a difficult change to make, and I don't think @IncompatibleModifiers has that many uses compared to other annotations in the artifact, but it's still something we'd want to be cautious about.

There are ways to do the change incrementally (like introducing the new way to specify modifiers in parallel with the current one, deprecate the current one, and wait a while before removing it). We could also introduce a separate Android-compatible version of the annotations artifact.

A few other stray thoughts: it would be nice if those javac diagnostics could be controlled by a flag (like -Xlint:-classfile). it would also be nice if there was a finer-grained version of -Werror that allowed opting in to individual javac diagnostics as errors, instead of everything. (Bazel supports something like that, but it isn't supported directly in javac.)

Finally, you mentioned in the thread

Short term, we could possibly fix this by taking that enum into its own jar and adding it as a dependency, then removing it (and the annotations using it) before dexing.

We're actually doing something similar to that, which is why we hadn't noticed this issue in our own code. As long as the dep is removed before dexing, and the annotations referencing the enum are CLASS retention and don't show up in dex anyways, it has been working fine for us.

[lazaroclapp]

There are ways to do the change incrementally (like introducing the new way to specify modifiers in parallel with the current one, deprecate the current one, and wait a while before removing it). We could also introduce a separate Android-compatible version of the annotations artifact.

This would be quite helpful. Something as simple as having a javax.[...].Modifier and com.google.errorprone.annotations.Modifier version of the @IncompatibleModifiers annotation (I believe it would need to have a different name), and using the second for all error_prone_annotations first-party annotations would probably solve the problem for us. Not sure if just having a class referencing Modifier is an issue, but my belief is that the compiler doesn't care at all until it tries to complete the annotation.

The above could theoretically break builds which use one version of the EP checkers and a different version of the EP annotations artifact (e.g. 2.5.1 core and 2.5.2 annotations could result in @Var's compatibility with final not being checked), but I've always assumed that's not a supported configuration to begin with...

A few other stray thoughts: it would be nice if those javac diagnostics could be controlled by a flag (like -Xlint:-classfile). it would also be nice if there was a finer-grained version of -Werror that allowed opting in to individual javac diagnostics as errors, instead of everything.

-Xlint:-xyz does that for a number of warnings, but afaik not for this one. Also, even then, disabling this "unknown enum constant" warning in the general case is probably not what we want either. We'd need something targeted enough to tell it to not warn about a specific enum class.

We're actually doing something similar to that, which is why we hadn't noticed this issue in our own code. As long as the dep is removed before dexing, and the annotations referencing the enum are CLASS retention and don't show up in dex anyways, it has been working fine for us.

So, @Var for example is RUNTIME, but then the @IncompatibleModifiers annotation itself has CLASS retention for the time being, which means the above would work. Annoyingly, though, Modifiers is GPLv2 with the classpath exception, so if we are pulling it in, even just for building, we'll probably need to end up adding safeguards to make sure it never ever gets included into the final application bundle.

Given that, my current thinking as to internal workarounds has evolved to favor forking error_prone_annotations and either doing my proposed patch or just removing the definition and usages of @IncompatibleModifiers, since what we really want to use is @Var and other annotations that use @IncompatibleModifiers as a meta-annotation.

That said, we'd love to have a good way to keep to mainline EP without an internal fork (have done that before, not a huge pain, but not the best either).

[cushon]

Future versions of Android may include java/lang/annotation/*, but there aren't plans for javax.lang.model.element.Modifier. (That would be ideal from my perspective since it avoids the change to the EP version, but even if it happened it'd be a while before we could assume all of our users were on a new enough version of Android to benefit from it...)

We could think about doing something like:

public @interface IncompatibleModifiers {
  /** @deprecated use {@link #modifier} instead. */
  @Deprecated
  javax.lang.model.element.Modifier[] value();
  com.google.errorprone.annotations.Modifier[] modifier();
}

And waiting a few releases, and then deleting value. We could also consider a multi-step migration to eventually make value just refer to the new enum:

public @interface IncompatibleModifiers {
  /** @deprecated use {@link #value} instead. */
  @Deprecated
  com.google.errorprone.annotations.Modifier[] modifier();
  com.google.errorprone.annotations.Modifier[] value();
}

If I had a time machine to use a different Modifier enum in the first place, I would do that. At this point it's enough hassle to fix that I'm not sure this is a super high priority for us, especially since it's at least one breaking API change, and there are work-arounds.

If the change I described happened, do you have any input on how quickly you'd want to see support for the old element be removed? I'm just curious if you have opinions about the rate of breaking changes in the API, we don't get a lot of feedback on that generally.

Given that, my current thinking as to internal workarounds has evolved to favor forking error_prone_annotations and either doing my proposed patch or just removing the definition and usages of @IncompatibleModifiers, since what we really want to use is @Var and other annotations that use @IncompatibleModifiers as a meta-annotation.

That makes sense to me in the short term, FWIW. I'd like to give you an alternative using mainline EP, but also can't promise we're going to have that alternative in the immediate future.

[lazaroclapp]

So, I think - but I am not sure - that this might just work immediately for us:

public @interface IncompatibleModifiers {
  /** @deprecated use {@link #modifier} instead. */
  @Deprecated
  javax.lang.model.element.Modifier[] value();
  com.google.errorprone.annotations.Modifier[] modifier();
}

But only if, at the same time the modifier() 'field' is introduced, all usages of @IncompatibleModifiers within Error Prone first party annotations switch to using the modifier = version.

I think it might be worth testing if that would be enough to prevent the compiler from trying to complete e.g. Modifier.FINAL when analyzing @Var, or if just the act of trying to load @IncompatibleModifiers will surface the same problem.

I might do a quick internal experiment on this. If that works, we might even be fine with value() lingering there forever for third-party Error Prone users (although I imagine everyone will be happier if there is a path to having a single way of doing things there eventually, anyways).

I understand this is not super high priority on your end, but would you guys consider a PR implementing the solution you detailed above if that fixed the issue for us?

Given that, my current thinking as to internal workarounds has evolved to favor forking error_prone_annotations and either doing my proposed patch or just removing the definition and usages of @IncompatibleModifiers, since what we really want to use is @var and other annotations that use @IncompatibleModifiers as a meta-annotation.

That makes sense to me in the short term, FWIW

We are currently doing this, and it does mitigate the problem for us. But we'd love to be able to go back to not forking any part of Error Prone and just being on mainline! (Plus, I suspect we aren't the only ones using EP for Android outside of Google...)

---

I'm just curious if you have opinions about the rate of breaking changes in the API, we don't get a lot of feedback on that generally.

So, this is separate from the issue at hand, but, since you ask...

We did run into an issue recently where to get NullAway to work with Error Prone 2.5.1, we had to drop compatibility with 2.3.x (uber/NullAway#447). So far, that hasn't been too painful a decision, to be honest. Internally, we are currently on Error Prone 2.4.0. Would have been more complicated if we were lagging further behind, though, and we might yet hear of some NullAway third-party user who has issues upgrading to 0.9.0 because of it.

As for why we aren't on 2.5.1 yet, we build massive monorepos with -Werror, so EP upgrades are unsurprisingly painful :)

However, the main reasons are usually not any changes to core or the APIs (we modify our own checkers to match whichever EP version we are on and that's often <1hr for all of them at worst), or new checkers being introduced (we can always set them to :OFF on the initial code change and then enable them independently).

The true upgrade pain points for us are:

a) dependency changes where upgrading Error Prone can easily involve a cascading upgrade of core deps for us (e.g. Guava), and

b) existing checkers that have been updated to be more precise.

The later is often a trade-off between adding a dozen to a few hundred suppressions or setting to :OFF a checker which we were running before (albeit possibly in a less comprehensive version). For 2.4.0, the javadoc checkers were notorious for us here.

The first issue, (a), just seems like a fact of life to me. Not sure (b) has an easy solution either, but at least in theory could be avoided if big changes to checkers resulted to checkers or in a world where you could say something like -Xep:MissingSummary:WARN:3.3.4 while keeping all other checkers on 2.4.0 by default and upgrading that one as a follow up 😉

We can simulate the later by forking a checker internally and renaming, but this is rarely worth it as an intermediate/mitigation step.

Just to be clear, though, I am not asking you to address any of this stuff. I am more concerned by the original issue. But you asked for feedback on breaking changes and EP versions, hence the brain-dump.

[ZacSweers]

FYI - R8 gives much more detailed warnings about this in AGP 7.0+

> Task :sample:minifyExternalStagingAndroidTestWithR8
WARNING:Missing classes detected while running R8. Please add the missing classes or apply additional keep rules that are generated in /Users/zsweers/dev/slack/keeper/sample/build/outputs/mapping/externalStagingAndroidTest/missing_rules.txt.

WARNING:R8: Missing class androidx.annotation.experimental.Experimental$Level (referenced from: androidx.test.services.storage.ExperimentalTestStorage)
Missing class androidx.annotation.experimental.Experimental (referenced from: androidx.test.services.storage.ExperimentalTestStorage)
Missing class java.awt.Dimension (referenced from: int org.checkerframework.checker.signedness.SignednessUtilExtra.dimensionUnsignedHeight(java.awt.Dimension) and 1 other context)
Missing class java.awt.image.BufferedImage (referenced from: int[] org.checkerframework.checker.signedness.SignednessUtilExtra.getUnsignedRGB(java.awt.image.BufferedImage, int, int, int, int, int[], int, int) and 1 other context)
Missing class javax.lang.model.element.Modifier (referenced from: javax.lang.model.element.Modifier[] com.google.errorprone.annotations.IncompatibleModifiers.value() and 4 other contexts)

[cushon]

I went ahead and deprecated {Required,Incompatible}Modifiers.value, and migrated uses within core Error Prone to a new modifier enum in 26f1f54. I hope to remove the deprecated attributes eventually, but hopefully there's no rush for that.

[lazaroclapp]

Really appreciated, I think with this, we should be able to stop forking the annotations artifact after the next EP release! (We shouldn't really care about the deprecated API so long as it's not being used within EP itself)