@NullMarked (and its negation) includes module and package targets

[kevinb9n]

Filing this so we can punt on it. [edit: haha no]

Trying to accommodate package- or module-level defaults is fraught with issues, so for now, we are going to require an annotation like @DefaultNotNull to appear on every class.

We know people aren't going to like this. So this issue is about continuing to try to figure something out eventually.

[kevinb9n]

Note that if we solve #35 then we can at least keep it down to just ONE annotation people put on every class, like @MyPreferredDefaults.

[cushon]

fraught with issues

To start enumerating those:

• package-info doesn't work well with split packages: a package-info on the classpath applies to all classes in that package on the classpath, regardless of which jar those classes were loaded from. So it's easy to forget to include a package-info on a classpath if a package is split across multiple jars, or to accidentally change the defaults for a library that shares a package with another library that defines a package-info.

• module-info solves the split package problem (packages and modules have to be 1:1), but it will be a long time before module adoption is widespread enough for a solution that depends on modules to be sufficient.

[kevinb9n]

It is also very uncomfortable that a package-info present at compile time may be different or missing when a dependent is compiled, and perceived nullabilities of symbols can change. We want class files to be more self-contained than that.

[PeteGillin]

There's also the refactoring issue: if you move a class to a package with a different package-level default then you have to remember to add a class-level default to preserve the semantics.

[kevinb9n]

Very good one. It's bad enough when moving a class causes or removes warnings in that class itself, but when it changes the meaning of things in that class as seen by other classes, that's pretty bad.

[PeteGillin]

(Credit for highlighting the refactoring issue should go to Neil Fuller.)

[kevinb9n]

I will try to pull together a full summary of all the pros/cons for package-level defaults.

[kevin1e100]

I'd like to propose allowing @WantDefaultForEveryClass (or something) on modules and packages, with the following syntactic rule: if this annotation is present then a defaulting annotation must be present on all top-level classes in its scope. This rule I believe addresses concerns around split packages, lost package-info.class files, and refactoring, simply because the intended default is evident in every class, and package-info and module-info files can therefore be ignored in bytecode. I also feel this is nice for readers, as they never have to look outside of a compilation unit to determine if a default is in effect.

A possible variant here is to let people just use one of the defaulting annotations on packages, but require the same or a different defaulting annotation to be placed on very top-level class in scope. I like the economy of avoiding an extra annotation but there could be confusion when the rule about top-level classes isn't followed/enforced.

[donnerpeter]

@kevin1e100 thanks, an interesting approach! It indeed solves the issue of a source file being self-contained.

Will we require all checkers to check this syntactic rule? Then we still must specify what the scope is exactly, and package-splitting issue will remain.

[abreslav]

I like the economy of avoiding an extra annotation but there could be confusion when the rule about top-level classes isn't followed/enforced.

I think our job is to enforce it :) Adding more annotations is not a total show-stopper, but where we can make it clear for the users what's expected of them through making checkers complain, there's no risk of misunderstanding

[abreslav]

Another question that's connected to enforcing: what do we do if the checker encounters a class that violates the "want default for every class rule"?

• Do we complain with an error and then consider it legacy?
• Or complain and just not check it until it has a default?

I think this should be in sync with how we treat illegally annotated entities. Do we have an issue for that?

[kevin1e100]

I haven't seen an issue around what to do when encounterting illegal annotations, so let's make one.

[sdeleuze]

As discussed today, I think it is important to support package-level defaults since it is the only realistic way to set defaults (like the very popular non-null one) with a scope which is wide enough to avoid cluttering the codebase with class level defaults. Ease of use is important even at the prototype stage, and lack of such feature would strongly impact adoption IMO. Defining that only at module level is not an option given the very low adoption.

But it is likely doable to avoid issues by introducing some constraints related to modules which are now widely adopted best practices even by libraries that does not use modules yet. In practice, most of the JVM ecosystem tries to be a good citizen and avoid providing classes with the same package in 2 different JARs. This seems to be a pattern much more widely seen than the module support itself, so I tend to think we could provide that as a rule to follow in the specification.

