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

Please consider a different library other than Checker-Framework for JSR305 replacement #3031

Closed
wltjr opened this issue Jan 15, 2018 · 27 comments
Assignees

Comments

@wltjr
Copy link

wltjr commented Jan 15, 2018

I understand the reasoning to move beyond JSR305. I do not think Checker-Framework to be a good replacement. First off JSR305 has no dependencies required to build from source.

That is not the case with Checker-Framework, which seems to not be that easy to package. I am having to package
checker-checker
checker-dataflow
checker-framework
checker-javacutil

Which themselves have their own dependencies. Even worse Checker-Framework has not been updated for Java 9, and has issues there as you can see here and here. I have fixed most via sed changes. But I have only packaged 2 of the needed pieces of Checker-Framework. I have to package more dependencies and fix more Java 9 issues for the remaining pieces.

Just so I can bump guava from 23.5 to 23.6. Checker-Framework IMHO was a VERY poor choice. It has to many dependencies. and does not build under Java 9. Which makes me think it is not a forward looking project. Java 10 will be out in a couple months....

Sadly I have come to expect such from Google.... Changes like this that go against logic reinforces that. Why replace a library that has no deps with one that has several deps, to a library project. Even worse a library that does not build under Java 9 nor has required changes in git for such. I haven't the time to submit PRs for everything. If Checker-Framework built under Java 9, then that would make a bit more sense. But then that still leaves adding more dependencies to a Library, which is not good IMHO. Libraries should have minimal deps. The other dependencies of Guava do not have their own deps.

There are LOTS of choices for alternatives to JSR305. Google already uses Intellij. I would think their NonNull stuff to be a better option that has no deps. Like JSR305 had no deps.

Really cannot believe the amount of work to replace JSR305 for a minor version release... This is the first time I am unable to zero day bump guava. In fact it is taking me some time and getting delayed by other things. As I have to allocate additional time in packaging Checker-Framework....

Please consider an alternative!!!!

@ooxi
Copy link

ooxi commented Jan 15, 2018

While I cannot comment on the resons for choosing Checker-Framework over JSR305 (or any other alternative) I personally would be interested why you aren't using Maven for dependency resolution.

Is this a policy thing e.g. only being able to depend on other packages already available for your distribution?

@jbduncan
Copy link
Contributor

I can't comment for the Guava team - I hope that someone from the team can share some insight on the decision to use Checker Framework - but it seems to me that the authors of Checker Framework are currently seeking seeking volunteers to update it to Java 9/10, so things don't look completely hopeless on that front to me.

If migrating to other nullness checking frameworks is still an option, I know of the existence of something called lastnpe.org, which seems to be an Eclipse-originated project or at least somehow linked with Eclipse. I've not looked much into it myself, but nonetheless I wonder if the Guava team considered it at some point.

@wltjr
Copy link
Author

wltjr commented Jan 15, 2018

@ooxi your assumption is correct. On Gentoo Maven and Gradle compete and go against Portage for dependency resolution. Packages cannot download stuff during build. They must have all things download prior. There was Ant integration which I have scrapped. It was to cumbersome to maintain with xml rewriting for classpath and source/target changes, now --release over source/target. At least with Ant, Ivy bits can be commented out, targets avoided. That is not possible with Maven or Gradle.

Maybe long term I can look to make a portable version of Portage for Java building purposes. I think the Gentoo Java build system is far superior to any Java solution, ant, maven, gradle, etc. I can package tons of stuff with minimal effort.

@jbduncan most all projects are seeking contributions and help for Java9/10. Seems to have snuck up on everyone. Though much of the java world seems stuck in the past with legacy stuff. Most grabbing binaries vs building from source. Not carrying so long as the binary works for their needs. It seems most of the Java world is not very forward looking. I for a long time on Gentoo have always looked to use the latest libraries, versions, etc. I tend to have to patch and modify a good deal of packages for a variety of reasons, updated deps, updated java, etc.

Checking for nullness is usually an annotation library thing. Most tend to be rather small. I really do not understand the logic or reasoning behind choosing Checker-Framework. I assume for other features of that project. If its just for the nullness aspect. Then I think it was a poor choice and lots of alternatives. Most build fine under Java 9. I bet the parts needed by guava build fine under Java 9. But to get there I have to build other parts of checker framework.

Google isnt doing anyone any favors by using a library with deps, and one that is seeking support for Java9+. I would think they would have a better eye towards the future and look to depend on something that has support for Java 9 already or builds under Java 9, like other nullness libraries.

