RFR: 8272564: Incorrect attribution of method invocations of Object methods on interfaces [v2]

[Maurizio Cimadamore]

On Thu, 19 Aug 2021 12:56:58 GMT, Jan Lahoda <jlahoda at openjdk.org> wrote:

>> Consider these declarations:
>> 
>>     interface I {
>>         public String toString();
>>     }
>>     interface J extends I {}
>> 
>> 
>> There are two issues with the `toString` inherited from `I` into `J`:
>> -`Trees.isAccessible(..., I.toString, J)` will return false, because `Resolve.isAccessible` will return false, because `Resolve.notOverriddenIn` returns false, because the `Object.toString` method is found as an implementation of `I.toString` in the context of `J`. This is unfortunate, because `Elements.getAllMembers(J)` will return `I.toString` as a member, not `Object.toString`, so any API client listing all members and then validating them using `Trees.isAccessible` will filter `toString` out. The proposed solution is to avoid using the methods from `java.lang.Object` as implementations of interface methods in `Resolve.notOverriddenIn`. (Interfaces don't inherit from `j.l.Object` per my understanding of JLS.)
>> -as a slightly less problematic case, consider:
>> 
>> I i = null;
>> i.toString(); //AST and classfile contain call to I.toString()
>> J j = null;
>> j.toString(); //AST and classfile contain call to j.l.Object.toString()
>> 
>> 
>> I believe the second invocation should also preferably be a call to `I.toString()`, not a call to `j.l.Object.toString()`. The reason for this behavior is that in javac, interfaces have `j.l.Object` as a supertype, so method lookups over interfaces will look into `j.l.Object` before looking into the superinterfaces, and the concrete method will prevail over the super interface method found later. The proposal is, for interfaces, to only look into `j.l.Object` is a method is not found in the interface and its superinterfaces.
>
> Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Removing unnecessary method call.

The implementation logic seems to be moving in the right direction.It is true that interface do not inherit members for `Object` - but the JLS is set up in a strange way:


 If an interface has no direct superinterface types, then the interface implicitly declares a public abstract member method m with signature s, return type r, and throws clause t corresponding to each public instance method m with signature s, return type r, and throws clause t declared in Object (§4.3.2), unless an abstract method with the same signature, same return type, and a compatible throws clause is explicitly declared by the interface. 
 ```
 
 This seems to suggest that resolution should return `I.toString` even if `I` does NOT declare a `toString` method. That makes sense, after all, implementations of `I` will _always_ have some toString(). This is not what your fallback Object lookup in `findMethod` does.

I also suggest running benchmarks, as the effect of these changes would be to generate invokeinterface where we used to have invokevirtual. VM should be smart enough to figure that one out, but better check.

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java line 1858:

> 1856:         InterfaceLookupPhase iphase = InterfaceLookupPhase.ABSTRACT_OK;
> 1857:         boolean isInterface = site.tsym.isInterface();
> 1858:         for (TypeSymbol s : isInterface ? List.of(intype.tsym) : superclasses(intype)) {

I wonder if the code here is making it more difficult because we include the initial type into `superclasses`. Maybe it could be worth to have an initial round where we just look into the initial type - followed by an (optional) round where we look at superclasses, followed by a round where we look at superinterfaces, followed by a fallback (optional) round where we look at Object.

-------------

PR: https://git.openjdk.java.net/jdk/pull/5165