Allow annotating null-markedness at method/constructor level? about jspecify HOT 31 CLOSED

The current, working decision, as of 2022-05-16, is that @NullUnmarked and @NullMarked are applicable to methods and constructors.

This issue must be finalized before the release of version 1.0 of the annotation JAR.

None of the discussion above has made a strong case for changing that decision, as far as I can tell.

I propose this decision be finalized for 1.0. If you agree, please add a thumbs-up emoji (👍) to this comment. If you disagree, please add a thumbs-down emoji (👎) to this comment and briefly explain your disagreement. Please only add a thumbs-down if you feel you can make a strong case why this decision will be materially worse for users or tool providers than an alternative.

Results: Six 👍; no other votemoji.

from jspecify.

Random point that shouldn't weigh too heavily here (and which I may have made previously somewhere?):

In domains other than nullness, we are likely to have annotations that apply at both the class scope and the method scope. So there could be a tiny argument for supporting that here for "consistency."

One example is @MustUseResult. However, that example is somewhat different: @MustUseResult is (arguably?) an intrinsically per-method concept, not a per-type concept. At the very least, there's no other annotation that it goes along with. (Contrast with @NullExplicit, which goes along with @Nullable.) So maybe consistency between @MustUseResult and @NullExplicit would actually be misleading.

Very minor point in either case -- just trying to get it out of my head.

from jspecify.

Using this annotation on something like an inner class (or a method) could
also be useful but does seem to create some complexity (or at least room
for misunderstanding) if the outer class has type parameters. In
particular, the question becomes how to interpret usages of the outer
class's type parameters in the inner class.

We have a class Outer<T> containing a method m and an inner class Inner (non-static named nested class, or local class in non-static context, including anonymous). Both m and Inner reverse whatever null-markedness Outer has.

Proposal (reacticons appreciated):

1. Every type variable is declared in one and only one place (here, Outer), and the null-markedness of that location is in effect. We should not pretend the same type variable is differently-bounded in one scope vs. another.
2. Also, when any code inside m or Inner forms a new parameterized type of type Outer (so, not using the type variable T, only giving a type argument to the type parameter T, exactly as an external consumer would do), it should again have the same meaning as if Outer were a sibling (not enclosing) class.
3. Task: we should review what kinds of confusion/surprise that might cause and add helpful documentation. Such confusion can also be cited as a good reason to actually complete the migrations you start -- but a less-good reason to delay starting them!

from jspecify.

Now to the overall issue,

Proposal (reacticons appreciated):

