Fwd: Proposal: Warnings for unnecessary warning suppression

[Jonathan Gibbons]

JavaDoc folk may be interested in this thread developing on 
compiler-dev at openjdk.org.  While primarily about javac -Xlint and 
matching @SuppressWarnings, there are potential parallels for -Xdoclint 
and its values for @SuppressWarnings.

https://mail.openjdk.org/pipermail/compiler-dev/2024-November/028573.html

-- Jon

-------- Forwarded Message --------
Subject: 	Proposal: Warnings for unnecessary warning suppression
Date: 	Sat, 9 Nov 2024 16:50:57 -0600
From: 	Archie Cobbs <archie.cobbs at gmail.com>
To: 	compiler-dev <compiler-dev at openjdk.org>



*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 wouldhave 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:
      o It is suppressed (this is what happens today), and
      o 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:
      o If the category is enabled, it logs the message
      o 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/javadoc-dev/attachments/20241110/60395ea0/attachment-0001.htm>