Community
Participate
Working Groups
Run the following program built with javac and with eclipse compiler (don't use the assert option). With javac, it prints false but with eclipse it prints true. ######## public class Test { public boolean assertTest(Object foo) { assert foo instanceof String; if (foo != null) { return true; } else { return false; } } public static void main(String[] args) { System.out.println(new Test().assertTest(null)); } } ###########
Ayushman, I think considering the side effect in the code generation, we should not make any assumption that the assert statement expression should be true for the null check. Optimizing the code generation based on this result can lead to false runtime behavior when assertion is disabled at runtime (the default).
(In reply to comment #1) > Ayushman, I think considering the side effect in the code generation, we should > not make any assumption that the assert statement expression should be true for > the null check. > Optimizing the code generation based on this result can lead to false runtime > behavior when assertion is disabled at runtime (the default). AFAIK, the fix for bug 303448 should've taken care of this. Perhaps some change we did recently has made this problem to resurface. I'll investigate.
Ok so i think I know whats wrong here. Bug 303448 only fixed the case for EqualExpression, which as this case shows, is clearly not sufficient. We need to extend this fix for other kinds of ASTNodes that can occur inside an assert expression. I tried doing so for instanceof expression and could easily fix the case. I dont really want to touch the existing behaviour with asserts ( that is, treating the assert expression as always true to evaluate downstream code), since I dont know what else it might break. Olivier, what do you think?
(In reply to comment #3) > Ok so i think I know whats wrong here. > Bug 303448 only fixed the case for EqualExpression, which as this case shows, > is clearly not sufficient. We need to extend this fix for other kinds of > ASTNodes that can occur inside an assert expression. I tried doing so for > instanceof expression and could easily fix the case. > > I dont really want to touch the existing behaviour with asserts ( that is, > treating the assert expression as always true to evaluate downstream code), > since I dont know what else it might break. Olivier, what do you think? As long as the code generation doesn't rely on the assert result to be optimized, I think this is fine. We cannot assume the assert to always be true to optimize the code generation. With all the cases we have with assert statement, I would be tempted to add an extra option to be able to disable the null analysis for assert statement. So we can make sure that we properly report unnecessary null checks for the users who want it and this could be disabled for the ones that don't like it. So please provide a fix for all expressions and if everything is fine, we might contribute it for M2. Having wrong code generation is evil as problems cannot be seen before runtime. Do you think we can target M2 ? I'd like to get Srikanth's thoughts on this.
Created attachment 178912 [details] patch under test Here's a patch that inserts the fix for 303448 at missing places. All NullReferenceTests pass with it. We should add the new option in a separate bug.
(In reply to comment #5) > We should add the new option in a separate bug. Definitely. Opened bug 325342.
Targetting M2 as the code generation is wrong.
All tests pass, Olivier please review. TIA
Patch looks good.
Released in HEAD for 3.7M2.
Verified using I20100921-1024