@wltjr
Copy link
Author

wltjr commented Jan 15, 2018

OFF TOPIC to issue!

@ooxi FYI even building maven from source seems like voodoo. Libraries are in use by Maven nothing else uses and no one seems to have a clue like Plexus and Modello. But that is just to package and build maven from source. Not sure about Gradle. Even when I have both those, neither can be used for packages in Gentoo. Developers can use them, but not system installed packages. I still have to try to fake out both with a local repo. I think can replicate a maven local repo. Gradle will not be as easy as it tends to use per user cache vs system.

@jbduncan
Copy link
Contributor

jbduncan commented Jan 15, 2018

@wltjr

... I assume for other features of that project. ...

Guava seem to be incorporating @MonotonicNonNull into the code base, if I understand the more recent commits correctly. That annotation is a Checker Framework-specific feature which is used to check lazy initializations at compile-time. But AFAICT it could be replaced with error-prone's @LazyInit if Checker Framework were to be dropped.

@jbduncan
Copy link
Contributor

@wltjr I'm interested to hear your thoughts on what other nullness-checking libraries or solutions you think Guava could use. :)

@jbduncan
Copy link
Contributor

Ah, I see you said that "Google already uses Intellij." (which I do believe is true), sorry for missing that!

Would you be happy to list, if not for the Guava team's benefit but at least for my own, what other alternatives you think Guava could use?

@wltjr
Copy link
Author

wltjr commented Jan 15, 2018

@jbduncan I am not really that familiar with the nullness libraries. I just recall packaging some as deps and the main reason was missing a null related class, not other annotations. I believe Java 9 has internal nullness that replaces JSR305.
Here are a few links for such
https://blog.codefx.org/java/jsr-305-java-9/#Alternatives-To-JSR-305
https://stackoverflow.com/questions/4963300/which-notnull-java-annotation-should-i-use

I would definitely recommend NOT using Lombok's NonNull. I really dislike Lombok, it has a ridiculous amount of deps.

Funny that Android also has one as well...

Now if there are further uses of Checker-Framework then that makes sense. it seems to be a full framework not just the minimal null annotation. Thus Guava may have some longer term plans for using Checker-Framework over the others. If it really is just for null, and not other features of the Checker-Framework code analysis. Then it was a bad choice.

@Maaartinus
Copy link

@wltjr I'd bet, the insider's reasons for using the Checker-Framework annotations go deeper than we can see. IMHO a set of annotations should be used which has no dependencies on anything else - annotations are meant to be lightweight in Java: They should cause no problems at all, that's why unknown annotations get simply ignored.

IMHO what should be done is to extract an annotations-only artifact from the Checker-Framework (or any other framework, but that train has left).

Btw., Lombok's @NonNull is meant to generate runtime nullness tests, it's not a general-purpose annotation. Moreover, there are no related annotations (like @ParametersAreNonnullByDefault) in Lombok, so it's out of question.

@wltjr
Copy link
Author

wltjr commented Jan 15, 2018

@Maaartinus I am assuming there are deeper needs, its the only logical justification for such. I am not familiar with nullness libraries. That is good that lombok would not be suffice, as it would bring in considerable unwanted deps.

One of the other things I have to package for Checker-Framework is another annotations library, Annotations Scene LIbrary. Which is rather strange, no maven artifacts, not sure on actual name. Its a project of typetools, where Checker-Framework comes from. Scene library is one of the last deps I think for checker-framework. Which I need for checker-checker, which has the actual classes needed by Guava...

Typetools also has a fork of Guava for some reason. That seems to have some commits not going back into Google's Guava. I hope Checker-Framework is beneficial for Guava in ways beyond nullness. It does not seem like a quality project.

@wltjr
Copy link
Author

wltjr commented Jan 15, 2018

It gets worse, now onto packaging Annotations Scene Library, I run into other deps. They have a modified version of ASM. Which seems to be required. Not to mention even worse, deps on Guava. Which creates a circular dependency situation... Or I use an older version of Guava for Annotations Scene LIbrary, so it can be used for Checker Framework, for the current version of Guava.

I already packaged javaparser just for Checker-Framework and seems its missing some classes.

