Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow annotating null-markedness at method/constructor level? [working decision: yes for both directions] #43

Closed
kevinb9n opened this issue Jun 28, 2019 · 29 comments
Labels
nullness For issues specific to nullness analysis.
Milestone

Comments

@kevinb9n
Copy link
Collaborator

It's possible we don't need to. It also has drawbacks, such as when you compare two signatures that look similar without realizing they're very different.

@donnerpeter
Copy link
Collaborator

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.

@kevinb9n
Copy link
Collaborator Author

kevinb9n commented Jul 2, 2019

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.]

@donnerpeter
Copy link
Collaborator

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.

@wmdietlGC
Copy link
Collaborator

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.

@cpovirk
Copy link
Collaborator

cpovirk commented Jul 15, 2019

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.

@amaembo
Copy link
Collaborator

amaembo commented Jul 23, 2019

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.

@kevinb9n
Copy link
Collaborator Author

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

@kevinb9n
Copy link
Collaborator Author

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).

@kevinb9n kevinb9n reopened this May 14, 2020
@cpovirk
Copy link
Collaborator

cpovirk commented May 14, 2020

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.

@kevinb9n
Copy link
Collaborator Author

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.

@cpovirk
Copy link
Collaborator

cpovirk commented Sep 28, 2020

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 (Even smaller scopes for defaulting annotations: PARAMETER and TYPE_PARAMETER #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 Even smaller scopes for defaulting annotations: PARAMETER and TYPE_PARAMETER #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.]

@cpovirk
Copy link
Collaborator

cpovirk commented Sep 28, 2020

(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.)

@cpovirk
Copy link
Collaborator

cpovirk commented Oct 5, 2020

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.

@kevinb9n
Copy link
Collaborator Author

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!

@kevinb9n kevinb9n added the nullness For issues specific to nullness analysis. label Jul 2, 2021
@kevinb9n
Copy link
Collaborator Author

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.

@cpovirk
Copy link
Collaborator

cpovirk commented Jul 16, 2021

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.

@kevinb9n kevinb9n changed the title Allow defaults at method/constructor level? Allow annotating null-markedness at method/constructor level? Jul 22, 2021
@kevinb9n
Copy link
Collaborator Author

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.

@kevinb9n
Copy link
Collaborator Author

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

@netdpb netdpb added this to the 1.0 milestone Oct 13, 2021
@kevinb9n
Copy link
Collaborator Author

kevinb9n commented Jan 28, 2022

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!

@cpovirk
Copy link
Collaborator

cpovirk commented Jan 28, 2022

[deleted]

@kevinb9n
Copy link
Collaborator Author

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.

@cpovirk
Copy link
Collaborator

cpovirk commented Jan 28, 2022

+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.

@kevin1e100
Copy link
Collaborator

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.

@kevinb9n
Copy link
Collaborator Author

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.

@kevinb9n kevinb9n changed the title Allow annotating null-markedness at method/constructor level? Allow annotating null-markedness at method/constructor level? [working decision: yes for both directions] May 16, 2022
@cpovirk
Copy link
Collaborator

cpovirk commented Jul 11, 2022

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.

@cpovirk
Copy link
Collaborator

cpovirk commented Jul 12, 2022

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.

@cpovirk
Copy link
Collaborator

cpovirk commented Jul 12, 2022

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.

netdpb added a commit to netdpb/jspecify that referenced this issue Aug 3, 2022
netdpb added a commit that referenced this issue Aug 8, 2022
* Add `METHOD` and `CONSTRUCTOR` targets to `@NullMarked` (#43).
* Update basic Javadoc for `@NullMarked` and `@Nullable`.
* Add `@NullUnmarked` (#127).
* Add `@NonNull` (#230).
* Add `@Implies` (#35) (and its containing annotation `@Implies.Container`).
@msridhar
Copy link
Collaborator

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).

@kevinb9n
Copy link
Collaborator Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
nullness For issues specific to nullness analysis.
Projects
None yet
Development

No branches or pull requests

8 participants