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

Wed Nov 7 13:40:23 UTC 2018

Hi Maurizio,

Thanks for the feedback.

On 11/7/18 7:03 AM, Maurizio Cimadamore wrote:
>
> 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;
>

right I had the initial idea of going for this alternative first and 
wrote a patch see [3], but later convinced myself that the patch I sent 
in the original mail was the way to go :(. So [3] is a fix on the lines 
of setting diamondEnv.info.selectSuper to true for diamonds that had a 
class definition attached even during speculative attribution. But the 
patch seemed a bit hacky to me. I guess that we should probably just add 
a field to JCNewClass to indicate if the ClassDef part has been removed 
just during speculative attribution as a cleaner solution, what do you 
think?

> 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
>

[3] http://cr.openjdk.java.net/~vromero/8210197/webrev.01/

> 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/4e0180e3/attachment-0001.html>