diff --git a/docs/code-guidelines.md b/docs/code-guidelines.md index 9a78c3c8..5bebc5fc 100644 --- a/docs/code-guidelines.md +++ b/docs/code-guidelines.md @@ -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. @@ -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.