<div dir="ltr"><div dir="ltr"><div dir="ltr"><div>Hi Maurizio,</div><div><br></div><div>Thanks for the feedback.<br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Nov 11, 2024 at 5:32 AM Maurizio Cimadamore <<a href="mailto:maurizio.cimadamore@oracle.com" target="_blank">maurizio.cimadamore@oracle.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><u></u>
<div><p>* 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.<br></p></div></blockquote><div>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 <span style="font-family:monospace">Options.java</span>, so you could say e.g., <span style="font-family:monospace">-Xlint:file-stuff</span> instead of <span style="font-family:monospace">-Xlint:classfile,path,output-file-clash</span>. This wouldn't require changing any of the existing warning logic.<br></div><div><br></div><div>OTOH a deeper refactoring would replace the <span style="font-family:monospace">LintCategory</span> enum with something more complicated in a tree-like hierarchy.</div><div><br></div><div>Are there any concrete design thoughts on this yet?<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><p>
</p>
<p>* 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.</p></div></blockquote><div>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.<br></div><div><br></div><div>As you can see my answer was to shoehorn it into <span style="font-family:monospace">Flow.java</span> (after the flow analysis) even though this has nothing to do with "flow analysis". This was simply following the precedent established by <span style="font-family:monospace">ThisEscapeAnalyzer.java</span><span style="font-family:arial,sans-serif">, which faced the same question before. That's twice now so maybe that's a hint we need a better answer :)<br></span></div><div><span style="font-family:arial,sans-serif"><br></span></div><div><span style="font-family:arial,sans-serif">Dumb question: would adding a new compiler "step" be (roughly speaking) as straightforward as adding </span><span style="font-family:monospace">CompileState.WARN</span> and doing<span style="font-family:arial,sans-serif"> something like this?</span></div><div><span style="font-family:arial,sans-serif"><br></span></div><div style="margin-left:40px"><span style="font-family:monospace">- generate(desugar(flow(attribute(x))));</span></div><div style="margin-left:40px"><span style="font-family:monospace">+ generate(desugar(warn(flow(attribute(x)))));</span></div><div style="margin-left:40px"><span style="font-family:monospace"><br></span></div><div><span style="font-family:arial,sans-serif">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 </span><span style="font-family:arial,sans-serif">gathered</span><span style="font-family:arial,sans-serif"> by the compiler.</span></div><div><span style="font-family:arial,sans-serif"><br></span></div><div><span style="font-family:arial,sans-serif">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</span><span style="font-family:arial,sans-serif"> immediately</span><span style="font-family:arial,sans-serif">.<br></span></div><div><span style="font-family:arial,sans-serif"><br></span></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">* 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.</blockquote><div><br></div><div>I assume you're referring to the <span style="font-family:monospace">lint.emit()</span> method with built-in suppression checking... and there are some other general cleanups in <span style="font-family:monospace">Lint.java</span> that could be included as well.</div><div><br></div><div>I'll work on a separate patch containing the "non-controversial" cleanups.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><p>* 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:</p></div></blockquote><div>Yes, this initial patch is a giant blob that is for testing only and obviously will need to be split up.</div><div><br></div><div>The various <span style="font-family:monospace">@SuppressWarnings</span> cleanup clearly deserve an umbrella issue.<br></div><div><br></div><div>Should I create issues for the following? <span style="font-family:arial,sans-serif">Let me know your thoughts:</span><br></div><div><ul><li>Minor lint refactoring cleanups<br></li><li>Add a new <span style="font-family:monospace">warn()</span> compiler step after flow analysis</li><ul><li>ThisEscapeAnalyzer is a charter member<br></li></ul><li>Refactor to move lint warnings into the warn() step (umbrella)</li><ul><li>Add issues over time under this umbrella for migrating existing warnings<br></li><li>Depends on: Add new warn() step</li></ul><li>Umbrella issue for the <span style="font-family:monospace">@SuppressWarnings </span>cleanup</li><ul><li>Existing cleanup issues go under here<br></li></ul><li>Suppression warnings (this proposal)</li><ul><li>Depends on: Add new warn() step</li><li>Depends on: Umbrella issue for the <span style="font-family:monospace">@SuppressWarnings </span>cleanup</li><li>Depends on: Minor lint refactoring cleanups</li></ul></ul></div><div>Thanks,<br></div><div>-Archie</div><div><br></div></div><span class="gmail_signature_prefix">-- </span><br><div dir="ltr" class="gmail_signature">Archie L. Cobbs<br></div></div>
</div>