8344079: Minor fixes and cleanups to compiler lint-related code

[archiecobbs]

Please review these changes with some minor lint-related fixes and cleanups.

See JDK-8344079 for a more detailed description.

---

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-8344079: Minor fixes and cleanups to compiler lint-related code (Enhancement - P5)

Reviewers

• Maurizio Cimadamore (@mcimadamore - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22056

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back acobbs! 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]

@archiecobbs 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:

8344079: Minor fixes and cleanups to compiler lint-related code

Reviewed-by: mcimadamore

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 6 new commits pushed to the master branch:

• f1d85ab: 8346773: Fix unmatched brackets in some misc files
• 9393897: 8346260: Test "javax/swing/JOptionPane/bug4174551.java" failed because the font size of message "Hi 24" is not set to 24 in Nimbus LookAndFeel
• e98f412: 8346922: TestVectorReinterpret.java fails without the rvv extension on RISCV fastdebug VM
• ca5390c: 8346924: TestVectorizationNegativeScale.java fails without the rvv extension on RISCV fastdebug VM
• 07c9f71: 8346921: Remove unused arg in markWord::must_be_preserved
• 84e6432: 8346288: WB_IsIntrinsicAvailable fails if called with wrong compilation level

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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]

@archiecobbs The following label will be automatically applied to this pull request:

• compiler

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

[mlbridge]

Webrevs

• 07: Full (d58f3ad6)
• 06: Full (b986eb32)
• 05: Full - Incremental (c2f770b5)
• 04: Full (d0d25725)
• 03: Full - Incremental (af342035)
• 02: Full - Incremental (7cb7aa0c)
• 01: Full - Incremental (8b961d48)
• 00: Full (de60c11c)

[mcimadamore]

Thanks for fixing all these. IMHO the fact we had so many bad usages points at the fact that the system we use to report warning is a bit too loose. I wonder if we should differentiate warning keys from lint warning keys, and have lint warnings be reported with a dedicated method in Log, so that there can be no confusion as to which is which (this is for a separate, possible, future PR)

[archiecobbs]

Agree... one of the best ways to ensure a policy is correctly followed is to make the build fail when it's not :)

[mcimadamore]

Filed:

https://bugs.openjdk.org/browse/JDK-8345263

I will take a look.

[mcimadamore]

Initial stab:
https://github.com/openjdk/jdk/compare/master...mcimadamore:jdk:lint_warning_strong_checks?expand=1

The idea is to mark in compiler.properties which keys are associated to which lint category, and track the association in the generated CompilerProperties file. Then we can just drop the additional LintCategory parameter from all the call sites.

[archiecobbs]

I like that a lot - very nice!

I have one comment in two parts...

(1) Even though the following current idiom is verbose and contains redundancy:

if (lint.isEnabled(LintCategory.DEP_ANN))
    log.warn(LintCategory.DEP_ANN, pos, Warnings.MissingDeprecatedAnnotation));

that redundancy has a nice side effect which is that it makes it easy to spot a mismatch between the lint category being tested and the lint category of the subsequent warning, where as your patch makes it easier to make a mistake like this:

if (lint.isEnabled(LintCategory.DEPRECATION))
    log.warn(pos, Warnings.MissingDeprecatedAnnotation));

Such a mismatch would result in a bug causing a very frustrating (dis)appearance of some warning, potentially failing builds, etc.

(2) The connection of a warning to a lint category, which is based on a properties file comment line, could easily get out of sync with the code without notice causing similar problems to (1). E.g., someone could put the wrong category in the comment (or change it later in the file but not the code), or accidentally forget to add the comment line, etc.

Both (1) and (2) could be addressed easily I think - for example, what if we baked the lint category more firmly into the property name, and used that to make it more visible in the code?

Example:

compiler.warn[dep-ann].missing.deprecated.annotation=\
    deprecated item is not annotated with @Deprecated

if (lint.isEnabled(LintCategory.DEP_ANN))
    log.warn(pos, Warnings.LintDepAnn.MissingDeprecatedAnnotation));

That way it's more visually obvious that the warning is associated to the DEP_ANN lint category in both the code and the properties file, and so they hopefully would have a harder time getting out of sync.

[mcimadamore]

