Bugzilla – Full Text Bug Listing
| Summary: | "Potential null pointer" analysis interferes with annotation-based analysis | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Sergey Bushkov <sbushkov> | ||||||||
| Component: | Core | Assignee: | Stephan Herrmann <stephan.herrmann> | ||||||||
| Status: | VERIFIED INVALID | QA Contact: | |||||||||
| Severity: | normal | ||||||||||
| Priority: | P3 | CC: | amj87.iitr, markus.kell.r, srikanth_sankaran, stephan.herrmann | ||||||||
| Version: | 3.7 | ||||||||||
| Target Milestone: | 3.8 M6 | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Windows 7 | ||||||||||
| Whiteboard: | |||||||||||
| Attachments: |
|
||||||||||
Created attachment 211211 [details]
default analysis settings
Created attachment 211212 [details]
default analysis setings + annotations analysis enabled
Created attachment 211213 [details]
analysis result
All results you are seeing are intended. Let my try to explain: (In reply to comment #0) > public void foo3(@Nullable String s) { > s.toString(); > expectNonNull(s); // [2] not detected by annotation analysis > } If s is null you will never reach the method call, so why should we warn? For local variables and arguments the flow analysis works exactly as it did before the introduction of null annotations, and from this flow analysis we know that in line [2] s cannot be null. > IMHO - there is no need for separate "Potential null pointer access" and > "Potential violation of null specification" settings. The first setting can be > used in both places (probably, it should be Warning; not Ignore). I would indeed suggest to set both to "warning", but there is a relevant difference: (a) if you pass a value that could be null into a method expecting @NonNull you may be violating a contract, which means *that other method* may break due to your mistake. That's bad. (b) if you dereference a variable for which null is a legal value, you are operating on your own risk, the current method may break but any damage is local. Perhaps with some local reasoning you kind-of know that in a particular situation null cannot occur - on your own risk. One purpose of null annotations is to make it possible to correctly assign the blame *if* things still go wrong (keep in mind that we cannot - yet - provide full safety). Violating a contract breaks the whole concept of blame assignment. This only means, both warnings have different qualities. You should care about both, but I'm sure people will find reasons why either of these warnings is "uninteresting" in their particular style of code, so we rather provide too many than too few options. We might at best discuss coupling the settings, so that maybe switching on null annotations also initially sets the other problem to warning. Apart from that I intend to close as INVALID. (In reply to comment #4) > We might at best discuss coupling the settings, so that maybe switching on > null annotations also initially sets the other problem to warning. This is a good point, but won't be free of side effects, since it'll unleash a lot of other pot. null warnings not otherwise caused due to null annotations. I like the current situation better when the user can switch on the pot. null warnings if they want more fine-grain analysis. However, many other users may encounter the same surprise as Sergey did. Perhaps, a popup asking the user "Do you also want to enable the Potential null pointer warnings?" may help. I filed bug 371968 for the UI aspect. Thanks, Sergey, for sharing your impressions. I hope the explanations above helped. Since no changes in JDT/Core are planned I'm closing this issue. |
Build Identifier: 4.2.M5; build I20120127-1145 I enabled the new annotation-based analysis; but did not enable "Potential null pointer access" problem (set to Ignore" by default). --- sample code --- public void foo2(@Nullable String s) { expectNonNull(s); // [1] error detected by annotation analysis } public void foo3(@Nullable String s) { s.toString(); expectNonNull(s); // [2] not detected by annotation analysis } public void expectNonNull(@NonNull String s) { s.toString(); } --- /sample code --- When a @Nullable parameter is passed to another method which expects @NonNull parameter, the compiler reports an error [1]. When a @Nullable parameter is dereferenced, compiler does not report error; and even worse - if the parameter is later passed to another method expecting @NonNull - compiler does not report error [2]. If the "Potential null pointer access" level is set to, say, Warning - the warning will be reported on the line with dereference; but still no error at [2]. IMHO - there is no need for separate "Potential null pointer access" and "Potential violation of null specification" settings. The first setting can be used in both places (probably, it should be Warning; not Ignore). Reproducible: Always Steps to Reproduce: Enable annotation-based analysis; compile the sample code.