JDK-8295975 : Method reference compilation uses incorrect qualifying type
  • Type: CSR
  • Component: tools
  • Sub-Component: javac
  • Priority: P4
  • Status: Closed
  • Resolution: Approved
  • Fix Versions: 20
  • Submitted: 2022-10-27
  • Updated: 2024-01-17
  • Resolved: 2022-11-18
Related Reports
CSR :  
Relates :  
Description
Summary
-------

While translating a method reference expression of the form Primary::m, where
m() is a method declared in a super interface of the compile time type of the expression
denoted by `Primary' AND this compile time type does not implement the method
m(),  javac translates the construct with an incorrect qualifier type.

The bootstrap attribute in this case refers to the interface declaring the method, 
rather than the actual receiver type (the static type of the expression `Primary'),
for example as in

```
BootstrapMethods:
  0: #38 REF_invokeStatic java/lang/invoke/LambdaMetafactory.metafactory:(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodHandle;Ljava/lang/invoke/MethodType;)Ljava/lang/invoke/CallSite;
    Method arguments:
      #45 ()V
      #46 REF_invokeInterface X$I.m:()V    // <<<------------------------------------- 
      #45 ()V
```

It should actually refer to the compiler time receiver type as the qualifying class or interface of the method invocation as called
for JLS 13.1 (Point 5) which would result in bootstrap attribute being:

```
BootstrapMethods:
  0: #38 REF_invokeStatic java/lang/invoke/LambdaMetafactory.metafactory:(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodHandle;Ljava/lang/invoke/MethodType;)Ljava/lang/invoke/CallSite;
    Method arguments:
      #45 ()V
      #46 REF_invokeVirtual X$C.m:()V  // <<<------------------------------- 
      #45 ()V
```

Problem
-------

Javac's translation of method reference expressions in the scenario spelled out, is not 
compliant with JLS 13.1 (Point 5). Method invocation expressions are correctly handled
for the same scenario.

Solution
--------

The present fix, attempts to harmonize the translation of method reference expression with
method invocation expressions so in both cases, the qualifying class or interface of the 
method invocation is computed so as to refer to the type of the receiver rather than the
method's declaring class or interface. 

Specification
-------------

Relevant part of JLS is 13.1: Point 5.

"5. Given a method invocation expression or a method reference expression in
a class or interface C , referencing a method named m declared (or implicitly
declared (§9.2)) in a (possibly distinct) class or interface D , we define the
qualifying class or interface of the method invocation as follows:
• If D is Object then the qualifying class or interface of the method invocation
is Object .
• Otherwise:
– If the method is referenced by a simple name, then if m is a member of the
current class or interface C , let Q be C ; otherwise, let Q be the innermost
lexically enclosing class or interface declaration of which m is a member. In
either case, Q is the qualifying class or interface of the method invocation.
– If the expression is of the form TypeName .m or ReferenceType ::m ,
then the class or interface denoted by TypeName, or the erasure of
ReferenceType, is the qualifying class or interface of the method
invocation
– If the expression is of the form ExpressionName .m or Primary .m or
ExpressionName ::m or Primary ::m , then:
...
› Otherwise, the erasure of the compile-time type of ExpressionName or
Primary is the qualifying class or interface of the method invocation."


Comments
See unexpected side-effect in JDK-8300787.
23-01-2023

Moving to Approved contingent on a release note being written.
18-11-2022

Thanks Dan! Resubmitting for approval.
18-11-2022

One other notable thing about noncompliance here: because method _invocations_ generate the right bytecode (and have for years), there's a discrepancy between how method references are treated and the equivalent lambda expression. `foo::bar` follows the "actual" behavior above `() -> foo.bar()` follows the "expected" behavior above There are occasions in which javac transparently converts a method reference into a lambda expression (for idiosyncratic reasons). In that case, the method reference will follow the "expected" behavior above, but subtle changes may cause it to once again follow the "actual" behavior.
17-11-2022

Some ways in which referencing a method's declaring class rather than the erased compile-time type can affect method resolution at link time: - The declaring class is made inaccessible. Expected: still executes; actual: IAE - The compile-time type is made inaccessible. Expected: IAE; actual: still executes - A static, conflicting method is added to the subclass. Expected: ICCE; actual: still executes the super method - Super is an interface with a default method, and a matching default method gets added to a different, unrelated super of the sub. Expected: ICCE; actual: still executes the super method Lots of different variations along these lines. More broadly: there are rules for how methods are supposed to be referenced and resolved at runtime, with lots of details related to corner cases when the runtime classes do not match the compile-time classes; javac-generated bytecodes are not following those rules.
17-11-2022

I will request Dan to elaborate on "what bad thing happens / what good thing fails to happen for the current code generation idiom". Here is my input: The present defective encoding of the qualifying class or interface of the method invocation results in behavior that would be different from that which would be exhibited by a strictly conforming implementation. For example, please see test/langtools/tools/javac/lambda/methodReference/8059632/MethodRefQualifyingTypeTest.java added as a part of the PR https://github.com/openjdk/jdk/pull/10809 (https://github.com/openjdk/jdk/pull/10809/files#diff-dca10109c7d1bd2085942b477df4bf29af1fe6f3b94b95472c8fa3b87eaf4636 ) This test would crash with JDK19 compilers with java.lang.invoke.LambdaConversionException: Invalid receiver type class MethodRefQualifyingTypeTest$MethodSupplierImpl; not a subtype of implementation type interface MethodRefQualifyingTypeTest$MethodSupplier while it would pass with the fix. So, the fix produces code that avoids surprising failures at runtime and improves compliance.
17-11-2022

