Review request for JDK-8020745: Suspicious MethodParameters attribute generated for local classes capturing local variables

[Alex Buckley]

Thanks for improving the clarity of the test cases within Encloser.

I have some comments on the expected flags of parameters. First, the 
good news:

- The makeInner case is correct. Since InnerTester is private, the only 
code that can instantiate it is in the same top level class. This means 
the immediately enclosing instance of InnerTester can be transmitted to 
InnerTester's ctor in a proprietary manner; the JLS does not specify it. 
Hence, this$1 is SYNTHETIC, not MANDATED.

- The makeAnon case is correct. Since the anonymous class has a 
superclass that is inner (Tester), there must be a standard, 
JLS-specified way of transmitting, to the anonymous class's constructor, 
the immediately enclosing instance of the anonymous class with respect 
to the superclass. Hence, this$1 is MANDATED, not SYNTHETIC.

- The makeAnonExtendsInner case is similar to makeAnon, in that the 
anonymous class's superclass is inner (InnerTester). But because 
InnerTester is private, it is possible to transmit the immediately 
enclosing instance of the anonymous class with respect to the superclass 
in a proprietary manner to the anonymous class's constructor. That 
argues for this$1 being SYNTHETIC, rather than MANDATED as in the test 
case. However, the spec is not clear on this scenario. I intend to 
clarify the spec ASAP, which will make the current makeAnonExtendsInner 
incorrect.

Now the bad news:

- The makeLocal case is incorrect. Since LocalTester is local, the only 
code that can instantiate it is in the body of the makeLocal method. 
This means the immediately enclosing instance of LocalTester can be 
transmitted to LocalTester's ctor in a proprietary manner; the JLS does 
not specify it. Hence, this$1 should be SYNTHETIC, not MANDATED.

- The makeAnonExtendsLocal case is tricky. Please recognize that the 
constructor of the abstract class LocalTester is not the same thing as 
the constructor of the anonymous class whose superclass is LocalTester. 
names() et al in the abstract class LocalTester are necessarily 
identical to those in the class LocalTester in makeLocal; they should be 
removed from makeAnonExtendsLocal. Then, please add names() et al to the 
body of the anonymous class, akin to the anonymous class declared in 
makeAnonExtendsInner. What values are expected by the new names()? Good 
question. The anonymous class in makeAnonExtendsLocal has a superclass 
(LocalTester) which is inner but also local, so the spec is not clear 
how to transmit the immediately enclosing instance of the anonymous 
class with respect to the superclass. Again, I intend to clarify the 
spec ASAP.

FWIW the Inner Classes Specification in 1997 described the transmission 
of enclosing instances to nested class constructors, but did not quite 
appreciate the subtlety of what needed to be "mandated" for compiler 
interoperability.

Alex

On 8/22/2013 8:05 AM, Eric McCorkle wrote:
> These have been addressed.  A new webrev has been posted.
>
> On 08/21/13 17:52, Alex Buckley wrote:
>> The major test cases are now covered, which is good.
>>
>> You should add a multi-line comment which enumerates them. The @summary
>> is just not enough.
>>
>> You could get rid of some "line noise" by using array initializers:
>>
>>    return new String[] { "this$1",
>>                          "innerparam" };
>>
>> This will minimize the number of variables with general names like
>> "names" and "types". Such variables already exist in Tester#check and
>> you _really_ want to avoid a hiding scenario.
>>
>> Tester#message is pretty obscure. I mean, you can't _force_ the methods
>> overriding it to capture the 'message' variable of ParameterNames. It
>> would be more direct for class InnerTester et al so simply declare their
>> own fields which reference ParameterNames.message. Then you could drop
>> Tester#message. Please document that ParameterNames.message exists only
>> to be captured, and that javac currently happens to choose val$message
>> for the name of the synthetic ctor parameter but it may choose another
>> name in the future.
>>
>> I don't see what Tester.field is doing.
>>
>> Alex
>>
>> On 8/21/2013 2:00 PM, Eric McCorkle wrote:
>>> I have addressed these issues and uploaded a new webrev.  Crucible
>>> reviews are also complete.
>>>
>>> On 08/20/13 21:17, Alex Buckley wrote:
>>>> - Method makeAnonExtendsInner does not return an instance of an
>>>> anonymous class whose superclass is inner. InnerList is a local class,
>>>> and its superclass ArrayList is top level. Also, you should document the
>>>> kinds of classes for which you're exercising parameter generation.
>>>>
>>>> - Encapsulate! The "expected" variables like Inner_names should be
>>>> closer to the classes declaring the "actual" variables (i.e. constructor
>>>> parameters) of interest. checkConstructor should merely have to ask that
>>>> class to check itself.
>>>>
>>>> Alex
>>>>
>>>> On 8/20/2013 3:36 PM, Eric McCorkle wrote:
>>>>> A new webrev has been uploaded which addresses these issues.
>>>>> http://cr.openjdk.java.net/~emc/8020745/
>>>>>
>>>>> On 08/19/13 20:28, Alex Buckley wrote:
>>>>>> The AnonymousParameters.java test is very confusing. Two classes with
>>>>>> main methods in the same compilation unit?!
>>>>>>
>>>>>> Bug 8020745 indicates a mismatch between parameter names and types,
>>>>>> but
>>>>>> this test only checks names. Rather than relying on the order of names
>>>>>> from the JVM to match the 'names' array, you should match up names and
>>>>>> types explicitly.
>>>>>>
>>>>>> List is a bad name for a helper class, and the @summary is not very
>>>>>> helpful.
>>>>>>
>>>>>> Most importantly, there are no anonymous classes here - List is a
>>>>>> local
>>>>>> class. The test should have an anonymous class too - in fact, an
>>>>>> anonymous class whose superclass is not inner and an anonymous class
>>>>>> whose superclass is inner. (See the notes in 8misc.pdf 2.2 8.8.9
>>>>>> for why
>>>>>> that matters.)
>>>>>>
>>>>>> Alex
>>>>>>
>>>>>> On 8/19/2013 4:31 PM, Eric McCorkle wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> Please review this patch, which causes captured locals to be added to
>>>>>>> the end of a MethodParameters attribute, like they should.
>>>>>>>
>>>>>>> The webrev is here:
>>>>>>> http://cr.openjdk.java.net/~emc/8020745/
>>>>>>>
>>>>>>> The bug report is here:
>>>>>>> http://bugs.sun.com/view_bug.do?bug_id=8020745
>>>>>>>
>>>>>>> This review is also on the new Crucible system.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Eric
>>>>>>>