Bugzilla – Full Text Bug Listing
| Summary: | [compiler][null] allow fine tuning of the effect of @NonNullByDefault | ||
|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Jan Koehnlein <jan> |
| Component: | Core | Assignee: | Stephan Herrmann <stephan.herrmann> |
| Status: | CLOSED DUPLICATE | QA Contact: | |
| Severity: | enhancement | ||
| Priority: | P3 | CC: | greensopinion, jarthana, sebastian.zarnekow, steffen.pingel, stephan.herrmann, timo.kinnunen, tomasz.zarna |
| Version: | 4.3 | ||
| Target Milestone: | BETA J8 | ||
| Hardware: | PC | ||
| OS: | Mac OS X | ||
| Whiteboard: | |||
|
Description
Jan Koehnlein
Stephan, please take a look. Thanks! Between the title and summary of this bug I'm not sure what's your use case. Do you have @Nullable annotations on fields? What warning are you seeing? Could you please give a code example and what exactly you want to disable? Example Java code:
import org.eclipse.jdt.annotation.NonNullByDefault;
import com.google.inject.Inject;
@NonNullByDefault
public class Foo {
@Inject
private String x;
// ERROR: The @NonNull field x may not have been initialized
}
The error marker did not appear before 4.3M5
If I go to Project > Properties > Java > Errors/Warnings > Null analysis and disable "Enable syntactic null analysis for fields" the error persists. It seems to depend on the preference "Violation of null specification" instead.
Thanks for the example. I think it points to the element that needs fine-tuning: @NonNullByDefault. Let me iterate the options that I see: (1) Adding more options to disable parts of the analysis. I doubt that globally disabling null analysis for fields will make many people happy (on the long run). (2) Configure "syntactic" analysis. Please note that the "syntactical" analysis is only meant to suppress warnings of low relevance, ie. this option only fine-tunes the null analysis of fields - no intention of disabling. (3) Regarding @Inject we'll continue the discussion in bug 400421. (4) Allow fine tuning of the effect of @NonNullByDefault Image the following code: @NonNullByDefault({METHOD, PARAMETER}) public class Foo { private String x; // not affected by the default } I would let the default be {METHOD, PARAMETER, FIELD} to encourage full null analysis on the long run. Note that a similar discussion is also relevant wrt. to JSR 308, where we'd need a variant of the above, because technically all null annotations will by TYPE_USE, but still a fine-tuning of @NonNullByDefault for specific code locations may be desirable. I changed the bug title to reflect that I see option (4) as the preferred solution (was: [compiler] Nullable annotation on fields cannot be switched off) Thanks for the clarification. I like (4) best, too, as it is most suggestive and the effect of the compilation depends on the code only and not on some hidden switch. Please also consider to improve labels or the semantics of the preferences. I was really assuming I could switch the entire null analysis for fields on and off. I am already having a hard time understanding the current switches: e.g. how does "'@NotNull' parameter not annotated in overriding method" relate to "Inherit null annotations". This is why I think (1) is only an option if things can be named in a suggestive way. (In reply to comment #5) > Please also consider to improve labels or the semantics of the preferences. I > was really assuming I could switch the entire null analysis for fields on and > off. Have you seen the description in the New&Noteworthy? http://download.eclipse.org/eclipse/downloads/drops4/S-4.3M5a-201302041400/news/ Does this help to clarify? A similar write-up for the help is still pending. > I am already having a hard time understanding the current switches: e.g. > how does "'@NotNull' parameter not annotated in overriding method" relate to > "Inherit null annotations". The effects of those options are exclusive: if "inherit null annotations" is disabled, than overriding a @NonNull parameter with an un-annotated parameter is a kind of (safe) contra-variant refinement. The option "'@NotNull' parameter not annotated in overriding method" adds a warning for this situation, to raise awareness in case this contra-variance was unintended. See bug 381443 for details. It's a bit clearer now, even though the n&n is also a bit vague on what kind of patterns are taken into consideration. All in all, even though it makes sense semantically, the field analysis feels a bit paranoid to me. There is always a trade-off between verbosity and strictness here. Guarding the same field multiple times within the same method should only be necessary in exceptional multithreaded cases. Given the current impl I'd prefer to be able to exclude fields from null analysis completely or at least to switch to a less strict setting. And if possible to have a single self-explaining switch for that. (In reply to comment #7) > It's a bit clearer now, even though the n&n is also a bit vague on what kind > of patterns are taken into consideration. That's kind-of intentional: users should not expect too much smart in this analysis. Only the most obvious patterns are recognized. > All in all, even though it makes > sense semantically, the field analysis feels a bit paranoid to me. There is > always a trade-off between verbosity and strictness here. Guarding the same > field multiple times within the same method should only be necessary in > exceptional multithreaded cases. No need to perform multiple checks. The most powerful solution is via bug 398995. I hope, you'll like it. > Given the current impl I'd prefer to be > able to exclude fields from null analysis completely or at least to switch > to a less strict setting. We've discussed various options to balance strictness and convenience. If you have an idea how to reduce paranoia without turning the analysis into a mere guessing-game, feel free to propose your scheme. After all we discussed so far, I'm convinced that the current solution is the best scheme available, but we might have missed s.t. in our discussion. In case you haven't seen it, much of the genesis of the current scheme can be found in http://wiki.eclipse.org/JDT_Core/Null_Analysis/Beta I made first experiments in this direction, but there are a few design decisions to be settled, first. My first naive thought was to re-use java.lang.annotation.ElementType to specify where a @NonNullByDefault should apply. In anticipation of JSR 308, however, we should take preparations for more fine-tuning already: - how should a default affect arrays, make it "@NonNull Object[] var", or "Object @NonNull[] var" or "@NonNull Object @NonNull[] var"? - should a default affect type parameters? At all nesting levels? - if the set of locations includes "TYPE_PARAMETER", "ARRAY_DIMENSION", how should these combine with locations like "METHOD", "FIELD"? Also: - do we want separate locations for method return vs. method parameters? - how should defaults with different locations spec be combined (from different nesting levels)? - should it be possible to cancel defaults for one kind of location? I can't make any promises that the discussion of this design can be finalized in time for M6 (API freeze), since the JDT team very much focused on Java 8 right now. For in immediate solution of the original problem I will first focus on bug 400421, which might already fix the bulk of issues for Xtext code? Next, let me mention that inside a code region with @NonNullByDefault it serves a good purpose to explicitly mark all exceptions with @Nullable. Leads me to a next question: would it make a significant difference if "@NonNullByDefault(false)" can also be applied to fields? I know that this increases the verbosity, but comes with two advantages: - more explicit code - new complex design discussion needed, since this extends the existing semantics in a straight-forward way :) See also bug 392245 for Java 8. (In reply to comment #9) > Leads me to a next question: would it make a significant difference if > "@NonNullByDefault(false)" can also be applied to fields? Yes, definitely. This would be useful when e.g. using a JSON parser to convert untrusted JSON data to POJOs so that deviations in the data from the expected structure leaves those fields in the POJO null. The next stage after that could then use @NonNullByDefault(true) in its fields while it semantically validates the data in the POJO. Given we have two votes in this bugs, are these votes for the following proposal? (In reply to comment #9) > ... would it make a significant difference if > "@NonNullByDefault(false)" can also be applied to fields? I don't see us doing more here, given that a more complete solution exists for Java 8 (which cannot directly be backported for SE5 annotations). . Given that @NonNullByDefault v2.x has this configurability, I don't see sufficient motivation to work on this for Java 1.7. @Voters, LMK if this is still relevant / unresolved for you. *** This bug has been marked as a duplicate of bug 392245 *** |