Skip to content
This repository has been archived by the owner. It is now read-only.

8231461: static/instance overload leads to 'unexpected static method found in unbound lookup' when resolving method reference #107

Closed

Conversation

@vicente-romero-oracle
Copy link

@vicente-romero-oracle vicente-romero-oracle commented Jan 12, 2021

This patch fixes the following issue, for code like:

import java.util.function.*;
 
public class Test {
    public String foo(Object o) { return "foo"; }
    public static String foo(String o) { return "bar"; }

    public void test() {
        Function<String, String> f = Test::foo;
    }
}

javac is issuing an invalid method reference error even though the static method is applicable. This is due to a regression on the implementation of the following assertion on section 15.13.1 of the JLS 14:

If the first search produces a most specific method that is static , and the set
of applicable methods produced by the second search contains no non- static
methods, then the compile-time declaration is the most specified method of
the first search.

Apart from fixing the mentioned regression, this patch also adds a debug feature for the compiler to print out the findings during the search for an applicable method reference. The information is printed out if a debug flag is passed to the compiler.

TIA


Progress

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

Issue

  • JDK-8231461: static/instance overload leads to 'unexpected static method found in unbound lookup' when resolving method reference

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk16 pull/107/head:pull/107
$ git checkout pull/107

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 12, 2021

👋 Welcome back vromero! 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.

Loading

@openjdk openjdk bot added the rfr label Jan 12, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 12, 2021

@vicente-romero-oracle 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 pull request command.

Loading

@openjdk openjdk bot added the compiler label Jan 12, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 12, 2021

Webrevs

Loading

Copy link
Contributor

@mcimadamore mcimadamore left a comment

The debugging logic seems good, but I don't get how the fix is supposed to work, since it seems to replicate the behavior that's already there. I'm probably missing something.

Loading

@@ -3280,12 +3324,12 @@ ReferenceLookupResult boundResult(ReferenceLookupResult boundRes) {

@Override
ReferenceLookupResult unboundResult(ReferenceLookupResult boundRes, ReferenceLookupResult unboundRes) {
if (boundRes.hasKind(StaticKind.STATIC) &&
if (boundRes.isSuccess() && boundRes.sym.isStatic() &&
Copy link
Contributor

@mcimadamore mcimadamore Jan 12, 2021

Choose a reason for hiding this comment

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

For consistency here I'd prefer to see boundRes.hasKind(StaticKind.STATIC) (which is set if the underlying symbol is static). E.g. I think the fix here is really adding the isSuccess part, not changing the static check.

Loading

Copy link
Contributor

@mcimadamore mcimadamore Jan 12, 2021

Choose a reason for hiding this comment

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

That said - I'm having issue understanding how this has been fixed: if the fix search succeeds and the second search fails to produce a non-static method, then the first search should be preferred. It seems to me that the old code already covers that? Note that isSuccess is true whenever the kind != UNDEFINED, so if we check that kind should be STATIC, we already check that is a successful lookup and that the result is static. In other words, I don't see how the new code is different from the old?

Loading

Copy link
Author

@vicente-romero-oracle vicente-romero-oracle Jan 12, 2021

Choose a reason for hiding this comment

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

I guess that the difference between the new code and the previous one is that in the old code when we check if boundRes.hasKind(StaticKind.STATIC) we are checking for a global property of all candidates, meaning that for the previous check to be true, all candidates have to be static. And this is not what the spec is saying, as the spec cares about the staticness, or lack of it, of the most specific method found in each search. Does this make sense?

Loading

Copy link
Contributor

@mcimadamore mcimadamore Jan 12, 2021

Choose a reason for hiding this comment

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

Ok - I missed that both symbols are applicable in the first search. So while the most specific symbol is the static one, the StaticKind is set to BOTH, since one is static and the other is not. So, yes, using StaticKind for things like "the first/second search returned a static/non-static method" is incorrect. Your patch fixes that.

I'm dubious as to whether isSuccess is correct though. E.g. isSuccess returns true whenever the StaticKind is != UNDEFINED - but that would also return true in cases where there's an ambiguity in the first search - which I would not describe as a success. Do you agree? If so, can we add a test for that case (e.g. a case where the first search has ambiguous result) and see what happens?

Loading

Choose a reason for hiding this comment

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

I have uploaded a new commit based on your comments and offline discussion, TIA

Loading

Copy link
Contributor

@mcimadamore mcimadamore left a comment

Looks good

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jan 12, 2021

@vicente-romero-oracle 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:

8231461: static/instance overload leads to 'unexpected static method found in unbound lookup' when resolving method reference

Reviewed-by: mcimadamore

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

  • 6bb6093: 8259657: typo in generated HELP page prevents localization
  • 5567530: 8258272: LoadVectorMaskedNode can't be replaced by zero con
  • a99df45: 8259560: Zero m68k: "static assertion failed: align" after JDK-8252049
  • efc36be: 8258985: Parallel WeakProcessor may use too few threads
  • 417e1d1: 8259061: C2: assert(found) failed: memory-writing node is not placed in its original loop or an ancestor of it
  • 793017d: 8259601: AArch64: Fix reinterpretX2D match rule issue
  • 15dd8f3: 8259275: JRuby crashes while resolving invokedynamic instruction
  • 1cf2378: 8259353: VectorReinterpretNode is incorrectly optimized out
  • 17b4db3: 8259636: Check for buffer backed by shared segment kicks in in unexpected places
  • 5f9cd72: 8259645: Revert JDK-8245956 JavaCompiler still uses File API instead of Path API in a specific case
  • ... and 11 more: https://git.openjdk.java.net/jdk16/compare/1973fbee379b0cd5f552dfc66fce551a1e59d8a7...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

Loading

@openjdk openjdk bot added the ready label Jan 12, 2021
Copy link
Contributor

@mcimadamore mcimadamore left a comment

Looks good

Loading

@vicente-romero-oracle
Copy link
Author

@vicente-romero-oracle vicente-romero-oracle commented Jan 13, 2021

/integrate

Loading

@openjdk openjdk bot closed this Jan 13, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jan 13, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 13, 2021

@vicente-romero-oracle Since your change was applied there have been 22 commits pushed to the master branch:

  • 42d2d6d: 8259063: Possible deadlock with vtable/itable creation vs concurrent class unloading
  • 6bb6093: 8259657: typo in generated HELP page prevents localization
  • 5567530: 8258272: LoadVectorMaskedNode can't be replaced by zero con
  • a99df45: 8259560: Zero m68k: "static assertion failed: align" after JDK-8252049
  • efc36be: 8258985: Parallel WeakProcessor may use too few threads
  • 417e1d1: 8259061: C2: assert(found) failed: memory-writing node is not placed in its original loop or an ancestor of it
  • 793017d: 8259601: AArch64: Fix reinterpretX2D match rule issue
  • 15dd8f3: 8259275: JRuby crashes while resolving invokedynamic instruction
  • 1cf2378: 8259353: VectorReinterpretNode is incorrectly optimized out
  • 17b4db3: 8259636: Check for buffer backed by shared segment kicks in in unexpected places
  • ... and 12 more: https://git.openjdk.java.net/jdk16/compare/1973fbee379b0cd5f552dfc66fce551a1e59d8a7...master

Your commit was automatically rebased without conflicts.

Pushed as commit ac4cd2e.

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

Loading

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
2 participants