8237074: Warning when obj.getClass() == SomeItf.class (from State-of-Valhalla)
Srikanth
srikanth.adayapalam at oracle.com
Wed Jan 29 05:18:59 UTC 2020
Another comment:
(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.
Thanks
Srikanth
On 28/01/20 7:36 pm, Srikanth wrote:
>
> 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