The rule java:S6204 suggests .collect(Collectors.toList()) to create a modifiable list, but the javadoc for Collectors.toList states that it does not make any guarantees about the returned list being modifiable.
Link to documentation: https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/stream/Collectors.html#toList()
It feels like an API design mistake. If I want a modifiable list, I have to use toCollection and specify the concrete list I want, it seems.
Hello @grossjohann and welcome to the Sonar Community.
Thank you for reporting this inaccuracy; [SONARJAVA-5056] - Jira will take care of updating the documentation.
Cheers,
Angelo
I would like to make some addions to this topic.
Qube version: Community Build v25.7.0.110598
The rule summary says: “Stream.toList() method should be used instead of collectors when unmodifiable list needed“
This seems to assume, that an unmodifiable list is always wanted. And this is kind of a dangerous assumption. At least the wording is problematic. If the resulting list is consumed in the local code, I can very well decide, if modifiable or unmodifiable is needed. But in many cases the resulting list is passed to other more or less complex code. And I cannot easily tell, if the list is (to be) modified in the consuming code. Hence the rule should not so easily push me to producing an unmodifiable list. While toList() gives no guaratees, whether the list is modifiable, we all know, it is modifiable. And my consuming code MAY assume the list to be modifiable. I (myself or other team members following the rule) don’t want to naively change my code to produce unmodifiable lists just to see my application fail the other day because of that.
At the same time the extended rule description lists non compliant and compliant code. The compliant code contains the exact example from the non compliant code with Collectors.toList(). So at least this is not correct.
Of course one could argument, just as Kai did, that this a design mistake. So, if toList() does not make any assumptions, but there is toUnmodifiableList() next to it, why should this be non compliant? The method clearly tells, what the result is about. And if the list type is not important for me, toUnmodifiableList() is just fine. A method toModifiableList() is missing here. I just wish, someone was able to report this to the JDK guys.
Using toCollection( ArrayList::new ) seems to be the only compliant solution, if I want a modifiable list. It is a bit bloated imho. But at least you exactly know, what kind of list will be produced.
My conclusion is, that this rule should be clear in the short description about code, that exactly tells, whether the resulting list is modifiable or unmodifiable. And the non compliant and compliant code lists should not be contradictory, but list working and non complained examples for a modifiable and unmodifiable result list.
What do you think? Can this rule be improved?
Hey @mfroehlich
Is this related to the current topic, or would it be more appropriate to create a new one? Let’s avoid making this thread a catch-all for all S6204 feedback.
Hey Colin,
I’m not sure. But I’d be fine with a separate topic. I just didn’t want to create duplicates as it looks “slightly related”.