[~sadayapalam], please give more detail on what bad thing happens / what good thing fails to happen for the current code generation idiom. For example, is it "the wrong method gets referred to if the subtype does an override"?" Moving back to Provisional.
17-11-2022

Please let me know if any further clarifications are required. Thanks!
17-11-2022

No, I wouldn't recommend preserving the bug in old source versions. It's not tied to a spec change, and isn't something anyone is likely to be relying on. As I noted above, the impact should be limited to "the timing and nature of the exception being thrown" when a binary incompatible change occurs.
10-11-2022

1. Summary section updated with examples of current (incorrect) bootstrap attribute and the bootstrap attribute. 2. The operational impact of the change: I would say entails no significant risk. It aligns javac behavior with the JLS and results in bootstrap attribute generation of a form which is also already produced by javac in cases where the static receiver type already implements the interface method mentioned in the method reference. For example, this code triggers the incorrect behavior with JDK19 compiler: ``` public class X { interface I { void m(); } abstract class C implements I {} public void test(C arg) { Runnable r = arg::m; } } ``` producing non-compliant bootstrap attribute of the form ``` BootstrapMethods: 0: #38 REF_invokeStatic java/lang/invoke/LambdaMetafactory.metafactory:(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodHandle;Ljava/lang/invoke/MethodType;)Ljava/lang/invoke/CallSite; Method arguments: #45 ()V #46 REF_invokeInterface X$I.m:()V // <<<--------------- Wrong qualifier, should mention X$C #45 ()V ``` BUT this slightly altered piece of code (same code as above but with an implementation of m() in C) ``` public class X { interface I { void m(); } abstract class C implements I { public void m() {} } public void test(C arg) { Runnable r = arg::m; } } ``` produces *with JDK19 itself* the following bootstrap attribute: ``` BootstrapMethods: 0: #38 REF_invokeStatic java/lang/invoke/LambdaMetafactory.metafactory:(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodHandle;Ljava/lang/invoke/MethodType;)Ljava/lang/invoke/CallSite; Method arguments: #45 ()V #46 REF_invokeVirtual X$C.m:()V #45 ()V ``` which is what we endeavor to produce with the fix. So I would say the code generation is changed from one form to another which has been in vogue all along entailing minimal risk. 3. Given the resultant new attribute generation, is not really new - IMHO it is a safe fix. However I'll invite [~dlsmith]Dan to comment on this. If deemed needed, the fix can be keyed off of source 20 or higher.
10-11-2022

Moving to Provisional, not Approved. [~sadayapalam], in the summary please include what is proposed to be changed, e.g. "While translating a method reference ... to $X instead of $Y to comply with JLS $SECTION." What is the operational impact in the 40/899 cases in the JDK where the method reference is generated differently? The proposal seems to be to make this code generation change in all source levels. Should gating the change on source 20 or higher be considered?
10-11-2022

A supplementary note: where this change may have an impact is when things like class/method accessibility or method 'static' flags change between compile time and runtime (with separate compilation). I can't think of any reasonable changes that would be impacted—it's all about observed behavior after binary incompatible changes are made, particularly the timing and nature of the exception being thrown.
08-11-2022

Using the patch below: ``` diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LambdaToMethod.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LambdaToMethod.java index 7bd195701cf..d5cd12c0ea8 100644 --- a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LambdaToMethod.java +++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LambdaToMethod.java @@ -519,7 +519,13 @@ public class LambdaToMethod extends TreeTranslator { } if (init != null) { + MethodSymbol refSymWas = refSym; refSym = (MethodSymbol) types.binaryQualifier(refSym, init.type); + if (refSymWas != refSym) { + log.note(tree, Notes.BinaryQualifierDiffers(refSymWas, refSymWas.owner, refSym, refSym.owner)); + } else { + log.note(tree, Notes.BinaryQualifierUnchanged); + } } List<JCExpression> indy_args = init==null? List.nil() : translate(List.of(init), localContext.prev); diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties b/src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties index 1c6cdd5429a..4e1111991c8 100644 --- a/src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties +++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties @@ -1630,6 +1630,13 @@ compiler.note.deprecated.filename.additional=\ compiler.note.deprecated.plural.additional=\ Some input files additionally use or override a deprecated API. +# 0: symbol, 1: symbol, 2: symbol, 3: symbol +compiler.note.binary.qualifier.differs=\ + Binary qualifier of the method reference changed from {0} of {1} to {2} of {3}. + +compiler.note.binary.qualifier.unchanged=\ + Binary qualifier of the method reference stays unchanged. + # 0: file name compiler.note.removal.filename=\ {0} uses or overrides a deprecated API that is marked for removal. ``` to instrument a complete clean build of OpenJDK, triggers a total of 899 notes. Of these 859 record that binary qualifier hasn't been subjected to any change by the current fix while the remaining cases of 40/899 (4.45%) the binary qualifier has undergone a change due to the code fix. Overall, I consider the risk to be minimal since the code generation scheme has always emitted both forms - 1) where the qualifying type was indeed the receiver's type (case where the method was implemented by the receiver type) and the invocation mode is invokevirtual and 2) where the qualifying type is incorrectly emitted to mention the declaring interface (and the invocation mode is invokeinterface) So the present fix while aligning javac to JLS is not necessarily creating new code patterns not exercised by the JVM.
08-11-2022