Community
Participate
Working Groups
Applying the annotations introduced in bug 186342 for inter-procedural null checking would require tremendous efforts if no smart defaults can be selected. A mechanism for defaults should have these properties: - be configurable at all levels of granularity: method, class, package, project. - be applicable to - just method parameters, or - method return, or - both - support cancellation of defaults from an outer scope. - work well with inheritance. NB, that canceling defaults from a super class is not a sound option. Just as in bug 186342, any annotation names used for this mechanism should be configurable.
Some more words on why I believe those defaults should be configurable, despite the fact that research indicates that non-null should actually be *the* default and exceptions can be expressed with a combination of explicit @Nullable and @SuppressWarnings: The basic question is: should nullness be a *boolean* property of each reference type or do we support a three-valued logic (including an 'unknown' value)? This seems to be a fundamental difference between the approaches of FindBugs and Checkers (correct me if I'm wrong, Mike). Rather than deciding one of those approaches is right and the other one wrong, I'd like to support both. In bug 186342 I'm proposing only two annotations (@NonNull and @Nullable), which serves the boolean approach. For those who want the three-valued logic we'd admit reference types *without* any null-annotation. The question now is, how can a project select between the two approaches? I propose to introduce a configurable warning for missing null-annotations. When configured to "ignore" we have the three-valued approach: missing annotation is one of the three values. When configured to "error" we have the boolean approach where every reference type must have a null-annotation. Choose "warning" while migrating from one approach to the other.
CCing Markus in case he wants to comment on the refactorings proposed below. Here's why I believe defaults at different levels of granularity are important: I envision many projects to adopt null-annotations in a long process of migration. If, e.g., "all parameters are considered non-null by default" is switched on globally for a large project, the number of new warnings will be overwhelming and after a few false positives the developers may quickly change back, i.e., disable the default and continue with the customary uncertainty about nullness. With per-package defaults we could support that a project enables the default for one package at a time. So developers could add @Nullable and @SuppressWarnings annotations for one package at a time, which helps to restrict the size of the backlog of warnings requiring investigation. Still within that package there could be (large) classes that are just different in some respect. E.g., a class produced by a generator that doesn't know about our annotations. You'd just want to exclude this class from the default. Note that this is different from adding @SuppressWarnings to the class header: With saying null-defaults=none you would express that clients may not make assumptions about the contract of this class's methods. This is weaker than saying all types in signatures are nullable. With per-package defaults we should be careful to choose whether or not to apply these defaults to "sub-packages", knowing that Java has no concept of sub-packages but developers may want to use namespace prefixes as a grouping mechanism. "Inheriting" defaults from various levels may make code more difficult to read. Firstly, I hope that the tool will be able to help (e.g., show the effective null-annotation in all hovers). More importantly, any non-global defaults should be seen as transitory means to support gradual migration. This is where I see refactoring come into the picture: When a project team is done annotating a package/project, it would be great if the effective annotations could be spelled out into the code, something like an "Inline Null-Annotations" refactoring, which would replace all non-global default settings with explicit null-annotations at individual type references. Also the opposite made by cool: given a class with no applicable default, but with many @NonNull annotations I may want to extract @NonNull as the new default. Or for a class within a scope of @NonNull-by-default but with many @Nullable annotations, there might be situations where extracting those to a class-wide @Nullable-by-default makes sense. The end of migration would be marked by the point where a project switches to "Missing Null-Annotation=Error". Some projects may then choose to also disallow any non-global configuration of defaults.
(In reply to comment #2) > CCing Markus in case he wants to comment on the refactorings proposed below. > [..] Perhaps, it will also be good to have a refactoring option to remove all these annotations from the source code, so that the code can also be easily compiled with javac/others?
> The basic question is: should nullness be a *boolean* property of each > reference type or do we support a three-valued logic (including an > 'unknown' value)? This seems to be a fundamental difference between the > approaches of FindBugs and Checkers (correct me if I'm wrong, Mike). Yes, this is a difference. There are also other, more important differences. For example, the Checker Framework is complete (it finds all errors) while FindBugs is not (even if it issues no warnings, the program may suffer a null pointer exception) As another example, the Checker Framework requires a small number of user annotations whereas FindBugs does the best it can without any user annotations at all.
> You'd just want to exclude this class > from the default. Note that this is different from adding @SuppressWarnings > to the class header: With saying null-defaults=none you would express that > clients may not make assumptions about the contract of this class's methods. > This is weaker than saying all types in signatures are nullable. I suspect that if you do not want to annotate a particular class, then you also do not want to typecheck uses of that class. Typechecking its uses would lead to a huge number of warnings. For the most part, if a class is not annotated, then you want to not only suppress warnings in it, but suppress warnings at all of its uses. You probably want to separate those two notions, though. If a class has its public signatures annotated (but not its private ones or its bodies), then it makes sense to typecheck uses of the class but not the class itself.
(In reply to comment #4) > Yes, this is a difference. There are also other, more important differences. Sure, my point was not to give a full comparison of tools, which I probably cannot :) > For example, the Checker Framework is complete (it finds all errors) while > FindBugs is not (even if it issues no warnings, the program may suffer a > null pointer exception) So, traditionally the JDT is closer to FindBugs than to the Checker Framework. I'm really curious how far we can push the tool towards complete analysis. For the final step we certainly need JSR 308, which we can't yet include in a release, but I hope we'll get there eventually. (In reply to comment #5) > I suspect that if you do not want to annotate a particular class, then you > also do not want to typecheck uses of that class. Typechecking its uses > would lead to a huge number of warnings. For the most part, if a class is > not annotated, then you want to not only suppress warnings in it, but > suppress warnings at all of its uses. You probably want to separate those > two notions, though. If a class has its public signatures annotated (but > not its private ones or its bodies), then it makes sense to typecheck uses > of the class but not the class itself. This is what I was referring to in my recent comment bug 186342 comment 50. Warnings due to the use of unannotated methods/variables can be configured (as one of ignore/warning/error) separately from all other diagnostics. As bug 186342 is converging towards an initial release I hope it doesn't block any future coolness, and I'll soon move towards this bug. Do any solutions already provide a default mechanism as requested in this bug? What should we learn from others?
The premise of this feature request is: > Applying the annotations introduced in bug 186342 for inter-procedural > null checking would require tremendous efforts if no smart defaults > can be selected. Where is the evidence regarding the "tremendous efforts", and the need for configuration at all levels of granularity? The Checker Framework has such a mechanism (@DefaultQualifier) that can be applied at a variety of granularities. Based on a poll of users (http://groups.google.com/group/checker-framework-discuss/browse_thread/thread/eb47b1c6ce406fa7), few or no developers use this mechanism. So, the mechanism wasn't really necessary. At least, that is the experience with the Checker Framework. For libraries that are not yet annotated, suppressing warnings seems more appropriate than writing annotations or defaults that are known not to be correct. So, I propose that this issue be closed, at least until there is evidence that the feature is needed.
I have a prototype implementation of this feature, by supporting two additional annotations, say @NonNullByDefault and @NullableByDefault. As in bug 186342 the concrete (qualified) names of these annotations are configurable. The annotations can be applied to types and packages (using package-info.java). The meaning is that any type reference missing an annotation will be considered as NonNull if the former annotation is used or as Nullable using the latter. Currently, only type references in method signatures are affected. Semantically, the following precedence rules are relevant: - Explicit annotation has precedence over default. - Inner default has precedence over outer (e.g., type over package, inner type over outer type). - Inheritance has precedence over default, i.e., when overriding a method with existing null contracts this contracts also apply to the overriding method, regardless of any defaults specified in scope. - in this rule the contract of the overridden method may be established by using a default applicable in the scope of the super type. No difference is made, whether the super contract is explicit or via default, it will always apply to the overridden method, too. (this doesn't preclude compatible explicit redefintions) When playing with my prototype on annotating the source code of JDT/Core the default mechanism is very helpful for working, e.g., on one package at a time, since enabling NonNullByDefault for the whole project would create thousands of new errors and warnings - too much to resolve in one step!
Some technical issues of my prototype: Using package-info.java for annotating a package works fine inside the compiler, but I found that model and dom explicitly exclude package-info.java from conversions. This is problematic, e.g, for quickfixes, which convert types from model elements. In this situation, the package annotations are not visible and thus any errors/warnings caused by package level defaults will not be visible, too. As a result, quickfixes can be applied only in the editor, not from the Problems view. This could easily be solved by including package-info.java in conversion. The AST has another problem regarding calls to methods with null contracts: Consider class A with a call someB.someMethod(null) and classes B and C: class C { void someMethod(@NonNull arg) { } } class B extends C { void someMethod(arg) {} // null contract is inherited } When creating the AST for class A, we currently don't see that someMethod has a null contract. My current implementation transfers the null contract from super to sub during verifyMethods(), however, the class B is accepted calling only completeTypeBindings(), but with no further processing. OTOH, the inherited contract is actually stored in the .class file for B, which is, however, not used during AST creation. At this point I'm unsure what is the best approach with minimal impact on performance to create AST for A with knowledge about B's null contract. I wouldn't want to rebuild override-analysis already during resolveTypesFor, hence my approach of extending verifyMethods().
(In reply to comment #9) > Some technical issues of my prototype: ... You can eliminate the problems that you mention by trimming down on the number of ways that can be used for controlling default nullity. (Not only will this simplify the tooling implementation but it will also make it easier on end users who have to remember the rules for resolving what the default nullity is). E.g., JmlEclipse has only two mechanisms for controlling default nullity: - A compiler option (which can be project specific). - @NonNullByDefault and @NullableByDefault but these can _only_ be applied to top-level types. These two have been sufficient for our purposes, including when we went through the exercise of annotating part of the JDT core a few years back. Note that, in particular, there is no inheritance of defaults. With these rules, a developer (end-user) need only keep in mind what the compiler (project) default nullity is and then, for any specific file under investigation look at the annotations of the top-level type. This means that there is always an *explicit visual cue* in a .java file if the nullity for top-level types is any different from the compiler (project) default. Hence, we have found that restricting default nullity annotations to top-level types offers just the right level of granularity in controlling the default.
Hi Patrice, thanks for commenting. (In reply to comment #10) > (In reply to comment #9) > > Some technical issues of my prototype: ... > > You can eliminate the problems that you mention thanks, but the issue with package level annotations should be fixed anyway (it also applies to @Deprecated annotations), and I do have a patch almost ready for that part, so technical reasons should be no real hindrance :) > by trimming down on the number > of ways that can be used for controlling default nullity. (Not only will this > simplify the tooling implementation but it will also make it easier on end > users who have to remember the rules for resolving what the default nullity > is). I was thinking about visibility, too. Frankly, for large classes the class- level annotation can also be quite far away from your focus of work. I think it would actually be great to let the hover of any method show the *effective null contract*, taking into account defaults & inheritance. Wouldn't that be the most effective visualization? > E.g., JmlEclipse has only two mechanisms for controlling default nullity: > > - A compiler option (which can be project specific). > - @NonNullByDefault and @NullableByDefault but these can _only_ > be applied to top-level types. > > These two have been sufficient for our purposes, including when we went through > the exercise of annotating part of the JDT core a few years back. But in your article you wrote that you also used a script to apply the default annotations to a set of classes, didn't you? Would you say, package level annotations are bad and should be avoided, or is it more like saying they're not strictly necessary? During my very first experiments I was quite courageous and turned on nonnull-by-default for the whole project jdt.core. The number of errors and warnings was overwhelming. In my current experiments I actually applied the default to a (large) package and just by automatically applying some quickfixes to all occurrences of a particular problem I could reduce errors/warnings to a manageable number. To me this feels like package level *could* be the right level of granularity for migration, at least in some situations. > Note that, in particular, there is no inheritance of defaults. To double check I understand you correctly, let's use an example: @NonNullByDefault class A { Object foo(Object arg) { ... } } class B extends A { @Override Object foo(Object arg) { ... } Object bar(Object arg) { ... } } @NullableByDefault class C extends B { Object bar(Object arg) { ... } } I'd interpret this spelled out as follows: A @NonNull Object foo(@NonNull Object arg) B @NonNull Object foo(@NonNull Object arg) Object bar(Object arg) C @Nullable Object bar(@Nullable Object arg) I.e., in my understanding, the @NonNullByDefault annotation is not inherited (so B#bar has no null-contract), but the effect it has on foo() must be inherited to ensure compatibility, right?
Hi Stephan, (In reply to comment #11) > ... > thanks, but the issue with package level annotations should be fixed > anyway (it also applies to @Deprecated annotations), and I do have a > patch almost ready for that part, so technical reasons should be no real > hindrance :) Ok. > I was thinking about visibility, too. Frankly, for large classes the class- > level annotation can also be quite far away from your focus of work. > I think it would actually be great to let the hover of any method show > the *effective null contract*, taking into account defaults & inheritance. > Wouldn't that be the most effective visualization? A hover would be a great idea. (Though the hover / default-nullity issues are somewhat orthogonal: we still sometimes review Java code outside of Eclipse -- though it maybe more rare nowadays). > But in your article you wrote that you also used a script to apply the > default annotations to a set of classes, didn't you? Yes. One could image a quick-fix-like feature in Eclipse to help emulate such a script. The advantage of such a script/quick-fix is that it helps keep the semantics (rules) related to default nullity as simple as possible. > Would you say, package level annotations are bad and should be avoided, > or is it more like saying they're not strictly necessary? Use of package-level annotations isn't common (as is partly confirmed by the fact that you are working on a bug related to this :) so I would shy away from their use (more below). > During my very first experiments I was quite courageous and turned on > nonnull-by-default for the whole project jdt.core. The number of errors > and warnings was overwhelming. Yes, I can imagine. > In my current experiments I actually applied > the default to a (large) package and just by automatically applying some > quickfixes to all occurrences of a particular problem I could reduce > errors/warnings to a manageable number. To me this feels like package > level *could* be the right level of granularity for migration, at least > in some situations. It could be, and in support of this why not instead have a quick-fix which adds top-level nullity annotations to the types in a package? Our reasoning in JML was that there are enough complexities in Java, that our prime objective was to keep the semantics related to default nullity as simple as possible. In the end, I am not *against* package-level annotations, I am just in favor of a simpler default nullity semantics ;). > > Note that, in particular, there is no inheritance of defaults. > > To double check I understand you correctly, let's use an example: > > @NonNullByDefault class A { > Object foo(Object arg) { ... } > } > > class B extends A { > @Override > Object foo(Object arg) { ... } > Object bar(Object arg) { ... } > } > > @NullableByDefault class C extends B { > Object bar(Object arg) { ... } > } > > I'd interpret this spelled out as follows: > A > @NonNull Object foo(@NonNull Object arg) > B > @NonNull Object foo(@NonNull Object arg) > Object bar(Object arg) > C > @Nullable Object bar(@Nullable Object arg) > > I.e., in my understanding, the @NonNullByDefault annotation is not inherited > (so B#bar has no null-contract), but the effect it has on foo() must be > inherited to ensure compatibility, right? No. Nullity annotations are not inherited in JML, be they regular or the ByDefault annotations --- and this has been the case for as long as I can remember (and possibly adopted from Larch/C++ during JML's inception). In JML, we prefer to force the need for an explicit visual cue in the file by requiring either explicit annotations in B#bar or B itself. If you choose to support "inheritance" of default nullity annotations, would it not then become natural for end-users to feel that normal nullity annotations would be inherited too (for sake of uniformity)? (Of course, I feel that neither inheritance should be allowed. :) Returning to your example, in JmlEclipse we would apply the two rules mentioned in comment #10. Thus assuming that the default nullity is nullable, then the absence of any nullity annotations in B would leave B#foo as @Nullable Object Foo(@Nullable Object).
(In reply to comment #12) > Yes. One could image a quick-fix-like feature in Eclipse to help emulate > such a script. Would you (or anyone reading this) have an idea what gesture should start that operation? For a quick-fix we need an element requiring a fix. A quick-assist works without a problem but still: where? Would it be OK to make this a quick-assist of a package declaration? Or what? > The advantage of such a script/quick-fix is that it helps keep the > semantics (rules) related to default nullity as simple as possible. Only on second reading of comment 10 I see the other simplification: No defaults per nested types. Given that Mike suggested to completely drop this issue, the simpler approach might actually be fine. Let's see if others, too, have a say on this. > Our reasoning in JML > was that there are enough complexities in Java, that our prime objective was > to keep the semantics related to default nullity as simple as possible. Sounds almost like lexical scoping were a horribly complex issue :) but I got your point. > No. Nullity annotations are not inherited in JML, [...] Oops? Given that other contract elements are inherited in JML (correct me if I'm wrong) this looks rather odd to me, and I actually assumed otherwise. > If you choose to support "inheritance" of default nullity annotations, would it > not then become natural for end-users to feel that normal nullity annotations > would be inherited too (for sake of uniformity)? Sure, sure. In my implementation all null annotations are inherited. I was essentially motivated from my Eiffel background (which is similar to how I remember requires/ensures in JML). Forcing people to repeat contracts from the super-method sounds like a waste of their time, to me. I'm not against a refactoring for making inherited contracts visible, but I would consider that as optional. > Returning to your example, in JmlEclipse we would apply the two rules mentioned > in comment #10. Thus assuming that the default nullity is nullable, then the > absence of any nullity annotations in B would leave B#foo as @Nullable Object > Foo(@Nullable Object). I.e., in JmlEclipse my example would be illegal, because B#foo tries to redefine the return type from @NonNull Object to @Nullable Object, even without mentioning @Nullable?
(In reply to comment #12) > Yes. One could image a quick-fix-like feature in Eclipse to help emulate > such a script. For the time being the following patterns work sufficiently well when applied to a selected resource, like a package or a source folder: Replace: ^public (.*)class\b With: @myannotationpackage.NonNullByDefault\npublic \1class [x] Regular expression Not extremely user-friendly, but allows us to go on and to defer ease of use considerations till later.
After discussions at EclipseCon Europe Markus and me agreed on the following approach: 1. The option to set @NonNull as the default is essential for the approach. 2. we don't want to recommend @Nullable as the default, so we drop this option for now; discussion will only be re-opened when we see evidence that this is a useful thing to have in real life. 3. we do want to ease the combination with legacy (unannotated) code. When subclassing a legacy class (or implementing a legacy interface) neither @NonNullByDefault nor @NullableByDefault are useful for overriding and implementing methods. The former makes parameters incompatible with the super declaration, the latter will cause undesirable warnings regarding the method return. We conclude that the most consistent way for such methods is to use the same legacy (unannotated) signature as the super declaration. This also allows the method implementation to handle nulls as *intended* by the super method, allowing the method implementation to remain unchanged when the super-method will eventually be annotated accordingly. If, however, this code sits within a scope where @NonNull is the default, the only way to declare a legacy signature is by cancelling the default. In order to support the above we agreed on: @NullableByDefault is no longer part of our strategy. @NonNullByDefault (or @NonNullByDefault(true)) to support 1. @NonNullByDefault(false) to support 3 (i.e., cancelling a default). i.e., the annotation has a boolean parameter which defaults to true. To enable the minimal useful scope for @NonNulByDefault(false) I suggest that this annotation be applicable also to individual methods (useful for those methods overriding/implementing a legacy method). Ie.: @Target({PACKAGE,TYPE,METHOD}) Accordingly, a compiler preference will support to define @NonNull as the global (project) default, but not the opposite. Additionally still, defaults may cause annotations to be redundant either when an explicit annotation repeats the default or when default-annotations are nested. We will raise a warning against these redundant annotations to provide a handle for quickfix/cleanup for removing all redundant annotations.
(In reply to comment #15) > After discussions at EclipseCon Europe Markus and me agreed on the > following approach: Sounds good! > Additionally still, defaults may cause annotations to be redundant either > when an explicit annotation repeats the default or when default-annotations > are nested. > We will raise a warning against these redundant annotations to provide a > handle for quickfix/cleanup for removing all redundant annotations. We can take this into a separate bug. Redundant annotations may not be just because of repeating a default, but also when they're applied to a method with return type 'void' or any base type like int, char, etc., no?
(In reply to comment #16) > > Additionally still, defaults may cause annotations to be redundant either > > when an explicit annotation repeats the default or when default-annotations > > are nested. > > We will raise a warning against these redundant annotations to provide a > > handle for quickfix/cleanup for removing all redundant annotations. > > We can take this into a separate bug. Redundant annotations may not be just > because of repeating a default, but also when they're applied to a method with > return type 'void' or any base type like int, char, etc., no? I already implemented the better part of detecting redundancy on the train back home from EclipseCon :) As for '@Nullable void' and friends I wouldn't call the annotation redundant, but rather misplaced or not-applicable, right? I can easily add a check for that, too.
Stephan, I think the proposals here have already been implemented in the patch part of the bug 186342. So, unless you want to extract the part relevant to this bug and put in a separate patch, we should just close it as a dup of 186342.
Released for 3.8 M4 as part of commit http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=305123b230bcfd1f733969b7cd2c687b75857ff0 on behalf of bug 186342.
Verified for 3.8 M4 using Build id: I20111202-0800
