Proposal: Warnings for unnecessary warning suppression

Archie Cobbs archie.cobbs at gmail.com
Sat Nov 9 22:50:57 UTC 2024 

*Overview*

This is a proposal to add the ability for the compiler to detect and report
unnecessary warning suppressions.

An "unnecessary warning suppression" is when one of the following happens:

   - There is a @SuppressWarnings("foo") annotation, but if it hadn't been
   there, no foo warning would have been generated within the annotation's
   scope
   - The compiler is passed -Xlint:-foo, but if it hadn't been, no foo
   warning would have been generated during the entire compilation

*Motivation*

@SuppressWarnings and -Xlint:-foo are blunt instruments. The latter is
maximally blunt: it covers the entire compilation. The former is somewhat
blunt, especially when the warning occurs at a specific statement other
than a variable declaration and so the annotation has to annotate and cover
the entire containing method.

In practice @SuppressWarnings and -Xlint:-foo are also very sticky: once
they get added to a source file or a build process, they are rarely
removed, because that would require an audit to determine if the original
problem is now resolved (or the compiler behavior has changed), which is
tedious.

Sometimes @SuppressWarnings annotations are never needed in the first
place: they're added to the code proactively as the code is written because
the developer thinks they *might* be needed. In this situation, the
compiler provides the same feedback either way (i.e. none), so this type of
mistake is almost never caught.

As code evolves over time, newly added bugs that warnings are designed to
catch can escape detection if they happen to appear within the scope of a
@SuppressWarnings or -Xlint:-foo flag. That problem can't be solved
completely, but it can be minimized by ensuring that all @SuppressWarnings
annotations and -Xlint:-foo flags that do exist are actually serving some
purpose.

More generally, there is the natural and healthy need to "declutter", and
also the "peace of mind" factor: We want to know we're doing everything we
reasonably can to prevent bugs... and since the compiler is the thing that
generates the warnings in the first place, shouldn't it also be able to
detect and report when a warning is being unnecessarily suppressed?

*Caveats*

There are real-world concerns with adding something like this. Lots of
people build with -Xlint:all. We don't want to constrict the compiler so
tightly that it becomes more frustrating than helpful for people trying to
build software in the real world. Warning behavior can differ not only
across JDK versions but also across operating systems, so we don't want to
force over-complexification of builds.

There is a balance to strike; the functionality should be easy to disable.

*Proposal*

Add two new lint categories, as described by this --help-lint output:

  suppression          Warn about @SuppressWarnings values that don't
actually suppress any warnings.
  suppression-option   Warn about -Xlint:-key options that don't actually
suppress any warnings (requires "options").

Notice that for suppression-option to work, you also have to enable options
(see below for discussion).

The behavior in a nutshell:

   - When warnable code is detected, the warning "bubbles up" until it hits
   the first @SuppressWarning annotation in scope, or if none, the
   -Xlint:-foo option (if any).
   - If the warning doesn't hit anything and "escapes", the warning is
   emitted (this is what happens today)
   - Otherwise, the warning has hit a *suppression* - either a
   @SuppressWarning annotation or global -Xlint:-foo option - and so:
      - It is suppressed (this is what happens today), and
      - NEW: That suppression is marked as *validated*
   - NEW: After processing each file, the suppression category warns about
   @SuppressWarning annotations in that file containing unvalidated
   categories
   - NEW: After processing the entire compilation, the
suppression-option category
   warns about unvalidated -Xlint:-foo options.

Here's an example using rawtypes to demonstrate the proposed behavior:

@SuppressWarnings("rawtypes")      // annotation #1
public class Test {

    @SuppressWarnings("rawtypes")  // annotation #2
    public Iterable obj = null;    // "rawtypes" warning here
}

For a rawtypes warning to be emitted, the following must be true:

   - -Xlint:rawtypes must be enabled
   - Annotation #1 and annotation #2 must both NOT be present

This is the same logic that we already have.

For a suppression warning to be emitted at outer annotation #1 the
following must be true:

   - -Xlint:suppression must be enabled
   - Annotation #1 AND annotation #2 must BOTH be present

Note that in this case either annotation could be declared as the
"unnecessary" one, but when nested annotations suppress the same warning,
we will always assume that the innermost annotation is the "real" one (it's
the first to "catch" the warning as it bubbles up) and any containing
annotations are therefore the "unnecessary" ones.

As a result, it would never be possible for a suppression warning to be
emitted at annotation #2.

Also note that the category being suppressed does not itself need to be
enabled: the lint categories rawtypes and suppression warn about two
different things, and so they are enabled/disabled independently (*)

