8351362: Post-process @Strict annotation for testing

[liach]

Added a test tool to strictify files and migrated the existing StrictFinalInstanceFieldsTest as an example.

---

Progress

• Change must not contain extraneous whitespace

Issue

• JDK-8351362: Post-process @Strict annotation for testing (Enhancement - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1395

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

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

8351362: Post-process @Strict annotation for testing

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

• 0f63f74: Merge jdk
• 2483340: 8352579: Refactor CDS legacy optimization for lambda proxy classes
• 1397ee5: 8334322: Misleading values of keys in jpackage resource bundle
• ... and 105 more: https://git.openjdk.org/valhalla/compare/ea9789ce9cce9da4deaa514090cf256001151060...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[mlbridge]

Webrevs

• 03: Full (84bdb7a6)
• 02: Full - Incremental (a44c269c)
• 01: Full - Incremental (30d78fb9)
• 00: Full (bb1fd0dc)

[liach]

Looking to integrate this as I have added all functionalities in the RFE (despite the ctor call relocation being extremely primitive)

/integrate

[openjdk]

@liach
Your change (at version a44c269) is now ready to be sponsored by a Committer.

[RogerRiggs]

Can we use import jdk.internal.vm.annotation.Strict and not introduce a duplicate annotation?
This code is already using jdk.internal.vm.annotation.NullRestricted.

And/or add a comment here indicating it is an intentional duplication.

[liach]

@dansmithcode aims to migrate all usages of the internal strict to this test-specific strict. This is actually the reason behind this PR - annotations in the Java Language must not change program semantics.

[RogerRiggs]

That would then apply to NullRestricted also?
That seems like an unnecessary split, since Strict is an internal annotation, it doesn't matter whether it is declared in a test specific library or jdk.internal.value. and it make maintenance harder because they are split.
And it makes prototyping uses of Strict more difficult.
And not defined or specified anywhere.

[dansmithcode]

The idea is that the @Strict annotation should live next to the tool that interprets it.

If we decide we need a translation tool in jdk.internal, then putting @Strict there too makes sense. If this is purely a test-writing tool, then @Strict belongs in tests.

@NullRestricted is interpret by the JVM, so it makes sense that it lives in the JVM's internal API.

[RogerRiggs]

It has been said that in tests every NullRestricted annotated element should also have @strict.
Since @strict is there only for test generation; would it be simpler to have the tool look for NullRestricted and do the right thing; avoiding both excessive editing of tests and parallel but dependent annotations.

[dansmithcode]

Possibly, yes. The trade-off is then it's harder or confusing to write tests of validation behavior—when you explicitly want a class file that has @NullRestricted but not ACC_STRICT. Maybe that's a corner case we can deal with though. It also obscures the dependency on the translation tool (importing the test library is a helpful signal that you intend to use the translation tool). But, sure, that's a possible enhancement.

[RogerRiggs]

The arguments should be documented.
Typically, command options like --enable-preview come before file (.java) file arguments.
The parsing below seems to have this reversed.
The only args that come after -- are file names that might otherwise be confused with options. (Nothing in this case).
--enable-preview and --source should be handled like --deferSuperCall
--source should have its own arg saved and propagated (like javac).

[dansmithcode]

I would not recommend integrating until #1389 is done.

At that point, there will be a large number of tests to experiment with, and after seeing if/how conveniently they can be updated to adopt this tool, we can then have a conversation with the rest of the team about whether this is a good solution going forward, or whether there are still concerns about this not being built into javac.

[bridgekeeper]

@liach 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!