Community
Participate
Working Groups
Once bug 186342 is fixed we might want to think of supporting those annotations also for fields. Without bending the semantics of Java this would imply that for any @NonNull field: - any exit from a constructor must establish that the field is definitely non-null - any assignment in a method must assign a definitely non-null value. More sophisticated and more flexible models may exist, e.g.: - allowing fields to be still null after the constructor but designating init methods to do the job, forcing clients to call init immediately after the constructor - allowing intermediate states with nulls given the invariant is restored at each exit of a method. Here costs and benefits must be weighed.
From some early statistics documented in bug 186342 comment 89 my gut feeling tells me we should also address fields pretty soon, in order to reduce the number of warnings produced. After some recent reading I see two main alternative proposals: (A) rawness as supported by the Checker Framework (Mike Ernst) (B) monotonic non-null as proposed by Chalin, James, Rioux Both approaches seem well capable of reducing the number of warnings. Also, both soundly address the issues of initialization and concurrency. (A) introduces new annotations for capturing references to not-fully initialized objects. This approach may require more specification up-front (methods called from constructors need to be marked @Raw, the same for any this-leaks from constructors), but then gives stronger guarantees for the usage of a non-null field. Following (B) only one annotation at the field (@EventuallyNonNull) is required but then guarantees for that field are weaker (field read may *not* assume non-null, but after one successful check for non-null the field is known to remain non-null, i.e., fields behave similar to locals with unknown initial nullness). I would like not to select one and reject the other because both approaches seem to reflect different styles of programming: (A) would probably work best for green-field development, where a strict discipline can be enforced in order to achieve strong guarantees. By contrast, (B) seems superior for capturing lazy initialization of fields, i.e., reasoning about fields that are not (definitely) initialized by the constructor. I'd like to hear comments on the following points: - Are there significantly other proposals for specifying nullness of fields? - Is it correct that neither approach subsumes the other? - Are there any reasons why both approaches might be incompatible, i.e., anything preventing a combination of both? Indeed, given that both approaches have different requirements and implications, and given that we are not a standardization body, I'm leaning to eventually supporting both as separate options, if possible. Technically (B) seems closer to the analysis we already have in the compiler, while (A) would open a new category of analysis. So, implementationwise (B) probably requires less efforts.
I just pushed a new feature branch: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/log/?h=sherrmann/NullAnnotationsForFields This shows my current implementation for this bug on top of bug 247564. In order to analyse annotated fields in all positions, more syntactic constructs (most subtypes of Reference) had to be included in the analysis. While I was at it I included also those constructs that don't refer to fields but have a constant nonnull property (like, e.g., 'this'). You may wonder, how often you'd find "if (this != null) ...", but ... if you set the following IProblems to error: RedundantNullCheckOnNonNullExpression NonNullExpressionComparisonYieldsFalse, (e.g., by deleting these from ProblemReporter#getIrritant()), just run all JDT/Core tests and be surprised about the 'nonsense' we have in our tests :) Field *initialization* is the main conceptual problem in this area. My current implementation only does the minimal checking: see if all fields are definitely assigned at the end of each constructor (using the same logic as it is used for final fields). Since assigning a pot.null or even unknown value to such a field is illegal anyway, def.assigned implies def.nonnull. Doing similar checks for static fields is a TODO. However, I made no attempts whatsoever to protect uninitialized @NonNull fields from being dereferenced. * If we want to protect an unassigned @NonNull variable against access from another method (indirectly) called from the constructor, we definitely need at least one more annotation, so I'm deferring this as of now. * OTOH, within the constructor it should be easily possible (though not yet done in the implementation) to detect when a @NonNull field is accessed before being definitely assigned. Implementation notes: The most tricky part is continuing the work from bug 247564 wrt resetNullInfoForFields etc. Some details: - new field ClassScope.fieldResetNullInfo: Here I collect the constantFieldsMask plus the default null status for these kinds of fields: - regular fields: default is 1001 (unknown) - @Nullable fields: default is 0100 (pot.null) other fields (constants and @NonNull) are not reset - both pieces of information (constantFieldsMask and default status) are collected into fieldResetNullInfo during FieldDeclaration.analyseCode() - TypeDeclaration only coordinates handling of fieldResetNullInfo - whenever UFI#resetNullInfoForFields is called the fieldResetNullInfo is passed in so that we can set status of all fields to the desired default by only performing bit operations, merging info from this and fieldResetNullInfo. Def.assignment check: previously only blankFinal() fields would record their assignment status -> extend this to @NonNull, too. BinaryTypeBinding now restores null annotations for fields, too. Some oddities: method NameReference.fieldBinding() turned out to be useless in one situation, because (to my surprise) for a QNR it returns the *first* field binding of a chain. Existing clients look safe as expecting only one-field references. -> Hence the new method lastFieldBinding(). Next, QNR#checkNPE was confusingly overloaded. -> I renamed one version to document that here we are checking whether the QNR *itself* can cause NPE, whereas the other checkNPE() checks if the expression can be null and hence cause NPE when being dereferenced by its parent AST node. I'm not even sure if protecting checkInternalNPE() with if (needValue) isn't a (old) bug. -> TODO Impl still contains some TODOs/FIXMEs, but their number is rapidly decreasing :)
I started the review by playing around with the branch a bit. Haven't dived into the code yet but a few very early questions: (0) Is the discussion in comment 1 still relevant, given that we neither use @Raw nor @EventuallyNonNull? (1) According to JavaCore, the effect of the COMPILER_PB_NULL_SPECIFICATION_VIOLATION is extended to fields too, but this has not been done for other null annotation specific options. Is this just a missing piece of doc, or is also missing in the implementation? (Ok I tested this a bit after writing the comment and it seems to work. So maybe its just the doc) (3) I see a few tests with changed error messages in NullAnnotationTest. Did anything change in the existing implementation for local variables too? (4) The "..may not have been initialized" warnings are mandated by the JLS and there are set rules on where they should be emitted. Should we re-word the "The @NonNull field .. may not have been initialized" warning to differentiate it from the compiler errors? viz. "The @NonNull field .. may not have been initialized to a definite non null value" (5) I switched off the "include fields in null analysis" option and yet I could see annotation-related warnings on fields. Shouldn't the "include fields" switch control this? (6) For the following example public class X { @Nullable Object o = new Object(); public void main(String[] args, @Nullable X x1) { if (o != null) { o.toString(); // no warning } if (x1.o != null) { x1.o.toString(); // pot. NPE } } } The story looks a bit inconsistent. I know why this happens - null analysis for this.o is done using the flowinfo while for x1.o only the annotation is read. But this difference may be hard to explain. (7) This gives a weaker warning: public class X { static final Object a = null; @NonNull Object o = a; // unknown nullness? } Shouldn't it be stronger since we know the definite nullness of a?
(8) Are the two new messages /** @since 3.8 */ int NonNullExpressionComparisonYieldsFalse = Internal + 685; /** @since 3.8 */ int RedundantNullCheckOnNonNullExpression = Internal + 686; related to the fix for this bug? May be we can spawn off a new bug for these?
(In reply to comment #3) > I started the review by playing around with the branch a bit. Great, thanks! > Haven't dived into the code yet but a few very early questions: > (0) Is the discussion in comment 1 still relevant, given that we neither use > @Raw nor @EventuallyNonNull? I'd say that discussion is deferred, i.e., won't be covered by this bug. > (1) According to JavaCore, the effect of the > COMPILER_PB_NULL_SPECIFICATION_VIOLATION is extended to fields too, but this > has not been done for other null annotation specific options. Is this just a > missing piece of doc, or is also missing in the implementation? (Ok I tested > this a bit after writing the comment and it seems to work. So maybe its just > the doc) IIRC one check is still missing in the impl: contradictory annotations on fields. Other than that, yes, it's a doc bug. > (2) Oops, s.o. stole item (2) :) > (3) I see a few tests with changed error messages in NullAnnotationTest. Did > anything change in the existing implementation for local variables too? * Regarding test_nonnull_return_008(): The method ProblemReporter#messageSendRedundantCheckOnNonNull() has been integrated into a much smarter method expressionNonNullComparison() which distinguishes a few more IProblems. In particular the NonNullMessageSendComparisonYieldsFalse variety wasn't present previously. This slightly changes the wording and also causes the subsequent dead code message. * the other one is _test_nonnull_return_009b() Here I only adjusted the result to the reality of the implementation and then enabled this previously disabled test. I felt that actually one warning is sufficient and only documented what would need to happen to create a warning for the other side, too. > (4) The "..may not have been initialized" warnings are mandated by the JLS and > there are set rules on where they should be emitted. Should we re-word the "The > @NonNull field .. may not have been initialized" warning to differentiate it > from the compiler errors? viz. "The @NonNull field .. may not have been > initialized to a definite non null value" The similarity is intentional, and I figured the change should be sufficient: The blank final field {0} may not have been initialized The local variable {0} may not have been initialized The @NonNull field {1} may not have been initialized All three messages mention why we expect the variable to be initialized. Clearly, the third reason does not refer to the JLS. You think the difference is not enough? OK, I don't want to start a legal case :) > (5) I switched off the "include fields in null analysis" option and yet I could > see annotation-related warnings on fields. Shouldn't the "include fields" > switch control this? I think I was suggesting to let "enable annotation based null analysis" imply "include fields". I don't think it makes sense to enable the former and disable the latter. At a closer look we may eventually want a switch for restricting the analysis of fields to the strict part (i.e., worst case assumption regarding concurrency) to get the most out of field annotations. > (6) For the following example > public class X { > @Nullable Object o = new Object(); > public void main(String[] args, @Nullable X x1) { > if (o != null) { > o.toString(); // no warning > } > if (x1.o != null) { > x1.o.toString(); // pot. NPE > } > } > } > The story looks a bit inconsistent. I know why this happens - null analysis for > this.o is done using the flowinfo while for x1.o only the annotation is read. > But this difference may be hard to explain. The latter part of the example is inevitable. It's the former part that is debatable. I'm avoiding the message here as a courtesy for the user (using bug 247564 for sure). Eventually, in the optional strict mode mentioned above we should also give a (mild) warning here. OTOH, it's that latter warning that explicitly mentions the reason for reporting, right? So we don't leave the user completely uninformed :) > (7) This gives a weaker warning: > public class X { > static final Object a = null; > @NonNull Object o = a; // unknown nullness? > } > > Shouldn't it be stronger since we know the definite nullness of a? Oops, a bug! (In reply to comment #4) > (8) Are the two new messages > /** @since 3.8 */ > int NonNullExpressionComparisonYieldsFalse = Internal + 685; > /** @since 3.8 */ > int RedundantNullCheckOnNonNullExpression = Internal + 686; > related to the fix for this bug? May be we can spawn off a new bug for these? Conceptually, they qualify for a separate bug for sure. Technically they're closely related and I wouldn't want to create separate patches like I motivated in this comment: from comment #2: > In order to analyse annotated fields in all positions, more syntactic > constructs (most subtypes of Reference) had to be included in the analysis. > While I was at it I included also those constructs that don't refer to fields > but have a constant nonnull property (like, e.g., 'this').
I merged the feature branch with the latest from master and added: - test & fix for item (7) - application of nullness defaults to fields - detection of redundant/contradictory annotations on fields Current head in the branch is http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=sherrmann/NullAnnotationsForFields&id=874e2c41609c64af417834df9af2e898d8dcf37a AFAICS this makes it feature complete and resolves the FIXMEs I still had in the code. JavaDoc still needs updating as mentioned above.
(In reply to comment #5) > All three messages mention why we expect the variable to be initialized. > Clearly, the third reason does not refer to the JLS. You think the difference > is not enough? > > OK, I don't want to start a legal case :) Yeah, while I don't say the wording is wrong, it just surprised me when I started playing around and got this error. Being used to the "..may not have been initialized" warning, I was confused on seeing one for a non-constant field. Srikanth, any comments on this? > I think I was suggesting to let "enable annotation based null analysis" imply > "include fields". I don't think it makes sense to enable the former and disable > the latter. Oh. But in that case the UI will have to auto-check "include fields.." option when a user checks "enable annotation..." and then it;ll have to be greyed out too? The only sense in decoupling would be to give the user some flexibility. I know it wont be a usual case when a user wants to enable annotations and not have them diagnose fields, but it may happen. > > The story looks a bit inconsistent. > The latter part of the example is inevitable. It's the former part that is > debatable. I'm avoiding the message here as a courtesy for the user (using bug > 247564 for sure). Eventually, in the optional strict mode mentioned above we > should also give a (mild) warning here. > > OTOH, it's that latter warning that explicitly mentions the reason for > reporting, right? So we don't leave the user completely uninformed :) Umm, I couldn't quite understand this point. The latter warning is good, that's beyond doubt. But in the former case can we do better than plain null analysis now that we have the annotation, and raise a suitable warning? (Assuming that there's only one mode for 3.8)
Is there a case when lines 5182-5186 in org.eclipse.jdt.internal.compiler.problem.ProblemReporter.expressionNonNullComparison(Expression, boolean) get triggered? Also, whats the needImplementation method for. Is it to handle cases such as if (null == null), if ( (o = b ? new Object(): new Object())== null)
(8) A scenario which gives contradictory warnings public class XXX { static final @Nullable Object o = null; static void goo() { if (XXX.o == null) { // oops System.out.println(XXX.o); } } }
(In reply to comment #9) > (8) A scenario which gives contradictory warnings > public class XXX { > static final @Nullable Object o = null; > static void goo() { > if (XXX.o == null) { // oops > System.out.println(XXX.o); > } > } > } Not contradictory. I mean an incorrect "null dereference" warning. This happens because of a wrong assumption in org.eclipse.jdt.internal.compiler.ast.QualifiedNameReference.checkInternalNPE(BlockScope, FlowContext, FlowInfo, boolean). The current binding being a field binding does not mean it is actually dereferenced.
(In reply to comment #10) > (In reply to comment #9) > > (8) A scenario which gives contradictory warnings Fixed this and a related source range problem with commit 0b82fa47eb78f9f59638ad9671b3e2a9d9c28ff6
(In reply to comment #2) > Next, QNR#checkNPE was confusingly overloaded. -> I renamed one version to I don't understand this completely. checkNPE was being called from various places earlier. Now that part of the behavior is captured in checkInternalNPE(..), yet the other places continue to call checkNPE which now is a completely new method. Is this intended?
(In reply to comment #6) [...] > AFAICS this makes it feature complete and resolves the FIXMEs I still had in > the code. I worry that the light at the end of the tunnel we spot could actually be the headlamp of a fast approaching train :-( https://bugs.eclipse.org/bugs/show_bug.cgi?id=369487#c18 offers the current feature as the remedy, but at least at a first glance things appear to be getting murkier and messier with this fix. Admittedly, I haven't studied the implementation fully and starting this thread after some minimal experimentation and could be wrong (hope to be :)) - Apologies in advance if I have misunderstood some thing. Borrowing Ayush's example from comment#3 (reproduced below) // ----------------- public class X { @Nullable Object o = new Object(); public void main(String[] args, @Nullable X x1) { if (o != null) { o.toString(); // no warning } if (x1.o != null) { x1.o.toString(); // pot. NPE } } } // ------------------- Obviously this triggers https://bugs.eclipse.org/bugs/show_bug.cgi?id=370185 and results in the inconsistency Ayush has called out. More direly, this warning should not be dismissed as "inevitable" because it appears that there is NOTHING that a user could do to act on this warning. -- A null check guard in the form of "if (x1.o != null)" (already present) would not eliminate the warning. -- An assignment of the form "x1.o = new Object();" just ahead of the x1.o.toString(); wouldn't eliminate the warning. -- An assignment of the form x1.o = someNonNullAnnotatedObject; ahead of the x1.o.toString(); wouldn't eliminate the warning. In fact there is no code change that a programmer can do to address this situation to eliminate this warning. These warnings are essentially non-actionable. To understand how serious this situation is, let us agree on some terminology. (1) Let us call data accesses and method invocations of the form "this.field." (with the terminal '.' included) as "self field dereferences" and (2) Data accesses and method invocations of of the form "that.field." (with the terminal '.' included) where `that` is of the same type as `this` as "alien field dereferences" and finally (3) Data accesses and method invocations of of the form "other.field." (with the terminal '.' included) where `other` is NOT of the same type as `this` as "alien ***type*** field dereferences" then for EVERY @Nullable annotated field f, EVERY alien field dereference and EVERY alien type field dereference would produce a potential NPE warning that is not addressable by a corresponding code change. Where does that leave the user ? Presented with such a situation, I fear that users turn off all potential null warnings (which would be very bad because most of the fields related null warnings are potential warnings at the moment, the stronger warnings we issue under limited situations may also eventually have to be downgraded to potential warnings (see bug# 370787)). Or they would simply stop using this feature. Chatting with a bunch of JDT coders in the hallway, this viewpoint was heard that alien type object dereferences should really not be an issue because fields should really be private and should not be accessed outside the class. This is the same point already heard in https://bugs.eclipse.org/bugs/show_bug.cgi?id=369487#c21 and as pointed out in that bug this standpoint ignores (a) currency/prevalence of the practice (b) Collaborating classes within the same package allowing default access to each other's data. Even if we were to accord *some* validity to that viewpoint, the fact that we would emit a warning for every alien field dereference makes it unacceptable IMHO. One further crucial difference: https://bugs.eclipse.org/bugs/show_bug.cgi?id=369487#c21 was about a warning that doesn't show up. The current discussion is about a warning that shows up and won't go away. There appears to be an ordering issue here: it is not clear to me that until and unless we have a coherent story for https://bugs.eclipse.org/bugs/show_bug.cgi?id=370185, we can solve the current situation satisfactorily. That bug has a set of complexities around aliases and concurrency, but we should be able to spell out a set of stipulations that "reasonable programmers would hold as being reasonable" and solve it within that framework and then consider this one. In short, IMHO the fix for bug 247564 was incomplete, self limiting, but was internally consistent and not incorrect. OTOH, the current story looks incorrect and incoherent to me. It has been heard before that https://bugs.eclipse.org/bugs/show_bug.cgi?id=370185 is not solvable in the current release schedule. If that is so, we should hold on the current work, till 3.9, notwithstanding the investment (emotion and/or industry-wise) Let me know if I have misunderstood something here. You may want to check what other frameworks do presented with this example.
Srikanth, Before I start answering you long post with an equally long answer, let me quickly check the premise: (In reply to comment #13) > In fact there is no code change that a programmer can do to address > this situation to eliminate this warning. These warnings are > essentially non-actionable. Quoting from bug 247564 comment 122: > Let me first try and correct all my typos from goo(): > @Nullable Object g; > void goo() { > g.toString(); // (d) warning > if (g != null) g.toString(); // (e) weaker warning > Object l; > if ((l=this.g) != null) l.toString(); // (f) OK > } > better? > > Now, (e) is very well actionable: transform it into (f)! > Still we want to distinguish warning (e) from other warnings, because for > single thread apps this *is* a solution, so those folks should be able > to ignore just (e), while others should go all the way to (f). Has this been included in your premise that the warning is non-actionable?
(In reply to comment #14) > Srikanth, > > Before I start answering you long post with an equally long answer, > let me quickly check the premise: > > (In reply to comment #13) > > In fact there is no code change that a programmer can do to address > > this situation to eliminate this warning. These warnings are > > essentially non-actionable. > > Quoting from bug 247564 comment 122: You are always ready with chapter and verse citation, aren't you ? :) My memory is no match unfortunately. I did have a sense of déjà vu, but nothing more. > > if (g != null) g.toString(); // (e) weaker warning > > Object l; > > if ((l=this.g) != null) l.toString(); // (f) OK > Has this been included in your premise that the warning is non-actionable? Obviously it hadn't been, otherwise I wouldn't have said there is "no code change". That being said, this proposed code transformation would not be acceptable to many users I fear, and all concerns raised in previous comment still bother me. I do not see programmer's changing their code from: if (g != null) g.toString(); to Object l; if ((l=this.g) != null) l.toString(); :-(
(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #9) > > > (8) A scenario which gives contradictory warnings > Fixed this and a related source range problem with commit > 0b82fa47eb78f9f59638ad9671b3e2a9d9c28ff6 The main fix is good, thanks. However, I don't see the source range problem. To the contrary, with your change the following gives a strange result return other.other.o.toString() ---------- 1. ERROR in X.java (at line 6) return other.other.o.toString(); ^^^^^ Potential null pointer access: The field other is declared as @Nullable ---------- 2. ERROR in X.java (at line 6) return other.other.o.toString(); ^^^^^ Potential null pointer access: The field other is declared as @Nullable ---------- Without your change this was: ---------- 1. ERROR in X.java (at line 6) return other.other.o.toString(); ^^^^^ Potential null pointer access: The field other is declared as @Nullable ---------- 2. ERROR in X.java (at line 6) return other.other.o.toString(); ^^^^^ Potential null pointer access: The field other is declared as @Nullable ---------- Do you mind if I revert the source range part?
(In reply to comment #8) > Is there a case when lines 5182-5186 in > org.eclipse.jdt.internal.compiler.problem.ProblemReporter.expressionNonNullComparison(Expression, boolean) > get triggered? Hoping we are on the same line I see a breakpoint triggered by, e.g.,: testBug247564b() regarding "if ((o == null))", where 'o' is a static final field that has already been dereferenced in the same method. More generally: all situations where we have definite null status for a field that has no @NonNull annotation. Do you see anything fishy here? > Also, whats the needImplementation method for. I have three calls to needImplementation(). Two of them are just defensive programming: (a) a null literal was reported with null status NonNull! (b) an unexpected expression type slipped into this method Regarding the other one you just caught me on a TODO item that had escaped my attention. > Is it to handle cases such as [..] > if ( (o = b ? new Object(): new Object())== null) Yes, that kind of thing. A slightly more realistic testcase and the missing implementation are in commit d06dca3193608aaecf6a0ecc6e783a566d9a941e
(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #9) > > > (8) A scenario which gives contradictory warnings > Fixed this and a related source range problem with commit > 0b82fa47eb78f9f59638ad9671b3e2a9d9c28ff6 While the main part of this fix is great, I reverted the source range part and added a test demonstrating why we should indeed respect the index into otherBinding for raising the problem at the correct location. This revert has been isolated into 1e74708f239861b45ea9c03fb332142e9b5b54a5 in case we need to re-revert :)
(In reply to comment #12) > (In reply to comment #2) > > Next, QNR#checkNPE was confusingly overloaded. -> I renamed one version to > I don't understand this completely. checkNPE was being called from various > places earlier. I tell you, beware of dangerous overloading! :) You probably mean this (introduced in Expression): checkNPE(BlockScope,FlowContext,FlowInfo) while I'm talking about this one (in QNR): checkNPE(BlockScope,FlowContext,FlowInfo,boolean) (Should we add a compiler warning for overloading? :} ) In master the latter is called from exactly these two locations: - QualifiedNameReference#analyseAssignment() - QualifiedNameReference#analyseCode() > Now that part of the behavior is captured in > checkInternalNPE(..), yet the other places continue to call checkNPE which now > is a completely new method. Is this intended? No change in behavior, pure renaming to avoid exactly this kind of confusion from now on :)
(In reply to comment #15) > > > if (g != null) g.toString(); // (e) weaker warning > > > Object l; > > > if ((l=this.g) != null) l.toString(); // (f) OK > > > Has this been included in your premise that the warning is non-actionable? > > Obviously it hadn't been, otherwise I wouldn't have said there is > "no code change". > That being said, this proposed code transformation would not be > acceptable to many users I fear, and all concerns raised in previous > comment still bother me. A clarification: I am not opposed to extraction of field reference expression into local variable per se. So I amend my earlier comment that these warnings are not actionable. What I am saying users will reject is having to transform perfectly valid code of the form: if (x1.o != null) { x1.o.toString(); // WRONG pot. NPE } to Object l; if ((l=x1.o) != null) l.toString(); just to eliminate a compiler warning their claim being their original code is perfect as it is and it is the compiler that is broken. So eliminating this distraction around actionable/non-actionable warnings for the moment, I think in the following program, users will declare the compiler broken in all 5 messages issued: //---------------- import org.eclipse.jdt.annotation.NonNull; import org.eclipse.jdt.annotation.Nullable; class X { @Nullable Object o; public @NonNull Object foo(X x) { // Puzzling message: Potential type mismatch: required '@NonNull Object' but nullness of the provided value is unknown return x.o != null ? x.o : new Object(); } public void goo(X x) { if (x.o != null) { // Puzzling message : Potential null pointer access: The field o is declared as @Nullable x.o.toString(); } } public void boo(X x) { if (x.o instanceof String) { // Puzzling message: Potential null pointer access: The field o is declared as @Nullable x.o.toString(); } } public void zoo(X x) { x.o = new Object(); // Puzzling message: Potential null pointer access: The field o is declared as @Nullable System.out.println("hashCode of new Object = " + x.o.hashCode()); } public void doo(X x) { x.o = foo(x); // foo is guaranteed to return @NonNull Object. // Puzzling message: Potential null pointer access: The field o is declared as @Nullable System.out.println("hashCode of new Object = " + x.o.hashCode()); } }
(In reply to comment #20) I entirely agree. All those warnings are bad and make the compiler look dumb. If we can't be smarter, then no warnings must be issued for those cases.
Before we decide what exactly the compiler should answer to a given piece of code, I suggest we first think about the exact question we are asking the compiler. IMHO, regarding null-safety for fields ways of looking at things differ too much. One-size-fits-all will at best give mediocre answers for everybody. If we allow users to ask different questions then they will get excellent answers. Initially I thought we might add tunability later, but I now think that would be a mistake. For details I started a little write-up at http://wiki.eclipse.org/JDT_Core/Null_Analysis/Options - as always: comments appreciated. Short version: for a sound analysis that respects concurrency (and side effects and aliasing) we must emit very strict warnings, that may look unnecessary at first (I just confirmed this with some academic contacts of mine). For projects with less bold expectations (expected to be the majority) pragmatic solutions must be found that soften (and reduce) the complaints raised by the compiler, but I'd hate a lock in to this kind of compromise. In the current implementation I first wanted to make sure that the analysis is precise enough to support all use cases. From here I don't see technical problems to offer also less pedantic modes (the opposite would be much more difficult). The main open questions I see rank around how we communicate to users, what different levels of strictness mean. In this regard the wiki page only contains initial brainstorming. I will make a more specific proposal once I find the time.
(In reply to comment #22) The resolution of https://bugs.eclipse.org/bugs/show_bug.cgi?id=186342 has brought some very powerful and welcome analysis capabilities to JDT. It is exciting to consider where/how this could be taken further along. However we seem to have hit a serious roadblock here. (To paraphrase Markus Keller's assertion from bug# 186342 comment# 68:) Eclipse/JDT (in its official avatar) is not an experimentation field or a research sandbox in the lines of say, the SUIF compiler suite. Work of exploratory nature ought to be carried out in a branch and once the open questions are understood and problems are resolved satisfactorily, we should evaluate the solution's readiness and suitability for an industrial/production compiler that JDT is. At the moment, the implementation as it stands triggers this circuit breaker :-( A few observations: (1) While debating the design issues in bug 369487, I somehow did not get the sense that you were philosophically opposed to foreign fields analysis or to integrating flow analysis outcome with field annotations, but the wiki write up seems to convey that impression. If so, the analysis out of bug 247564 will not dovetail into the current work and inconsistencies will persist (unless users are willing to significantly change their coding style and current code base to conform to a set of guidelines) (2) While your point about one-size-fits-all approach is valid, the current implementation cannot be absolved of that very charge ? It seems to careen towards the pedantic end. I grant you that, this is not the envisioned end state and you are considering addressing the needs of more pragmatic use case scenarios. But only after the contours of such a solution become available for inspection, can we then assess release readiness. > I will make a more specific proposal once I find the > time. Thanks for that offer. This is indeed a complex problem with many open questions most certainly worthy of tackling.
Although I'm not yet skilled enough to contribute directly to this feature's implementation, I see enormous potential in null contract analysis and want to express my support. Even if we must fork a strategic retreat to a new branch, it would be worthwhile towards the attainment of stricter pre- and post-condition enforcement than the language itself provides. At the risk of betraying my naïveté - is it not possible (with eclipse being rooted in OSGI) to contribute this and other supplemental analyses as separate plugins, obviating the need to integrate directly into the JDT core? If not already possible, it would be neat to be able to derive new compilers at run-time by attaching decorative analysis plugins.
Dear Reuben, Many thanks for your offer! I do see a field where help is welcome. Actually this doesn't necessarily include coding. In my private own view we have three essential ingredients already in place: - An understanding of an optimistic model, where fields are analysed just like locals, which will give only expected warnings, but will miss to report many errors that are caused by aliasing, side effects and concurrency. - An understanding of a pessimistic model, that reflects that every dereference of a @Nullable field is in fact unsafe under certain conditions, but this model produces more warnings than most people would like to see. - An implementation that can be parameterized to conform to either the optimistic, or the pessimistic model, and probably also to a good number of compromises between both extremes. The thing that most urgently needs being worked out, IMHO, is a definition, *where* between the two extremes people would like to see the analysis. Technically, most of this can be explained using this example: Given a nullable field: @Nullable Object o; How would you like the compiler to respond in the following cases: (a): o.toString(); (b): if (o != null) o.toString(); (c): double v = 0.3, w = 1.2; if (o != null) { for (int i = 0; i < Integer.MAX_VALUE; i++) v = v/w+w/v; o.toString(); } System.out.println(v); (d): if (o != null) { otherObject.foo(); o.toString(); } (e): Object l = o; if (l != null) l.toString(); (f): if (o != null) { otherObject.o = null; o.toString(); } The extreme positions are: - only (a) is problematic, compiler should warn (only) here - only (e) is safe, compiler should report all others We could now give technical explanations describing exactly what risks remain if certain examples other than (e) are silently accepted. Also this level is clear. But perhaps we can give more help to users if we define 2 or 3 "profiles" / "safety levels" that can be explained with few words. The difficulty lies in aligning a useful level of warnings with the users' expectations. You might also have a look at http://wiki.eclipse.org/JDT_Core/Null_Analysis/Options for some background on the individual risks involved in these examples (you may skip the section on incompleteness, though). The question is: which warnings would you be interested in? And how would you describe your safety requirements? Thanks.
(In reply to comment #24) > Although I'm not yet skilled enough to contribute directly to this feature's > implementation, I see enormous potential in null contract analysis and want to > express my support. Even if we must fork a strategic retreat to a new branch, > it would be worthwhile towards the attainment of stricter pre- and > post-condition enforcement than the language itself provides. Thanks for the feedback. Ideally, we would like to and hoping to be able to deliver this right in the standard Eclipse SDK, so the analysis becomes available right out of the box. We re figuring out what course corrections are required to achieve that.
On a different road there might be an easy way out of the currently perceived dilemma: when discussing this with Markus at EclipseCon Europe, we decided to ignore all proposals of specific annotations for fields, because the need wasn't clear. Judging from today's perspective, fields do pose more of a problem than what we touched in that discussion. OTOH, all research in this field seems to converge in one observation: a large number of problematic fields can be efficiently tamed by specifying monotonic semantics: such a field may start with a null value but once assigned nonnull it can never be reset to null again. Analysis of this kind of fields is actually easy. From a user perspective: one null check per field and enclosing method suffices: void foo() { if (this.o != null) { // arbitrary code here this.o.toString(); } } With such monotonic semantics this code is 100% safe (in a program fully checked against its specification). Some annotation names used by different research groups are: @MonoNonNull @EventuallyNonNull @LazyNonNull In one study covering 700 MLOC of code it was found that this annotation would solve the problem of about half of the nullable fields (given that nonnull fields pose no problem anyway). While also other models have been discussed in the literature, there's a broad consensus on the semantics and also usefulness of this model of monotonicity. I'm open to pursuing either road (or both): one that tries to match user expectations by one or more suitable compromises between the two extremes mentioned previously - or - one that leverages one more annotation for making fields amenable to flow analysis with neither flooding the user with uninteresting warnings nor sweeping potential bugs under the rug.
(In reply to comment #25) > clear. But perhaps we can give more help to users if we define 2 or 3 > "profiles" / "safety levels" that can be explained with few words. The notion that there is a continuum of user personas with differing needs appears sound and so we need to think of configurability/tuning options to accommodate those disparate needs. The set of examples cases you have constructed make for a great starting point in understanding this. Support for well crafted profiles would ground this effort solidly in practical applicability and ameliorate the concerns I had articulated earlier (some times in dramatic/melodramatic terms if only, to convey a sense of urgency). Thanks.
Since the JDT team members are not conveniently co-located, we held "virtual design meetings" to discuss and debate the various issues involved in developing this capability. At the moment, it is clear that a solution that is general enough to accommodate the various view points is not feasible in the 3.8 time frame and eliminating delivery pressures would be a good step to the eventual solution of this problem. I'll summarize the key points discussed internally and post it here shortly.
We discussed the design issues in detail and concluded that we need to support both an "optimistic model" and a "pessimistic model" with these characteristics: Optimistic model: This model operates under the following stipulations: (a) The analysis is unaware of any modifications that happen via aliases. (b) The analysis is unaware of any modifications that happen in other concurrently executing threads. (c) Since the flow analysis is intra-procedural in nature, the compiler is unaware of what modifications could be made in a method call. So the compiler adopts a conservative approach and this may result in potential warnings that a human could readily ascertain to be false positives via code inspection. (d) Absence of a warning does not imply absence of NPE. A pessimistic model: (a) nullable fields could be completely excluded from any flow analysis. (b) Every dereference of such a nullable field will be flagged as dangerous (c) For all examples in comment 25 except (e) (transfer to a local variable before access) a context can be constructed that actually triggers NPE on the field, even after the null check (d) this model will raise many warnings that many people will perceive as unjustified. (e) BUT this model is strong enough to give positive guarantees about absence of NPE. We are exploring what it would take to support both models so users can pick & choose the level of safety they desire in their code base. Both models could turn out to be too extreme in practice, so we may have to introduce more annotations like the one from comment 27.
Removing dependency on bug 247564 because I'm currently cooking a patch that handles null-annotated fields without any flow analysis for fields.
I have pushed a new topic branch sherrmann/NullAnnotationsForFields2 [1] for take 2 regarding this RFE. The topic branch currently has two commits: 1: a slice from the former sherrmann/NullAnnotationsForFields without any flow analysis for fields (which means the entire infra structure for squeezing field IDs into the null analysis is not needed). This slice purely implements the pessimistic model. 2: a fix for Bug 382789 (which previously was a "hidden" part of the patch. Plus some small improvements of the patch here and there. I'm happy to report that this patch is much less complex than the previous combined approach. All JDT/Core tests are green. This is basically what I'm planning to put into the experimental patch feature for public consumption. I might want to add to this (time permitting): + Special case detection of "if (f != null) f.op();", i.e., null-check *immediately* followed be a dereference. This is the easiest compromise I can think of for avoiding almost-irrelevant warnings regarding fields that are not @NonNull. This detection would be off by default but exposed as a preference. (I already made an experiment in this direction, just have to find the patch :) ). + Maybe I'll even add a quick fix for extracting to a new local variable plus a null check. Not sure to what extent I could re-use the "extract to local" refactoring here. Any hints appreciated. A few questions when moving towards deployment: - Am I required to add disclaimers to the source code or is it sufficient to mark the download accordingly? - Since we don't have a central build ready for this branch, is it OK to simply export from the IDE? - When preparing a patch feature for deployment, should this project be pushed to git, too? Anything to observe in order not to confuse the SDK build etc? - Who should I talk to regarding the final upload? [1] http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/log/?h=sherrmann/NullAnnotationsForFields2
(In reply to comment #32) > + Maybe I'll even add a quick fix for extracting to a new local variable > plus a null check. I've attached a patch with a new quickfix in bug 337977 comment 18. I guess that's about it for now.
(In reply to comment #32) > A few questions when moving towards deployment: > > - Am I required to add disclaimers to the source code or is it sufficient > to mark the download accordingly? Marking it on the download page should be enough, though we are not speaking about the "official" download page here. > - Since we don't have a central build ready for this branch, is it OK > to simply export from the IDE? You would have to create a feature patch. > - When preparing a patch feature for deployment, should this project be > pushed to git, too? No, I would not add the feature patch to the repo a this point. > Anything to observe in order not to confuse the SDK build etc? As along as all your work stays in your branch there's nothing to worry. Or did you have something special in mind? > - Who should I talk to regarding the final upload? I assume this will be added to this Wiki page: http://wiki.eclipse.org/JDT_Core/Null_Analysis/Options I don't expect this to appear on any official download page at this point.
I test drove the patch and everything seems to be ok. I guess you chose to omit the null analysis for fields because of the inconsistency between fields of 'this' and alien references. However, this solution also has its own inconsistencies. > + Special case detection of "if (f != null) f.op();", i.e., null-check > *immediately* followed be a dereference. This is the easiest compromise > I can think of for avoiding almost-irrelevant warnings regarding fields > that are not @NonNull. This detection would be off by default but exposed > as a preference. (I already made an experiment in this direction, just > have to find the patch :) ). So, the patch works well for @NonNull annotated fields but for @Nullable the analysis will be weak right? OIOW, even if i initialize a field to a non-null value, I will still see potential NPE warnings on each dereference? Also in cases such as these: @Nullable Object foo = new Object(); void goo(@org.eclipse.jdt.annotation.NonNull Object o, @org.eclipse.jdt.annotation.Nullable Object o2) { this.foo = new Object(); o2 = new Object(); o = o2; // no warning here o = this.foo; // warning here } The story for fields and locals is becoming a bit inconsistent. Can't we just use the patch that we earlier reverted?
(In reply to comment #35) > I test drove the patch and everything seems to be ok. Thanks! > I guess you chose to omit > the null analysis for fields because of the inconsistency between fields of > 'this' and alien references. However, this solution also has its own > inconsistencies. > > + Special case detection of "if (f != null) f.op();", i.e., null-check > > *immediately* followed be a dereference. This is the easiest compromise > > I can think of for avoiding almost-irrelevant warnings regarding fields > > that are not @NonNull. This detection would be off by default but exposed > > as a preference. (I already made an experiment in this direction, just > > have to find the patch :) ). > So, the patch works well for @NonNull annotated fields but for @Nullable the > analysis will be weak right? OIOW, even if i initialize a field to a non-null > value, I will still see potential NPE warnings on each dereference? Exactly. You call it weak, I call the model pessimistic and emphasized that I'm not doing *any* flow analysis for fields, in the hope to be very clear. (Should I omit the above mentioned special case in order not to blur the picture?) > Also in cases such as these: > @Nullable Object foo = new Object(); This initializer doesn't help any, right? > void goo(@org.eclipse.jdt.annotation.NonNull Object o, > @org.eclipse.jdt.annotation.Nullable Object o2) { > this.foo = new Object(); > o2 = new Object(); > o = o2; // no warning here > o = this.foo; // warning here > } We *have* to make a difference, because for non-trivial examples we have no idea if foo still holds a valid reference when dereferenced. And currently we don't have the means to distinguish trivial from non-trivial situations in this regard. However, this reminds me that I was thinking of including final fields with an immediate non-null initializer. We could classify those as "effectively non-null" and spare the user the need to declare so. What do you think? My experience says this would significantly alleviate the effort, and it should be really simple to add - and communicate. > The story for fields and locals is becoming a bit inconsistent. IMO, it only reflects: locals are different from fields in general (the former are solely controlled by the current method, the latter are not). Since we currenlty can't go all the way to perfect analysis of all variable references, we do have to draw a line somewhere. Could there be a clearer line than saying: "We never perform flow analysis for fields"? Where do you suggest to draw the line? > Can't we just use the patch that we earlier reverted? Sure can, but I personally prefer not do so. Aside from all the open issues regarding that patch I think publishing the minimalistic / pessimistic analysis now can give us valuable feedback to a very clear-cut question: "Do we need flow analysis for fields or is assignment to local a tolerable and effective means to avoid the entire battle?" If we'd go to the other patch with flow analysis, what would be the research question? I mean this work is classified as experimental, so let's just do the experiment with the most interesting hypothesis, OK? My today's feeling says: flow analysis for fields may be most useful in scenarios *without* null annotations. *With* those annotations I'm really curious how far we get with a black-and-white model: @NonNull fields are good! We can check both relevant risks: missing initialization and assignment from null. No warning means: you're safe! @Nullable fields are dangerous! If you want to do real work with such a field *always* fetch a copy into a local variable and from there on our flow analysis is safe. After deploying this solution, if users convince us that this is not a good model, we'll have gained very valuable information towards the next release.
(In reply to comment #34) > (In reply to comment #32) > > A few questions when moving towards deployment: > > > > - Am I required to add disclaimers to the source code or is it sufficient > > to mark the download accordingly? > Marking it on the download page should be enough, though we are not speaking > about the "official" download page here. Good, thanks. > > - Since we don't have a central build ready for this branch, is it OK > > to simply export from the IDE? > You would have to create a feature patch. Sure. I'll assume this is a "yes" to "export from the IDE?" :) > > - When preparing a patch feature for deployment, should this project be > > pushed to git, too? > No, I would not add the feature patch to the repo a this point. OK, I'll post the patch feature to this bug for review, then. > > Anything to observe in order not to confuse the SDK build etc? > As along as all your work stays in your branch there's nothing to worry. Or did > you have something special in mind? I was asking just for the case when I would push the patch feature to git. The question is obsolte now. > > - Who should I talk to regarding the final upload? > I assume this will be added to this Wiki page: > http://wiki.eclipse.org/JDT_Core/Null_Analysis/Options > I don't expect this to appear on any official download page at this point. Alright, and comparing to the Java7 patch feature this all may boild down to the request to get write access to some directory below /shared/eclipse on build.eclipse.org if that's OK?
(In reply to comment #37) > Alright, and comparing to the Java7 patch feature this all may boild down > to the request to get write access to some directory below > /shared/eclipse on build.eclipse.org if that's OK? I don't like to see people doing stuff on the server (aka shell access). We can put the bits into the JDT Core web site like we did with patches: CVS root: eclipse.org path: www/jdt/core/<someFolder> This will end up at: http://www.eclipse.org/jdt/core/<someFolder>
(In reply to comment #36) > [..] > Exactly. You call it weak, I call the model pessimistic and emphasized that > I'm not doing *any* flow analysis for fields, in the hope to be very clear. > (Should I omit the above mentioned special case in order not to blur the > picture?) Is this special case already in the branch? I assumed its not in yet. I think its ok to have this for now because its annoying for a user to get a warning immediately after he puts a null check. > > Also in cases such as these: > > @Nullable Object foo = new Object(); > > This initializer doesn't help any, right? > > > void goo(@org.eclipse.jdt.annotation.NonNull Object o, > > @org.eclipse.jdt.annotation.Nullable Object o2) { > > this.foo = new Object(); > > o2 = new Object(); > > o = o2; // no warning here > > o = this.foo; // warning here > > } > > We *have* to make a difference, because for non-trivial examples we have > no idea if foo still holds a valid reference when dereferenced. > And currently we don't have the means to distinguish trivial from > non-trivial situations in this regard. > > However, this reminds me that I was thinking of including final fields > with an immediate non-null initializer. We could classify those as > "effectively non-null" and spare the user the need to declare so. > What do you think? My experience says this would significantly alleviate > the effort, and it should be really simple to add - and communicate. Yeah, that will certainly be valuable! > > The story for fields and locals is becoming a bit inconsistent. > > IMO, it only reflects: locals are different from fields in general > (the former are solely controlled by the current method, the latter are not). > Since we currenlty can't go all the way to perfect analysis of all > variable references, we do have to draw a line somewhere. Could there be > a clearer line than saying: "We never perform flow analysis for fields"? > Where do you suggest to draw the line? I'm ok as long as the messaging is correct. We don't want the users to be expecting "analysis" for fields but only diagnostics that result out of manually inserted annotations. And nothing changes these annotations for fields, which is unlike local variables where even a Nullable variable can be made not null by assignment. So, even null annotations for fields and variables behave a bit differently and we clearly need to spell out it. > > Can't we just use the patch that we earlier reverted? > > Sure can, but I personally prefer not do so. Aside from all the open issues > regarding that patch I think publishing the minimalistic / pessimistic > analysis now can give us valuable feedback to a very clear-cut question: > > "Do we need flow analysis for fields or is assignment to local a tolerable > and effective means to avoid the entire battle?" > > > If we'd go to the other patch with flow analysis, what would be the > research question? I mean this work is classified as experimental, so > let's just do the experiment with the most interesting hypothesis, OK? > > > My today's feeling says: flow analysis for fields may be most useful in > scenarios *without* null annotations. > > *With* those annotations I'm really curious how far we get with a > black-and-white model: > > @NonNull fields are good! We can check both relevant risks: missing > initialization and assignment from null. No warning means: you're safe! > > @Nullable fields are dangerous! If you want to do real work with such a > field *always* fetch a copy into a local variable and from there on > our flow analysis is safe. Yup, I guess we've already discussed these things at length so thats fine by me. I just mentioned the reverted patch because I think it adds to the analysis and as we saw earlier, gives helpful warnings in a lot of cases. So even when a user has not moved to null annotations, null analysis for fields can be switched on to clean up the code.
(In reply to comment #32) > I might want to add to this (time permitting): > > + Special case detection of "if (f != null) f.op();", i.e., null-check > *immediately* followed be a dereference. This is the easiest compromise > I can think of for avoiding almost-irrelevant warnings regarding fields > that are not @NonNull. This detection would be off by default but exposed > as a preference. (I already made an experiment in this direction, just > have to find the patch :) ). I've filed Bug 383368 for this part. A patch (tests & impl) has been pushed to the topic branch. The patch actually covers quite a range of patterns. I'm quite happy about separating these issues as "syntactic analysis" from the proper flow analysis. It keeps the impl simpler (I believe) and allows us to communicate to users: "flow analysis is always exact, but you may tell the compiler to filter errors/warnings by recognizing some syntactic patterns that are less dangerous than other null errors/warnings". This does not imply any guarantee that recognition of syntactic patterns will be complete in any sense. Extract to local variable is still preferable. Ayush, do you have some minutes for reviewing this for inclusion in the patch feature?
(In reply to comment #39) > (In reply to comment #36) > > However, this reminds me that I was thinking of including final fields > > with an immediate non-null initializer. We could classify those as > > "effectively non-null" and spare the user the need to declare so. > > What do you think? My experience says this would significantly alleviate > > the effort, and it should be really simple to add - and communicate. > Yeah, that will certainly be valuable! That's commit 5f4383ced9e2f64f3e8c52f26352af8d9ae159b9 in the topic branch.
I have a release candidate available at this update-site: http://build.eclipse.org/tools/objectteams/beta-null-annotations-for-fields Feature version is 0.7.0.201206232329 It supports everything listed on http://wiki.eclipse.org/JDT_Core/Null_Analysis/Beta Plugins are branched off the Juno release with these additions: For the JDT/Core side everything can be seen in the topic branch. For the JDT/UI part it contains these patches: bug 337977 comment 20 (update existing quick fixes) bug 337977 comment 18 (new quickfix: extract to null-checked local) bug 383368 comment 3 (preferences UI) If no serious issues are found I'd like to publish this on Juno-day. Or do we need a couple more days for reviews?
Marking as null beta to indicate availability on the feature patch and not on master
(In reply to comment #42) > I have a release candidate available at this update-site: > http://build.eclipse.org/tools/objectteams/beta-null-annotations-for-fields I know you posted that URL in our e-mail conversation, where it said "preview" and I didn't jump on it. But when I now look at it, it looks wrong to me to have a pure JDT Core/UI prototype served via http://build.eclipse.org/tools/objectteams. This is misleading for users, given this is a JDT prototype that will hopefully be included in JDT in the future. If possible it should be at a JDT Core location (e.g. in /cvsroot/org.eclipse /www/jdt/code/...).
(In reply to comment #44) > (In reply to comment #42) > > I have a release candidate available at this update-site: > > http://build.eclipse.org/tools/objectteams/beta-null-annotations-for-fields > > I know you posted that URL in our e-mail conversation, where it said "preview" > and I didn't jump on it. But when I now look at it, it looks wrong to me to > have a pure JDT Core/UI prototype served via > http://build.eclipse.org/tools/objectteams. This is misleading for users, given > this is a JDT prototype that will hopefully be included in JDT in the future. > If possible it should be at a JDT Core location (e.g. in /cvsroot/org.eclipse > /www/jdt/code/...). Yes, we will publish a link <http://www.eclipse.org/jdt/core/to_be_determined> as mentioned in http://wiki.eclipse.org/JDT_Core/Null_Analysis/Beta
(In reply to comment #45) > (In reply to comment #44) > > (In reply to comment #42) > > > I have a release candidate available at this update-site: > > > http://build.eclipse.org/tools/objectteams/beta-null-annotations-for-fields > > > > I know you posted that URL in our e-mail conversation, where it said "preview" > > and I didn't jump on it. But when I now look at it, it looks wrong to me to > > have a pure JDT Core/UI prototype served via > > http://build.eclipse.org/tools/objectteams. This is misleading for users, given > > this is a JDT prototype that will hopefully be included in JDT in the future. > > If possible it should be at a JDT Core location (e.g. in /cvsroot/org.eclipse > > /www/jdt/code/...). > > Yes, we will publish a link <http://www.eclipse.org/jdt/core/to_be_determined> > as mentioned in http://wiki.eclipse.org/JDT_Core/Null_Analysis/Beta My intention exactly! :) Would it be a release *candidate* if I published it at the final location? Also, I don't want to spam the CVS with unnecessary versions of binary files. I will remove the preview/release candidate as soon as the final beta will be published (using the objectteams location now only for convenience because that's where I have direct write access).
Created attachment 217931 [details] project to show minor problems in null annotation beta support (In reply to comment #42) > I have a release candidate available at this update-site: > http://build.eclipse.org/tools/objectteams/beta-null-annotations-for-fields > [..] > If no serious issues are found I'd like to publish this on Juno-day. > Or do we need a couple more days for reviews? I tested this on a fresh RC4 installation and seems to work fine. Only a few minor issues as pointed out by comments in Test.java in attached project. They should be easy to fix but even if they aren't, it should be ok to wait a couple of days to fix these before we publish the link. Is the exception thrown on the quick fix because UI changes haven't gone in?
(In reply to comment #47) > I tested this on a fresh RC4 installation and seems to work fine. Only a few > minor issues as pointed out by comments in Test.java in attached project. They > should be easy to fix but even if they aren't, it should be ok to wait a couple > of days to fix these before we publish the link. Is the exception thrown on the > quick fix because UI changes haven't gone in? Thanks for testing! Three of the issues you recorded relate to if (f == null) ... According to http://wiki.eclipse.org/JDT_Core/Null_Analysis/Beta#Syntactic_analysis this syntactic analysis only recognizes "!= null" not "== null". The former is important to allow dereferencing any nullable field. Do you see a strong reason why also recognizing "== null" would be important? We get an error anyway, right? Isn't that enough? IOW, I intend syntactic analysis only for avoiding essentially uninteresting errors/warnings - to become less pessimistic, not for reporting stronger errors. Any objections? Regarding this snippet: if (this.abc2 == o2) { return this.abc2; // isn't this the same as this.abc2 == null? We shouldn't get an error here, no? } is there some typo involved? o2 is declared @NonNull. I guess you mean it should be equivalent to "this.abc2 != null", right ? The other issues I will have to investigate on my other machine later today.
(In reply to comment #48) > [..] > > Thanks for testing! > > Three of the issues you recorded relate to > if (f == null) ... > According to > http://wiki.eclipse.org/JDT_Core/Null_Analysis/Beta#Syntactic_analysis > this syntactic analysis only recognizes "!= null" not "== null". > The former is important to allow dereferencing any nullable field. > Do you see a strong reason why also recognizing "== null" would be important? > We get an error anyway, right? Isn't that enough? I see your point. I thought you must've added some code for EqualExpression to support this special case so was surprised that it did not work for != . So, i'm ok with what is currently supported, since nobody dereferences a variable that they just compared to null in the previous line anyway. :) > Regarding this snippet: > if (this.abc2 == o2) { > return this.abc2; // isn't this the same as this.abc2 == null? > We shouldn't get an error here, no? > } > is there some typo involved? o2 is declared @NonNull. > I guess you mean it should be equivalent to "this.abc2 != null", right ? Yeah, that's what I meant. :)
The test class has this line: @NonNull public Object abc; // when this is static the error comes at this location instead of on the class declaration. Should always be reported on the same location, no? I had to recheck, but this is indeed the same behavior as for final fields: uninitialized static final is reported against the class, non-static against the field. Perhaps we should align locations in all cases? For now, which consistency is more important: - treat @NonNull like final - treat @NonNull static like @NonNull non-static?
(In reply to comment #50) > against the field. Perhaps we should align locations in all cases? > For now, which consistency is more important: > - treat @NonNull like final > - treat @NonNull static like @NonNull non-static? Can't all of them be reported against either the class or the individual field? My concern is not so much about the warning being at 2 different places for 2 different cases but about the precarious situation that arises when the 2 are mixed (eg. one field static other non-static). For one field, we complain on the class and for another we complain on the field. This is a bit weird.
(In reply to comment #51) > (In reply to comment #50) > > against the field. Perhaps we should align locations in all cases? > > For now, which consistency is more important: > > - treat @NonNull like final > > - treat @NonNull static like @NonNull non-static? > Can't all of them be reported against either the class or the individual field? They can, that's what I meant by: > > Perhaps we should align locations in all cases? But should I really change the implementation for final fields on behalf of this current bug and for the patch feature? > My concern is not so much about the warning being at 2 different places for 2 > different cases but about the precarious situation that arises when the 2 are > mixed (eg. one field static other non-static). For one field, we complain on > the class and for another we complain on the field. This is a bit weird. agree. The same holds for mixing uninitialized final fields (static & non-static).
> this.abc = o; // incorrect quick fix, throws exception Thanks for the test case! This is a simple demonstration of why we need tests for those quick fixes. The existing logic just didn't properly distinguish all possible cases. I'm working on some tests to make sure when I change this case it won't break 10 others.
(In reply to comment #52) > [..] > They can, that's what I meant by: > > > Perhaps we should align locations in all cases? > But should I really change the implementation for final fields on behalf of > this current bug and for the patch feature? No, we can let that be as it is since its existing behaviour. Just for the new warnings we can be consistent if the fix is trivial. If thats complicated, then even that can be left as such with a note on the wiki page.
(In reply to comment #54) > (In reply to comment #52) > > [..] > > They can, that's what I meant by: > > > > Perhaps we should align locations in all cases? > > But should I really change the implementation for final fields on behalf of > > this current bug and for the patch feature? > No, we can let that be as it is since its existing behaviour. Just for the new > warnings we can be consistent if the fix is trivial. Not difficult. So I'll count your vote like this: (In reply to comment #50) > - treat @NonNull like final NO > - treat @NonNull static like @NonNull non-static? YES I'll update this in the topic branch (always report against the field). The case of final fields is now in Bug 383690.
This snippet from comment 47 (abc is @NonNull, abc2 is @Nullable): if (t.abc2 == null) this.abc = t.abc2; // should say "provided value is null produced an error saying "is inferred as @Nullable", which is wrong. I've pushed a simple fix to the topic branch, which corrects this to saying "is specified as @Nullable" (commit contains a test): http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=sherrmann/NullAnnotationsForFields2&id=1b2babf22d9054679a4b8f35db671abd674f38ba One location in ProblemReporter wasn't yet updated for handling fields, too.
This snipped from comment 47 (abc is @NonNull, t2f is @NonNull by package default): this.abc = t2.t2f; // why type safety? t2f is already @NonNull (goes away if t2f has @NonNull explicit) produced a bogus warning regarding unchecked conversion. Test & fix is in the topic branch at http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=sherrmann/NullAnnotationsForFields2&id=4e788ed0d52a1656beee9be3ee408fb2867ef1b0 The issue here was, that a nullness default was not propagated into binary fields (since the implementation in this bug pre-dated bug 366063 which changed the game).
This snippet from comment 47 (abc2 is @Nullable, o2 is @NonNull, return is @NonNull, typo in comment fixed) if (this.abc2 == o2) { return this.abc2; // isn't this the same as this.abc2 != null? We shouldn't get an error here, no? } produced an avoidable type mismatch error (specified @Nullable vs. @NonNull). At a closer look it was simple to fix so that "== nonnull" is handled the same way as "!= null" (not sure if this is relevant in real life :) ). Test&fix are in http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=sherrmann/NullAnnotationsForFields2&id=9ad82e71b09cf3e79f0cf9c5fe7c414251c32119 This concludes the work on the examples from comment 47. Thanks again, those examples were very helpful. I'm re-running JDT/Core and /UI test suites to see if we're good to go now.
Thanks for taking care of these issues Stephan! :)
(In reply to comment #58) > I'm re-running JDT/Core and /UI test suites to see if we're good to go now. After all tests signaled green, I uploaded the patch feature to http://www.eclipse.org/jdt/core/beta-null-annotations-for-fields and updated the wiki page accordingly. Apart from the wiki there isn't really a public announcement, yet, but I submitted an article on EclipseZone, waiting for moderation.
I tried the beta feature, but I'm getting internal compiler errors. I narrowed down to very small repro: package-info.java: @org.eclipse.jdt.annotation.NonNullByDefault package test; Test.java: package test; class Test { enum A { B } } I tested on a clean install of eclipse classic 4.2 (windows 64-bit) with no plugins other than this feature (installed from the update site http://www.eclipse.org/jdt/core/beta-null-annotations-for-fields). 'Enable annotation-based null analysis' needs to be checked to see the repro. Thanks.
(In reply to comment #61) > I tried the beta feature, but I'm getting internal compiler errors. I narrowed > down to very small repro: Thanks for the concise test case. I can reproduce. BTW this is a sad little demonstration that this tool isn't self-applied because we can't use annotations in the compiler implementation: We're dereferencing a field which clearly should be marked @Nullable ...
The real solution is to fix bug 380896. Test & fix for both issues have been pushed to the branch via commit d7159ad7486a45852823b45141eb68a3dc975ba3.
(In reply to comment #55) > (In reply to comment #50) > > - treat @NonNull like final > NO > > > - treat @NonNull static like @NonNull non-static? > YES > > I'll update this in the topic branch (always report against the field). To clarify: If multiple constructors assign to @NonNull non-static, but some don't I want a warning for each constructor, not one warning on the field. I don't want to search the failing constructors myself. So "always" refers to "always in absence of explicit constructors"! CONTRA CHANGE: keep old final handling If no explicit constructor was written, reporting warnings against the class reminds me of automatically getting an implicit constructor. This could be a little extra hint for noobies to learn the difference between static and non-static. PRO CHANGE: never report against class Reporting those warnings against the field directly will point me to the most likely position, I want to fix the bug by adding an initial assignment or removing the final keyword / the annotation. OK, I'd give it a try, meager 51% for the change, as I don't consider myself a nooby anymore *g*
(In reply to comment #64) > (In reply to comment #55) > > (In reply to comment #50) > > > - treat @NonNull like final > > NO > > > > > - treat @NonNull static like @NonNull non-static? > > YES > > > > I'll update this in the topic branch (always report against the field). > > To clarify: ... I've released a fix for bug 383690 regarding this issue. Beyond that I see no reasson for further changes. Feel free to check the test cases in that bug if anything is amiss (or try a nightly build > N20120730-2000 with examples of your own).
I've released updated versions of the patch feature to these locations: Juno SR1: - http://www.eclipse.org/jdt/core/beta-null-annotations-for-fields/R4.2.1 Kepler M2: - http://www.eclipse.org/jdt/core/beta-null-annotations-for-fields/4.3milestones Apart from the fixes for bug 380896 and bug 388630 these updates contain nothing new from my side, but integrate the beta feature into the respective milestone / release of JDT.
(In reply to comment #66) > http://www.eclipse.org/jdt/core/beta-null-annotations-for-fields/R4.2.1 > http://www.eclipse.org/jdt/core/beta-null-annotations-for-fields/4.3milestones 404 - File not found ... these links were not supposed to be clicked in an ordinary web-browser, were they?
(In reply to comment #67) > (In reply to comment #66) > > http://www.eclipse.org/jdt/core/beta-null-annotations-for-fields/R4.2.1 > > http://www.eclipse.org/jdt/core/beta-null-annotations-for-fields/4.3milestones > > 404 - File not found ... these links were not supposed to be clicked in an > ordinary web-browser, were they? No, those are update sites as usual.
A new version of the patch feature matching Kepler M3 is on the server. If p2 is slow detecting the update at http://www.eclipse.org/jdt/core/beta-null-annotations-for-fields/4.3milestones feel free to point directly to the current actual location: http://archive.eclipse.org/jdt/core/beta-null-annotations-for-fields/4.3milestones The first address is still recommended, but since that's just a redirecting composite repo ATM, p2 seems to be dense in understanding that s.t. has changed behind the fassade. Again not much has changed regarding this bug (except a correction regarding quick fixes applied in multi mode). The major part is the rebase on top of jdt 4.3M3 (which contains, e.g., the better part of bug 388281).
As I'm currently testing the re-base of this feature on top of SDK 4.3 M4 I'd like to know: - does anyone need previous milestone versions, or is it OK if I simply replace the beta download with the latest version?
I'm right now merging this feature back into master (for Kepler). The patch will include fixes for bug 382789, bug 380896, bug 383368, bug 388630. To be perfectly clear, the solution will take the approach that started in comment 32 whereby no flow analysis is applied to fields; see bug 383368 for "syntactic analysis" which instead of flow analysis covers the most obvious cases of checked access to @Nullable fields. This is the solution described also in http://wiki.eclipse.org/JDT_Core/Null_Analysis/Beta
(In reply to comment #71) > The patch will include fixes for [...] bug 388630. Correction: that particular fix is already in master.
While reviewing my changes here're a few notes on the implementation: Checks for missing initialization of @NonNull fields are in Clinit and ConstructorDeclaration. These checks are in exact analogy to definite assignment checks for final fields. FieldDeclaration.analyseCode helps by marking @NonNull fields as assigned. FieldDeclaration.analyseCode performs more checks to detect: - @NonNull field wrongly initialized with a nullable value - final fields initialized to nonnull -> mark as effectively @NonNull A simple new method Expression.nullAnnotatedVariableBinding() detects whether the lhs in an assignment is constrained to @NonNull (local or field). Method EqualExpression.checkNullComparison() has been considerably re-worked. That method was known to be incomplete, marked with: // TODO: handle all kinds of expressions (cf. also https://bugs.eclipse.org/364326 I have added detailed code comments to the new implementation. Another comprehensive case analysis is in ProblemReporter.expressionNonNullComparison(), which is where also much of bug 382789 happens. Changes in BinaryTypeBinding, SourceTypeBinding and FieldBinding relate to retrieving null annotations for fields - from byte code - from defaults from an enclosing scope Everything else is small, pretty straight forward and should cause no headache.
(In reply to comment #73) > Everything else is small, pretty straight forward and should cause no headache. and will certainly relieve many headaches as well! thank you, Stephan, for all your work on Null Analysis in Eclipse. i would love to see this develop into a standard one day! ~ RNPG
Here's one little feature I am currently withdrawing from the patch: I thought it neat to treat final fields with a non-null initializer as effectively @NonNull (comment 41). On closer investigation I don't see how this could possibly be supported for binary types, since the byte code doesn't have the info about non-null initialization. If we'd support this convenience feature, the compiler would give different results when read a class from source vs. binary, which is not acceptable. If anybody has an idea how this can be solved feel free to open a new bug, but I believe we just have to ask the user to make nonnull explicit using an annotation.
After more reviewing, cleanup and testing against suites from jdt.core and jdt.ui plus field test I'm confident that all is well. Release for 4.3 M5 via commit a846071c58f9098177eef02be0134294158f9c4f
(In reply to comment #76) > After more reviewing, cleanup and testing against suites from jdt.core and > jdt.ui plus field test I'm confident that all is well. > > Release for 4.3 M5 via commit a846071c58f9098177eef02be0134294158f9c4f I noticed that you added the option for the syntactic analysis: COMPILER_PB_SYNTACTIC_NULL_ANALYSIS_FOR_FIELDS If this is in good shape, then please raise a bug against JDT UI, so we can add it for M5.
(In reply to comment #77) > (In reply to comment #76) > > After more reviewing, cleanup and testing against suites from jdt.core and > > jdt.ui plus field test I'm confident that all is well. > > > > Release for 4.3 M5 via commit a846071c58f9098177eef02be0134294158f9c4f > > I noticed that you added the option for the syntactic analysis: > COMPILER_PB_SYNTACTIC_NULL_ANALYSIS_FOR_FIELDS > If this is in good shape, then please raise a bug against JDT UI, so we can > add it for M5. Sure, I was going to raise a bug for the option and propose improvements for the quick fixes. I have some code for both of these from the beta feature (on a different machine than I'm on right now). I'll clean-up and post everything tomorrow. Thanks
(In reply to comment #78) > (In reply to comment #77) > > (In reply to comment #76) > > > After more reviewing, cleanup and testing against suites from jdt.core and > > > jdt.ui plus field test I'm confident that all is well. > > > > > > Release for 4.3 M5 via commit a846071c58f9098177eef02be0134294158f9c4f > > > > I noticed that you added the option for the syntactic analysis: > > COMPILER_PB_SYNTACTIC_NULL_ANALYSIS_FOR_FIELDS > > If this is in good shape, then please raise a bug against JDT UI, so we can > > add it for M5. > > Sure, I was going to raise a bug for the option and propose improvements for the > quick fixes. I have some code for both of these from the beta feature (on a > different machine than I'm on right now). I'll clean-up and post everything > tomorrow. Thanks X-ref to new bugs (patch available in both): - Bug 398965: [preferences] UI option for syntactic analysis for fields - Bug 398995: [quick fix] Extract field access to checked local variable Status of Bug 337977 is not perfectly clear and it may still have more pending action items, but I'll collect information from various comments and bugs on some later day - to present an overview of where we stand, and propose fixes if I can.
*** Bug 403192 has been marked as a duplicate of this bug. ***
Verified for 4.3 M6 with build I20130310-2000.
*** Bug 480717 has been marked as a duplicate of this bug. ***