# grep import /tmp/portage/dev-java/checker-framework-2.3.1/temp/build.log
import scenelib.annotations.Annotation;
import scenelib.annotations.el.AClass;
import scenelib.annotations.el.ADeclaration;
import scenelib.annotations.el.AElement;
import scenelib.annotations.el.AField;
import scenelib.annotations.el.AMethod;
import scenelib.annotations.el.AScene;
import scenelib.annotations.el.ATypeElement;
import scenelib.annotations.el.AnnotationDef;
import scenelib.annotations.el.BoundLocation;
import scenelib.annotations.el.DefException;
import scenelib.annotations.el.InnerTypeLocation;
import scenelib.annotations.el.LocalLocation;
import scenelib.annotations.io.IndexFileParser;
import scenelib.annotations.io.IndexFileWriter;
            PackageDeclaration pkgDecl, List<ImportDeclaration> importDecls, AScene scene) {
import com.github.javaparser.ast.StubUnit;
import com.github.javaparser.ast.visitor.SimpleVoidVisitor;
import com.github.javaparser.ast.StubUnit;
import scenelib.annotations.Annotation;
import scenelib.annotations.Annotations;
import scenelib.annotations.el.ABlock;
import scenelib.annotations.el.AClass;
import scenelib.annotations.el.ADeclaration;
import scenelib.annotations.el.AElement;
import scenelib.annotations.el.AExpression;
import scenelib.annotations.el.AField;
import scenelib.annotations.el.AMethod;
import scenelib.annotations.el.AScene;
import scenelib.annotations.el.ATypeElement;
import scenelib.annotations.el.ATypeElementWithType;
import scenelib.annotations.el.AnnotationDef;
import scenelib.annotations.el.DefException;
import scenelib.annotations.el.ElementVisitor;
import scenelib.annotations.field.ArrayAFT;
import scenelib.annotations.field.BasicAFT;
import scenelib.annotations.io.IndexFileParser;
import scenelib.annotations.io.IndexFileWriter;
import scenelib.annotations.io.ParseException;
import scenelib.annotations.Annotation;
import scenelib.annotations.el.AClass;
import scenelib.annotations.el.AField;
import scenelib.annotations.el.AMethod;
import scenelib.annotations.el.AScene;
import scenelib.annotations.el.ATypeElement;
import scenelib.annotations.el.DefException;
import scenelib.annotations.el.InnerTypeLocation;
import scenelib.annotations.io.IndexFileParser;
import scenelib.annotations.io.IndexFileWriter;
import scenelib.annotations.el.AClass;
import scenelib.annotations.el.AField;
import scenelib.annotations.el.AMethod;
import scenelib.annotations.util.JVMNames;
import scenelib.annotations.Annotation;
import scenelib.annotations.el.AnnotationDef;

Then for scene library...

# grep import /tmp/portage/dev-java/scene-lib-3.6.49/temp/build.log
import com.google.common.escape.CharEscaperBuilder;
import com.google.common.escape.Escaper;
import plume.ArraysMDE;
import plume.UtilMDE;
import org.objectweb.asm.TypeAnnotationVisitor;
import com.google.common.collect.HashMultimap;
import com.google.common.collect.SetMultimap;
import plume.FileIOException;
import plume.FileIOException;
import org.objectweb.asm.TypeAnnotationVisitor;
import org.objectweb.asm.MethodAdapter;
import org.objectweb.asm.ClassAdapter;
import org.objectweb.asm.MethodAdapter;
import plume.Option;
import plume.Options;
import plume.*;
import org.objectweb.asm.ClassAdapter;
import org.objectweb.asm.TypeAnnotationVisitor;
import org.objectweb.asm.MethodAdapter;
import org.objectweb.asm.commons.EmptyVisitor;
import org.objectweb.asm.TypeAnnotationVisitor;
import org.objectweb.asm.commons.EmptyVisitor;
import plume.FileIOException;
import plume.ArraysMDE;
import plume.FileIOException;
import plume.Pair;
import com.google.common.collect.BiMap;
import com.google.common.collect.HashBiMap;

This stuff is a real nightmare to package, and projects a mess. That it introduces a circular dependency alone should be a show stopper to Checker-Framework. No way to build that without guava. Very likely a modified guava like a modified asm. I haven't tried adding guava yet. I dislike having to keep 2 versions for circular dependencies reasons.

All reasons why I myself will never use Guava. I question why projects use it. Many get stuck on older versions like jclouds. Though most things in my overlay that need guava can use the latest. Though pretty sure Intellij, or at least Android Studio is shipping with an older Guava... Guava does seem to break, thus on Gentoo it is slotted due to API changes, etc.

Sad to say but thus far not really impressed with any Google Java efforts. They seem to be making a real mess of things. With no regard to impact on others or the larger world. Like forking forked netty-tcnative, fork of tomcat-native, to conscrypt. Which requires borringssl, a fork of openssl... I tried using Google libraries for Youtube, and after packing way to many. I opted to just go a simpler route using other libraries not from Google.... You can see a variety of google packages in my overlay. Likely outdated as i packaged them for my development needs. Then realized the were over engineered, PITA to work with, and never used them. Likely will remove.

@kluever
Copy link
Member

kluever commented Jan 15, 2018

Related: #2960

@wltjr
Copy link
Author

wltjr commented Jan 15, 2018

@kluever thanks I missed that, and I would prefer that over Checker-Framework as I have spot bugs annotations packaged already. I think I have most alternatives just not Checker-Framework, its a pain to package sadly.

@PhilippWendler
Copy link

@wltjr Why are you attempting to build the full checker framework from source? Guava needs only the annotations, which are packaged in separate JARs, and it should be sufficient to just build that single JAR. A one-minute search through the checker framework's build files let me find the ant target checker-compat-qual-jar, it seems like it has no dependencies and should produce exactly what is needed for Guava. Does this work?

@wltjr
Copy link
Author

wltjr commented Jan 16, 2018

@PhilippWendler seems I should have checked the ant build. Thanks for mentioning that. I was going by the project structure in source tarball/git. Which has various subdir/src directories which usually represent the various pieces of a project. Which I am packaging as checker-subdir. As I do for most projects I package.

After looking at the ant pattern I see its using a deep subdir compatqual. Normally I would think that to be in its own package like compatqual/src/..., like the others checker, dataflow, framework, javacutil, etc. That is how spotbugs-annotations and intellij, both named annotations. Which I packaged as such for each spotbugs-annotations and intellij-platform-annotations .Where checker-compatqual is rather strange name and does not make it clear its annotations or null related.I guess they have their reasons for such strangeness.

Again I should have doubled checked ant build, I assumed wrong based on source layout. That gets get me around my dependency gripe. Though still may be an odd choice per other points mentioned.

@cpovirk
Copy link
Member

cpovirk commented Jan 16, 2018

OK, that is a relief. I'd known that the Checker Frameworks annotation package is lightweight in our internal build, but reading the beginning of this thread, I was starting to worry that I hadn't looked at the Maven dependencies. I'm glad to see that there aren't any. (We'd tried to shake out such problems by adding the dependency in 23.5 but not using it until 23.6, but for obvious reasons that didn't help in the Gentoo case.)

As for using the Checker Framework in general: We (not the Guava team specifically, but a related team) have some people looking more deeply into nullness analysis in general, and I'm interested to see how that shakes out. The Checker Framework is the only project with a "full" nullness type system, so we'd love to be able to take advantage. But we don't take advantage yet: We promise Java 7 compatibility, so we can't use type annotations. And even once we can, we'd need to do the work of annotating Guava. And then, as you point out, the Checker Framework hasn't moved to Java 9 yet. So it's not clear that this is the long-term solution. Sorry that it's been such a pain in the meantime.

Finally, thanks for your work on Gentoo, which is of personal interest to me :)

