Specify @SuppressWarnings suppression strings relevant to analyses powered by our annotations

[amaembo]

It seems we have a consensus that our project not only specifies which code is correct and which is not but also specifies which kinds of warnings at which code locations should be issued. So it seems reasonable also to specify ways to suppress such warnings. I believe it's not the top priority problem. Probably we may even skip it for the first release, allowing tools to use their current suppression mechanisms. But having a conversion about this won't harm.

Note that we should provide suppressions for the specified warnings only. And we probably should not overspecify. E.g.

void foo() {
  getNullable().bar(); // we specify that the warning should be issued here
  if (getNotNull() == null) { // we don't specify (at least for now) whether here should be a warning
  }
}

I believe, all the tools which report nullability violations already have ways to suppress the warnings. Most of them support a standard @SuppressWarnings annotation. A notable exception is bytecode analyzers such as FindBugs and SpotBugs. As they don't see the source code normally, and @SuppressWarnings annotation has SOURCE retention, they cannot see it.. So if we recommend using @SuppressWarnings, these tools will be unable to follow our recommendations. Both of them define a CLASS-level @SuppressFBWarnings annotation (also they allow to define such annotation in any package you like if you don't want an external dependency).

Suppression of FindBugs problem requires to specify the internal ID of the issued warning. E.g. @SuppressFBWarnings("NP_NULL_PARAM_DEREF_NONVIRTUAL") for non-virtual method parameter dereference. They are too granular and, I believe, we don't need to specify such granularity. On the other hand, it's possible to suppress by prefix like @SuppressFBWarnings("NP"): suppresses all NPE-related warnings (though IIRC not all of them actually start with "NP").

IntelliJIDEA supports standard @SuppressWarnings annotation, as well as special comment like //noinspection ... which allows suppressing warning on every statement inside method body (not only on the declaration statement). As a value, you should specify a "suppressionId" which often coincides with internal inspection ID. Here we have the opposite picture: most of the nullability problems are reported by too broad inspection named "Constant Conditions & Exceptions", so you have to write //noinspection ConstantConditions or @SuppressWarnings("ConstantConditions"). This looks crazy and also may suppress many more warnings like "Condition is always true" or "Array index is out of bounds". So I would definitely not recommend using IntelliJIDEA suppression ID as a standard.

Eclipse supports standard @SuppressWarnings annotation (no way to suppress for individual statement), and their suppress IDs looks much better than ones IntelliJ or FindBugs has. For nullability violation, it's just a @SuppressWarnings("null"). This suppression is not too specific and not too broad. To me, it looks the best way to suppress the warnings about unsafe conversion from nullable type to non-nullable (assign nullable to not-null; pass nullable argument as not-null parameter; dereference nullable; throw nullable; unbox nullable; etc.). However, we also need to be able to suppress strict mode warnings (conversion from nullable to unknown and from unknown to not-null), these should have separate suppress ID (I'm inclined to have single suppress ID for both cases). @SuppressWarnings("unchecked-null") looks fine to me, though you may suggest something better.

Now it's unclear whether we should have the same suppress ID when the implicit conversion of container element nullability is performed. E.g.:

List<@Nullable String> list1 = ...
List<@NotNull String> list2 = list1;

I think we should specify that the warning must be issued at list2 = list1 declaration. Should it have the same "null" suppress ID or something different?

Please share how suppression works in your tools (Sonar, ErrorProne, Checker Framework, etc.) Thank you!

[stephan-herrmann]

Thanks for bringing this up!

In addition to @SuppressWarnings Eclipse also has a few precedents where line comments control reporting of warnings, like //$NON-NLS-1$, //$FALL-THROUGH$ etc. If the group considers per-line suppression important, I would be fine with adding one or two of these (for statements that are not local variable declarations).

If the group thinks @SuppressWarnings("unchecked-null") worthwhile, I might agree, too. Just wondering if "null-unchecked" would be more appropriate (giving emphasis to "null"), but that's close to splitting hairs already :)

[wmdietlGC]

For the Checker Framework, the relevant documentation is here:

• https://checkerframework.org/manual/#suppressing-warnings-nullness
• https://checkerframework.org/manual/#suppressing-warnings

In @SuppressWarnings we use a type-system specific prefix, in this case nullness, possibly followed by a specific error key.
For example @SuppressWarnings("nullness:assignment.type.incompatible") suppresses warnings from incompatible types in an assignment, for the Nullness Checker only.
One can also only specify a type system or only an error message key.

For finer-grained suppressions we support both assert statements and checkNotNull methods.

[kevinb9n]

I see a lot of value in standardizing suppression strings. They are API!

I believe we can think of this as an orthogonal effort to standardizing the annotations and their semantics. But to me it does belong under the 'codeanalysis' banner, and even though our current github and group names happen to include "annotations" I am totally fine with treating this as in scope.

[kevinb9n]

I think giving some hierarchical structure to the strings is an interesting idea that could prove useful. Suppose checker A cares about distinguishing "a:b:c" from "a:b:d", but checker B only recognizes all of "a:b" as a single category. The convention could be, then, that it ignores the unrecognized ":c" or ":d" suffix. A lot more thought will have to go into this, though.

[cpovirk]

One thing that I could imagine is wanting is to see errors for unsafe conversions due to unannotated code but wanting not to see them for cases like Map.get. I'm not sure that that's particularly well defined or easy to understand, though, nor whether I'd really want it when push came to shove.

[cpovirk]

(One possible approach: Map.get is declared @UnknownNullness("key-not-present"), and I can suppress key-not-present errors (which might also include Table.get, Android APIs for looking up UI elements by key, etc.).)

[kevinb9n]

I still believe in this one and have a proposal to pitch at some point. It doesn't feel urgent, but if anyone else is thinking about this, drop me a note.

[netdpb]

Current decision: We will not include this feature in 1.0.

Proposal for 1.0: Finalize the current decision. If you agree, please add a thumbs-up emoji (👍) to this comment. If you disagree, please add a thumbs-down emoji (👎) to this comment and briefly explain your disagreement. Please only add a thumbs-down if you feel you can make a strong case why this decision will be materially worse for users or tool providers than an alternative.

[kevinb9n]

Simple thing we can do today: add to our spec that "nullness" must be respected as a suppression string for all related warnings.

Less simple things we can consider to finally properly solve the problem: see a detailed draft proposal from 2021:
https://docs.google.com/document/d/1yVUhgFaoxE8V8geX8SlrJ5WnKs_9tBplcHkYBmKtJUA/edit?usp=sharing