FR: Prefer ImmutableSet.toImmutableSet() over Collectors.toSet() for `public static final` fields

[iamdanfox]

What happened?

In an internal library, we a nice public static final field:

public static final Set<String> WHITELISTED_HEADERS = ImmutableSet.of(
    HttpHeaders.ACCEPT,
    ...
).stream().map(String::toLowerCase).collect(Collectors.toSet());

But it turns out that Collectors.toSet() gives us a mutable HashMap, so anyone could actually just access this WHITELISTED_HEADERS field and add/remove stuff!!

What did you want to happen?

I think we should write an error-prone check to just prefer the toImmutableSet() collector!

 public static final Set<String> WHITELISTED_HEADERS = ImmutableSet.of(
     HttpHeaders.ACCEPT,
     ...
-).stream().map(String::toLowerCase).collect(Collectors.toSet());
+).stream().map(String::toLowerCase).collect(ImmutableSet.toImmutableSet());

[markelliot]

Probably should only fire this if the thing being set is final.

[schlosna]

Does this get caught by promoting https://errorprone.info/bugpattern/MutableConstantField to error? That would also flag the downstream modification via https://errorprone.info/bugpattern/ImmutableModification

[stale]

This issue has been automatically marked as stale because it has not been touched in the last 60 days. Please comment if you'd like to keep it open, otherwise it'll be closed in 7 days time.

[iamdanfox]

In Java9+, we might actually want to recommend Collectors.toUnmodifiableList() as this uses the new List.of() implementation under the hood rather than relying on guava :)