8254274: Development to add 'lint' warning for @ValueBased classes

[sadayapalam]

New javac lint mode (-Xlint:synchronize) to warn about client side as well JDK internal attempts to synchronize on instances of value based classes

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Testing

	Linux x64	Windows x64	macOS x64
Build	✔️ (5/5 passed)	✔️ (2/2 passed)	✔️ (2/2 passed)
Test (tier1)	✔️ (9/9 passed)	✔️ (9/9 passed)	✔️ (9/9 passed)

Issue

• JDK-8254274: Development to add 'lint' warning for @valuebased classes

Reviewers

• Dan Smith (@dansmithcode - Committer) ⚠️ Review applies to 1a66082
• Jim Laskey (@JimLaskey - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/valhalla pull/237/head:pull/237
$ git checkout pull/237

[bridgekeeper]

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

[mlbridge]

Webrevs

• 02: Full - Incremental (6a6c67e)
• 01: Full - Incremental (1a66082)
• 00: Full (8c962dc)

[sadayapalam]

A few questions for reviewers:

(1) Do we want the lint mode to be on my default ? Presently it is off and has to be expressly opted into.
(2) Do we want to not use up a bit for VALUE_BASED flag and simply walk the annotations to determine if a type is value based ??

[mlbridge]

Mailing list message from Remi Forax on valhalla-dev:

----- Mail original -----

De: "Srikanth Adayapalam" <sadayapalam at openjdk.java.net>
?: "valhalla-dev" <valhalla-dev at openjdk.java.net>
Envoy?: Lundi 26 Octobre 2020 12:11:23
Objet: Re: RFR: 8254274: Development to add 'lint' warning for @valuebased classes

On Mon, 26 Oct 2020 11:01:59 GMT, Srikanth Adayapalam <sadayapalam at openjdk.org>
wrote:

New javac lint mode (-Xlint:synchronize) to warn about client side as well JDK
internal attempts to synchronize on instances of value based classes

A few questions for reviewers:

(1) Do we want the lint mode to be on my default ? Presently it is off and has
to be expressly opted into.

yes !
It should be enable by default.

Not enough people are using -Xlint:all in the wild.

[...]

R?mi

[dansmithcode]

(1) Do we want the lint mode to be on my default ? Presently it is off and has to be expressly opted into.

Given that it's already been established that there's an ad hoc set of on-by-default lint warnings, yeah, I think this one makes sense to put in that set. (Inventing a new concept of on-by-default lint warnings is something I'd be more hesitant about.)

[sadayapalam]

I have pushed additional changes to incorporate review comments:

- The lint mode's token is "synchronization" (instead of "synchronize")
- This lint mode is on by default
- Redundant warnings on wait/notify calls are removed.
- We don't burn up a flag bit to mark value based classes. We simply walk the declaration annotations on a class to discover the ones that are @ValueBased
- Ensure descriptions (in the neighbourhood) in javac.properties end with a period for consistency's sake. 
- Streamline and simplify tests.

[sadayapalam]

/integrate

[openjdk]

@sadayapalam This PR has not yet been marked as ready for integration.

[JimLaskey]

Wondering if keyword SYNCHRONIZATION is too broad, suggest VALUE_ SYNCHRONIZATION. Would it make sense to have a test for a class.ref type value?

[openjdk]

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

8254274: Development to add 'lint' warning for @ValueBased classes

Reviewed-by: dlsmith, jlaskey

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 no new commits pushed to the jep390 branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

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

[sadayapalam]

Wondering if keyword SYNCHRONIZATION is too broad, suggest VALUE_ SYNCHRONIZATION. Would it make sense to have a test for a class.ref type value?

class.ref is a valhalla'ism - but the present work is targetting JDK16 as a precursor/preparation for Valhalla. (even though the integration first happens to jep390 branch of Valhalla repo as a temporary home). So the .ref syntax is really not available to test/need not be tested.

As for the keyword, I am comfortable with it, but I will make it a point to solicit input at the next stage of review. (The accumulated cross component changes will get to a RFR when targetting jdk16)

[sadayapalam]

/integrate

[openjdk]

@sadayapalam Pushed as commit 2ac97b2.

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