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

Consider a bundleded signature set that would suggest replacing HashMap, HashSet, Collectors.toSet, Collectors.toMap with Linked alternatives for deterministic oder #199

Open
vlsi opened this issue May 29, 2022 · 2 comments

Comments

@vlsi
Copy link
Contributor

vlsi commented May 29, 2022

HashSet, HashMap, Collectors.toSet(), and similar APIs do not keep the element order, and it might result in surprising and unexpected results.

What if error-prone could catch those types of errors and suggest replacing the calls via LinkedHash*?

See jqwik-team/jqwik#348
See google/error-prone#3230

@uschindler
Copy link
Member

Technically this is easy to do. With forbiddenapis 3.3 it is also now easier to forbid all overloads of a method. You can also forbid the whole class, but this may lead to problems with API signatures expecting those classes.

I am not sure if this should be shipped with forbiddenapis. Maintaining the existing signatures is painful already. If one would publish the signatures on Maven Central as artifacts, one can easily use them in Maven, there's direct support to link to them. In Gradle it also works but not as intuitive.

A signature file could look like:

@defaultMessage Consider using LinkedHashSet
java.util.HashSet#<init>(**)
java.util.stream.Collectors#toSet()

I'd suggest to put something like this into your own build.

@vlsi
Copy link
Contributor Author

vlsi commented May 29, 2022

  1. Unfortunately, it is not really easy to put JDK-related signatures to custom builds since forbidden-apis forbids "unknown signatures".

For instance, Java 10+ has Collectors#toUnmodifiableSet, so the signatures easily become version-dependent.
Then, there's a non-trivial step to figure out the needed Java.

  1. Even though the signatures are small, I believe they are generally useful. For instance, in jqwik case, I did found a couple of bugs caused by hidden reliance on HashSet. In other words, the test outcomes looked nice and shiny, however, the outcomes were just wrong, and HashSet masked the results.

  2. Kotlin instantiates LinkedHashSet for mutableSetOf<K, V>() which, I believe, is nice to avoid accidental bugs. Of course, it might be a bit slower, however, I believe it makes sense to keep the order by default.

  3. Stream#distinct() keeps order by default, and its javadoc explicitly mentions that users should opt-in with .unordered() if they do not care on the order of the elements.


Here's how I put the signatures for now, however, I would say, it took me significantly more time than I wanted, mainly because the signature file does not support things like "if (java10+)".

jqwik-team/jqwik@0c3f049 (it is the part of the PR348 which I mention in the first comment)

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

No branches or pull requests

2 participants