8199429: [lworld] value type (re)constructors expressions value type should not be discardable

[jespersm]

…be discardable as per https://bugs.openjdk.org/browse/JDK-8199429 .

This is a first stab at it, only with the manual marking on a method, and without the loading of the annotation in ClassReader.

---

Progress

• Change must not contain extraneous whitespace

Integration blocker

 ⚠️ Title mismatch between PR and JBS for issue JDK-8199429

Issue

• JDK-8199429: [valhalla] value type (re)constructors expressions should not be discardable (Enhancement - P4) ⚠️ Title mismatch between PR and JBS.

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1274

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[mlbridge]

Webrevs

• 01: Full - Incremental (c2130bd2)
• 00: Full (793f07d4)

[liach]

I think the "reconstrcutor" proposal is out-of-date; the latest approach would be "deconstructors" openjdk/amber#94 and "Derived record creation" https://openjdk.org/jeps/468

For @MustUse: in the "CheckReturnValue at Google" presentation in JVMLS 2024, Google finds that the pattern is simple: All builder/fluent methods can have return values ignored by default, and all other methods should have return values "must use" by default. Only a tiny fraction of the methods can have their return value ignored, like List.add.

[jespersm]

Thanks for the feedback, @liach

Re. reconstructors: Good point, the case of an JCExpressionStatement "p with { y = 0}" is of course easy to flag as an error, (just from structure alone, like x+1 in the original motivation).

As for the annotation: I could not find the Google presentation on YT, but I guess that what you're saying is that the annotation would make sense for any and all methods which return a computed value without causing any mutations or side-effects. Since values are immutable, only side effects need consideration, so the bar is a lot lower (and enforceable in practice, since we don't want to break people code, even nonsensical parts).

So in the example given in the PR, the method next() could be automatically annotated as @MustUse because we know that the entire method call will be useless if the value is not put to good use.
If, however, the method's control flow was ever to leave the method (even by throwing an exception) or a value was assigned to any field (on some referenced object, obviously not on the value), the @MustUse annotation could not be set automatically.

As for identity objects, I'm guessing that developers in the future could use @MustUse on methods where the only sane usage is to use the returned value, such as in the builder pattern as you mention, to alert their callers of the intent, since we can't apply the same rules as with the value types.

Or am I overthinking this? 😀

[bridgekeeper]

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