Inappropriate Collection Calls
ID |
kotlin.inappropriate_collection_calls |
Severity |
high |
Remediation Complexity |
trivial |
Remediation Risk |
low |
Remediation Effort |
low |
Resource |
Reliability |
Language |
Kotlin |
Tags |
best-practice, collection, reliability |
Description
Reports remove(2) on a mutable list of integers, which removes the element 2 rather than the
element at index 2.
On a list of integers these two operations are spelled almost identically — remove(index) against
removeAt(index) — and only the second one is about position. Every other element type makes the
mistake impossible to write, which is why a list of integers is the one shape this rule reports.
Rationale
val ids: MutableList<Int> = mutableListOf(10, 20, 30)
ids.remove(0) // searches for the value 0, finds none, removes nothing
ids.removeAt(0) // drops 10 — what the line above reads asremove on a MutableList<Int> takes an element, not an index. The call compiles, returns a
Boolean that callers rarely inspect, and quietly does nothing whenever no element happens to
equal the index. When one does, it removes an element from somewhere else in the list — a bug that
depends on the data and so tends to survive testing.
All four of the following must hold before anything is reported, and each condition exists to keep a deliberate removal-by-value out:
-
The receiver is declared as a list type —
MutableList,ArrayList,LinkedListand friends. Only a list has positions, so only a list has aremoveAtforremoveto be confused with.MutableMap.remove(key)andMutableSet.remove(element)have no index overload and are correct. -
The element type is integral (
Int,Long,Short,Byte). AMutableList<Any>orMutableList<Number>accepts an integer too, but there the author is far more likely to mean the value, so those are left alone. -
The argument is a plain decimal integer literal.
remove(position)says nothing either way;remove(-1)cannot be an index and is therefore unambiguously a value; andremove(2L)andremove(0x02)both spell a value on purpose. -
The call is
removewith exactly one argument.
A list whose element type is only inferred — val ids = mutableListOf(1, 2, 3) — is not reported.
The element type is read from the declaration, and that one states no type arguments, so the
receiver must be declared as MutableList<Int> for the rule to see anything.
Why this differs from the same rule in other languages
In Java and C# this rule id covers a different defect: a collection call whose argument type can
never match the element type, such as List<String>.contains(anInt). That is possible there because
the JDK and .NET methods take a bare Object.
It cannot happen in Kotlin. The standard library declares the parameter as the element type
instead — Collection<out E>.contains(element: E), MutableCollection<E>.remove(element: E),
List<out E>.indexOf(element: E), Map<K, V>.containsKey(key: K) — so List<String>.contains(1)
is a compile error, not a silently-false call. (The @UnsafeVariance annotation on those
parameters only suppresses the declaration-site variance error; the call site still requires the
element type.) Porting the check literally would produce a rule that can only fire on source the
compiler already rejects, so this rule covers the Kotlin member of the same family instead: a
collection call that does not do what it looks like.
Remediation
Decide whether the argument is a position or a value, and use the operation that says so.
ids.removeAt(0) // remove by position
ids.remove(value) // remove by value, where `value` names a valueWhen the intent really is removal by value and the value is a literal, naming it removes the ambiguity for the next reader as well:
val placeholderId = 0
ids.remove(placeholderId)Note that removeAt throws IndexOutOfBoundsException for a position outside the list, whereas
remove returns false for a missing value. Code that relied on the silent no-op needs a bounds
check once it is corrected.