JavaDoc folk may be interested in this thread developing on
compiler-...@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.co...@gmail.com>
To: compiler-dev <compiler-...@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