(*) This might be debatable. One could argue that if rawtypes is not
enabled, then all activity related to the rawtypes warning should be shut
down, including determining whether there is any unnecessary suppression of
it. This would be a more conservative change, but it would mean that only
the warnings that are actually enabled could be detected as unnecessarily
suppressed, which is a less robust check. In addition, it would mean that
for any given lint category, only one of the suppression or
suppression-option categories could be applicable at a time, which seems
too limiting.

For a suppression-option warning to be emitted for the above example, the
following must be true:

   - -Xlint:options must be enabled
   - -Xlint:suppression-option must be enabled
   - -Xlint:-rawtypes must be specified (i.e., it must be actively
   suppressed, not just disabled which is the default)
   - At least one of annotation #1 or annotation #2 must be present

The reason for requiring options is that the warning does in fact relate to
a command line option and so it seems appropriate that it be included. In
practice, options appears to be already in use as a "catch-all" when
building on multiple operating systems and/or JDK versions, etc., so this
will make for a cleaner upgrade path.


*Gory Details*

Some lint categories don't support @SuppressWarnings annotation scoping,
e.g, classfile, output-file-clash, path, and text-blocks (the latter
because it is calculated by the scanner before annotation symbols are
available). Putting them in a @SuppressWarnings annotation is always
useless (and will be reported as such). However, they are still viable
candidates for the suppression-option warning.

Some lint categories will be omitted from "suppression tracking" altogether:

   - path
   - options
   - suppression
   - suppression-option

The path category is omitted because it is used too early in the pipeline
(before singletons are created).

The options category is omitted because including it would be pointless:

   - It doesn't support @SuppressWarnings, so suppressions doesn't apply
   - If there's -Xlint:-options, then suppression-option is also disabled

What about the self-referential nature of suppressing suppression itself?
Consider this example:

@SuppressWarnings({ "rawtypes", "suppression" })
public class Test { }

There is no rawtypes warning in there, so the suppression of rawtypes is
indeed unnecessary and would normally result in a suppression warning. But
we also are suppressing the suppression warning itself, so the end result
is that no warning would be generated.

OK what about this?

@SuppressWarnings("suppression")
public class Test { }

If suppression were itself subject to suppression tracking, this example
would lead to a paradox. Instead, we exclude suppression itself from
suppression tracking. So that example would generate no warning. Analogous
logic applies to suppression-option - it doesn't apply to itself.

Note that @SuppressWarnings("suppression") is not totally useless, because
it can affect nested annotations:

@SuppressWarnings("suppression")   // this is NOT unnecessary
public class Test {

     // Suppression of "rawtypes" is unnecessary - but that won't be
reported
@SuppressWarnings("rawtypes")
public int x = 1;
}

Making suppression-option a separate warning from suppression seems a
reasonably obvious thing to do but there are also some subtle reasons for
doing that.

First, any system that does incremental builds (like the JDK itself) can
have a problem if the suppression-option warning is applied to a partial
compilation, because what if the file(s) that generate the warning being
suppressed are not part of that particular build? Then you would get a
false positive. So incremental builds could disable suppression-option  but
still safely leave suppression enabled.

Also, different versions of the JDK support different lint flags and have
different warning logic, so that warnings in some versions don't occur in
other versions. When the same source needs to be compiled under multiple
JDK versions, some -Xlint:-foo flags may be necessary in some versions and
useless in others. We want to ensure there's a reasonably simple way to use
the same command line flags when compiling under different JDK versions
without having to disable suppression tracking altogether.

Similarly, for some warnings the operating system might affect whether
warnings are generated.

*Prototype Status*

What follows is probably TMI but I figured I'd include a full brain dump
while top of mind...

I originally implemented this just to see how hard it would be and to play
around with the idea; it seems like the experiment has worked fairly well.

Of course the first thing I wanted to try was to run it on the JDK itself.
This revealed 400+ unnecessary @SuppressWarnings annotations and 11
unnecessary -Xlint:foo flags detected. That showed that the issue being
addressed is not imaginary.

Of course, because most of the JDK is built with -Xlint:all (or close to
it), that also meant tracking down and removing all of the unnecessary
suppressions; I had to semi-automate the process. A side-effect of that
effort is a series of separate PR's
<https://github.com/openjdk/jdk/pulls?q=author%3Aarchiecobbs+is%3Apr+%22Remove+unnecessary%22+in%3Atitle+>
to remove unnecessary @SuppressWarnings annotations and -Xlint:-foo flags.
Of course, those PR's can be evaluated independently from this proposal.

