(original) (raw)

Btw,
I think where javac is really tripping up is that it's not setting up the env.info.selectSuper parameter ahead of performing the resolution step. This parameter is setup like this:

diamondEnv.info.selectSuper = cdef != null;

I wonder, is speculative attribution completely messing this up (since it is removing bodies from anon classes using diamond) ? If so, then that is your issue (but it would still be nice if the spec text would be made clearer).

Maurizio

Hi Vicente,
I have some reservation about the fix; the idea behind the code in Resolve is that there exist some 'ordering' of error messages (by usefulness) - typically if you have something that is NOT APPLICABLE and ACCESSIBLE you'd want that to be reported instead of some inaccessible symbol found somewhere else. This logic makes a lot of sense given that javac scans supertypes from the bottom up - meaning that if you reported every single access error, you'd get a lot of spurious hits. So this patch has the problem of altering that behavior: now accessibility errors can 'trump' applicability ones.

Another problem in the fix (maybe latent) is that you only create the access error if the bestSoFar is WRONG\_MTH - but not if it's WRONG\_MTHS, meaning that if the arity of the inapplicable symbols > 1 your patch is not effective.

That said, instead of going down this route, I think it's worth asking a deeper (and JLS-related) question: what should be the modifiers of the synthetic constructor symbols that are used to resolve an anonymous class expression as per 15.9.3; that section says that the new constructors to be checked must be defined in the supertype of C (in this case MyClass itself) and that they must retain same modifiers (protected in this case):

"If the class instance creation expression uses <>, then:

\[...\]If C is an anonymous class, let D be the superclass or superinterface of C named by the class instance creation expression.

If D is a class, let c1...cn be the constructors of class D. \[...\]

A list of methods m1...mn is defined for the purpose of overload resolution and type argument inference. For all j (1 ≤ j ≤ n), mj is defined in terms of cj as follows:

...

The modifiers of mj are those of cj"

Under those rules, it is not clear to me how could the logic described in the spec succeed in finding the right method - they should both appear as non accessible (they are defined in D and in a different package, but with 'protected' modifier), so this should apply:

"To choose a constructor, we temporarily consider m1...mn to be members of D. One of m1...mn is chosen, as determined by the class instance creation expression's argument expressions, using the process specified in §15.12.2.

If there is no unique most specific method that is both applicable and accessible, then a compile-time error occurs."

For what is worth, even the non-diamond path of the spec text seems to suffer from this problem: it delegates blindly to 15.12.2 w/o saying what should happen if one of the constructors has protected access, but for some reason, javac doesn't seem to have a problem with that - to show that, simply remove all generic arguments for the instance creation expressions in the test case (e.g. create raw instances of MyClass) and see that it works w/o issues.

There are a bunch of possible fixes here - if we are checking a diamond anon class we could either run 15.12.2 \_pretending we are in D\_, or we could drop any protected modifier. Both changes, I think, seem to imply some spec update.

Maurizio

On 07/11/2018 01:45, Vicente Romero wrote:

Hi,

Please review patch for \[1\] at \[2\]. The issue was can be explained with these two simple classes:

\------------------------------------------------------------------------------------------------------
package pkg1;

public class MyClass {
protected MyClass() {} // (i)
protected MyClass(String s) {} // (ii)
}

\------------------------------------------------------------------------------------------------------

package pkg2;

import pkg1.\*;

class Client {
void foo(MyClass m) {}

void bar() {
foo(new MyClass<>(){});
}
}

\------------------------------------------------------------------------------------------------------

both constructors are protected and given that we are dealing with a diamond expression, speculative attribution is used. During speculative attribution, javac removes the class definition from the equation meaning that:
new MyClass<>(){} is rewritten as:
new MyClass<>()

the first expression has access to the protected constructors, the other one not. So as both are protected, they are considered not accessible during overload resolution. Still constructor (i) is applicable but constructor (ii) is not applicable. So my proposal is to create an access error at Resolve::selectBest also for this case. Right now an access error is created only if no other method is found. When an access error is returned during overload resolution, the inaccessible symbol is still used and attached to the AST till the check phase can double check and issue an error or not. As during the check phase the diamond expression is attributed without rewriting, the compiler can then find out that constructor (i) is both applicable and accessible.

Thanks,
Vicente

\[1\] https://bugs.openjdk.java.net/browse/JDK-8210197
\[2\] http://cr.openjdk.java.net/\~vromero/8210197/webrev.00/