The warning “Replace this usage of ‘Stream.collect(Collectors.toUnmodifiableList())’ with 'Stream.toList()” is incorrect.
What language is this for?
Java 16
Which rule?
Replace this usage of ‘Stream.collect(Collectors.toUnmodifiableList())’ with ‘Stream.toList()’
Why do you believe it’s a false-positive/false-negative?
While toList() does return an unmodifiable list, it permits null values within the list. To ensure a list without any null values, one must use collect(toUnmodifiableList()) as an alternative. However, when using this method, Sonar flags it as incorrectly, potentially overlooking errors that could be identified during list initialization.
Used product:
SonarLint - IntelliJ Version 10.11.1.79663 in connected mode with SonarQube
How can we reproduce the problem? Give us a self-contained snippet of code (formatted text, no screenshots)
// Example using Stream.collect(Collectors.toList())
List<String> listWithNulls = Stream.of("A", "B", null, "C")
.collect(Collectors.toList());
System.out.println("List with nulls (toList): " + listWithNulls);
// Example using Stream.collect(Collectors.toUnmodifiableList())
try {
List<String> unmodifiableList = Stream.of("A", "B", null, "C")
.collect(Collectors.toUnmodifiableList());
System.out.println("Unmodifiable list (toUnmodifiableList): " + unmodifiableList);
} catch (NullPointerException e) {
System.out.println("NullPointerException caught: Collectors.toUnmodifiableList() does not allow nulls");
}
// Example using Stream.toList()
List<String> unmodifiableListWithNulls = Stream.of("A", "B", null, "C")
.toList();
System.out.println("Unmodifiable list with nulls (toList): " + unmodifiableListWithNulls);
Hey @MonkeyFist,
Welcome to the community and thank you for reporting this issue.
This is a very nice catch because we do not want to suggest changes that would break the semantic guarantees of the value returned by Collectors.toUnmodifiableList.
My suggestion, as detailed in this ticket, is that we keep raising an issue on the code but suggest replacing it with a .filter(e -> e != null).toList().
But then the rule documentation should be updated because what is gained in being explicit is lost in the brevity.
Hi Dorian,
thank you for your reply and the suggestion! I think generally speaking, it does make sense. However, I still have some doubts regarding this issue.
The replacement with .filter(e -> e != null).toList() is a good suggestion; however, in our codebase, we use Collectors.toUnmodifiableList. We want the NullPointerException to be thrown because it makes it easier for us to pinpoint possible sources of errors. If the erroneous items are filtered out, we might never know that they existed in the first place. They indicate to us as programmers that we have failed to handle incorrect data properly before creating the list.
Additionally, handling the NullPointerException directly helps us maintain the integrity and reliability of our data processing pipeline. By allowing the exception to occur, we can ensure that all elements in our list are valid and that any issues with null values are addressed promptly during development. This proactive approach helps us deliver more robust and error-free code.
Overall, while filtering out null values can simplify some parts of the code, it might obscure underlying issues that need to be addressed.
Therefore, it might be reasonable to consider deactivating the issue flag for Collectors.toUnmodifiableList entirely.
Somewhat unrelated side note: The recommendation to use Stream.toList() instead of Collectors.toList() was also a pitfall for us when using the GWT framework, since the list returned by the former does not seem to be GWT-compatible whereas the one returned by the latter is (since it just creates a plain ArrayList).
Not sure if they already fixed that by adding better JRE API emulation in one of the latest two releases, since I don’t see an explicit mention of it in the release notes. But it was enough for us to disable that rule altogether.
Thank you for taking the time to share why allowing exceptions is useful to the way you work.
I think there is some room for discussion about the relevance and scope of this rule more generally, and having this kind of user feedback will help with that.
Is it useful for you to have Collections.toList() flagged by the rule? If not, is it possible to disable S6204 from your quality profile?
Thank you for sharing this. I am not sure we considered the impact on alternative runtimes, and I will bring that up with the group in our discussion of the rule.
In our codebase, we avoid using Collections.toList(). However, suggesting toList() as an alternative might not be ideal, as it produces an immutable list, while Collections.toList() returns a mutable one. I’m not certain if Sonar flags list access appropriately in this context. In my view, the rule should either be disabled completely or flagged only when the list isn’t accessed, and only for Collectors.toList()
The whole issue is made more tricky by the fact that Collectors.toList() actually makes “no guarantees on the type, mutability, serializability, or thread-safety of the List returned”. It just happens to be a regular ArrayList in the JDK implementations I’ve seen so far, but there is no contract in place that prevents a future JDK or a distribution from a specific vendor from arbitrarily changing it to something else (except for not wanting to break all the legacy code that has been written with the current implementation in mind). Even the fact that it doesn’t break GWT RPC serialization due to being a proper ArrayList is more like a random implementation detail and not something that is guaranteed by the API.
So solely from an API point of view, a Sonar rule cannot not really make substantial assumptions about whether Collectors.toList() can or cannot be used in the same places as Collections.toList(). It could only reason that the replacement is definitely not safe if it can determine that the place where the list is used attempts to mutate it. But that usage could be so far removed from the actual call hierarchy that determining a mutation might be unreasonable.
On the other hand, if pretty much all known implementations in the JDK do the same and it’s exceedingly unlikely that they will ever change it for no reason, then not using that fact in the rule’s reasoning would just be an unnecessary handicap.