Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

SirYwell
Copy link
Member

@SirYwell SirYwell commented Aug 12, 2022

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
  • Commit message must refer to an issue
  • Change requires CSR request JDK-8292467 to be approved

Issues

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

Reviewers

Contributors

  • Chen Liang <liach@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git 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.git 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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 12, 2022

👋 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
Copy link

openjdk bot commented Aug 12, 2022

@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.

@openjdk openjdk bot added build build-dev@openjdk.org compiler compiler-dev@openjdk.org labels Aug 12, 2022
@openjdk openjdk bot added the rfr Pull request is ready for review label Aug 12, 2022
@mlbridge
Copy link

mlbridge bot commented Aug 12, 2022

import toolbox.TestRunner;
import toolbox.ToolBox;

public class ImplicitParameters extends TestRunner {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to this test, an annotation processing-based test could also be attempted here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate on that? Should I add a processor to the JavacTask in this test or write a separate one? (Maybe there is a similar test I can use as reference?)
Thank you.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate on that? Should I add a processor to the JavacTask in this test or write a separate one? (Maybe there is a similar test I can use as reference?) Thank you.

There are tests that can be used as a model around

test/langtools/tools/javac/processing/model/util/elements

HTH

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, it looks like there is no way to access the synthetic/mandated information using the annotation processing API. Am I missing something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, it looks like there is no way to access the synthetic/mandated information using the annotation processing API. Am I missing something?

For reasons of providing a minimal core language model that could be reused, less essential functionality is split in the language model API to be provided by an implementation of javax.lang.model.util.Elements.

So if you subclass JavacTestingAbstractProcessor files that test directory, you could use

eltUtils.getOrigin(elementForParameter)

to see if an element is marked as javax.lang.model.util.Enum Elements.Origin.{MANDATED, SYNTHETIC}.

Many of the annotation processing tests use a pattern of using a runtime-retention annotation to encode what the expected answer is. In this case, I'd recommend a method/constructor annotation to hold the expected origin of the parameters.

(The annotation processing API does filter out some SYNTHETIC constructs, but I don't recall if that is done at a parameter level.)

HTH

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I started working on a test, but the results are somewhat inconsistent (no implicit parameter is present for inner class ctors, no synthetic parameters for enum ctors, parameter of valueOf is MANDATED, implicit parameters in compact record ctor are MANDATED).
I'm not sure how we want to proceed with that or if there are known issues about that, but it might be better to tackle that separately. What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I started working on a test, but the results are somewhat inconsistent (no implicit parameter is present for inner class ctors, no synthetic parameters for enum ctors, parameter of valueOf is MANDATED, implicit parameters in compact record ctor are MANDATED). I'm not sure how we want to proceed with that or if there are known issues about that, but it might be better to tackle that separately. What do you think?

At least some of that behavior is expected/not surprising given the current state of javac's implementation. I linked the existing bug related bug

JDK-8250919: Mark compiler-generated elements as mandated

to JDK-8292275 in JBS. When possible, arguably javac should present the "source view" of a construct, so it would omit, say, the synthetic parameters javac chooses to add to enum constructors.

I suggest including a test for the mandated structures that "work" and the test could be expanded as that property is tracked and used more uniformly.

Thanks and HTH

@@ -4020,7 +4020,7 @@ protected JCClassDecl recordDeclaration(JCModifiers mods, Comment dc) {
for (JCVariableDecl param : headerFields) {
tmpParams.add(F.at(param)
// we will get flags plus annotations from the record component
.VarDef(F.Modifiers(Flags.PARAMETER | Flags.GENERATED_MEMBER | param.mods.flags & Flags.VARARGS,
.VarDef(F.Modifiers(Flags.PARAMETER | Flags.GENERATED_MEMBER | Flags.MANDATED | param.mods.flags & Flags.VARARGS,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the use of MANDATED be conditional on the target class file version?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I advise also writing a core reflection test that uses access flags (java.lang.reflect.AccessFlag).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the use of MANDATED be conditional on the target class file version?

I don't think this is needed. The JLS mentions the parameters of canonical record constructors in Java 14 already. Previous versions don't have records and shouldn't enter that method at all.

I advise also writing a core reflection test that uses access flags (java.lang.reflect.AccessFlag).

Will do. I assume the jdk/java/lang/reflect/AccessFlag folder is a good place for that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the use of MANDATED be conditional on the target class file version?

I don't think this is needed. The JLS mentions the parameters of canonical record constructors in Java 14 already. Previous versions don't have records and shouldn't enter that method at all.

Agreed; I double-checked and the ACC_MANDATED flag was defined in that location since before JDK 14's format.

I advise also writing a core reflection test that uses access flags (java.lang.reflect.AccessFlag).

Will do. I assume the jdk/java/lang/reflect/AccessFlag folder is a good place for that?

Yes; that an appropriate home for such tests.

@openjdk openjdk bot removed the rfr Pull request is ready for review label Aug 13, 2022
@vicente-romero-oracle
Copy link
Contributor

CSR needed I guess

@SirYwell
Copy link
Member Author

CSR needed I guess

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

@openjdk openjdk bot added the rfr Pull request is ready for review label Aug 16, 2022
@TheShermanTanker
Copy link
Contributor

TheShermanTanker commented Aug 16, 2022

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
Copy link
Contributor

/csr needed

@openjdk
Copy link

openjdk bot commented Aug 16, 2022

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

@openjdk
Copy link

openjdk bot commented Apr 28, 2023

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

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

Co-authored-by: Chen Liang <liach@openjdk.org>
Reviewed-by: vromero, jwaters

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 508 new commits pushed to the master branch:

  • 6d6d00b: 8306954: Open source five Focus related tests
  • bb7608b: 8307088: Allow the jdbc.drivers system property to be searchable
  • a2d3fc8: 8304837: Classfile API throws IOOBE for MethodParameters attribute without parameter names
  • d43a5a2: 8307135: java/awt/dnd/NotReallySerializableTest/NotReallySerializableTest.java failed
  • 1f68924: 8306955: Open source several JComboBox jtreg tests
  • b8de394: 8307080: Open source some more JComboBox jtreg tests
  • 4818c79: 8307110: zero build broken after JDK-8304265
  • da9efee: 8296935: Arrays.asList() can return a List that throws undocumented ArrayStoreException
  • 05af487: 8306681: Open source more AWT DnD related tests
  • ec5c792: 8306133: Open source few AWT Drag & Drop related tests
  • ... and 498 more: https://git.openjdk.org/jdk/compare/ddf1e34c1a0815e8677212f1a7860ca7cf9fc2c9...master

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. Possible candidates are the reviewers of this PR (@vicente-romero-oracle, @TheShermanTanker) but any other Committer may sponsor as well.

➡️ 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).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Apr 28, 2023
@vicente-romero-oracle
Copy link
Contributor

please remember to wait for #13167 to be integrated before integrating this one

Co-authored-by: liach <7806504+liach@users.noreply.github.com>
@SirYwell
Copy link
Member Author

Switched to the ternary operator as suggested, thanks for your review @vicente-romero-oracle.

@SirYwell
Copy link
Member Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Apr 29, 2023
@openjdk
Copy link

openjdk bot commented Apr 29, 2023

@SirYwell
Your change (at version 400e1ec) is now ready to be sponsored by a Committer.

@TheShermanTanker
Copy link
Contributor

Windows Failures are due to a longstanding (and annoying) build system bug that's been around for quite a while by this point, and aren't related to this PR in any way

Shall I do the honours?

@liach
Copy link
Member

liach commented Apr 29, 2023

Windows Failures are due to a longstanding (and annoying) build system bug that's been around for quite a while by this point, and aren't related to this PR in any way

Shall I do the honours?

If it's the vscode toolchain one, go for it. It fixes itself on github from time to time, and definitely doesn't break anything when built by openjdk or adoptium.

@TheShermanTanker
Copy link
Contributor

Alright, here goes

@TheShermanTanker
Copy link
Contributor

/sponsor

@openjdk
Copy link

openjdk bot commented Apr 30, 2023

Going to push as commit b3dbf28.
Since your change was applied there have been 508 commits pushed to the master branch:

  • 6d6d00b: 8306954: Open source five Focus related tests
  • bb7608b: 8307088: Allow the jdbc.drivers system property to be searchable
  • a2d3fc8: 8304837: Classfile API throws IOOBE for MethodParameters attribute without parameter names
  • d43a5a2: 8307135: java/awt/dnd/NotReallySerializableTest/NotReallySerializableTest.java failed
  • 1f68924: 8306955: Open source several JComboBox jtreg tests
  • b8de394: 8307080: Open source some more JComboBox jtreg tests
  • 4818c79: 8307110: zero build broken after JDK-8304265
  • da9efee: 8296935: Arrays.asList() can return a List that throws undocumented ArrayStoreException
  • 05af487: 8306681: Open source more AWT DnD related tests
  • ec5c792: 8306133: Open source few AWT Drag & Drop related tests
  • ... and 498 more: https://git.openjdk.org/jdk/compare/ddf1e34c1a0815e8677212f1a7860ca7cf9fc2c9...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Apr 30, 2023
@openjdk openjdk bot closed this Apr 30, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Apr 30, 2023
@openjdk
Copy link

openjdk bot commented Apr 30, 2023

@TheShermanTanker @SirYwell Pushed as commit b3dbf28.

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

@liach
Copy link
Member

liach commented Apr 30, 2023

Nice! One of the most troubling java reflection bugs will be no more:

public enum TestEnum {
    ;
    TestEnum(Optional<String> genericParameter) {}
}

with this, TestEnum.class.getDeclaredConstructors()[0].getParameters()[2].getParameterizedType() will return java.util.Optional<java.lang.String> than class java.util.Optional, which is why I've been eager for this patch.

@TheShermanTanker
Copy link
Contributor

Haha, glad to have been of help

Now, if only someone would also take a look at my javac patch for the JNI header generator... :(

@vicente-romero-oracle
Copy link
Contributor

good to see this one done, thanks to all!

@vicente-romero-oracle
Copy link
Contributor

Haha, glad to have been of help

Now, if only someone would also take a look at my javac patch for the JNI header generator... :(

what's the PR for that one?

@TheShermanTanker
Copy link
Contributor

Haha, glad to have been of help
Now, if only someone would also take a look at my javac patch for the JNI header generator... :(

what's the PR for that one?

Ah, sorry for the trouble, I was mildly joking for that one, but the corresponding PR is #13457

@vicente-romero-oracle
Copy link
Contributor

Haha, glad to have been of help
Now, if only someone would also take a look at my javac patch for the JNI header generator... :(

what's the PR for that one?

Ah, sorry for the trouble, I was mildly joking for that one, but the corresponding PR is #13457

I see, it seems like it is already being looked at

@TheShermanTanker
Copy link
Contributor

It was yeah, Jan hasn't come back to reviewing it in quite a while unfortunately :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler compiler-dev@openjdk.org integrated Pull request has been integrated
6 participants