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

Srikanth srikanth.adayapalam at oracle.com
Tue Jan 28 14:06:50 UTC 2020


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