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

Is it safe to have jspecify be an optional dependency? #389

Open
bowbahdoe opened this issue Jun 2, 2023 · 14 comments
Open

Is it safe to have jspecify be an optional dependency? #389

bowbahdoe opened this issue Jun 2, 2023 · 14 comments
Labels
dev development tasks, build issues...

Comments

@bowbahdoe
Copy link

Apologies, I am bumping up against my knowledge of bytecode compatibility.

If I compile a class or module with nullness annotations, can consumers use that class/module without having jspecify on the class or module path?

@cpovirk
Copy link
Collaborator

cpovirk commented Jun 2, 2023

Thanks, we should write something about this, and I'll keep this issue open until we do.

In principle, it should be safe to omit the annotations jar around at runtime. That said, there have been bugs in this area, like this one fixed in JDK 9 (which we at least documented here, though I just noticed and fixed a mistake in that doc :)).

Note that Maven's provided/optional also omits the annotations jar at compile time, and that can lead to various issues in certain setups. (Other tools, like Gradle and Bazel, have actual ways of setting up "compile-only" dependencies, which would avoid those problems.)

(I'll also add that we're likely to keep to this pace of adding maybe an annotation per year, so we hope that the jar size doesn't become a reason for people to feel the need to omit the dependency. There are, of course, other downsides to having a dependency, though we'd expect those to be minor in most cases, too, especially once everyone is using our jar, anyway :-P)

@bowbahdoe
Copy link
Author

bowbahdoe commented Jun 3, 2023

For my case, I want students who download my jar directly to just have it work.

import org.jspecify.annotations.NullMarked;

@NullMarked
module dev.mccue.json {
    requires static org.jspecify;

    exports dev.mccue.json;
    exports dev.mccue.json.stream;
}
        <dependency>
            <groupId>org.jspecify</groupId>
            <artifactId>jspecify</artifactId>
            <version>0.3.0</version>
            <scope>provided</scope>
        </dependency>

In which case the "cost" of depending on jspecify is low. Obviously i dont have access to any analysis on the nullness annotations, but I wanted to annotate.

But cool that the answer is yes.

@cpovirk
Copy link
Collaborator

cpovirk commented Jun 5, 2023

Sounds good. I'd guess that that will work fine, but there will be no substitute for actually trying it out :)

