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

Scope of package-level default annotations #8

Closed
dzharkov opened this issue Nov 26, 2018 · 16 comments
Closed

Scope of package-level default annotations #8

dzharkov opened this issue Nov 26, 2018 · 16 comments
Labels
design An issue that is resolved by making a decision, about whether and how something should work. nullness For issues specific to nullness analysis.
Milestone

Comments

@dzharkov
Copy link
Collaborator

dzharkov commented Nov 26, 2018

The question was initially raised by @amaembo:

If it’s applied to the package, I suggest that it should not be applied to any subpackages, so it’s required to write DefaultNotNull once per every package.

cpovirk edit much later:

(See, for example, this question, these feature requests (IntelliJ, JDK, jOOQ), these StackOverflow questions (1, 2), and this behavior in the Checker Framework (and I want to say maybe some behavior in Spring?).)

@dzharkov
Copy link
Collaborator Author

dzharkov commented Nov 26, 2018

Comments from the document:

[@kevin1e100] agreed, I do mean the annotation to only apply to the package. I’ll make that explicit.
[@donnerpeter] some people complain about having to add these annotations to every package (example)

@donnerpeter
Copy link
Collaborator

Additionally, it's not clear how the scope is applied across module dependencies. E.g. if module A depends on B, both have the same package, which is annotated with a default annotation, should A get it as well? What if B is a library jar, not a source module?

@kevinb9n
Copy link
Collaborator

Naively I would suggest that no defaulting mechanism should ever affect code in different modules under any circumstances.

@kevinb9n
Copy link
Collaborator

To the original question, what are the advantages of either approach? (direct package only vs. subpackages in same module)

@kevinb9n kevinb9n added the nullness For issues specific to nullness analysis. label Nov 26, 2018
@kevin1e100
Copy link
Collaborator

Because there is technically no hierarchy of Java packages (all packages are independent from each other) my intention was for @DefaultNotNull on package a.b.c to not affect "sub-"packages a.b.c.{d,e,f.g} etc. I tried to make that clearer in the proposal writeup. Please reopen if you disagree.

@kevinb9n kevinb9n changed the title Scope of DefaultNotNull annotation Scope of package-level default annotations Oct 30, 2019
@NicklasWallgren
Copy link

I think there should be an option to make subpackages inherit the NullMarked annotation. What other alternatives are there for projects not utilizing JPMS?

@cpovirk
Copy link
Collaborator

cpovirk commented Jan 12, 2024

The current approach has the advantage that there's no chance of confusion if a superpackage from another module is @NullUnmarked but the current module is @NullMarked: Because we never look at superpackages, it's clear that the current module's setting "wins."

This also matches Java's definition of "enclosing elements," though I admit that I'm not sure how significant that usually is above the package level.

This current approach can be inconvenient for users (as we've seen inside Google, for example), and we hope that some tools will offer a way to enforce that packages or classes are annotated with @NullMarked where desired.

@agentgt
Copy link

agentgt commented Jan 12, 2024

I will just add to what @cpovirk in that there is no such thing as a "subpackage" or "super package" in Java. The facilities to apparently navigate package trees at runtime in things like Spring are not really reliable in a compiler setting.

It is sad that JPMS solves the problem but the uptake of it has been so poor. I'm perhaps overly optimistic but I'm hoping for a future for where that isn't the case.

In an open source library IMO there is no excuse to not have package-info.java but I feel the pain for applications.

@cpovirk one possible solution is to put something in META-INF similar to Automatic-Module-Name but I'm not sure how reliable retrieving that information is across tools.

@cpovirk
Copy link
Collaborator

cpovirk commented Jan 12, 2024

Interesting, I don't think we'd considered META-INF. One concern would be that it's theoretically possible to consume directories of class files instead of jars, and (I assume??) tools won't look for META-INF unless there's a jar. Probably lots of people do always consume jars, though, so it's less of a risk. My concern then would be tools that, e.g., build fat jars: It's unlikely that everything would "just work" when merging together the META-INF data from multiple jars. Or who knows, maybe it would if we could invent a scheme like META-INF/null-marked-packages/com.foo? I've never looked into that.

I guess there's also the principle that we want for the nullness specifications to be clear from the Java source files, without need to look at separate configuration files. (Ideally, we want it to be clear from looking at only the Java source file that you care about. But obviously we've felt it necessary to compromise there a little by supporting package-level and module-level defaulting at all. Even that leads to issues sometimes, since people will put @NullMarked on a a package but not want it to apply to the tests that they put in the same package... :) There are really no great options as far as we've been able to tell.)

