Community
Participate
Working Groups
v_A14 I was looking for test cases triggering the potentially-null warning. For the following situation I was surprised NOT to trigger this warning (i.e., the test currently fails): this.runNegativeTest( new String[] { "X.java", "public class X {\n" + " void foo(Object o) {\n" + " if (o != null) {/* */}\n" + " Object p = o;\n" + " p.toString();\n" + // complain here " }\n" + "}"}, "----------\n" + "1. ERROR in X.java (at line 5)\n" + " p.toString();\n" + " ^\n" + "Potential null pointer access: The variable p may be null at this location\n" + "----------\n", JavacTestOptions.Excuse.EclipseWarningConfiguredAsError); What surprised me is that o.toString() would trigger the warning whereas p.toString() does not, although we know both variables have the same value. I played with the implementation and found a pretty easy way to propagate the info from o to p. Even with a incomplete implementation the above test runs green without breaking any of NullReferenceTest. I think this would make the analysis even more valuable. Would you like me to extend the experiment to a complete patch?
Ayushman, you seem to be the expert in this area? Any comments?
I had a quick look and found this - when o is compared against null it gets tainted and henceforth in the stream we consider it as maybe null. During the step where p is initialised, we check if o is null or not, and accordingly set the null status of p. But in case the definite null status of o is not known, we do not decide upon p's null status. Hence it only gets marked as definitely assigned, but not as potentially null. I don't see any reason why the null info of o can't be passed as it is to p. If you have a way to do so, maybe you can submit the patch and I can have a look. Thanks!
Created attachment 174559 [details] proposed patch Contained in this patch: * new constant POTENTIALLY_NULL and new method markAsPotentiallyNull declared empty in FlowInfo, implemented in UnconditionalFlowInfo * SingleNameReference now produces POTENTIALLY_NULL * LocalDeclaration, Expression and Assignment propagate POTENTIALLY_NULL The patch is tested like such: * testBug292478(): test propagation in LocalDeclaration (from initialization) * testBug292478a(): test propagation in Assignment * testBug292478b(): variant with different pre-state * testBug292478c(): test use of extraBits * testBug292478d(): test how Assignment may affect initsOnFinally Note: The modified block in Expression#nullStatus is probably dead code, I don't see how "local" can be non-null her: localVariableBinding() is implemented only in SingleNameReference, CastExpression and Assignment, all of which override nullStatus() without calling super.
Marking dependency on bug 320170 because testBug292478d() fails without the patch from the other bug.
Created attachment 177820 [details] same fix with few modifications The fix itself looks good. Fixed the following in the above patch: 1) Modified the new method FlowInfo#markAsPotentiallyNull(..) to make sure the test harness covers it. 2) Added copyrights to relevant files 3) Fixed formatting 4) Changed the method markAsPotentiallyNull(..) to abstract and added its implementation in ConditionalFlowInfo on the lines of other markAs.. methods 5) Added new transformation to NullReferenceImplTransformations for markAsPotentiallyNull
Released in HEAD for 3.7M2
Thanks for adopting and improving my patch. I appreciate the co-operation :)
Reopening since the fix causes the following issue: On org.eclipse.jdt.internal.compiler.lookup.Scope.java:1733, there's a warning- "Null comparison always yields false: The variable foundInsideProblem can only be null at this location". This seems bogus since foundInsideProblem may be assigned some value at line 1713, which may be potentially null at best.
(In reply to comment #8) > Reopening since the fix causes the following issue: > On org.eclipse.jdt.internal.compiler.lookup.Scope.java:1733, there's a warning- > "Null comparison always yields false: The variable foundInsideProblem can only > be null at this location". This seems bogus since foundInsideProblem may be > assigned some value at line 1713, which may be potentially null at best. Here's a smaller version showing the problem: Object foo(int i, boolean b1, boolean b2) { Object o1 = null; done : while (true) { switch (i) { case 1 : Object o2 = null; if (b2) o2 = new Object(); o1 = o2; if (o1 != null) // [1] return o1; break; case 2 : break done; } } if (o1 != null) // [2] return o1; // [3] return null; } Gives three bogus warnings, all due to wrong analysis that o1 can only be null. Removing either the enclosing while or switch makes the problem disappear. I'm looking into this.
(In reply to comment #9) > I'm looking into this. Thanks, Stephan. Maybe fixing it will also fix the issue with bug 133125 that is also causing grief with last I-build.
(In reply to comment #9) >[..] > Here's a smaller version showing the problem: > > Object foo(int i, boolean b1, boolean b2) { > Object o1 = null; > done : while (true) { > switch (i) { > case 1 : > Object o2 = null; > if (b2) > o2 = new Object(); > o1 = o2; > if (o1 != null) // [1] > return o1; > break; > case 2 : > break done; > } > } > if (o1 != null) // [2] > return o1; // [3] > return null; > } > > Gives three bogus warnings, all due to wrong analysis that o1 can only > be null. Removing either the enclosing while or switch makes the problem > disappear. Preliminary investigation suggests that the problem happens while reporting deferred null checks for the looping context in WhileStatement.analyseCode(). In this method call, we add potential null info of action info (with o1 marked pot. null) to the upstream info (with o1 marked def. null). This adding results in a stream with o1 marked as def null, which looks suspicious. This doesnt happen without the patch because in its absence, we'd just mark o1 as definitelly unknown when we analyse o1 = o2, while with the patch we mark it as pot. null and thus record o1 for deffered null check. However, I am yet to dig deeper. Stephen, let me know your findings as well. One thing though - I feel [2] and [3] are legitimate warnings, since o1 can only be assigned via o1=o2, and the statement next to that says that as soon as o1 is assigned a non null value, return o1. So [2] will only be reached if o1 is still null.
(In reply to comment #11) > Stephen, let me know your findings as well. I have a patch upcoming in a minute :)
Created attachment 178454 [details] fix for the regression Here are two new test cases (putting both scenarios in one example (comment 9) wasn't a good idea, but in individual tests all warnings are indeed bogus, right? The regression reveals the following assumption underlying the null-analysis: Analysing potentially-null and potentially-non-null must be symmetric. Previously the status transfer for assignments only handled the null-case but this broke clients of flowInfo.cannotBeDefinitelyNullOrNonNull(local): if only pot-null is set wrong conclusions are drawn, i.e., flowContext.recordUsingNullReference() reported too many problems. The patch fixes this by adding two more constants to FlowInfo: POTENTIALLY_NON_NULL and POTENTIALLY_NULL_OR_NON_NULL plus corresponding methods. (Future cleanup might consider using bitsets also here). I did some refactoring to reduce the bloat incurred by adding more branches to various places. I'm going to check the connection to bug 324762 / bug 133125 in a minute.
More comments: (In reply to comment #11) > Preliminary investigation suggests that the problem happens while reporting > deferred null checks for the looping context in WhileStatement.analyseCode(). > In this method call, we add potential null info of action info (with o1 marked > pot. null) to the upstream info (with o1 marked def. null). This adding results > in a stream with o1 marked as def null, which looks suspicious. In my understanding the deferred check itself was/is correct, its the initial recordUsingNullReference() that was propagated (from switch to encl. while) where it shouldn't. > This doesnt happen without the patch because in its absence, we'd just mark o1 > as definitelly unknown when we analyse o1 = o2, while with the patch we mark it > as pot. null and thus record o1 for deffered null check. Well, marking as pot.null was fine, but o2 also has pot.non-null at this point which I previously missed to transfer, too.
Created attachment 178458 [details] Improved fix for regression Bug 324762 showed that I missed one location where non-null must be propagated, too (ConditionalExpression.nullState(..)). Also included: additional test in the vein of Bug 324762.
Created attachment 178468 [details] fix for regression v3 The real-life situation from bug 324762 triggered yet one more combination as witnessed by testBug324762a(): also 'unknown' state must be propagated from ternary (as pot.unknown). To avoid combinatorial explosion I changed the encoding of nullStatus from enum-style (-1,0,1,2..) to bitset-style (0,1,2,4..) which allows us to OR the bits for pot.null, pot.non-null and pot.unknown. However, the logic in UnconditionalFlowInfo.addPotentiallyUnknownMark() is not complete: it assumes that no other bits than pot.null and/or pot.non-null have been set before (see the Assert.isTrue()).
(In reply to comment #16) > Created an attachment (id=178468) [details] > fix for regression v3 > > The real-life situation from bug 324762 triggered yet one more combination > as witnessed by testBug324762a(): also 'unknown' state must be propagated > from ternary (as pot.unknown). > > To avoid combinatorial explosion I changed the encoding of nullStatus from > enum-style (-1,0,1,2..) to bitset-style (0,1,2,4..) which allows us to > OR the bits for pot.null, pot.non-null and pot.unknown. > > However, the logic in UnconditionalFlowInfo.addPotentiallyUnknownMark() > is not complete: it assumes that no other bits than pot.null and/or > pot.non-null have been set before (see the Assert.isTrue()). Patch looks good. I have a small question though. I'm not clear about the usage of addPotentiallyUnknownMark() here. As far as i understand, it is used to set the following configurtaions - 0011 pot. nn & pot. un 0101 pot. n & pot. un 0111 pot. n & pot. nn & pot. un which would only work if pot. n or pot. non-null, or both are set. But do we do anything to make sure addPotentiallyUnknownMark() is not called in other cases?
(In reply to comment #17) > [..] I have a small question though. I'm not clear about the usage > of addPotentiallyUnknownMark() here. As far as i understand, it is used to > set the following configurtaions - > 0011 pot. nn & pot. un > 0101 pot. n & pot. un > 0111 pot. n & pot. nn & pot. un plus: 0001 pot. unknown The idea is to update any 0xy0 -to-> 0xy1 > which would only work if pot. n or pot. non-null, or both are set. But do we do > anything to make sure addPotentiallyUnknownMark() is not called in other cases? That's a valid concern and it's the reason why I inserted the Assert.isTrue(). By construction (see FlowInfo.markNullStatus) addPotentiallyUnknownMark() is only called after either of markAsPotentiallyNull() -> 0100 markAsPotentiallyNonNull() -> 0010 markAsPotentiallyNullOrNonNull() -> 0110 markAsDefinitelyUnknown() -> 1001 And as I'm writing this I see the last case should actually trigger the Assert! I'll try to find a test case to cover this one, too. Thanks for asking!
(In reply to comment #18) >[..] > That's a valid concern and it's the reason why I inserted the Assert.isTrue(). > By construction (see FlowInfo.markNullStatus) addPotentiallyUnknownMark() is > only called after either of > markAsPotentiallyNull() -> 0100 > markAsPotentiallyNonNull() -> 0010 > markAsPotentiallyNullOrNonNull() -> 0110 > markAsDefinitelyUnknown() -> 1001 > And as I'm writing this I see the last case should actually trigger the > Assert! I'll try to find a test case to cover this one, too. The last case wont trigger the assert because addPotentiallyUnknownMark() cant be called after it. See that FlowInfo.markNullStatus() always returns if markAsDefinitelyUnknown() is called. :)
(In reply to comment #19) > The last case wont trigger the assert because addPotentiallyUnknownMark() cant > be called after it. See that FlowInfo.markNullStatus() always returns if > markAsDefinitelyUnknown() is called. :) True. Seems I couldn't remember my own thinking of last night :-/ Still I don't quite like the asymmetric situation. I'll come up with cleaned-up version shortly, where pot. n/nn/un are treated equally. You could help me finding useful tests if you have a hint how to test if pot.un has been transferred correctly (on its own or combined with pot.n or pot.nn).
Created attachment 178507 [details] fix for regression v4 This version I like much better. All relevant methods now handle state in two steps: (1) Is any of un, n, nn set? -> Use it directly. (2) Otherwise accumulate a non-empty set of pot.un, pot.n, pot.nn. If all fails answer un (may not be possible to trigger, but fail safe) This implies I had to switch from directly setting any "potentially" state to starting with resetNullInfo() and making all three markAsPotentiallyX() methods work incrementally, i.e., manipulating one bit only. Added the Assert to all three of them. All of NullReferenceTests pass, I'm currently running the full compiler.regression suite. I didn't consistently update the coverage things (to be honest I haven't yet figured out how exactly that is used, seems NullReferenceImplTests are a bit incomplete in this regard?). Other than that I feel quite confident that this version is ready for commit.
(In reply to comment #21) > Created an attachment (id=178507) [details] > fix for regression v4 > > [..] Thanks Stephen. I had already updated the NullReferenceImplTests for the two new markAs..() methods. So i'll take care of that. I was trying to dig out some test where pot. un. is passed around. I'll take a look at the new patch and get back.
(In reply to comment #21) > I'm currently running the full compiler.regression suite. this suite is green. I also re-checked by compiling org.eclipse.osgi. All diagnostics now make sense to me.
(In reply to comment #23) > this suite is green. > I also re-checked by compiling org.eclipse.osgi. > All diagnostics now make sense to me. Thanks Stephan. As usual your help is much appreciated. Ayushman, please release so that we can respin a build if needed.
Small issue with the patch. It refers to the Assert class that doesn't belong to JDT/Core. Classes in the compiler cannot use any classes or interfaces outside the compiler code as it needs to run as a standalone application. I would simply remove the call to the Assert class.
If you believe the assertion is useful, you could add a small Assert class or add the required method in the org.eclipse.jdt.internal.compiler.util.Util class.
Created attachment 178515 [details] Updated patch Patch based on the last patch but without the usage of the Assert class. Ayushman, Stephan, please review and if fine, we will release.
(In reply to comment #27) > Created an attachment (id=178515) [details] > Updated patch > > Patch based on the last patch but without the usage of the Assert class. > Ayushman, Stephan, please review and if fine, we will release. +1 from my side. Is it a consciously made design decision not to use runtime assertions in the compiler, or has the need just never before occurred?
(In reply to comment #28) > Is it a consciously made design decision not to use runtime assertions > in the compiler, or has the need just never before occurred? This is not really the issue. You can make runtime assertions (we do have calls to org.eclipse.jdt.internal.compiler.problem.ShouldNotImplement), but we cannot use types outside the compiler code itself as the compiler has to be able to run as a pure standalone batch compiler.
Created attachment 178528 [details] patch to commit Here's the final patch. I made a few small changes: - Renamed the new markAs... methods as markXXXXBit(). This is because the old markAs.. methods all set the bit configuration to reflect a particular state, while the new ones just add on to the existing info in the form of a one-bit change. So in the future if we need, say, a method to change the bit to reflect the "pot null" config, we can conveniently name it as markAsPotentiallyNull() - Removed the markAsPotentiallyNull's test from the test harness, since the harness only has tests for methods that change entire state, and not just a bit. - Qualified the constants UNKNOWN, POTENTIALLY_NULL, etc with the class name. Stephan, let me know if you have any issues. I'll commit this patch in a while.
(In reply to comment #30) > Created an attachment (id=178528) [details] > patch to commit > [..] > Stephan, let me know if you have any issues. I'll commit this patch in a while. Looks good to me. Go! :)
(In reply to comment #29) > (In reply to comment #28) > This is not really the issue. You can make runtime assertions (we do have calls > to org.eclipse.jdt.internal.compiler.problem.ShouldNotImplement), I see. > but we cannot > use types outside the compiler code itself as the compiler has to be able to > run as a pure standalone batch compiler. I know. Is this actually checked during build, or is it your eagle eye that watches over this rule?
(In reply to comment #32) > I know. Is this actually checked during build, or is it your eagle eye that > watches over this rule? No, there is no way (unfortunately) to check this during the build. The way I kind of automated it is by creating a separate project that has only two source folders linked to the source folders inside jdt.core (compiler and batch) and that has a foundation 1.0 library on the classpath. Like that I ended being sure that code outside of these two source folders cannot be used without generating errors and I also ensure that only Foundation 1.0 API are used. This is clearly a setting that I recommend for anyone working on the compiler code.
(In reply to comment #30) > Created an attachment (id=178528) [details] > patch to commit Released in HEAD for 3.7M2
Verified for 3.7M2 using build I20100909-1700
