Community
Participate
Working Groups
Build ID: M20080911-1700 Steps To Reproduce: I have set the "Redundant null check" to "Warning" (at Java->Compiler->Errors/Warnings). In the following example code fragment, a "Rendundant null check" warning is shown at the assert statement: TestClass foo = map.get(key); if (foo == null) { foo = new TestClass(); map.put(key, foo); } assert foo != null; // Redundant null check warning One purpose of asserts is to do redundant checks, so maybe they should be generally excepted from the "Redundant null check" warning?
Changing Version tag to something more believable.
(In reply to comment #0) > One purpose of asserts is to do redundant checks, so maybe they should be > generally excepted from the "Redundant null check" warning? If you mean that the expression within the assert statement should be exempted from null analysis - let me tell you that expressions like a!=b, or a==b are analysed irrespective of whether they occur in a for, if , or assert statement, So selectively filtering out this analysis for asserts will require more information to be passed to the analysis of such expressions, which doesnt seem like a good thing to do. Assert is a runtime feature, while reporting the redundant checks is a compile time feature. Unless you have a very strong reason to exempt asserts from null analysis, I'd recommend leaving this as it is. OTOH, if you mean that checks inside the assert expressions should not have any effect on the subsequent code, viz. assert transformer != null : "null transformer for " + key; if (transformer == null) //Warning issued here but maybe you dont want it return null; We dont intend to change this behaviour. You can check bug 198044 comment 1. Olivier, what do you think about this?
(In reply to comment #2) > Assert is a runtime feature, while reporting the > redundant checks is a compile time feature. Unless you have a very strong > reason to exempt asserts from null analysis, I'd recommend leaving this as it > is. Assert is not just a runtime feature IMHO. I agree if you mean that it is only actively checked at runtime. But asserts also serve as redundant checks, to make the code more robust. Asserts are also a way to comment the code and make it easier to read and understand by other developers. Let me give an example related to my code snippet from the bug description: TestClass foo = map.get(key); if (foo == null) { foo = new TestClass(); map.put(key, foo); } assert foo != null; // Redundant null check warning To get rid of the warning, I decide to remove the assert line. TestClass foo = map.get(key); if (foo == null) { foo = new TestClass(); map.put(key, foo); } Now, first of all, it is not clear any more to the reader of the source code that foo is intended to be not null after this code block. For the sake of this example, let's assume that the "new TestClass()" needs to be replaced by the invocation of a factory method: TestClass foo = map.get(key); if (foo == null) { foo = TestClassFactory.createTestClass(); map.put(key, foo); } Due to the missing assert, is is now even more unclear that foo is expected to be not null here. Even more, if createTestClass() returns null for whatever reason, a test run with asserts enabled would point to the bug immediately. There is no compile time warning that foo could be null here. > So selectively filtering out this analysis for asserts will require more > information to be passed to the analysis of such expressions, which doesnt seem > like a good thing to do. A non-tivial implementation should not be the reason for closing this bug as WONTFIX. It would only lead to that developers who use asserts, would turn off rendundant null checks at all, which cannot be your intention. A better approach might be to make it configurable that assert statements are not checked at all.
(In reply to comment #3) > TestClass foo = map.get(key); > if (foo == null) { > foo = new TestClass(); > map.put(key, foo); > } > assert foo != null; // Redundant null check warning If you do this, then yes the assert is redundant. If you want to keep it for clarity, you could still leave it as a line comment. > TestClass foo = map.get(key); > if (foo == null) { > foo = TestClassFactory.createTestClass(); > map.put(key, foo); > } If you add an assert statement after this code snippet, it would be tagged with a "redundant null check" warning as there is no way for the compiler to know that foo is definitely not null after the code. TestClassFactory.createTestClass() could return null, so after this code, foo can still be potentially null. > A non-tivial implementation should not be the reason for closing this bug as > WONTFIX. It would only lead to that developers who use asserts, would turn off > rendundant null checks at all, which cannot be your intention. Agreed. This being said, where I have a problem is for: assert foo != null; if (foo != null) { System.out.println("not null"); } A warning is reported against the if condition saying that foo cannot be null at this location. This is not true. If assertion are disabled, the assert condition has no effect so the next line can be reached and needs a null check. This issue is reported in another bug report (see bug 173725).
Created attachment 158003 [details] proposed fix v0.5 + regression tests Here's the solution that I and Olivier decided on: We'd exempt the assert statements from null check warnings, but we will go on using the null analysis info from the assert expression in our flow analysis as usual. The implementation is as follows: A flag hideNullComparsionWarnings is used in FlowContext, which is set by the AssertStatement.analyseCode() method. This flag prevents the following warnings from being raised: 1. Redundant null check 2. Null comparison always yields false. It also disables the deferring of null checks for expressions inside the assert statements in loops, etc. What it DOES NOT prevent is the following warnings: 1. Null reference/potential null reference warnings 2. Redundant null assignment 3. Null in instanceof This is because these warnings are relevant for the assert expressions.
Created attachment 158043 [details] proposed fix v1.0 + regression tests Small change in new test test0957_assert(), and code cleanup (changing dont to don't).
Released Ayush's fix in HEAD for 3.6M6.
(In reply to comment #5) Just found this bug after scratching my head on why the code ends up in the if statement when the condition is clearly false. Actually asked around (http://stackoverflow.com/questions/2280017/java-assert-nasty-side-effect-compiler-bug), did not realize at first this is a JDT (EJC?) specific. The discussion here appears to center around warnings, however it looks like the introduction of assertion also affects (actually eliminates) the condition evaluation that follows. The following snippet (already mentioned in the comments) appears to always print "not null" in 3.5M5 (when assertions are disabled): assert foo != null; if (foo != null) { System.out.println("not null"); } (Tested with eclipse-java-helios-M5-win32-x86_64 and 1.6.0_18 JRE.) This does appear to be a bug to me - unless I am missing something, side-effect free assertions are not allowed/expected to modify behavior of other code. It was not clear to me from the discussion here if this aspect was discussed, whether it is part of the same issue or a different one, whether this is recognized as a bug and whether this is fixed by the patch.
Right. This is a bug. We should not optimize the if condition when it is inferred from the assert condition.
I missed that case. The null analysis of the assert condition should not impact further null analysis. This might need to revert some of the latest changes. Ayushman, could you please investigate a fix for next I-build ? Thanks.
> I missed that case. The null analysis of the assert condition should not impact > further null analysis. > This might need to revert some of the latest changes. So what this implies is that in the following case: assert foo != null; if (foo != null) { // Redundant null check warning here System.out.println("not null"); } We want to keep the redundant null warning , but we dont want to optimize the code gen for the if statement. Is this right? Also, I think that the original issue mentioned in this bug (that of the warning raised for the assert expressions) has been correctly fixed for this bug. Wouldn't it be better to raise a new bug for this case and then proceed?
(In reply to comment #11) > We want to keep the redundant null warning , but we dont want to optimize the > code gen for the if statement. Is this right? > > Also, I think that the original issue mentioned in this bug (that of the > warning raised for the assert expressions) has been correctly fixed for this > bug. Wouldn't it be better to raise a new bug for this case and then proceed? Yes, let's do that. I opened bug 303448. Let's continue all comments into this new bug.
(In reply to comment #11) > We want to keep the redundant null warning , but we don't want to optimize the > code gen for the if statement. Is this right? Yes, the redundant null warning should stay as this indicates that the user doesn't trust the assert result.
Verified for 3.6M6 using I20100307-2000.
