google
Any simple way to completely disable messages from plugin checks at default `SUGGESTION` level?
See https://github.com/uber/NullAway/issues/1363. We included a new RequireExplicitNullMarking check in NullAway 0.12.13 at SUGGESTION level. It's matches get reported as NOTEs since it's at SUGGESTION, so they still show up in the console for a Gradle build by default (I imagine their printing is build-system dependent). Is there any easy way to provide this check but have it as completely off by default, with no printed messages? I think I can hack something using VisitorState.severityMap() to just return Description.NO_MATCH when the check is at SUGGESTION but it'd be nice to have a way to completely disable the check. Thanks!
@rickie I think a bunch of the error-prone-support checks are at SUGGESTION level by default; have you all run into this issue?
@cushon assuming this is not possible now except via hacks, would you be open to some kind of PR to enable this? Adding some kind of OFF severity seems like an obvious way to accomplish it, but not sure what else that might impact. I modified our code to work around this issue for now, but the change seems fragile.
I think that having a -XepDisableAllSuggestions flag would be consistent with other configuration we already have. Would that meet the requirements here?
I think that internally checks with 'suggestion' severity are mostly things that get run as refactorings, not surfaced in build logs or code review. With the external Error Prone integrations, build diagnostics are sort of the only hammer we have, so suggestions get surfaced there. Similar to #5367, I tend to think that only putting errors in build logs is often a good choice, and finding other ways to surface non-error level checks.
Maybe we should have a more generic mechanism to configure all checks of a particular default severity as a different severity, or just go ahead and add all combinations of -XepDisableAll{Suggestions,Warnings,Errors} and -XepAll{Suggestions,Warnings,Errors}As{Suggestions,Warnings,Errors}. I'm not requesting a PR for that, maybe let's just add -XepDisableAllSuggestions for now :)
@msridhar We don't run into this issue, as we always resolve all cases when introducing a new check.
In our build, we use:
-XepAllErrorsAsWarnings
-XepAllSuggestionsAsWarnings
combined with the failOnWarning. Because of that, we don't have many violations. It will only give some messages when someone is working on code before it's merged.
My goal is to be able to ship a plugin check that is disabled by default (like the "Experimental" checks shipped with Error Prone). So I don't think -XepDisableAllSuggestions does quite what I want, since by default our check would still be enabled. If SUGGESTION level checks were disabled by default and needed to be explicitly enabled (so, something like -XepEnableAllSuggestions), that would do the trick. So I think I'm proposing something similar to #5367 but for suggestions instead.
There should probably be a new annotation, or a new attribute in @BugPattern, to signify that the check should be disabled by default, while still providing a default severity for when one wants to enable it without specific level, like for disabled-by-default built-in checks.
There should probably be a new annotation, or a new attribute in
@BugPattern, to signify that the check should be disabled by default, while still providing a default severity for when one wants to enable it without specific level, like for disabled-by-default built-in checks.
That SGTM
I am also curious what you all think of disabling suggestions and warnings by default, and only emitting errors by default, and allowing suggestions and warnings to be enabled with -XepEnableAllSuggestions / -XepEnableAllWarnings? if that would be disruptive I don't think there's a rush to do it, but in hindsight it seems like a better experience.
I wouldn't mind suggestions being disabled by default, partly because there's none enabled by default, and I don't think I use any check that defaults to a suggestion level.
I use -Werror though, so that would have to be replaced with -XepAllWarningsAsErrors or complemented with -XepEnableAllWarnings. Such a change would be disruptive though, as all of a sudden you update Error Prone and many checks aren't applied anymore without notice (other than a few words in the release notes).
Would it be possible to have (discovered / extension) suggestion-level checks disabled by default but without entirely turning off the suggestion level? I.e. the NullAway check (from 0.12.13) being disabled by default, but -Xep:RequireExplicitNullMarking (without level) enabling it at suggestion level? I personally wouldn't use it, but it might be a good compromise (assuming people don't use the suggestion level much in their custom checks)
Personally, I'd say the NullAway check should "default to error (or warning) level, disabled by default", so enabling it with its default level would be "meaningful" (like other disabled-by-default built-in checks, e.g. SystemExitOutsideMain or DefaultLocale)
I agree that updating Error Prone to disable warnings by default could be disruptive. If we add the feature to the @BugPattern annotation to disable a check unless explicitly enabled, I would use it for the extra NullAway check and also probably set the default level to WARN. For suggestion-level checks it seems they don't come up that often in practice. I only used SUGGESTION because I thought it was equivalent to disabled-by-default for some reason.
Having a mechanism for indicate that plugin checks should be disabled by default, separate from their default severity, SGTM.
(I see that we once had a SeverityLevel.NOT_A_PROBLEM to indiciate that a check should be disabled, but that conflates whether it's enabled with what it's severity is: https://github.com/google/error-prone/commit/ff0c9b22d52c6e0599967d1eec289e27a8cd1ebd.)
I can try to put up a PR for this but it will probably take me some time...in case someone else wants to take it on 🙂