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

8236842: Surprising 'multiple elements' behaviour from getTypeElement when cross-compiling with --release #200

Closed
wants to merge 5 commits into from

Conversation

@lahodaj
Copy link
Contributor

@lahodaj lahodaj commented Sep 16, 2020

Unqualified Elements.getTypeElement(CharSequence) and Elements.getPackageElement(CharSequence) search for elements across all modules in the module graph, and only return a value when they find exactly one element. This is troublesome, as an element (uniquely) visible from a root module may be "hidden" by an element that is not visible from any root module (i.e. is internal to some module that is not in the root module set). The idea proposed here is that these unqualified methods would first look for elements visible from the root modules, and would search the internals of other modules only if nothing would be found in the first round.

The draft of the corresponding CSR is here: https://bugs.openjdk.java.net/browse/JDK-8253168.

/csr JDK-8253168


Progress

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

Testing

Linux x64 Linux x86 Windows x64 macOS x64
Build ✔️ (5/5 passed) ✔️ (2/2 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ✔️ (9/9 passed) (1/9 running) ✔️ (9/9 passed)

Issue

  • JDK-8236842: Surprising 'multiple elements' behaviour from getTypeElement when cross-compiling with --release

Reviewers

Download

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

… when cross-compiling with --release
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 16, 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 openjdk bot added the rfr label Sep 16, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Sep 16, 2020

@lahodaj The command csr cannot be used in the pull request body. Please use it in a new comment.

@openjdk
Copy link

@openjdk openjdk bot commented Sep 16, 2020

@lahodaj The following label will be automatically applied to this pull request: compiler.

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

@openjdk openjdk bot added the compiler label Sep 16, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 16, 2020

@lahodaj
Copy link
Contributor Author

@lahodaj lahodaj commented Sep 16, 2020

/csr JDK-8253168

@openjdk
Copy link

@openjdk openjdk bot commented Sep 16, 2020

@lahodaj usage: /csr [needed|unneeded], requires that the issue the pull request refers to links to an approved CSR request.

@lahodaj
Copy link
Contributor Author

@lahodaj lahodaj commented Sep 16, 2020

/csr needed

@openjdk openjdk bot added the csr label Sep 16, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Sep 16, 2020

@lahodaj has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@lahodaj please create a CSR request and add link to it in JDK-8253168. This pull request cannot be integrated until the CSR request is approved.

@lahodaj lahodaj changed the title 8253168: Surprising 'multiple elements' behaviour from getTypeElement when cross-compiling with --release 8236842: Surprising 'multiple elements' behaviour from getTypeElement when cross-compiling with --release Sep 16, 2020
@lahodaj
Copy link
Contributor Author

@lahodaj lahodaj commented Sep 16, 2020

/csr unneeded

@openjdk openjdk bot removed the csr label Sep 16, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Sep 16, 2020

@lahodaj determined that a CSR request is no longer needed for this pull request.

@lahodaj
Copy link
Contributor Author

@lahodaj lahodaj commented Sep 16, 2020

/csr needed

@openjdk openjdk bot added the csr label Sep 16, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Sep 16, 2020

@lahodaj this pull request will not be integrated until the CSR request JDK-8253168 for issue JDK-8236842 has been approved.

Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle left a comment

I have also made some small edits to the CSR and added myself as reviewer

*
* If running with modules, packages of the given name are searched in a
* two-stage process:
* -find non-empty packages with the given name returned by {@link #getPackageElement(ModuleElement, CharSequence)}, where the provided ModuleSymbol is any root module,

This comment has been minimized.

@vicente-romero-oracle

vicente-romero-oracle Sep 16, 2020
Contributor

it would be nice to try to keep the line width as close as possible to 80 chars, same for the CSR

*
* If running with modules, type elements of the given name are searched in a
* two-stage process:
* -find type elements with the given name returned by {@link #getTypeElement(ModuleElement, CharSequence)}, where the provided ModuleSymbol is any root module,

This comment has been minimized.

@vicente-romero-oracle

vicente-romero-oracle Sep 16, 2020
Contributor

same comment as above

@@ -199,42 +200,44 @@ private ClassSymbol doGetTypeElement(ModuleElement module, CharSequence name) {
return (S) resultCache.computeIfAbsent(Pair.of(methodName, nameStr), p -> {
Set<S> found = new LinkedHashSet<>();

for (ModuleSymbol msym : modules.allModules()) {
S sym = nameToSymbol(msym, nameStr, clazz);
for (Set<ModuleSymbol> allModules : Arrays.asList(modules.getRootModules(), modules.allModules())) {

This comment has been minimized.

@vicente-romero-oracle

vicente-romero-oracle Sep 16, 2020
Contributor

does it make sense to obtain a diff between the the root modules and the all-modules set in order to avoid searching twice in the root modules if the symbol is not found there in the first try?

}
return Optional.empty();
} else {
//not found, try another option

This comment has been minimized.

@vicente-romero-oracle

vicente-romero-oracle Sep 16, 2020
Contributor

probably this else {} can be removed right?

This comment has been minimized.

@lahodaj

lahodaj Sep 17, 2020
Author Contributor

It could be removed, but I added it intentionally to make it clear the "else" option has been considered, and is intentional. I can remove it, if you strongly prefer, though.

… of modules that have already been searched.
@lahodaj
Copy link
Contributor Author

@lahodaj lahodaj commented Sep 17, 2020

@vicente-romero-oracle thanks for the comments! I've updated the patch and CSR based on the comments.

@vicente-romero-oracle
Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle commented Sep 17, 2020

I'm OK with the second iteration, thanks for doing the changes! approved
Vicente

@openjdk
Copy link

@openjdk openjdk bot commented Sep 17, 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:

8236842: Surprising 'multiple elements' behaviour from getTypeElement when cross-compiling with --release

Reviewed-by: vromero

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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 Sep 17, 2020
* <ul>
* <li>find non-empty packages with the given name returned by
* {@link #getPackageElement(ModuleElement, CharSequence)},
* where the provided ModuleSymbol is any root module,

This comment has been minimized.

@jddarcy

jddarcy Sep 23, 2020
Member

Does "root module" mean one of the modules in the result of Elements.getAllModuleElements? If so, I suggest making "root module" a link to the method.

This comment has been minimized.

@lahodaj

lahodaj Sep 23, 2020
Author Contributor

Root module is meant in the java.lang.module sense, i.e. the set of modules that are being compiled. In a new revision, I added a link to the appropriate package javadoc in java.lang.module; and a link to getAllModules for "all modules" in the next point. Does this make sense?

I've generated the javadoc for convenience here:
http://cr.openjdk.java.net/~jlahoda/8236842/docsv2/api/java.compiler/javax/lang/model/util/Elements.html#getTypeElement(java.lang.CharSequence)
http://cr.openjdk.java.net/~jlahoda/8236842/docsv2/api/java.compiler/javax/lang/model/util/Elements.html#getPackageElement(java.lang.CharSequence)

@openjdk openjdk bot added the core-libs label Sep 23, 2020
@openjdk openjdk bot removed the ready label Sep 23, 2020
@openjdk openjdk bot added ready and removed csr labels Oct 29, 2020
@lahodaj
Copy link
Contributor Author

@lahodaj lahodaj commented Nov 2, 2020

/integrate

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

@openjdk openjdk bot commented Nov 2, 2020

@lahodaj Pushed as commit d05df7c.

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

openjdk-notifier bot referenced this pull request Nov 2, 2020
… when cross-compiling with --release

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