@cpovirk cpovirk closed this as completed Jan 16, 2018
@wltjr
Copy link
Author

wltjr commented Jan 16, 2018

@cpovirk Yes not an issue for Guava per se. Ideally upstream, Checker-Framework does something better/cleaner with their nullness/annotation package.

I had assumed based on the name and looking into Checker-Framework that long term there maybe interest to use the framework more. Thus mentioning Checker-Framework vs just the annotation jar package being used. The project itself seems like it could use some love. Not sure about its forks of asm and guava.... That may make an interesting circular dep issue for guava and checker-framework. Or at least requiring older guava to build checker-framework for analysis of newer guava... :) Along those lines not removing the parts of Checker-Framework I have packaged, and someday will finish the remaining bits. Once I get around the forked asm/guava stuff.

I understand the legacy Java support. I consider 7 and really even 8 legacy at this point. The Java ecosystem is quite strange. A ton of legacy stuff, some in wide use like Zookeeper/Hadoop that uses older netty. Many projects like guava keeping backward compliance. Its a mess for Lombok to balance such, building under newer, providing support for older. At the same time Oracle and Java seem to be moving forward faster and faster. Not sure what that means long term.

Many things have not moved to Java 9. I am about to start working toward Java 10, EA builds have been out since Java 9 release. Seems like all may have to start looking forward vs backward more. Making sure code is good for newer Java versions rather than older. Maybe means to kick the Java eco system and shed all this legacy stuff. It is a big problem for IcedTea, which at RH they tend to get stuck supporting 6,7 and 8, and have little to no time for anything 9, much less 10. Very interesting times.

