Proposal: Warnings for unnecessary warning suppression
Jonathan Gibbons
jjg3 at pobox.com
Sat Nov 9 23:28:40 UTC 2024
Archie,
Various concerns for your consideration:
1. The set of strings that are valid for @SuppressWarnings is undefined
and open-ended. Yes, the JDK compiler `javac` defines a set of strings
but there is no guarantee that code will only be compiled by `javac` and
that there are no other strings in use. Some IDEs may also use the
@SuppressWarnings mechanism.
2. JLS 9.6.4.5 says:
The Java programming language defines four kinds of warnings that can be
specified by|@SuppressWarnings|:
*
Unchecked warnings (§4.8
<https://docs.oracle.com/javase/specs/jls/se23/html/jls-4.html#jls-4.8>,§5.1.6
<https://docs.oracle.com/javase/specs/jls/se23/html/jls-5.html#jls-5.1.6>,§5.1.9
<https://docs.oracle.com/javase/specs/jls/se23/html/jls-5.html#jls-5.1.9>,§8.4.1
<https://docs.oracle.com/javase/specs/jls/se23/html/jls-8.html#jls-8.4.1>,§8.4.8.3
<https://docs.oracle.com/javase/specs/jls/se23/html/jls-8.html#jls-8.4.8.3>,§15.12.4.2
<https://docs.oracle.com/javase/specs/jls/se23/html/jls-15.html#jls-15.12.4.2>,§15.13.2
<https://docs.oracle.com/javase/specs/jls/se23/html/jls-15.html#jls-15.13.2>,§15.27.3
<https://docs.oracle.com/javase/specs/jls/se23/html/jls-15.html#jls-15.27.3>)
are specified by the string "|unchecked|".
*
Deprecation warnings (§9.6.4.6
<https://docs.oracle.com/javase/specs/jls/se23/html/jls-9.html#jls-9.6.4.6>)
are specified by the string "|deprecation|".
*
Removal warnings (§9.6.4.6
<https://docs.oracle.com/javase/specs/jls/se23/html/jls-9.html#jls-9.6.4.6>)
are specified by the string "|removal|".
*
Preview warnings (§1.5
<https://docs.oracle.com/javase/specs/jls/se23/html/jls-1.html#jls-1.5>)
are specified by the string "|preview|".
Any other string specifies a non-standard warning. A Java compiler must
ignore any such string that it does not recognize.
Note that last sentence: /A Java compiler must ignore any such string
that it does not recognize.
/I think this means the proposal should be more clear that the proposed
effect only applies to strings that are recognized by the tool running
the check. JLS also says Compiler vendors are encouraged to document
the strings they support for|@SuppressWarnings|, and to cooperate to
ensure that the same strings are recognized across multiple compilers.
so we should document this in such a way that other tools are encouraged
to support the same string with the same semantics.
3. I note that the JDK `javadoc` tool supports variations on
`@SuppressWarnings("doclint"), corresponding to the `-Xdoclint` option
supported by both `javac` and `javadoc`, although there has been
informal discussions on dropping support from `javac`. See here:
https://docs.oracle.com/en/java/javase/23/docs/specs/man/javadoc.html#doclint
-- Jon
On 11/9/24 2:50 PM, Archie Cobbs wrote:
> *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/compiler-dev/attachments/20241109/ce547f13/attachment-0001.htm>
More information about the compiler-dev
mailing list