However, collect(Collectors.toList()) and toList() return different results. The former returns an ArrayList (which can call the set method), while the latter returns a List (which cannot call the set method). Therefore, I believe that the java:S6204 rule should be removed to avoid trapping newcomers. When my code was causing an error, I spent 10 minutes troubleshooting before finding the cause.
We have discussed improving or deprecating the rule in the past but it would help us gain a better understanding of how it broke. Was integerList returned or passed as an argument to some method or constructor?
We should have a comprehensive range of tests for modifying method calls (e.g., add, remove), but we might have failed to detect calls outside of the method where the list was created.
Could you share a snippet of code that reproduces the issue (with the creation of the list and where the bug is triggered in the code)?
/**
* Test the difference between .toList() and .collect(Collectors.toList())
* JDK 17
* @param args args
*/
public static void main(String[] args) {
//------------------------------------------------------Preparation
// Create an ArrayList to store string elements
java.util.List<String> stringList = new java.util.ArrayList<>();
// Add the element "1" at index 0
stringList.add("1");
// Replace the element at index 0 with "2" (this operation is allowed because stringList is of type ArrayList.class, so it can use the set(int index, E element) method)
stringList.set(0, "2");
//------------------------------------------------------Test .toList()
// However, if .toList() is used to obtain integerList, it is actually of type List.class, so it cannot call the set() method
java.util.List<Integer> integerList = stringList.stream().map(Integer::valueOf).toList();
// The following line of code will throw an error: java.lang.UnsupportedOperationException
integerList.set(0, 3);
//------------------------------------------------------Test .collect(Collectors.toList())
// However, if we use the traditional .collect(Collectors.toList()) (common in JDK 8)
// Then integerList is actually of type ArrayList.class, so it can call the set() method
java.util.List<Integer> integerList2 = stringList.stream().map(Integer::valueOf).collect(Collectors.toList());
integerList2.set(0, 3);
}
Actually, I want to point out that when writing code like .collect(Collectors.toList()), S6204 will suggest replacing it with .toList().
However, .toList() and .collect(Collectors.toList()) return fundamentally different results.
Therefore, this S6204 suggestion can lead beginners to believe that .toList() is a direct replacement for .collect(Collectors.toList()),
ultimately causing errors when they try to call the set() method in the future.
Thank you @15102045545 for sharing both the code and your explanation.
I can see how this code fails at runtime but SonarQube for IDE does not raise any issue in this specific snippet because it can tell that the list integerList2 is modified after creation (if you replace the call to set with a non-modifying call like get, S6204 will flag the list creation as expected)
In the problems you encountered while fixing your code, was the list modified in a different block (method, class, branch, …) than the one where it was created?
Cheers,
Dorian
PS: would it be possible for you to upgrade to a more recent version of SonarQube for IDE in IntelliJ?
Yes, your statement is entirely correct. When creation and usage occur together, S6204 does not appear.
Furthermore, as you suspected, the List that triggered the UnsupportedOperationException was indeed created and used in different methods and classes. The “creation” and “usage” are separated by one class, one method, and over 20 lines of code.
Additionally, the problematic List is a nested List<List>. >_<!
I won’t upgrade SonarLint for now, but I will in the future. SonarLint is a great tool, and I really like it.