RFR: JDK-8247790: javac shouldn't allow type variable references from local static declarations

[Jan Lahoda]

Looks good to me. Thanks for updating the patch!

Jan

On 15. 07. 20 22:40, Vicente Romero wrote:
> Hi Jan,
> 
> I have updated the patch and added more tests after our offline chat, 
> please see [1]. How does it look?
> 
> Vicente
> 
> [1] http://cr.openjdk.java.net/~vromero/8247790/webrev.03/
> 
> On 7/14/20 11:04 PM, Vicente Romero wrote:
>> Hi,
>>
>> I have realized of a couple of issues on the last iteration of the 
>> patch so I have produced a new one at [1]. How does it look?
>>
>> Thanks,
>> Vicente
>>
>> [1] http://cr.openjdk.java.net/~vromero/8247790/webrev.02/
>>
>> On 6/29/20 2:53 PM, Vicente Romero wrote:
>>> Hi Jan,
>>>
>>> Thanks for your review, there have been a lot of internal discussion 
>>> about the spec and the examples you proposed. Given that this spec is 
>>> new, it is hard to see where the lines are. Your examples are good, 
>>> thanks, and I have added them as additional tests. I didn't see how 
>>> to leverage on the existing "staticOnly" flag. Instead in order to 
>>> implement this assertion in the spec see [1]:
>>>
>>>   * If the type name appears in a nested class or interface
>>>     declaration of /C/, then the immediately enclosing class or
>>>     interface declaration of the type name must specify an inner
>>>     class (8.1.3
>>>     <https://cr.openjdk.java.net/~gbierman/jep384/jep384-20200506/specs/local-statics-jls.html#jls-8.1.3>)
>>>     declared in the body of /m/, or an inner class of an inner class
>>>     declared in the body of /m/.
>>>
>>> I added a helper method named: "isInnerClassOfMethod", but this one 
>>> still needs the original environment, as an alternative I can pass 
>>> the enclosing class relative to the original environment but still, 
>>> what do you think?
>>>
>>> Thanks,
>>> Vicente
>>>
>>> [1] 
>>> https://cr.openjdk.java.net/~gbierman/jep384/jep384-20200506/specs/local-statics-jls.html#jls-6.5.5.1
>>> new webrev: http://cr.openjdk.java.net/~vromero/8247790/webrev.01/
>>>
>>> On 6/23/20 5:12 AM, Jan Lahoda wrote:
>>>> Hi Vicente,
>>>>
>>>> I think this is a good direction, but probably not sufficient. My 
>>>> question would be if it is possible to piggy back more on the 
>>>> existing "staticOnly" flag, instead of checking the innermost 
>>>> environment.
>>>>
>>>> Specifically, consider this example:
>>>> ---
>>>> package javaapplication29;
>>>>
>>>> public class JavaApplication29 {
>>>>
>>>>     public static <T> void main(String[] args) {
>>>>         String hello = "hello";
>>>>         interface I {
>>>>             public default void test1() {
>>>>                 class X {
>>>>                     public void test2() {
>>>>                         System.err.println(hello);
>>>>                         T t = null;
>>>>                     }
>>>>                 }
>>>>                 new X().test2();
>>>>             }
>>>>         }
>>>>         record R(int i) {
>>>>             public void test1() {
>>>>                 class X {
>>>>                     public void test2() {
>>>>                         System.err.println(hello);
>>>>                         T t = null;
>>>>                     }
>>>>                 }
>>>>                 new X().test2();
>>>>             }
>>>>         }
>>>>         enum E {
>>>>             A;
>>>>             public void test1() {
>>>>                 class X {
>>>>                     public void test2() {
>>>>                         System.err.println(hello);
>>>>                         T t = null;
>>>>                     }
>>>>                 }
>>>>                 new X().test2();
>>>>             }
>>>>         }
>>>>         new I() {}.test1();
>>>>         new R(0).test1();
>>>>         E.A.test1();
>>>>     }
>>>>
>>>> }
>>>> ---
>>>>
>>>> The behavior here does not seem to be quite right - there are no 
>>>> errors reported, but javac crashes on "E", produces wrong classfile 
>>>> for "I" and suspicious (but working) classfile for "R".
>>>>
>>>> Jan
>>>>
>>>> On 23. 06. 20 0:29, Vicente Romero wrote:
>>>>> Hi,
>>>>>
>>>>> Please review the fix for [1] at [2]. The issue here is that local 
>>>>> interfaces, enums and records shouldn't be allow to refer to type 
>>>>> variables defined in an enclosing context. The compiler was 
>>>>> checking for this if the type variables were defined by the 
>>>>> enclosing class but references to type variables defined by the 
>>>>> enclosing method were allowed. This patch is covering this gap,
>>>>>
>>>>> Thanks,
>>>>> Vicente
>>>>>
>>>>> [1] http://cr.openjdk.java.net/~vromero/8247790/webrev.00/
>>>>> [2] https://bugs.openjdk.java.net/browse/JDK-8247790
>>>
>>
>