SONARKT-301 - Implement rule S6524: collection should be immutable

[erwan-serandour-sonarsource]

No description provided.

[erwan-serandour-sonarsource]

add new clean code taxonomy

[johann-beleites-sonarsource]

In terms of how we go about visiting the AST, it looks fine on first glance. Although perhaps it is worth noting that we currently do not look at properties defined outside of functions (private class & file-level properties). Was that intended?

In terms of determining if a collection has been written to, so needs to be mutable, I think we need to improve the logic a bit.

The current logic assumes that a collection is not written to by default and only if one of the listed funs is called, do we recognize it as written to. If I define a custom collection extension fun or one that takes a collection as argument, which writes to the respective collection, we will miss the fact that this function is a writing function everywhere else. We can probably depend on the type information instead, to see if a collection is only used in contexts where an immutable type is required. However, if we don't understand what a call to a function does w.r.t. the collection argument/receiver, we should assume it is modified to avoid FPs. In other words, our default assumption should be "the collection IS modified" and we need to prove otherwise to raise an issue.

In general, I strongly recommend focusing more on the test cases. In fact, for rules like this, it helps to really go with TDD and think of all kinds of crazy ways you could modify collections first, even before you start writing the rule. Then start simple and see how that tricks the logic you write, accommodating the complexities bit by bit. Especially if you're not sure of the logic, write more test cases! Have some complex examples in there, where you have functions that call functions or where you pass function calls as an argument to another function call. Use .let, with(), etc. This way you have to depend way less on the PR review and ultimately it will save you some time too, because of less back-and-forth.

[johann-beleites-sonarsource]

As discussed

[johann-beleites-sonarsource]

You can directly return from this if-else, no need in saving the result in a val, just to check it on the next line.

[johann-beleites-sonarsource]

These mutateCollection functions are called like this because they filter for expressions that mutate a collection, correct? I would call them mutatesCollection with an s, to make that even more clear.

[johann-beleites-sonarsource]

I think it would be readable in this instance to get rid of the val and move the condition into the if (...) directly.

[johann-beleites-sonarsource]

You can chain the calls directly, no need for the val.

[johann-beleites-sonarsource]

I think this may be related to this, we currently raise issues where we shouldn't. E.g.:

    fun MutableList<Int>.compliantDelegate(): Int { // Compliant
        if(add(1)) {}
        return reduce { acc, it -> acc + it }
    }

[sonarqube-next]

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
97.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

[johann-beleites-sonarsource]

As discussed today, let's double-check the results during verification later today.