Add module descriptor

[XakepSDK]

Switch to module descriptor from automatic module name, so this library will have full support of java module system

[ben-manes]

Until Gradle 7.0, the build system wasn't compatible and external plugins like bnd were not either. That blocked prior attempts to add the descriptor.

The current blocker is that adding it in breaks Eclipse, which is where I do development. I don't know if this is a Gradle Buildship issue or Eclipse's. It is unable to handle a split project (main, test, jmh).

I believe that the library's module-info.java would be,

module com.github.benmanes.caffeine {
  exports com.github.benmanes.caffeine.cache.stats;
  exports com.github.benmanes.caffeine.cache;

  requires transitive org.checkerframework.checker.qual;
  requires transitive com.google.errorprone.annotations;
}

However until there is proper support in the toolchain I don't know how to include it. One approach would be package into the jar as a generated resource, which is what I've seen Maven users do, but that seems error prone.

[ben-manes]

If anyone wants to try and make this work a PR is welcome.

[XakepSDK]

Should annotations with runtime retention be transitive? Wouldn't be better to hide as much as possible?

[ben-manes]

requires static would be fine.

[ben-manes]

Released in 3.0.2 (thanks @A248!)

[soc]

I'm seeing build failures caused by this, as we explicitly exclude these dependencies in Maven:

[ERROR] module not found: org.checkerframework.checker.qual [ERROR] module not found: com.google.errorprone.annotations

@ben-manes suggested using requires static, but the patch by @A248 that was merged uses requires.

Could this be fixed?

[ben-manes]

Can you provide a reproduction? The static is in the descriptor. I’m not sure if this is a Maven bug or our misunderstanding.

https://github.com/ben-manes/caffeine/blob/b498408d9c36c2d29f75b1d73695f879639ae795/caffeine/src/main/java/module-info.java#L1-L7

[ben-manes]

This explains that it is needed during application compilation, but not runtime. That’s unlike Maven’s provided scope where the dependency needed it at compile time only but consumers did not. It seems there is no equivalent. Maybe you can change scoping rather than exclude?

[soc]

@ben-manes Oh, apologies, I scanned over the module-info incorrectly, the static is indeed there!

Well, I guess then that approach of avoiding the leak of those annotation packages into the project/IDE auto-import won't work naymore. :-(

[soc]

@ben-manes I'll give it a try, but I fear this doesn't provide the desired semantics ... we pretty much depend on annotation declarations being optional in the JVMS and not providing the jar file at all, but I think this is not something that is conceptually supported by Java modules.

[ben-manes]

I think you might need to have the exclusions and then also add the provided / optional dependencies explicitly with that scope setting. I don't think Maven's exclusions lets one change the scoping of a transitive, so you need to re-establish it at a lower scope. But maybe there is a neater way, e.g. some plugin that handles scope overriding.

[soc]

The problem is not at the level of Maven dependencies, but at the level of Java modules. Getting fancy at the Maven level doesn't help if the Java module system simply doesn't accept it.

Both requires and requires static require the dependency to be there at compile-time, so I guess there is not much I can do. (No worries, this isn't a problem you have to worry about ;-))

[ben-manes]

Sorry, I kept thinking that this was a compilation failure due to excluding the dependency, rather than a runtime validation by the Java module system. If it is at runtime then there is no workaround.

[soc]

Yes, it's a compilation failure, but one based on the module system rules.

If it is at runtime then there is no workaround.

Yes, I think this too.

Thanks for your thoughts and sorry for the noise, @ben-manes!

[ben-manes]

My naive expectation is that if you could scope the dependencies at compileOnly (which is what provided/optional do, iirc), then javac compilation should pass its module validation and at runtime static should pass by not requiring it on the module graph for class loading. Of course this is probably wrong or there are bugs since Java modules is so rarely used in practice.

[soc]

I think the problem here is that compileOnly doesn't provide the desired semantics: making it impossible (at compile time) that the dependency gets accidentally used by my code (that hasn't been modularized already).

[soc]

Btw, what's the reason for the transitive?

I just realized that not even the modularized parts of my code would be safe from these dependencies due to transitive.

[ben-manes]

You’re probably right it’s not needed. I probably had it above just trying to figure out the compatibility issues with Eclipse, before @A248 worked it out. I’m unsure though since it is annotations on the api and if removing would be invalid if called through reflection?

What do you think @A248?

[soc]

I think it shouldn't have any influence on reflection, it only adds an invisible "requires com.google.errorprone.annotations..." to all modules that use caffeine.

