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

Jesper Steen Møller jesper at selskabet.org
Sun Feb 2 10:33:47 UTC 2020


Hi list and Srikanth in particular

Thanks for the thorough review. I’ve updated the patch according to the review comments:

On 28 Jan 2020, at 15.06, Srikanth <srikanth.adayapalam at oracle.com> wrote:
> Hello Jesper,
> 
> (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.
> 

Fixed. I explained it as "Warn about issues related to migration of JDK classes.”, but I’m sure a better wording can be written.


> (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:
> […]

Fixed.

> (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.
> 

Ah, that explains it. Fixed.

I took the liberty of explaining this in the comment inside CheckExamples, so the next person can stumble upon it (optional).

> 
> (4) I think the diagnostic text is perhaps better worded as "return value of getClass() can never equal the class literal of an interface"
> 
Fixed.

> (5) CheckInterfaceComparison.java: the comment // key: compiler.warn.get.class.compared.with.interface can be dropped.

Fixed.

> (6) For completeness the test could also include != operator.

Good catch. Fixed.

> (7) Is the method com.sun.tools.javac.comp.Check#checkObjectIdentityComparison better named checkForSuspectClassLiteralComparison

Fixed.

> (ii) The diagnostic position need not be a parameter and could be computed on demand.
Fixed.

> (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) {}
>     }
> }
> 

Fixed and a (negative) test expression added.

> (9) Is the method isApplyGetClass better named as isInvocationOfGetClass() ?? Likewise is isClassOfSomeInterface better named as isClassLiteralOfSomeInterface
> 

Fixed: I trust your taste-judgment here (as in, I don’t know why visitApply takes a JCMethodInvocation when visitTypeApply takes a JCTypeApply ?, but I’m guessing “history”). isInvocationOfGetClass it is! 

> (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.
> 

Fixed. I ad-libbed an “//and"

-Jesper



—— Patch below:

diff -r e2f1c4d5f39e 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	Tue Jan 28 17:12:01 2020 +0530
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java	Sun Feb 02 11:32:49 2020 +0100
@@ -216,6 +216,11 @@
         MODULE("module"),
 
         /**
+         * Warn about practices which need to change to remain supported om future versions
+         */
+        MIGRATION("migration"),
+
+        /**
          * Warn about issues regarding module opens.
          */
         OPENS("opens"),
diff -r e2f1c4d5f39e 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	Tue Jan 28 17:12:01 2020 +0530
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java	Sun Feb 02 11:32:49 2020 +0100
@@ -3817,6 +3817,7 @@
                 if (!types.isCastable(left, right, new Warner(tree.pos()))) {
                     log.error(tree.pos(), Errors.IncomparableTypes(left, right));
                 }
+                chk.checkForSuspectClassLiteralComparison(tree, left, right);
             }
 
             chk.checkDivZero(tree.rhs.pos(), operator, right);
diff -r e2f1c4d5f39e 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	Tue Jan 28 17:12:01 2020 +0530
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java	Sun Feb 02 11:32:49 2020 +0100
@@ -1045,6 +1045,39 @@
         return varType;
     }
 
