[java:S6204] Improve suggestions

Qube: Community 9.9

The Java rule S6204 suggests to replace the use of

new ArrayList<>().stream().collect( Collectors.toList() );

with

new ArrayList<>().stream().toList();

More precisely it suggests “… when unmodifiable list needed”. In the inline hint from Lint it just says “Replace this usage of ‘Stream.collect(Collectors.toList())’ with ‘Stream.toList()’”. This leads you in a wrong direction. Hence the text should be improved.

First of all the Java API is a bit odd here.

Stream.toList() should do the same thing, that Stream.collect( Collectors.toList() ) does. But it doesn’t. The first unfortunately produces an unmodifiable List, the latter has no guaratee about that. At least there is Collectors.toUnmodifiableList(), which implies, that Collectors.toList() does the opposite. Two methods for the same pupopse from the very same framework should do the same thing.

Ok this is not your problem. But thing is, that rule S6204 does not reflect that enough.

The short hint from Lint just wants me to use Stream.toList(), which is a bad replacement, if I usually expect the list to be mutable (which Collectors.toList() used to do). So the short hint should somehow mention a solution for mutable and one of immutable to not lead you in the wrong direction.

And the rule description should not call Collectors.toUnmodifiableList() non compliant, since Stream.toList() might be a good replacement for that. But it is not for Collectors.toList(). And APIs should always be even, which Stream.toList() with the lack of Stream.toUnmodifiableList() is not, which Collectors.toList() and Collectors.toUnmodifiableList() is in theory, but documentation is not.

Of course there should be an attempt to fix that in JDK. Stream.toList() should guarantee a modifiable list and there should be Stream.toUnmodifiableList() for the opposite.

Hello @mfroehlich,

Rule S6204 intent is to use unmutable lists instead of mutable lists whenever possible. Specifically, the rule correctly suggests replacing stream.collect(Collectors.toList()); with stream.toList(); when there is no modification detected on the returned list, and thus, it is incorrect to define it as a mutable list.

Please provide a code example or reproducer if the rule doesn’t behave accordingly.

Cheers,
Angelo

Hey Angelo,

sorry for the late response.

The point is, that there can be use cases (and we have many of them), where the resulting List ist used much later in the code flow, where the rule cannot see it. And if the List is immutable there, you’ll get an unexpected exception.

Hello @mfroehlich,

The rule detects any modification on the collection within the file in which it is defined. When there is at least one modification detected, it raises an issue since an immutable collection would be more appropriate.

Modifying a collection somewhere else, especially on a different file, comes with the risk of data corruption and inconsistency; the consequences can be dangerous in a concurrent system. The best solution in such cases is to encapsulate the collection that is supposed to be shared between files and control its access.

I hope this makes sense to you. Please correct me if I’m missing or misunderstanding your use case.

Cheers,
Angelo

Hi Angelo,

tbh this doesn’t make any sense to me at all.

The return value is not an internally used collection, which’s outside modification would introduce a big risk indeed, but it is a freshly created collection jhust to be used outside. Hence no risk at all. Returning an unmodifiable collection can be a good option. But you can never ever do this change blindly. You have to know the outside usages exactly. Having a read only collection as return value is a very special case. We use it, where we return RO views on internally used collections. But there’s not even a performance gain on RO collections. So, I don’t even see the point here.