Loop body always jumps out, so it runs at most one iteration

ID

vbnet.correctness.loop_at_most_one_iteration

Severity

high

Remediation Complexity

medium

Remediation Risk

medium

Remediation Effort

low

Resource

Control Flow

Language

VB.NET

Description

Flags an Exit For / Exit While / Exit Do / Exit Sub / Exit Function / Return statement that sits directly in the body of a For, For Each, While or Do loop with no If, Select Case or Try anywhere in that loop guarding it. Because the jump is reached on every pass, the loop leaves after its first iteration and every element after the first is never seen.

Rationale

A loop that always leaves on its first iteration is not a loop - it is a single statement wearing a loop’s clothing, and the two readings of it disagree. A reader (and any reviewer) assumes the body runs for each element; the code runs it once. This normally means the exit is in the wrong place: a guard If was deleted or never added during a refactor, the exit was pasted one nesting level too high, or the loop was left behind after being replaced by a direct lookup. The result is a silent data-processing bug - only the first record is validated, only the first file is imported, only the first retry is attempted - which passes any test whose fixture has a single element and fails in production against real, multi-element data.

The following code illustrates the pattern detected by this rule:

Public Function FirstCarrier(ByVal carriers As List(Of String)) As String
    For Each carrier In carriers
        ' FLAGGED: Loop body always jumps out, so it runs at most one iteration
        Return carrier
    Next
    Return Nothing
End Function

Remediation

Decide which of the two readings was intended. If only the first element was ever wanted, delete the loop and say so directly: carriers.FirstOrDefault(), list(0), or a single TryGetValue - a reader then sees the intent without having to reason about an exit. If the body really should run for each element, move the jump under the condition that should govern it (If carrier = wanted Then Return carrier), or use Continue For to skip just the current element. When the loop was a retry and should stop only on success, put the exit inside the success branch and leave the failure path to loop round.

Configuration

This detector does not need any configuration.

References