Java bug using Collectors::toMap() with ArrayList::new

java

(Davehoch) #1
  • description of the Rule answering to the question “why?” : what’s the impact to keep this code as it is ?
    This bug was found in a production app, resulting in it using way more memory than intended. The developers were trying to implement something like a Multimap, but didn’t realize that the code was calling the ArrayList constructor that takes an argument for the initial size. They intended to have empty lists.
    I have already successfully implemented this rule, and am ready to submit the code.

  • snippet of Noncompliant Code

// Create a Multimap of account ID -> List of attributes.  Make sure that there's an entry for every account.
Arrays.asList(1, 2, 54000).stream().collect(Collectors.toMap(Function.identity(), ArrayList::new));
  • snippet of Compilant Code (fixing the above noncompliant code)
Arrays.asList(1, 2, 54000).stream().collect(Collectors.toMap(Function.identity(), id -> new ArrayList<>()));
  • external references and/or language specifications

  • type : Bug, Vulnerability, Code Smell ?
    Bug

  • tags


(Nicolas Harraudeau) #3

Hi @davehoch,

Thank you for this suggestion. This is indeed a tricky bug.
I expect this problem to be valid every time ArrayList::new, HashMap::new or any other collection constructor is given as a java.util.function.Function argument. It is already rare to set the initial capacity of a collection, it would be even more so to do it via a generic Function.

Thus I would make the rule more generic: “Collections’ constructors should not be used as Functions”.

Of course this rule would raise only when the constructors are given as java.util.function.Function arguments. No issue would be raised when they are provided as java.util.function.Supplier, as it is the case in java.util.stream.Collector.of(...).

Do you see any issue with such a rule?


(Davehoch) #4

Hi @Nicolas_Harraudeau,

Thanks for the suggestion. I don’t see any issues with it, and it definitely makes the rule more general, and more likely to catch errors. I’ll see if I can implement it that way.

-Dave


(Davehoch) #5

Hi @Nicolas_Harraudeau,

I have the rule coded up the way you suggested. I’d like to contribute it to SonarSource. How do I get an RSpec Jira case created?

Thanks!