Decide affirmative: both @NullMarked and its negation (#127 / #109) may target methods and constructors.

(Records-specific complications should be filed separately.)

More broadly, I'm advocating this principle: features that we expect to help users apply @NullMarked a little sooner, more easily, or less annoyingly should be prioritized highly even pre-1.0. The more adoption we can get before finalizing designs the better.

from jspecify.

AFAIU signatures in one class may not differ by nullability only, not even in Kotlin. Or do you mean signatures in different classes? Then those can be different due to different class-level defaults.

from jspecify.

By "similar" I don't mean "exactly the same" -- suppose they have different names. Suppose I see two methods listed in my IDE index like

foo(Bar a, @Nullable Bar b)
baz(Qux c, @NullnessUnknown Qux d)

When I see that, I think "Probably this is a @DefaultNotNull class, so a and c are @NotNull", or something like that, but I probably don't think about the possibility that the two methods are operating under different defaults.

[EDIT: this may not really constitute an argument against the feature; just part of our advice to users why the feature shouldn't be overused.]

from jspecify.

Thanks for the clarification! Those signatures can still reside in different classes with the same effect, although probably that wouldn't happen often.

I wouldn't add method-level defaults until we have a demand for that.

from jspecify.

I think the use-case we had in mind for method/constructor-level defaults is classes that can be mostly migrated, but have a few methods that need to remain @NullnessUnknown.

Having method/constructor-level defaults requires only a single annotation on those method declarations, instead of having to make sure all parameters/return types (and all type arguments, etc.) are annotated as @NullnessUnknown.

Regarding IDE representations: I would imagine settings to highlight what defaults have been applied and what the effective nullness annotations are. So hopefully this isn't a big concern.

But if we're no longer concerned with this migration use case, I'm fine with leaving out method-level defaults until users request them.

from jspecify.

I think I'm the one that got Kevin to file this bug. I think I'm pretty well convinced by the arguments here. Probably much of my concern was that people would apply @DefaultNullable as a "convenience," which I think would be misguided. But @DefaultNullnessUnknown makes more sense, and my concerns about @DefaultNullable aren't specific to per-method usage, anyway.

from jspecify.

Here's the case where such annotation could be desired (though, again, not necessary). Consider the following java.util.Map method:

static <K, V> Map<K, V> of(K k1, V v1, K k2, V v2, K k3, V v3, K k4, V v4, K k5, V v5,
                               K k6, V v6, K k7, V v7, K k8, V v8, K k9, V v9, K k10, V v10) { ... }

All args and return value are not-nullable. We can annotate it either

static <K, V> @NotNull Map<K, V> of(@NotNull K k1, @NotNull V v1, 
        @NotNull K k2, @NotNull V v2, 
        @NotNull K k3, @NotNull V v3, 
        @NotNull K k4, @NotNull V v4, 
        @NotNull K k5, @NotNull V v5,
        @NotNull K k6, @NotNull V v6, 
        @NotNull K k7, @NotNull V v7, 
        @NotNull K k8, @NotNull V v8, 
        @NotNull K k9, @NotNull V v9, 
        @NotNull K k10, @NotNull V v10) { ... }

Or simply

@DefaultNotNull
static <K, V> Map<K, V> of(K k1, V v1, K k2, V v2, K k3, V v3, K k4, V v4, K k5, V v5,
                               K k6, V v6, K k7, V v7, K k8, V v8, K k9, V v9, K k10, V v10) { ... }

Again, this is an extreme case and adding 21 explicit NotNulls is not a really big problem.

If we have method-level @DefaultNullable, will it affect only the method itself or the overriding methods in subclasses as well?

By the way, I may have two methods defined in different classes within the hierarchy with different nullability defaults. E.g.:

@DefaultNotNull
class Super {
  public void foo(Object x) {}
}

@DefaultNullable
class Sub extends Super {
  public void bar(Object x) {}
}

Now having an object sub of type Sub I see completion options in my IDE like sub.foo(Object) and sub.bar(Object). I can quickly check method JavaDoc or definition, but unless it's explicitly mentioned in the JavaDoc this doesn't give me a clue about an effective nullability. On the other hand, this could be solved on the IDE level: IDE authors may add new features to display somehow an effective nullability.

from jspecify.

It is probably easier to not allow this for now and consider it later (as evidence demands) than vice-versa.

from jspecify.

I consider this question to be back on the table. After we get a revised/improved requirement about incrementality/adoptability I will come argue why I think we might need method/constructor-level defaults at least. We would still persuade users to really try not to leave things that way but use it as a temporary stepping stone only (in full knowledge that the advice won't always be heeded).

from jspecify.

I should point out that, our adoptability requirements notwithstanding, supporting partially annotated files is out of scope for 0.1, so we shouldn't worry about this issue right now.

from jspecify.

By coincidence, I just learned about annotations on Java's forthcoming "records." The impact on our defaulting annotations looks "interesting"....

It sounds like records will let you write:

record Foo(@DefaultNonNull Bar bar) {}

Then Java will effectively "paste" @DefaultNonNull onto the field, getter method, and parameter of the constructor it generates -- or rather, all of those locations where it is permitted. So:

• Suppose that we forbid defaults on methods. Then surely we'll forbid them on fields and parameters, too. That would make @DefaultNonNull useless on records. (Hopefully javac would outright forbid it, but I haven't confirmed.) But it's consistent (which is good), in contrast to...

• Suppose that we allow defaults on methods but not fields or parameters. Then Java will definitely allow the annotation on components in the record header, and it will:

  • copy it to getters: That's presumably as intended.
  • not copy it to fields: That's presumably "wrong" but fairly harmless: The fields are private fields, and there is not any source code that directly accesses those fields, so checkers aren't likely to care about the annotations. (I'm not even sure yet if javac generates getter-implementation bytecode at compile time (which a bytecode-analyzing tool could theoretically look at -- though it could easily exclude such generated code) or if it's all done at runtime with invokedynamic or similar. [edit: It's invokedynamic.])
  • not copy it to the constructor parameter: That's presumably wrong, and it matters: If I thought that I was making Bar non-nullable in my example above, I turn out to be wrong. Worse, the getter says that it's non-nullable, but it's a lie!

• Suppose that we allow defaults on methods and parameters (#128) -- and let's set aside fields, which are less interesting, as discussed above. Then Java will copy the annotation to the constructor parameter, as well. This makes the getter and the constructor parameter consistent, but it opens up the potential confusion discussed in #128.

I would be sad to leave off method-level defaults, but records now have me thinking twice....

[edit: I was probably overreacting. See next post.]

from jspecify.

(Note that record authors can manually declare the getters or constructors themselves, specifying the appropriate annotations. And they can probably just annotate the whole record at once already -- especially since records will appear only in newer code, which we can hope is more likely to get annotated for nullness than legacy code is. So I am probably making too much of a minor issue that arises only if users do something highly weird.)

from jspecify.

I still think it's reasonable for this not to be our top priority, but another quick thought:

Even if we wanted to tell users "Don't expose a half-annotated class to users," I think that method-level defaults could be useful: When I go to annotate a legacy type, it could be nice to be able to annotate a single method or two, deal with the fallout, and then repeat until the whole class is done. Then I would switch to a class-level annotation before submitting my change. This iterative process is probably less intimidating -- and maybe even easier, since I can easily pick out all errors related to a given getFoo() method to address together.

from jspecify.

I still think method scope makes good sense, but I'm really leery about anything that lets @NullMarked go any narrower than that. That really messes with my desire to believe that there's just two different "modes" that my code can be in. I would rather there was a plain old @NonNull type annotation at that point!

from jspecify.

About records: if we have a "scope annotation", and that includes target type METHOD, and then this automatically allows it to be used on a record attribute .... well, perhaps we can at least explicitly specify that it has no meaning when used on a record attribute (or, I'm not sure here, but maybe specify it to be a wrong application of the annotation?) This seems similar to how javac lets you put TYPE_USE annotations in three places that aren't type usages, and we may similarly have something to say about what happens if you do it.

from jspecify.

We could definitely say that @NullMarked on a record component has no meaning to JSpecify. But I looked into what happens if someone compiles code with such an annotation (either by using a tool that doesn't care about JSpecify annotations or by suppressing any tool warnings/errors). It turns out that there will be no way for consumers of the .class file to distinguish it from a record in which someone applied @NullMarked to the accessor method. That is, the following two source files produce essentially identical class files:

import org.jspecify.nullness.NullMarked;

record RecordAnnotation(@NullMarked Object o) {}

import org.jspecify.nullness.NullMarked;

record RecordAnnotation(Object o) {
  @NullMarked 
  public Object o() {
    return o;
  }
}

(I confirmed this with jdk-16-ea+34, which is what I had handy. I just had to edit the java9 copy of NullMarked to apply to METHOD. And this fits with the section on annotations in JEP 395, which I think is the final version, replacing the JEP 384 link I used above?)

We might have to just live with this. But it's something to keep in mind for #128.

from jspecify.

copying a @kevin1e100 comment from #127 (comment) that seems relevant here:

Using this annotation on something like an inner class (or a method) could
also be useful but does seem to create some complexity (or at least room
for misunderstanding) if the outer class has type parameters. In
particular, the question becomes how to interpret usages of the outer
class's type parameters in the inner class. Minimally we'll have to define
that case, but I'm also simply not sure what most users will expect to
happen in that case, and how close we can and should get to that
expectation.

from jspecify.

(TODO: slice off record-specific discussion to its own issue)

from jspecify.

[deleted]

from jspecify.

+1 for affirmative to this question.

As for the broader point, I have a partial disagreement that is resisting my attempts to summarize it here. I'll finish up the doc that I've been working on and share it, hopefully within a few days.

from jspecify.

I'm not strictly against this, but I do wish we could avoid it: the conceptual simplicity of entire classes either in or out seems very appealing.

from jspecify.

In our last meeting we made a "working decision" of "yes" for the time being. We'll proceed with documenting and implementing that way and find out how it goes.

from jspecify.

I had observed in #50 (comment) that bytecode doesn't always fully distinguish between what's done by a field initializer and what's done by a constructor. We avoided the important part of that problem by rejecting @NullMarked on fields, but we're still theoretically vulnerable to part of it by permitting @NullMarked on constructors [edit: and methods].

Fortunately, constructors [and methods] present much less of a problem: Basically any class defined inside a constructor [or method] produces bytecode that identifies it as "enclosed by" that constructor [or method]. Thus, tools that are looking at that class will know to consult the annotations on the constructor [or method]. (Presumably the same would hold true if Java were to permit methods defined directly inside constructors/methods in the future.)

(This is different than the situation with fields: If we see an anonymous class with no enclosing method/constructor, then we probably(?) know that it's defined either in a static (clinit) block or in the initializer of some field, but we don't know which of those it is, and we certainly don't know which field. Thus, we wouldn't know how to react if a class had a clinit method or if only some fields in a class were null-marked. [edit: Oh, and Java offers instance initializers { ... }, too. I forgot about them.])

But wait, not every class defined inside a constructor [or method] produces the bytecode we want:

$ cat EnclosingInLambda.java 
class EnclosingInLambda {
  final Supplier supplier;

  EnclosingInLambda() {
    supplier = () -> new Object() {};
  }

  public static void main(String[] args) {
    System.out.println(new EnclosingInLambda().supplier.get().getClass().getEnclosingClass());
    System.out.println(new EnclosingInLambda().supplier.get().getClass().getEnclosingMethod());
    System.out.println(new EnclosingInLambda().supplier.get().getClass().getEnclosingConstructor());
  }

  interface Supplier {
    Object get();
  }
}

$ javac EnclosingInLambda.java && java EnclosingInLambda
class Enclosing
private java.lang.Object EnclosingInLambda.lambda$new$0()
null

new Object() {} is quite clearly enclosed by the EnclosingInLambda constructor as far as the syntax tree goes. But its bytecode (and thus the java.lang.reflect API) disagrees. [edit: Granted, you could guess that it's enclosed by the constructor on account of the $new in the name... I think? Still, that's only a guess, and it wouldn't help in case of classes with multiple constructors or multiple methods of the same name (i.e., overloads).] So, in theory, a tool that tried to analyze any APIs (or implementation code) declared inside that anonymous class could behave differently based on whether it operated on source code or bytecode.

Fortunately again, this feels like a theoretical problem, unlikely to bite anyone for a very, very long time and unlikely to be a significant problem even when it does.

from jspecify.

Oh, cool, that got fixed:

$ ~/jdk-19-ea+26/bin/javac EnclosingInLambda.java && ~/jdk-19-ea+26/bin/java EnclosingInLambda
class EnclosingInLambda
null
EnclosingInLambda()

See JDK-8215470.

from jspecify.

OK, so the other thought I had here, which is even less important, is that that bytecode for "pure implementation code" doesn't offer us a way to know whether it comes from source code inside a constructor or source code inside an initializer (whether a field initializer or an instance initializer).

Again, this seems much, much, much less likely to matter. Like, is a tool that consumes bytecode really going to differentiate between a Foo[] local variable declared inside a @NullMarked constructor ("an array of non-nullable Foo") a Foo[] local variable declared inside a non-@NullMarked instance initializer ("an array of unspecified-nullness Foo")?

So I'm noting this for completeness and to get it out of my head.

from jspecify.

As another data point in support of allowing @NullUnmarked at the method / constructor level, the NullAway Annotator tool automatically inserts @NullUnmarked at the narrowest scope that removes errors it cannot automatically resolve. We chose to insert @NullUnmarked at the method / constructor level whenever possible, so that the rest of the code in the containing class still gets checked by NullAway (which makes future changes to that other code safer).

from jspecify.

Very good to know that this looks like a valuable escape valve during adoption.

from jspecify.

Decision: @NullUnmarked and @NullMarked are applicable to methods and constructors.

from jspecify.