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

JDK-8256950: Add record attribute support to symbol generator CreateSymbols #1480

Closed
wants to merge 13 commits into from

Conversation

@lahodaj
Copy link
Contributor

@lahodaj lahodaj commented Nov 27, 2020

Adding support for record classes in the historical data for ct.sym. This includes a few changes not strictly needed for the change:
-updating and moving tests into test/langtools, so that it is easier to run them.
-fixing Record attribute reading in javac's ClassReader (used for tests, but seems like the proper thing to do anyway).
-fixing the -Xprint annotation processor to print record component annotations.

Changes to jdk.jdeps' classfile library are needed so that the ct.sym creation works.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8256950: Add record attribute support to symbol generator CreateSymbols

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1480/head:pull/1480
$ git checkout pull/1480

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 27, 2020

👋 Welcome back jlahoda! 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 openjdk bot commented Nov 27, 2020

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

  • build
  • compiler
  • core-libs

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 the rfr label Nov 27, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 27, 2020

@@ -959,6 +962,21 @@ private void addAttributes(ClassHeaderDescription header,
attributes.put(Attribute.NestMembers,
new NestMembers_attribute(attributeString, nestMembers));
}
if (header.recordComponents != null && !header.recordComponents.isEmpty()) {

This comment has been minimized.

@ChrisHegarty

ChrisHegarty Nov 30, 2020
Member

I am not sure of the exact logic here, but it is perfectly fine for a record attribute to contain zero components, and for the class to still be considered a "record class". But maybe that is not all that significant here? I just want to call it out so that it is considered.

This comment has been minimized.

@lahodaj

lahodaj Nov 30, 2020
Author Contributor

Ah, right - fixed in:
e1ec2b7

Thanks for the comment!

@openjdk
Copy link

@openjdk openjdk bot commented Nov 30, 2020

⚠️ @lahodaj This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

Copy link
Contributor

@jonathan-gibbons jonathan-gibbons left a comment

Looks mostly OK; some minor comments inline.

"--add-exports", "jdk.javadoc/jdk.javadoc.internal.api=ALL-UNNAMED",
"--add-exports", "jdk.javadoc/jdk.javadoc.internal.tool=ALL-UNNAMED",
Comment on lines 109 to 110

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons Dec 1, 2020
Contributor

I'm surprised CreateSymbolsTest needs access to javadoc internals; is it because you're adding all of toolbox in lines 93, 94? Would it be better to filter out the classes you don't want to compile, such as JavadocTask and JavapTask ?

import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import toolbox.JavacTask;
import toolbox.Task;
import toolbox.Task.Expect;
import toolbox.ToolBox;
import build.tools.symbolgenerator.CreateSymbols;
import build.tools.symbolgenerator.CreateSymbols.ClassDescription;
import build.tools.symbolgenerator.CreateSymbols.ClassList;
import build.tools.symbolgenerator.CreateSymbols.CtSymKind;
import build.tools.symbolgenerator.CreateSymbols.ExcludeIncludeList;
import build.tools.symbolgenerator.CreateSymbols.VersionDescription;
import java.util.Enumeration;
import java.util.jar.JarEntry;
import java.util.jar.JarFile;
Comment on lines 40 to 54

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons Dec 1, 2020
Contributor

weird order of imports

"}\n",
"t.Visible",
"package t;\n\n" +
"@java.lang.annotation.Retention(java.lang.annotation.RetentionPolicy.RUNTIME)\n" +
// "@java.lang.annotation.Retention(java.lang.annotation.RetentionPolicy.RUNTIME)\n" + //XXX

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons Dec 1, 2020
Contributor

Is this line still supposed to be here?

@@ -261,7 +267,8 @@ void testAnnotations() throws Exception {
"}\n",
"t.Visible",
"package t;\n\n" +
"@java.lang.annotation.Retention(java.lang.annotation.RetentionPolicy.RUNTIME)\n" +
// "@java.lang.annotation.Retention(java.lang.annotation.RetentionPolicy.RUNTIME)\n" + //XXX

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons Dec 1, 2020
Contributor

Is this line still supposed to be here?

}

@Test
// @Test XXX

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons Dec 1, 2020
Contributor

Another XXX line

"""
\n\
public static record R(int i, java.util.List<java.lang.String> l) {
\n\
public R(int i,
java.util.List<java.lang.String> l);
\n\
public final java.lang.String toString();
\n\
public final int hashCode();
\n\
public final boolean equals(java.lang.Object arg0);
\n\
public int i();
\n\
public java.util.List<java.lang.String> l();
}
""",
Comment on lines 496 to 513

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons Dec 1, 2020
Contributor

I don't understand why the lines with \n\ in a text block

This comment has been minimized.

@lahodaj

lahodaj Dec 1, 2020
Author Contributor

There is a combination of factors here:
-jcheck (AFAIK) does not allow trailing whitespaces, even not on otherwise empty lines inside textblocks
-textblocks only remove indentation that is common on all lines.

So, without having '\n', we would have to strip all the whitespace on the empty lines (to pass jcheck), which would mean the text block's content would no longer match the output. There are a few ways to solve this (almost surely an incomplete list):
-do some trick to have the common indent, but no trailing whitespace. '\n' is one of them.
-not indent the text block
-do some post-processing on the text block's value or the actual test output, to make them match
-not use textblocks

This comment has been minimized.

@lahodaj

lahodaj Dec 3, 2020
Author Contributor

I stand corrected here - blank lines do not count when the common indent is computed. Removed here:
3aaaf28

if (createSymbols == null) {
System.err.println("Warning: cannot find CreateSymbols, skipping.");
return ;
}
Comment on lines 66 to 69

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons Dec 1, 2020
Contributor

I can believe this test is worth just a warning, because you're trying to reach into the make directory, but the subsequent checks for CreateSymbolsTestImpl.java and toolbox.* seem worthy of errors, not warnings. On the other hand, I accept that all of these checks are very unlikely to fail, so maybe it doesn't matter.

@@ -25,6 +25,10 @@
* @test
* @bug 8072480
* @summary Unit test for CreateSymbols
* @modules java.compiler
* jdk.compiler
* jdk.javadoc

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons Dec 1, 2020
Contributor

See related comments in code ... seems strange to need jdk.javadoc.

).call();
if (!res) {
throw new IllegalStateException("Cannot compile test.");
}
}

