RFR: 8337980: Javac allows invocation of an inherited instance method from a static method [v2]
Maurizio Cimadamore
mcimadamore at openjdk.org
Tue Oct 8 13:34:13 UTC 2024
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.
Something like this seems to work ok:
$ git diff
diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java
index 5bf1bc0ead1..55293535ff8 100644
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java
@@ -1853,6 +1853,10 @@ Symbol findMethod(Env<AttrContext> env,
bestSoFar,
allowBoxing,
useVarargs);
+ if (bestSoFar.kind == AMBIGUOUS) {
+ AmbiguityError a_err = (AmbiguityError)bestSoFar.baseSymbol();
+ bestSoFar = a_err.mergeAbstracts(site);
+ }
return bestSoFar;
}
// where
@@ -2757,7 +2761,7 @@ Symbol resolveMethod(DiagnosticPosition pos,
return lookupMethod(env, pos, env.enclClass.sym, resolveMethodCheck,
new BasicLookupHelper(name, env.enclClass.sym.type, argtypes, typeargtypes) {
@Override
- Symbol doLookup(Env<AttrContext> env, MethodResolutionPhase phase) {
+ Symbol lookup(Env<AttrContext> env, MethodResolutionPhase phase) {
return findFun(env, name, argtypes, typeargtypes,
phase.isBoxingRequired(),
phase.isVarargsRequired());
@@ -2789,7 +2793,7 @@ private Symbol resolveQualifiedMethod(MethodResolutionContext resolveContext,
List<Type> typeargtypes) {
return lookupMethod(env, pos, location, resolveContext, new BasicLookupHelper(name, site, argtypes, typeargtypes) {
@Override
- Symbol doLookup(Env<AttrContext> env, MethodResolutionPhase phase) {
+ Symbol lookup(Env<AttrContext> env, MethodResolutionPhase phase) {
return findMethod(env, site, name, argtypes, typeargtypes,
phase.isBoxingRequired(),
phase.isVarargsRequired());
@@ -2913,7 +2917,7 @@ private Symbol resolveConstructor(MethodResolutionContext resolveContext,
List<Type> typeargtypes) {
return lookupMethod(env, pos, site.tsym, resolveContext, new BasicLookupHelper(names.init, site, argtypes, typeargtypes) {
@Override
- Symbol doLookup(Env<AttrContext> env, MethodResolutionPhase phase) {
+ Symbol lookup(Env<AttrContext> env, MethodResolutionPhase phase) {
return findConstructor(pos, env, site, argtypes, typeargtypes,
phase.isBoxingRequired(),
phase.isVarargsRequired());
@@ -2972,7 +2976,7 @@ Symbol resolveDiamond(DiagnosticPosition pos,
return lookupMethod(env, pos, site.tsym, resolveMethodCheck,
new BasicLookupHelper(names.init, site, argtypes, typeargtypes) {
@Override
- Symbol doLookup(Env<AttrContext> env, MethodResolutionPhase phase) {
+ Symbol lookup(Env<AttrContext> env, MethodResolutionPhase phase) {
return findDiamond(pos, env, site, argtypes, typeargtypes,
phase.isBoxingRequired(),
phase.isVarargsRequired());
@@ -3503,18 +3507,6 @@ abstract class BasicLookupHelper extends LookupHelper {
super(name, site, argtypes, typeargtypes, maxPhase);
}
- @Override
- final Symbol lookup(Env<AttrContext> env, MethodResolutionPhase phase) {
- Symbol sym = doLookup(env, phase);
- if (sym.kind == AMBIGUOUS) {
- AmbiguityError a_err = (AmbiguityError)sym.baseSymbol();
- sym = a_err.mergeAbstracts(site);
- }
- return sym;
- }
-
- abstract Symbol doLookup(Env<AttrContext> env, MethodResolutionPhase phase);
-
@Override
Symbol access(Env<AttrContext> env, DiagnosticPosition pos, Symbol location, Symbol sym) {
if (sym.kind.isResolutionError()) {
@@ -3561,10 +3553,6 @@ ReferenceLookupHelper unboundLookup(InferenceContext inferenceContext) {
abstract JCMemberReference.ReferenceKind referenceKind(Symbol sym);
Symbol access(Env<AttrContext> env, DiagnosticPosition pos, Symbol location, Symbol sym) {
- if (sym.kind == AMBIGUOUS) {
- AmbiguityError a_err = (AmbiguityError)sym.baseSymbol();
- sym = a_err.mergeAbstracts(site);
- }
//skip error reporting
return sym;
}
(not saying you have to use this, mostly trying to explain more clearly what I meant with the above)
-------------
PR Comment: https://git.openjdk.org/jdk/pull/20533#issuecomment-2399858704
More information about the compiler-dev
mailing list