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

[Eric McCorkle]

Crucible appears to have eaten my review :(

I've uploaded a new webrev with Alex's changes incorporated, except for
renaming MethodContext.  It refers to the context (instance vs static)
in which a call to T.<something, depending on MethodCall> is made.

The method names in MethodCall are deliberately different.  I use
"iterator", because the class type variable extends Collection, the
method names are a way of requiring resolution to have a particular result.

I also added comments explaining everything.

The webrev is here:
http://cr.openjdk.java.net/~emc/7118412/

On 08/16/13 16:00, Eric McCorkle wrote:
> (Been gone for 2+ weeks)
> 
> I will incorporate these changes.
> 
> However, I'm also going to give the new Crucible review system a try.
> As such, I've uploaded this patch (not yet with Alex's suggested changes).
> 
> However, anyone not on the Crucible system should feel free to
> contribute a review.
> 
> On 07/24/13 19:51, Alex Buckley wrote:
>> 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
>>>>>>>>
>>>>>>
>>>>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: eric_mccorkle.vcf
Type: text/x-vcard
Size: 314 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20130816/26f21a75/eric_mccorkle.vcf