8384068: [lworld] ValueClassPlugin misses some annotations

[xmas92]

There are few places which uses the fully qualified name for the AsValueClass annotation. As a result the plugging does not modify these classes when compiling, so they are still identity classes.

I propose improving the robustness of this plugin. We need to do this during parsing so we cannot actually check 100% that it will resolve to the correct annotation. However we can do a best effort, which handles same package, fully qualified, imported and rejects other annotations with the same class name.

---

• I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

---

Progress

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

Issue

• JDK-8384068: [lworld] ValueClassPlugin misses some annotations (Bug - P4)

Reviewers

• Joel Sikström (@jsikstro - Committer)
• Johan Sjölen (@johan-sjolen - no project role)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2405

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/2405.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

[openjdk]

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

8384068: [lworld] ValueClassPlugin misses some annotations

Reviewed-by: jsikstro, jsjolen

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 14 new commits pushed to the lworld branch:

• 6663aa3: 8386623: [lworld] Cleanup module support for system modules in JDK exploded build with preview resources
• 43640ba: 8386316: [lworld] Replace Valhalla since version with 28
• 8e1cb51: 8386464: [lworld] Random core libs cleanup
• ... and 11 more: https://git.openjdk.org/valhalla/compare/042a35d51ab4f405fb73caf05a660d39314c8066...lworld

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 lworld branch, type /integrate in a new comment.

[mlbridge]

Webrevs

• 03: Full - Incremental (029a1b0f)
• 02: Full - Incremental (e0f36bb3)
• 01: Full - Incremental (9d8f7d52)
• 00: Full (9b4a1cd8)

[xmas92]

Just a friendly ping.

The only tests that are currently being affected is gc/stress/gcbasher. So we are missing some EnablePreview testing here. Would be nice to get this in both for test coverage and to ensure that we do not miss future uses of @jdk.test.lib.valueclass.AsValueClass.

[jsikstro]

I think this is going in the right direction. One question:

What if we have a case like below, that will match as true when it shouldn't. But I guess that's rare/bad code.

package jdk.test.lib.valueclass;

import other.AsValueClass;

@AsValueClass
public final class ShadowInSamePackage {}

[jsikstro]

This is not my area, but I think the revised logic around checking for the AsValueClass annotation is sound and seem to slot it nicely with the surrounding code.

[johan-sjolen]

This should obviously be done at semantic analysis-time (well, post-semantic analysis-time). I don't get why it has to be done at parse time, but OK, if you say so then I believe you. Besides, this is an improvement.