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

[Jesper Steen Møller]

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