<div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div><b>Overview</b><br></div><div><br></div><div>This is a proposal to add the ability for the compiler to detect and report unnecessary warning suppressions.</div><div><br></div><div>An "unnecessary warning suppression" is when one of the following happens:<br></div><div><ul><li><span style="font-family:arial,sans-serif">There is a </span><span style="font-family:monospace">@SuppressWarnings("foo")</span><font face="arial,sans-serif"> annotation, but if it hadn't been there, no </font><span style="font-family:monospace">foo </span><font face="arial,sans-serif">warning would have been generated within the annotation's scope<br></font></li><li><span style="font-family:arial,sans-serif">The compiler is passed </span><span style="font-family:monospace">-Xlint:-foo</span>, but if it hadn't been, no <span style="font-family:monospace">foo</span> warning would<font face="arial,sans-serif"> have been generated during the entire compilation<br></font></li></ul></div><div><b>Motivation</b></div><div><br></div><div><span style="font-family:monospace">@SuppressWarnings</span> and <span style="font-family:monospace">-Xlint:-foo</span> 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.<br></div><div><br></div><div>In practice <span style="font-family:monospace">@SuppressWarnings</span> and <span style="font-family:monospace">-Xlint:-foo</span> 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.<br></div><br><div>Sometimes <span style="font-family:monospace">@SuppressWarnings</span> 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 <i>might</i> 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.<br></div><div><div><br></div><div>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 <span style="font-family:monospace">@SuppressWarnings</span> or <span style="font-family:monospace">-Xlint:-foo</span> flag. That problem can't be solved completely, but it can be minimized by ensuring that all <span style="font-family:monospace">@SuppressWarnings</span> annotations and <span style="font-family:monospace">-Xlint:-foo</span> flags that do exist are actually serving some purpose.</div><div><br></div><div>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?</div><div><br></div><div><b>Caveats</b></div><div><br></div><div>There are real-world concerns with adding something like this. Lots of people build with <span style="font-family:monospace">-Xlint:all</span>. 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.</div><div><br></div><div>There is a balance to strike; the functionality should be easy to disable.<br></div><br></div><div><b>Proposal</b><br></div><div><br></div><div>Add two new lint categories, as described by this <span style="font-family:monospace">--help-lint</span> output:<br></div><div><br></div><div><span style="font-family:monospace"> suppression Warn about @SuppressWarnings values that don't actually suppress any warnings.<br> suppression-option Warn about -Xlint:-key options that don't actually suppress any warnings (requires "options").<br></span></div><div><br></div><div>Notice that for <span style="font-family:monospace">suppression-option</span> to work, you also have to enable <span style="font-family:monospace">options</span> (see below for discussion).</div><br><div><div>The behavior in a nutshell:</div><div><ul><li>When warnable code is detected, the warning "bubbles up" until it hits the first <span style="font-family:monospace">@SuppressWarning</span> annotation in scope, or if none, the <span style="font-family:monospace">-Xlint:-foo</span>
option (if any).</li><li>If the warning doesn't hit anything and "escapes", the
warning is emitted (this is what happens today)<br></li><li>Otherwise, the warning has hit a <i>suppression</i> - either a
<span style="font-family:monospace">@SuppressWarning</span> annotation or global <span style="font-family:monospace">-Xlint:-foo</span> option - and so:</li><ul><li>It is suppressed (this is what happens today), and</li><li>NEW: That suppression is marked as <i>validated</i></li></ul><li>NEW: After processing each file, the <span style="font-family:monospace">suppression</span> category warns about <span style="font-family:monospace">@SuppressWarning</span> annotations in that file containing unvalidated categories<br></li><li>NEW: After processing the entire compilation, the <span style="font-family:monospace">suppression-option</span> category warns about unvalidated <span style="font-family:monospace">-Xlint:-foo</span> options.</li></ul></div></div><div>Here's an example using <span style="font-family:monospace">rawtypes</span> to demonstrate the proposed behavior:</div><div><br></div><div style="margin-left:40px"><span style="font-family:monospace">@SuppressWarnings("rawtypes") // annotation #1<br></span></div><div style="margin-left:40px"><span style="font-family:monospace">public class Test {</span></div><div style="margin-left:40px"><span style="font-family:monospace"><br></span></div><div style="margin-left:40px"><span style="font-family:monospace"> @SuppressWarnings("rawtypes") // annotation #2<br></span></div><div style="margin-left:40px"><span style="font-family:monospace"> public Iterable obj = null; // "rawtypes" warning here<br></span></div><div style="margin-left:40px"><span style="font-family:monospace">}</span></div><div><br></div><div>For a <span style="font-family:monospace">rawtypes</span> warning to be emitted, the following must be true:</div><div><ul><li><span style="font-family:monospace">-Xlint:rawtypes</span> must be enabled</li><li>Annotation #1 and annotation #2 must both NOT be present</li></ul></div><div>This is the same logic that we already have.<br></div><div><br></div><div>For a <span style="font-family:monospace">suppression</span> warning to be emitted at outer annotation #1 the following must be true:</div><div><div></div><div><ul><li><span style="font-family:monospace">-Xlint:</span><span style="font-family:monospace">suppression</span> must be enabled</li><li>Annotation #1 AND annotation #2 must BOTH be present</li></ul></div></div><div>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.</div><div><br></div><div>As a result, it would never be possible for a <span style="font-family:monospace">suppression</span> warning to be emitted at annotation #2.<br></div><div><br></div><div>Also note that the category being suppressed does not itself need to be enabled: the lint categories <span style="font-family:monospace">rawtypes</span> and <span style="font-family:monospace">suppression</span> warn about two different things, and so they are enabled/disabled independently (*)<br></div><div><br></div><div>(*) This might be debatable. One could argue that if <span style="font-family:monospace">rawtypes</span> is not enabled, then all activity related to the <span style="font-family:monospace">rawtypes</span> 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 <span style="font-family:monospace">suppression</span> or <span style="font-family:monospace">suppression-option</span> categories could be applicable at a time, which seems too limiting.<br></div><div><br></div><div><div>For a <span style="font-family:monospace">suppression-option</span> warning to be emitted for the above example, the following must be true:</div><div><div></div><div><ul><li><span style="font-family:monospace">-Xlint:options</span> must be enabled</li><li><span style="font-family:monospace">-Xlint:</span><span style="font-family:monospace">suppression</span><span style="font-family:monospace">-option</span> must be enabled</li><li><span style="font-family:monospace">-Xlint:-rawtypes</span> must be specified (i.e., it must be actively suppressed, not just disabled which is the default)<br></li><li>At least one of annotation #1 or annotation #2 must be present</li></ul></div></div></div><div>The reason for requiring <span style="font-family:monospace">options</span> is that the warning does in fact relate to a command line option and so it seems appropriate that it be included. In practice, <span style="font-family:monospace">options</span> 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.<br></div><div><b><br></b></div><div><b>Gory Details<br></b></div><br><div>Some lint categories don't support <span style="font-family:monospace">@SuppressWarnings</span> annotation scoping, e.g, <span style="font-family:monospace">classfile</span>, <span style="font-family:monospace">output-file-clash</span>, <span style="font-family:monospace">path</span>, and <span style="font-family:monospace">text-blocks</span> (the latter because it is calculated by the scanner before annotation symbols are available). Putting them in a <span style="font-family:monospace">@SuppressWarnings</span> annotation is always useless (and will be reported as such). However, they are still viable candidates for the <span style="font-family:monospace">suppression-option</span> warning.</div></div><div dir="ltr"><div><div><b><br></b></div>Some lint categories will be omitted from "suppression tracking" altogether:</div><div><ul><li><span style="font-family:monospace">path</span><br><span style="font-family:monospace"></span></li><li><span style="font-family:monospace">options</span></li><li><span style="font-family:monospace">suppression</span></li><li><span style="font-family:monospace">suppression-option</span><br></li></ul></div><div>The path category is omitted because it is used too early in the pipeline (before singletons are created).</div><div><br></div><div>The <span style="font-family:monospace">options</span> category is omitted because including it would be pointless:<br></div><div><ul><li>It doesn't support <span style="font-family:monospace">@SuppressWarnings</span>, so <span style="font-family:monospace">suppressions</span> doesn't apply<br></li><li>If there's <span style="font-family:monospace">-Xlint:-options</span>, then <span style="font-family:monospace">suppression-option</span> is also disabled<br></li></ul></div><div>What about the self-referential nature of suppressing <span style="font-family:monospace">suppression</span> itself? Consider this example:</div><div><br></div><div style="margin-left:40px"><span style="font-family:monospace">@SuppressWarnings({ "rawtypes", "suppression" })</span></div><div style="margin-left:40px"><span style="font-family:monospace">public class Test { }<br></span></div><div><br></div><div>There is no <span style="font-family:monospace">rawtypes</span> warning in there, so the suppression of <span style="font-family:monospace">rawtypes</span> is indeed unnecessary and would normally result in a <span style="font-family:monospace">suppression</span> warning. But we also are suppressing the <span style="font-family:monospace">suppression</span> warning itself, so the end result is that no warning would be generated.<br></div><div><br></div><div>OK what about this?</div><div><br></div><div><div style="margin-left:40px"><span style="font-family:monospace">@SuppressWarnings("suppression")</span></div><div style="margin-left:40px"><span style="font-family:monospace">public class Test { }<br></span></div><div><br></div><div>If <span style="font-family:monospace">suppression</span> were itself subject to suppression tracking, this example would lead to a paradox. Instead, we exclude <span style="font-family:monospace">suppression</span> itself from suppression tracking. So that example would generate no warning. Analogous logic applies to <span style="font-family:monospace">suppression-option</span> - it doesn't apply to itself.<br></div></div><div><div><br></div><div>Note that <span style="font-family:monospace">@SuppressWarnings("suppression")</span> is not totally useless, because it can affect nested annotations:</div><div><br></div><div><div style="margin-left:40px"><span style="font-family:monospace">@SuppressWarnings("suppression") // this is NOT unnecessary<br></span></div><div style="margin-left:40px"><span style="font-family:monospace">public class Test {</span></div><div style="margin-left:40px"><span style="font-family:monospace"><br></span></div><div style="margin-left:40px"><span style="font-family:monospace"> </span><span style="font-family:monospace">// Suppression of "rawtypes" is unnecessary - but that won't be reported<br></span></div><div style="margin-left:40px"><div style="margin-left:40px"><span style="font-family:monospace"></span></div><div style="margin-left:40px"><span style="font-family:monospace">@SuppressWarnings("rawtypes")<br></span></div><div style="margin-left:40px"><span style="font-family:monospace">public int x = 1;<br></span></div></div><div style="margin-left:40px"><span style="font-family:monospace">}<br></span></div><div><br></div></div><div>Making <span style="font-family:monospace">suppression-option</span> a separate warning from <span style="font-family:monospace">suppression</span> seems a reasonably obvious thing to do but there are also some subtle reasons for doing that.<br></div><div><div><div><br></div><div>First, any system that does incremental builds (like the JDK itself) can have a problem if the <span style="font-family:monospace">suppression-option</span> 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 <span style="font-family:monospace">suppression-option</span> but still safely leave <span style="font-family:monospace">suppression</span> enabled.<br></div></div><div><b><br></b></div></div><div>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 <span style="font-family:monospace">-Xlint:-foo</span>
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.</div><div><br></div><div>Similarly, for some warnings the operating system might affect whether warnings are generated.</div></div><div><br></div><div><b>Prototype Status</b><br></div><div><br></div><div>What follows is probably TMI but I figured I'd include a full brain dump while top of mind...<br></div><div><br></div><div>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.<br></div><div><div><br></div><div>Of course the first thing I wanted to try was to run it on the JDK itself. This revealed 400+ unnecessary <span style="font-family:monospace">@SuppressWarnings</span> annotations and 11 unnecessary <span style="font-family:monospace">-Xlint:foo</span> flags detected. That showed that the issue being addressed is not imaginary.</div><div><br></div><div>Of course, because most of the JDK is built with <span style="font-family:monospace">-Xlint:all </span>(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 href="https://github.com/openjdk/jdk/pulls?q=author%3Aarchiecobbs+is%3Apr+%22Remove+unnecessary%22+in%3Atitle+" target="_blank">a series of separate PR's</a> to remove unnecessary <span style="font-family:monospace">@SuppressWarnings</span> annotations and <span style="font-family:monospace">-Xlint:-foo</span> flags. Of course, those PR's can be evaluated independently from this proposal.<br><div><div><br></div><div><div>(You may wonder: How did all those useless suppressions get in there? See <a href="https://github.com/openjdk/jdk/pull/21853#issuecomment-2462566874" target="_blank">this PR comment</a>.)<br><br></div><div>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 <span style="font-family:monospace">foo</span> warning that gets reported somewhere, but then when you add the <span style="font-family:monospace">@SuppressWarnings("foo")</span>
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).</div><div><br></div><div><div>The key challenges as you might guess are:</div><div><ul><li>Ensuring warning detection logic is no longer skipped when a category is suppressed if <span style="font-family:monospace">suppression</span> is enabled (easy)<br></li><li>Ensuring that anywhere a warning is detected but isn't reported
because the category is suppressed, the suppression is still validated (harder)<br></li></ul></div><div><div>Summary of internal compiler changes:<br></div><div><ul><li><span style="font-family:monospace">Lint</span> now keeps track of the current symbol "in scope" - this is whatever symbol was last used for <span style="font-family:monospace">Lint.augment()</span>. Validations are tracked against these symbols, or null for the global scope.</li><li>A new singleton <span style="font-family:monospace">LintSuppression</span> is responsible for maintaining this tracking information on a per-symbol and per-category basis, and generating warnings as needed when the time comes.</li><li>A new method <span style="font-family:monospace">Lint.isActive()</span> 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 <span style="font-family:monospace">Check.checkSerialStructure()</span>.</li><li>A new method <span style="font-family:monospace">Lint.validate()</span> 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.<br></li><li>A new method <span style="font-family:monospace">Lint.emit()</span> simplifies the logic when a lint warning is detected:</li><ul><li>If the category is enabled, it logs the message<br></li><li>If the category is suppressed, it validates the suppression<br></li></ul></ul><div>So code that looked like this:</div><div><br></div><div style="margin-left:40px"><span style="font-family:monospace">if (lint.isEnabled(LintCategory.FOO)) {<br></span></div><div style="margin-left:40px"><span style="font-family:monospace"> log.warning(LintCategory.FOO, pos, SomeWarning</span><span style="font-family:monospace">(x, y)</span><span style="font-family:monospace">);</span></div><div style="margin-left:40px"><span style="font-family:monospace">}<br></span></div><div><br></div><div>can be simplified to this:</div><div><br></div><div><div style="margin-left:40px"><span style="font-family:monospace">lint.emit</span><span style="font-family:monospace">(log, LintCategory.FOO, pos, SomeWarning(x, y));</span></div><div><br></div></div><div>A minor downside of that simplification is that the <span style="font-family:monospace">Warning</span> 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:<br></div></div><div><br></div><div><div style="margin-left:40px"><span style="font-family:monospace">if (lint.validate(</span><span style="font-family:monospace">LintCategory.FOO</span><span style="font-family:monospace">).isEnabled(LintCategory.FOO)) {<br></span></div><div style="margin-left:40px"><span style="font-family:monospace"> log.warning(LintCategory.FOO, pos, SomeWarning</span><span style="font-family:monospace">(x, y)</span><span style="font-family:monospace">);</span></div><div style="margin-left:40px"><span style="font-family:monospace">}<br></span></div><div><br>When we're ready to report on unnecessary suppressions in a file, we scan the file for <span style="font-family:monospace">@SuppressWarnings</span> (and <span style="font-family:monospace">@Deprecated</span>) 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 <span style="font-family:monospace">suppression-option</span> warnings, using validations that escape individual source files, at the end of the overall compilation.<br></div></div><br><div>There were two tricky refactorings: The <span style="font-family:monospace">overloads</span> warning reports when two methods are ambiguous when called with lambdas, but the warning itself has the property that a <span style="font-family:monospace">@SuppressWarnings("overloads")</span> annotation on <i>either</i> 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.</div><div><br></div><div>The other is the "this-escape" analyzer. When a constructor invokes <span style="font-family:monospace">this()</span> or a method, control flow jumps to that constructor or method; when it executes <span style="font-family:monospace">super()</span>, control flow jumps to all the field initializers and non-static initializer blocks. This jumping around conflicts with the AST tree-based scoping of <span style="font-family:monospace">@SuppressWarnings</span> 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.<br></div><div><br></div><div>What about <span style="font-family:monospace">DeferredLintHandler</span> and <span style="font-family:monospace">MandatoryWarningHandler</span>? These were not really an issue; all you need is a handle on the correct <span style="font-family:monospace">Lint</span> instance and one is always available.<br></div><div><br></div><div>The prototype is available here: <a href="https://github.com/archiecobbs/jdk/tree/suppression" target="_blank">https://github.com/archiecobbs/jdk/tree/suppression</a><br></div><div><br></div><div>This prototype patch is a little unwieldy because it includes:</div><div><ul><li>Compiler changes to support the new lint categories (in the diff starting with <span style="font-family:monospace">Lint.java</span>)<br></li><li>Removal of 400+ <span style="font-family:monospace">@SuppressWarnings </span>annotations to continue to allow the use of <span style="font-family:monospace">-Xlint:all</span> everywhere (<a href="https://github.com/archiecobbs/jdk/actions/runs/11728281642" target="_blank">build logs</a>)<br></li><li>Several build-related cleanups, e.g., adding <span style="font-family:monospace">-Xlint:-suppression-option</span> to unbreak incremental builds</li><li>Temporary build workaround for JDK-8340341</li></ul></div><div><div>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.<br></div></div><div><br></div><div>Thanks,<br></div><div>-Archie<br></div><div><b><br></b></div><span class="gmail_signature_prefix">-- </span><br><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature">Archie L. Cobbs<br></div></div></div></div></div></div></div></div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>