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

Srikanth srikanth.adayapalam at oracle.com
Mon Feb 3 09:58:37 UTC 2020


Hi Jesper,

Congratulations on your first contribution to Valhalla/Inline types project!

Fix and tests pushed here: 
http://hg.openjdk.java.net/valhalla/valhalla/rev/b4bf177bac61

Look forward to many more contributions.

Thanks and regards,
Srikanth


On 03/02/20 1:39 pm, Jesper Steen Møller wrote:
> Hi Srikanth
>
> On 3 Feb 2020, at 07.49, Srikanth <srikanth.adayapalam at oracle.com 
> <mailto:srikanth.adayapalam at oracle.com>> wrote:
>>
>>
>> (1) There is a typo "supported om future versions"
>>
>
> Fixed, and made comment and lint option text identical
>
>
>> (2) I am surprised by "return sym.name == names.getClass && 
>> !sym.isStatic();" in isInvocationOfGetClass()
>>
>
> Fixed by querying the method’s symbol instead, and added your example 
> as a negative test. This approach
>
>>
>> (3) I am checking with the right folks on the copyright notice. My 
>> understanding is that it should not mention the individual's name.
>> But I will confirm as soon as I have heard from the "horse's mouth"
>>
>
> No problem, fixed by assigning the copyright to Oracle, as Oracle is 
> free to do under the OCA anyway.
>
> Patch attached.
>
> -Jesper
>
>
>
>
> 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.javaTue 
> Jan 28 17:12:01 2020 +0530
> +++ 
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.javaMon 
> Feb 03 09:03:11 2020 +0100
> @@ -216,6 +216,11 @@
>          MODULE("module"),
>          /**
> +         * Warn about issues related to migration of JDK classes.
> +         */
> +        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.javaTue 
> Jan 28 17:12:01 2020 +0530
> +++ 
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.javaMon 
> Feb 03 09:03:11 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.javaTue 
> Jan 28 17:12:01 2020 +0530
> +++ 
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.javaMon 
> Feb 03 09:03:11 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;
> +            MethodSymbol msym = 
> (MethodSymbol)TreeInfo.symbol(apply.meth);
> +            return msym.name == names.getClass && 
> msym.implementedIn(syms.objectType.tsym, types) != null;
> +        }
> +        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.propertiesTue 
> Jan 28 17:12:01 2020 +0530
> +++ 
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.propertiesMon 
> Feb 03 09:03:11 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.propertiesTue 
> Jan 28 17:12:01 2020 +0530
> +++ 
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.propertiesMon 
> Feb 03 09:03:11 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.javaTue Jan 28 
> 17:12:01 2020 +0530
> +++ b/test/langtools/tools/javac/diags/CheckExamples.javaMon Feb 03 
> 09:03:11 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/nullThu Jan 01 00:00:00 1970 +0000
> +++ 
> b/test/langtools/tools/javac/diags/examples/CheckGetClassComparedWithInterfaceLiteral.javaMon 
> Feb 03 09:03:11 2020 +0100
> @@ -0,0 +1,31 @@
> +/*
> + * Copyright (c) 2020, Oracle and/or its affiliates. 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 <http://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/nullThu Jan 01 00:00:00 1970 +0000
> +++ 
> b/test/langtools/tools/javac/valhalla/lworld-values/CheckInterfaceComparison.javaMon 
> Feb 03 09:03:11 2020 +0100
> @@ -0,0 +1,72 @@
> +/*
> + * @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
> +    }
> +
> +    public Class<?> getClass(String x) {
> +        return null;
> +    }
> +
> +    public boolean compare(Object o, String arg, Class<?> clazz) {
> +        return getClass(arg) == Runnable.class; // No warning 
> required for non-object.getClass()
> +    }
> +}
> diff -r e2f1c4d5f39e 
> test/langtools/tools/javac/valhalla/lworld-values/CheckInterfaceComparison.out
> --- /dev/nullThu Jan 01 00:00:00 1970 +0000
> +++ 
> b/test/langtools/tools/javac/valhalla/lworld-values/CheckInterfaceComparison.outMon 
> Feb 03 09:03:11 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