Meaningless Size Comparison
ID |
kotlin.meaningless_size_comparison |
Severity |
high |
Remediation Complexity |
trivial |
Remediation Risk |
low |
Remediation Effort |
low |
Resource |
Reliability |
Language |
Kotlin |
Tags |
collection, reliability, suspicious-comparison |
Description
Reports a comparison of a size against a bound that size can never cross, so the test always
yields the same answer. size on a collection, map or array, length on a CharSequence and
the argument-less count() are all non-negative by construction: >= 0 and > -1 are therefore
always true, and < 0 and ⇐ -1 always false.
Both operand orders are recognised, so 0 ⇐ items.size is reported alongside
items.size >= 0.
Rationale
if (items.size >= 0) { // always taken — guards nothing
process(items)
}
if (items.size < 0) { // never taken — the body is dead code
reportCorruption()
}
if (items.count() > -1) { // always taken
process(items)
}The condition compiles and runs, so nothing warns about it, yet it carries no information. Either the guard is redundant and hides the fact that the body is unconditional, or — far more often — an emptiness check was intended and the off-by-one bound now lets every input through, including the empty one the guard was written to reject.
Only the bounds 0 and -1 are considered. Every other bound states a real condition:
items.size >= 42 and items.size < 1 are both perfectly meaningful. Equality is out of scope
too, because items.size == 0 is a legitimate — if unidiomatic — emptiness check rather than a
constant result.
A member named size is trusted to be non-negative when the receiver resolves to a standard
collection, sequence, array or character-sequence type, or when the receiver’s type cannot be
resolved at all. A receiver that resolves to a type declared in the analysed project is not
trusted, since a hand-written val size: Int really can go negative. A bare identifier named
size is never matched, for the same reason: a plain Int counter can be negative, so
size >= 0 on one is a genuine test.
Remediation
Decide what the guard was meant to say and write that instead. When the intent was an emptiness check, use the idiomatic property rather than a comparison against zero.
if (items.isNotEmpty()) { // what `items.size >= 0` usually meant
process(items)
}
if (items.isEmpty()) { // what `items.size < 0` usually meant
reportEmptyInput()
}
if (items.size >= MIN_BATCH) { // a bound the size can actually cross
process(items)
}When the guard really is redundant, delete it and let the body run unconditionally — that is what the code already does, and saying so removes a false suggestion that some inputs are filtered out.