RFR: JDK-8210197: candidate selection during overload resolution can lead to unexpected results for diamond expressions

[Maurizio Cimadamore]

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 |m_j | are those of |c_j |"


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<T> {
>     protected MyClass() {}                // (i)
>     protected MyClass(String s) {}    // (ii)
> }
>
> ------------------------------------------------------------------------------------------------------ 
>
>
> package pkg2;
>
> import pkg1.*;
>
> class Client {
>     <T> void foo(MyClass<T> 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/
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20181107/60d44546/attachment-0001.html>