8292275: javac does not emit SYNTHETIC and MANDATED flags for parameters by default

[SirYwell]

With this change, javac emits the MethodParameters attribute in cases where the JLS requires the information about synthetic and mandated parameters to be stored (see issue). Parameter names are not emitted unless the -parameter flag is set.

The relevant changes are in ClassWriter, where we go through the params to see if we need the attribute if the -parameter flag is not set (if it is set, both names and flags will be emitted). For records, the mandated flag wasn't set at all, this is solved by the one line fix in JavacParser.

The changes to CreateSymbols and ClassReader are needed as they weren't able to deal with missing names in the attribute. I also had to update some tests as they got a new constant pool entry.

Only the mandated flag is covered by tests at the moment, as the occurrences are well-specified in the JLS. Please let me know if you want tests for specific appearances of synthetic parameters.

---

Progress

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

Issue

• JDK-8292275: javac does not emit SYNTHETIC and MANDATED flags for parameters by default

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9862

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

Using diff file

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

[bridgekeeper[bot]]

:wave: Welcome back SirYwell! 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[bot]]

@SirYwell The following labels will be automatically applied to this pull request:

• build
• compiler

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

[mlbridge[bot]]

Webrevs

• 07: Full - Incremental (400e1ec9)
• 06: Full - Incremental (5ac3a214)
• 05: Full - Incremental (43780935)
• 04: Full - Incremental (883a2dc1)
• 03: Full - Incremental (ebb09f8d)
• 02: Full - Incremental (b3931507)
• 01: Full - Incremental (e453380b)
• 00: Full (c9ba84c7)

[vicente-romero-oracle]

CSR needed I guess

[SirYwell]

CSR needed I guess

I don't have access to the JBS. Could you help me with this?

[TheShermanTanker]

CSR needed I guess

I don't have access to the JBS. Could you help me with this?

No problem! ~~https://bugs.openjdk.org/browse/JDK-8292431~~ EDIT: Mistakenly created a broken entry, the correct one is at https://bugs.openjdk.org/browse/JDK-8292467

[TheShermanTanker]

/csr needed

[openjdk[bot]]

@TheShermanTanker only the pull request author and Reviewers are allowed to use the csr command.

[SirYwell]

/csr needed

[openjdk[bot]]

@SirYwell has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@SirYwell please create a CSR request for issue JDK-8292275 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

[magicus]

/label -build

[openjdk[bot]]

@magicus The build label was successfully removed.

[SirYwell]

Hi, I addressed your comments. The annotation is definitely a cleaner solution, thank you for that suggestion.

[bridgekeeper[bot]]

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

[SirYwell]

Hi, is there anything else that needs to be done from my side?

[bridgekeeper[bot]]

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

[bridgekeeper[bot]]

@SirYwell This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

[SirYwell]

/open

[openjdk[bot]]

@SirYwell This pull request is now open

[SirYwell]

Hi, I merged master recently and updated the copyright year. Is there anything else I can do to get the CSR approved and my changes integrated?

[bridgekeeper[bot]]

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

[SirYwell]

Still waiting for any kind of feedback.

[jddarcy]

@vicente-romero-oracle , can you review the CSR for this? Thanks.

[vicente-romero-oracle]

@vicente-romero-oracle , can you review the CSR for this? Thanks.

sure, I added some suggestions in a comment in the CSR

[SirYwell]

@vicente-romero-oracle thank you for the feedback. As I don't have a JBS account, I can't incorporate it myself. @TheShermanTanker could you do that for me as you created the CSR? Thanks.

[vicente-romero-oracle]

@vicente-romero-oracle thank you for the feedback. As I don't have a JBS account, I can't incorporate it myself. @TheShermanTanker could you do that for me as you created the CSR? Thanks.

I will incorporate the changes to the CSR, please let me know if you have any suggestions

[SirYwell]

I will incorporate the changes to the CSR, please let me know if you have any suggestions

THanks again. I don't have any more suggestions.

[liach]

Just checking on this. the CSR has a response:

Please refine the discussion in "Solution" to explicitly state which --release/-target versions are intended to be impacted by the change. Presumably, at a minimum the synthetic and mandated information would only be written to class file formats where those bits are defined.

A more conservative form of this change would be to only change the behavior for --release/-target 21. However, given the low expected impact of making the change, I'd expect it would be fine to make the change in behavior for all class file versions where that information is supported.

Our current implementation applies the change of behavior to all --release/-target versions. And as far as I see, MethodParameters attribute has been properly supported by the Reflection API to filter out synthetic and mandated parameters since 1c54a00f752a54a7a68b09d3df5d3a6081f4fcc8 (Java 8, when MethodParameters is introduced). So I suggest to move along by specifying that this change applies to all release/target versions, which will fix the reflection issue on all versions.

It ultimately depends on @SirYwell to decide what to do here. I can help you update the CSR's Solution Section as you desire, as I am eager to see this bug fixed on javac's end as well.

[liach]

This appear related to https://bugs.openjdk.org/browse/JDK-8213329 as well, for this patch makes javac emit parameter attributes reliably.

[SirYwell]

Sorry for the late response.

Our current implementation applies the change of behavior to all --release/-target versions. And as far as I see, MethodParameters attribute has been properly supported by the Reflection API to filter out synthetic and mandated parameters since 1c54a00 (Java 8, when MethodParameters is introduced). So I suggest to move along by specifying that this change applies to all release/target versions, which will fix the reflection issue on all versions.

I agree with this, I'd be happy if you could update the CSR accordingly.

I will try to address your review comment as soon as possible.