Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion docs/code-guidelines.md
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ The result is a probabilistic defect, so a green test run is not evidence. `carg
**Order-sensitive consumers.** The iteration is only a problem once something downstream cares about position. These all care:

- Positional indexing, including round-robin selection by index.
- `min_by_key` and `max_by_key`. Both return the FIRST extremum, so they tie-break on input order.
- `min_by_key` and `max_by_key`, which tie-break on input order in opposite directions. `min_by_key` and `min_by` return the **first** minimum; `max_by_key` and `max_by` return the **last** maximum. A fix therefore has to face the tie component the right way for the call it is fixing, and two arms of one policy can need opposite directions.
- A prefix: `take(n)`, or an early `break`, or an early `return` out of the loop.
- A loop-carried accumulator, such as a seed advanced once per element.
- Anything documented as a priority order, whether or not the current consumer reads it that way.
Expand Down Expand Up @@ -197,3 +197,5 @@ There is no `make verify-*` target, no CI job and no script for this rule. That
- **A `Vec` built from an unordered-map view and consumed order-sensitively.** Twenty-two flags on the current tree. It flags all three #1286 sites *after* PR #1288 fixed them, because the fix keeps the shape and changes only whether the sort key is total, a distinction no regex can draw. Its 3-of-7 catch rate held only against pre-fix trees; on any tree where the class has been fixed, those three become permanent false positives. It reaches none of the four separately filed instances, each for a different structural reason: #1265 is a loop-carried seed with no `Vec`, no sort and no index; #1267 iterates the map directly in a `for` and never calls `.keys()` or `.values()` at all; #1276 returns early out of a `for` with no `Vec` binding to bind a consumer to; and #1277's producer and its order-sensitive consumers are in different modules. A tree-wide scan for `type X = HashMap<...>` does remove the type-alias blind spot that `WeightMap` creates, so that one limit is fixable; the other three are not, at this level of analysis.

The general form is undecidable from the source alone, because whether an iteration order matters depends on a consumer that is often in another module. So this rule is enforced by review and by the testing rule above. Re-open the question if a future instance takes a shape either candidate would have caught.

That blind spot has already cost a finding, recorded here so the decision above stays honest rather than only favourable. #1293 is an eighth instance, in `BatchScheduler::select_eviction_victim`, and unlike the four latent ones its tie is reachable: it breaks on `generated_tokens.len()`, a small integer that sequences decoding in lockstep share routinely. Neither candidate check flagged it, because the receiver is a cross-module accessor method (`ActiveBatch::iter_sequences`) rather than a literal `.values()` on a map declared in the same file, which is exactly the limitation described above for #1277. A human sweep found it. That is the cost of the decline, and it is the number to weigh if the question is re-opened.