8237074: Warning when obj.getClass() == SomeItf.class (from State-of-Valhalla)

Srikanth srikanth.adayapalam at oracle.com
Wed Jan 29 05:18:59 UTC 2020



Another comment:

(10) Some of the methods in Check.java could be declared private. Javac 
has a coding convention to denote "local" methods
using a

     // where

comment. You may want to use that too.

Thanks
Srikanth
On 28/01/20 7:36 pm, Srikanth wrote:
>
> Hello Jesper,
>
> Great attempt for a first patch to valhalla javac - that too with no 
> help worth mentioning. Kudos!
>
> Here are some review comments for follow up:
>
> (1) I think we may want to introduce a new lint category called 
> "migration" (I am surprised that we don't have one such already !!!) 
> as an umbrella under which all migration pain points related warnings 
> could be emitted. I think introducing a full blown category called 
> Interfaces to "warn about the use of interfaces which have previously 
> been classes" may not be optimal.
>
> (2) While you have introduced a lint category, the corresponding 
> warning is emitted as a plain warning. i.e I am not able to control it 
> via -Xlint options. For comparison, see how the -Xlint:cast (Warn 
> about use of unnecessary casts) option works in this test case:
>
> $ cat Y.java
> public class Y {
>     public static void main(String [] args) {
>         String s = (String) "Hello";
>     }
> }
>
> $ ~/jdk/jdk13/jdk-13/bin/javac -g Y.java  // no warnings, no lint mode 
> is enabled
> $ ~/jdk/jdk13/jdk-13/bin/javac -Xlint Y.java // all lint checks are 
> enabled, so warn about cast
> Y.java:3: warning: [cast] redundant cast to String
>         String s = (String) "Hello";
>                    ^
> 1 warning
> $ ~/jdk/jdk13/jdk-13/bin/javac -Xlint:deprecation Y.java // only 
> deprecation lint warnings, no cast warnings
> $ ~/jdk/jdk13/jdk-13/bin/javac -Xlint:all Y.java // all lint modes.
> Y.java:3: warning: [cast] redundant cast to String
>         String s = (String) "Hello";
>                    ^
> 1 warning
> $ ~/jdk/jdk13/jdk-13/bin/javac -Xlint:cast Y.java // express 
> enablement of cast
> Y.java:3: warning: [cast] redundant cast to String
>         String s = (String) "Hello";
>                    ^
> 1 warning
> $ ~/jdk/jdk13/jdk-13/bin/javac -Xlint:-cast Y.java // express 
> disablement of cast
> $
>
> (3) The comment in the file examples.not-yet.txt i.e "# if this isn't 
> listed here, CheckExamples.java will complain" can be dropped.
> For every diagnostic key added, the SOP is to write a test that would 
> cause that key to be emitted. Sometimes, it is too hard to devise a 
> test case and the examples.not-yet.txt is a grab bag to "white list" 
> all those property keys.
>
> During early prototyping it is reasonable to just add the key to the 
> examples.not-yet.txt but it is mildly frowned upon. In any case as a 
> practice you may want to figure out how to do this. Basically you have 
> to add a snippet to
> test/langtools/tools/javac/diags/examples directory that would trigger 
> the diagnostic key to be emitted.
> See test/langtools/tools/javac/diags/examples/CallMustBeFirst.java for 
> example.
>
> (4) I think the diagnostic text is perhaps better worded as "return 
> value of getClass() can never equal the class literal of an interface"
>
> (5) CheckInterfaceComparison.java: the comment // key: 
> compiler.warn.get.class.compared.with.interface can be dropped.
> (6) For completeness the test could also include != operator.
> (7) Is the method 
> com.sun.tools.javac.comp.Check#checkObjectIdentityComparison better 
> named checkForSuspectClassLiteralComparison
> (ii) The diagnostic position need not be a parameter and could be 
> computed on demand.
> (8) I think it would be better to tighten the check for getClass: For 
> example, see that the following code triggers a warning when it
> should not: (cf com.sun.tools.javac.comp.Attr#adjustMethodReturnType's 
> check of getClass)
>
> class X {
>
>     static Class<?> getClass(int x) {
>         return null;
>     }
>
>     public static void main(String [] args) {
>         if (getClass(0) == Runnable.class) {}
>     }
> }
>
> (9) Is the method isApplyGetClass better named as 
> isInvocationOfGetClass() ?? Likewise is isClassOfSomeInterface better 
> named as isClassLiteralOfSomeInterface
>
> I will study your other patch next. That is the top item on my todo 
> list now that the support for top interfaces got pushed.
>
> Thanks!
> Srikanth
>
>
> On 22/01/20 3:37 am, Jesper Steen Møller wrote:
>> Hi list.
>>   - another patch for review for another (smaller) task opened by 
>> Srikanth from the 'State of Valhalla’ writeup.
>>
>> This patch is for 8237074 [1], where we’d want to catch the case 
>> where Optional or Integer migrate from being classes to interfaces, 
>> and where existing code tries to query their type by using 
>> Object.getClass() and compare with a class literal for something 
>> which is now a literal.
>>
>> I’m sure the wording of the error message and possibly the lint 
>> category could be improved by a native English-speaker.
>>
>> One small quirk: There was a problem with the CheckExamples test not 
>> picking up that “// key:”-marker in the test source, so I ended up 
>> adding the warning key to "examples.not-yet.txt", which feels like a 
>> bit of a hack. I can see there’s a bug JDK-8209907 related to 
>> problems with CheckExamples in “lworld", so maybe I’ll examine the 
>> reason.
>>
>> [1]: https://bugs.openjdk.java.net/browse/JDK-8237074
>>
>> -Jesper
>>
>> diff -r 9d2ec504577f 
>> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java
>> --- 
>> a/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java 
>> Mon Jan 13 17:18:47 2020 -0800
>> +++ 
>> b/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java 
>> Tue Jan 21 23:06:20 2020 +0100
>> @@ -211,6 +211,11 @@
>>           FINALLY("finally"),
>>             /**
>> +         * Warn about the use of interfaces which have previously 
>> been classes
>> +         */
>> +        INTERFACES("interfaces"),
>> +
>> +        /**
>>            * Warn about module system related issues.
>>            */
>>           MODULE("module"),
>> diff -r 9d2ec504577f 
>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java
>> --- 
>> a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java 
>> Mon Jan 13 17:18:47 2020 -0800
>> +++ 
>> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java 
>> Tue Jan 21 23:06:20 2020 +0100
>> @@ -3814,6 +3814,7 @@
>>                   if (!types.isCastable(left, right, new 
>> Warner(tree.pos()))) {
>>                       log.error(tree.pos(), 
>> Errors.IncomparableTypes(left, right));
>>                   }
>> +                chk.checkObjectIdentityComparison(tree.pos(), tree, 
>> left, right);
>>               }
>>                 chk.checkDivZero(tree.rhs.pos(), operator, right);
>> diff -r 9d2ec504577f 
>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java
>> --- 
>> a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java 
>> Mon Jan 13 17:18:47 2020 -0800
>> +++ 
>> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java 
>> Tue Jan 21 23:06:20 2020 +0100
>> @@ -1045,6 +1045,39 @@
>>           return varType;
>>       }
>>   +    boolean isApplyGetClass(JCExpression tree) {
>> +        tree = TreeInfo.skipParens(tree);
>> +        if (tree.hasTag(APPLY)) {
>> +            JCMethodInvocation apply = (JCMethodInvocation)tree;
>> +            Symbol sym = TreeInfo.symbol(apply.meth);
>> +            return sym.name == names.getClass;
>> +        }
>> +        return false;
>> +    }
>> +
>> +    boolean isClassOfSomeInterface(Type someClass) {
>> +        if (someClass.tsym.flatName() == names.java_lang_Class) {
>> +            List<Type> arguments = someClass.getTypeArguments();
>> +            if (arguments.length() == 1) {
>> +                return arguments.head.isInterface();
>> +            }
>> +        }
>> +        return false;
>> +    }
>> +
>> +    public void checkObjectIdentityComparison(
>> +            final DiagnosticPosition pos,
>> +            final JCBinary tree,
>> +            final Type leftType,
>> +            final Type rightType) {
>> +
>> +        if (isApplyGetClass(tree.lhs) && 
>> isClassOfSomeInterface(rightType)) {
>> +            log.warning(LintCategory.INTERFACES, pos, 
>> Warnings.GetClassComparedWithInterface(rightType));
>> +        } else if (isApplyGetClass(tree.rhs) && 
>> isClassOfSomeInterface(leftType)) {
>> +            log.warning(LintCategory.INTERFACES, pos, 
>> Warnings.GetClassComparedWithInterface(leftType));
>> +        }
>> +    }
>> +
>>       Type checkMethod(final Type mtype,
>>               final Symbol sym,
>>               final Env<AttrContext> env,
>> diff -r 9d2ec504577f 
>> src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties
>> --- 
>> a/src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties 
>> Mon Jan 13 17:18:47 2020 -0800
>> +++ 
>> b/src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties 
>> Tue Jan 21 23:06:20 2020 +0100
>> @@ -1778,6 +1778,10 @@
>>   compiler.warn.incubating.modules=\
>>       using incubating module(s): {0}
>>   +# 0: type
>> +compiler.warn.get.class.compared.with.interface=\
>> +    result of calling getClass() compared to class of interface {0}
>> +
>>   # 0: symbol, 1: symbol
>>   compiler.warn.has.been.deprecated=\
>>       {0} in {1} has been deprecated
>> diff -r 9d2ec504577f 
>> src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties
>> --- 
>> a/src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties 
>> Mon Jan 13 17:18:47 2020 -0800
>> +++ 
>> b/src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties 
>> Tue Jan 21 23:06:20 2020 +0100
>> @@ -203,6 +203,9 @@
>>   javac.opt.Xlint.desc.finally=\
>>       Warn about finally clauses that do not terminate normally.
>>   +javac.opt.Xlint.desc.interfaces=\
>> +    Warn about interfaces which may previously have been classes.
>> +
>>   javac.opt.Xlint.desc.module=\
>>       Warn about module system related issues.
>>   diff -r 9d2ec504577f 
>> test/langtools/tools/javac/diags/examples.not-yet.txt
>> --- a/test/langtools/tools/javac/diags/examples.not-yet.txt Mon Jan 
>> 13 17:18:47 2020 -0800
>> +++ b/test/langtools/tools/javac/diags/examples.not-yet.txt Tue Jan 
>> 21 23:06:20 2020 +0100
>> @@ -109,6 +109,7 @@
>>   compiler.warn.annotation.method.not.found.reason        # ClassReader
>>   compiler.warn.big.major.version                         # ClassReader
>>   compiler.warn.future.attr                               # ClassReader
>> +compiler.warn.get.class.compared.with.interface         # if this 
>> isn't listed here, CheckExamples.java will complain
>>   compiler.warn.illegal.char.for.encoding
>>   compiler.warn.incubating.modules                        # requires 
>> adjusted classfile
>>   compiler.warn.invalid.archive.file
>> diff -r 9d2ec504577f 
>> test/langtools/tools/javac/valhalla/lworld-values/CheckInterfaceComparison.java
>> --- /dev/null    Thu Jan 01 00:00:00 1970 +0000
>> +++ 
>> b/test/langtools/tools/javac/valhalla/lworld-values/CheckInterfaceComparison.java 
>> Tue Jan 21 23:06:20 2020 +0100
>> @@ -0,0 +1,33 @@
>> +/*
>> + * @bug 8237074
>> + * @test /nodynamiccopyright/
>> + * @summary Result of .getClass() should never be compared to an 
>> interface class literal
>> + *
>> + * @compile/ref=CheckInterfaceComparison.out -XDrawDiagnostics 
>> CheckInterfaceComparison.java
>> + */
>> +// key: compiler.warn.get.class.compared.with.interface
>> +public class CheckInterfaceComparison {
>> +    public boolean bogusCompareLeft(Object o) { // Should be warned 
>> against
>> +        return (o.getClass()) == Runnable.class;
>> +    }
>> +
>> +    public boolean bogusCompareRight(Object o) { // Should be warned 
>> against
>> +        return Iterable.class == o.getClass();
>> +    }
>> +
>> +    public boolean goodCompareLeft(Object o) { // Is fine, no 
>> warning required
>> +        return o.getClass() == Integer.class;
>> +    }
>> +
>> +    public boolean goodCompareRight(Object o) { // Is fine, no 
>> warning required
>> +        return Long.class == o.getClass();
>> +    }
>> +
>> +    public boolean rawCompareLeft(Object o, Class<?> clazz) { // Is 
>> fine, no warning required
>> +        return o.getClass() == clazz;
>> +    }
>> +
>> +    public boolean rawCompareRight(Object o, Class<?> clazz) { // Is 
>> fine, no warning required
>> +        return clazz == o.getClass();
>> +    }
>> +}
>> diff -r 9d2ec504577f 
>> test/langtools/tools/javac/valhalla/lworld-values/CheckInterfaceComparison.out
>> --- /dev/null    Thu Jan 01 00:00:00 1970 +0000
>> +++ 
>> b/test/langtools/tools/javac/valhalla/lworld-values/CheckInterfaceComparison.out 
>> Tue Jan 21 23:06:20 2020 +0100
>> @@ -0,0 +1,3 @@
>> +CheckInterfaceComparison.java:10:31: 
>> compiler.warn.get.class.compared.with.interface: 
>> java.lang.Class<java.lang.Runnable>
>> +CheckInterfaceComparison.java:14:31: 
>> compiler.warn.get.class.compared.with.interface: 
>> java.lang.Class<java.lang.Iterable>
>> +2 warnings
>>
>



More information about the valhalla-dev mailing list