Community
Participate
Working Groups
Take the following code: package nulla; import org.eclipse.jdt.annotation.NonNullByDefault; @NonNullByDefault public class NullaTest { Object foo() { return new Object(); } void bar() { o = foo(); } } Invoke the quickfix 'create local variable o'. The resulting code will be: @NonNull Object o = foo(); The @NonNull annotation is not necessary, especially not for local variables.
+1 .. this is a bit of a PITA - I'd like my code to be the least cluttered possible, so when Inherit null annotations is on, it shouldn't add @Nullable if not required, agreed?
I get your point. Still priorities are set by the team, not the user :)
New Gerrit change created: https://git.eclipse.org/r/89820
New Gerrit change created: https://git.eclipse.org/r/89821
(In reply to Eclipse Genie from comment #3) > New Gerrit change created: https://git.eclipse.org/r/89820 jdt.core change (ImportRewrite) (In reply to Eclipse Genie from comment #4) > New Gerrit change created: https://git.eclipse.org/r/89821 jdt.ui change. will obviously only compile once the jdt.core change is merged. TODO: - add tests
(In reply to Michael Vorburger from comment #1) > +1 .. this is a bit of a PITA - I'd like my code to be the least cluttered > possible, so when Inherit null annotations is on, it shouldn't add @Nullable > if not required, agreed? Sorry, but this won't be supported by the implementation I'm doing. AFAIK, "Inherit null annotations" doesn't really work with type annotations anyway and I think @Nullable should be explicit if possible. Also, this problem doesn't really match the bug title or comment#0. What *is* implemented: No redundant @NonNull added on the main type or any detail (like type args, bounds or array contents) at least for the following tools: - extract local variable - create local variable for missing symbol - create field for missing symbol - create parameter for missing symbol - override method - create method (parameters, details of return type) - change method: (add parameter) - change method: (change parameter) - assign expression to new local variable - create constructor using fields - generate delegate methods @Nullable is furthermore omitted for local variables, unless their type is a free type variable. This already includes support for @NonNullByDefault on LOCAL_VARIABLE and FIELD and with DefaultLocation.ARRAY_CONTENTS. Possible I'll add other easily adaptable cases to the current patch. Some other changes may need a little more refactoring (e.g. type changes via TypeChangeCorrectionProposal), so should they'll probably be done later in a separate bug.
To implement this, I have add some "api-level" code to ImportRewrite (in jdt.core): - enum TypeLocation - ImportRewriteContext.removeRedundantTypeAnnotations(IAnnotationBinding[], TypeLocation) - ImportRewrite.addImport(ITypeBinding, AST, ImportRewriteContext, TypeLocation) @Jay, @Markus, @Noopur, I'm adding you to the CC-List to make sure you're aware of this and OK with this - feel free the add feedback :-)
(In reply to Till Brychcy from comment #5) > TODO: > - add tests Done (In reply to Till Brychcy from comment #6) > Possible I'll add other easily adaptable cases to the current patch. Some Done: All other easily adaptable cases. I will look at more complicated cases like TypeChangeCorrectionProposal once this is merged. I have created bug 511866 for the jdt.core change.
@Stephan, I think this is ready for you to look at :-)
I'm finally looking at this. Retelling the strategy behind the change, both for documentation and to check my understanding: Clients in JDT/UI use ImportRewrite not just for adding imports but for creating Type DOM nodes in the most suitable form. Initially the main decision here was: qualified name or import plus simple name - but meanwhile much more information is provided via the incoming ITypeBinding, which is intended for creating a Type with all relevant details. In and around bug 417937 ImportRewrite was extended to also cover TYPE_USE annotations. This is the mechanism to be improved by this current bug, in order to avoid creating redundant annotations. Prior art in this area: bug 463360, which was narrower in two dimensions: - only addressed [override method] - only addressed declaration annotations (we have a follow-up bug 465335). Already bug 463360 shows that there are two parts to the solution: finding the currently applicable @NNBD, and filtering any annotations that are redundant due to this @NNBD (or for syntactic reasons). Observation: the previous bug introduced Bindings.findNullnessDefault() which uses a binding-based strategy for finding a @NNBD at an enclosing scope. By contrast the current patch adds RedundantNullnessTypeAnnotationsFilter.determineNonNullByDefaultLocations(ASTNode, String), which is purely AST based. This leads me to think that the new code may not be able to find @NNBD in package-info.java?? The new patch elegantly hides this logic (viz. setting up a filter which knows the current @NNBD) inside constructors of ContextSensitiveImportRewriteContext, so for this part, many clients in JDT/UI are already covered without any client side changes. Great! It's the second part of the solution that spreads into many locations: potentially all clients of ImportRewrite.addImport(..) should specify the purpose / location of a type to process. That's where the new enum ImportRewrite.TypeLocation comes into the picture. I still need to spend more time to inspect all locations, but generally each individual change is simple, and the chosen approach looks like a very suitable strategy to me (just for a hypothetical alternative: leaving ImportRewrite agnostic to @NNBD would require each and every client to manipulate either the type binding it passes to ImportRewrite, or modify the resulting Type AST (by removing redundant annotations) - this would cause much more trouble then what we have here). In addition to reviewing the clients of ImportRewrite.addImport(..) I still need to make up my mind regarding TypeLocation.OTHER. Two quick observations here: - some locations mentioned as irrelevant are not irrelevant for all type annotations, just for null type annotations. I.e., should we later intend to use the given mechanism also for other type annotations, then we need to revisit the assumptions made here. - I saw callers using OTHER where the type cannot be ascribed to any location, as it doesn't actually end up in the resulting AST, e.g., when using a variable's type to propose a suitable name. Would it make sense to literally cover all possible locations as per JSR 308 (plus perhaps values UNKNOWN and DONT_CARE)?
Till, with M6 approaching we need to make a call about the new API involved. Let me highlight the questions from my previous "essay": (In reply to Stephan Herrmann from comment #10) > Observation: the previous bug introduced Bindings.findNullnessDefault() > which uses a binding-based strategy for finding a @NNBD at an enclosing > scope. By contrast the current patch adds > RedundantNullnessTypeAnnotationsFilter.determineNonNullByDefaultLocations(ASTNode, > String), which is purely AST based. This leads me to think that the new code > may not be able to find @NNBD in package-info.java?? *Does* the patch find @NNBD in package-info.java? If uncertain about the best approach, this part _could_ be improved after M6 still. > Would it make sense to literally cover all possible locations as per JSR 308 > (plus perhaps values UNKNOWN and DONT_CARE)? I think this should be considered to ensure that new API introduced is not too much biased towards the current use case. => Decision necessary for inclusion in M6.
(In reply to Stephan Herrmann from comment #11) > Till, with M6 approaching we need to make a call about the new API involved. Sorry, for the slow response, I just came home from skiing :-) > > Let me highlight the questions from my previous "essay": > > (In reply to Stephan Herrmann from comment #10) > > Observation: the previous bug introduced Bindings.findNullnessDefault() > > which uses a binding-based strategy for finding a @NNBD at an enclosing > > scope. By contrast the current patch adds > > RedundantNullnessTypeAnnotationsFilter.determineNonNullByDefaultLocations(ASTNode, > > String), which is purely AST based. This leads me to think that the new code > > may not be able to find @NNBD in package-info.java?? > > *Does* the patch find @NNBD in package-info.java? Yes! Should also be checked in one of the tests. > > If uncertain about the best approach, this part _could_ be improved after M6 > still. > > > > Would it make sense to literally cover all possible locations as per JSR 308 > > (plus perhaps values UNKNOWN and DONT_CARE)? > > I think this should be considered to ensure that new API introduced is not > too much biased towards the current use case. => Decision necessary for > inclusion in M6. OK for me, actually I thought about it, but wasn't sure if this is ever needed. I would certainly feel "cleaner". Do you want to work on this? Otherwise I can have a look later this evening...
(In reply to Till Brychcy from comment #12) > > *Does* the patch find @NNBD in package-info.java? > > Yes! Should also be checked in one of the tests. Interesting. I may have to debug that test to see how it achieves that, wasn't apparent from the source. > Do you want to work on this? Otherwise I can have a look later this > evening... I'm currently busy with some other fixes, so I'd be happy to pick up/review an improved version tomorrow :)
(In reply to Stephan Herrmann from comment #13) > (In reply to Till Brychcy from comment #12) > > > *Does* the patch find @NNBD in package-info.java? > > > > Yes! Should also be checked in one of the tests. > > Interesting. I may have to debug that test to see how it achieves that, > wasn't apparent from the source. See (in RedundantNullnessTypeAnnotationsFilter.getNNBDAnnotation), the code commented as: // special case: when reaching the root of the ast, check the package annotations.
(In reply to Stephan Herrmann from comment #13) > > > Do you want to work on this? Otherwise I can have a look later this > > evening... > > I'm currently busy with some other fixes, so I'd be happy to pick up/review > an improved version tomorrow :) Sorry, I fear I am simply too tired today and time is running out, but here are the thoughts I had about this: There is some special handling: The location for type variables and wildcards is always changed to OTHER in ImportRewrite, because @NonNullByDefault must not apply to them and it certainly will never apply to OTHER. (I think this is documented somewhere, but just can't find it, so maybe more documentation should be added for that.). see: // @NonNullByDefault has no effect for type variables and wild cards Because of that, the code is specific to null annotations anyway. Maybe we should move the code that does that to another method in ImportRewriteContext? Beyond that, I see two aspects: 1) We could add constants for more locations, especially including those where null annotations are disallowed, but other type annotations are allowed (e.g. instanceof, see where IProblem.NullAnnotationUnsupportedLocationAtType is reported), even if these constant values are currently not needed in the code yet. 2) We could replace OTHER by a drilled down list of type usages disallowed for type annotations by JSR 308. ("JSR 308 does not extend Java to allow annotations on type names that are not type uses, such as the type names that appear in import statements, class literals, static member accesses, and annotation uses"). I think 1) could be done later, if needed. On the other hand, 2) doesn't seem too useful as the reason why type annotations shouldn't be present in the current situation at all shouldn't matter for other implementations either.
Following bug 511866 comment 4 I uploaded a correspondingly modified change for JDT/UI (patch set #7). First I adjusted the patch to the updated protocol. Then I was confused by the fact the ALWAYS_REDUNDANT_LOCATIONS were merged into nonNullByDefaultLocations. IMHO these are different animals, representing different reasons why some annotations should be filtered. So I tried to more clearly distinguish different reasons for filtering a type annotations: (1) TypeLocation.OTHER: all type locations are filtered (2) for a well-known set of NEVER_NULLNESS_LOCATIONS plus type variables & wildcards: - filter all null annotations (3) for locations affected by @NNBD - filter only @NonNull It's item (2) that obsoletes special re-mapping in cases of type variables and wildcards. I also added a test where a cast is generated, expecting annotations on type arguments to be copied, but no annotation on the top-level type.
(In reply to Stephan Herrmann from comment #16) > Following bug 511866 comment 4 I uploaded a correspondingly modified change > for JDT/UI (patch set #7). > > First I adjusted the patch to the updated protocol. Then I was confused by > the fact the ALWAYS_REDUNDANT_LOCATIONS were merged into > nonNullByDefaultLocations. IMHO these are different animals, representing > different reasons why some annotations should be filtered. > > So I tried to more clearly distinguish different reasons for filtering a > type annotations: > > (1) TypeLocation.OTHER: all type locations are filtered > > (2) for a well-known set of NEVER_NULLNESS_LOCATIONS plus type variables & > wildcards: > - filter all null annotations > > (3) for locations affected by @NNBD > - filter only @NonNull > > It's item (2) that obsoletes special re-mapping in cases of type variables > and wildcards. > > I also added a test where a cast is generated, expecting annotations on type > arguments to be copied, but no annotation on the top-level type. Awesome improvements, thanks a lot! Just one thing has crept in (obviously caused by my inappropriate misuse of OTHER): As NonNullByDefault doesn't apply to type variables and wildcards, any nullness annotation on them should be KEPT not REMOVED. Fixed in patch set 8 (including tests)
I also introduced another quirk: without thinking I named one constant in TypeLocation as NEW_. In capitalized spelling the trailing underscore is rubbish. When looking for uses of NEW_, I found none in existing quickfix implementations. After searching a bit, I found LambdaExpressionsFix.CreateAnonymousClassCreationOperation as a cool candidate how we could test treatment of new expressions wrt type annotations. Drilling into its implementation I arrived at ASTNodeFactory.newCreationType(AST, ITypeBinding, ImportRewrite, ImportRewriteContext) which has a few calls into ImportRewrite, which all(?) could be augmented with a location argument. ASTNodeFactory looks like a very suitable place for making such changes. As I don't want to further interfere with your work, may I suggest: I believe today's I-build to happen at 2PM EST, which should be 8PM CET. Please make sure that this build picks up the final version of the API in JDT/Core. The result of that build should then be visible to the JDT/UI gerrit job. I will then check when the latter job succeeds and release it.
Well, seeing no response I *did* push the correction from NEW_ to NEW, to not risk API getting published with the bogus name (JDT/Core only, so far).
(In reply to Stephan Herrmann from comment #19) > Well, seeing no response I *did* push the correction from NEW_ to NEW, to > not risk API getting published with the bogus name (JDT/Core only, so far). Sorry, I didn't have enough time to write an answer earlier today. I would have done it later, but thanks for doing it.
(In reply to Stephan Herrmann from comment #11)> > Let me highlight the questions from my previous "essay": BTW, Great explanation of the how and why, thanks for writing it! (In reply to Stephan Herrmann from comment #18) > When looking for uses of NEW_, I found none in existing quickfix > implementations. After searching a bit, I found > LambdaExpressionsFix.CreateAnonymousClassCreationOperation as a cool > candidate how we could test treatment of new expressions wrt type > annotations. Drilling into its implementation I arrived at > ASTNodeFactory.newCreationType(AST, ITypeBinding, ImportRewrite, > ImportRewriteContext) which has a few calls into ImportRewrite, which all(?) > could be augmented with a location argument. ASTNodeFactory looks like a > very suitable place for making such changes. Good idea for 4.7M7 :-)
Gerrit change https://git.eclipse.org/r/89821 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=3b6698b8ddb0e1f6566e324d6a8b4e87397a8b97
(In reply to Eclipse Genie from comment #22) > Gerrit change https://git.eclipse.org/r/89821 was merged to [master]. > Commit: > http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/ > ?id=3b6698b8ddb0e1f6566e324d6a8b4e87397a8b97 Released the most part for 4.7 M6. Unfortunately, my latest patch set missed the changes from the previous patch set #8, these will be supplied later today.
New Gerrit change created: https://git.eclipse.org/r/92412
(In reply to Stephan Herrmann from comment #23) > Unfortunately, my latest patch set missed the changes from the previous > patch set #8, these will be supplied later today. (In reply to Eclipse Genie from comment #24) > New Gerrit change created: https://git.eclipse.org/r/92412 I've prepared this gerrit for the missed changes from patch set #8
(In reply to Till Brychcy from comment #25) > (In reply to Stephan Herrmann from comment #23) > > Unfortunately, my latest patch set missed the changes from the previous > > patch set #8, these will be supplied later today. > > (In reply to Eclipse Genie from comment #24) > > New Gerrit change created: https://git.eclipse.org/r/92412 > > I've prepared this gerrit for the missed changes from patch set #8 Very convenient :) thanks (If only jenkins would behave more reliably - I just retriggered after infra-related failure).
Gerrit change https://git.eclipse.org/r/92412 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=2300e8ec0811db9e87389488eb05bc120a87b089
(In reply to Eclipse Genie from comment #27) > Gerrit change https://git.eclipse.org/r/92412 was merged to [master]. > Commit: > http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=2300e8ec0811db9e87389488eb05bc120a87b089 > While jenkins/nexus where not in the office, I locally ran (most) tests and then released the addendum for 4.7 M6. Thanks, Till!
