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