Initial proof of concept for implementation of -Xlint:hiddenclass

Thu Sep 20 08:19:40 PDT 2012

Hi,

After applying this patch I get 3 failures in the warnings test directory.

Also this test that shouldn't fail (IMO) fails as well:

/**                                                                                                                                                     
 * @test                                                                                                                                                
 * @compile -Werror -Xlint:auxiliaryclass SelfClassWithAux.java                                                                                         
 */                                                                                                                                                     

class SelfClassWithAux {                                                                                                                                
}                                                                                                                                                       

class AuxiliaryClass {                                                                                                                                  
    AuxiliaryClass c = null;                                                                                                                            
}

Running manually:

./dist/bin/javac -Xlint:auxiliaryclass test/tools/javac/warnings/AuxiliaryClass/SelfClassWithAux.java 
test/tools/javac/warnings/AuxiliaryClass/SelfClassWithAux.java:33: warning: auxiliary class AuxiliaryClass in test/tools/javac/warnings/AuxiliaryClass/SelfClassWithAux.java should not be accessed from outside its own source file
    AuxiliaryClass c = null;
    ^
1 warning

I can't find where you check that you are actually accessing a class outside your file, is that check missing?

Also, IIRC we aren't supposed to use j.l.model inside javac you can rewrite ClassReader change to:

+                    if (c.owner.kind == Kinds.PCK &&
+                        !n.toString().equals(c.name.toString()+".java")) {
+                        c.flags_field |= AUXILIARY;
+                    }

cheers
/Joel

On Sep 20, 2012, at 2:23 PM, Fredrik Öhrström <oehrstroem at gmail.com> 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
>>>>>>> 
>>>>>> 
>>>>> 
>>>> 
>>> 
>> 