Introduce CollectorMutability check

[oxkitsune]

Implement BugPattern to flag collect(to{List,Map,Set}()) and suggest to either use an Immutable variant or a clear mutable variant, in order to emphasise the (im)mutability of the resulting collection.

Example:

- Flux.just(1).collect(toList());
+ Flux.just(1).collect(toImmutableList());
OR
+ Flux.just(1).collect(toCollection(ArrayList::new));

- Flux.just("foo").collect(toMap(String::getBytes, String::length));
+ Flux.just("foo").collect(toImmutableMap(String::getBytes, String::length));
OR
+ Flux.just("foo").collect(toMap(String::getBytes, String::length, (a, b) -> a, HashMap::new));

- Flux.just(1).collect(toSet());
+ Flux.just(1).collect(toImmutableSet());
OR
+ Flux.just(1).collect(toCollection(HashSet::new));

Suggested commit message:

Add BugPattern to flag `collect(to{List,Map,Set}())` to emphasize (im)mutability (#135)

[rickie]

Have training right now, so already flushing the comments I have so far. PTAL :).

[rickie]

Adding some suggestions and ideas. Let me know what you think :).

Nice work already, seeing these tests makes me happy. It'll have some nice impact downstream! 🚀

[rickie]

I'm thinking about the name of the checker. Not too strong of an opinion yet but; we could also name it CollectorImmutabilty`, because we prefer it being immutable. However, AFAIK the current name doesn't mention that we prefer either one of the two. What are your thoughts 😄?

[oxkitsune]

I feel like the name is fine like this, as we want emphasis on the (im)mutability of the resulting collection.

[Stephan202]

Rebased and added a commit. Tnx for picking this up!

Alternative suggested commit message:

Introduce `CollectorMutability` check (#135)

This check flags `Collectors.to{List,Map,Set}` usages, suggesting that they be
replaced with expressions that clearly indicate the (im)mutability of the 
resultant collection.

Refaster templates that violated the new check are simplified by removing the
relevant expressions; they were too contrived to be updated some other way.

[Stephan202]

Here and above: IIUC we may also need to introduce a (static) import.

[Stephan202]

Here and below: this breaks if the original code didn't statically import to{List,Map,Set}. (Will add a test case.)

[Stephan202]

We can also convert the three-argument case.

[Stephan202]

I guess WARNING is more appropriate. (Within the Picnic context this doesn't matter, as we build with -Werror.)

[Stephan202]

Or FRAGILE_CODE? 🤔

[Stephan202]

We also have a few violations of this checker in the current repo; will add a commit to resolve those. (In this case IMHO simply dropping the relevant Refaster templates seems appropriate.)

[Stephan202]

(Also verified that the code now has 100% mutation coverage.)

[rickie]

Nice feedback @Stephan202.

The code LGTM!

Pushed some changes, we could further cleanup some of the tests.

[Stephan202]

Tnx for the extra cleanup @rickie! Feel free to merge if no further changes are planned.

[Stephan202]

@rickie another thought I had (and this one will be tricky to implement, I suspect, so not high on the list): we should also be able to automatically validate that elidedTypesAndStaticImports is minimal. (And perhaps sorted?)

[rickie]

Nice, will write it down.

[rickie]

😄