For the issues related to tests, I think this should not be a showstopper since the scope of the specification is limited to public API. While tests have of course a public API, it is not part of the application or library public API so could maybe be excluded from the scope of the annotations.

[cpovirk]

One concern about package defaults that I thought I remembered seeing was from someone working on incremental compilation -- presumably an IDE, perhaps even IntelliJ? The concern was over recognizing that files in a package should be recompiled when its package-info is changed. I haven't managed to dig up the reference to this, though.

[ edit: maybe #38 (comment) ]

[cpovirk]

If we were to decide against support for package-level defaults, presumably that decision would apply not just to nullness but to all annotations. That might lead to a world in which libraries like Guava end up starting every class like this:

@DefaultNotNull
@MustUseResult

Not to mention common but not universal annotations, which could include:

@Immutable
@GwtCompatible
@PublishParameterNames

I wonder if this leads us down the road of wanting...

@GuavaDefaultAnnotations // defined to imply our preferred set :(

[edit: Oops, I see that that's #35, mentioned above.]

[cpovirk]

Is using compiler flags to set defaults even less attractive than using package-info? (Sorry if this has come up before -- or if it's a really bad idea, as it is just occurring to me as I run out the door :))

[cpovirk]

Oh, compiler flags are probably a non-starter because of IDEs. (They of course also make individual source files not self-contained, though that's true of a package-level default, too.)

[stephan-herrmann]

From scanning this issue I see three problems mentioned:

• Split packages (that's DefaultNotNull and the split-package case #30).
• Solution should not depend on JPMS (but woulnd't it be great if adopters of JPMS could benefit from their effort?)
• Class files should be self-contained

Did I miss any problems?

For the last item I'd like to read some more background / motivation. What tool would need to analyze a class file without the context of its containing artifact? What's the use case?

[kevin1e100]

• Class files should be self-contained

Minor correction: I believe the desire was to have self-contained source files.

There was also a concern around refactorings, i.e., moving source files out of or into a package (or module). This is arguably related to the self-contained source file concern, but refactorings could possibly also be made aware.

Finally there's also the concern of forgetting to include a package-info.java file in a Jar. This can for instance somewhat easily happen in Google's build system, Bazel.

Did I miss any problems?

For the last item I'd like to read some more background / motivation. What tool would need to analyze a class file without the context of its containing artifact? What's the use case?

I think the base requirements document discusses this a bit. The main concern here, I believe, are humans reading source files and being able to understand them without looking elsewhere.

[stephan-herrmann]

Class files should be self-contained

Minor correction: I believe the desire was to have self-contained source files.

Thanks, this concern I understand, but I believe Java is already far beyond the point where all relevant information is explicit in each source file. Most prominently: any use of type inference removes essential type information from the source code.

This is where IDEs help: by providing the elided information on demand. I think it safe to assume that users have gotten used to receiving type information via text hovers, code lenses etc. rather from the source code. By the same token, implicit nullness annotations are integrated into the same existing IDE functionality. In fact, a text hover to show the effective signature of a method will be easier to consult, than even scrolling to the top of a source file to find the nullness regime of this file.

[stephan-herrmann]

There was also a concern around refactorings, i.e., moving source files out of or into a package (or module). This is arguably related to the self-contained source file concern, but refactorings could possibly also be made aware.

Even moving a method from one class to another may require adjustment of nullness annotations. So, yes, this is an issue to be handled by refactoring implementations.

[stephan-herrmann]

Finally there's also the concern of forgetting to include a package-info.java file in a Jar. This can for instance somewhat easily happen in Google's build system, Bazel.

I assume you meant to say package-info.class. This looks like a bug in that build system, no? Given that Java allows annotations on package-info, and given these annotations can have retention longer than SOURCE, not delivering package-info.class results in an incomplete jar. The inconsistence thus is between Java and the build system, not an issue of a particular annotation applied on a package.

[cpovirk]

Allow me to put some words in Kevin's mouth while he's busy at JVMLS and can't defend himself :)

I think his concern isn't so much with Bazel per se, which will always include a package-info.class if you give it a package-info.java, as with build systems that make it relatively easy to define multiple jars in the same package (like, well, Bazel :)):

API_SRCS = [
    "Foo.java",
    "Bar.java",
]

java_library(
    name = "api",
    srcs = API_SRCS,
    visibility = ["//visibility:public"],
)

java_library(
    name = "impl",
    srcs = glob(["*.java"], exclude = API_SRCS),
    deps = ["api"],
)

# And assume that there's a package-info.java file in the directory.

This ends up producing a libimpl.jar with a package-info.class but a libapi.jar without. Bazel users who are careful enough can avoid this by putting the package-info in a separate jar that both jars depend on. But it's reasonable for people to look at this and figure that Bazel is encouraging problems. (And of course that's only become more true in a JPMS world.)

[stephan-herrmann]

Thanks, @cpovirk. That's exactly the information about Bazel that I was missing.

But it's reasonable for people to look at this and figure that Bazel is encouraging problems. (And of course that's only become more true in a JPMS world.)

You may count me as a member of that band.

Put differently, the build issue is a special variant of the split-package problem, nothing that would normally happen, when ppl avoid split packages is the first place, right?

In #30 I argued why I prefer to ship on optimal nullness strategy over helping people to maintain their split packages for many more years to come. Let me add that split packages not only prevent those artifacts themselves from being "modularized", it also renders such libraries unusable in any modularized application, because those would need to consume the library as a set of automatic modules. But artifacts with split packages are illegal also as auto modules.

[cpovirk]

Put differently, the build issue is a special variant of the split-package problem, nothing that would normally happen, when ppl avoid split packages is the first place, right?

Right. Split packages are the problem (as they are elsewhere).

[stephan-herrmann]

Protocol from 2019-08-01 mentions:

Target type MODULE wouldn't be compilable in JDK 8.

Solution use by Eclipse: don't put any @Target on the default annotation, which makes it applicable for all declarations except for type parameters. Proposing to do the same here, too.

[cpovirk]

(cross-reference to #50)

[kevinb9n]

@stephan-herrmann I had never thought of that. That may indeed be a solution. I guess it's either that or use "multi-release jars", which for all I know might not work as well as promised.

[wmdietlGC]

My understanding was that a missing @Target means to apply all ElementTypes that existed in Java 1.5.
This means that a missing @Target is interpreted consistently in Java 5 and all later versions.
This means that ElementType.TYPE_USE and ElementType.TYPE_PARAMETER from Java 8 are excluded, but also that ElementType.MODULE from Java 9 is excluded by default.

I tried this with the following two files:

A module-info.java file:

@pkg.AnnoModule
module Test {
}

and a pkg/AnnoModule.java file:

package pkg;

import java.lang.annotation.ElementType;
import java.lang.annotation.Target;

// @Target({ElementType.MODULE})
public @interface AnnoModule {}

If I run javac module-info.java pkg/AnnoModule.java with the @Target commented out I get an error:

module-info.java:1: error: annotation type not applicable to this kind of declaration
@pkg.AnnoModule
^
1 error

If I add the explicit @Target(ElementType.MODULE) the compilation succeeds.

Reading through https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/annotation/Target.java#L34:

 * <p>If an {@code @Target} meta-annotation is not present on an annotation type
 * {@code T} , then an annotation of type {@code T} may be written as a
 * modifier for any declaration except a type parameter declaration.

I think that comment was not correctly updated when the MODULE declaration location was added.

To me the current behavior of javac seems desirable, because it treats a missing @Target consistently.

However, the javadoc needs to be updated one way or the other.
If people agree with this, I'll file an issue with OpenJDK.

[cpovirk]

IDEA-273004 reports that IntelliJ has some difficulty with @NullMarked on a module because of our use of a multi-release jar.

• @akozlova is aware. Anna and @amaembo, let us know if you think we should reconsider the multi-release jar for this reason.
• It would be a good idea for us to test with Eclipse, NetBeans, and any other commonly used IDEs. If anyone is up for trying, please do.
• We've noted before that the mrjar spec requires that "The public API exported by the classes in a multi-release JAR file must be exactly the same across versions." It's never been completely clear what counts as "API" for that requirement. Quoting myself from a discussion with some Android people:

As I understand it, the different @Target will have no implications on linkage. That may be all that the spec is aiming for: "If you compile against the old version, you'd better run against the new version."

(Sort of support for that: The spec permits a module-info in a per-release directory even if one is not present in the root. Perhaps they consider the module to not be "exported by the classes?" The docs are confusing even about this: "A modular multi-release JAR file is a multi-release JAR file that has a module descriptor, module-info.class, in the root at the top." But then: "A multi-release modular [jar] need not have a module descriptor at the located root.")

Here's one quote that suggests that the full details may be unsettled -- but that, even under the strictest interpretation, the limitation is meant to refer to signature equality, which we're preserving:

Every version of the library should offer the same API; investigation is required to determine whether this should be strict backwards compatibility where the API is exactly the same (byte code signature equality), or whether this can be relaxed to some degree without necessarily enabling the introduction of new enhancements that would blur the notion of one unit of release.

I do feel a little iffy about the whole thing, but I've become reasonably comfortable with it. Plus, we have a tolerable (if unfortunate) exit strategy: Just add MODULE to the Java 8 version, too, and tell people they have to avoid using -Werror or compile with a classpath that includes MODULE....

Anyway, I'm happy to hear further thoughts on this, and I've wondered if we should try reaching out to actual JDK developers for their interpretation and/or the world at large for any problems they've encountered.

[cpovirk]

I'm told that it works OK with NetBeans. If anyone has Eclipse already installed, here's how to get a jar to test with:

git clone git@github.com:jspecify/jspecify.git && cd jspecify && ./gradlew

That produces a build/libs/jspecify-0.1.0-SNAPSHOT.jar for you to use.

[edit: Or nowadays, just download https://repo1.maven.org/maven2/org/jspecify/jspecify/0.2.0/jspecify-0.2.0.jar.]

[cpovirk]

Switching to a separate concern about large-scope defaults:

Something reminded me that a Java package or module may contain both Java and Kotlin code. That means that a @NullMarked annotation on those scopes would apply to enclosed Kotlin code, which currently does not contain JSpecify nullness annotations.

• Users can work around that by avoiding using @NullMarked on such scopes for cross-language code.
• Alternatively, they could apply the annotation but also add @Nullable to their Kotlin code wherever they use ?.
• Maybe someday they will be able to apply the annotation but undo it with @NotNullMarked annotations on the Kotlin classes.
• Or maybe by then Kotlin will have begun writing JSpecify annotations.

[cpovirk]

To spell out something that I'm now realizing people have been referring to all along:

While it's true that one obstacle to putting @NullMarked on your module is that you use modules, there's another obstacle: If you're a library author and you want your users to see that @NullMarked annotation, then they need to use modules, too. Otherwise, tools that ask "What module is this package in?" are going to get the answer "the unnamed module," even if they're pulling your code from a modularized jar file. And "the unnamed module" doesn't have the @NullMarked annotations.

[edit: The following is overstated: See the correction in my next post.]

(And as noted above, some people have constructed builds that make it difficult to adopt modules. Often that's because of split packages, but it can also arise with tools that merge multiple jars into one -- and probably in other cases that I haven't thought about, since I'm only generally familiar with all this. Such builds are already taking some risks: A module-level @NullMarked is just another step in making modules "real API.")

I wouldn't call that sufficient reason to not support module-level annotations, but it's something for library authors to keep in mind when deciding at what scope to annotate.

[edit: An attempted GitHub search for annotations with ElementType.MODULE finds no results, but that's suspiciously missing our own annotation and some test annotations that I know of. I trust our search tools for the Google depot more, so I tried searching there... and I don't see any inside Google, at least not in source form. (That is, there could be some in prebuilt jars, as the search doesn't look there.)]

[cpovirk]

Sorry, that was overstated: It's easier for users to "use modules" than I thought: They don't need to write a module-info.java and ensure that all their dependencies are modules, too: They can just put any modules that they do use on the module path and pass appropriate --add-modules flags, continuing to simultaneously use the classpath as normal. I think :) Please correct me if I have that wrong.

[kevinb9n]

(Module-specific topics to #222 now.)

[kevinb9n]

Propose: we will include MODULE and PACKAGE in the targets, and

• When users use modules, these should have relatively well-defined behaviors (and see Spec/artifact issues specific to Java modules #222 for any issues that may arise with this)
• When they don't, many problems can result from using package annotations in this way, but we believe these problems are addressable in various ways (not necessarily filed yet), and don't add up to an argument to actually not support package annotations.

Please view this issue on github and add a comment or even just an emoji reaction! to express your feedback.

[netdpb]

Looks like we have consensus.

[cpovirk]

I made an accidental discovery: I could use @NullMarked on a method even before we added METHOD to its @Target... because it had MODULE in its @Target?!

$ tail -n +1 *.java
==> Anno.java <==
import static java.lang.annotation.ElementType.MODULE;

import java.lang.annotation.Target;

@Target(MODULE)
@interface Anno {}

==> Foo.java <==
class Foo {
  @Anno
  void go() {}
}

$ javac Foo.java Anno.java
# no errors?!

$ javac -version
javac 11.0.13

I do get the expected error with a newer javac:

$ ~/jdk-19-ea+26/bin/javac Foo.java Anno.java
Foo.java:2: error: annotation interface not applicable to this kind of declaration
  @Anno
  ^
1 error

I didn't immediately find a bug about this upstream. [edit: But it looks like it was fixed in JDK 16.]

Anyway, that's fine because now we allow @NullMarked on methods anyway :)

The not-as-fine part is that this same bug also allows @NullMarked on fields, parameters, and local variables (and probably packages, but I didn't check), which we don't want. (It doesn't allow it in type-use-only locations, at least.)

I don't think that rises to the level that should make us actually avoid @Target(MODULE) (even though this isn't the first problem we've seen with @Target(MODULE) :)). But it does give tool authors more reason to write code to explicitly disregard annotations in locations where they aren't expected—and to issue errors when they see source code with such annotations.

[netdpb]

The current, working decision, as of 2022-03-20, is that users can use @NullMarked on modules and packages, and @NullUnmarked on packages, even though there are pitfalls. The pitfalls are common to all annotations used at those levels, however, not specific to JSpecify's.

The argument for changing this decision is that the pitfalls are interesting, including the fact that package-level annotations apply to split packages, including tests, and the fact that module-level annotations apply only when the artifact is installed in the module-path by clients, and not in the classpath.

However, because the pitfalls are not JSpecify-specific, we should allow the obvious application at these levels.

This must be decided before version 1.0 of the jar.

I propose the current 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: Seven 👍; no other votemoji.

[cpovirk]

I agree (and I have given my thumbs up), but I feel obligated call attention to one more pitfall that I don't think came up on this particular thread:

Some users expect for annotations on a package to apply to subpackages, or at least they wonder if that might be the behavior. I expect us to finalize our working decision that annotations do not apply to subpackages (#8). Regardless of what we decide, some users will have different expectations than that. I think that's unfortunate but not a reason to pull back from supporting package-level annotations at all.

Please still see the proposal and vote above!

[netdpb]

Decision: @NullMarked can be applied to modules and packages, and @NullUnmarked on packages.