(You may wonder: How did all those useless suppressions get in there? See this
PR comment
<https://github.com/openjdk/jdk/pull/21853#issuecomment-2462566874>.)

I played around with a couple of different API designs. The API design is
key to ensuring we avoid various annoying inconsistencies that can easily
occur; a worst case scenario is a foo warning that gets reported somewhere,
but then when you add the @SuppressWarnings("foo") annotation to suppress
it, the annotation is reported as unnecessary - a catch-22. So I tried to
design & document the API to make it easy for compiler developers to avoid
inconsistencies (regression tests also contribute to this effort).

The key challenges as you might guess are:

   - Ensuring warning detection logic is no longer skipped when a category
   is suppressed if suppression is enabled (easy)
   - Ensuring that anywhere a warning is detected but isn't reported
   because the category is suppressed, the suppression is still validated
   (harder)

Summary of internal compiler changes:

   - Lint now keeps track of the current symbol "in scope" - this is
   whatever symbol was last used for Lint.augment(). Validations are
   tracked against these symbols, or null for the global scope.
   - A new singleton LintSuppression is responsible for maintaining this
   tracking information on a per-symbol and per-category basis, and generating
   warnings as needed when the time comes.
   - A new method Lint.isActive() answers the question "Should I bother
   doing some non-trivial calculation that might or might not generate a
   warning?" It returns true if the category is enabled OR if it's suppressed
   but subject to suppression tracking and the current suppression in scope
   has not yet been validated. This is entirely optional and usually not
   needed. An obvious example: before invoking Check.checkSerialStructure().
   - A new method Lint.validate() means "If this lint category is currently
   suppressed, then validate that suppression". In other words, you are saying
   that a warning would be generated here.
   - A new method Lint.emit() simplifies the logic when a lint warning is
   detected:
      - If the category is enabled, it logs the message
      - If the category is suppressed, it validates the suppression

So code that looked like this:

if (lint.isEnabled(LintCategory.FOO)) {
    log.warning(LintCategory.FOO, pos, SomeWarning(x, y));
}

can be simplified to this:

lint.emit(log, LintCategory.FOO, pos, SomeWarning(x, y));

A minor downside of that simplification is that the Warning object is
constructed even if the warning is suppressed. The upside is that
suppression validation happens automatically. Since warnings are relatively
rare, I felt this was a worthwhile trade-off, but it's not forced on people
- you can always do this instead:

if (lint.validate(LintCategory.FOO).isEnabled(LintCategory.FOO)) {
    log.warning(LintCategory.FOO, pos, SomeWarning(x, y));
}

When we're ready to report on unnecessary suppressions in a file, we scan
the file for @SuppressWarnings (and @Deprecated) annotations, then look at
the validatations of the corresponding symbol declarations, and do the
"propagation" step where all the validations bubble up. Any suppressions
that aren't validated are then reported as unnecessary. A similar thing
happens at the global scope to generate the suppression-option warnings,
using validations that escape individual source files, at the end of the
overall compilation.

There were two tricky refactorings: The overloads warning reports when two
methods are ambiguous when called with lambdas, but the warning itself has
the property that a @SuppressWarnings("overloads") annotation on *either*
of two such methods suffices to suppress the warning. So we have to be
careful with the logic, e.g., if both methods have the annotation, we don't
want to randomly validate one of them and then declare the other as
unnecessary, etc. To avoid this, both annotations are validated
simultaneously.

The other is the "this-escape" analyzer. When a constructor invokes this()
or a method, control flow jumps to that constructor or method; when it
executes super(), control flow jumps to all the field initializers and
non-static initializer blocks. This jumping around conflicts with the AST
tree-based scoping of @SuppressWarnings annotations. We apply "fixups" so
the suppression effect follows the control flow, not the AST. This is how
it already worked, but the code had to be updated to validate properly.

What about DeferredLintHandler and MandatoryWarningHandler? These were not
really an issue; all you need is a handle on the correct Lint instance and
one is always available.

The prototype is available here:
https://github.com/archiecobbs/jdk/tree/suppression

This prototype patch is a little unwieldy because it includes:

   - Compiler changes to support the new lint categories (in the diff
   starting with Lint.java)
   - Removal of 400+ @SuppressWarnings annotations to continue to allow the
   use of -Xlint:all everywhere (build logs
   <https://github.com/archiecobbs/jdk/actions/runs/11728281642>)
   - Several build-related cleanups, e.g., adding -Xlint:-suppression-option
   to unbreak incremental builds
   - Temporary build workaround for JDK-8340341

I'm interested in any opinions and/or folks who have large bodies of code
or specific test cases they would like to run this on.

Thanks,
-Archie

-- 
Archie L. Cobbs
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/compiler-dev/attachments/20241109/26ee195b/attachment-0001.htm>

More information about the compiler-dev mailing list