No worries on it being a pain. I am sorry for not looking at the ant build system. I should know better. I had to do some stuff along those lines for Tomcat. Which its jars contain stuff from various directories, some overlapping jar contents. Though most projects tend to have sources split in directories that map to jar contents. Which is where my assumption came from.

@cpovirk if your interested in Java on Gentoo you should check out my overlay. It completely replaces all Java on Gentoo. I was a Gentoo developer long ago and Foundation Trustee. Sadly the project long ago decided social issues were more important than technical contributions. In trying to deal with social issues, which I feel tech creates more than it solves. Gentoo has really hurt itself in many ways. Sadly I was encouraged and forced over many years to not contribute. Thus my massive overlay that replaces all Java in Gentoo. Gentoo and the world could benefit from such. But others decided rather than be part of the solution they rather be the problem and stand in the way of progress. Gentoo I see as playing a vital role in development. Like for Java building stuff using the latest and greatest of everything and addressing issues along the way. Which is what I have been doing for a couple years now :) Making noise, upsetting people, being rude and polite at the same time, with good intentions. For the benefit of all :)

Anyway thanks for being receptive and again sorry for the noise.

@deruf
Copy link

deruf commented Apr 19, 2018

Are there plans to use anything from the checker-framework except the annotations in Guava? Checker annotations are MIT licensed other parts of the checker framework are GPL.

@cpovirk
Copy link
Member

cpovirk commented Apr 23, 2018

No plans. I wouldn't rule out the possibility that we would someday use the Checker Framework's actual checking, but even that wouldn't involve linking in the GPL parts of the framework, and it would even still be possible to compile with the checking off if desired.

@fzyzcjy
Copy link

fzyzcjy commented Mar 6, 2020

Sorry for a silly question: Where does Guava use the Checker Framework? I tried searching but failed...

@cpovirk
Copy link
Member

cpovirk commented Mar 6, 2020

We don't actually run the checker. We just use the annotations -- and not even following proper semantics :(

For example:

boolean put(@Nullable K key, @Nullable V value);

You can follow our efforts to stop doing that on #2960 (which I'll try to update this afternoon or early next week).

@fzyzcjy
Copy link

fzyzcjy commented Mar 6, 2020

@cpovirk Thanks!

@cpovirk
Copy link
Member

cpovirk commented Mar 7, 2020

I should also say: While it's true that we don't run the Checker Framework against the master branch or automatically, I do have an experimental fork (which is already several months out of date) in which I've annotated things properly for the Checker Frameworks (and added suppressions where needed). That work led to a8107fa and 5345f11, and it has helped with some of the work on #2960. Eventually we'll publish it for people to look at, even though it's not expected to end up in the master branch.

@fzyzcjy
Copy link

fzyzcjy commented Mar 7, 2020

@cpovirk Thanks very much! By the way, what is your opinion towards the Checker Framework? Is it ready to be used in enterprise big applications?

@cpovirk
Copy link
Member

cpovirk commented Mar 17, 2020

My only significant experience with the Checker Framework is with Guava, which is reasonably large (165k LoC) but of course not representative of an average application. There are certainly apps at Google that use it, particularly Android apps, since a NullPointerException there can crash an app -- which is much worse than just having some web server fail to serve one request. I don't have enough hands-on experience to compare it to other options, though.

@fzyzcjy
Copy link

fzyzcjy commented Mar 18, 2020

@cpovirk Thanks!

@joachim-durchholz-six
Copy link

I'd suggest not trying to use Java 9 or 10 with CF. Last time I looked the policy was "we support only LTS versions of Java".
Building it on your own seems to be difficult, but it's just the beginning: You have to annotate the JDK to get any value out of CF, and that means analyzing the whole JDK source. (Obviously you can carry over the analysis from Java 8 and retrofit changes made for Java 11, but it's still going to take person-months, at least. This does not seem to be a happy road.)

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

No branches or pull requests

12 participants