[edit: cross-link to #234 (comment), which speaks about some of this]

@bowbahdoe
Copy link
Author

Would it be reccomended to use

    requires static org.jspecify;

or

    requires static transitive org.jspecify;

Will that make a difference to the reference checker?

@cpovirk
Copy link
Collaborator

cpovirk commented Jun 7, 2023

Based on my skim of ben-manes/caffeine#535, I would suggest trying requires static transitive, though maybe someday you'd be able to omit transitive without triggering warnings. Really, I wouldn't be surprised if the reference checker doesn't care what you do. But my guess would be that, if it did care, it would prefer having transitive.

@bowbahdoe
Copy link
Author

@cpovirk So I finally got around to actually testing what would happen here.

Take these two modules. The only difference between them is that one requires static jspecify, the other requires static transitive.

json-0.2.4-static.jar.zip
json-0.2.4-static-transitive.jar.zip

(zipped because github doesn't like jars)

When I compile a dependent module without jspecify on the module path, the requires static transitive version fails.

requires static org.jspecify

static

requires static transitive org.jspecify

static-transitive

So it looks like the "correct" way to not have runtime dependence on the jspecify jar is to use requires static, but if you compile with -Xlint:all -Werror like all good children should then only the requires static transitive will work.

This feels like something that should be raised on compiler-dev to see what the best solution is/if this exact category of "type exposed from module is okay iff its just an annotation w/ no enum components" can be accounted for.

@agentgt
Copy link

agentgt commented Aug 1, 2023

In the long run it really should not be requires static but just requires transitive.

I think @cpovirk and I have had similar conversations about dependency and I believe we have the shared opinion that optional dependencies are actually kind of broken in general.

Now the reason requires static is not really right is because you are exporting the types all the time and while intellij does not give a shit I can tell you Eclipse makes complaints all the time about this.

JDT aka Eclipse will eventually stop complaining once it realizes the annotations are being used for null analysis if you turn that on. Technically it should stop complaining if the annotations are not RUNTIME.

Strangely javac does not seem to care (or either that or I don't have the right flags) that you are exporting an annotation type (this is assuming you put some JSpecify annotation on an exported package aka exports).

@bowbahdoe
Copy link
Author

Now the reason requires static is not really right is because you are exporting the types all the time and while intellij does not give a shit I can tell you Eclipse makes complaints all the time about this.

Is this not just an eclipse bug? Java seems to behave properly by hiding the annotations from reflective callers when they aren't present.

it really should not be requires static but just requires transitive.

That doesn't really work when you want someone to not have to use maven infrastructure to use the jar.

Strangely javac does not seem to care (or either that or I don't have the right flags) that you are exporting an annotation type (this is assuming you put some JSpecify annotation on an exported package aka exports).

I've noticed the warnings happening spuriously. Couldn't tell you what causes them and what doesn't at this point.

optional dependencies are actually kind of broken in general.

Isn't the way optional+provided deps are broken is that they don't let you provide a "compile only" dependency? That doesn't feel like it affects this situation?

@agentgt
Copy link

agentgt commented Aug 1, 2023

Is this not just an eclipse bug? Java seems to behave properly by hiding the annotations from reflective callers when they aren't present.

Let us ignore what Java aka Open JDK javac currently does. Javac does lots of incorrect things for TYPE_USE at the moment.

TYPE_USE annotations are part of the type. Think of it has hiding Optional.

That doesn't really work when you want someone to not have to use maven infrastructure to use the jar.

I agree. I'm saying ideally it requires transitive because the JSpecify annotations are retention=RUNTIME that means some one could access them reflectively... regardless of modules... you see when you ask a class for a single annotation it it will try to load all of them. This is one of Micronauts arguments for saving memory is using their compile time annotation access tools (this is just a side note).

Isn't the way optional+provided deps are broken is that they don't let you provide a "compile only" dependency? That doesn't feel like it affects this situation?

It is broken in that different tools will do different things. Graal VM native, running with jpackage, Maven, insert other build tool, Eclipse, etc all have different ideas what optional is.

The modular JDK idea of optional is really only the service loader. But you are right that if we are talking about what is capable with the current JDK modular:

"optional+provided deps are broken is that they don't let you provide a "compile only" dependency"

is correct.

The reason what I mention is relevant is because we are talking about tools and tools have to do different things than the JDK.

@bowbahdoe
Copy link
Author

bowbahdoe commented Aug 1, 2023

TYPE_USE annotations are part of the type. Think of it has hiding Optional.

I'm more thinking of it like hiding the String in List<String>.

The difference is that Optional<T> is a >different< type than T. List<String> is a refinement of the raw type List and some APIs won't carry over full information. @Nullable T and T feel similar to that.

Let us ignore what Java aka Open JDK javac currently does.

I'm referencing the behavior of the jvm, not javac.

@agentgt
Copy link

agentgt commented Aug 1, 2023

Yes but the parameterized type information is preserved depending on where (like methods or class definitions) but I agree your analogy is better in terms of erasure.

But just to be clear TYPE_USE annotations are available at runtime like I mentioned before (probably more so then generics). Also if at any point if you have the parameter's type exposed ... you are going to need in the classpath/modulepath so its not an analog either.

I'm referencing the behavior of the jvm, not javac.

Yeah I was kind of unclear on that because you have a video image of intellij and it looks like its patching in modules (my vision is bad so I can't say for sure). That goes to my point of what the tools do (especially JUnit but let us ignore that for now).

Anyway I assume as you do that requires static should be good enough but I'm saying I too have been seeing odd behavior in modular environments as well as GraalVM native. What avoids these headaches if the classes are actually present.

I made a case awhile back that JSpecify might want to consider having retention=CLASS as the Eclipse annotations do it that way but I found similar problems with Eclipse's annotations.

@bowbahdoe
Copy link
Author

Also if at any point if you have the parameter's type exposed ... you are going to need in the classpath/modulepath so its not an analog either.

Do you have an example call that works like this?

import org.jspecify.annotations.Nullable;
import org.jspecify.annotations.NullMarked;

@NullMarked
class A {
    @Nullable String f(@Nullable String g) { return g; }
}

Like - what would I have to do at runtime (without jspecify available) to cause an issue?

@agentgt
Copy link

agentgt commented Aug 1, 2023

Just to be clear in a non-modular runtime (e.g. everything on classpath but jdk) I have zero problems.

What I have seen in both Graal VM native and some jpackaged/jlinked environments are errors but it was not in that simple case you brought up. I should have isolated the problem but I lacked time.

Unfortunately the final executable jars in my companies environment are not using the module path so I have not been seeing the issue these days but I can certainly go testing with you as I need to know as well since I ship a library that does do requires static org.eclipse-willeventuallbe.jspecify.

What I think causes the issue is probably a bug in the annotation loading. If something calls A.class.getAnnotation(... any annotation ...) it will try to load @NullMarked even though it was not requested as all annotations get loaded. It should silently fail but I found that not to be in the case in certain environments.

Anyway here was some doc I think I was looking at when I came across the issue:

https://www.graalvm.org/sdk/javadoc/org/graalvm/nativeimage/AnnotationAccess.html

The getAnnotation implementation in the JDK for Class, Field, and Method initializes the classes of all annotations present on that element, not just the class of the queried annotation. This leads to problems when not all annotation classes are present on the classpath/modulepath: querying an annotation whose class is present can fail with an exception because not all annotation classes are present.

Anyway I'll try to go back and reproduce my issues sometime next week and or collaborate with you on reproducing problems.

@cpovirk
Copy link
Collaborator

cpovirk commented Mar 18, 2024

We may also want to see what effect optional has on whatever Proguard plugins may exist for different build tools (proguard-maven-plugin, Proguard Gradle Plugin) and on the Android Gradle Plugin in general.

I base this on jhy/jsoup#2129, which suggests that optional (or at least provided) may be trouble.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev development tasks, build issues...
Projects
None yet
Development

No branches or pull requests

4 participants