StaticImportCheck: Add `java.util.Collections` candidates #23

rickie · 2021-12-14T09:57:33Z

Based on an offline discussion with @Stephan202 and a discussion in this PR, we decided to add java.util.Collections as static import candidate.

Actually, we did specifically discuss Collections.disjoint and not the others of that class. However, there are many methods that look as good candidates for statically importing. Whether all these methods are good to import is still up for debate. So lets discuss what you think!

Should we exclude some of the methods?
Should we only list some of the methods as candidates?

Note: the support for exclusion of specific methods is not in this PR but in #22.

nathankooij · 2021-12-14T10:27:18Z

I think this makes sense in general. I do wonder though since I haven't used many of these often, it can be jarring to know where it's coming from. It does read nicer in most cases though (i.e. Collections.binarySearch vs binarySearch). My only concern is that some method names are rather generic fill or swap which could refer to own implementations, but that might just stem from me not using most of these on a regular basis.

On another note: I liked that we had a vote for Comparator. Is it an idea to also do that for these and others?

rickie · 2021-12-15T12:49:27Z

My only concern is that some method names are rather generic fill or swap which could refer to own implementations, but that might just stem from me not using most of these on a regular basis.

Yeah those are good examples of when it is not the best idea to statically import. An option could be to exclude the "generic" methods from statically importing.

ibabiankou

We should definitely NOT import methods like of, valueOf, create, but here I think we are fine.

I've looked at the methods and usages of Collections.* across the repositories and I must say no matter what the method is used, having Collections in front of it does not add much context for readability. I do feel a bit less concerned about the implementation, but I still want to read the docs in order to understand better what it does.

rickie · 2021-12-17T12:26:17Z

We should definitely NOT import methods like of, valueOf, create, but here I think we are fine.

Nice, you mention three methods that are listed as "bad import" in the BadImport BugPattern. However, currently it is not turned on within Picnic. It is on the list to fix that as well. We have to make an improvement in the fork (ideally they also accept it upstream of course) before we want to turn it on for all codebases.

Stephan202

As discussed offline, I'm not 100% sure we should statically import all of these. Some of the method names are very short and generic, which could make a reader wonder whether (e.g.) a locally defined method is referenced. An example would be list. I'm also not sure about min and max, as these could clash with static imports of Math.min and Math.max.

That said, all those checked*, empty*, singleton*, synchronized* and unmodifiable* methods can indeed be statically imported without ambiguity.

(Added a small commit.)

Stephan202 · 2022-01-01T12:45:31Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StaticImportCheckTest.java

