Review request for JDK-7118412: Shadowing of type-variables vs. member types

[Alex Buckley]

In the test:

- Check @summary :-(

- I wanted to like the nested switch statements in MethodCall.succeeds, 
but found them inscrutable. I think more descriptive names for enum 
constants would help a lot, e.g. INNER_CLASS. And why are type variables 
a.k.a. TYPEVAR so interested in a method called "iterator", can't any 
reference type have a method called "iterator" ?

- Shadowing is not specific to method calls, so I think the succeeds 
logic should be moved to an instance method of ShadowingTest. doTest 
already has to pass three things to call.succeeds, in order for 
MethodCall to work out the answer, so passing 'call' as a fourth 
argument is no big deal. It is a good thing to have single-purpose enums 
to model the language constructs being combined, since the constructs 
are already complicated enough themselves.

- MethodContext suggests some entity apart from a method that provides 
an environment for the method. But in fact, MethodContext represents 
whether the method iself is a class method or instance method. 
MethodKind would be a better name.

- InsideDef would be better called MethodDeclaration. (And "NONE"? None 
what?) TyVar would be better called TypeParameterDeclaration. (<T 
extends ...> is a _type parameter_ declaration, and introduces a _type 
variable_ which may be referred to as T in contexts where reference 
types are used, e.g. the type in a field declaration. Can you tell I've 
been adding annotations on type uses to the JLS recently?)

- main calls run calls doTest, but doTest is declared before run - and 
is private too? Why are some members private and others package-private?

Alex

On 7/24/2013 2:42 PM, Eric McCorkle wrote:
> This RFR was dormant for a while pending CCC approval, which happened
> today.  Please note that I've made updates since the last reviews.
>
> I will be running the JCK test suite, as this affects the core compiler.
>
> The updated webrev is here:
> http://cr.openjdk.java.net/~emc/7118412/webrew.02/
>
> On 05/21/13 11:38, Eric McCorkle wrote:
>> I hit a couple of corner cases with the regression tests mid last week.
>>   Expect another update sometime in the near future.
>>
>> On 05/21/13 06:36, Maurizio Cimadamore wrote:
>>> On 21/05/13 11:35, Maurizio Cimadamore wrote:
>>>> Fix looks nice - only bit that seems suspicious is the check for
>>>>
>>>> sym == null
>>>>
>>>> The symbol could be different than null, but still not be a 'valid'
>>>> symbol (i.e. because it's an error). Actually, a lookup should never
>>>> return a null symbol. I suggest you return the 'bestSoFar' (which
>>>> would be typeNotFound') and then use the useful method Symbol.exists()
>>>> which should do what you need (check if the symbol is a valid symbol).
>>>> I think in cases where the found symbol is inaccessible it's important
>>>> to stop and to log the error, you should not keep going recursively
>>>> until you find a correct symbol.
>>> The test also needs some work in order to match the combo test idiom (I
>>> should have sent you an example of such tests already).
>>>
>>> Maurizio
>>>>
>>>> Maurizio
>>>>
>>>> On 15/05/13 16:32, Eric McCorkle wrote:
>>>>> I have updated this patch to correct the shadowing of type variables by
>>>>> both member and static member class definitions.
>>>>>
>>>>> I have also included an improved testing framework that exhaustively
>>>>> tests the possible combinations.
>>>>>
>>>>> Note that a CCC has been filed for this patch, as it changes the way
>>>>> that type names are resolved in the compiler.
>>>>>
>>>>> Please review and comment.
>>>>>
>>>>> On 05/07/13 13:14, Eric McCorkle wrote:
>>>>>> Hello,
>>>>>>
>>>>>> Please review this patch which fixes an issue with shadowing by type
>>>>>> variables.
>>>>>>
>>>>>> Note: despite the very small size of this patch, it makes changes to
>>>>>> the
>>>>>> way that types are resolved in scopes, thus it needs to be treated with
>>>>>> extreme care.  Please review it and the test I have included carefully.
>>>>>>
>>>>>> The webrev is here:
>>>>>> http://cr.openjdk.java.net/~emc/7118412/webrew.00/
>>>>>>
>>>>>> The bug can be found here:
>>>>>> http://bugs.sun.com/view_bug.do?bug_id=7118412
>>>>>>
>>>>>> Thanks,
>>>>>> Eric
>>>>>>
>>>>
>>>