Not sure I buy your arguments :-)
While the mechanics of lint warning is a bit different, they are still warnings (e.g. the compiler still says warning: <text>, and it is still something that counts towards Werror. Making them completely separate seems like trying to make them more different than they really are.

And, if we have both log(Warning) and log(LintWarning) whether they are the same type or not becomes a mot question, because you can still use them in place of another. And, if you have subclassing and have only a single log(Warning) method which adds the category or not depending on the runtime type of the warning, then the problem you describe above cannot occur (I think).

So, while I see how one can argue this several ways, the advantage for going down the path you describe seems less clear to me. In terms of object-oriented hierarchy, saying that a lint warning is a warning with a category added on top seems to me like a good modelling of the situation we have.

[archiecobbs]

OK I think you're right and it will all work out - using the runtime type ensures the prefix always appears, and the other thing I was worried about, which is someone accidentally logging a lint warning without checking for enablement because they thought it was a generic warning, should be prevented now because the in the code you will see LintWarnings.FooBar instead of Warnings.FooBar so hopefully it will be very obvious.

[mcimadamore]

I've updated the branch in place. This adds LintWarnings in CompilerProperties but does not add the intermediate grouping based on the lint category. E.g. We have LintWarnings.DivZero but not LintWarnings.DivZero.DivZero. The reason is twofold:

• Implementation-wise, collecting different warnings associated with the same category starts to be some work in the build tool we use to generate CompilerProperties.
• More importantly, while converting the use cases to use the new LintWarning class it seems to me that either the isEnabled check is immediately preceding (in which case the new Lint::logIfEnabled can be used), or the check is typically far enough that we have no idea on which category we are handling. E.g. pattern like if (lint) { log ... } is extremely common, where lint is either a method parameter or a class field.

I think I'm fairly happy with where this has landed. Note that the new type LintWarning can be used to make the signature of some methods (notably those in MandatoryWarningHandler) tighter, so that we can detect more issues at compile-time (and at runtime). In fact, this has helped me fixing a couple of warning keys which were missing the lint category in the compiler.properties file.

I'm not 100% happy with Lint::logIfEnabled: the use sites look good, but having Lint keep track of a log seems a bit odd (given that Lint objects are duplicated on the fly).

[mcimadamore]

I'll probably issue a PR against that branch, so that we can discuss that there. Sorry for hijacking your PR :-)

[archiecobbs]

I think this sounds like a good plan and agree with your reasoning for not over-complicating the class hierarchy.

I'm sure I'll have some minor comments on the patch but will wait for you to create a new PR and comment there when it appears.

Thanks.

I'm not 100% happy with Lint::logIfEnabled: the use sites look good, but having Lint keep track of a log seems a bit odd (given that Lint objects are duplicated on the fly).

I was thinking that a log parameter would have to be passed as a parameter, because there is at least one subclass somewhere (ReusableLog extends Log), but maybe I was being too conservative.

[mcimadamore]

Looks good, I've left some additional style-related comment

[bridgekeeper]

@archiecobbs This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[mcimadamore]

Looks good

[archiecobbs]

Thanks for the review.

[archiecobbs]

/integrate

[openjdk]

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

• ccf3d57: 8346985: Convert test/jdk/com/sun/jdi/ClassUnloadEventTest.java to Class-File API
• 594e519: 8346984: Remove ASM-based benchmarks from Class-File API benchmarks
• c027f2e: 8346983: Remove ASM-based transforms from Class-File API tests
• e0695e0: 8346981: Remove obsolete java.base exports of jdk.internal.objectweb.asm packages
• dfaa891: 8346569: Shenandoah: Worker initializes ShenandoahThreadLocalData twice results in memory leak
• f1d85ab: 8346773: Fix unmatched brackets in some misc files
• 9393897: 8346260: Test "javax/swing/JOptionPane/bug4174551.java" failed because the font size of message "Hi 24" is not set to 24 in Nimbus LookAndFeel
• e98f412: 8346922: TestVectorReinterpret.java fails without the rvv extension on RISCV fastdebug VM
• ca5390c: 8346924: TestVectorizationNegativeScale.java fails without the rvv extension on RISCV fastdebug VM
• 07c9f71: 8346921: Remove unused arg in markWord::must_be_preserved
• ... and 1 more: https://git.openjdk.org/jdk/compare/a77ed30fcc3360cd16a11b1899f52f7e871df1df...master

Your commit was automatically rebased without conflicts.

[openjdk]

@archiecobbs Pushed as commit dd81f8d.

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