Community
Participate
Working Groups
The following section in the spec has been struck out but notable is this one: "It is a compile-time error if the ReferenceType mentioned after the instanceof operator does not denote a reference type that is reifiable (4.7)." As per this, Javac has started accepting the following code at compliance 14 but continues to reject at levels 13 and below: public boolean foo(T obj) { if (obj instanceof T) { return false; } return true; }
New Gerrit change created: https://git.eclipse.org/r/161369
New Gerrit change created: https://git.eclipse.org/r/161370
Gerrit change https://git.eclipse.org/r/161369 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=7bd737f958efefb05ab8cc567eff85d4f5efca69
Gerrit change https://git.eclipse.org/r/161370 was merged to [BETA_JAVA14]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=2f46ebebf9b4524ef55179986f4aea95fdc652c8
Sorry for reopening, but I think now not having an error at all is wrong in the following situation. If you already have another bug for this, just reclose. E.g. for : public class Test<T> { public boolean foo(Object obj) { if (obj instanceof T) { return false; } return true; } } javac --enable-preview --release 14 reports: src/cast14/Test.java:5: error: Object cannot be safely cast to T if (obj instanceof T) { ^ where T is a type-variable: T extends Object declared in class Test Note: src/cast14/Test.java uses preview language features. Note: Recompile with -Xlint:preview for details. 1 error (I guess this would be the last sentence of 14.30.2 in https://docs.oracle.com/javase/specs/jls/se14/preview/specs/patterns-instanceof-jls.html#jls-15.20.2)
(In reply to Till Brychcy from comment #5) > Sorry for reopening, but I think now not having an error at all is wrong in > the following situation. If you already have another bug for this, just > reclose. > Thanks Till, I will fix it right here. On seeing your report, I thought I didn't pay close attention and I may have simply adjusted all failing tests. But AFAIK, we weren't testing this particular case, which is surprising.
New Gerrit change created: https://git.eclipse.org/r/161422
(In reply to Eclipse Genie from comment #7) > New Gerrit change created: https://git.eclipse.org/r/161422 (In reply to Jay Arthanareeswaran from comment #6) > But AFAIK, we weren't testing this particular case, which is surprising. Sorry, there is a test, indeed. I am including the modified test in the patch. Just going by the quoted spec: "At compile time, the instanceof operator (15.20.2) checks compatibility of its first operand, an expression, with the type of its second operand." a simple isCompatibleWith() check seems to be enough. But then I am not an expert in this area. Till, can you take a look at the change, please?
(In reply to Jay Arthanareeswaran from comment #8) > (In reply to Eclipse Genie from comment #7) > > New Gerrit change created: https://git.eclipse.org/r/161422 > Till, can you take a look at the change, please? I've only had a quick look, but I doubt it is enough. See (ii) in the previous sentence in https://docs.oracle.com/javase/specs/jls/se14/preview/specs/patterns-instanceof-jls.html#jls-14.30.2: "An expression that is not the null literal is compatible with a type test pattern of type T if (i) the expression can be converted to type T by casting conversion (5.5), and (ii) the casting conversion does not make use of a narrowing reference conversion which is unchecked (5.1.6.2)." So I guess the code that is (currently) in org.eclipse.jdt.internal.compiler.ast.CastExpression.checkUnsafeCast(Scope, TypeBinding, TypeBinding, TypeBinding, boolean) and sets ASTNode.UnsafeCast probably needs to be used. I haven't tried, but maybe the core logic can be extracted in a static method, so it can be reused from InstanceOfExpression? Also, javac's error message is better.
New Gerrit change created: https://git.eclipse.org/r/161466
(In reply to Till Brychcy from comment #9) > "An expression that is not the null literal is compatible with a type test > pattern of type T if (i) the expression can be converted to type T by > casting conversion (5.5), and (ii) the casting conversion does not make use > of a narrowing reference conversion which is unchecked (5.1.6.2)." These two conditions are covered by checkCastTypesCompatibility(), which is what we use in InstanceofExpression#resolveType(). My quick attempt proves this to be inaccurate for the testcase from comment #5. (In reply to Eclipse Genie from comment #10) > New Gerrit change created: https://git.eclipse.org/r/161466 This reverts the fix (tests are kept but disabled) and will leave it to the experts of this area (read Stephan/Till) to take this over.
Gerrit change https://git.eclipse.org/r/161466 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=ee3ca7af25d58effe0bfa136078955ba02fff70e
New Gerrit change created: https://git.eclipse.org/r/162886
(In reply to Eclipse Genie from comment #13) > New Gerrit change created: https://git.eclipse.org/r/162886 This fixes the original issue and other cases reported so far. Pretty much along what Till suggested in comment #9. When I last tried this, I overlooked the fact that Bit8 is used both by the ASTNode.UnsafeCast as well as operator IDs. I have fixed this now. Till/Stephan, do either of you have some time to go over the patch for M3?
(In reply to Jay Arthanareeswaran from comment #14) > (In reply to Eclipse Genie from comment #13) > > New Gerrit change created: https://git.eclipse.org/r/162886 > > This fixes the original issue and other cases reported so far. Pretty much > along what Till suggested in comment #9. When I last tried this, I > overlooked the fact that Bit8 is used both by the ASTNode.UnsafeCast as well > as operator IDs. I have fixed this now. This kind of collisions is mean. The solution works, but it might be hard to find this place, if somebody later tries to change the ASTNode constants to resolve this kind of conflict (maybe by changing bits to long). It would be good to add at least a comment at ASTNode.UnsafeCast pointing to the location in InstanceOfExpression. Maybe also a JUnit with similar comment that tests that (ASTNode.UnsafeCast & (OperatorIds.INSTANCEOF << ASTNode.OperatorSHIFT)) != 0. > > Till/Stephan, do either of you have some time to go over the patch for M3? I also added a note in gerrit about a commented method. LGTM otherwise.
(In reply to Till Brychcy from comment #15) > (In reply to Jay Arthanareeswaran from comment #14) > > (In reply to Eclipse Genie from comment #13) > > > New Gerrit change created: https://git.eclipse.org/r/162886 > > > > This fixes the original issue and other cases reported so far. Pretty much > > along what Till suggested in comment #9. When I last tried this, I > > overlooked the fact that Bit8 is used both by the ASTNode.UnsafeCast as well > > as operator IDs. I have fixed this now. > > This kind of collisions is mean. The solution works, but it might be hard to > find this place, if somebody later tries to change the ASTNode constants to > resolve this kind of conflict (maybe by changing bits to long). Yes, please unclutter this bit space. Similarly, I recently suggested to Manoj to start a second set of tag bits for type bindings (which was already long). Reusing bit constants is quite painful.
(In reply to Stephan Herrmann from comment #16) > Yes, please unclutter this bit space. Similarly, I recently suggested to > Manoj to start a second set of tag bits for type bindings (which was already > long). Reusing bit constants is quite painful. I have removed that code, but by pushing out the operator bits by couple of positions to the left. AFAIK, the newly used bits, 13 and 14 don't seem to have any clash. Also the previous patch was only checking narrowing conversions. I have fixed this by calling checkCastTypesCompatibility() instead of checkUnsafeCast() and this required some more refactoring by bringing a overridden checkUnsafeCast() in InstanceOfExpression. Sometime soon, we should run all GenericTypeTest with --enable-preview and compare with Javac.
Now it looks really good! Just one suggestion: Maybe I've overlooked it, but I didn't see a test case that does a non-trivial instanceof test that is now allowed. Maybe it would be good to add one, e.g.: import java.util.ArrayList; import java.util.List; import java.util.function.Function; import java.util.function.UnaryOperator; public class Test<T> { public boolean foo(Function<ArrayList<T>, ArrayList<T>> obj) { if (obj instanceof UnaryOperator<? extends List<T>>) { return false; } return true; } }
(In reply to Till Brychcy from comment #18) > Now it looks really good! > > Just one suggestion: Maybe I've overlooked it, but I didn't see a test case > that does a non-trivial instanceof test that is now allowed. Maybe it would > be good to add one, e.g.: Thanks Till and Stephan. I have added more tests and released the patch now.
The git patch was not shared here. Here it is: https://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=9295bf84c9dbff60af20ec946d9d9cb0c37008df
Verified for Eclipse Version: 2020-06 4.16 M3 with Build id: I20200519-1800