@agentgt
Copy link

agentgt commented Jan 12, 2024

I'm not really a fan of the META-INF solution either. I have had some users particularly Spring users complain about the fact my library requires annotations on each package or modules.

My preferred solution as @dsyer can tell you is JPMS (its an ongoing joke we have that when this comes up I just say use modules 😄 ).

jstachio/jstachio#69

@agentgt
Copy link

agentgt commented Jan 12, 2024

Even more why I don't think META-INF is a problem is that most IDEs can auto create package-info.java with a template.

In fact Eclipse (I'm like 1 of the 10 only people left that use it) will if null analysis is turned on warn you if miss not putting a NullMarked annotation of your choosing on a package-info as an option. Search for Missing @NonNullByDefault annotation on package in the preference window.

@NicklasWallgren are you using Eclipse or IntelliJ out of curiosity?

@cpovirk
Copy link
Collaborator

cpovirk commented Jan 12, 2024

Thanks for the link, which I admit I only skimmed after seeing you mention runtime concerns. So now, yikes, I'm afraid of runtime questions when looking for parent packages: If my classloader loads a.b.c and a but another classloader loads a.b, then do we inherit from a or from a.b? This seems like further evidence that we'd be swimming upstream if we tried to make subpackages "a thing."

@agentgt
Copy link

agentgt commented Jan 12, 2024

I think once JSpecify is released the tooling in IDEs will get better and enforce package-info.java on null analyzed project which IMO is the best solution other than modules.

That is something like IntelliJ can just auto turn on create package-info if null analyzed project.

@kevinb9n kevinb9n added the design An issue that is resolved by making a decision, about whether and how something should work. label Mar 13, 2024
@kevinb9n kevinb9n added this to the 0.3 milestone Mar 27, 2024
@kevinb9n kevinb9n changed the title Scope of package-level default annotations Scope of package-level default annotations [working decision: not subpackages] Apr 10, 2024
@kevinb9n kevinb9n added the needs-decision-before-spec-1.0 Issues needing a finalized decision for the 1.0 specification label Apr 10, 2024
@kevinb9n
Copy link
Collaborator

This thread has covered a lot of ground, but my impression is that the decision it represents is specifically that a package-level annotation will affect only that package (in that module) and no "subpackages" (which are not even a real concept in Java). If there was any other significant/debatable aspect please file separately.

@netdpb
Copy link
Collaborator

netdpb commented Apr 24, 2024

Current decision: JSpecify annotations on a Java package affect only symbols declared in that package, and not in "subpackages" (packages with a prefix that matches the annotated package).

Argument for changing: It might be convenient for authors to annotate a number of packages with such a mechanism. However, Java does not consider packages to have any enclosing relationship with each other. As an alternative, module-level annotations do provide a way to apply an annotation to symbols in more than one package.

Timing: This must be decided before version 1.0 of the spec.

Proposal for 1.0: Finalize the current decision. 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: Three 👍s; no other votemojis.

@netdpb
Copy link
Collaborator

netdpb commented Jun 13, 2024

Decision: JSpecify annotations on a Java package affect only symbols declared in that package, and not in "subpackages" (packages with a prefix that matches the annotated package).

@netdpb netdpb changed the title Scope of package-level default annotations [working decision: not subpackages] Scope of package-level default annotations Jun 13, 2024
@netdpb netdpb removed the needs-decision-before-spec-1.0 Issues needing a finalized decision for the 1.0 specification label Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design An issue that is resolved by making a decision, about whether and how something should work. nullness For issues specific to nullness analysis.
Projects
None yet
Development

No branches or pull requests

8 participants