Bugzilla – Full Text Bug Listing
| Summary: | [quick assist] Lambda: improve handling of wildcards & captures and avoid creating redundant @NonNull | ||
|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Till Brychcy <register.eclipse> |
| Component: | UI | Assignee: | Till Brychcy <register.eclipse> |
| Status: | RESOLVED FIXED | QA Contact: | Stephan Herrmann <stephan.herrmann> |
| Severity: | normal | ||
| Priority: | P3 | CC: | markus.kell.r |
| Version: | 4.7 | ||
| Target Milestone: | 4.7 M7 | ||
| Hardware: | PC | ||
| OS: | Mac OS X | ||
| See Also: |
https://git.eclipse.org/r/93841 https://bugs.eclipse.org/bugs/show_bug.cgi?id=443146 https://bugs.eclipse.org/bugs/show_bug.cgi?id=514991 https://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=d560a31d4a90bee367ebfb27ae5d826d07dc7ac5 |
||
| Whiteboard: | |||
New Gerrit change created: https://git.eclipse.org/r/93841 @Stephan, this is the cool candidate (for using TypeLocation.NEW) you mentioned in bug 443146# :-) In patch set 3 I've just updated the copyright years. @Stephan, I think this is ready to be commited, can you please do that? TIA (In reply to Till Brychcy from comment #0) > While adding TypeLocation-parameters to avoid creating redundant > @NonNull-Annotations to quick assists for lambdas, I noted a few things: > > In ASTNodeFactory.newCreationType, the > "else if (typeBinding.isParameterizedType()) {" > is dead code... > [...] > ... it is not needed at all, because the recursive invocation > that removes all nested wildcards is bogus. > [...] > Another problem I noted was that captures are not handled at all > [...] > Finally, in ASTNodeFactory.newCreationType, top level type annotations on a > parametrized type were forgotten. All are good findings. @Markus: since you added the method in question (on behalf of bug 424273), do you want to check if we miss-read any of your intentions? Should we split the bug correction part (concerning ASTNodeFactory) into a separate bug for easier reviewing? Otherwise I'll continue a thorough review after a quick nod from your side. From what I see on first glance, all points raised by Till seem to be valid. I only fail to see an example where the toplevel annotation thing would make a difference (given that @NonNull should never be generated for 'new' expressions). @Till, is this covered by any of your tests? (In reply to Stephan Herrmann from comment #4) > (In reply to Till Brychcy from comment #0) > > Finally, in ASTNodeFactory.newCreationType, top level type annotations on a > > parametrized type were forgotten. > > I only fail to see an example where the toplevel annotation thing would make > a difference (given that @NonNull should never be generated for 'new' > expressions). @Till, is this covered by any of your tests True, it will never make a difference for toplevel @NonNull-Annotations - I noted the problem, while adding typelocation-arguments to the original recursive implementation, and the state of the tests from that phase was covering it. With the recursion removed in the current implementation, it would make a difference if a typebinding with other top-level type annotations was passed in, but I was not able to create a test case for that. (In reply to Till Brychcy from comment #5) > (In reply to Stephan Herrmann from comment #4) > > (In reply to Till Brychcy from comment #0) > > > Finally, in ASTNodeFactory.newCreationType, top level type annotations on a > > > parametrized type were forgotten. > > > > I only fail to see an example where the toplevel annotation thing would make > > a difference (given that @NonNull should never be generated for 'new' > > expressions). @Till, is this covered by any of your tests > > True, it will never make a difference for toplevel @NonNull-Annotations - I > noted the problem, while adding typelocation-arguments to the original > recursive implementation, and the state of the tests from that phase was > covering it. > With the recursion removed in the current implementation, it would make a > difference if a typebinding with other top-level type annotations was passed > in, but I was not able to create a test case for that. Wouldn't s.t. like @Immutable Comparator<List<?>> c = ... request a toplevel type annotation @Immutable on the creation type? (In reply to Stephan Herrmann from comment #6) > Wouldn't s.t. like > @Immutable Comparator<List<?>> c = ... > request a toplevel type annotation @Immutable on the creation type? I tried to extend the existing unit test with someting like that and failed, but now I can see it works in an editor - I'll retry with the unit test. (In reply to Till Brychcy from comment #7) > I tried to extend the existing unit test with someting like that and failed, > but now I can see it works in an editor - I'll retry with the unit test. Done in patch set 4. It doesn't work if the target is a local variable: public class Example { @java.lang.annotation.Target(java.lang.annotation.ElementType.TYPE_USE) public @interface X { } interface SAM<T> { T f(T t); } void f() { @X SAM<String> c = a -> a; c.hashCode(); // just avoid the unused warning } } but it does work if it is a field as in the added test case (return type of a method works, too): public class Example { @java.lang.annotation.Target(java.lang.annotation.ElementType.TYPE_USE) public @interface X { } interface SAM<T> { T f(T t); } @X SAM<String> c = a -> a; } (In reply to Till Brychcy from comment #8) For the local variable case, the resolved type of the LambdaExpression appears in the debugger as: ParameterizedTypeBinding[153]=Example.SAM<String> For the field case, it appears as: ParameterizedTypeBinding[152]=Example.@X SAM<String> I'll create a jdt.core bug for this. (In reply to Till Brychcy from comment #9) > I'll create a jdt.core bug for this. Bug 514991 (In reply to Stephan Herrmann from comment #4) > @Markus: since you added the method in question (on behalf of bug 424273), [...] > Otherwise I'll continue a thorough review after a quick nod from your side. > @Markus, I guess you haven't seen this - Stephan is still waiting for you "quick nod" In patch set #7 I made two changes: - Replace one more code duplicate with a call to replaceWildcardsAndCaptures() - Pull TypeLocation.NEW into newCreationType(), I don't believe we want to create creation types in other positions than "new" Let me know if I misunderstood s.t. I didn't fully grasp the reasoning why newCreationType() doesn't need an arm for array types. Obviously it was never missed (being dead code), but what did you mean in comment 0 speaking about the "recursive invocation" being bogus? Other than that (lack of understanding on my part), all looks fine to me. Message from gerrit: java.io.IOException: No space left on device (In reply to Stephan Herrmann from comment #12) > In patch set #7 I made two changes: > > - Replace one more code duplicate with a call to > replaceWildcardsAndCaptures() Good! > > - Pull TypeLocation.NEW into newCreationType(), I don't believe we want to > create creation types in other positions than "new" > > Let me know if I misunderstood s.t. > > > I didn't fully grasp the reasoning why newCreationType() doesn't need an arm > for array types. Obviously it was never missed (being dead code), but what > did you mean in comment 0 speaking about the "recursive invocation" being > bogus? The recursive invocation that was in the first if (typeBinding.isParameterizedType())-branch of newCreationType, and which lead to this, incorrect result: Comparator<List<?>> c = new Comparator<List<Object>> This invocation was also the reason I added a typeLocation parameter to this method (to to be able switch to TypeLocation.TYPE_ARGUMENT). As this is not needed anymore, your change to remove the parameter was a good idea. > > Other than that (lack of understanding on my part), all looks fine to me. Thanks! (In reply to Till Brychcy from comment #14) > (In reply to Stephan Herrmann from comment #12) > > I didn't fully grasp the reasoning why newCreationType() doesn't need an arm > > for array types. Obviously it was never missed (being dead code), but what > > did you mean in comment 0 speaking about the "recursive invocation" being > > bogus? > > The recursive invocation that was in the first if > (typeBinding.isParameterizedType())-branch of newCreationType, and which > lead to this, incorrect result: > Comparator<List<?>> c = new Comparator<List<Object>> Thanks. At this point I decided to document the purpose of this method: Create a Type suitable as the creationType in a ClassInstanceCreation expression. :) With all bogus recursion gone, the typeBinding can only be a class or interface type (parameterized or plain). With this contract, it's obvious that neither of arrays, wildcards nor captures should be expected / handled. So I removed that other "probably unused" branch as well (patch set # Gerrit change https://git.eclipse.org/r/93841 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=d560a31d4a90bee367ebfb27ae5d826d07dc7ac5 (In reply to Eclipse Genie from comment #16) > Gerrit change https://git.eclipse.org/r/93841 was merged to [master]. > Commit: > http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=d560a31d4a90bee367ebfb27ae5d826d07dc7ac5 > Released for 4.7 M7. Thanks! |
While adding TypeLocation-parameters to avoid creating redundant @NonNull-Annotations to quick assists for lambdas, I noted a few things: In ASTNodeFactory.newCreationType, the "else if (typeBinding.isParameterizedType()) {" is dead code. It should have been "else if (typeBinding.isArrayType()) {" but then I noted it is not needed at all, because the recursive invocation that removes all nested wildcards is bogus. For example, doing "Convert to anonymous class creation" in: import java.util.Comparator; import java.util.List; public class Lambda1 { Comparator<List<?>> c = (l1, l2) -> Integer.compare(l1.size(), l2.size()); } results in the invalid result: import java.util.Comparator; import java.util.List; public class Lambda1 { Comparator<List<?>> c = new Comparator<List<Object>>() { @Override public int compare(List<?> l1, List<?> l2) { return Integer.compare(l1.size(), l2.size()); } }; } Another problem I noted was that captures are not handled at all, e.g. when doing "Convert to method reference", "Add inferred lambda parameter types", or "Convert to anonymous class creation" for the lambda in: public class Lambda2 { interface Sink<T> { void receive(T t); } interface Source<U> { void sendTo(Sink<? super U> c); } void f(Source<? extends Number> source) { source.sendTo(a -> a.doubleValue()); } } Finally, in ASTNodeFactory.newCreationType, top level type annotations on a parametrized type were forgotten.