Enable/disable patterns by category

[Stephan202]

With Error Prone 2.0.15 and -XepAllDisabledChecksAsWarnings, the Android-specific StaticOrDefaultInterfaceMethod bug pattern is enabled. For a non-Android code base one can of course easily disable the pattern again with -Xep:StaticOrDefaultInterfaceMethod:OFF. But it did make me wonder: is there a flag to enable or disable all bug patterns in a given category?

I checked the documentation and the source code, but it seems there currently isn't. I think this would be a good feature to have. As the aforementioned bug pattern shows, Android-specific recommendations may be at odds with Java 8 SE best practises.

[JakeWharton]

That check is useful on Android for minSdk 25, so you can't just blindly enable/disable based on platform.

On Sat, Dec 3, 2016, 6:09 AM Stephan Schroevers [email protected] wrote:

With Error Prone 2.0.15 and -XepAllDisabledChecksAsWarnings, the Android-specific StaticOrDefaultInterfaceMethod bug pattern is enabled. For a non-Android code base one can of course easily disable the pattern again with -Xep:StaticOrDefaultInterfaceMethod:OFF. But it did make me wonder: is there a flag to enable or disable all bug patterns in a given category?

I checked the documentation http://errorprone.info/docs/flags and the source code, but it seems there currently isn't. I think this would be a good feature to have. As the aforementioned bug pattern shows, Android-specific recommendations may be at odds with Java 8 SE best practises.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/google/error-prone/issues/488, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEEEfyFlsxkRcjAFKLhyLJoI8fmuBbhks5rEU3egaJpZM4LDRAm .

[Stephan202]

Ah, I'm not suggesting to make this an automated procedure. Android was an example, but {dis,en}abling all Dagger or Guice checks could be just as relevant.

[Stephan202]

Just noticed that #1227 will remove category support (see b5adf02cae0edd283921227d42ade3427124d5d9). I guess this issue should now be rephrased as Enable/disable patterns by tag :).

[commonquail]

#1942 looks like a variant of this.

Error Prone 2.36.0 has over 600 bugpatterns, and although Error Prone is fast, if you only need a few of those bugpatterns you can incur a noticeable compilation time penalty. In my own ongoing management of my Error Prone integrations, because I choose something more complex than the defaults, this is the biggest recurring source of (a little bit of) friction. The greater issue is not the size of the configuration but the necessity of choosing between 1) spending time on irrelevant checks, or 2) considering every single check. If I don't program for Android I don't really want to spend any time thinking about Android, and if I want to follow Google's Java style guide I'd generally like to follow all of it.

I wondered what something like this could feel like so I made a couple of sample implementations:

• https://github.com/google/error-prone/compare/master...commonquail:error-prone:toggle-tag-in-order
• https://github.com/google/error-prone/compare/master...commonquail:error-prone:toggle-tag-by-type

Both implementations add an -XepTag option with the same structure as the existing -Xep, except taking the name of a tag instead of a check. While this does work, it reveals several difficulties:

• Defining new options is easy but by no means gratis.
• Because tags cannot be interacted with today, they are minimally useful, and seemingly inconsistently and arbitrarily applied. A change like this would elevate tags to the public API and greatly increase their maintenance burden.
• The way tags overlap can dramatically impact the behavior and usefulness of this functionality. Consider a bugpattern that manifests both in JDK and in Android; should a hypothetical -XepTag:JDK:ERROR -XepTag:Android:OFF leave that check on or off?
• The second implementation has a smaller footprint but it changes a pretty significant property of Error Prone's behavior in ways that I think are surprising, counterintuitive, and impractical. I only list it to note that it seems like a really bad idea.
• The first implementation preserves the later-severity-wins property but this is a little difficult to do as-is. The relative order of -XepTags and -Xeps needs to be preserved but obviously the parser isn't geared for that at the moment. It might be possible to inflate -XepTag into corresponding -Xeps, in which case everything afterwards can remain unchanged; I opted instead to qualify the type of name and had to touch several unrelated places.
• Both implementations have nominally terrible performance characteristics but it's not like there was any point worrying about that at the time.

I am now skeptical that the superficial ease of use offsets the loss of transparency.

[cushon]

Thanks for the investigation @commonquail

Overall I think that the most transparent and least confusing way to enable checks is for projects to just list all of the checks that they want to enable. Especially for large codebases that's often going to be the best way to enable additional checks anyways, since enabling an entire category of checks would require atomically fixing all of the diagnostics at once, and it's probably easier to do that for one check at a time.

I think tags/categories could be useful even if they were just documentation, to help decide which of the checks might be useful for a particular project, even if the checks then had to be enabled individually.

One of the challenges with categories was that it was hard to ensure they were consistently applied to new checks and kept up-to-date, and that problem hasn't really been solved for tags either.