Community
Participate
Working Groups
We should add a new option to let the user choose if he/she wants to see the null analysis of the assert statement being used for further diagnosis. For example: int foo(String s) { assert s != null; if (s != null) { return s.length(); } return -1; } right now we would flag the if condition as a redundant null check.
Created attachment 179233 [details] proposed fix v1.0 + tests This patch adds the new option org.eclipse.jdt.core.compiler.problem.suppressNullInfoFromAsserts It'll be good to discuss where best to put this option on the Java>Compiler>Errors/Warnings page The option itself is a kind of suboption for 3 different options - redundant null check, null pointer access, and potential null ptr access. I would suggest adding this option just after "treat optional errors like fatal compile errors.." which currently appears at the end of the errors/warnings page. Olivier, whats your take on this? Also, can you please review the patch? Thanks!
Looks good, but you must also add the new option inside the JavaCore class so that it can be referenced from the UI.
The UI will have to be reorganized a little to put all null warnings together.
Markus, I believe this option is required as we got request to make this diagnosis optional.
Created attachment 179470 [details] proposed fix v1.1 + regression tests same fix with the option added to JavaCore.
Released in HEAD for 3.7M3
That we issue a redundant null checks has been reported previously. I think if we're going to fix that then we should also change the default given that assertions are also disabled by default. Also, the other options must get additional Javadoc and mention the impact of the newly added option.
> * Compiler option ID: Suppress null related warnings arising because >of assert statements. > * <p>When enabled, the compiler will not flag null warnings on >variables that got marked as maybe or definitely The Javadoc should talk about "warning or error" or "problem". Given my previous comment and given that -ea is off by default we should invert the preference and name it COMPILER_PB_INCLUDE_ASSERT_IN_NULL_ANALYSIS [default: Disabled]
(In reply to comment #7) > That we issue a redundant null checks has been reported previously. I think if > we're going to fix that then we should also change the default given that > assertions are also disabled by default. Also, the other options must get > additional Javadoc and mention the impact of the newly added option. Its true that asserts are disabled by default, but a code such as: assert (foo != null); if (foo != null) { } means that the user is maybe just using it as a marker in the code to tell someone reading it that at this point, foo cannot be null. In this kind of scenario, not giving off the null warning on the if statement can be contradictory to what the user may expect. Till now, we had always concurred on the strategy to always report null problems arising out of asserts. It is just for some special cases, when the user doesnt want to see this warnings, that we wanted an option to turn them off. So renaming it to COMPILER_PB_INCLUDE_ASSERT_IN_NULL_ANALYSIS [default: Disabled] will reverse the logic behind introducing the new option - which was - to let the user suppress a warning in some cases in which it irks him. I agree with your point about fixing the javadoc though.
(In reply to comment #9) > (In reply to comment #7) > > That we issue a redundant null checks has been reported previously. I think if > > we're going to fix that then we should also change the default given that > > assertions are also disabled by default. Also, the other options must get > > additional Javadoc and mention the impact of the newly added option. > > > Its true that asserts are disabled by default, but a code such as: > > assert (foo != null); > > if (foo != null) { > } > > means that the user is maybe just using it as a marker in the code to tell > someone reading it that at this point, foo cannot be null. In this kind of > scenario, not giving off the null warning on the if statement can be > contradictory to what the user may expect. If he wanted to add a comment then add a comment, not an assert. Also, in that case he could enable to also check 'assert'. > Till now, we had always concurred on the strategy to always report null > problems arising out of asserts. It is just for some special cases, when the > user doesnt want to see this warnings, My point: out of the box we should not see the warning. The default behavior should be close to what the code does by default when executed. Hence the null check is not redundant. Once a user decides he uses asserts and enables them on launching (via -ea), he can also enable it for the 'null' analysis and then clean up his code accordingly.
Ayushman, please describe the new constant in JavaCore inside the buildnotes.
This will need to be described inside the N&N for 3.7M3 and the doc should also be updated to reflect the new constant (org.eclipse.jdt.doc.isv\guide\jdt_api_options.htm).
Created attachment 179730 [details] same fix with suggested changes This patch: -> Modifies javadoc of org.eclipse.jdt.core.JavaCore.COMPILER_PB_REDUNDANT_NULL_CHECK, org.eclipse.jdt.core.JavaCore.COMPILER_PB_POTENTIAL_NULL_REFERENCE, org.eclipse.jdt.core.JavaCore.COMPILER_PB_NULL_REFERENCE to reflect the new option. -> Changes the name of the new option to org.eclipse.jdt.core.JavaCore.COMPILER_PB_INCLUDE_ASSERTS_IN_NULL_ANALYSIS. -> Small modifications to the tests since now by default we dont show warnings arising out of asserts. -> Changes the name of batch compiler option to includeAssertNull -> Updates the jdt doc guide to include the new option and update the doc for the 3 null warning options. Dani, can you please have a quick glance to see if the documentation, etc is ok? Thanks!
Initial tests show that the new option seems to work. Here some Javadoc correction: "errors/warnings": all other options use "error or a warning". I see no reason to introduce a new pattern here. This needs to be fixed in the html doc too. * Compiler option ID: Suppress null related errors/warnings arising because of assert statements. The initial descriptions is inverse compared to the preference name. I would describe what it does when it gets enabled. * <p>This diagnostic is further tuned in the presence of assert statements * by the option {@link #COMPILER_PB_INCLUDE_ASSERTS_IN_NULL_ANALYSIS} This is missing a '.' and a </p> at the end (see also bug 326514). The sentence could be more user friendly, e.g. Assert statements are ignored unless {@link #COMPILER_PB_INCLUDE_ASSERTS_IN_NULL_ANALYSIS} is enabled.
Created attachment 179836 [details] patch with doc changes
Released in HEAD for 3.7M3.
*** Bug 198044 has been marked as a duplicate of this bug. ***
Verified in I20101025-1800
Thanks Rajesh. Verified for 3.7 M3 using I20101025-1800.