URLClassLoader cl = new URLClassLoader(new URL[] {testClasses.toUri().toURL(), compileDir.toUri().toURL()});

This comment has been minimized.

@jonathan-gibbons

jonathan-gibbons Dec 1, 2020
Contributor

line 132: the name of the local variable createSymbols seems more specific than the method might imply!

Jan Lahoda
@lahodaj
Copy link
Contributor Author

@lahodaj lahodaj commented Dec 1, 2020

Thanks for the comments Jon! I've tried to resolve them here:
9ab7153

@openjdk
Copy link

@openjdk openjdk bot commented Dec 1, 2020

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

8256950: Add record attribute support to symbol generator CreateSymbols

Reviewed-by: jjg, chegar

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

  • f148915: 8245107: Use Unified Logging in trace_method_handle_stub()
  • 0201a33: 8255959: Timeouts in VectorConversion tests

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Dec 1, 2020
@lahodaj
Copy link
Contributor Author

@lahodaj lahodaj commented Dec 2, 2020

/reviewer credit jjg

@openjdk
Copy link

@openjdk openjdk bot commented Dec 2, 2020

@lahodaj Reviewer jjg has already made an authenticated review of this PR, and does not need to be credited manually.

@lahodaj
Copy link
Contributor Author

@lahodaj lahodaj commented Dec 9, 2020

/integrate

@openjdk openjdk bot closed this Dec 9, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Dec 9, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 9, 2020

@lahodaj Since your change was applied there have been 2 commits pushed to the master branch:

  • f148915: 8245107: Use Unified Logging in trace_method_handle_stub()
  • 0201a33: 8255959: Timeouts in VectorConversion tests

Your commit was automatically rebased without conflicts.

Pushed as commit 6eff931.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants