Skip to content

Document how to declare that your JPMS module uses JSpecify #495

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

Open
xenoterracide opened this issue Mar 14, 2024 · 28 comments
Open

Document how to declare that your JPMS module uses JSpecify #495

xenoterracide opened this issue Mar 14, 2024 · 28 comments

Comments

@xenoterracide
Copy link

xenoterracide commented Mar 14, 2024

I'm not certain what libraries should do as far as their own jpms

should I have

import org.jspecify.annotations.NullMarked;

@NullMarked module com.xenoterracide.jpa {
  requires transitive static org.jspecify;
}

or

import org.jspecify.annotations.NullMarked;

@NullMarked module com.xenoterracide.jpa {
  requires static org.jspecify;
}

I would generally think the second, but then I'm constantly plagued by warnings from java of annotations at compile time that are missing, or missing dependent fields of. Is static actually appropriate? I notice that the annotations are marked as "runtime" but I don't understand the use case for that.

@SentryMan
Copy link

transitive and static cannot both take effect at the same time

@xenoterracide
Copy link
Author

xenoterracide commented Mar 15, 2024

is that true? I would think it'd be roughly equivalent to compileOnlyApi in gradle... it doesn't result in a compile error. I can't find anything that says either way.

https://www.oracle.com/corporate/features/understanding-java-9-modules.html

is like, we're going to glaze over that... reading it sounds like it would be optional, but any module, if it's available? would also be able to read it based on using mine... which doesn't sound mutually exclusive.

@agentgt
Copy link

agentgt commented Mar 15, 2024

I think and I’m tired so this is probably wrong

requires static transitive

The sort of reason is JSpecify annotations are retention RUNTIME.

I would say less than 1% of Java devs know this so it is a good point.

edit I will say I have never done requires transitive and static both so maybe there are compat issues or maybe it doesn’t work but iirc it does.

I use the eclipse annotations which are CLASS so requires static is good enough.

@SentryMan
Copy link

SentryMan commented Mar 15, 2024

is that true?

I guess not, but modules that depend on it will need to have jspecify in the dependency tree or they'll get a module not found error.

@agentgt
Copy link

agentgt commented Mar 15, 2024

I’m not sure of that when it comes to annotations. Like if you are just sniffing via reflection which is precisely why you want the transitive for nullness.

That is a third party runtime library trying to deduce if a field to inject is nullable or not.

@agentgt
Copy link

agentgt commented Mar 15, 2024

I wasn't at my computer before but it appears on googling @lukaseder had compat issues with Maven and I believe I did as well: jOOQ/jOOQ#13619

I will need to double check but I believe mine were with Javadoc of all things.

Ultimately I don't think it matters as the annotations are public but I believe requires static transitive is technically correct assuming tooling works in an application. For a library that may not be desired.

Later today I will try experimenting. @bowbahdoe do you have any experience with requires static transitive? I am sorry to ping you but there are not many (like I said I wager 1% of java devs are aware of most of the module rules) and at this point I think its more about compat with external tooling.

@bowbahdoe
Copy link

bowbahdoe commented Mar 15, 2024

@agentgt Yeah - i'm not too sure of a good usage for requires static transitive. You need it at compile time and dependent modules need it at compile time, but none need it at runtime.

Annotations that aren't on the module path just become invisible, so its perfectly fine to not require the jspecify annotations downstream. For me, I think thats interesting/important only when you want to have a zero dependency library. Once you have one dependency might as well have two.

I don't really understand the state of tooling either.

@cpovirk cpovirk changed the title provide JPMS advice in the user guide Document how to declare that your JPMS module uses JSpecify Mar 15, 2024
@cpovirk
Copy link
Collaborator

cpovirk commented Mar 15, 2024

  • Thanks for all the information so far. I don't know who in the group knows the most about the module system, but it is definitely not me. (I still haven't even absorbed the previous discussion on Is it safe to have jspecify be an optional dependency? #389.)
  • Given many experiences with the sort of warnings and errors discussed in this thread, I am always nervous about advising people that they don't really need annotations to be visible, even at compile time. (See the notes about annotations in this part of the Guava wiki.) I would be happy if we could end presenting both "Here is a policy that we're sure is safe" and "Here is what you might be able to get away with if you're trying very hard to avoid dependencies, and here are the associated risks that we know of."
  • I am re-titling this issue to be a little more specifically about how to declare your dependency on JSpecify. I'm doing that because I want to open another issue about what happens if you annotate your module but your users aren't using JPMS, which I have been failing to follow up on for a while now.
  • The main specific purpose of runtime retention is for libraries like Guice that want to reject nulls. The more general reason is that runtime retention has few downsides relative to class retention (which we obviously need), with the main exception being Android (which we largely sidestep because @Nullable is a type-use annotation, which doesn't become part of the Android dex). I am probably forgetting things from Decide retention for our annotations #28, Annotation targets and retentions for nullness annotations #96, and Ensure 1.0 does not preclude supporting both retention levels well later #234, but I could brush up on them if you want to get into this more (on whichever of those threads seems best to you).

@xenoterracide
Copy link
Author

xenoterracide commented Mar 15, 2024

I have one further comment about when annotations currently seem to be required on the compile class path, but I've only for sure seen this coming from things that you use the automatic modules. Since most libraries only use automatic modules currently.

The example would be if spring had a non-null (on my phone) annotation that had a field that supported A Jspecify annotation, then, in my experience spring needs to export in some way jspecify as well or you will get compiler warnings. This only happens of course if you're using that specific annotation from spring. I do not know and haven't tested if this is a problem when full JPMS is used. I can link an issue I opened in spring boot with this recently if it's helpful. Since the solution and Gradle would be to use the API or compile only API dependency scope.

@agentgt
Copy link

agentgt commented Mar 15, 2024

The example would be if spring had a non-null (on my phone) annotation that had a field that supported A Jspecify annotation, then, in my experience spring needs to export in some way jspecify as well or you will get compiler warnings. This only happens of course if you're using that specific annotation from spring. I do not know and haven't tested if this is a problem when full JPMS is used. I can link an issue I opened in spring boot with this recently if it's helpful. Since the solution and Gradle would be to use the API or compile only API dependency scope.

With javac you do not get compiler warnings or errors unless of course you actually access the annotations methods (the methods on the annotation itself). With ecj aka Eclipse Java compiler you will get a warning if you have a generic parameter annotated albeit I believe that is a bug. I can't recall if I have already told Stephan Herrmann about that.

You can actually see this through my current two projects code bases of jstachio and rainbowgum where I constantly have to SuppressWarnings of "exported".

@SuppressWarnings("exports")
public @Nullable /* this one is fine */ Object blah(List<@Nullable /* this one causes warnings */ Object> o) {
}

EDIT I think I misunderstood you. Yes you will need to requires as Spring does not have true modules.

Speaking of which you should never requires transitive any of Springs stuff because they are automatic modules. Which is annoying.

@xenoterracide
Copy link
Author

xenoterracide commented Mar 15, 2024

For clarity on warnings I'm referring to issues like this (linking to reproducer comment) spring-projects/spring-boot#39901 (comment)

spring-core currently has this exact kind of issue with jsr305

@cpovirk
Copy link
Collaborator

cpovirk commented Jul 16, 2024

(further discussion of this on #503)

@cpovirk
Copy link
Collaborator

cpovirk commented Sep 10, 2024

log4j2 has had some kind of trouble with modules and/or OSGi in apache/logging-log4j2#2929 / apache/logging-log4j2#2930, in which they cite bndtools/bnd#2713. I think maybe they're autogenerating their module-info and ending up with requires static transitive, and they're planning to switch to requires static because they don't want JSpecify on their users' classpaths? As usual (like with this recent Gradle discussion), I can't recommend incomplete classpaths, but I at least want to make a note here so that we can find this example later if we want.

@agentgt
Copy link

agentgt commented Sep 10, 2024

My concern with Log4j2 doing that is they may access the JSpecify annotations in the future via reflection.

This is because Log4j2 uses reflection to bind configuration last I checked unlike my own logging project rainbowgum (shameful plug) which generates reflection free configuration binding code using an annotation processor (it is jspecify aware to boot 😄 ).

That is they better not pull the annotation with reflection to check if the option is optional otherwise a JLinked application will fail.

I suppose I should raise that issue to the log4j2 team.

@cpovirk
Copy link
Collaborator

cpovirk commented Mar 21, 2025

Prompted by google/guava#7732 by @jjohannes (thanks!), I am reluctantly trying to learn things :)

It seems very clear that Guava's total lack of a requires line for JSpecify is entirely wrong.

It seems that requires static is a strict improvement upon that.

I would still like to keep investigating whether requires, requires transitive, or requires transitive static is actually the more technically correct approach. (See, e.g., #389.) Even if so, I suspect that each of those carries some downsides relative to requires static.

Relative to having a requires line without static (which, again, is not the state we are in now, since we have no requires line for JSpecify at all), and from what I've read so far, I've gotten the impression that static arguably has at least one small concrete downside:

As a consequence, even if a module is present on the module path (or in the JDK for that matter), it will not be added to the module graph just because of an optional dependency. It will only make it into the graph if it is also a regular dependency of some other module that is being resolved or...

So, if I annotate my library with JSpecify annotations but use requires static org.jspecify, then my users who call getAnnotations() won't see JSpecify annotations unless they declare their own dependency on JSpecify, maybe? That seems unlikely to be much of a problem in practice, but it's a little sad for any tool (e.g., a DI framework) that tries to look for all annotations named "Nullable." (Not that a DI framework should be scanning Guava itself :) So even if static could contribute to problems when using some library, that doesn't mean it would contribute to problems when using Guava.)

As for transitive, it was necessary in some cases when last we heard, but I haven't checked back.

I should probably figure out some javac command lines that I can run that demonstrate some of the different behaviors we can see with the different options.

@jjohannes
Copy link

Just a few thoughts, as I have been fighting with this topic quite a bit recently and opened the Guava PR linked above.

I think what is "correct" is not that clear for when an annotation library (like JSpecify) is used by an OSS library (like Guava), because

  • many users will not need the annotations at compile time because they don't do annotation processing
  • many users will not need them at runtime if the library using the annotations does not add any runtime code that reads them at runtime (I think Guava does not do such a thing).

I think the major "drawbacks" for consumers of Guava (the library that uses JSpecify) who "do not care" are the following:

  1. require static Only the compiler cares. Since it is not transitive, typically it has no effects on consumers of Guava. The only effect for consumers I know of is when they turn on extended linting functionality (see description of Guava PR on the topic). Consumers who do not care about the annotations can choose to not to have JSpecify on any classpath.
    No drawback.
  2. require static transitive Only the compiler cares. Due to the transitive however, the compiler will enforce that the required module is present. This means, if the jspecify.jar is not on the compile classpath, it won't compile. Typically it'll be there, if it is declared in the metadata so that Maven or Gradle will provide it, but who knows what some consumers would like to do.
    Drawback: even if I don't do annotation processing and do not use JSpecify myself, I need it at compile time. (Thanks @ppkarwasz for pointing this out to me recently.)
  3. requires or requires transitive Now also the runtime cares!
    Drawback: Sticking with the Guava example, java will not start any application (as Module) that contains the guava.jar without also having the jspecify.jar on the runtime classpath. Which seems overly strict when there is no runtime code looking at the annotations.

That's why in the case of Guava I would choose option (1).

@ben-manes
Copy link
Contributor

fwiw, Caffeine uses option (1) which was the JPMS team's recommendation. You may want to also suppress a javac exports warning it results in. There were some weird Maven bugs wrt option (2) which I think have all been fixed by now.

@xenoterracide
Copy link
Author

a little sad for any tool (e.g., a DI framework) that tries to look for all annotations named "Nullable."

at the same time them doing this is problematic I have an open bug with errorprone because they are treating jakarta.validation as a nullity annotation in the same way as an annotation like jspecify would be used. So I think that these tools doing this is probably not a good idea.

So, if I annotate my library with JSpecify annotations but use requires static org.jspecify, then my users who call getAnnotations() won't see JSpecify annotations unless they declare their own dependency on JSpecify, maybe?

Do they only need to requires static too? I mean yeah that kind of sucks but it's not super awful, see above point.

@bowbahdoe
Copy link

Do they only need to requires static too? I mean yeah that kind of sucks but it's not super awful, see above point.

Downstream libraries won't need any requirement on jspecify. .getAnnotations() will leave off annotations that aren't on the path(s)

@ppkarwasz
Copy link

So, if I annotate my library with JSpecify annotations but use requires static org.jspecify, then my users who call getAnnotations() won't see JSpecify annotations unless they declare their own dependency on JSpecify, maybe?

Most tools that handle JSpecify will have a dependency on jspecify, which will be on the runtime classpath.

I am not sure about annotation processors, since the annotation processor class path can be separate from the classpath of the compiled application. I suspect however that if the annotation processor depends on jspecify, it will be able to read JSpecify annotations, even if jspecify is not on the compile classpath.

@cpovirk
Copy link
Collaborator

cpovirk commented Mar 25, 2025

Thanks for all the details. I've been trying to use this as an opportunity to motivate myself to understand modules at least a tiny bit, and it seems to be helping. I now can reproduce the -Xlint:classfile warning fairly easily, though that didn't stop me from messing it up a few times first :)

Much as I encourage people to keep the tiny JSpecify jar on their compile-time and runtime classpath, I agree that requiring it at runtime is too strong. So I'm with you on ruling out requires and requires transitive.

I am not necessarily opposed to requiring the JSpecify jar at compile time for users who are building against Guava. But I'd want to do it only if it provided some clear benefit: If a Gradle, Bazel, or Maven user (or a person who runs javac manually) really wants to reduce the compile-time classpath beyond what I'd recommend, it would be hard to justify fighting the user over it. (Notably, the problem I worry about most arises not when JSpecify annotations are missing but when types whose usages are annotated with JSpecify annotations are missing. Requiring JSpecify on the module path does not help with that problem.)

So the big questions are likely around the behavior of reflection and annotation processors.

At the moment, I'm poking around with reflection. As best I can tell, the only way to guarantee that ListenableFuture.class.getAnnotations() returns @NullMarked in a modularized app is to do one of the following:

  • have Guava uses requires or requires transitive (i.e., no static)—the approach that I ruled out above
  • otherwise have the JSpecify module resolved (through a requires line in another module in the app or through --add-modules)

That's arguably not great, but I take the point that code should really query for specific annotations, not everything by a given name. (Hmm, that's probably bad news for @Implies, which really wants to look up all the annotations and then follow their edges!)

I may or may not look into annotation processors. It's another good point that they have their own classpath. It's possible that the module path has little impact there, given that annotation processors at least should operate more in the TypeMirror world, not the Class world.

@ppkarwasz
Copy link

Thanks for all the details. I've been trying to use this as an opportunity to motivate myself to understand modules at least a tiny bit, and it seems to be helping. I now can reproduce the -Xlint:classfile warning fairly easily, though that didn't stop me from messing it up a few times first :)

How did you reproduce it with JSpecify? I can easily reproduce with annotation that have a value (e.g. @BaselineIgnore("1.2.3")), but JSpecify annotations do not accept parameters.

@cpovirk
Copy link
Collaborator

cpovirk commented Mar 25, 2025

Annotation processors can see the annotations fine with requires static, even if they're not on the annotation-processor classpath/modulepath and not on the "normal" classpath/modulepath. (Of course, users may see Cannot find annotation method 'value()' if they use annotations with elements (which the JSpecify annotations do not have) and they omit them from the "normal" classpath/modulepath. (They'll also see that warning if a module has no requires line for the module of such an annotation, as in Guava 33.4.5's error_prone_annotations problem. But that should not be a problem for modules that are built properly.))

@cpovirk
Copy link
Collaborator

cpovirk commented Mar 25, 2025

Oh, yes, sorry, to be clear, I've reproduced it only with annotations that do have parameters. (I was testing with @DoNotMock.)

That does suggest that users who build their modules in unusual ways could get away without even requires static org.jspecify (assuming that we continue to successfully avoid having parameters on any future JSpecify annotations). But I don't think there's any upside to that. And again, it is possible to build such a module only with unusual build setups (like Guava's, which builds the module-info separately from the "real" classes).

@agentgt
Copy link

agentgt commented Mar 25, 2025

My 2 cents is that if you expose public types with JSpecify annotations and you are normal project and not Guava it should be requires transitive. If you don't then it is is requires. If you really know what you are doing aka Guava than its requires static. But requires transitive is what we should recommended.

The above comments with Guava seem to be making a big deal about the .00001% of users concerned with JSpecify being in the deployed application.

Why is that a problem (especially because there are a plethora of workarounds)?

Let me remind you that almost all applications now have SLF4J as a dependency almost always transitively (albeit through build) and the types are not even exported. The JSpecify jar has far less surface area and version change.

Furthermore if you as a user do blanket require static everywhere and all the libraries do as well you will have issues with runtime reflection as jmod will may not package up the jar unless you explicitly tell it to.

I mean its like exporting Optional and then saying we don't actually need that type at runtime. I think this would confuse most users. Lets make the recommended easy path for these users consistent and easy.

For the users or library owners that have complicated reasons why they can't include the JSpecify jar as transitive they can figure that out on their own.

@cpovirk
Copy link
Collaborator

cpovirk commented Mar 25, 2025

I do hope to suggest requires transitive for users in general. (I could even imagine trying to use that for Guava someday, but it would break someone's workflow, so I don't want to further complicate adoption of the modularized Guava.)

I'm sure there's still more that I'll need to learn about all this....

@xenoterracide
Copy link
Author

One thing that might be good advise to users is how to add this for their various spotbugs configuration. At least meet we should make a note that it needs to be. Spot bugs being the only static analysis tool that looks at the runtime code. I have literally seen why having annotations that aren't preserved at runtime cause a problem because it has problems with the Jakarta annotations because they are compiled time only. I'm not actually talking about JPMS here I'm just talking about in general. However I believe that this also means that if Jspecify isn't on the module path for runtime that means it wouldn't pick it up either.

Note: I see no reason this should be requires transient. In fact annotations of any kind should rarely be exposed as public API as they are intended to be consumed reflectively. In other words in order for a consumer to use them you need to open your module in the first place; another thing to consider especially with spot bugs... I don't think spotbugs works with jpms yet... It honestly almost sounds like a problem.

@agentgt
Copy link

agentgt commented Apr 2, 2025

In other words in order for a consumer to use them you need to open your module in the first place;

You do not. Any public class with public methods is visible to reflection provided it is exported (e.g. export some.package.with.publicclass.withjspecifyannotations. This is precisely why you need requires transitive: public methods or types exporting types that are not yours. BTW if you are using modules (and modulepath) and annotate the top level module with NullMarked it is visible as well.

Currently though there is not much enforcement of this and I'm not sure exactly other than JLink what sort of things verify transitive.

Annotations I admit get weird treatment because they do not blindly fail with a class load exception like you would if you returned a type and I guess somewhat to your point it has to mostly be accessed by reflection but the modules do not have to be open (otherwise every templating engine or whatever would not work for java.util.List for example).

If Guava did not have such a complicated history of versioning/dependencies I would recommend they requires transitive full stop because it is a library producing container types that are exported that very well may have annotations that need to be read by some library such as a templating engine or a serialization engine.

That is @Nullable String should be treated like Optional<String> and not exporting @Nullable is like not exporting Optional (java.base is inherently requires transitive on all modules similar to how java.lang is always implicitly imported).

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

No branches or pull requests

8 participants