Community
Participate
Working Groups
Build: 3.5 M2 candidate Given this code: public class A { Object val; public void test() { if (val == null && val.hashCode() == 0) System.err.println("won't get here"); } } The "val.hashCode()" expression will NPE because the "val" field is sure to be null. I expected the null reference analysis to flag this. Since it is a field it is theoretically possible that another thread could assign a non-null value to the field after the null check, so I could buy only flagging this as "potential null pointer access".
I agree that we could improve (potential) null analysis in certain scenarios, but we have to be careful though here since fields can be modified in different threads.
For the given case, it would indeed be nice to have a "null pointer access" problem, especially since the field is not volatile and there's nothing else that can provide proper synchronization between the null check and the access to the field. However, if "val == null" and the access to "val" are farther apart, then you can probably run into cases that are properly synchronized my other means, and there we must not say that "val can only be null". If you can distinguish these cases, then I would be bold and flag the first case with the "null pointer access" problem id and a slightly modified message, e.g. "Null pointer access: The field val can only be null at this location, unless modified in a different thread". More complicated cases and accesses to volatile fields should only show up as "potential null pointer access".
Null analysis for fields is uncharted waters for the JDT, so here's some quick archeology regarding mentions in early scriptures: An interesting source is rev 1.1 of NullReferenceTest, it has numerous mentions of "The field o is likely null; it was either set to null or checked for null when last used" but already commit 1.3 by pmulet (on behalf of maxime?) commented out these warnings. A comparison with problem/messages.properties confirms that these warnings never existed. However, the field UnconditionalFlowInfo.maxFieldCount is actually used to reserve space regarding fields within nullBit1-4. These unused bits can straightforwardly be used for fields, using the 'id' as the bit index. Such use is only blocked by, e.g., FlowInfo.markNullStatus(LocalVariableBinding, int) due to the parameter type LocalVariableBinding, but it might be quite simple to refactor this to using VariableBinding instead. So from the infrastructure point of view this looks well doable. Question is, how to approximate the desired semantics: We could easily let all null info for fields "age", i.e., wipe soon after, but that would be all-or-nothing, no easy solution to detect locations at various distance. Any thoughts, Ayush?
(In reply to comment #3) > Null analysis for fields is uncharted waters for the JDT, so here's some > quick archeology regarding mentions in early scriptures: > > An interesting source is rev 1.1 of NullReferenceTest, it has numerous > mentions of > "The field o is likely null; it was either set to null or checked for null > when last used" > but already commit 1.3 by pmulet (on behalf of maxime?) commented out these > warnings. A comparison with problem/messages.properties confirms that these > warnings never existed. > > However, the field UnconditionalFlowInfo.maxFieldCount is actually used to > reserve space regarding fields within nullBit1-4. These unused bits can > straightforwardly be used for fields, using the 'id' as the bit index. > > Such use is only blocked by, e.g., > FlowInfo.markNullStatus(LocalVariableBinding, int) > due to the parameter type LocalVariableBinding, but it might be quite > simple to refactor this to using VariableBinding instead. > > So from the infrastructure point of view this looks well doable. Yup. I have already implemented this much, and my patch is almost done. It just has a couple of problems that need to be tackled. I'm still working on it. > Question is, how to approximate the desired semantics: > We could easily let all null info for fields "age", i.e., wipe soon after, > but that would be all-or-nothing, no easy solution to detect locations > at various distance. I didnt quite understand what you meant by this. Do you mean the null info being specific to a particular method or block and not be carrying between them? I guessed it'll be sufficient to preserve the null info for final fields across all methods, while for the other fields, we anyways dont need to carry the info.
(In reply to comment #4) > (In reply to comment #3) > > Question is, how to approximate the desired semantics: > > We could easily let all null info for fields "age", i.e., wipe soon after, > > but that would be all-or-nothing, no easy solution to detect locations > > at various distance. > > I didnt quite understand what you meant by this. Do you mean the null info > being specific to a particular method or block and not be carrying between > them? I didn't mean anything inter-procedural (although, I was indeed curious about this issue in anticipation of null annotations). I meant to talk about the difference between if (val == null && val.hashCode() == 0) System.err.println("won't get here"); and if (val == null) { someOtherStuffHere(); if (val.hashCode() == 0) System.out.println("Can get here if method call has modified val"); } Are we going to discard all info regarding 'val' when we pass 'someOtherStuffHere()'? Will we do so also in this case: if (val == null) { if (int i=0; i<1000000; i++) otherVal = 1234 / i * 4234; if (val.hashCode() == 0) System.out.println("Can get here if other thread has modified val"); } So my notion of distance referred to Markus' comment "... if "val == null" and the access to "val" are farther apart ... " or: how long after seeing "if (val == null)" will we still assume that indeed "val == null"?
(In reply to comment #5) > [..] Ah! Now i see what you mean. I was thinking of ignoring this in the initial implementation. The first case with a method call will be supported when we support inter-procedural analysis. (I guessed if we just discard the null info because a method was called, in almost all cases this new warning will be useless as it wont be emitted at all!) The second case where you say that another thread can modify val is being taken care of by emitting the "potential null reference warning" and not the "null reference warning". Let me know what you think.
(In reply to comment #6) > Ah! Now i see what you mean. I was thinking of ignoring this in the initial > implementation. The first case with a method call will be supported when we > support inter-procedural analysis. (I guessed if we just discard the null info > because a method was called, in almost all cases this new warning will be > useless as it wont be emitted at all!) I find as soon as a warning produces false positives, people start to disable it. Therefore I would suggest as a starting point that you *do* discard null info as soon as any method is called, rather than assuming methods calls will not affect field values. Thus as a starting point, this warning would only work for simple cases like the one in comment #0.
(In reply to comment #7) > this warning would only work for simple cases like the one in comment #0. +1
(In reply to comment #3) > [..] > However, the field UnconditionalFlowInfo.maxFieldCount is actually used to > reserve space regarding fields within nullBit1-4. These unused bits can > straightforwardly be used for fields, using the 'id' as the bit index. While finalising my patch, I realised that this approach might actually fail. Consider the following ex: class Super { int field1, field2, field3; } class A extends super { int a; void m() { int local1, local2, local3; if (this.field3 == 1) {} } } Here when we try to refer to local 2, its id becomes local.id + maxFieldCount = 1 + 1 = 2. Again while trying to access info for field3, we use local.id which is again 2. So the null infos for both local2 and field3 are conflicting here. We need to work out another way to index into the null bits. Stephan, what do you think?
(In reply to comment #9) > (In reply to comment #3) > > [..] >[..] > We need to work out another way to index into the null bits. Stephan, what do > you think? We could probably work around this by keeping maxFieldCount as a cumulative count of the fields from not just the current type but also the supertypes. However, ReferenceBinding currently has no api which can just return the no. of fields in the type without resolving them and doing all the fancy stuff. I think we'll need such an api. Requesting it in bug 332359.
Created attachment 185536 [details] proposed fix v1.0 + regression tests This patch basically leverages the existing null analysis infrastructure. The only difference being that this fix uses the bit positions below the maxFieldCount, to correspond to the fields being analysed and stores and retreived null info for fields using VariableDeclaration$id. This fix also replaces LocalVariableDeclaration by VariableDecalaration at all places, since now we deal with both LVD and FieldDeclaration AST nodes. Four new error messages are intoruduced: /** @since 3.7 */ int NullFieldReference = Internal + FieldRelated + 670; /** @since 3.7 */ int PotentialNullFieldReference = Internal + FieldRelated + 671; /** @since 3.7 */ int RedundantNullCheckOnNullField = Internal + FieldRelated + 672; /** @since 3.7 */ int NullFieldComparisonYieldsFalse = Internal + FieldRelated + 673; /** @since 3.7 */ int RedundantNullCheckOnNonNullField = Internal + FieldRelated + 674; /** @since 3.7 */ int NonNullFieldComparisonYieldsFalse = Internal + FieldRelated + 675; /** @since 3.7 */ int RedundantFieldNullAssignment = Internal + FieldRelated + 676; /** @since 3.7 */ int NullFieldInstanceofYieldsFalse = Internal + FieldRelated + 677; These are on the lines of similar messages for local variables, and they are controlled using the same compiler preferences that control the existing null analysis options. Some points to note are: 1) The null info for the fields(non-static final) gets erased on a method call (MessageSend), and also when we exit a method or a block. 2) The null status for fields is only marked as potentially null or non null instead of definitely null or non null, since fields can be modified in other thread as well. 3) For static final fields (class fields), FieldBinding$nullStatus has been introduced. Since static final fields are always initialized at the time of declaration or a static initializer, we can store the null info at the time of initialization and rest assured that the value will not change further on. So the mechanism of storing and retrieving null info for static final fields has been differentiated from the usual fields. 4) In synchronised methods, all fields are treated as locals, i.e. a definite null status is not converted to potential null status, etc. 5) In methods org.eclipse.jdt.internal.compiler.flow.UnconditionalFlowInfo.markAsDefinitelyNonNull(VariableBinding, boolean) and markAsDefinitelyNull(..), I noticed that there isn't any provision for expanding the extra vector when required. This was causing problem in cases where no. of fields was very large. Fixed in this patch. 6) This patch also has the fix for bug 332359 in BinaryTypeBinding, and ParameterizedTypeBinding.(I'll release it separately though). 7) Currently most places where we were earlier using LocalVariableDeclaration still have the variable name as 'local'. I'm not changing this right now to keep the patch manageable. Once it is reviewed, i'll refactor the names. Added tests in NullReferenceTests. All JDT/Core tests pass. Stephan, can you please review?
After reading through the patch I do have some questions, but firstly let me just show a counter example: public class TestClass { volatile static int state = 0; interface TwoRuns extends Runnable { void run2(); } static synchronized TwoRuns outerTest() { return new TwoRuns() { Object val; public void run() { val = null; state = 1; while (state == 1) ; // nothing(); if (val == null) return; System.out.println("DeadCode?"); } public void run2() { while (state == 0) ; val = new Object(); state = 2; } void nothing() {} }; } public static void main(String[] args) { TwoRuns tests = outerTest(); new Thread(tests).start(); tests.run2(); } } Depending on whether the new analysis is used or not (disable e.g. by un-commenting the call to nothing() which resets null-info) this program will (not) print "DeadCode?", and the difference is not because of changed timing but because of wrong deadcode elimination. So, how did I trick the compiler into thinking that this is dead code? The analysis uses a method isOuterMostMethodSynchronized() to determine whether concurrency must be handled. However, synchronizing the outermost method not necessarily protects the field in question. Synchronization must happen regarding the instance or class that holds the field. Here's another case that's not handled correctly: public class A { static Object classVal; synchronized void test() { classVal = null; double d = 0; for (int i=0;i<1000000;i++) // just some non-trivial computation d = (d + 3) / 7; System.out.println(classVal==null); } public static void main(String[] args) throws InterruptedException { new Thread(new Runnable() { public void run() { new A().test(); } }).start(); Thread.sleep(10); classVal = new Object(); } } Compiler claims at "classVal==null": "Redundant null check: The field classVal can only be null at this location" however, the program prints: "false" This illustrates that static field and non-static 'synchronized' must not be mixed in the analysis. Sorry, I wish I had better news ...
Next issue: references to fields of another instance. Consider these three references to a field and how they implement checkNPE: SingleNameReference handles fields - can only be a field of 'this', good. QualifiedNameReference explicitly tests for bit Binding.LOCAL, - will not handle fields, good. FieldReference re-uses the inherited checkNPE(..) from Expression - with the patch this will handle fields, too: bad. I assume, FieldReference should override checkNPE(..) like this: if (this.receiver.isThis()) super.checkNPE(..) Here's a witness for the issue: public class A { Object val; A getA() { return new A(); } public synchronized void test() { if (getA().val == null && this.val.hashCode() == 0) System.err.println("may actually get here"); } } Compiler says (at "this.val.hashCode()"): "Null pointer access: The field val can only be null at this location" This is not correct.
(In reply to comment #12) The issues pointed out here make me feel its better to not introduce special analysis for synchronizations, because in order to be completely sure whether a field's access is *actually* synch'd we will need to be sure that all the methods changing its value are also synch'd, or if the instance or class that holds the field is synch'd. I don't see a way of doing it in the way we currently do static analysis. So, I think its better to let the special treatment for synchronized methods go away for now. I'll take care of comment 13 here, and leave the definitely null warnings for static final fields only.
(In reply to comment #14) > (In reply to comment #12) > The issues pointed out here make me feel its better to not introduce special > analysis for synchronizations, because ... While all these obstacles should in principle be resolvable without architectural changes, I agree that for this bug it is probably better to keep the solution lean. Let's see where bug 331649 will take us, maybe with @NonNull annotations for fields the issue of sync'ed methods won't be all that important any more? > I'll take care of comment 13 here, and leave the definitely null warnings for > static final fields only. Why are you restricting special treatment to static final vs. just final?
(In reply to comment #15) > [..] > Why are you restricting special treatment to static final vs. just final? Because final fields can be initialized in different constructors for different instances and there's no way to know from which constructor the value is coming from in any particular case.
(In reply to comment #16) > (In reply to comment #15) > > [..] > > Why are you restricting special treatment to static final vs. just final? > > Because final fields can be initialized in different constructors for different > instances and there's no way to know from which constructor the value is coming > from in any particular case. So we would have to merge null info from all constructors and you don't want to do that in order to keep the analysis local, right? Here are some detailed comments from reading the patch: (1) MethodDeclaration calls flowInfo.resetNullInfoForFields(); This seems unnecessary because nobody holds a reference to the flowInfo beyond that point. (2) TypeDeclaration loops over superclasses to count fields. Have you considered storing the number of fields per type so that each type only needs to ask its direct supers? (3) Some cosmetics regarding ProblemReporter: regarding all those methods that you split into three variants (report.., local..., field...): I don't see why we need three methods rather than dynamically determining the problemId based on the given variable binding. The prefix "report" is redundant in a class called "..Reporter", and finally: you're breaking alphabetic order. (4) In LoopingFlowContext locals and fields are treated uniformly in most cases, except for localVariableRedundantNullAssignment and localVariableNullInstanceof. If this is intentional maybe a comment saying why these are different would be in place. (5) I noticed many changes in NullInfoRegistry where you change an index into 'extra' from 2 to 0. Why? I would assume that indeed the NullInfoRegistry doesn't care about the lower two positions, which are concerned with definite assignment rather than nullness, no? (6) In UnconditionalFlowInfo.resetNullInfoForFields() you might want to use a bitmask like "-1 << maxFieldCount" to blank out all field-bits in one operation instead of the loop :) (this holds only for the lower bits, 'extra' still needs to be handled in a loop). How come, this method would need to grow the extra arrays? If it hasn't grown before there won't be any bits to reset, no? (7) Constructor FieldBinding(FieldBinding,ReferenceBinding) misses to initialize nullStatus.
(In reply to comment #17) >[..] Your'e right about points 1, 3, 4 and 7. Points 5 and 6 are also right. Actually, the check for whether the "extra" array is null or not was put in resetNullInfoForFields(..) because in this method, we reset bits for all fields, and some of them might not have been encountered yet in null analysis, and thereby no space would've been created in the 'extra' array. But yes the obvious solution would've been to just skip attempting to mask the bits for these fields. I guess I should just use "-1 << maxFieldCount" and avoid the hassle. Also, I dont remember the reason for those changes in NullInfoRegistry -they were not supposed to be here. :( Point 2 is something that i've already considered and rejected. This is because we dont know whether we will obtain a SourceTypeBinding or a BinaryTypeBinding for a type (maybe there's no source attached), and in case we do obtain BTB, we wont be able to derive the TypeDeclaration from it. So storing a cumulative field count will fail in this case. Hence calculating it in all cases.
(In reply to comment #17) > [..] > > (5) I noticed many changes in NullInfoRegistry where you change an index > into 'extra' from 2 to 0. Why? I would assume that indeed the > NullInfoRegistry doesn't care about the lower two positions, which > are concerned with definite assignment rather than nullness, no? I think I found the reason why I'd done this. The JDT/Core code itself throws NPE while trying to compile using the new patch. This is because in NullInfoRegistry, this.extra[0] and extra[1] are always null. But when it calls a method such as markPotentiallyNonNullBit(..) it does that via UnconditionalFlowInfo, where we use this.extra[0].length to check if we need to grow the arrays. hence an NPE.
Created attachment 185729 [details] proposed fix v1.1 + regression tests This fix takes care of all the above points. I have removed the code for special handling of synchronized methods. The problem pointed out in comment 13 has been fixed by fixing ReferenceBinding#variableBinding(). I didn't see any need to touch checkNPE() here. We should just make sure that in cases like getA().val, where val is not a static field, we should not return the field binding to be analysed since we dont know what object the field belong to. I've added some more tests as well. (Currently this fix also includes the fix for bug 333089, though I will release it separately).
Stephan, will you be able to review this for M5?
(In reply to comment #21) > Stephan, will you be able to review this for M5? Yes, sure.
(In reply to comment #16) > (In reply to comment #15) > > [..] > > Why are you restricting special treatment to static final vs. just final? > > Because final fields can be initialized in different constructors for different > instances and there's no way to know from which constructor the value is coming > from in any particular case. I agree that this is a bit more involved: it would involved merging the null info from all constructors, which in turn requires flow infos to be stored until all constructors have been analyzed and then we would have to enforce that all constructors are analyzed before starting to analyze methods. So while I believe it's possible I don't mind leaving this for a possible future enhancement.
(In reply to comment #18) > > (2) TypeDeclaration loops over superclasses to count fields. Have you > > considered storing the number of fields per type so that each type only > > needs to ask its direct supers? ... > Point 2 is something that i've already considered and rejected. This is because > we dont know whether we will obtain a SourceTypeBinding or a BinaryTypeBinding > for a type (maybe there's no source attached), and in case we do obtain BTB, we > wont be able to derive the TypeDeclaration from it. So storing a cumulative > field count will fail in this case. Hence calculating it in all cases. My naive reaction to this would be to move that method from TypeDeclaration to ReferenceBinding. But this is just a tiny issue with no need for action.
While challenging the patch I found two changes that I could safely revert without breaking any of NullReferenceTest: 1) the change from bug 333089 (now at UnconditionalFlowInfo:1535-1538) 2) within NullInfoRegistry the three new overrides markPotentiallyNullBit ff. Are any of these covered by tests in another test class? For 1) we know it's relevant, perhaps you want to add a test in bug 333089? Can you demonstrate relevance for 2) ?
last comment for now: (In reply to comment #20) > The problem pointed out in comment 13 has been fixed by fixing > ReferenceBinding#variableBinding(). I didn't see any need to touch checkNPE() > here. We should just make sure that in cases like getA().val, where val is not > a static field, we should not return the field binding to be analysed since we > dont know what object the field belong to. As long as FieldBinding#variableBinding() is only used for null-analysis this is OK. If other uses occur we will be surprised to get null for non-static fields. I played a bit more with combinations of different references to the same static field by adding these lines to testBug247564f(): " if (X.o1 == null && getX().o1.hashCode() == 0) return;\n" + " if (X.o1 == null && this.o1.hashCode() == 0) return;\n" + " if (this.o1 == null && getX().o1.hashCode() == 0) return;\n" + " if (this.o1 == null && X.o1.hashCode() == 0) return;\n" + " if (X.o1 == null && X.o1.hashCode() == 0) return;\n" + I was surprised to see that none of them triggers another error, where I'd expect all of them to do so. Is that intended? This partly contradicts my comment 13, but strictly speaking comment 13 might only apply to non-static fields. So how should we handle qualified references to static fields? I mean, getX().o1 and this.o1 are deprecated any way (non-static access to static field), but X.o1 is perfectly OK. Apart from the findings mentioned here and in previous comments the patch looks good! For a final +1 I'd like to first read your comments re comment 25 and this one.
Sorry for the flood of comments, but ... (In reply to comment #26) > [...] So how should we handle qualified > references to static fields? I mean, getX().o1 and this.o1 are deprecated > any way (non-static access to static field), but X.o1 is perfectly OK. This was wishful thinking: conceptually these might be sound arguments, but technically we cannot use FlowInfo to check any fields that don't belong to the current type. So this.o1 can be handled but both getX().o1 and X.o1 will get us into trouble, confusing field ids among classes. Here's a test that reports a problem where it shouldn't: public class X { static Object o; Y getY() { return new Y(); } void foo() { o = new X(); if (getY().y == null && this.o.hashCode() == 0) return; } } public class Y { static Object y; } 1. ERROR in X.java (at line 11) if (getY().y == null && this.o.hashCode() == 0) return; ^ Potential null pointer access: The field o may be null at this location
(In reply to comment #25) > While challenging the patch I found two changes that I could safely revert > without breaking any of NullReferenceTest: > 1) the change from bug 333089 (now at UnconditionalFlowInfo:1535-1538) > 2) within NullInfoRegistry the three new overrides markPotentiallyNullBit ff. > > Are any of these covered by tests in another test class? > For 1) we know it's relevant, perhaps you want to add a test in bug 333089? > Can you demonstrate relevance for 2) ? I'll try to add a test case for (1). For 2), you're right. I'd added it initially since I was adding it for UnconditionalFlowInfo. But NullInfoRegistry doesnt really need it. > (In reply to comment #26) > This was wishful thinking: conceptually these might be sound arguments, > but technically we cannot use FlowInfo to check any fields that don't > belong to the current type. So this.o1 can be handled but both getX().o1 > and X.o1 will get us into trouble, confusing field ids among classes. I had intentionally left qualified reference to be handled because of confusion of same ids. But... > Here's a test that reports a problem where it shouldn't: > > public class X { > static Object o; > Y getY() { return new Y(); } > void foo() { > o = new X(); > if (getY().y == null && this.o.hashCode() == 0) return; > } > } > public class Y { > static Object y; > } > > 1. ERROR in X.java (at line 11) > if (getY().y == null && this.o.hashCode() == 0) return; > ^ > Potential null pointer access: The field o may be null at this location ... this testcase shows that even method calls should be left out!
(In reply to comment #28) > ... this testcase shows that even method calls should be left out! ... unless you check that the receiver of the static field access is indeed the current type. Is it worth it? BTW: are fields of enclosing types included in the analysis? (sorry, I'm too lazy to check in the implementation right now ...)
(In reply to comment #29) > (In reply to comment #28) > > ... this testcase shows that even method calls should be left out! > > ... unless you check that the receiver of the static field access is > indeed the current type. Is it worth it? Yes, i was currently testing this as you were writing this comment. :) > BTW: are fields of enclosing types included in the analysis? > (sorry, I'm too lazy to check in the implementation right now ...) I think this may have some bugs, since again the ids might conflict. Let me check on this.
(In reply to comment #30) > > BTW: are fields of enclosing types included in the analysis? > > (sorry, I'm too lazy to check in the implementation right now ...) > > I think this may have some bugs, since again the ids might conflict. Let me > check on this. So turns out that this does have a problem becoz of conflicting id's. I attempted to fix this by making sure we continue the id's of fields of enclosed types from the no. of fields in the enclosing types. But even that backfired. Consider this: class A { int field0; int field1; class B { int field2; Object field3; void m() { String var = null; if (field3.toString() = ""){} } } Here with the new approach the field id's will be exactly what they are in the name suffix. But id of 'var' will be local.id + B's maxFieldCount, which is 3. This is same as field3's id. So, to follow this approach we will have to change the calculation of id's of local variables in the inner classes. :(
(In reply to comment #31) > So turns out that this does have a problem becoz of conflicting id's. That's a pity. > Consider this: > > class A { > int field0; > int field1; > class B { > int field2; > Object field3; > void m() { > String var = null; > if (field3.toString() = ""){} > } > } > > Here with the new approach the field id's will be exactly what they are in the > name suffix. But id of 'var' will be local.id + B's maxFieldCount, which is 3. > This is same as field3's id. So, to follow this approach we will have to change > the calculation of id's of local variables in the inner classes. :( Have you looked at TypeDeclaration.updateMaxFieldCount() ? Is the conflict caused by the fact that updateMaxFieldCount isn't called before analyseCode? Also, updateMaxFieldCount computes the max instead of summing up. I would naively think that traversals up the type hierarchy and out the class nesting should be combined in one location to get unique ids for all fields in scope. On a similar note I just played with the following test case and was positively surprised it already prints the expected error :) public class Outer { void outerMethod() { final Object local = null; new Runnable() { public void run() { Object dummy = new Object(); if (local != null) System.out.println(local.toString()); } }.run(); } } gives: if (local != null) ^^^^^ Null comparison always yields false: The variable local can only be null at this location So locals can already be analyzed across nesting levels, good. Do you remember if we already have a test case for such a scenario, or should I add this to the suite?
Created attachment 186938 [details] proposed fix v1.2 + regression tests This fix attempts to fix both issues with member types - that of the field id's being the same (this is done through the new field ClassScope.cumulativeFieldCount), and also the id's of locals and fields being same (by modifying the TypeDeclaration.analyseCode() and its overriden implementations to make sure the maxFieldCount field of flowInfo shows a cumulative no. of fields). Though the above issues are fixed, the patch gives weird errors in InitializationTests . So, still needs to be worked out.
Created attachment 187094 [details] fix v 1.2 updated for HEAD Updated above patch for HEAD. Stephan, would you have any solution to the member types' local variable id's conflict issue?
(In reply to comment #34) > Created attachment 187094 [details] > fix v 1.2 updated for HEAD > > Updated above patch for HEAD. > > Stephan, would you have any solution to the member types' local variable id's > conflict issue? I'm not quite up-to-date as to what conflict still occurs. Is it a new conflict between local variables at different levels of type nesting? (this used to work, see example in comment 32). Which test shows the problem?
(In reply to comment #35) > > Which test shows the problem? comment 31 shows a case where there was a conflict between locals and fields of outer type. The new patch tries to handle this using a cumulative count, as elucidated in comment 33, but there are still other cases that fail, such as the tests in InitializationTests.java.
Created attachment 188023 [details] proposed fix v1.2.1 + regression tests This is mostly the same fix as above, except for how it builds the field id's. It takes a cumulative count of all fields in supertypes, superinterfaces and enclosing types and then assigns id's to the current types' fields. (in TypeDeclaration.resolve() and ClassScope.buildFields()) This solves all problems reported above, but some tests now fail in EnumTests.java. This might be due to the way field id's are updated in TypeDeclaration.resolve(), but I havent investigated in depth. (Changes in BlockScope.java can be ignored.)
Hi Ayush, I'm back from the intermission ... (In reply to comment #37) > This solves all problems reported above, but some tests now fail in > EnumTests.java. This might be due to the way field id's are updated in > TypeDeclaration.resolve(), but I havent investigated in depth. Your guess is quite right: code generation assumes that the fields representing the enum constants are numbered starting from 0. I found two locations: Clinit.generateCode(ClassScope, ClassFile, int) calls: codeStream.generateInlinedValue(fieldDecl.binding.id) and with raised ids this points beyond the array it is supposed to index. Second: CodeStream.generateSyntheticBodyForSwitchTable(..) similar issue. It might be possible to fix this by subtracting scope.localTypeFieldIdStart but that field doesn't seem to be maintained consistently. I assume it doesn't refer to "local types" but to the current type, right? OTOH, I would feel more comfortable if the field.id is actually left in its original range and if you add the currentTypeFieldIdStart only when performing lookup in a flow info. Would that be possible? It seems we would need to store the minFieldId right next to the maxFieldCount inside each FlowInfo, which looks natural to me. Wait, that would have to be one id per nesting level, right? Hm... Anyway, I plan to use this as a basis for null annotations on fields. I might have a comment on how this will fit before the end of the week.
(In reply to comment #38) > Second: > CodeStream.generateSyntheticBodyForSwitchTable(..) > similar issue. > > It might be possible to fix this by subtracting scope.localTypeFieldIdStart > but that field doesn't seem to be maintained consistently. Bad news for this approach: when generating the SWITCH_TABLE method we don't have the scope of the enum. Hence we don't know what offset to subtract. Sorry, that's a dead end. Hope the other idea works better .. Otherwise we might need to store both ids in the FieldBinding: one relative to the current type and a second one that's unique over all types in scope. Or, here's yet another one: why not store the offset in the ReferenceBinding instead of the scope? This would always be accessible as f.declaringClass. Field id could always start from 0, only FlowInfo would add the offset found in the declaring class. That should be the cleanest solution, no?
New IProblems are new API, right? So, it would have to happen before M6 or after 3.7, right? Ayush, let me know if you'd like some help here.
(In reply to comment #40) > New IProblems are new API, right? So, it would have to happen before M6 > or after 3.7, right? > > Ayush, let me know if you'd like some help here. I think we're too late already for M6. I'd have liked to release it in time so that others can test it and we can fix major problems, if any. Releasing it too close to the M week will not make that possible. So lets keep working on this and have a patch ready, which we can put into 3.8M1.
Just a quick reminder that a (small) part of this patch has already been released in bug 341499.
Created attachment 206514 [details] proposed fix v1.2.1 updated for HEAD Same patch as in comment 37, updated for HEAD. The enum constant issue remains to be fixed.
Created attachment 206570 [details] same patch rebased on top of bug 186342 patch v9 In order to test compatibility of this patch with the latest in bug 186342 I rebased this one on top of the other one. After quite some (E)Git dance I got it merged and NullReferenceTests and NullAnnotationTests are both green, so both approaches seem to be compatible. Good! I will soon look into the remaining issues of this bug, and also start experimenting with null annotations for fields.
Created attachment 206572 [details] same patch rebased on top of bug 186342 patch v9 (corrected) Ups, previous patch was incomplete, this one should be better.
Ayush, while I'm smoothing out the rough edges of the patch here's one question to you: in TypeDeclaration.resolve() you use - superClassBinding.unResolvedFields() but then in findFieldCountFromSuperInterfaces(): - superinterfaces[i].fieldCount() which calls fields(), not unResolvedFields Did you see specific reasons for using unResolvedFields in the first case or was that just general caution? Put differently, is fields() another one of those candidates that could trigger an error due to missing indirectly referenced type? Should we strictly avoid triggering resolveTypeFor(FieldBinding) for any super type unless we really need the field with its type resolved?
(In reply to comment #46) > Ayush, while I'm smoothing out the rough edges of the patch here's one > question to you: > > in TypeDeclaration.resolve() you use > - superClassBinding.unResolvedFields() > but then in findFieldCountFromSuperInterfaces(): > - superinterfaces[i].fieldCount() which calls fields(), not unResolvedFields Looks like an oversight to me. There's no reason why fields should be resolved here. :)
Created attachment 207019 [details] improved handling of field ids (relative patch) Here's a sketch for fixing the regressions in EnumTests plus some cleanup: I suggest to revert the initialization of FieldBinding.id to the previous state (those changes caused the trouble regarding enum constants), but add the additional offset only during analyse, for which I introduced a method VariableBinding.getAnalysisId(int), with an override in FieldBinding. Also I improved the interplay of the various field counts: - ClassScope.localTypeFieldIdStart: was never read, removed - ClassScope.cumulativeFieldCount: Moved to SourceTypeBinding as suggested in comment 39 last para. With this change getAnalysisId() has access to this offset. - TypeDeclaration.maxFieldCount: changed initialization strategy. The previous patch had a mix of adding up all field counts (cumulative..) and computing a max (maxFieldCount) which didn't quite work. Originally, maxFieldCount was propagated outside-in during resolve and propagated back out during analyse. With this patch instead of computing a max during resolve, we *add up* all fields from outer and supers while going into nested types (computation happens in cumulatedFieldCount) and at each type we copy the result into maxFieldCount. Later during analyse we use the old updateMaxFieldCount() for propagating the max inside-out, but now its actually the max of cumulative counts. With these changes I saw all tests pass. I will provide a full updated patch after bug 186342 has been released.
I've pushed a feature branch "NullAnalysisForFields" http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/log/?h=NullAnalysisForFields starting from current HEAD it so far contains two commits: - Ayush's patch v1.2.1 originally from comment 43 - my suggested changes from comment 48 Ayush, whenever you resume work on this bug let me know what you think of the strategy proposed in comment 48. Once we agree on the general approach I'll give it another once-over. First experiments in bug 331649 showed that this patch is well-suited as a basis for that bug.
(In reply to comment #22) > (In reply to comment #21) > > Stephan, will you be able to review this for M5? > > Yes, sure. Stephan, where are we with this review request ? Are we in reasonable shape to push this out for M5 ? I can also spend some cycles reviewing and testing this but after it has gone through some amount of review/validation already.
(In reply to comment #50) > Stephan, where are we with this review request ? Are we in reasonable shape > to push this out for M5 ? I can also spend some cycles reviewing and testing > this but after it has gone through some amount of review/validation already. I think we're well on track for M5. I'm waiting for a handshake with Ayush regarding the changes I proposed in the feature branch (see comment 49). My latest version in that branch passes all tests. I was still going to give it another once-over, but I'm not expecting serious surprises, just details and polish items. In fact I'd appreciate if we could get this released within - say - a week so I can start working seriously on bug 331649.
(In reply to comment #48) > Also I improved the interplay of the various field counts: > - ClassScope.localTypeFieldIdStart: was never read, removed > - ClassScope.cumulativeFieldCount: > Moved to SourceTypeBinding as suggested in comment 39 last para. > With this change getAnalysisId() has access to this offset. > - TypeDeclaration.maxFieldCount: changed initialization strategy. Thanks Stephan. These changes look good and also simplify the logic a bit. The whole fix is quite small actually. :) I think we can proceed to test the patch.
(In reply to comment #52) > Thanks Stephan. These changes look good and also simplify the logic a bit. The > whole fix is quite small actually. :) > I think we can proceed to test the patch. Is there some UI work involved for this ? If yes, has a bug been raised for it with a sketch of the expectations ?
(In reply to comment #53) > Is there some UI work involved for this ? If yes, has a bug been raised for it > with a sketch of the expectations ? In the current implementation there's nothing to configure. We could potentially make it configurable in two independent ways: - [ ] include fields in null analysis Or: - Report null field access [E/W/I] - Report potential null field access [E/W/I] - Report redundant null-check for field [E/W/I] - more? I don't have a clear preference, except, that with null annotations enabled we'd want all results from analyzing fields, too.
(In reply to comment #54) > We could potentially make it configurable in two independent ways: > - [ ] include fields in null analysis > Or: > - Report null field access [E/W/I] > - Report potential null field access [E/W/I] > - Report redundant null-check for field [E/W/I] > - more? Given this functionality is targetted for for the latter half of the 3.8 release, perhaps it makes sense to have the eager beavers opt in. Markus, what do you think ?
After we've agreed on the strategy for assigning ids for fields I'm resuming the review. First, some left-overs from previous comments: (In reply to comment #26) > (In reply to comment #20) > > The problem pointed out in comment 13 has been fixed by fixing > > ReferenceBinding#variableBinding(). I didn't see any need to touch checkNPE() > > here. We should just make sure that in cases like getA().val, where val is not > > a static field, we should not return the field binding to be analysed since we > > dont know what object the field belong to. > > As long as FieldBinding#variableBinding() is only used for null-analysis > this is OK. If other uses occur we will be surprised to get null for > non-static fields. I guess we both meant Expression#variableBinding() and its override in FieldReference. Anyway, could you please add a note in the javadoc explaining the case of static fields? (In reply to comment #47) > (In reply to comment #46) > > Ayush, while I'm smoothing out the rough edges of the patch here's one > > question to you: > > > > in TypeDeclaration.resolve() you use > > - superClassBinding.unResolvedFields() > > but then in findFieldCountFromSuperInterfaces(): > > - superinterfaces[i].fieldCount() which calls fields(), not unResolvedFields > > Looks like an oversight to me. There's no reason why fields should be resolved > here. :) This is still a TODO, right? See also the FIXME comments in the code.
Comments concerning the current HEAD in the feature branch: (1) please make sure the meaningless change in CodeSnippetClassFile doesn't make it into the final patch (could be my bad) (2) IIRC you intentionally deferred some renamings (e.g., local -> var) to avoid clutter in the patch, right? As an example we have lots of variables "VariableBinding local" in the flow package now. (3) please add a comment inside UnconditionalFlowInfo#markAsComparedEqualToNonNull: why are fields treated differently here? (I believe it's correct but we may not remember in a few years :) ) (+similar cases?) (4) would we want to get rid of the overloading re FlowInfo#markAsDefinitelyAssigned ? Should be trivial using the new VariableBinding#getAnalysisId() (+similar cases?) (5) Caveat: if bug 358903 is released before this one, we will have to update a few occurrences of "local != null" from that patch, which will need to be changed to "var instanceof LocalVariableBinding". (6) I believe the code in FieldReference#variableBinding() is intended to implement comment 30 (first part)? I don't think it correctly captures the "the current type" part? Do you see what I mean or should I (try to) construct a counter example? (7) I couldn't immediately see the motivation for the change in TypeDeclaration#internalAnalyseCode(..) Could you please educate me? :)
(In reply to comment #55) > > - [ ] include fields in null analysis I think this one new preference is enough for now. It should be disabled by default, until we're confident about the implementation. I checked out the NullAnalysisForFields branch, but to be honest, I don't quite understand which features are already implemented here. Is this only about local analysis inside a single method body, similar to what we already have for local variables, but only emitting "potential" warnings because you can never be sure when a field is changed externally?
Here's a test that breaks the current patch: Compile JDT/Core at compliance 1.6+ and you'll see an IAE in StackMapFrame.addStackItem(..) I could narrow it down to being caused by inconsistent maxFieldCounts in the case of multiple sibling inner classes. The effect of the bug is through CodeStream.isDefinitelyAssigned which mistakenly answers false, due to the use of a too large this.maxFieldCount (ask me for the chain of consequences if you really care). The current patch only ensures that counts are consistent comparing outer<->inner, but not between siblings. For illustration: Outer f1 f2 Inner1 -> maxFieldCount = 3 f3 Inner2 -> maxFieldCount = 5 f4 f5 f6 While resolving Inner1 we set cumulativeFieldCount to 3, transfer this to maxFieldCount (Inner1 and Outer) and things look fine. When resolving Inner2 we compute a cumulativeFieldCount of 5, propagate a maxFieldCount of 5 back up, which is now inconsistent with the 3 in Inner1. This becomes relevant when during analysis flowInfos are constructed using the maxFieldInfo of each current type, whereas later during codegen, CodeStream gets the maxFieldInfo from the outermost type. These counts MUST MATCH. At a quick glance it seems to suffice to use the same initialization for both maxFieldCounts fields. In ClassFile we find: this.codeStream.maxFieldCount = aType.scope.outerMostClassScope().referenceType().maxFieldCount; using this pattern also in TypeDeclaration#analyseCode(ClassScope) like internalAnalyseCode(null, FlowInfo.initial(this.scope.outerMostClassScope().referenceType().maxFieldCount)); fixes the immediate regression and passes the most relevant tests. We could try to restore the original down-and-up propagation for maxFieldCount, but once we better understand *why* the old solution worked, and how exactly our change broke that, we could more comfortably just let it go and use the miniature patch above. I'll also create a regression test for this once I find the time. Update: all compiler.regression.tests pass with the above change!
(In reply to comment #58) > (In reply to comment #55) > > > - [ ] include fields in null analysis > > I think this one new preference is enough for now. It should be disabled by > default, until we're confident about the implementation. That's fine with me. I'd just suggest that this preference is automatically enabled when null annotations are enabled. Would this look awkward in the UI? > I checked out the NullAnalysisForFields branch, but to be honest, I don't quite > understand which features are already implemented here. Is this only about > local analysis inside a single method body, similar to what we already have for > local variables, but only emitting "potential" warnings because you can never > be sure when a field is changed externally? This is one side, and it matches the original request in comment 0. The other is final static fields for which null status is used globally. The latter improvement will help to avoid many false positives when using null annotations. Without annotations on fields we can't do much more. But then bug 331649 is actually next on my agenda :)
(In reply to comment #59) > At a quick glance it seems to suffice to use the same initialization > for both maxFieldCounts fields. In ClassFile we find: > > this.codeStream.maxFieldCount = > aType.scope.outerMostClassScope().referenceType().maxFieldCount; > > using this pattern also in TypeDeclaration#analyseCode(ClassScope) like > > internalAnalyseCode(null, > FlowInfo.initial(this.scope.outerMostClassScope().referenceType().maxFieldCount)); > > fixes the immediate regression and passes the most relevant tests. Update: doing this adjustment the other way around yields simpler code and also passes compiler.regression.tests: instead of making TypeDeclaration more complicated we can make ClassFile simpler by saying: this.codeStream.maxFieldCount = aType.scope.referenceContext.maxFieldCount; (This also has the advantage that we are using smaller indices in a few cases, meaning we can longer avoid using extraInits).
(In reply to comment #60) > This is one side, and it matches the original request in comment 0. > The other is final static fields for which null status is used globally. > The latter improvement will help to avoid many false positives when using > null annotations. Yup, so we again have two warnings - pot. null ref and null ref., with the latter being emitted only for static final fields. Also, we discard null status as soon as another method is called, as per comment 8.
(In reply to comment #57) > Comments concerning the current HEAD in the feature branch: I'll look at these soon. One thing i noticed was that somehow in the fix v1.2.1 which was released in the branch changed the reach mode from UNREACHABLE_OR_DEAD to UNREACHABLE in TypeDeclaration. This change happened when you rebased the patch on top of fix for bug 186342. Is that required, given that at those places the unreachability is not really because of null analysis?
(In reply to comment #63) > I'll look at these soon. One thing i noticed was that somehow in the fix v1.2.1 > which was released in the branch changed the reach mode from > UNREACHABLE_OR_DEAD to UNREACHABLE in TypeDeclaration. This change happened > when you rebased the patch on top of fix for bug 186342. Is that required, > given that at those places the unreachability is not really because of null > analysis? Oops. Isn't this your point (7) in comment 57? Ah! So you're the culprit for the change. ;)
I had another close look at the issue I reported in comment 59 and pushed my results to the feature branch (after merging it with master). In FlowAnalysisTest.testInnerClassesWithFields1() you'll find a witness. I first tried the simpler fix (comment 61) but then the following argumentation led me to a witness showing why that is wrong: INVARIANT 1: in any context UnconditionalFlowInfo.maxFieldCount and CodeStream.maxFieldCount must be the same, because both communicate with MethodScope.definiteInits. INVARIANT 2: the analysisIndex of a local variable must never collide with the analysisIndex of any field in scope. No consistency is, however, required between sibling inner classes. I.e., a local variable in Inner1 (see comment 59) may well have the same analysis index as a field in Inner2, because no analysis information can flow from one context to the other. When crossing type boundaries, flow info regarding fields and locals is only passed outside->in, nothing is passed back. This would imply that during processing of Inner1 the local maxFieldCount of 3 is sufficient, because all directly accessible fields are within this boundary. So, no access of outerMostClassScope() were necessary. This holds true for member classes, BUT local classes also need to avoid conflicts with local variables from their declaring method. Therefore, local classes directly receive the flowInfo from the enclosing method thus sharing the maxFieldCount of their enclosing class, whereas the code stream is initialized the same way as for member types. With the simpler fix this would mean: the maxFieldCount of the member itself. -> This breaks INVARIANT 1 above (inconsistenly using maxFieldCount of enclosing and member). Witness is in testInnerClassesWithFields1a (to be pushed in a minute) which with the simpler fix throws IAE just like the initial issue. This leaves us with the slightly more involved fix: always using the outermost maxFieldCount. Another issue is addressed by the commit in the feature branch, that occurred while compiling the Eclise SDK with the patch. See testInnerClassesWithFields2. The point here is simply that checking for "instanceof NestedTypeBinding" is not sufficient in case we have a parameterized inner class. Search for "instanceof NestedTypeBinding" found another occurrence also introduced in the current patch. Both occurrences are corrected by adding a call to original(). (Attempt to use enclosingType() and check for STB failed since enclosingType() may, e.g., return a RawTypeBinding of a member type).
I was thinking that maybe in place of completely rejecting the null info when a method call is detected, we should look at the receiver of the method. Only when the receiver is 'this', can it actually change the value of a non-static field. So, perhaps when the method is called from a receiver other than 'this', we should discard null info for only static fields
(In reply to comment #66) > I was thinking that maybe in place of completely rejecting the null info when a > method call is detected, we should look at the receiver of the method. Only > when the receiver is 'this', can it actually change the value of a non-static > field. So, perhaps when the method is called from a receiver other than 'this', > we should discard null info for only static fields The canonical treatment in compilers is to say all bets are off when a function call is encountered. The receiver of a particular call you are looking at could be one thing, but that method may in turn call other methods some of which retrieve various objects from assorted collections, pass them around and mutate them in ways that cannot be forseen by the compiler. Some C/C++ compilation systems rely on programmer supplied annotations (called "pragma's") which document that certain methods are "side effect free" and can be considered to be pure functions in a mathematical sense. In the absence of such annotations, assuming anything but the worst is dangerous.
(In reply to comment #67) > In the absence of such annotations, assuming anything but the worst > is dangerous. Yeah I should've thought more before writing that comment. I was just trying to test the patch and almost in all cases didn't really get any new warnings, because method calls are common. I guess the real power of this enhancement will come only with Null annotations. Without those, its not very useful.
(In reply to comment #57) > (3) please add a comment inside > UnconditionalFlowInfo#markAsComparedEqualToNonNull: > why are fields treated differently here? > (I believe it's correct but we may not remember in a few years :) ) > (+similar cases?) Treated differently? Sorry I didn't understand this part. :\ > (7) I couldn't immediately see the motivation for the change in > TypeDeclaration#internalAnalyseCode(..) > Could you please educate me? :) As mentioned in comment 64, this got introduced during rebasing of the patch. I think this should be reverted. Let me know what you think.
(In reply to comment #67) > (In reply to comment #66) > > I was thinking that maybe in place of completely rejecting the null info when a > > method call is detected, we should look at the receiver of the method. Only > > when the receiver is 'this', can it actually change the value of a non-static > > field. So, perhaps when the method is called from a receiver other than 'this', > > we should discard null info for only static fields > > The canonical treatment in compilers is to say all bets are off > when a function call is encountered. The receiver of a particular call > you are looking at could be one thing, but that method may in turn > call other methods some of which retrieve various objects from assorted > collections, pass them around and mutate them in ways that cannot be > forseen by the compiler. When I was still teaching I used the term "boomerang" for method calls that directly or indirectly come back to the current object. A boomerang is that thing that may come back and hit you at the head :-P (In reply to comment #68) > (In reply to comment #67) > > In the absence of such annotations, assuming anything but the worst > > is dangerous. > > Yeah I should've thought more before writing that comment. I was just trying to > test the patch and almost in all cases didn't really get any new warnings, > because method calls are common. I guess the real power of this enhancement > will come only with Null annotations. Without those, its not very useful. Well, knowing the null status of static finals is very valuable. And yes, once we have null annotations for fields marking a field as @Nullabel will reverse the situation: you get warnings for every deref unless you say s.t. like if (f != null) f.bar(); And that will be understand by the current patch, right?
(In reply to comment #69) > (In reply to comment #57) > > (3) please add a comment inside > > UnconditionalFlowInfo#markAsComparedEqualToNonNull: > > why are fields treated differently here? > > (I believe it's correct but we may not remember in a few years :) ) > > (+similar cases?) > Treated differently? Sorry I didn't understand this part. :\ I'm looking at this: @@ -1008,15 +1087,21 @@ protected static boolean isTrue(boolean expression, String message) { throw new AssertionFailedException("assertion failed: " + message); //$NON-NLS-1$ return expression; } -public void markAsComparedEqualToNonNull(LocalVariableBinding local) { +public void markAsComparedEqualToNonNull(VariableBinding local) { // protected from non-object locals in calling methods if (this != DEAD_END) { this.tagBits |= NULL_FLAG_MASK; int position; + if (local instanceof FieldBinding) { + this.markNullStatus(local, FlowInfo.POTENTIALLY_NON_NULL); + return; + } else { + position = local.id + this.maxFieldCount; + } In similar methods fields and locals are only distinguished in how the id is computed. But here we do completely different stuff. I'd love to see a comment why that is so. > > (7) I couldn't immediately see the motivation for the change in > > TypeDeclaration#internalAnalyseCode(..) > > Could you please educate me? :) > > As mentioned in comment 64, this got introduced during rebasing of the patch. I > think this should be reverted. Let me know what you think. I don't exactly recall who did the change: I, me or myself. But I'll ask him anyway.
(In reply to comment #71) > + if (local instanceof FieldBinding) { > + this.markNullStatus(local, FlowInfo.POTENTIALLY_NON_NULL); > + return; I see. Sorry I was so stuck on the obviousness of marking potentially null instead of surely null that I thought you're referring to something else. Will add a comment.
(In reply to comment #57) > (6) I believe the code in FieldReference#variableBinding() is intended > to implement comment 30 (first part)? I don't think it correctly > captures the "the current type" part? Do you see what I mean or > should I (try to) construct a counter example? While I was left scratching my head as to why I even wrote this code, I see that FieldRef.variableBinding() has no info to conclude whether the static field is indeed coming from the current type. We may add a parameter to variableBinding to capture the current scope, to be able to conclude that part.
(In reply to comment #73) > While I was left scratching my head as to why I even wrote this code, I see > that FieldRef.variableBinding() has no info to conclude whether the static > field is indeed coming from the current type. We may add a parameter to > variableBinding to capture the current scope, to be able to conclude that part. Released fix into the branch. Will release tests separately.
(In reply to comment #13) > QualifiedNameReference explicitly tests for bit Binding.LOCAL, > - will not handle fields, good. We should handle fields in QNR, if the field is static, and if it belongs to the "current type". Coming up in the next batch of fixes. This will suport cases such as: class Test{ String field1; void foo() { if (X.field1 == null && X.field1.hashCode() == 0) return; ^^^^^^ } }
At some point after the rough edges are ironed out but before the release of the patch, we should ensure that all new decorations, attribute propagation, analysis etc happen under a switch that is turned on/off by a preference. This would also ensure that out of the box there will not be any performance degradation and any penalty is incurred only by opting in for extra analysis.
(In reply to comment #75) > (In reply to comment #13) > > QualifiedNameReference explicitly tests for bit Binding.LOCAL, > > - will not handle fields, good. > We should handle fields in QNR, if the field is static, and if it belongs to > the "current type". Coming up in the next batch of fixes. > This will suport cases such as: My latest commit has - fix for the above - regression tests to cover comment 73 and comment 75. - doc for comment 57 point (3)
(In reply to comment #76) > At some point after the rough edges are ironed out but before the release > of the patch, we should ensure that all new decorations, attribute > propagation, analysis etc happen under a switch that is turned on/off > by a preference. I would say that in this case we should only stop the warning when the option is off. There are 4 fundamental changes in this bug: - Calculation of field id's to have unique id's for all fields: Switching this off according the option is not relevant. - Marking nullness information in the flow info bits: These are usually bit operations and are quite cheap. Inserting checks to see if the option is enabled or not will only degrade performance. - Looking into the bit stream to read the null status: This is done in a general way to tie it to the existing implementation, and lookups are usually cheap. So, again I don't see much need of doing this only when option is on. - Reporting the null problem: This is the place we should check for the option and bail out if the option is off.
(In reply to comment #78) > - Marking nullness information in the flow info bit > - Looking into the bit stream to read the null status > - Reporting the null problem On second thoughts, all these depend on obtaining a field binding via Expression#variableBinding(). So a check in its implementations should bypass all these stages!
A test case that gives a bogus error because of the fix of comment 48 public class Z { final int field1 = 0; { class ZInner { final int fieldz1; final int fieldz2 = 0; } } } Need to investigate why this fails.
Added new compiler for option to toggle inclusion of fields in null analysis + comment 79 + batch compiler option + tests via commit 3c882459f49f009bb37aae8b76fcd4da5135425f
Created attachment 209475 [details] doc patch Contains doc for compiler pref and batch compiler option.
(In reply to comment #56) > This is still a TODO, right? See also the FIXME comments in the code. Fixed via commit 2209e101fc6c84e4b5da48d3a7d53674159e1c98
Hi, I'm back from bug 358903 :) (In reply to comment #73) > (In reply to comment #57) > > (6) I believe the code in FieldReference#variableBinding() is intended > > to implement comment 30 (first part)? I don't think it correctly > > captures the "the current type" part? Do you see what I mean or > > should I (try to) construct a counter example? > While I was left scratching my head as to why I even wrote this code, I see > that FieldRef.variableBinding() has no info to conclude whether the static > field is indeed coming from the current type. We may add a parameter to > variableBinding to capture the current scope, to be able to conclude that part. Solution looks good. (In reply to comment #75) > (In reply to comment #13) > > QualifiedNameReference explicitly tests for bit Binding.LOCAL, > > - will not handle fields, good. > We should handle fields in QNR, if the field is static, and if it belongs to > the "current type". Coming up in the next batch of fixes. > This will suport cases such as: > > class Test{ > String field1; > void foo() { > if (X.field1 == null && X.field1.hashCode() == 0) return; > ^^^^^^ > } > } OK. Looking at QNR#variableBinding() I'm wondering if the use of TypeDeclaration#declarationOf(..) should be avoided (performance). Why not compare fieldBinding.declaringClass with type.binding? You may need some original() calls, but you could avoid iterating through all fields per class, no?
Nit-pick re FieldReference#variableBinding(): Why call super.variableBinding() ?? Just return null ! (If super were relevant, early exits above would need to respect this, too). And somewhere around this method I'd like to see documented that this is not a general purpose accessor, but works from the POV of null analaysis. Encode it in the method name or put in the javadoc in Expression.
(In reply to comment #83) > (In reply to comment #56) > > This is still a TODO, right? See also the FIXME comments in the code. > Fixed via commit 2209e101fc6c84e4b5da48d3a7d53674159e1c98 OK, from all my code comments in that region this leaves only one unresolved: // 2.: consistently check field.kind() (also in findFieldCountFromSuperInterfaces())? Frankly, I don't understand why kind() is used anyway. Once that is resolved, please clean-up stale code comments.
(In reply to comment #80) > A test case that gives a bogus error because of the fix of comment 48 > > public class Z { > final int field1 = 0; > { > class ZInner { > final int fieldz1; > final int fieldz2 = 0; > } > } > } May be happening because org.eclipse.jdt.internal.compiler.flow.UnconditionalFlowInfo.discardNonFieldInitializations() is discarding too much info
(In reply to comment #87) > (In reply to comment #80) > > A test case that gives a bogus error because of the fix of comment 48 > > > > public class Z { > > final int field1 = 0; > > { > > class ZInner { > > final int fieldz1; > > final int fieldz2 = 0; > > } > > } > > } > > May be happening because > org.eclipse.jdt.internal.compiler.flow.UnconditionalFlowInfo.discardNonFieldInitializations() > is discarding too much info No, the type defined in the initializer is not counted into maxFieldCount. I should have a patch in a minute.
(In reply to comment #88) > No, the type defined in the initializer is not counted into maxFieldCount. > I should have a patch in a minute. Yes the maxFieldCount is wrong because TypeDeclaration:1124 overwrites the action of updateMaxFieldCount() that happened during resolution of the initializer.
(In reply to comment #89) > (In reply to comment #88) > > No, the type defined in the initializer is not counted into maxFieldCount. > > I should have a patch in a minute. > > Yes the maxFieldCount is wrong because TypeDeclaration:1124 overwrites the > action of updateMaxFieldCount() that happened during resolution of the > initializer. Yep, we're on the same page. For the immediate case doing a max(..) on said line suffices, but for a full solution we may have to dig deeper: Member types are carefully resolved *after* fields have been resolved so we can build our cumulativeFieldCount. However, a type defined in an initializer cuts the line. Not nice.
(In reply to comment #90) > Yep, we're on the same page. For the immediate case doing a max(..) on > said line suffices, but for a full solution we may have to dig deeper: > Member types are carefully resolved *after* fields have been resolved > so we can build our cumulativeFieldCount. However, a type defined > in an initializer cuts the line. Not nice. I'm a bit confused, or maybe its too late in the night - I'm thinking that the cumulativeCount for Z should become 3 at line 1124 after the initializer has been resolved, no? <rubbing eyes> :P
Created attachment 209486 [details] fixlet for local class in initializer (In reply to comment #91) > (In reply to comment #90) > > Yep, we're on the same page. For the immediate case doing a max(..) on > > said line suffices, but for a full solution we may have to dig deeper: > > Member types are carefully resolved *after* fields have been resolved > > so we can build our cumulativeFieldCount. However, a type defined > > in an initializer cuts the line. Not nice. > > I'm a bit confused, or maybe its too late in the night - I'm thinking that the > cumulativeCount for Z should become 3 at line 1124 after the initializer has > been resolved, no? <rubbing eyes> :P Sorry, my fingers were quicker than my mind. All seems to be simple: just move up the assignment to maxFieldCount and all (should) be well. NullReferenceTests are fine.
(In reply to comment #92) > Created attachment 209486 [details] > fixlet for local class in initializer > > (In reply to comment #91) > > (In reply to comment #90) > > > Yep, we're on the same page. For the immediate case doing a max(..) on > > > said line suffices, but for a full solution we may have to dig deeper: > > > Member types are carefully resolved *after* fields have been resolved > > > so we can build our cumulativeFieldCount. However, a type defined > > > in an initializer cuts the line. Not nice. > > > > I'm a bit confused, or maybe its too late in the night - I'm thinking that the > > cumulativeCount for Z should become 3 at line 1124 after the initializer has > > been resolved, no? <rubbing eyes> :P > > Sorry, my fingers were quicker than my mind. > > All seems to be simple: just move up the assignment to maxFieldCount > and all (should) be well. NullReferenceTests are fine. I pushed as commit 055478d72ad477a3e7cd23123dc801e665064bbc what I believe to be still better: re-establish the original order of resolving member types and fields, just push up the maxFieldCount assignment. NullReferenceTests pass, compiling the Eclise SDK works to, let's see what the rest of the test suite has to say. (I also did half of the comment-cleanup I previously requested :) )
(In reply to comment #78) > (In reply to comment #76) > > At some point after the rough edges are ironed out but before the release > > of the patch, we should ensure that all new decorations, attribute > > propagation, analysis etc happen under a switch that is turned on/off > > by a preference. > > I would say that in this case we should only stop the warning when the option > is off. There are 4 fundamental changes in this bug: > - Marking nullness information in the flow info bits: These are usually bit > operations and are quite cheap. Inserting checks to see if the option is > enabled or not will only degrade performance. Sigh. I forgot that Stephan already pointed out to me that in https://bugs.eclipse.org/bugs/show_bug.cgi?id=365519#c10 that retrieving compiler options can be expensive. This is totally counter intuitive, but being the case, I mostly agree with your outline. So I would amend my recommendation to say, diagnostics must be protected by the switch. Any any other expensive/complicated computations per committer's judgement.
(In reply to comment #93) > I pushed as commit 055478d72ad477a3e7cd23123dc801e665064bbc To answer your question in the new commit- why do we call kind()? We do so because we don;t want to update the field count when we see an initializer. So we check for FIELDS and ENUM_CONST. See org.eclipse.jdt.internal.compiler.ast.Initializer.getKind(). If this answer is satisfactory I'll remove the TODO and write a comment to the effect.
(In reply to comment #95) > (In reply to comment #93) > > I pushed as commit 055478d72ad477a3e7cd23123dc801e665064bbc > To answer your question in the new commit- why do we call kind()? We do so > because we don;t want to update the field count when we see an initializer. So > we check for FIELDS and ENUM_CONST. See > org.eclipse.jdt.internal.compiler.ast.Initializer.getKind(). And the difference to findFieldCountFromSuperInterfaces() is that interfaces have no initializers, I see now. > If this answer is satisfactory I'll remove the TODO and write a comment to the > effect. Yes, great.
(In reply to comment #84) > Looking at QNR#variableBinding() I'm wondering if the use of > TypeDeclaration#declarationOf(..) should be avoided (performance). > Why not compare fieldBinding.declaringClass with type.binding? > You may need some original() calls, but you could avoid iterating > through all fields per class, no? Fixed in both QNR and FieldRef and added regression test. (In reply to comment #85) > Why call super.variableBinding() ?? Just return null ! > (If super were relevant, early exits above would need to respect this, too). Fixed > And somewhere around this method I'd like to see documented that this is > not a general purpose accessor, but works from the POV of null analaysis. > Encode it in the method name or put in the javadoc in Expression. Done. (In reply to comment #86) > (4) would we want to get rid of the overloading re > FlowInfo#markAsDefinitelyAssigned ? > Should be trivial using the new VariableBinding#getAnalysisId() > (+similar cases?) Done. (In reply to comment #93) > I pushed as commit 055478d72ad477a3e7cd23123dc801e665064bbc what I > believe to be still better: re-establish the original order of resolving > member types and fields, just push up the maxFieldCount assignment. > NullReferenceTests pass, compiling the Eclise SDK works to, let's see > what the rest of the test suite has to say. I moved this test to InitializationTest. commit 153a98abc10a4ca42a1d6865f6371358f6ffbaed
An interesting side effect of this fix is the pot. NPE warning here if (val != null && val.hashCode() == 0) ^^^ OR if (val != null) int i = val.hashCode(); ^^^ I was a bit surprised to see this right next to val != null, but I guess its not a false warning, since the field may get changed to null in some other thread after having been compared to non null. Another observation about this warning is that there is no quick fix available, unlike the usual Pot. NPE warnings This is because there is already a check against null. So, the user gets to see a warning in a case where he can't really do anything to prevent it. I wonder if we should even the warning here. Any opinions?
(In reply to comment #98) > An interesting side effect of this fix is the pot. NPE warning here > if (val != null && val.hashCode() == 0) > ^^^ > OR > > if (val != null) int i = val.hashCode(); > ^^^ > I was a bit surprised to see this right next to val != null, but I guess its > not a false warning, since the field may get changed to null in some other > thread after having been compared to non null. Another observation about this > warning is that there is no quick fix available, unlike the usual Pot. NPE > warnings This is because there is already a check against null. So, the user > gets to see a warning in a case where he can't really do anything to prevent > it. I wonder if we should even the warning here. Any opinions? I don't think we want that. We were saying potential only to make warnings weaker, not to raise more warnings. What do we get when mixing non-null and unknown? I'd expect that to give a nullness status that gives no warning when dereferenced nor a redundant-check-warning when checked for non-null again.
A bug. I thought we were handling the case of fields coming from super types already :\ public class A { Object val; public void test() { if (val == null && bval.hashCode() == 0) // OOPS! System.err.println("won't get here"); } } class B{ protected Object bval; } Released this and comment 98 as disabled tests, numbered a_4 and j
(In reply to comment #100) > A bug. I thought we were handling the case of fields coming from super types > already :\ > > public class A { > Object val; > public void test() { > if (val == null && bval.hashCode() == 0) // OOPS! > System.err.println("won't get here"); > } > } > class B{ > protected Object bval; > } > > Released this and comment 98 as disabled tests, numbered a_4 and j I assume A extends B. So, what behavior do you see?
(In reply to comment #99) > What do we get when mixing non-null and unknown? I'd expect that to > give a nullness status that gives no warning when dereferenced nor a > redundant-check-warning when checked for non-null again. I think its my fault. In markAsDefinitelyNonNull, I marked all fields as potentially non null. Shouldn't we let the fields get marked definitely non null here?
I've pushed commit e1454d8de0e417e803707e9280493fc0158df25e with some proposals: (In reply to comment #98) > An interesting side effect of this fix is the pot. NPE warning here > if (val != null && val.hashCode() == 0) > ^^^ > OR > > if (val != null) int i = val.hashCode(); > ^^^ > I was a bit surprised to see this right next to val != null, but I guess its > not a false warning, since the field may get changed to null in some other > thread after having been compared to non null. Another observation about this > warning is that there is no quick fix available, unlike the usual Pot. NPE > warnings This is because there is already a check against null. So, the user > gets to see a warning in a case where he can't really do anything to prevent > it. I wonder if we should even the warning here. Any opinions? Frankly, I don't see this with the current head from the branch. Maybe you got confused by the test case testBug247564a_4()? The compiler only flags the *second* o.toString(). I've slightly remastered the test to make things clearer. (In reply to comment #100) > A bug. I thought we were handling the case of fields coming from super types > already :\ > > public class A { > Object val; > public void test() { > if (val == null && bval.hashCode() == 0) // OOPS! > System.err.println("won't get here"); > } > } > class B{ > protected Object bval; > } > > Released this and comment 98 as disabled tests, numbered a_4 and j Here the solution was indeed incomplete. Using cumulativeFieldCount we intend to leave room for fields from supertypes, but the field.ids are not computed accordingly. Commit contains a one-line change to fix this. I still need to figure out how to accommodate initializers in this fix (currently some tests raise ExceptionInInitializerError). Stay tuned.
(In reply to comment #103) > (In reply to comment #100) > Here the solution was indeed incomplete. Using cumulativeFieldCount we > intend to leave room for fields from supertypes, but the field.ids are > not computed accordingly. Commit contains a one-line change to fix this. > I still need to figure out how to accommodate initializers in this fix > (currently some tests raise ExceptionInInitializerError). Stay tuned. Here I was going in circles (it's been a while since I worked out the id-strategy...). Once more (speaking to myself): raising the field.ids is not a good idea, because enum constants MUST start at 0. That's why we added an offset in FieldBinding#getAnalysisIndex(). Only that method was incomplete, as it would only consider fields of enclosing types, but not super types. From various options how we could obtain the number of super fields I chose not to put more computational effort into getAnalysisIndex() but my latest commit[1] proposes to simply store one more count in STB: a fieldAnalysisOffset which the FieldBinding can directly use in getAnalysisId() to add to its own id. Sorry for the detour (are we getting closer again to your strategy pre comment 48 ?), but this looks promising, I'd say. With a bit more tweakig one of the fields in STB *could* possibly be removed again, since it should always hold that fieldAnalysisOffset + fields.length == cumulativeFieldCount But let's first convince ourselves that ids are now correct for all situations. [1] commit fe090af89b9f2734e464cc22b519aa330b99772f
(In reply to comment #103) > Frankly, I don't see this with the current head from the branch. > Maybe you got confused by the test case testBug247564a_4()? > The compiler only flags the *second* o.toString(). I think this is what i saw and didn't realize that the issue is specific to this case: public class TstFinlaly { Object f; public void foo(String[] args) { try { int i = 10; while (i <20) { if (args == null) { f = null; break; } i++; } return; } finally { if (f != null && f.toString() == "") {} // Pot. NPE } } } The warning goes away if either break or return is commented out. However, since the problem does not seem like one arising out of assigning the id's and seems like an analysis issue, I favour opening a bug for it and fixing it later.
(In reply to comment #105) > I favour opening a bug for it and fixing it > later. On second thoughts, the bug is a manifestation of something that may hamper deferred analysis generally. Although we mark the field as potentially NON null and in normal case this status will not give the "pot. NPE" warning, in the given case the field gets recorded as "MAY NULL" for deferred analysis via FlowContext.recordUsingNullReference. This is then checked for in org.eclipse.jdt.internal.compiler.flow.FinallyFlowContext.complainOnDeferredChecks(FlowInfo, BlockScope). In case of locals, we'd still have the null info from f!= null, and that pot. non null will mix with the pot. null from the catch block to cancel off any warning. But here we lost the info from f!= null when we resetNullInfoForFields() during analysing f.toString(). Because of this loss, the deferred reference, coupled with a pot. null status coming from catch block, raises this bogus warning. Maybe in case of fields we should not defer after f!=null kind of statements.
(In reply to comment #106) > Maybe in case of fields we should not defer after f!=null kind of statements. The solution should be to reach line 407 in FinallyFlowContext. Perhaps, we should mark a def. non null field as protected non null? And at line 406 and similar cases, treat field+prot. non null => no need to record for deferred check?
(In reply to comment #107) > should mark a def. non null field as protected non null? Using def. unknown + pot. non null seems to solve this issue. Though we may miss report some valid cases. Another bug: I had forgotten to handle constant fields in isDefinitelyUnknown. Duh
Yet another bug: org.eclipse.jdt.internal.compiler.lookup.FieldBinding.setNullStatusForStaticFinalField(int) fails to handle cases like these static { if (statField0.hashCode() == 2) { statField = 1; } else { statField = null; } } i.e. does not merge info from all possible paths. :\
commit 62489c237ccf906b51a8dff82611f5af4a1ccb59 fixes comment 105, comment 108, comment 109
(In reply to comment #110) > commit 62489c237ccf906b51a8dff82611f5af4a1ccb59 fixes comment 105, comment 108, > comment 109 Cool! In my workspace I have merged in the latest from master. Do you mind me pushing this?
(In reply to comment #111) > In my workspace I have merged in the latest from master. > Do you mind me pushing this? Sure, thanks! pull my changes and push the merged version. But, oh, watch out for something like comment 57, point (7) ;)
(In reply to comment #112) > (In reply to comment #111) > > In my workspace I have merged in the latest from master. > > Do you mind me pushing this? > > Sure, thanks! pull my changes and push the merged version. But, oh, watch out > for something like comment 57, point (7) ;) Done. I did check the results from two directions of merge, so I'm pretty sure it introduced no artifacts. As you mention comment 57, I also resolved the caveat from point (5). NullReferenceTests and the new ResourceLeakTests are green, will do more testing later.
(In reply to comment #108) > (In reply to comment #107) > > should mark a def. non null field as protected non null? > Using def. unknown + pot. non null seems to solve this issue. Though we may > miss report some valid cases. Looking at UFI#markAsComparedEqualToNonNull I think you're cheating: this.markNullStatus(local, FlowInfo.POTENTIALLY_NON_NULL); this.markAsDefinitelyUnknown(local); Here the second call kills the result of the first. This silences the warning because in state unknown we don't give *any* warnings. OTOH, when I tried this: this.markNullStatus(local, FlowInfo.POTENTIALLY_NON_NULL); this.markPotentiallyUnknownBit(local); the unwanted warning came back and I, too, was tempted to add more special case treatment to the FinallyFlowContext, hm.. (In reply to comment #102) > I think its my fault. In markAsDefinitelyNonNull, I marked all fields as > potentially non null. Shouldn't we let the fields get marked definitely non > null here? Exactly, that's what we need to make up our mind on. If we say for fields we want to respect the uncertainty that concurrency brings, do we record weaker null info right from the beginning or do we just make our warnings weaker (e.g., right in ProblemReporter). Let's have a look at null annotations for fields (focusing on read for now): class C { Object f; @Nullable Object g; @NonNull Object h; void foo() { f.toString(); // (a) silent if (f != null) f.toString(); // (b) silent if (f == null) f.toString(); // (c) warning } void goo() { f.toString(); // (d) warning if (f != null) f.toString(); // (e) weaker warning Object l; if ((l=this.f) != null) f.toString(); // (f) OK } void hoo() { f.toString(); // (g) OK if (f != null) f.toString(); // (h) warning redundant check if (f == null) f.toString(); // (i) warning always false } } Do you agree on the warnings I expect? How do we get there? Perhaps like this: * when reading a field start with: pot.null for @Nullable, def.nonnull for @NonNull, unknown else * during flow analysis use the exact same computation as for locals * when doing checkNPE do one more merge: add the field's intrinsic null status plus the one from the flowInfo Let's check this for the examples: (a) unknown || noinfo -> silent (b) unknown || nonnull -> silent (c) unknown || null -> warn (pot.) (d) pot.null || noinfo -> warn (pot.) (e) pot.null || nonnull -> warn (pot.) (f) local 'l' is def.nonnull For @NonNull we can actually ignore the flowInfo :) Questions: Do we need to consider more patterns than those in the example? Can we make warning (e) weaker than warning (d)? In fact, (e) would be a compromise for lazy folks. When using null annotations they should actually go all the way to (f)! I'm going to experiment a bit with this strategy, and see how it will blend with annotations for fields. Let me know what you think!
(In reply to comment #58) > (In reply to comment #55) > > > - [ ] include fields in null analysis > > I think this one new preference is enough for now. It should be disabled by > default, until we're confident about the implementation. Ayush, Please raise a defect against UI for this support and share a sketch of what preference has been introduced for JDT/Core. Thanks!
(In reply to comment #114) > (In reply to comment #108) > > (In reply to comment #107) > > > should mark a def. non null field as protected non null? > > Using def. unknown + pot. non null seems to solve this issue. Though we may > > miss report some valid cases. > > Looking at UFI#markAsComparedEqualToNonNull I think you're cheating: > this.markNullStatus(local, FlowInfo.POTENTIALLY_NON_NULL); > this.markAsDefinitelyUnknown(local); > Here the second call kills the result of the first. This silences the > warning because in state unknown we don't give *any* warnings. Well, I considered going all the way to add a new bit, nullBit5 to the flowInfo to capture this special status for fields. However, I wanted an easier solution and didn't want to complicate null analysis unless necessary. So looking at your mail and the usage of isDefinitelyUnknown(), I figured that its harmless to use the def. unknown status here, because it only short circuits the defering of such fields. And really, we never will want to defer a "definitely non null" field. So this solution works. However, it may have side effects on locals that are assigned such a field. But I couldn't think of examples where that would really cause a tangible problem over what we have right now. We may just miss a few pot. NPE warnings on locals at best. > do we record > weaker null info right from the beginning or do we just make our warnings > weaker (e.g., right in ProblemReporter). If we do that then don't we have the risk of the definite null status for fields flowing into a local variable? Don't think we want to do that. > Do you agree on the warnings I expect? First, I think you meant to use f, g, and h in foo, goo and hoo. :) Second, I don't quite see much of a point in warning (e). As it is people tend to say that pot. NPE warning case distractions. :) > How do we get there? Perhaps like this: > * when reading a field start with: > pot.null for @Nullable, def.nonnull for @NonNull, unknown else > * during flow analysis use the exact same computation as for locals > * when doing checkNPE do one more merge: > add the field's intrinsic null status plus the one from the flowInfo By intrinsic status you mean something like what we do for constant fields right now? Hmm. > Questions: > Do we need to consider more patterns than those in the example? > Can we make warning (e) weaker than warning (d)? I don't think we should have any warning weaker than (d), lets just skip the warning. Its like the user can do much about it anyway. :) Now from comment 58, we have only (1), (2) and (7) left. Stephan, let me know if you find any issues with the last few fixes. Ofcourse, we'll continue to discuss about the deferring issue.
(In reply to comment #116) > > How do we get there? Perhaps like this: > > * when reading a field start with: > > pot.null for @Nullable, def.nonnull for @NonNull, unknown else > > * during flow analysis use the exact same computation as for locals > > * when doing checkNPE do one more merge: > > add the field's intrinsic null status plus the one from the flowInfo > By intrinsic status you mean something like what we do for constant fields > right now? Hmm. I was thinking if we could use this strategy for deciding on deferring the analysis. i.e. instead of marking an extra definitelyUnknown, let's use the field's binding.nullStatus to record a definite non null. And in recordUsing...() methods, check this bit in the MAY_NULL case. This will let the null info in the flowInfo be as POTENTIALLY_NULL only.
(In reply to comment #115) > (In reply to comment #58) > > (In reply to comment #55) > > > > - [ ] include fields in null analysis > > > > I think this one new preference is enough for now. It should be disabled by > > default, until we're confident about the implementation. > > Ayush, Please raise a defect against UI for this support and > share a sketch of what preference has been introduced for > JDT/Core. Thanks! For the records: I think it is already there, see bug 368390 :)
Hi Ayush, thanks for the detailed answer. Let me start with a simple answer: (In reply to comment #116) > (In reply to comment #114) > First, I think you meant to use f, g, and h in foo, goo and hoo. :) Thanks for mind-reading, you're right of course :) > Second, I don't quite see much of a point in warning (e). As it is people tend > to say that pot. NPE warning case distractions. :) Here I disagree: Once we have null annotations for fields we should really educate people to learn the difference between (e) and (f) because for (f) we can guarantee absence of NPE, while (e) remains intrinsically unsafe when occurring in a concurrent setting. (e) may be good enough in single thread applications and also in transition from fully unprotected code to better code, but we shouldn't implement a solution now that will make it impossible to detect this difference. I think what I'll do is develop the null annotations for fields in parallel so that I can report how each approach in this bug supports the next step.
(In reply to comment #119) > Here I disagree: > Once we have null annotations for fields we should really educate people > to learn the difference between (e) and (f) because for (f) we can > guarantee absence of NPE, while (e) remains intrinsically unsafe > when occurring in a concurrent setting. Yes, i don't say that (e) is bogus. All i'm saying is that, how do we expect the user to act when he sees (e)? The warning will be educational, but not actionable? > I think what I'll do is develop the null annotations for fields in parallel > so that I can report how each approach in this bug supports the next step. Do you want to make a fundamental change in the analysis we're doing for fields as of this bug? I'm asking because I'm testing and fixing any false positives on the way. So if you're working in parallel with a different code in your workspace, let me know! :)
A case which I'm not sure is a false positive public class X { private Object field; void goo(Object var) throws Exception{ if (field != null) field.hashCode(); int i = 20; while (i<10) { if (field == null) { field = new Object(); } field.toString(); //Pot. NPE i--; } } } I know why this happens. We defer the analysis for field in field.toString, but by the time we're back to it we've lost all change in field's nullness due to the toString() method call. So, the tainted nullness from the upstream gets considered in which field is pot. null. We should not let field to be deferred for analysis A related point - When we reset the null info for fields on encountering a messageSend, we should do for not just the current flowInfo, but also the upstream ones. Sigh.
Created attachment 209552 [details] alternative recording of field nullness (In reply to comment #120) > (In reply to comment #119) > > Here I disagree: > > Once we have null annotations for fields we should really educate people > > to learn the difference between (e) and (f) because for (f) we can > > guarantee absence of NPE, while (e) remains intrinsically unsafe > > when occurring in a concurrent setting. > Yes, i don't say that (e) is bogus. All i'm saying is that, how do we expect > the user to act when he sees (e)? The warning will be educational, but not > actionable? 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). > > I think what I'll do is develop the null annotations for fields in parallel > > so that I can report how each approach in this bug supports the next step. > Do you want to make a fundamental change in the analysis we're doing for fields > as of this bug? I'm asking because I'm testing and fixing any false positives > on the way. So if you're working in parallel with a different code in your > workspace, let me know! :) ATM my workspace is a multi-parallel universe :) I'm attaching a patch that I had played with last night. Currently I'm in a branch that starts from our last shared state in NullAnalysisForFields and adds null annotations for fields. My current goal is to find out, which variant better suites annotated fields: w/ or w/o this patch... I'll keep you posted with my findings from this experiment...
(In reply to comment #114) > Here the second call kills the result of the first. This silences the > warning because in state unknown we don't give *any* warnings. Ah, I see now. I didn't notice that markAsDefinitelyUnknown erases the earlier set bits to make 1001. Sorry, this isn;t what I intended to do. Just saw your new patch while writing this. Will take a look.
(In reply to comment #122) > I'll keep you posted with my findings from this experiment... Good news chapter one: - I have initial support for null annotations on fields in my workspace on top of HEAD in NullAnalysisForFields - I could easily mend a few regressions -> NullReferenceTests and NullAnnotationTests are green. Regarding annotated fields this only tests for the very basics. I will now gradually drill into trickier scenarios. Ayush, please also keep me posted on all changes on your side so I can quickly check their effects also in the annotations setting.
(In reply to comment #124) > Ayush, please also keep me posted on all changes on your side so I can > quickly check their effects also in the annotations setting. I had a few changes, but i'm confused between your patch above and those changes. I saw your patch and it does go in the right direction. I didn't understand why we're not propogating the infoToReport outside after marking a variable pot. null in it? Will this pot. nullness not be needed later?
(reply to comment 125 following in a minute) A design question: when checking these kinds of fields: - constants - annotated @Nullable or @NonNull we could do either of: (1) always go to the FieldBinding and inspect what information we have there (2) go through flowInfo. If state for a field is 0000 (=start) and if field is one of the above, initialize the state into the flowInfo My idea of doing (2) is that it could reduce the load of always checking various properties of a field before we know where to look for nullness info. I think caching this in the flowInfo would be really cheep and could make the code cleaner actually. A snippet in UFI#is*Null() would look like this int position = local.getAnalysisId(this.maxFieldCount); long nullAnnotationBits; if (isField && ((nullAnnotationBits = local.tagBits) & TagBits.AnnotationNonNull|TagBits.AnnotationNullable) != 0) { checkInitialize(position, nullAnnotationBits); } ... continue like normal with private void checkInitialize(int position, long nullAnnotationBits) { if (position < BitCacheSize) { // use bits long mask = 1L << position; if ((this.nullBit1 & mask) == 0 && (this.nullBit2 & mask) == 0 && (this.nullBit3 & mask) == 0 && (this.nullBit4 & mask) == 0) { if (nullAnnotationBits == TagBits.AnnotationNullable) { // set potentially null this.nullBit2 |= mask; } else if (nullAnnotationBits == TagBits.AnnotationNonNull) { // set assigned non null this.nullBit1 |= mask; this.nullBit3 |= mask; } } } else { // FIXME use extraBits } } Integrating static finals into this scheme should be easy. We could then challenge method resetNullInfoForFields: either we just let it do and re-initialize when needed (loosing most of the advantage of caching) or keep another bitset for fields with definitely known status (to be maintained in checkInitialize()) so resetNullInfoForFields can directly use that as its mask. Alternatively, if we go route (1) I'd probably add a query to VariableBinding: int getNullAnalysisKind() which would answer one of LOCAL, PLAIN_FIELD, CONSTANT_FIELD, NULLABLE_FIELD, NONNULL_FIELD so clients can quickly switch over these cases. Let me know which you like better.
(In reply to comment #125) > (In reply to comment #124) > > Ayush, please also keep me posted on all changes on your side so I can > > quickly check their effects also in the annotations setting. > > I had a few changes, but i'm confused between your patch above and those > changes. I saw your patch and it does go in the right direction. I didn't > understand why we're not propogating the infoToReport outside after marking a > variable pot. null in it? Will this pot. nullness not be needed later? This patch is an implementation of that idea: (In reply to comment #114) > How do we get there? Perhaps like this: > * when reading a field start with: > pot.null for @Nullable, def.nonnull for @NonNull, unknown else > * during flow analysis use the exact same computation as for locals > * when doing checkNPE do one more merge: > add the field's intrinsic null status plus the one from the flowInfo (And yes, with intrinsic status I mean info that is independent of the flow, like for constants or annotated fields). I'm using the infoToReport to weaken the message but I keep the original flowInfo so downstream has a chance to still see the stronger info. So much for the theory. Next I will search for an example that we can use for the proof of the pudding.
(In reply to comment #71) > (In reply to comment #69) > > (In reply to comment #57) > > > (7) I couldn't immediately see the motivation for the change in > > > TypeDeclaration#internalAnalyseCode(..) > > > Could you please educate me? :) > > > > As mentioned in comment 64, this got introduced during rebasing of the patch. I > > think this should be reverted. Let me know what you think. > > I don't exactly recall who did the change: I, me or myself. But I'll > ask him anyway. As a start to clean up some of the pending questions I asked around who of these three guys did the change and none of them would confess. A clue to the mystery lies in attachment 206514 [details] (but I won't tell anyone :) ). After this bit of archeology we can quickly agree that UNREACHABLE_OR_DEAD is the correct bit, the mask UNREACHABLE should only ever be used for checks, never as an argument to setReachMode(int).
(In reply to comment #126) > we could do either of: > (1) always go to the FieldBinding and inspect what information we have there > (2) go through flowInfo. If state for a field is 0000 (=start) and if > field is one of the above, initialize the state into the flowInfo Hmm. What happens to the markAs..() methods in (2). We will still have to check for the kind of field and then make sure a constant field is not affected by a markAs..() , no? Also how will is*Null() methods initialize constant fields? Where does it get the initialization info from? With (1), resetNullInfo..() doesn't have to deal with constant fields as such. So, (1) seems abit straightforward, if not the best solution, or maybe I don't completely understand yet.
(In reply to comment #129) > (In reply to comment #126) > > we could do either of: > > (1) always go to the FieldBinding and inspect what information we have there > > (2) go through flowInfo. If state for a field is 0000 (=start) and if > > field is one of the above, initialize the state into the flowInfo > Hmm. What happens to the markAs..() methods in (2). We will still have to check > for the kind of field and then make sure a constant field is not affected by a > markAs..() , no? How can markAs..() be triggered for a constant? Certainly not via an assignment. But if a constant had unknown nullness, then a check will actually add valuable info, no? (I just created an interesting test for this, pushed in commit 179bb7fc2719bb1b36627a6096fe9cec93defbc1). > Also how will is*Null() methods initialize constant fields? > Where does it get the initialization info from? It wouldn't compute those, just cache from FieldBinding#nullStatus, but we could do this uniformly in UFI#checkInitialize(). > With (1), resetNullInfo..() doesn't have to deal with constant fields as such. > So, (1) seems abit straightforward, if not the best solution, or maybe I don't > completely understand yet. My argumentation may be a bit unfair: I'm looking at code that has special treatment for constants AND for annotated fields. And in conjunction this looks a bit convoluted. That's what motivated me to look for a more uniform implementation. But anyway, the representation shouldn't be all that relevant in this case. I'll let you have a look at the test mentioned above. If that can be easily fixed in the current impl I'm fine with that. I'll check it in my variant. Additional challenge regarding checked constants: for these we want the info from a check to be propagated as-is: after checking a constant for nonnull we *definitely* know for all times that it is nonnull, no "potentially"! Also contained in my last commit: a few tests regarding double checking normal fields. I'll comment about that later.
(In reply to comment #121) > [...] > A related point - When we reset the null info for fields on encountering a > messageSend, we should do for not just the current flowInfo, but also the > upstream ones. Sigh. I think this being taken care of by Block#analyse(..): // don't let the flow info collected for fields from this block persist. flowInfo.resetNullInfoForFields(); This is a bit drastic, but in this case it seems to help us. Or which direction of "up" did you mean in "upstream ones"?
(In reply to comment #121) > A case which I'm not sure is a false positive > public class X { > private Object field; > void goo(Object var) throws Exception{ > if (field != null) field.hashCode(); > int i = 20; > while (i<10) { > if (field == null) { > field = new Object(); > } > field.toString(); //Pot. NPE > i--; > } > } > } > I know why this happens. We defer the analysis for field in field.toString, but > by the time we're back to it we've lost all change in field's nullness due to > the toString() method call. So, the tainted nullness from the upstream gets > considered in which field is pot. null. We should not let field to be deferred > for analysis No, AFAICS it's not because of deferring but because ... (In reply to comment #131) > (In reply to comment #121) > > [...] > > A related point - When we reset the null info for fields on encountering a > > messageSend, we should do for not just the current flowInfo, but also the > > upstream ones. Sigh. > > I think this being taken care of by Block#analyse(..): > // don't let the flow info collected for fields from this block persist. > flowInfo.resetNullInfoForFields(); > This is a bit drastic, but in this case it seems to help us. ... this is actually *too drastic*. Uncomment that line and your test above remains silent. To check that it must remain silent indeed just uncomment the enclosing loop. Do you recall the rationale behind letting block ends kill field null status?
OK, let me try to step back a bit and summarize questions and positions. (I) How to signal for a normal field when it has been checked for nonnull? I've been trying to find examples where we would need the definite info in order to correctly continue with the analysis. Yet, I haven't found any such example. Part of the reasons is: we don't consider synchronized, so *every* knowledge we have in time t1 will by blurred *immediately after*. So, contrary to my prior belief, I don't see anything wrong with encoding (f != null) with markAsDefinitelyUnknown(f). There is nothing we should warn about f after this check. Conversely, after a null-check markAsPotentiallyNull is the proper answer. Additionally, if user wants to know exactly, she should first assign into a local variable! (In the far future we may want to add a warning when state unknown is encountered, but that will be the final step towards a full guarantee of absence of NPE). So, what previously I called cheating now looks like the correct solution to me. To double check have a look at testBug247564l_1() from commit [1]: no matter if this way or inverting all null/nonnull checks: we will never get a warning on seemingly redundant or contradictory checks. The test looks funny, as if it should raise some warnings, but any warning about redundant checks should be definite and safe. We can't be absolutely sure so lets just remain silent here. If we agree about this test we can also agree that unknown and pot.null are correct encodings of nonnull/null checks. Conversely, I don't see deferred checking as a root issue to decide about. Consistently applying the above encoding should automatically yield the desired diagnostics. (II) Using bits in FlowInfo for constant fields or not? For constant fields the question is not about definite or weak info, but about static info vs. flow info. With commit [1] I pushed a test case testBug247564b_4(), which indicates that inspecting the FieldBinding#nullStatus is not sufficient for this test. I have a working version following the lines of comment 126. I'll back port that into the public branch, so you can better compare. I don't think this test can be fixed without involving the bits in flowInfo also for constants. In this case we're not encoding the status of the field, but the program's knowledge about the field, which indeed is flow-dependent. For both (I) and (II) my answers seem to be compatible with null annotations. I'll continue working on that. I hope you can follow my reasoning - for me matters are clearing up. If you agree then we should probably go into polishing mode. We still have items (1) and (2) from comment 57. Would it actually make sense to release the patch *before* the final refactorings, so that the master branch has one commit that is not cluttered by those? [1] commit 179bb7fc2719bb1b36627a6096fe9cec93defbc1
(In reply to comment #132) > Do you recall the rationale behind letting block ends kill field null status? Not really. I, me and myself have no clue either. :) In reply to comment #131) > (In reply to comment #121) > I think this being taken care of by Block#analyse(..): > // don't let the flow info collected for fields from this block persist. > flowInfo.resetNullInfoForFields(); > This is a bit drastic, but in this case it seems to help us. > Or which direction of "up" did you mean in "upstream ones"? By upstream I meant to ask should we erase null info for the flow from which a loop originated. Currently, we only do the reset for the looping context's flowInfo when there's a message send in the loop. So, assuming that the loop doest not touch a field that was assigned null above the loop, we will still raise the pot. NPE on the field after the loop.
(In reply to comment #133) > So, contrary > to my prior belief, I don't see anything wrong with encoding (f != null) with > markAsDefinitelyUnknown(f). There is nothing we should warn about f after this > check. Conversely, after a null-check markAsPotentiallyNull is the proper > answer. > So, what previously I called cheating now looks like the correct solution to > me. Yes, so markAsDefUnknown() should be sufficient in markAsDefNonNull() and markAsComparedEqualToNonNull(). I should remove the marking as pot. non null in these methods because it's anyway overwritten by marAsDefUnknown() > To double check have a look at testBug247564l_1() from commit [1]: > If we agree about this test we can also agree that unknown and pot.null are > correct encodings of nonnull/null checks. I agree with being silent on all. > (II) Using bits in FlowInfo for constant fields or not? > > For constant fields the question is not about definite or weak info, but about > static info vs. flow info. With commit [1] I pushed a test case > testBug247564b_4(), which indicates that inspecting the FieldBinding#nullStatus Wow! You stole my test case. I had the same test case in my local workspace which Id forgotten to push yesterday. :D > I hope you can follow my reasoning - for me matters are clearing up. > If you agree then we should probably go into polishing mode. Yes, we should remove the extra resetNullInfo.. call in Block#analyseCode() and add your proposal (II) above. There are a few false positives I found in the SDK, for which I will try to construct minimal tests and investigate. > We still have > items (1) and (2) from comment 57. Would it actually make sense to release the > patch *before* the final refactorings, so that the master branch has one commit > that is not cluttered by those? Yes, exactly what I planned. We can have a separate commit for refactorings. So, finally we have 4 things left to do listed in this comment, plus anything we find during testing. I hope I didn't leave out anything.
Another test that should be fixed by proposal (II) in above comment: public void testBug247564b_5() { this.runNegativeTest( new String[] { "X.java", "public class X {\n" + " static final Object o;\n" + " static final Object o1 = new Object();\n" + " static {\n" + " if (o1.hashCode() == 2){\n" + " o = new Object();\n" + " } else {\n" + " o = null;\n" + " }\n" + " }\n" + " void foo1() {\n" + " if (o == null) \n" + " return;\n" + " o.toString();\n" + // cant be null for sure, dont complain " }\n" + "}\n"}, "" ); }
commit bdaa90ce470f5a58bbc365c74e43162bf2353bdf This commit nicely combines Stephan's and my strategies and improves the handling of constant fields - both in storing and analysing their nullness and in preventing discarding their info on a method call. I've tried to leverage the first analysis of fields inside TypeDeclaration in further analysis in methods and c'tors. Previously, we were discarding this useful information. Now, I carry forward the null info of constant fields from this info into further analysis. A bit mask records the constant fields' positions right when they are found during the analysis. This mask is also available to subsequent c'tors and methods. The second part is to discard the special handling of constant fields. We no longer consult the binding to get the null status but treat final fields the same as final locals, as suggested by Stephan. This gives us better diagnostics. Egs are in the new tests numbered b_5 onwards. With these we've covered all false positives that we had found above. Will test more. Stephan, let me know if there's something fishy. Note that the same approach can be used for null annotations on fields too. In TypeDeclaration.analyseCode(), you'll just need to make sure you collect the null info of fields from both, the staticFieldInfo and nonStaticFieldInfo.
(In reply to comment #137) > commit bdaa90ce470f5a58bbc365c74e43162bf2353bdf All tests pass
A minor false positive public class TestFinally { Object field; void someMethod(){} void foo() { try { if (field != null) { } } finally { if (field != null) { someMethod(); field.toString(); // Bogus Pt. NPE } } } } Happens because the def. uknown status that was set to field in the field!= bnull check in the finally block is reset by someMethod(), leading to recording field for deferred check at field.toString().
(In reply to comment #139) > Happens because the def. uknown status that was set to field in the field!= > bnull check in the finally block is reset by someMethod(), leading to recording > field for deferred check at field.toString(). Makes me wonder if instead of setting bits to 0000 in resetNullInfoForFields(), we should set them to 1001 (def. unknown). This may help in these situations.
(In reply to comment #137) > commit bdaa90ce470f5a58bbc365c74e43162bf2353bdf > This commit nicely combines Stephan's and my strategies and improves the > handling of constant fields - both in storing and analysing their nullness and > in preventing discarding their info on a method call. General comment: well done! We're getting there. Details: (a) TypeDeclaration: before calling updateConstantFieldsMask() you only check AccFinal, shouldn't that be (modif & (AccFinal|AccStatic)) == (AccFinal|AccStatic) ? (Do we want a VariableBinding#isConstant()?) (b) Also TypeDeclaration: when constructing the constructorInfo you changed from using outerInfo (fieldless) to a modified flowInfo: could that have undesired side-effects for non-constant fields? Null-info in flowInfo has been reset at that point, but what about defInits? (c) FlowInfo.constantFieldsMask should probably be moved to UFI, since CFI delegates corresponding access to its children anyway. Only access from TypeDeclaration needs a change: Just add another method similar to updateConstantFieldsMask, accepting a bitset. (At that location if we indeed had a CFI updating its constantFieldMask won't have any effect currently!) (d) Assignment: call "flowInfo.markNullStatus(fieldBinding, nullStatus)" is redundant, the entire enclosing if can be deleted without any loss. (e) UFI: left-overs from my experiments with checkInitializeForField can safely be removed :) (f) UFI: redundant mark* before markAsDefinitelyUnknown can be removed (we already agreed on that). (g) UFI#resetNullInfoForFields: always use "1L" for constructing long bitmasks! (h) same method: loop over position is still not quite right: many redundant iterations using the same vectorIndex. I think all these can easily be fixed, right?
(In reply to comment #141) > (a) TypeDeclaration: before calling updateConstantFieldsMask() you only check > AccFinal, shouldn't that be (modif & (AccFinal|AccStatic)) == > (AccFinal|AccStatic) ? We're already inside a if (field.isStatic()) block so thats not required. > (e) UFI: left-overs from my experiments with checkInitializeForField can safely > be removed :) > > (f) UFI: redundant mark* before markAsDefinitelyUnknown can be removed (we > already agreed on that). > > (g) UFI#resetNullInfoForFields: always use "1L" for constructing long > bitmasks! I already have these in my workspace. Will get back on the other points soon.
(In reply to comment #140) > (In reply to comment #139) > > Happens because the def. uknown status that was set to field in the field!= > > bnull check in the finally block is reset by someMethod(), leading to > recording > > field for deferred check at field.toString(). > > Makes me wonder if instead of setting bits to 0000 in resetNullInfoForFields(), > we should set them to 1001 (def. unknown). This may help in these situations. Sounds reasonable to me. Have you tried that? Thinking of @Nullable fields, however, I will want to reset to pot.null. So on the long run we may let TypeDeclaration create another flowInfo recording the default status for non-constants to be used during resetNullInfoForFields. For the current bug, however, all fields can be reset to the same status, so if 1001 works that be it.
(In reply to comment #141) (b), (d), (e), (f), (g), (h) done. (a) -> no action, (c) -> I couldn't really get the point. For (b), I've just used addNullInfoFrom, so that we don't have to care about inits. (h) opened my eyes to comprehensively fix the handling of the mask bits for constant fields and masking of non-constant field infos. Frankly speaking, I got very confused with making it part of the 2D array 'extra', which is basically meant for the null info + init streams. We don;t need that really, plus increasing the extended length to 7 was now trying to mess around with extra[6] in all methods which should only deal with null info + inits. So, I just introduced a new 1D array to specifically handle the constant field mask. The only challenge here was to create the proper mask streams. Also, per comment 140, the end result of resetNullInfoForFields(), should be 1001 for every non-constant field, instead of the 0000 previously. I've tried to explain as much as I could through comments. Let me know if it is still fuzzy. Will require summoning some blood to review this method. ;) This is commit commit e4a60c2fd3a0cbed770d3f19dd819b3c27a25786. Stephan, please take a look if this +1able. :)
(In reply to comment #144) > (c) -> I couldn't really get the point. Still by introducing FlowInfo#addConstantFieldMask() you kind-of addressed this issue, handling looks consistent now, but what's the use now of having the field constantFieldsMask in FlowInfo vs. UnconditionalFlowInfo? Same for the new field extraConstantFieldMask?
(In reply to comment #145) > (In reply to comment #144) > > (c) -> I couldn't really get the point. > > Still by introducing FlowInfo#addConstantFieldMask() you kind-of addressed this > issue, handling looks consistent now, but what's the use now of having the > field constantFieldsMask in FlowInfo vs. UnconditionalFlowInfo? Same for the > new field extraConstantFieldMask? This is so that we don;t have to cast anything to UnconditionalFlowInfo to be able to call methods such as addConstantFieldsMask(FlowInfo), and others which may pass a FlowInfo as an argument, and we need to work with these mask bits.
(In reply to comment #146) > (In reply to comment #145) > > (In reply to comment #144) > > > (c) -> I couldn't really get the point. > > > > Still by introducing FlowInfo#addConstantFieldMask() you kind-of addressed this > > issue, handling looks consistent now, but what's the use now of having the > > field constantFieldsMask in FlowInfo vs. UnconditionalFlowInfo? Same for the > > new field extraConstantFieldMask? > > This is so that we don;t have to cast anything to UnconditionalFlowInfo to be > able to call methods such as addConstantFieldsMask(FlowInfo), and others which > may pass a FlowInfo as an argument, and we need to work with these mask bits. Is it you believe that the argument will always be a UFI and you just want to avoid the cast? Then if the assumption is broken and you do get a CFI you will simply access the wrong field instead of throwing CCE? Or, is it that *sometimes* you do intend to maintain a constantFieldsMask in a CFI instance? Or, what?
Created attachment 209734 [details] proposal re item (c) FYI, here's how I would have addressed item (c): with this patch we statically know that constantFieldsMask (and extraConstantFieldMask) are only ever maintained in an UnconditionalFlowInfo, thus nothing can ever get inconsistent in a ConditionalFlowInfo (plus those objects are a tiny bit smaller :) ). Am I missing anything?
While going through your latest changes I only made two minor observations (i) Nit-picking: field UFI#indexOfConstantFieldBitStream seems to be noise :) (j) Shouldn't the constructors of NullInfoRegistry also copy (extra)constantFieldsMask? I was surprised to see that one of them doesn't even copy maxFieldCount. Hm? Everything else looks good! As a final pass (assuming previous points are not controversial) I'll have a look at the tests to see if all expected results are OK.
(In reply to comment #148) > Created attachment 209734 [details] > proposal re item (c) Thanks for this. Actually, i was confused about your point that the ConditionalFlowInfo might do something wrong with the new fields. But now I see that you were trying to say that CFI containing those fields is redundant since it anyway depends on its constituent UFIs to handle those two. :) (In reply to comment #149) > While going through your latest changes I only made two minor observations Will take care of both of these.
(In reply to comment #150) > > While going through your latest changes I only made two minor observations > Will take care of both of these. I'm also thinking if we should short circuit the operations on the contstantFieldsMask when the option is not enabled. Should suffice if we safeguard the call to updateConstantFieldsMask() with a check. The resetNullInfoForFields can be similarly protected. Sounds good?
(In reply to comment #150) > (In reply to comment #148) > > Created attachment 209734 [details] > > proposal re item (c) > Thanks for this. Actually, i was confused about your point that the Btw, isn't it weird now that FlowInfo will have two methods on adding and updating a property (contstantFieldsMask) that it does not even have? :)
(In reply to comment #151) > (In reply to comment #150) > > > While going through your latest changes I only made two minor observations > > Will take care of both of these. > I'm also thinking if we should short circuit the operations on the > contstantFieldsMask when the option is not enabled. Should suffice if we > safeguard the call to updateConstantFieldsMask() with a check. The > resetNullInfoForFields can be similarly protected. Sounds good? Good idea, just: do you have the option value at hand? It might be easier for the TypeDeclaration to retrieve that value? I think the most relevant gain would be in resetNullInfoForFields, can you short-circuit that somehow? This method is called far more often than updateConstantFieldsMask(). What about keeping a tagBit in each FlowInfo indicating whether fields are being analysed for null? Would that be difficult to implement?
(In reply to comment #153) > Good idea, just: do you have the option value at hand? It might be easier for > the TypeDeclaration to retrieve that value? I think the most relevant gain > would be in resetNullInfoForFields, can you short-circuit that somehow? This > method is called far more often than updateConstantFieldsMask(). > What about keeping a tagBit in each FlowInfo indicating whether fields are > being analysed for null? Would that be difficult to implement? Apart from TypeDeclaration, reset... is only called from MessageSend. In both we have the scope available, from where we can retrieve compiler options. So, its already done in my workspace :)
(In reply to comment #152) > Btw, isn't it weird now that FlowInfo will have two methods on adding and > updating a property (contstantFieldsMask) that it does not even have? :) No, not weird. Sometimes even interfaces have such methods :)
Created attachment 209741 [details] proposed test cleanup I'm done re-checking the tests and found all to be OK! Please find attached a proposal for a few clean-ups, like whitespace issues, trying to make tests even stronger, documentation. I even removed one obviously redundant test case. Some other tests are also pretty close to being redundant making it difficult to get the special point of each test, but nothing needing to be fixed. Ayush, do you want to push one more commit as a release candidate so we know we're on the same page?
(i), (j) and comment 151 fixed via http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=NullAnalysisForFields&id=f66efbdc3081dcf241c033f43363418b530216c3
(In reply to comment #156) > Created attachment 209741 [details] > proposed test cleanup Oops saw this only after my previous comment. Maybe i can club these with the other rename and cleanups which still remain. I am also yet to move the new fields from FI to UFI. Keeping these as pending, can you review the final batch and see if its ok? (you can also merge with master when you pull my changes and then push back the final thing). If everything is good, should i squash all commits into one? And then on top of that single commit push refactoring changes?
(In reply to comment #148) > Created attachment 209734 [details] > proposal re item (c) commit 191102b85107063832a41c075bfbbbf41836afec
(In reply to comment #158) > (In reply to comment #156) > > Created attachment 209741 [details] > > proposed test cleanup > Oops saw this only after my previous comment. Maybe i can club these with the > other rename and cleanups which still remain. I am also yet to move the new > fields from FI to UFI. Keeping these as pending, can you review the final batch > and see if its ok? Done, based on your commit 191102b85107063832a41c075bfbbbf41836afec everything is ready to go: +1. :) > (you can also merge with master when you pull my changes > and then push back the final thing). Done: commit f44bd4458f2f98a78f2a50575dc000f2fbe0b04e > If everything is good, should i squash all commits into one? And then on top of > that single commit push refactoring changes? Sounds good. As extra safety what I usually do is: - start a new local branch - from command line create a single patch by specifying from-to commits - apply patch to local branch (should cleanly apply!) - compare new branch with previous NullAnalysisForFields - if no differences found ... - ... update buildnotes :) - ... run all tests :)) - push NewLocalBranch to origin/master (It's always nice to keep the local branch unsquashed anyway...) YMMV
OK, compiling the Eclipse SDK as a final test revealed one more problem when a large number of fields is involved: The computation of the first mask in UFI#resetNullInfoForFields() may produce wrong results if maxFieldCount >= 64. The computation should look s.t. like long mask = this.maxFieldCount < BitCacheSize ? (-1L << this.maxFieldCount) : 0L; mask |= this.constantFieldsMask; This still passes all NullReferenceTests and clears a false positive observed in DebugPlugin.java:581. Please include this fix in your final commit. I'll provide a test case shortly.
(In reply to comment #161) > long mask = this.maxFieldCount < BitCacheSize ? (-1L << this.maxFieldCount) : > 0L; > mask |= this.constantFieldsMask; Ok thats a good catch. Here's a minimal test case public class X { Object field0, field1, field2, field3, field4, field5, field6, field7, field8, field9, field10, field11, field12, field13, field14, field15, field16, field17, field18, field19, field20, field21, field22, field23, field24, field25, field26, field27, field28, field29, field30, field31, field32, field33, field34, field35, field36, field37, field38, field39, field40, field41, field42, field43, field44, field45, field46, field47, field48, field49, field50, field51, field52, field53, field54, field55, field56, field57, field58, field59, field60, field61, field62, field63, field64; static final Object o2 = new Object(); public X() { field23 = null; o2.toString(); if (field23.hashCode() == 0){} } }
Created attachment 209759 [details] test & fix re comment 161 This patch brings two regression tests using the following boundary conditions: * field <boundary> constant local * constant field <boundary> local Where <boundary> is between bits and extra bits. Given that the new method resetNullInfoForFields has some complexity I definitely wanted to have a test case that toggles when the new comparison is changed from "<" to "<=". Would be nice if this could get included without the need to file a new bug :)
(In reply to comment #163) > Created attachment 209759 [details] > test & fix re comment 161 Cool. I'll include your tests too. Thanks :) Let me run the whole test suite now on a fresh branch
Released into master via commit d6c2a90130be430285c5f6a000090d250c310afd. Cleanups and refactorings + doc to follow.
doc validated and released to master via commit 4c78a9541a9d7cf86f5bdb66d4409bd2fb5e2484
Created attachment 209774 [details] final cleanups and refactorings This includes all sundry refactorings and cleanups.
Created attachment 209775 [details] Eclipse SDK problems discovered A list of valid new warnings generated in the SDK projects hosted in a runtime workbench. There are several of warning of each kind, so the list tries to cover mostly unique cases.
(In reply to comment #167) > Created attachment 209774 [details] > final cleanups and refactorings commit 51e1f00f8d474f0b209b47afa85e44328a517f92 Thanks Stephan for reviewing and helping out on this bug over the past one year. :)
(In reply to comment #168) > Created attachment 209775 [details] > Eclipse SDK problems discovered How would I interpret this list ? These are the methods were a new issue was reported by the compiler ?
(In reply to comment #170) > (In reply to comment #168) > > Created attachment 209775 [details] > > Eclipse SDK problems discovered > > How would I interpret this list ? These are the methods were > a new issue was reported by the compiler ? Yes, when you switch the option on, you will see new null-field related diagnostics in these methods.
I am reviewing the patch deep enough to allow me to design some scenarios for white box testing. I'll post comments in batches so progress be made even as the review is still happening. Here is the first batch: (1) org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/BatchCompilerTest test311_warn_options() references the wrong bug id. (2) org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/Block.java There are no changes in this file ?? This needn't have been released. (3) org.eclipse.jdt.compiler.tool/src/org/eclipse/jdt/internal/compiler/tool/Options.java org.eclipse.jdt.core/batch/org/eclipse/jdt/internal/compiler/batch/messages.properties org.eclipse.jdt.internal.compiler.lookup.VariableBinding (at least) These files are missing copyright change. (4) org.eclipse.jdt.internal.compiler.ast.Expression.java I think Expression.variableBinding(Scope) should be renamed to be getVariableBindingForNullAnalysis(Scope) (or some similar name.) The way the current name sounds, it has the feel of a general API, but it doesn't work that way. While javadoc can be improved to capture the fact that null can be returned by the method even where valid field binding exists, changing the method name is a better way. (5) resetNullInfoForFields should be done not only for MessageSends but also for other code artifacts executions. For example, in the following, there is an incorrect warning: public class X { static Object lastObject; public static void main(String [] args) { if (lastObject == null) { new X(); lastObject.toString(); } } static void foo() { } X() { lastObject = this; } }
(In reply to comment #172) > I am reviewing the patch deep enough to allow me to design > some scenarios for white box testing. I'll post comments in > batches so progress be made even as the review is still happening. > Here is the first batch: I guess it would be better to capture this in (a) separate bug(s).
(In reply to comment #173) > (In reply to comment #172) > I guess it would be better to capture this in (a) separate bug(s). Yes, Ayush also wanted it that way: I am moving review comments to bug 369487
Patch looks good. Review comments for follow up posted at bug 369487. I'll continue to test it a bit more and any issues discovered will be logged at bug 369487 Verified for 3.8 M5 using build id: I20120122-2000
After much discussion, we have decided to withdraw the support added for null analysis of fields in 3.8 M5 (via the current bug) so that a more general solution could be considered in future without being constrained by the present implementation. For details, see: https://bugs.eclipse.org/bugs/show_bug.cgi?id=369487 https://bugs.eclipse.org/bugs/show_bug.cgi?id=370185 https://bugs.eclipse.org/bugs/show_bug.cgi?id=370183 A future implementation should also make consider the scenarios in: https://bugs.eclipse.org/bugs/show_bug.cgi?id=370787 https://bugs.eclipse.org/bugs/show_bug.cgi?id=237236
(In reply to comment #166) > doc validated and released to master via commit > 4c78a9541a9d7cf86f5bdb66d4409bd2fb5e2484 Doc has been removed via commit 97d7a449b0fd269405efff8b2afa86a609a9ffbc
Can we consider this a dup of bug 331649?
Bug 331649 did not bring any flow analysis for fields. It's bug 414237 that would bring flow analysis for (@LazyNonNull) fields. I'm marking this as a duplicate of the latter. *** This bug has been marked as a duplicate of bug 414237 ***