+    public void checkForSuspectClassLiteralComparison(
+            final JCBinary tree,
+            final Type leftType,
+            final Type rightType) {
+
+        if (lint.isEnabled(LintCategory.MIGRATION)) {
+            if (isInvocationOfGetClass(tree.lhs) && isClassOfSomeInterface(rightType) ||
+                    isInvocationOfGetClass(tree.rhs) && isClassOfSomeInterface(leftType)) {
+                log.warning(LintCategory.MIGRATION, tree.pos(), Warnings.GetClassComparedWithInterface);
+            }
+        }
+    }
+    //where
+    private 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;
+    }
+    //where
+    private boolean isInvocationOfGetClass(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 && !sym.isStatic();
+        }
+        return false;
+    }
+
     Type checkMethod(final Type mtype,
             final Symbol sym,
             final Env<AttrContext> env,
diff -r e2f1c4d5f39e 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	Tue Jan 28 17:12:01 2020 +0530
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties	Sun Feb 02 11:32:49 2020 +0100
@@ -1778,6 +1778,9 @@
 compiler.warn.incubating.modules=\
     using incubating module(s): {0}
 
+compiler.warn.get.class.compared.with.interface=\
+    return value of getClass() can never equal the class literal of an interface
+
 # 0: symbol, 1: symbol
 compiler.warn.has.been.deprecated=\
     {0} in {1} has been deprecated
diff -r e2f1c4d5f39e 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	Tue Jan 28 17:12:01 2020 +0530
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties	Sun Feb 02 11:32:49 2020 +0100
@@ -206,6 +206,9 @@
 javac.opt.Xlint.desc.module=\
     Warn about module system related issues.
 
+javac.opt.Xlint.desc.migration=\
+    Warn about issues related to migration of JDK classes.
+
 javac.opt.Xlint.desc.opens=\
     Warn about issues regarding module opens.
 
diff -r e2f1c4d5f39e test/langtools/tools/javac/diags/CheckExamples.java
--- a/test/langtools/tools/javac/diags/CheckExamples.java	Tue Jan 28 17:12:01 2020 +0530
+++ b/test/langtools/tools/javac/diags/CheckExamples.java	Sun Feb 02 11:32:49 2020 +0100
@@ -45,12 +45,22 @@
 
 /**
  * Check invariants for a set of examples.
+ *
+ * READ THIS IF THIS TEST FAILS AFTER ADDING A NEW KEY TO 'compiler.properties':
+ * The 'examples' subdirectory contains a number of examples which provoke
+ * the reporting of most of the compiler message keys.
+ *
  * -- each example should exactly declare the keys that will be generated when
  *      it is run.
+ * -- this is done by the "// key:"-comment in each fine.
  * -- together, the examples should cover the set of resource keys in the
  *      compiler.properties bundle. A list of exceptions may be given in the
  *      not-yet.txt file. Entries on the not-yet.txt list should not be
  *      covered by examples.
+ * -- some keys are only reported by the compiler when specific options are
+ *      supplied. For the purposes of this test, this can be specified by a
+ *      comment e.g. like this: "// options: -Xlint:empty"
+ *
  * When new keys are added to the resource bundle, it is strongly recommended
  * that corresponding new examples be added here, if at all practical, instead
  * of simply and lazily being added to the not-yet.txt list.
diff -r e2f1c4d5f39e test/langtools/tools/javac/diags/examples/CheckGetClassComparedWithInterfaceLiteral.java
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/test/langtools/tools/javac/diags/examples/CheckGetClassComparedWithInterfaceLiteral.java	Sun Feb 02 11:32:49 2020 +0100
@@ -0,0 +1,31 @@
+/*
+ * Copyright (c) 2020, Jesper Steen M\u00f8ller. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+// key: compiler.warn.get.class.compared.with.interface
+// options: -Xlint:migration
+
+class CheckGetClassComparedWithInterfaceLiteral {
+    boolean checkObject(Object o) {
+        return o.getClass() == Iterable.class;
+    }
+}
diff -r e2f1c4d5f39e 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	Sun Feb 02 11:32:49 2020 +0100
@@ -0,0 +1,64 @@
+/*
+ * @test /nodynamiccopyright/
+ * @bug 8237074
+ * @summary Result of .getClass() should never be compared to an interface class literal
+ *
+ * @compile/ref=CheckInterfaceComparison.out -XDrawDiagnostics -Xlint:migration CheckInterfaceComparison.java
+ */
+public class CheckInterfaceComparison {
+    public boolean bogusCompareLeft(Object o) { // Should be warned against
+        return (o.getClass()) == Runnable.class;
+    }
+
+    public boolean bogusCompareNELeft(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 bogusCompareNERight(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 goodCompareNELeft(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 goodCompareNERight(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 rawCompareNELeft(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();
+    }
+
+    public boolean rawCompareNERight(Object o, Class<?> clazz) { // Is fine, no warning required
+        return clazz != o.getClass();
+    }
+
+    static Class<?> getClass(int x) {
+        return null;
+    }
+
+    public static boolean compare(Object o, Class<?> clazz) {
+        return getClass(0) == Runnable.class; // No warning required for static getClass
+    }
+}
diff -r e2f1c4d5f39e 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	Sun Feb 02 11:32:49 2020 +0100
@@ -0,0 +1,5 @@
+CheckInterfaceComparison.java:10:31: compiler.warn.get.class.compared.with.interface
+CheckInterfaceComparison.java:14:31: compiler.warn.get.class.compared.with.interface
+CheckInterfaceComparison.java:18:31: compiler.warn.get.class.compared.with.interface
+CheckInterfaceComparison.java:22:31: compiler.warn.get.class.compared.with.interface
+4 warnings







More information about the valhalla-dev mailing list