New error-prone rule: PreferImmutableStreamExCollections

[iamdanfox]

Before this PR

The apollo codebases use StreamEx heavily. As @avisbal and @athorwall have commented in recent CRs, we try to use toImmutableMap where we remember but aren't completely consistent with this.

After this PR

==COMMIT_MSG==
A new error-prone rule PreferImmutableStreamExCollections converts the StreamEx toMap() -> toImmutableMap(), toImmutableList() and toImmutableSet()
==COMMIT_MSG==

Possible downsides?

So if we want to be really picky here, then technically the Collections.unmodifableSet wrapper means we're gonna allocate 1 (one) whole extra object. I don't think this is a blocker, because if someone is already tolerating a whole stream full of lambdas, then I think they can deal with one extra allocation.

    public Set<T> toSet() {
        return rawCollect(Collectors.toSet());
    }

    public Set<T> toImmutableSet() {
        Set<T> result = toSet();
        if (result.size() == 0)
            return Collections.emptySet();
        return Collections.unmodifiableSet(result);
    }

[changelog-app]

Generate changelog in changelog/@unreleased

Type

• Feature
• Improvement
• Fix
• Break
• Deprecation
• Manual task
• Migration

Description

A new error-prone rule PreferImmutableStreamExCollections converts the StreamEx toMap() -> toImmutableMap(), toImmutableList() and toImmutableSet()

Check the box to generate changelog(s)

• Generate changelog entry

[carterkozak]

Nit: this is the only message most people see, I'd exclude overriding the more verbose message from the class definition since that explains the intent.

[iamdanfox]

If someone out there is relying on the mutability of their streamex collections, e.g.:

Map<String, String> foo = myStream.toMap();
foo.remove("bar");
return foo;

Then this auto-fix will make a change which still compiles but would throw when they run this. This should get caught by tests, but if they also don't have tests then this could break someone in production.

[carterkozak]

I think we can avoid a bit of duplication using the SuggestedFixes helpers:

SuggestedFixes.renameMethodInvocation(tree, "toImmutableMap", state)

[carterkozak]

Limiting to streamex as opposed to standard streams reduces some danger, but I wouldn't be surprised if there are places which expect to mutate the result. I wouldn't want the robots to cause p0s.
It's unfortunate that the toImmutable*() methods don't return a special type like ImmutableList where the type system could help us out. Flow analysis is more difficult.

[iamdanfox]

Yep, this is a theoretical possibility. In practise, I think it's gonna be a very small number of repos who have decided they like the functional style of stream-ex but then rely on mutability (who are these people ?!?!) and they'd also need to have no unit/integration test coverage in order for a release to get out. Moreover we have blue-green deploys which are supposed to minimize the user-facing impact if something slips through the net 🤷

[carterkozak]

What if we don't auto-fix, but update the description to include both a warning about mutability, and instructions to apply the fixes ./gradlew classes -PerrorProneApply=PreferImmutableStreamExCollections

[iamdanfox]

I think I wanna stick with the autofix here. Immutability is such a core tenet of conjure and the way we write software at Palantir, that I think it's worth ratcheting this towards a better world. This PR should have a pretty minimal scope, because it won't touch the regular usage of java.util.streams.Stream.

Also since this is potentially gonna make code changes, people will have to +1 it before it goes in.

[carterkozak]

Error-prone automatically uses the message from the class-level annotation if we don't set a message here!

[iamdanfox]

So I've intentionally avoided targetting java.util.stream.Stream for now, because I think it's extremely widely used, but perhaps for users of stream-ex I should actually try to zap the regular .collect() methods, e.g.:

-StreamEx.of().collect(Collectors.Set())
+StreamEx.of().collect(Collectors.toUnmodifiableSet())

I'd need to detect if sourceCompatibility was >= 10 somehow...

[carterkozak]

Well, if you have StreamEx, you probably already have a guava dependency -- can use MoreCollectors.toImmutable*() in that case (across all source versions)

[iamdanfox]

Interestingly I ran ./gradlew classes -PerrorProneApply=PreferImmutableStreamExCollections on a large internal project and it didn't make a single edit... although it seems there was only one mention of EntryStream in the whole project, so maybe stream-ex is just more of a foundry thing??

Got a +500 -500 diff on https://internal-github/f/a/pull/8412 though

[svc-autorelease]

Released 3.71.0