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

[Maurizio Cimadamore]

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

On 07/11/2018 08:59, Maurizio Cimadamore wrote:
>
> 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/080577c2/attachment.html>