[A248]

I've been following this conversation loosely, however I haven't been able to come up with any stellar solutions. I know IDEs have the ability to filter imports, but I am guessing that you want this handled by the build system? There are tools which can check your build for usage of certain classes. I think ArchUnit can do this for you, though I've never used it. PMD also has this feature.

To the best of my knowledge, using transitive is correct is because Caffeine exposes these annotation types. requires static transitive is the equivalent of Gradle's compileOnlyApi. Like with Maven or Gradle, libraries really should declare their dependencies properly. Unlike Maven or Gradle, however, JPMS has no way to perform dependency exclusions, so I do not see an easy way out of this.

I will ask for clarification on the jigsaw-dev mailing list to see if there's a more appropriate solution.

[soc]

using transitive is correct is because Caffeine exposes these annotation types

I don't get this ... with that logic, wouldn't all dependencies be declared transitive?

The only effect transitive has is that consuming modules don't need to write down the requires for it.

(I would argue that transitive is a misfeature, and should be avoided in pretty much any case.)

[A248]

I don't get this ... with that logic, wouldn't all dependencies be declared transitive?

The only effect transitive has is that consuming modules don't need to write down the requires for it.

transitive is not merely a feature of convenience, but the means of declaring a dependency which should be visible to API consumers, because a type from the dependency is exposed in the module's API.

This may help for explanation: https://nipafx.dev/java-modules-implied-readability/

Ignoring annotations for a moment, suppose you have the following method in an exported package. If Bar is from a dependency, transitive is a necessity here because API consumers will see the Bar return type:

public Bar foo();

[soc]

Ignoring annotations for a moment, suppose you have the following method in an exported package. If Bar is from a dependency, transitive is a necessity here because API consumers will see the Bar return type:

It really isn't – either the consumer refers to type Bar, then he can require it, or the consumer doesn't, then he can elide it.

I see nothing wrong with being explicit. If some module isn't required in the consumer's module-info file, I know it's not being used. That's a huge plus over Maven dependencies, where everything is transitive by default.

Having modularized around 400 modules, I never felt the need to use transitive even once.

[A248]

Just FYI, use of transitive is further elaborated in the specification: https://openjdk.java.net/projects/jigsaw/spec/sotms/#implied-readability

Regardless, we'll receive helpful clarification from the jigsaw-dev mailing list with respect to handling annotation dependencies as used here. I've already sent them a message. Hopefully that will clear up any issue here.

[ben-manes]

@A248 I don't think your message went through. You have to subscribe to the mailing list to post.

[A248]

@soc You are right that we don't need to use transitive here.

The jigsaw-dev members (Alex Buckley and Remi Forax specifically) confirmed that requires static, without transitivity, is sufficient and works best. https://mail.openjdk.java.net/pipermail/jigsaw-dev/2021-June/thread.html

So, the caffeine module can be updated to remove the transitive declaration.

[ben-manes]

Thanks @A248. Let's make the change then. You're welcome to send a PR or I'll make the changes at some point soon.

[soc]

@ben-manes Does https://github.com/ben-manes/caffeine/pull/563 look as desired?

[ben-manes]

@soc lgtm

[A248]

All's good from what I see, for what it's worth.

[ben-manes]

fyi, there is a warning in the build that I am suppressing using -Xlint:all,-exports.

warning: [exports] class Nullable in module org.checkerframework.checker.qual is not indirectly exported using requires transitive

[XakepSDK]

Should we ask jigsaw-dev about this?

[ben-manes]

Sure, you are welcome to. They explicitly said annotations in the api don’t need to be transitive, so they might view this as ignorable or a bug. It’s certainly confusing and contradictory advice.

[A248]

I've mentioned this on jigaw-dev, so we'll find out what's happening here: https://mail.openjdk.java.net/pipermail/jigsaw-dev/2021-June/014675.html

[hohonuuli]

This just bit me. A note for folks coming across the issue. The quickest fix is to downgrade from caffeine 3.0.2 to 3.0.1.

Also, @ben-manes caffeine is awesome! Thanks for your continued work on it.

[ben-manes]

Released in 3.0.3

[nipafx]

Alex Buckley replied to the thread.

[soc]

Also, it seems that requires static without transitive does not break the Maven dependency exclusions, as I previously anticipated. So that's a pretty good outcome, I guess.

[A248]

Released in 3.0.3

It might be valuable to briefly mention this in the 3.0.3 release notes.