Skip to content

Commit 1fc8ec2

Browse files
authored
fix: simplify_for_range lint rule (#169)
## Description This PR fixes long-standing **false-positive** issues in the `simplify_for_range` lint rule. Previously, the rule would incorrectly suggest rewriting counter-based loops even when the loop could **not** be safely converted to a `range` loop. Example false-positive from real Gno code: ```go for attempt := 0; attempt < maxUint256SampleAttempts; attempt++ { ... } ``` This was incorrectly flagged as: ``` counter-based for loop can be simplified to range-based loop ``` But `maxUint256SampleAttempts` is a numeric bound, not an iterable. The suggestion: ```go for attempt := range maxUint256SampleAttempts { ... } ``` is invalid code. This PR redesigns the entire detection algorithm to match Go/Gno semantics and eliminate invalid suggestions. ## Root Cause The previous implementation relied solely on syntactic matching: * init: `i := 0` * condition: `i < something` * post: `i++` It **never verified** that: * `something` is `len(xs)` * `xs` is iterable (slice/array/string) * the identifier `i` refers to the same object throughout * shadowed variables in the loop body do *not* modify the loop counter * declarations (`var i`) inside the body were not considered * the AST declared-object identity (`Ident.Obj`) was correct * type information was used only partially As a result, any counter loop that "looked like a typical index loop" was flagged, even when the bound was: * numeric constant * numeric variable * arithmetic expression * function call * map length * anything non-iterable Additionally, the loop-variable modification logic only inspected `AssignStmt` and `IncDecStmt`, so declarations such as: ```go var i int // shadows loop var, not a modification ``` were mis-analyzed. ### Testing The PR adds tests for: * numeric bounds * constant bounds * arithmetic expressions * function call bounds * non-iterable `len(...)` expressions * map and struct fields * loop variable shadowing * multiple modification patterns All false-positive cases now pass. ## New Algorithm (Pseudocode) ```pseudo function detectSimplifiableForRange(forStmt): # 1. Match canonical "i := 0" if init is not "i := 0 (short var decl)": return no match # 2. Match canonical "i < len(xs)" if condition is not "i < len(expr)": return no match # 3. Ensure expr is iterable (slice, array, string) if type(expr) not in {slice, array, string}: return no match # 4. Match canonical increment "i++" if post statement is not exactly "i++": return no match # 5. Ensure loop variable is not modified in body if body contains assignment, inc/dec, or multi-assign targeting the loop variable: return no match # 6. However: declaration of the same name (shadow) is allowed shadowing := detectDeclStmtsCreatingNewIdent("i") inside body shadowing does NOT count as modification # 7. Ensure loop variable is not used after loop if any use of ident("i") occurs after forStmt.End: return no match # If all canonical conditions satisfied: emit suggestion: "for i := range xs { ... }" ```
1 parent b7b3c7c commit 1fc8ec2

File tree

2 files changed

+609
-155
lines changed

2 files changed

+609
-155
lines changed

0 commit comments

Comments
 (0)