@@ -109,6 +111,9 @@ void replacement() {
            "    ImmutableSet.toImmutableSet();",
            "    ImmutableSet.<String>toImmutableSet();",
            "",
+            "    boolean b = Collections.disjoint(ImmutableSet.of(), ImmutableSet.of());",


Suggested change

" boolean b = Collections.disjoint(ImmutableSet.of(), ImmutableSet.of());",

" Collections.disjoint(ImmutableSet.of(), ImmutableSet.of());",

Stephan202 · 2022-01-01T12:45:37Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StaticImportCheckTest.java

@@ -109,6 +111,9 @@ void replacement() {
            "    ImmutableSet.toImmutableSet();",
            "    ImmutableSet.<String>toImmutableSet();",
            "",
+            "    boolean b = Collections.disjoint(ImmutableSet.of(), ImmutableSet.of());",
+            "    Collections.reverse(new ArrayList<>(0));",


Suggested change

" Collections.reverse(new ArrayList<>(0));",

" Collections.reverse(new ArrayList<>());",

rickie · 2022-02-07T13:37:33Z

Looked at these suggestions:

That said, all those checked*, empty*, singleton*, synchronized* and unmodifiable* methods can indeed be statically imported without ambiguity.

Based on these suggestions grep'ed in picnic-java-all to see whether they are used or not.
In the future, it could be nice to remove some of synchronizedNavigableMap and unmodifiableNavigableSet. I'm not sure how good these methods are and whether they should be replaced by something different.

This selection of methods can be a good first list for java.util.Collections. Let me know what you think 😄.

Stephan202

Based on these suggestions grep'ed in picnic-java-all to see whether they are used or not.

I don't think we should restrict ourselves to the set of methods currently used. Rather, let's define the full set that currently makes sense, so that we also provide a consistent experience in the future.

Rebased and added a counter-proposal; PTAL.

Suggested commit message:

Suggest static imports for most `java.util.Collections` methods (#23)

rickie · 2022-04-11T07:50:33Z

Rebased.

The self-apply detects some violations of the newly added static import candidates in some Refaster templates. However, to make it clear what we are rewriting, I think it makes sense not to statically import them. WDYT?

[INFO] /repos/error-prone-support/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/AssortedTemplates.java:[165,25] [StaticImport] Identifier should be statically imported
  Did you mean 'return disjoint(set1, set2);'?
[INFO] /repos/error-prone-support/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/AssortedTemplates.java:[180,22] [StaticImport] Identifier should be statically imported
  Did you mean 'disjoint(ImmutableSet.copyOf(collection1), collection2),'?
[INFO] /repos/error-prone-support/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/AssortedTemplates.java:[181,22] [StaticImport] Identifier should be statically imported
  Did you mean 'disjoint(new HashSet<>(collection1), collection2),'?
[INFO] /repos/error-prone-support/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/AssortedTemplates.java:[182,22] [StaticImport] Identifier should be statically imported
  Did you mean 'disjoint(collection1, ImmutableSet.copyOf(collection2)),'?
[INFO] /repos/error-prone-support/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/AssortedTemplates.java:[183,22] [StaticImport] Identifier should be statically imported
  Did you mean 'disjoint(collection1, new HashSet<>(collection2)));'?
[INFO] /repos/error-prone-support/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/AssortedTemplates.java:[188,25] [StaticImport] Identifier should be statically imported
  Did you mean 'return disjoint(collection1, collection2);'?
[INFO] /repos/error-prone-support/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ImmutableSetTemplates.java:[64,25] [StaticImport] Identifier should be statically imported
  Did you mean 'return singleton(element);'?
[INFO] /repos/error-prone-support/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ImmutableMapTemplates.java:[69,22] [StaticImport] Identifier should be statically imported
  Did you mean 'singletonMap(key, value));'?
[INFO] /repos/error-prone-support/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ImmutableListTemplates.java:[66,25] [StaticImport] Identifier should be statically imported
  Did you mean 'return singletonList(element);'?

Changes LGTM.

Stephan202 · 2022-04-11T08:26:27Z

However, to make it clear what we are rewriting, I think it makes sense not to statically import them.

Hmm, but eventually we should raise these INFOs to WARNINGs, so either we should not flag these methods at all, or make the suggested change. 🤔

(We should likely avoid severity = SUGGESTION, as it doesn't make the build fail. But that's a separate discussion.)

rickie · 2022-04-11T13:28:54Z

Then let's import the methods statically. Will push it in a bit.

W.r.t. the severity, it should indeed be raised to WARNING. Should I open a PR for that?

rickie · 2022-04-12T07:49:40Z

Yesterday we discussed this and decided to leave it as is, right? @Stephan202

Singleton will be rewritten to something else such that it doesn't occur in the code.

Stephan202 · 2022-04-12T08:48:06Z

@rickie yeah, let's just do it. Do you wanna resolve the conflicts?

rickie · 2022-04-12T09:37:47Z

Rebased.

Stephan202 · 2022-04-12T09:43:12Z

Tnx! @EnricSala anything from your side? :)

EnricSala

🚀

rickie · 2022-04-12T10:50:52Z

Rebased and added a small commit.

Stephan202 · 2022-04-12T12:39:09Z

Updated suggested commit message:

Require static import of most `java.util.Collections` methods (#23)

While there, statically import `ZoneOffset.UTC` in a few places.

rickie · 2022-04-12T12:44:44Z

Why would we now use a different message from previous commits?
Require static import for ....?

Stephan202 · 2022-04-12T12:57:55Z

Updated. (Though given that suppression is possible, arguable "suggest" is better.)

rickie · 2022-04-12T14:57:23Z

LGTM, let's merge 😄!

rickie requested review from Stephan202, ibabiankou, EnricSala and nathankooij December 14, 2021 09:57

ibabiankou approved these changes Dec 17, 2021

View reviewed changes

rickie changed the title ~~StaticImport: add java.util.Collections as candidate~~ StaticImportCheck: add java.util.Collections as candidate Dec 21, 2021

Stephan202 reviewed Jan 1, 2022

View reviewed changes

rickie changed the title ~~StaticImportCheck: add java.util.Collections as candidate~~ WIP: StaticImportCheck: add java.util.Collections as candidate Jan 15, 2022

rickie changed the title ~~WIP: StaticImportCheck: add java.util.Collections as candidate~~ StaticImportCheck: add java.util.Collections as candidate Feb 7, 2022

rickie changed the title ~~StaticImportCheck: add java.util.Collections as candidate~~ StaticImportCheck: Add java.util.Collections candidates Feb 7, 2022

rickie requested a review from Stephan202 February 7, 2022 13:38

Stephan202 force-pushed the rossendrijver/static_import_collections_disjoint branch from 3bdadf4 to 7748f2a Compare April 10, 2022 09:56

Stephan202 approved these changes Apr 10, 2022

View reviewed changes

rickie force-pushed the rossendrijver/static_import_collections_disjoint branch from 7748f2a to de984da Compare April 11, 2022 07:41

rickie removed the request for review from nathankooij April 11, 2022 13:28

rickie force-pushed the rossendrijver/static_import_collections_disjoint branch from 2877c6b to 0f6549a Compare April 12, 2022 09:37

EnricSala approved these changes Apr 12, 2022

View reviewed changes

rickie and others added 2 commits April 12, 2022 11:52

StaticImport: add java.util.Collections as static import candidate

e63bcff

Tweaks

8683d8e

rickie and others added 4 commits April 12, 2022 11:52

StaticImport: Add candidates for java.util.Collections

0b38f6e

Suggestion

2b6e2ce

Statically import some methods from Collections

8586827

Post-rebase fix and statically import UTC

898432a

rickie force-pushed the rossendrijver/static_import_collections_disjoint branch from 0f6549a to 898432a Compare April 12, 2022 10:50

Stephan202 added this to the 0.1.0 milestone Apr 12, 2022

Stephan202 merged commit 1cb4ce9 into master Apr 12, 2022

Stephan202 deleted the rossendrijver/static_import_collections_disjoint branch April 12, 2022 15:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

StaticImportCheck: Add `java.util.Collections` candidates #23

StaticImportCheck: Add `java.util.Collections` candidates #23

rickie commented Dec 14, 2021

nathankooij commented Dec 14, 2021

rickie commented Dec 15, 2021

ibabiankou left a comment

rickie commented Dec 17, 2021 •

edited

Loading

Stephan202 left a comment

Stephan202 Jan 1, 2022

Stephan202 Jan 1, 2022

rickie commented Feb 7, 2022 •

edited

Loading

Stephan202 left a comment

rickie commented Apr 11, 2022 •

edited

Loading

Stephan202 commented Apr 11, 2022

rickie commented Apr 11, 2022 •

edited

Loading

rickie commented Apr 12, 2022

Stephan202 commented Apr 12, 2022

rickie commented Apr 12, 2022

Stephan202 commented Apr 12, 2022

EnricSala left a comment

rickie commented Apr 12, 2022

Stephan202 commented Apr 12, 2022 •

edited

Loading

rickie commented Apr 12, 2022

Stephan202 commented Apr 12, 2022

rickie commented Apr 12, 2022

	" boolean b = Collections.disjoint(ImmutableSet.of(), ImmutableSet.of());",
	" Collections.disjoint(ImmutableSet.of(), ImmutableSet.of());",

	" Collections.reverse(new ArrayList<>(0));",
	" Collections.reverse(new ArrayList<>());",

StaticImportCheck: Add java.util.Collections candidates #23

StaticImportCheck: Add java.util.Collections candidates #23

Conversation

rickie commented Dec 14, 2021

nathankooij commented Dec 14, 2021

rickie commented Dec 15, 2021

ibabiankou left a comment

Choose a reason for hiding this comment

rickie commented Dec 17, 2021 • edited Loading

Stephan202 left a comment

Choose a reason for hiding this comment

Stephan202 Jan 1, 2022

Choose a reason for hiding this comment

Stephan202 Jan 1, 2022

Choose a reason for hiding this comment

rickie commented Feb 7, 2022 • edited Loading

Stephan202 left a comment

Choose a reason for hiding this comment

rickie commented Apr 11, 2022 • edited Loading

Stephan202 commented Apr 11, 2022

rickie commented Apr 11, 2022 • edited Loading

rickie commented Apr 12, 2022

Stephan202 commented Apr 12, 2022

rickie commented Apr 12, 2022

Stephan202 commented Apr 12, 2022

EnricSala left a comment

Choose a reason for hiding this comment

rickie commented Apr 12, 2022

Stephan202 commented Apr 12, 2022 • edited Loading

rickie commented Apr 12, 2022

Stephan202 commented Apr 12, 2022

rickie commented Apr 12, 2022

StaticImportCheck: Add `java.util.Collections` candidates #23

StaticImportCheck: Add `java.util.Collections` candidates #23

rickie commented Dec 17, 2021 •

edited

Loading

rickie commented Feb 7, 2022 •

edited

Loading

rickie commented Apr 11, 2022 •

edited

Loading

rickie commented Apr 11, 2022 •

edited

Loading

Stephan202 commented Apr 12, 2022 •

edited

Loading