Community
Participate
Working Groups
I was trying to use null annotations in o.e.jdt.ui but it becomes extremely hard very quickly. Actually I should say it is impossible to use them today. (Or am I missing something?) 1. I started off by adding @NonNull to those places where a parameter or a return value is specified to be non-null either via a javadoc or an assert statement. However, there were too many such places, and I thought that this was not the right approach. 2. Next I thought maybe I should just specify @NonNullByDefault on all the packages and then add @Nullable where ever needed. However, there are too many packages and none of them have package-info.java. I certainly do not want to have to do all this manually. - Need a cleanup to create package-info.java in all packages where missing and specify the default. - Need a quick assist/clean-up to replace statements like Assert.isNotNull(obj) with @NonNull on obj. - Maybe we can also have quick assists/clean-up for adding appropriate annotations based on javadoc. e.g. if the javadoc for a parameter includes 'not null' we can propose to add @NonNull to the parameter. Just a thought... Also, I am still not sure what is the right approach - 1 or 2. Is one approach better than other? Or it really depends on case to case? If one is better than other we should try to convey that to the user through documentation, or through adding quick fixes/assists and clean-ups for that approach.
Another possibility would be that the Quick Fix which adds the library instead invokes a wizard that helps with the initial setup.
(In reply to comment #0) Deepak, thanks for reporting your observations. > ... it is impossible to use them today. > (Or am I missing something?) There's an important distinction: what you report may be very true for existing code, but for new code adding annotations as you write the code is very straight-forward (modulo bug 331651). > 1. I started off by adding @NonNull to those places where a parameter or a > return value is specified to be non-null either via a javadoc or an assert > statement. However, there were too many such places, and I thought that this > was not the right approach. I think for existing projects this is actually a good approach, just it calls for more automation. > 2. Next I thought maybe I should just specify @NonNullByDefault on all the > packages and then add @Nullable where ever needed. However, there are too many > packages and none of them have package-info.java. I certainly do not want to > have to do all this manually. Yep. > - Need a cleanup to create package-info.java in all packages where missing and > specify the default. Agree. That's basically the idea behind bug 372012. > - Need a quick assist/clean-up to replace statements like Assert.isNotNull(obj) > with @NonNull on obj. cool, yes please :) > - Maybe we can also have quick assists/clean-up for adding appropriate > annotations based on javadoc. e.g. if the javadoc for a parameter includes 'not > null' we can propose to add @NonNull to the parameter. Just a thought... lovely... > Also, I am still not sure what is the right approach - 1 or 2. Is one approach > better than other? Or it really depends on case to case? Yes, it depends. One way to find out is to add @NonNullByDefault to one package at the time (starting at 'the bottom'). Eventually, we'd even want refactorings to extract/inline the nullness default, which means you could start with one approach and than switch to the other. > If one is better than > other we should try to convey that to the user through documentation, or > through adding quick fixes/assists and clean-ups for that approach. all of the above. I'll be happy to help with both the documentation and more quick fixes / assists. I guess documentation is the only thing we can still do for Juno, right?
(In reply to comment #2) > Eventually, we'd even want refactorings to extract/inline the nullness default, > which means you could start with one approach and than switch to the other. That could also be interesting... > I'll be happy to help with both the documentation and more quick fixes / > assists. I guess documentation is the only thing we can still do for Juno, > right? Yeah, documentation is the only thing we can do now. But mid June is not far away, when we can start adding more features again. Though you can always start the work in a separate branch if you have nothing else to do for Juno :-)
I briefly tried to set this up but had trouble figuring it out. I found the compiler preference for annotation-based null analysis, and turned it on. Now what - I guess I need to add something to my project build path, but what? I happened to know about the org.eclipse.jdt.annotation bundle and I knew where to find it on disk, so I added to my bundle's classpath manually and this seems to work. But it seems like it should offer to setup my project classpath as soon as I enable the preference to use it.
Thanks for trying this John! (In reply to comment #4) > I briefly tried to set this up but had trouble figuring it out. I found the > compiler preference for annotation-based null analysis, and turned it on. > Now what - I guess I need to add something to my project build path, but > what? :-) > I happened to know about the org.eclipse.jdt.annotation bundle and I > knew where to find it on disk, so I added to my bundle's classpath manually > and this seems to work. But it seems like it should offer to setup my > project classpath as soon as I enable the preference to use it. Maybe... but We do have quick fixes which set up project classpath as soon as you use one of the annotations in your code. Maybe we need a wizard as suggested in comment 1 to enable the compiler preferences, setup the project classpath, and do some more stuff...
Just to set the expectations straight: I don't think we have a convincing story yet, since - we're missing external annotations (e.g. for libraries, bug 331651), and - we don't know yet what's the way to go for annotations on fields (bug 331649, bug 383371) Therefore, the null annotations support should currently be considered beta. We don't plan to spend more resources on making it easier to set up null annotations, unless these problems are resolved.
I also tried to use these annotations, and I ran into the following problems: 1. When using arrays, it would be nice to have an annotation that allows declaring the array as @NonNullElements (or similar). This would greatly simplify code when it sets/gets array elements. 2. For a library collection, if it supports null values, then get is @Nullable and set allows @Nullable values. A specific use of the collection should be able to override this and enforce @NonNullElements. Otherwise one gets a lot of conflicts of trying to assign @Nullable values to @NonNull fields and I have not yet figured out how to solve this conflict. This example generates a "Null type mismatch" error: MyList<E> l = ...; // get(int) has @Nullable return value. // Throws exception for invalid indices. @NonNull E element = l.get(0); // assuming there is at least one element Any suggestion how to work around this (without changing the compiler error settings)? I truly believe that this feature is a significant contribution. Just by trying to implement this in two packages, I felt that it increases confidence in the code significantly, even though I did not actually find any problems with my code. However, I wonder how far one can go without making this a standard part of Java (OpenJDK contribution?) and support it at least on all major Java IDEs (and I would agree that Eclipse is the best place to start).
I found a work around for the problem just reported: use an assertion that verifies non-nullness. This example avoids the error message: MyList<E> l = ...; // get(int) has @Nullable return value. // Throws exception for invalid indices. E e = l.get(0); // assuming there is at least one element assert e != null; @NonNull E element = e; // No error! While this works fine, it does require extra code.
(In reply to comment #7) > 1. When using arrays, it would be nice to have an annotation that allows > declaring the array as @NonNullElements (or similar). This would greatly > simplify code when it sets/gets array elements. This wouldn't solve the issue of multi-dimensional arrays, but fortunately a complete solution is around the corner: by way of JSR 308 we'll be able to say: @Nullable String @NonNull[] args; to denote an array (can be null) of non-null elements. JSR 308 will be part of Java 8. There's still some time until the release of Java 8, but this also means we will not invent any interim solution now. > 2. For a library collection, if it supports null values, then get is > @Nullable and set allows @Nullable values. A specific use of the collection > should be able to override this and enforce @NonNullElements. The full answer is again JSR 308, here we'll be able to say: @Nullable List<@NonNull String> args; with pretty much the same meaning as above, viz: all occurrences T in List<T> will be substituted with "@NonNull String" (which assumes "T get(int)" does *not* have any null annotations. > Otherwise one > gets a lot of conflicts of trying to assign @Nullable values to @NonNull > fields and I have not yet figured out how to solve this conflict. This > example generates a "Null type mismatch" error: > > MyList<E> l = ...; // get(int) has @Nullable return value. > // Throws exception for invalid indices. > @NonNull E element = l.get(0); // assuming there is at least one element > > Any suggestion how to work around this (without changing the compiler error > settings)? For now you might try to work around this using inheritance: public class MyList<T> extends ArrayList<T> { @Override @SuppressWarnings("null") public boolean add(@NonNull T e) { return super.add(e); } @Override @SuppressWarnings("null") public @NonNull T get(int index) { return super.get(index); } } Unfortunately, for this to work you have to enable "Suppress optional errors with @SuppressWarnings", but if you apply this pattern consistently, class MyList will be safe despite all the suppress problems, and it will be the only place where you have to fight this current limitation (so it might improve over your version from comment 8). > I truly believe that this feature is a significant contribution. Just by > trying to implement this in two packages, I felt that it increases > confidence in the code significantly, even though I did not actually find > any problems with my code. Cool. Thanks for sharing your experience. > However, I wonder how far one can go without > making this a standard part of Java (OpenJDK contribution?) and support it > at least on all major Java IDEs (and I would agree that Eclipse is the best > place to start). The canonical place to make this a standard would have been JSR 305 which unfortunately is "dormant". I'm not enough into "politics" to bring it back to life.
(In reply to comment #9) > (In reply to comment #7) Thanks, Stephan. JSR 308 rocks! Also see my second posting for the workaround. The nice thing about the assertion is that it does not cause a performance penalty in a production environment. The local variable will probably be inlined at runtime.
I'd like to suggest more quick fixes for these two situations: 1. On a class declaration with warning: "A default nullness annotation has not been specified for the type SomeClass" a quickfix could offer to: a) insert @NonNullByDefault directly above into this class b) create a package-info.java with @NonNullByDefault for the current package Both should remove redundant @NonNull and @NonNullByDefault annotations within the scope of the newly added @NonNullByDefault to not introduce more warnings. 2. Consider this snippet: class MyClass { @NonNull private final Object member; protected MyClass(@Nullable final Object parameter) { member = parameter; // error } } "Null type mismatch: required '@NonNull Object' but the provided value is specified as @Nullable" The illegal assignment can be resolve by either: a) making parameter @NonNull (already implemented) or b) making member @Nullable (missing)
Ups, the existing quick fix for case 2.a) does not respect an eventually applying @NonNullByDefault. Instead of always changing to @NonNull, it should remove the @Nullable and let the default apply, to not raise another warning of redundant @NonNull
Another ups :-) @NonNullByDefault public class MyClass { @NonNull // <- here private final Object member = 1; } The quickfix to remove the redundant @NonNull does not cover member fields yet, though it is offered correctly. Issued via keyboard ctrl-1, nothing happens. Issued via mouse-click in the hover an error-dialog says: An Exception occurred while applying this quick fix. Reason: The fix 'Remove redundant nullness annotation' generated a null change.
Generating getters and setters for nullable fields does not inherit the correct annotation. Any quickfix to toggle nullness of a field should also update getters and setter.
Cast obscures nullness of a field: class NPEonCast { @Nullable private Object nullable; public void test() { if (nullable instanceof Number) { ((Number)nullable).intValue(); // A } if (nullable != null) { nullable.toString(); // B } nullable.toString(); // C } } Error missing for A (while correct for B and C)
(In reply to comment #15) Thanks for the test case, I've filed bug 401017 to track this issue.
Created attachment 229409 [details] proposed fix for comment 12 (In reply to comment #12) > Ups, the existing quick fix for case 2.a) does not respect an eventually > applying @NonNullByDefault. Instead of always changing to @NonNull, it > should remove the @Nullable and let the default apply, to not raise another > warning of redundant @NonNull Here's a fix for situations like this: @NonNullByDefault class Target { @Nullable Object bar() { return this; } } class Client { @NonNull Object foo(Target t) { return t.bar(); // error, need a non-null value but got nullable } } The fix does 3 things: - enable the quick fix for changing bar() (proposal creation just wasn't triggered along that particular path) - make all quick fixes of this kind respect the existing @NonNullByDefault (including @NonNullByDefault(false)) - clean up some TODO and minor code issues. Note: patch was built on top of patches in bug 400668, bug 405086 and bug 395555.
As this bug became a dump for lots of different things let me collect a few issues that might be worth pursuing (now or later) (1) Refactorings for migrating from individual annotations to @NonNullByDefault (comment 2 ff) (2) Proposals for adding missing @NonNullByDefault (comment 11 (1)) (3) adjust nullity of a field (comment 11 (2.b)) (4) respect @NonNullByDefault when changing from @Nullable to @NonNull (comment 12) (5) bug "The fix '...' generated a null change" (comment 13) What's the status of these: (1) doesn't really require a refactoring: just add the @NonNullByDefault and once the cleanup for removing redundant @NonNull works reliably that's all you need. (2) is simple and valid and could be addressed via this bug. (3) not a priority since adding @NonNull to a field is a conscious design decision, and reverting that decision will (hopefully) not be a frequent scenario. Local changes are more suitable for quick fixes. Feel free to file an explicit, motivated RFE, however. (4) done as of comment 17 (5) is already being fixed via bug 400668. So I'm only planning to address (2) via this bug. Once that's done I suggest to close this bug and ask people to add new bugs with specific requests.
(In reply to comment #18) > (2) Proposals for adding missing @NonNullByDefault (comment 11 (1)) > [...] > (2) is simple and valid and could be addressed via this bug. While working on a quick fix for adding a fresh new package-info.java with @NonNullByDefault, I'm at the point where the quick fix works in unit tests, but it is never offered in the UI, because org.eclipse.jdt.internal.ui.text.correction.CorrectionMarkerResolutionGenerator only considers markers with an associated CU. I'm wondering if there's any precedent for this? I guess this is the first relevant marker that is applied to a package, right? If my guess is right, offering such a quick fix would need some extension in ContributedProcessorDescriptor or s.t. in that vicinity. I further guess that this makes it an unlikely candidate for Kepler, right?
What's your view on tighter binding between fields and their respective getters and setters as per comment 14 ?
(In reply to comment #20) > What's your view on tighter binding between fields and their respective > getters and setters as per comment 14 ? For generating getters/setters this should be pretty straight forward. For updating existing getters/setters I'm not so sure. Typically, quick fixes don't affect many different code locations, so sounds more like a refactoring? Part of it will be offered via follow-up quick fixes any way: Making a field @NonNull will create an error at the setter, which is quick fixable, right? Anyway, these would touch code regions that I'm not so well familiar with, so that'll have to wait to Luna.
Comment on attachment 229409 [details] proposed fix for comment 12 Thanks for the patch. Committed with http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=17c7783177bc359beb03a7575237fd347cc3ba49 together with other fixes.
.
