Initial proof of concept for implementation of -Xlint:hiddenclass

[Remi Forax]

On 09/20/2012 02:31 PM, Maurizio Cimadamore wrote:
> Excellent - I would add a test to check that the flag work under 
> separate compilation environment.
>
> Maurizio

I think the check in ClassReader should be improved a little to take care
of languages that are not Java but compile to .class, i.e. allow className.*

!n.toString().equals(c.name.toString()+".java")

should be in my opinion

if (c.getNestingKind() == TOP_LEVEL) {
   String classAttribute = n.toString();
   String className = c.name.toString();
   if (classAttribute).startsWith(className) &&
       (classAttribut.length() == className.length() || 
classAttribute.charAt(className.length()) == '.')) {

Rémi

> On 20/09/12 13:23, Fredrik Öhrström wrote:
>> Third webrev: http://cr.openjdk.java.net/~ohrstrom/webrev-7153951-v3
>>
>> 2012/9/13 Jonathan Gibbons <jonathan.gibbons at oracle.com>:
>>> The arity of the message is wrong. I Check 3111, you provide 3 args.
>>> In the resource file you only expect 2.
>>>
>>> -- Jon
>>>
>>>
>>> On 09/12/2012 07:07 PM, Jonathan Gibbons wrote:
>>>> In the message file, the comment on 1721 is redundant since the 
>>>> same info
>>>> is on
>>>> line 1722.   These comments are used by the localization team. They 
>>>> are in
>>>> a stylized form
>>>> so that they can be mechanically checked by utilities in the
>>>> test/tools/diags/examples world.
>>>>
>>>> Since there is a new message, you will need a new example, in order to
>>>> pass the tests.
>>>>
>>>> Check:3107
>>>> The convention is to use the kind, not use instanceof
>>>>
>>>> -- Jon
>>>>
>>>> On 09/13/2012 08:06 AM, Maurizio Cimadamore wrote:
>>>>> On 12/09/12 16:00, Fredrik Öhrström wrote:
>>>>>> Second round of implementation:
>>>>>> http://cr.openjdk.java.net/~ohrstrom/webrev-7153951-v2/
>>>>>> <http://cr.openjdk.java.net/%7Eohrstrom/webrev-7153951-v2/>
>>>>>>
>>>>>> When building the openjdk, it outputs 316 warnings like these:
>>>>>> .../ClientHandshaker.java:723: warning: secondary class
>>>>>> SupportedEllipticCurvesExtension in …./HelloExtensions.java 
>>>>>> should not be
>>>>>> accessed from outside its own source file
>>>>> Great work - couple of comments:
>>>>>
>>>>> *) it seems like the warning should not be generated when 
>>>>> isAccessible
>>>>> fails, so, why not using that method as a condition to filter out the
>>>>> warning (instead of inlining that logic in the method guard) ?
>>>>>
>>>>> *) is there a better name than 'secondary class'  (maybe a 
>>>>> question for
>>>>> our spec gurus?)
>>>>>
>>>>> Maurizio
>>>>>
>>>>>>
>>>>>> //Fredrik
>>>>>>
>>>>>> 16 mar 2012 kl. 16:03 skrev Jonathan Gibbons:
>>>>>>
>>>>>>> Fredrik,
>>>>>>>
>>>>>>> Good start.
>>>>>>>
>>>>>>> 1. It should be sufficient to remove references to the file 
>>>>>>> manager and
>>>>>>> use something like
>>>>>>>
>>>>>>>     c.sourcefile == env.toplevel.sourcefile
>>>>>>>
>>>>>>> to check if the class is in the "right" file.
>>>>>>>
>>>>>>> 2. I think line numbers are desirable, and would be easy if you 
>>>>>>> move
>>>>>>> the checkForHiddenAccess from Resolve into Check, and call it 
>>>>>>> from Attr.
>>>>>>>
>>>>>>> 3. I think a warning per reference is acceptable: it is not 
>>>>>>> necessary
>>>>>>> to optimize it to a warning per hidden class.
>>>>>>>
>>>>>>> 4. We'll need a CCC approval for the new Xlint suboption.
>>>>>>>
>>>>>>> 5. We should check to see if there is a preferred term for what you
>>>>>>> call "hidden class".
>>>>>>>
>>>>>>> -- Jon
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 03/16/2012 05:12 AM, Fredrik Öhrström wrote:
>>>>>>>>>  From the bug:
>>>>>>>> Although legal, the use of multiple top level classes in the 
>>>>>>>> same file
>>>>>>>> is somewhat questionable to begin with, but it is particularly bad
>>>>>>>> when
>>>>>>>> in some package class A in A.java refers to class B defined in 
>>>>>>>> C.java.
>>>>>>>> This requires that at times the files must be compiled 
>>>>>>>> together, and
>>>>>>>> prevents implicit compilation from locating such "hidden classes".
>>>>>>>>
>>>>>>>> Proof of concept impl:
>>>>>>>> http://cr.openjdk.java.net/~ohrstrom/webrev-7153951-v1/
>>>>>>>> <http://cr.openjdk.java.net/%7Eohrstrom/webrev-7153951-v1/>
>>>>>>>>
>>>>>>>> It seems to detect the problem correctly. And there are a few 
>>>>>>>> of these
>>>>>>>> in the jdk, it seems, ~100.
>>>>>>>>
>>>>>>>> How to do isSameFile test properly?
>>>>>>>> How to report each hidden class reference only once?
>>>>>>>> Are line numbers neccesary?
>>>>>>>> Where to put checkForHiddenAccess?
>>>>>>>> Does it cover all possible use cases?
>>>>>>>> What else did I miss?
>>>>>>>>
>>>>>>>> //Fredrik
>>>>>>>>
>