8345263: Make sure that lint categories are used correctly when logging lint warnings #22553

mcimadamore · 2024-12-04T17:11:26Z

This PR tightens up the logic by which javac reports lint warnings. Currently, lint warnings are typically emitted using the following idiom:

if (lint.isEnabled(LintCategory.DIVZERO) {
    log.warning(LintCategory.DIVZERO, pos, Warnings.DIVZERO);
}

There are some issues with this approach:

logging a lint warning has to be preceded by the correct isEnabled check
the check and the log::warning call must share the same LintCategory
the selected warning key in the Warnings class must also make sense for the selected LintCategory

This PR addresses these issues, so that the above code is now written as follows:

lint.logIfEnabled(pos, LintWarnings.DIVZERO);

The new idiom builds on a number of small improvements:

the lint category is now tracked directly in the compiler.properties file;
a new LintWarning class is added to JCDiagnostic to model a warning key that is also associated with a speicfic LintCategory enum constant;
the CompilerProperties class has a new group of compiler keys, nested in the new LintWarnings class. This class defines the LintWarning objects for all the warning keys in compiler.properties that have a lint category set
A new method Lint::logIfEnabled(Position, LintWarning) is added - which simplifies the logging of lint warnings in many common cases, by merging the isEnabled check together with the logging.

As bonus points, the signatures of some methods in Check and MandatoryWarningHandler have been tightened to accept not just a Warning, but a LintWarning. This makes it impossible, for instance, to use Check::warnUnchecked with a warning that is not a true lint warning.

Many thanks @archiecobbs for the useful discussions!

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8345263: Make sure that lint categories are used correctly when logging lint warnings (Enhancement - P4)

Reviewers

Vicente Romero (@vicente-romero-oracle - Reviewer)
Jan Lahoda (@lahodaj - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/22553/head:pull/22553
$ git checkout pull/22553

Update a local copy of the PR:
$ git checkout pull/22553
$ git pull https://git.openjdk.org/jdk.git pull/22553/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22553

View PR using the GUI difftool:
$ git pr show -t 22553

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22553.diff

Using Webrev

Link to Webrev Comment

Initial stab at adding a new LintWarning info

Readded Lint kind to processing warnings

Add Lint::logIfEnabled

bridgekeeper · 2024-12-04T17:12:07Z

👋 Welcome back mcimadamore! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-12-04T17:13:11Z

@mcimadamore This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8345263: Make sure that lint categories are used correctly when logging lint warnings

Reviewed-by: vromero, jlahoda

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 153 new commits pushed to the master branch:

ef6e987: 8346040: Zero interpreter build on Linux Aarch64 is broken
f7f07b9: 8345804: Update copyright year to 2024 for langtools in files where it was missed
1bdb7b4: 8345622: test/langtools/tools/javac/annotations/parameter/ParameterAnnotations.java should set processorpath to work correctly in the agentvm mode
0ad6423: 8345944: JEP 492: extending local class in a different static context should not be allowed
68aa4d4: 8346063: java/lang/Thread/virtual/Starvation.java missing @requires vm.continuations
77e4932: 8344026: Ubsan: prevent potential integer overflow in c1_LIRGenerator_.cpp file
3f2556b: 8345984: Remove redundant checkXXX methods from java.management Util class
ceb4366: 8345955: Deprecate the UseOprofile flag with a view to removing the legacy oprofile support in the VM
72c59de: 8345876: Update nativeAddAtIndex comment to match the code
75cfb64: 8310691: [REDO] [vectorapi] Refactor VectorShuffle implementation
... and 143 more: https://git.openjdk.org/jdk/compare/4000e923e8b4472fe022f1fd78a1c42b2045683f...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-12-04T17:13:45Z

@mcimadamore The following labels will be automatically applied to this pull request:

build
compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

archiecobbs · 2024-12-04T17:51:32Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java

-                                           if (lint.isEnabled(LintCategory.STRICTFP)) {
-                                               log.warning(LintCategory.STRICTFP,
-                                                           pos, Warnings.Strictfp); }
+            deferredLintHandler.report(_ -> {


Nit: we can remove the lambda braces now. Also in a few other places below.

archiecobbs · 2024-12-04T17:57:19Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java

-                 log.warning(LintCategory.SERIAL,
-                             TreeInfo.diagnosticPositionFor(svuid, tree),
+                 log.warning(
+                         TreeInfo.diagnosticPositionFor(svuid, tree),
                             Warnings.ImproperSVUID((Symbol)e));


Shouldn't this be changed to LintWarnings.ImproperSVUID? There are several other similar examples in this class.

Good catch, I probably misclassified some of these warnings

archiecobbs · 2024-12-04T17:59:51Z

src/jdk.compiler/share/classes/com/sun/tools/javac/file/BaseFileManager.java

@@ -529,6 +525,6 @@ synchronized void newOutputToPath(Path path) throws IOException {

        // Check whether we've already opened this file for output
        if (!outputFilesWritten.add(realPath))
-            log.warning(LintCategory.OUTPUT_FILE_CLASH, Warnings.OutputFileClash(path));
+            log.warning(LintWarnings.OutputFileClash(path)); // @@@: shouldn't we check for suppression?


Re: "shouldn't we check for suppression?": If -Xlint:-output-file-clash is in effect, then outputFilesWritten will be null and you'll never get here.

mcimadamore · 2024-12-04T19:06:58Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java

@@ -4470,9 +4469,9 @@ public void visitSelect(JCFieldAccess tree) {
            // If the qualified item is not a type and the selected item is static, report
            // a warning. Make allowance for the class of an array type e.g. Object[].class)
            if (!sym.owner.isAnonymous()) {
-                chk.warnStatic(tree, Warnings.StaticNotQualifiedByType(sym.kind.kindName(), sym.owner));
+                chk.lint.logIfEnabled(tree, LintWarnings.StaticNotQualifiedByType(sym.kind.kindName(), sym.owner));


This is an odd one - the rest of Attr uses env.info.lint but if I do that here I get some NPE in some unrelated JVM test. I wonder how robust using two different lints is... but for now I'll keep compatibility with what we have.

mcimadamore · 2024-12-04T19:08:09Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java

            });
        }
    }

    void checkModuleRequires(final DiagnosticPosition pos, final RequiresDirective rd) {
        if ((rd.module.flags() & Flags.AUTOMATIC_MODULE) != 0) {
-            deferredLintHandler.report(_l -> {
+            deferredLintHandler.report(_ -> {
                if (rd.isTransitive() && lint.isEnabled(LintCategory.REQUIRES_TRANSITIVE_AUTOMATIC)) {


This one is a bit odd - the warning can be reported under two categories, so the code makes sure we don't report twice. This is deliberate I've checked with @lahodaj

mcimadamore · 2024-12-04T19:11:03Z

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java

+     * Helper method. Log a lint warning if its lint category is enabled.
+     */
+    public void logIfEnabled(DiagnosticPosition pos, LintWarning warning) {
+        if (isEnabled(warning.getLintCategory())) {


I'm not 100% sure about this method. On the one hand it makes clients simpler - but it does require Lint keeping track of a log. An alternative could be to add a method in AbstractLog, e.g.:

warningIfEnabled(DiagnosticPosition pos, Lint lint, LintWarning key)

elsewhere, @archiecobbs suggested to maybe make log a parameter of the method. I like the simplicity of the suggestion, and I will go with it.

mlbridge · 2024-12-04T19:13:32Z

Webrevs

Simplify some lambda expressions Simplify Lint::logIfEnabled by accepting a log parameter

lahodaj · 2024-12-05T14:34:29Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java

@@ -4294,13 +4256,12 @@ private boolean isCanonical(JCTree tree) {
    /** Check that an auxiliary class is not accessed from any other file than its own.
     */
    void checkForBadAuxiliaryClassAccess(DiagnosticPosition pos, Env<AttrContext> env, ClassSymbol c) {
-        if (lint.isEnabled(Lint.LintCategory.AUXILIARYCLASS) &&
-            (c.flags() & AUXILIARY) != 0 &&
+        if ((c.flags() & AUXILIARY) != 0 &&


Here, and on a few other places, the checks were ordered so that the potentially more expensive checks were done after the isEnabled check. Checking the exact places, I don't think delaying the check is terrible for the cases in this PR, but if we would like to be adding new lints, we probably need something that will avoid running the lint's code completely in a (semi-)automatic way. That could be part of a more generic 22088.

(Admittedly, on some places, the isEnabled check was last, after the more expensive checks. This is particularly the case for the TRY lint, where the checks might be considerable.)

we probably need something that will avoid running the lint's code completely in a (semi-)automatic way. That could be part of a more generic 22088.

Agreed.

Slight comment hijack follows...

The unnecessary suppression warning proposal would create a natural answer for this, because there would be a new concept "active" along with "enabled": A category is active if it's enabled OR we are tracking unnecessary suppressions, the category is being suppressed at this point in the code, and so far that suppression is unnecessary (i.e., no warnings would have fired yet).

So linter code would want to check for "active" status (instead of "enabled" status) before doing a non-trivial calculation, which would end up looking like this:

if (lint.isActive(LintCategory.FOO) && complicatedCalculationA() && complicatedCalculationB()) { lint.logIfEnabled(log, pos, LintWarnings.FooProblem); }

But perhaps then we should then consider lambda-ification, which would simplify this down to:

lint.check(log, pos, LintWarnings.FooProblem, () -> complicatedCalculationA() && complicatedCalculationB());

This could be your "(semi-)automatic" option.

Here, and on a few other places, the checks were ordered so that the potentially more expensive checks were done after the isEnabled check. Checking the exact places, I don't think delaying the check is terrible for the cases in this PR, but if we would like to be adding new lints, we probably need something that will avoid running the lint's code completely in a (semi-)automatic way. That could be part of a more generic 22088.

(Admittedly, on some places, the isEnabled check was last, after the more expensive checks. This is particularly the case for the TRY lint, where the checks might be considerable.)

I was aware of that. As you might notice, in some cases I preserved the original code structure - in other cases I've changed it when the preceding checks seemed O(1) -- I might have made some mistakes, but that was the spirit.

But perhaps then we should then consider lambda-ification, which would simplify this down to:

lint.check(log, pos, LintWarnings.FooProblem, () -> complicatedCalculationA() && complicatedCalculationB());

I thought about this as well but found it a bit too convoluted for my liking. One might already argue that Lint::logIfEnabled is doing two things at once (check and log). Making it an open box where you check if the lint is enabled, you check some user provided condition, and if both are true then you log" seems a rather ad-hoc API to add :-)

Overall I just think the problem of "you checked for A, but are logging a B" is not big enough to warrant an hammer like this.

What would be better, IMHO, is to investigate an automate way to e.g. enhance something like CheckExamples to automatically test for suppression of lint warnings. E.g. for each diagnostic example that refers to a lint warning, it would be great if we could run the example with -Xlint:-xyz (to test command-line suppression) and maybe to auto-decorate the whole thing with @SuppressWarnings("xyz") and try again (to test programmatic supression) -- where xyz is the lint category associated to the compiler key under test. While this is probably too big to tacked as part of this PR, I don't see why we couldn't automate part of this -- and this will ensure that any issue where suppression is not respected is identified early in the process.

What would be better, IMHO, is to investigate an automate way to e.g. enhance something like CheckExamples to automatically test for suppression of lint warnings.

Indeed... and I've already got a precursor which could serve as a starting point for something like that. What's missing in the precursor is automatic verification that every lint warning is tested, since until this PR that was impractical. But with this PR we can now enumerate all the LintWarning's, so it would be easy to automatically verify we have complete coverage. Of course, we'd also have to add all the missing warnable code examples, which is not hard but somewhat tedious. Let me know if you are interested in putting something together, I'm happy to help.

But with this PR we can now enumerate all the LintWarning

Yes, this is indeed the part I like the most about this PR. I'm already using it to collect a bunch of stats on how many lint warnings we have, how many warnings by category, etc. - which is very useful to spot inconsistencies etc.

vicente-romero-oracle · 2024-12-05T22:08:48Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java

@@ -3002,7 +2968,7 @@ void checkAccessFromSerializableElement(final JCTree tree, boolean isLambda) {
                isEffectivelyNonPublic(sym)) {
                if (isLambda) {
                    if (belongsToRestrictedPackage(sym)) {
-                        log.warning(LintCategory.SERIAL, tree.pos(),
+                        log.warning(tree.pos(),
                                    Warnings.AccessToMemberFromSerializableLambda(sym));


so this one won't be a lint warning anymore I guess

Actually I think this is another accidentally omitted conversion from Warnings to LintWarnings.

True, this is a bug in compiler.properties.

vicente-romero-oracle · 2024-12-05T22:14:46Z

src/jdk.compiler/share/classes/com/sun/tools/javac/util/JCDiagnostic.java

        public Warning(String prefix, String key, Object... args) {
            super(DiagnosticType.WARNING, prefix, key, args);
        }
    }

+    /**
+     * Class representing warning diagnostic keys.


representing lint warning ...?

vicente-romero-oracle

really nice simplification and clean-up

magicus · 2024-12-05T22:47:45Z

/label -build

openjdk · 2024-12-05T22:48:40Z

@magicus
The build label was successfully removed.

vicente-romero-oracle

lvgtm

lahodaj

Looks sensible to me.

mcimadamore · 2024-12-13T09:49:39Z

I'm ready to integrate. I have, however, a question. We also have other localized compiler.properties files:

compiler_de.properties
compiler_ja.properties
compiler_zh.properties

Eyeballing, it seems that these files seem to also be correctly annotated. Should I do anything in particular here?

lahodaj · 2024-12-13T11:44:54Z

I'm ready to integrate. I have, however, a question. We also have other localized compiler.properties files:
* compiler_de.properties

* compiler_ja.properties

* compiler_zh.properties
Eyeballing, it seems that these files seem to also be correctly annotated. Should I do anything in particular here?

I assume you mean the # 0: symbol annotations, right? I would expect that not to matter at all for the non-generic/non-English files: the generic/English properties bundle should be used to generate the access classes, and the other languages are only used for provide correct key->value mapping, and should be (I hope) ignored w.r.t. the annotations.

So, I would expect that there should be nothing to do? (And even if something was needed, we usually do not modify the language-specific property files.)

Do I miss something?

mcimadamore · 2024-12-13T18:39:24Z

So, I would expect that there should be nothing to do? (And even if something was needed, we usually do not modify the language-specific property files.)

Do I miss something?

Our build process should not depend on that right. However I noted that these files do contain annotations already. Whose job it is to update those?

mcimadamore · 2024-12-13T18:41:53Z

So, I would expect that there should be nothing to do? (And even if something was needed, we usually do not modify the language-specific property files.)
Do I miss something?

Our build process should not depend on that right. However I noted that these files do contain annotations already. Whose job it is to update those?

To reply to my own question, it seems these properties are bulk-updated at the end of the release cycle. Here's the one for 24:

#22588

So, this PR doesn't need to do anything there. Thanks!

mcimadamore · 2024-12-16T10:19:05Z

/integrate

openjdk · 2024-12-16T10:20:17Z

Going to push as commit dbffe33.
Since your change was applied there have been 184 commits pushed to the master branch:

32c8195: 8345801: C2: Clean up include statements to speed up compilation when touching type.hpp
9286018: 8345322: RISC-V: Add concurrent gtests for cmpxchg variants
4fc43b0: 8345770: javadoc: API documentation builds are not always reproducible
3518b4b: 8344171: Clone and initialize Assertion Predicates in order instead of in reverse-order
c88e081: 8346160: Fix -Wzero-as-null-pointer-constant warnings from explicit casts
ab1dbd4: 8346202: Correct typo in SQLPermission
6b022bb: 8344453: Test jdk/jfr/event/oldobject/TestSanityDefault.java timed out
3b9de11: 8319875: Add macOS implementation for jcmd System.map
ebb27c2: 8346139: test_memset_with_concurrent_readers.cpp should not use
... and 174 more: https://git.openjdk.org/jdk/compare/4000e923e8b4472fe022f1fd78a1c42b2045683f...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-12-16T10:20:28Z

@mcimadamore Pushed as commit dbffe33.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

mcimadamore added 7 commits December 4, 2024 11:54

Tweak warning keys in compiler.properties

72b2d5a

Initial stab at adding a new LintWarning info

Builds ok

b878483

Use diagnostic info line to declare lint category

ad337b4

All tests pass

c14773a

All langtools tests pass

ebc5110

Readded Lint kind to processing warnings

Simplify code

06df4f7

Separate LintWarnings from Warnings in CompilerProperties

c49f7a5

Add Lint::logIfEnabled

openjdk bot added build build-dev@openjdk.org compiler compiler-dev@openjdk.org labels Dec 4, 2024

mcimadamore changed the title ~~Separate LintWarnings from Warnings in CompilerProperties~~ 8345263: Make sure that lint categories are used correctly when logging lint warnings Dec 4, 2024

archiecobbs reviewed Dec 4, 2024

View reviewed changes

Fix regression

9a657f2

mcimadamore commented Dec 4, 2024

View reviewed changes

mcimadamore marked this pull request as ready for review December 4, 2024 19:08

openjdk bot added the rfr Pull request is ready for review label Dec 4, 2024

mcimadamore commented Dec 4, 2024

View reviewed changes

Add missing lint categories in compiler.properties

64883bb

Simplify some lambda expressions Simplify Lint::logIfEnabled by accepting a log parameter

lahodaj reviewed Dec 5, 2024

View reviewed changes

vicente-romero-oracle reviewed Dec 5, 2024

View reviewed changes

openjdk bot removed the build build-dev@openjdk.org label Dec 5, 2024

Add other missing lint categories

a3393ca

vicente-romero-oracle approved these changes Dec 6, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 6, 2024

lahodaj approved these changes Dec 12, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Dec 16, 2024

openjdk bot closed this Dec 16, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 16, 2024

8345263: Make sure that lint categories are used correctly when logging lint warnings #22553

8345263: Make sure that lint categories are used correctly when logging lint warnings #22553

Conversation

mcimadamore commented Dec 4, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Dec 4, 2024

openjdk bot commented Dec 4, 2024 • edited Loading

openjdk bot commented Dec 4, 2024

archiecobbs Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlbridge bot commented Dec 4, 2024 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicente-romero-oracle left a comment

Choose a reason for hiding this comment

magicus commented Dec 5, 2024

openjdk bot commented Dec 5, 2024

vicente-romero-oracle left a comment

Choose a reason for hiding this comment

lahodaj left a comment

Choose a reason for hiding this comment

mcimadamore commented Dec 13, 2024

lahodaj commented Dec 13, 2024

mcimadamore commented Dec 13, 2024

mcimadamore commented Dec 13, 2024

mcimadamore commented Dec 16, 2024

openjdk bot commented Dec 16, 2024

openjdk bot commented Dec 16, 2024

mcimadamore commented Dec 4, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Dec 4, 2024 •

edited

Loading

archiecobbs Dec 4, 2024 •

edited

Loading

mlbridge bot commented Dec 4, 2024 •

edited

Loading