Proposal: Warnings for unnecessary warning suppression

[Archie Cobbs]

Hi Maurizio,

Thanks for the feedback.

On Mon, Nov 11, 2024 at 5:32 AM Maurizio Cimadamore <
maurizio.cimadamore at oracle.com> wrote:

> * There have been other requests to alter and/or enhance Lint warnings
> recently, and some changes in the area are likely unavoidable because of
> null-restricted types (Valhalla). One common request is to allow for
> "families" of logically-related warnings (so that, on the command line, I
> can decide at which level of granularity I want lint warnings to be
> enabled/disabled). Something like this has a non-trivial chance to interact
> with your proposal.
>
Grouping warnings into "families" is a nice idea. I suppose a lot depends
on whether it were implemented at the "parsing" layer or more deeply. In
other words, it would be easy to build a simple grouping concept based on
"grouping aliases" into Options.java, so you could say e.g.,
-Xlint:file-stuff instead of -Xlint:classfile,path,output-file-clash. This
wouldn't require changing any of the existing warning logic.

OTOH a deeper refactoring would replace the LintCategory enum with
something more complicated in a tree-like hierarchy.

Are there any concrete design thoughts on this yet?

* Implementation-wise, I've always been unhappy about the way in which Lint
> warnings are so deeply integrated inside the core javac classes. Maybe for
> some of them (e.g. the file manager ones) not much can be done... but for
> most of them I do wonder whether they should all be done in a separate
> pass, after flow analysis is complete -- and I also wonder if this should
> be some kind of mechanism that should be open for extension (e.g. so that
> developers can define their own analyses). Of course, the more Lint
> warnings are a core part of javac, and the harder this is.
>
The same thoughts occurred to me early in the process when trying to answer
the question, "Where do these new warnings get reported?" There's no
central obvious place to add new warning calculations.

As you can see my answer was to shoehorn it into Flow.java (after the flow
analysis) even though this has nothing to do with "flow analysis". This
was simply following the precedent established by ThisEscapeAnalyzer.java,
which faced the same question before. That's twice now so maybe that's a
hint we need a better answer :)

Dumb question: would adding a new compiler "step" be (roughly speaking) as
straightforward as adding CompileState.WARN and doing something like this?

-    generate(desugar(flow(attribute(x))));
+    generate(desugar(warn(flow(attribute(x)))));

As for consolidating all of the warning logic, I like that idea and the
potential ability to make warnings more pluggable. The refactoring would be
tedious but straightforward, at least for those warnings that simply
inspect the information already being gathered by the compiler.

Also, refactoring wouldn't need to be done all at once and could be done
over time, but new valhalla warnings would have a home available immediately
.

* Even if we never implement the proposal in full, I think there are useful
> bits and pieces in your email - such as consolidating the way in which lint
> warnings are reported. I believe these are good changes to make, and I'd
> like to perhaps consider them separately. It is likely that any improvement
> and consolidation in this area will make any subsequent change to the way
> in which lint warnings are reported easier to implement, so I support such
> changes and refactorings.


I assume you're referring to the lint.emit() method with built-in
suppression checking...  and there are some other general cleanups in
Lint.java that could be included as well.

I'll work on a separate patch containing the "non-controversial" cleanups.

* On a similar note, it would perhaps be better to separate the javac
> changes required to enable the new analysis, from the JDK changes to remove
> redundant suppressions. In other similar cases where a new analysis has
> been added, we have been filing umbrella issues for the various JDK
> subcomponents:
>
Yes, this initial patch is a giant blob that is for testing only and
obviously will need to be split up.

The various @SuppressWarnings cleanup clearly deserve an umbrella issue.

Should I create issues for the following? Let me know your thoughts:

   - Minor lint refactoring cleanups
   - Add a new warn() compiler step after flow analysis
      - ThisEscapeAnalyzer is a charter member
      - Refactor to move lint warnings into the warn() step (umbrella)
      - Add issues over time under this umbrella for migrating existing
      warnings
      - Depends on: Add new warn() step
   - Umbrella issue for the @SuppressWarnings cleanup
      - Existing cleanup issues go under here
      - Suppression warnings (this proposal)
      - Depends on: Add new warn() step
      - Depends on: Umbrella issue for the @SuppressWarnings cleanup
      - Depends on: Minor lint refactoring cleanups

Thanks,
-Archie

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