search

GlenKPeterson / Paguro

Generic, Null-safe, Immutable Collections and Functional Transformations for the JVM
Other
312 stars 24 forks source link

Findbugs reports lots of issues #25

Closed GlenKPeterson closed 6 years ago

GlenKPeterson commented 6 years ago

@cprice404 says (in issue #24):

Our build system runs findbugs automatically. With (as far as I know) a pretty basic findbugs setup, there are 4 or 5 "high priority" warnings on the project, and a few dozen "medium priority" ones. Based on your unit test coverage i somewhat doubt that the things findbugs is calling out are critical bugs, so that won't prevent me from evaluating the lib or anything... But I thought you might be interested to know. The kind of stuff that findbugs picks up in general are spiritually similar to what you are doing with equality tests in your testutils lib, so you might find them interesting.

Thanks for reporting this @cprice404! One quick thought is that these collections come from Clojure and are designed to be fast. They break many rules about type-safety internally, but are tested very carefully to enforce all relevant rules externally. So without seeing the report, I'm concerned that there may be some issues I just can't fix.

On the other hand, actual bugs are not allowed and I hope to be able to fix any soon!

GlenKPeterson commented 6 years ago

I fixed everything that seemed to have a clear solution. Thanks for letting me know about this! At least one was a real bug - an exposed mutable array. It had a note like, "Fix this before a release" next to it. Oops! There was a "possible circular instantiation" issue that I've eliminated, though it never caused problems. I also was able to delete a very small amount of duplicated or unnecessary code to get rid of a few other issues. I think this is a significant improvement!

There are a few things I don't know what to do about. The Comparator issues are complicated and appear to work as is. I actually agree with the error, but don't want to fix it because I think the behavior might be useful (you might want to sort Comparables that contains nulls).

I think the serialization issues are false positives. Write a unit test that proves me wrong and I'll fix it!

Thanks again!

GlenKPeterson commented 6 years ago

Fixed in https://github.com/GlenKPeterson/Paguro/releases/tag/3.0.17