Community
Participate
Working Groups
This was discovered and reported as issue (25) and (26) during code review here: https://bugs.eclipse.org/bugs/show_bug.cgi?id=365387#c17 I am spawning a separate bug so that this can be tracked independently for verification purposes. A patch with fix & tests is available at https://bugs.eclipse.org/bugs/show_bug.cgi?id=365387#c32
Ayush, please review in early M5 for inclusion.
The changes look good. only issue I found was in the following example: void callVarargs(@Nullable String... str) { String[] str = null; vargsMethod(1, null, "", null ); // warning on each element:bad vargsMethod(1, null); // warning: good vargsMethod(1, str); // warning:good } public @NonNull Object vargsMethod(int i, @NonNull final String... objects) { objects[1] = ""; System.out.println(objects); return objects; } I think its better to treat the @NonNull as a modifier on the varargs parameter reference as a whole and not individual elements (just like the adding the final modifier only enforces the whole reference to be final and not the referands). So passing null as a lone argument or a null array as an argument should trigger the warning, but passing individual elements shouldn't, since individual elements will be collected into an array and then passed into the method.
(In reply to comment #2) > The changes look good. > only issue I found was in the following example: > > void callVarargs(@Nullable String... str) { > String[] str = null; > vargsMethod(1, null, "", null ); // warning on each element:bad > vargsMethod(1, null); // warning: good > vargsMethod(1, str); // warning:good Also, new X().vargsMethod(1, (String) null); // warning: bad. > I think its better to treat the @NonNull as a modifier on the varargs parameter > reference as a whole and not individual elements (just like the adding the > final modifier only enforces the whole reference to be final and not the > referands). I agree that annotations being modifiers, should modify the reference and not the referents. There is no syntax annotate the array elements themselves.
(In reply to comment #2) > I think its better to treat the @NonNull as a modifier on the varargs parameter > reference as a whole and not individual elements (just like the adding the > final modifier only enforces the whole reference to be final and not the > referands). So passing null as a lone argument or a null array as an argument > should trigger the warning, but passing individual elements shouldn't, since > individual elements will be collected into an array and then passed into the > method. I did some quick research on JSR 308 and from my current understanding we should indeed interpret these declarations @NonNull Object[] objects @NonNull Object ... objects as constraining the elements, i.e., defining an array of '@NonNull Object'. Constraints on the array itself will (once we have JSR 308) be expressed before the brackets or before the dots: Object @NonNull[] objects Object @NonNull ... objects (similarly for intermediate arrays at the respective brackets). Please correct me, if my understanding of JSR 308 is wrong. We wouldn't want to do anything now, that will be incompatible with JSR 308, right? However, since adding analysis for array element access deserves a second thought, I propose a quick way out for the next I-Build: - avoid AIOOBE - flag a warning against the vararg declaration: "Compiler limitation: nullness is not analyzed for access to array elements" - keep analysis of var-args calls as implemented in my patch (should be in line with JSR 308) -> need to re-consider passing a null array (should not be flagged IMO) Next I can see if checking of array element access can be easily analyzed. Checking the array itself will then be postponed to JSR 308. What do you think?
(In reply to comment #4) > I did some quick research on JSR 308 and from my current understanding > we should indeed interpret these declarations > @NonNull Object[] objects > @NonNull Object ... objects > as constraining the elements, i.e., defining an array of '@NonNull Object'. I'm not sure if JSR308 should be the correct reference point. The JLS seems silent on the effect of annotating an array variable. However, if you say @NonNull Object[] objects also constrains the array elements, then I should also get an error in the below example with some individual elements being null? void callVarargs(@Nullable String... str) { String[] str = null; vargsMethod(1, new String{null, "", null} ); } public @NonNull Object vargsMethod(int i, @NonNull String[] objects){ objects[1] = ""; System.out.println(objects); return objects; } > - flag a warning against the vararg declaration: > "Compiler limitation: nullness is not analyzed for access to array elements" This does not seem right. We have many limitations in the compiler currently too but we don't give a corresponding warning for each. ;)
A reading of JSR308 section B3 (http://types.cs.washington.edu/jsr308/specification/java-annotation-design.html#array-syntax) says, "A potential criticism is that a type annotation at the very beginning of a declaration does not refer to the full type, even though declaration annotations (which also occur at the beginning of the declaration) do refer to the entire variable. As an example, in @NonNull String[] arr2; the variable arr2 is not non-null. This is actually a criticism of Java itself, not of the JSR 308 annotation extension" which means that the annotation on the far left of a declaration being applied to each array element is a very JSR308-specific feature and is not the case in Java currently. I don't think we should comply with JSR308 unless it actually comes out.
(In reply to comment #6) > A potential criticism is that a *type annotation* at the very beginning of > a declaration does not refer to the full type, even though *declaration > annotations* ... do refer to the entire variable Simply speaking, in @NonNull String[] str, @NonNull is currently a declarative annotation in the java world, and not yet a "type annotation"
(In reply to comment #6) > I don't think we should comply with JSR308 unless it actually comes out. If we follow that road I think we have to to prohibit any null annotations on any arrays and varargs, because otherwise users will insert annotations into their code that will be wrong (change their semantics) once JSR 308 is released. This also means, arrays and varargs need to be explicitly excluded from @NonNullByDefault. I'd still prefer to carefully support what is possible today, without breaking users' code tomorrow. (In reply to comment #5) > > - flag a warning against the vararg declaration: > > "Compiler limitation: nullness is not analyzed for access to array elements" > This does not seem right. We have many limitations in the compiler currently > too but we don't give a corresponding warning for each. ;) We can drop this warning for now, and I could make a more general proposal on how we can inform users when protection against NPE is incomplete although they did their best to specify nullness.
(In reply to comment #7) > (In reply to comment #6) > > A potential criticism is that a *type annotation* at the very beginning of > > a declaration does not refer to the full type, even though *declaration > > annotations* ... do refer to the entire variable > > Simply speaking, in @NonNull String[] str, @NonNull is currently a declarative > annotation in the java world, and not yet a "type annotation" Currently, we have a similar ambiguity regarding method return: @NonNull Object foo() { return this; } is a legal annotation in Java 5 so it must be a declarative annotation, however, once type annotations are added, the same syntax will be re-interpreted and users get what they expect. @NonNull public Object foo() { return this; } is currently equivalent (because order between modifiers/annotations is not considered), but if we want users to change this to public @NonNull Object foo() { return this; } this can be automated because (in this position) @NonNull is not useful as a declarative annotation as soon has we have that distinction. By contrast, if we interpret @NonNull String[] str as affecting the whole declaration, then this will inherently be ambiguous once we have JSR 308. If we support the interpretation that the annotation affects the whole declaration, we will never be able to distinguish this from a nullable array containing non-null elements. One consequence of the discussion might be: - check order of modifiers / annotations of a method to enforce that any null annotation comes directly before the type. This would help users to write code that is legal today AND doesn't require change due to the introduction of JSR 308.
(In reply to comment #9) > One consequence of the discussion might be: > - check order of modifiers / annotations of a method to enforce that > any null annotation comes directly before the type. > This would help users to write code that is legal today AND doesn't > require change due to the introduction of JSR 308. Nevermind this proposal, I should have continued reading the JSR 308 discussion (thanks for the link, BTW, I missed that discussion part before!) With JSR 308 the NonNull annotation will have to change its @Target from METHOD etc. to TYPE_USE, which then avoids the ambiguity in the first place, irrelevant of order. Just for arrays this doesn't help ...
(In reply to comment #10) > With JSR 308 the NonNull annotation will have to change its @Target > from METHOD etc. to TYPE_USE, which then avoids the ambiguity in the > first place, irrelevant of order. > > Just for arrays this doesn't help ... Comptability-wise, note that we will anyway have to undertake a major effort to support type annotations, during which we will make sure the new semantic for @NonNull on array variable is also obeyed.
(In reply to comment #5) > However, if you say @NonNull Object[] objects also constrains the array > elements, then I should also get an error in the below example with some > individual elements being null? > > void callVarargs(@Nullable String... str) { > String[] str = null; > vargsMethod(1, new String{null, "", null} ); > > } I'm not sure what you want to show in line 2 but a vargsMethod call vargsMethod(1, new String[]{null, "", null}); would ideally flag both nulls. Currently we can't look inside the array. We may need to wait until we can write new @NonNull String[] { null, "", null } to actually analyze such expressions. (*maybe* the expectedType from the call method can already help to find out). > public @NonNull Object vargsMethod(int i, @NonNull String[] objects){ > objects[1] = ""; > System.out.println(objects); > return objects; > } The return is unsafe in my interpretation, we don't yet know about the nullness of the array.
(In reply to comment #12) > > I'm not sure what you want to show in line 2 but a vargsMethod call > vargsMethod(1, new String[]{null, "", null}); > would ideally flag both nulls. I wanted to point a contrasting example with an array parameter as compared to a varargs parameter in reply to comment 4's claim > we should indeed interpret these declarations > @NonNull Object[] objects > @NonNull Object ... objects > as constraining the elements, i.e., defining an array of '@NonNull Object'. In my example, the param is an array of String, yet at the call site we don't flag the 'null' elements. However, the similar case when the param is a varargs of type String, we flag 'null' elements passed in an array at the call site. That is, if comment 4 is correct, I'd expect calls to both vargsMethod and vargsMethod2 to give same error: void callVarargs(@Nullable String... str) { String[] str = null; vargsMethod(1, null, "", null ); // implicitly equal to new String{null,"",null} vargsMethod2(1, new String{null, "", null} ); } public void vargsMethod(int i, @NonNull String... objects) { return objects; } public void vargsMethod2(int i, @NonNull String[] objects) { return objects; } but only the varargs call gives the error currently
This thread makes for increasingly confusing reading. To clarify matters, I would keep considerations on JSR308 aside at the moment and try to stick to Java 7. So is there still some debate purely from a java7 perspective, about whether in @NonNull String [] array; array cannot be null or array elements cannot be null or both ? My reading was that considering only Java 7 annotations and drawing analogy with how final modifier works, it is the array itself that is tagged non null and not the array elements. If there is agreement on this, then I suggest we proceed on that basis ignoring JSR 308 at the moment. To make sure this issue surfaces at the right time and gets a second look I would add a test that would simply fail for JDK level > 1.7 with a message that draws attention to this issue.
(In reply to comment #14) > This thread makes for increasingly confusing reading. To clarify matters, > I would keep considerations on JSR308 aside at the moment and try > to stick to Java 7. Sorry for that. Perhaps I was biased too much by looking at the Checker Framework (which already does it the JSR 308 way). > So is there still some debate purely from a java7 perspective, > about whether in @NonNull String [] array; > array cannot be null or array elements cannot be null or both ? I've read about a quick survey that reported a 50/50 split between people who intuitively associated the annotation with the array and those who think it would constrain the element type. From an intuition POV the ambiguity seems to be inherent. > My reading was that considering only Java 7 annotations and drawing > analogy with how final modifier works, it is the array itself that is > tagged non null and not the array elements. Meanwhile I realized that e.g. FindBugs does it the way you propose. Since a major concern in these questions is about migration paths for adopters, I agree that associating the annotation with the array makes sense for now. It will certainly ease the migration from FindBugs to our analysis. This comes at the price that we now train the users to think one way and when JSR 308 comes out everybody has to be re-trained and all their code must be refactored. > If there is agreement on this, then I suggest we proceed on that basis > ignoring JSR 308 at the moment. OK. During the discussion it come up that for adopting JSR 308 we'll have to change the @Target for the annotations from METHOD etc. to TYPE_USE. This will eventually help us drawing a line between both approaches. Based on this the mentioned refactoring should be possible. > To make sure this issue surfaces at the right time and gets a second look > I would add a test that would simply fail for JDK level > 1.7 with a > message that draws attention to this issue. Good idea, I'll do that. I'll prepare a new patch by tomorrow, which will apply the Java 7 POV.
Created attachment 208473 [details] test & fix with changed strategy. This patch checks nullness of varargs at the array-level, not its elements.
(In reply to comment #16) > Created attachment 208473 [details] > test & fix with changed strategy. This patch is good. One suggestion though: Instead of having an independent failing test for 'compliance'>1.7, it is better to have the added regression tests itself fail for 'source'>1.7 i.e. if (source>1.7) { runNegativeTest(...); } else { fail(..); } PS: 'source' should be used instead of 'compliance'
(In reply to comment #17) > (In reply to comment #16) > > Created attachment 208473 [details] [details] > > test & fix with changed strategy. > This patch is good. One suggestion though: > Instead of having an independent failing test for 'compliance'>1.7, it is > better to have the added regression tests itself fail for 'source'>1.7 i.e. > > if (source>1.7) { > runNegativeTest(...); > } else { > fail(..); > } Just to forestall potential confusion, the decision pathways look inverted in the above. Assuming JSR308 becomes official for Java8, any new syntax that allows finer grained annotations should be accepted only for source level 8, but purely for the case at hand, compliance testing should be good enough, though it is purer to check for source level.
(In reply to comment #18) > Just to forestall potential confusion, the decision pathways look > inverted in the above. Oops! :)
(In reply to comment #18) [...] > Assuming JSR308 becomes official for Java8, any new syntax that allows > finer grained annotations should be accepted only for source level 8, but > purely for the case at hand, compliance testing should be good enough, > though it is purer to check for source level. Sauce for the goose is sauce for the gander ? Looking at the infrastructure (org.eclipse.jdt.core.tests.util.AbstractCompilerTest.buildAllCompliancesTestSuite(TestSuite, Class)), we may have to go with just testing compliance levels, since this is what we have always done in the test suite universe. So it looks like we can go with the patch as it is.
I've integrated the JSR 308-reminder with 'real' tests as suggested in comment 17 (w/ corrected logic :)). With this change released for 3.8 M5 via http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=fda975d98b069f678ae89a663957463bb7bd4088
*** Bug 366331 has been marked as a duplicate of this bug. ***
Verified for 3.8 M5 using build id: I20120122-2000
