RFR: 8337980: Javac allows invocation of an inherited instance method from a static method [v2]

[Maurizio Cimadamore]

On Fri, 6 Sep 2024 16:34:36 GMT, Archie Cobbs <acobbs at openjdk.org> wrote:

>> The method `Resolve.findFun()` performs the lookup for an unqualified method invocation like `foo()`. It does this by invoking `findMethod()` to find the method in each containing class scope, working from the inside out.
>> 
>> In this loop, as soon as it finds a matching method, but before returning, it performs the checks that prevent (a) invoking an instance method from a static context, and (b) invoking an instance method of the same class from an early-initialization context.
>> 
>> However, these checks don't account for the fact that in some cases `findMethod()` can return an `AmbiguityError` even though the code is perfectly valid. This is because `findMethod()` performs the "maximally specific" logic of JLS §15.12.2.5 ("Choosing the Most Specific Method"), but not the "preferred method" logic. Instead, in the case there are multiple "maximally specific" methods, they are all returned wrapped in an `AmbiguityError`; disambiguation is the caller's job.
>> 
>> When this happens, if the code is actually valid, there's no problem, but if the code has one of the errors (a) or (b), the error does not get detected, with resulting downstream chaos.
>> 
>> Here's an example of when `findMethod()` can return an `AmbiguityError` for perfectly valid code:
>> 
>> public class Bug {
>> 
>>     public interface A {
>>         int op();
>>     }
>> 
>>     public abstract static class B {
>>         public abstract int op();
>>     }
>> 
>>     public abstract static class C extends B implements A {
>> 
>>         public int test() {
>>             return op();   // findMethod("op") returns both A.op() and B.op() here
>>         }
>>     }
>> }
>> 
>> The fix in this PR is to add a few lines to `findFun()` to resolve the ambiguity (if possible) before performing checks (a) and (b). It's possible there is an opportunity for a more thorough refactoring here, but short of that, I tried to add a few helpful comments.
>
> Archie Cobbs has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision:
> 
>  - Merge branch 'master' into JDK-8337980
>  - Avoid crash caused by failure to resolve a valid AmbiguityError.

I've double checked the code again, and I think I see where I made a wrong assumption. The "early construction" check in the code is based on the `env`. As such, it does not depend on whatever symbol is returned by `mergeAbstracts`. The code in this PR is ok.

Ultimately, this bug has to do with `findMethod` being called in a place (`findFun`) that does not follow up with the correct call to `mergeAbstract` (as this call is typically part of the `lookup` method in a `LookupHelper` - see e.g. `resolveMethod/resolveQualifiedMethod`).

What I'm a bit worried about with this fix is that it results in `mergeAbstracts` to be called *twice* when the code is correct. This seems bad, as `mergeAbstracts` is quite a complex routine. Ideally, `findFun` should return immediately if the result of `findMethod` "exists", and defer the check for static/early construction context to _after_ (e.g. when `mergeAbstracts` is normally called). Which I realize is challenging because by the time we call `mergeAbstract` outside `findFun`, we no longer have the `env` in which the symbol was found...

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

PR Comment: https://git.openjdk.org/jdk/pull/20533#issuecomment-2399774573