|
| 1 | +/** |
| 2 | + * @fileoverview Catch the silent-no-op bug where the fleet's canonical |
| 3 | + * cached-length `for` loop is applied to a Set / Map / Iterable |
| 4 | + * instead of an array. |
| 5 | + * |
| 6 | + * The bug shape: |
| 7 | + * |
| 8 | + * const s: Set<string> = new Set() |
| 9 | + * … |
| 10 | + * for (let i = 0, { length } = s; i < length; i += 1) { |
| 11 | + * const item = s[i]! // s isn't indexable; type is undefined |
| 12 | + * … // body never runs (length is undefined) |
| 13 | + * } |
| 14 | + * |
| 15 | + * `Set` / `Map` / `WeakSet` / `WeakMap` / generic `Iterable` don't |
| 16 | + * expose `.length`, and `s[i]` isn't a defined access either. The |
| 17 | + * destructure `{ length } = s` reads `s.length === undefined`, the |
| 18 | + * test `i < undefined` is `false`, and the loop body never executes. |
| 19 | + * No type error, no runtime error — the iteration just silently |
| 20 | + * does nothing. Production code shipped with this pattern across |
| 21 | + * 4 files in socket-wheelhouse before the fleet hand-fix; this rule |
| 22 | + * blocks regression. |
| 23 | + * |
| 24 | + * Why it happens: the fleet's `socket/prefer-cached-for-loop` rule |
| 25 | + * rewrites array `.forEach` and array `for...of` into the cached- |
| 26 | + * length shape. Devs then apply the same shape by hand to Set / Map |
| 27 | + * iteration without remembering that those collections aren't |
| 28 | + * integer-indexable. |
| 29 | + * |
| 30 | + * Detection (no TypeScript type-checker available in the plugin): |
| 31 | + * |
| 32 | + * 1. Walk every `VariableDeclarator` and `Parameter` in scope to |
| 33 | + * build a per-file map `identifierName -> kind` where `kind` |
| 34 | + * ∈ {set, map, iterable, array, unknown}. Recognized signals: |
| 35 | + * |
| 36 | + * - `new Set(...)` / `new Map(...)` / `new WeakSet(...)` / |
| 37 | + * `new WeakMap(...)` → set/map kind |
| 38 | + * - `: Set<...>` / `: ReadonlySet<...>` / `: Map<...>` / |
| 39 | + * `: ReadonlyMap<...>` / |
| 40 | + * `: WeakSet<...>` / `: WeakMap<...>` annotations |
| 41 | + * → set/map kind |
| 42 | + * - `: Iterable<...>` / `: AsyncIterable<...>` annotations |
| 43 | + * → iterable kind |
| 44 | + * - `[…]` array literal / `: T[]` / `: Array<...>` / |
| 45 | + * `: ReadonlyArray<...>` → array kind (negative — |
| 46 | + * do NOT flag) |
| 47 | + * - everything else → unknown kind (skip) |
| 48 | + * |
| 49 | + * 2. On `ForStatement`, inspect the `init` for the canonical |
| 50 | + * shape: |
| 51 | + * |
| 52 | + * let i = 0, { length } = X |
| 53 | + * |
| 54 | + * i.e. `VariableDeclaration` with ≥ 2 declarators, the second |
| 55 | + * of which has an `ObjectPattern` LHS with a single `length` |
| 56 | + * property and an `Identifier` RHS `X`. Look up `X` in the |
| 57 | + * scope map — if it resolves to `set` / `map` / `iterable`, |
| 58 | + * report. |
| 59 | + * |
| 60 | + * False-negative bias on purpose: when the kind is `unknown` we |
| 61 | + * skip silently. Better to miss a bug than to nag every cached-for |
| 62 | + * loop in the codebase. The 4 fleet incidents that motivated the |
| 63 | + * rule all had a clear `new Set(...)` / `: Set<T>` annotation in |
| 64 | + * scope; the high-signal cases are the ones we catch. |
| 65 | + * |
| 66 | + * No autofix: the right rewrite depends on intent. If the loop |
| 67 | + * needs index access, the human must materialize via |
| 68 | + * `Array.from(X)`. If it only needs item access, `for (const item |
| 69 | + * of X)` is correct. Auto-choosing the wrong one would silently |
| 70 | + * change semantics (e.g. `Array.from(map)` returns entry pairs, |
| 71 | + * not values). Report-only; pair with a clear remediation hint. |
| 72 | + */ |
| 73 | + |
| 74 | +import type { AstNode, RuleContext } from '../lib/rule-types.mts' |
| 75 | + |
| 76 | +// Type-annotation strings that mark a binding as a known collection |
| 77 | +// kind. Matched against the source-code slice of the annotation — |
| 78 | +// keeps the rule simple at the cost of false positives on shadowed |
| 79 | +// generic names (acceptable: the fleet doesn't shadow these). |
| 80 | +const SET_TYPE_NAMES = new Set([ |
| 81 | + 'Set', |
| 82 | + 'ReadonlySet', |
| 83 | + 'WeakSet', |
| 84 | +]) |
| 85 | + |
| 86 | +const MAP_TYPE_NAMES = new Set([ |
| 87 | + 'Map', |
| 88 | + 'ReadonlyMap', |
| 89 | + 'WeakMap', |
| 90 | +]) |
| 91 | + |
| 92 | +const ITERABLE_TYPE_NAMES = new Set([ |
| 93 | + 'Iterable', |
| 94 | + 'AsyncIterable', |
| 95 | + 'IterableIterator', |
| 96 | +]) |
| 97 | + |
| 98 | +const ARRAY_TYPE_NAMES = new Set([ |
| 99 | + 'Array', |
| 100 | + 'ReadonlyArray', |
| 101 | +]) |
| 102 | + |
| 103 | +type Kind = 'set' | 'map' | 'iterable' | 'array' | 'unknown' |
| 104 | + |
| 105 | +// The non-array kinds — these are the ones we flag. |
| 106 | +const FLAGGED_KINDS: ReadonlySet<Kind> = new Set(['set', 'map', 'iterable']) |
| 107 | + |
| 108 | +/** |
| 109 | + * Classify a TS type-annotation AST node into a Kind. Recognizes the |
| 110 | + * shallow forms (`Set<…>`, `Map<…>`, etc.); generic wrappers like |
| 111 | + * `Promise<Set<…>>` are not unwrapped — they resolve to `unknown`, |
| 112 | + * which is the safe (skip-silently) outcome. |
| 113 | + */ |
| 114 | +function classifyTypeAnnotation(annotation: AstNode | undefined): Kind { |
| 115 | + if (!annotation || !annotation.typeAnnotation) { |
| 116 | + return 'unknown' |
| 117 | + } |
| 118 | + const t = annotation.typeAnnotation |
| 119 | + // `: T[]` → array. |
| 120 | + if (t.type === 'TSArrayType') { |
| 121 | + return 'array' |
| 122 | + } |
| 123 | + // `: Set<T>` / `: Map<K, V>` / `: Iterable<T>` / `: Array<T>` etc. |
| 124 | + if (t.type === 'TSTypeReference') { |
| 125 | + const name = |
| 126 | + t.typeName && t.typeName.type === 'Identifier' |
| 127 | + ? t.typeName.name |
| 128 | + : undefined |
| 129 | + if (!name) { |
| 130 | + return 'unknown' |
| 131 | + } |
| 132 | + if (SET_TYPE_NAMES.has(name)) { |
| 133 | + return 'set' |
| 134 | + } |
| 135 | + if (MAP_TYPE_NAMES.has(name)) { |
| 136 | + return 'map' |
| 137 | + } |
| 138 | + if (ITERABLE_TYPE_NAMES.has(name)) { |
| 139 | + return 'iterable' |
| 140 | + } |
| 141 | + if (ARRAY_TYPE_NAMES.has(name)) { |
| 142 | + return 'array' |
| 143 | + } |
| 144 | + } |
| 145 | + return 'unknown' |
| 146 | +} |
| 147 | + |
| 148 | +/** |
| 149 | + * Classify the initializer expression a `VariableDeclarator` is |
| 150 | + * bound to. |
| 151 | + */ |
| 152 | +function classifyInit(init: AstNode | undefined): Kind { |
| 153 | + if (!init) { |
| 154 | + return 'unknown' |
| 155 | + } |
| 156 | + // `[…]` array literal → array (we'll skip these). |
| 157 | + if (init.type === 'ArrayExpression') { |
| 158 | + return 'array' |
| 159 | + } |
| 160 | + // `new Set(...)`, `new Map(...)`, etc. |
| 161 | + if (init.type === 'NewExpression' && init.callee.type === 'Identifier') { |
| 162 | + const name = init.callee.name as string |
| 163 | + if (SET_TYPE_NAMES.has(name)) { |
| 164 | + return 'set' |
| 165 | + } |
| 166 | + if (MAP_TYPE_NAMES.has(name)) { |
| 167 | + return 'map' |
| 168 | + } |
| 169 | + if (ARRAY_TYPE_NAMES.has(name)) { |
| 170 | + return 'array' |
| 171 | + } |
| 172 | + } |
| 173 | + // `Array.from(...)` / `Array.of(...)` / `Object.keys/values/entries(...)` |
| 174 | + // → array. These are common materialization sites and worth |
| 175 | + // catching as negative signals so the rule doesn't fire on |
| 176 | + // post-fix code. |
| 177 | + if ( |
| 178 | + init.type === 'CallExpression' && |
| 179 | + init.callee.type === 'MemberExpression' && |
| 180 | + init.callee.object.type === 'Identifier' && |
| 181 | + !init.callee.computed && |
| 182 | + init.callee.property.type === 'Identifier' |
| 183 | + ) { |
| 184 | + const objName = init.callee.object.name as string |
| 185 | + const propName = init.callee.property.name as string |
| 186 | + if (objName === 'Array' && (propName === 'from' || propName === 'of')) { |
| 187 | + return 'array' |
| 188 | + } |
| 189 | + if ( |
| 190 | + objName === 'Object' && |
| 191 | + (propName === 'keys' || propName === 'values' || propName === 'entries') |
| 192 | + ) { |
| 193 | + return 'array' |
| 194 | + } |
| 195 | + } |
| 196 | + return 'unknown' |
| 197 | +} |
| 198 | + |
| 199 | +/** |
| 200 | + * The cached-for-loop init shape we're looking for: |
| 201 | + * |
| 202 | + * let i = 0, { length } = X |
| 203 | + * |
| 204 | + * Returns the identifier `X` if the shape matches and `X` is a |
| 205 | + * bare Identifier, otherwise undefined. |
| 206 | + */ |
| 207 | +function matchCachedForInit(init: AstNode | undefined): string | undefined { |
| 208 | + if (!init || init.type !== 'VariableDeclaration') { |
| 209 | + return undefined |
| 210 | + } |
| 211 | + const decls = init.declarations |
| 212 | + if (!decls || decls.length < 2) { |
| 213 | + return undefined |
| 214 | + } |
| 215 | + // The `{ length } = X` declarator. Could be at any position after |
| 216 | + // the counter, but the canonical fleet shape puts it second. |
| 217 | + for (let i = 0, { length: declsLen } = decls; i < declsLen; i += 1) { |
| 218 | + const d = decls[i] |
| 219 | + if ( |
| 220 | + d.id && |
| 221 | + d.id.type === 'ObjectPattern' && |
| 222 | + d.id.properties && |
| 223 | + d.id.properties.length === 1 && |
| 224 | + d.id.properties[0].type === 'Property' && |
| 225 | + d.id.properties[0].key && |
| 226 | + d.id.properties[0].key.type === 'Identifier' && |
| 227 | + d.id.properties[0].key.name === 'length' && |
| 228 | + d.init && |
| 229 | + d.init.type === 'Identifier' |
| 230 | + ) { |
| 231 | + return d.init.name as string |
| 232 | + } |
| 233 | + } |
| 234 | + return undefined |
| 235 | +} |
| 236 | + |
| 237 | +const rule = { |
| 238 | + meta: { |
| 239 | + type: 'problem', |
| 240 | + docs: { |
| 241 | + description: |
| 242 | + "Don't apply the cached-length `for (let i = 0, { length } = X; …)` pattern to Sets, Maps, or generic Iterables — it silently no-ops (X has no `.length` and isn't integer-indexable).", |
| 243 | + category: 'Correctness', |
| 244 | + recommended: true, |
| 245 | + }, |
| 246 | + fixable: undefined, |
| 247 | + messages: { |
| 248 | + noCachedForOnIterable: |
| 249 | + '`{{name}}` is a {{kind}} — cached-length `for` is a silent no-op (no `.length`, not integer-indexable). Use `Array.from({{name}})` for indexed iteration, or `for (const item of {{name}})` for sequential access.', |
| 250 | + }, |
| 251 | + schema: [], |
| 252 | + }, |
| 253 | + |
| 254 | + create(context: RuleContext) { |
| 255 | + // Per-file map: identifier name → known kind. |
| 256 | + const kinds = new Map<string, Kind>() |
| 257 | + |
| 258 | + // Helper: record a kind for an identifier name; only overwrite if |
| 259 | + // the new info is more specific (unknown loses to everything else). |
| 260 | + function record(name: string | undefined, kind: Kind): void { |
| 261 | + if (!name || kind === 'unknown') { |
| 262 | + return |
| 263 | + } |
| 264 | + kinds.set(name, kind) |
| 265 | + } |
| 266 | + |
| 267 | + return { |
| 268 | + // Catch: |
| 269 | + // const s = new Set() |
| 270 | + // const s: Set<string> = … |
| 271 | + // const s: Set<string> = new Set() |
| 272 | + // const arr: string[] = [] |
| 273 | + // const arr = [1, 2, 3] |
| 274 | + VariableDeclarator(node: AstNode) { |
| 275 | + if (!node.id || node.id.type !== 'Identifier') { |
| 276 | + return |
| 277 | + } |
| 278 | + const name = node.id.name as string |
| 279 | + // Type annotation takes priority — explicit > inferred. |
| 280 | + const annotated = classifyTypeAnnotation(node.id.typeAnnotation) |
| 281 | + if (annotated !== 'unknown') { |
| 282 | + record(name, annotated) |
| 283 | + return |
| 284 | + } |
| 285 | + record(name, classifyInit(node.init)) |
| 286 | + }, |
| 287 | + |
| 288 | + // Catch annotated parameters: |
| 289 | + // function f(items: Set<string>) { … } |
| 290 | + // const f = (items: Map<string, number>) => { … } |
| 291 | + FunctionDeclaration(node: AstNode) { |
| 292 | + recordParams(node.params, record) |
| 293 | + }, |
| 294 | + FunctionExpression(node: AstNode) { |
| 295 | + recordParams(node.params, record) |
| 296 | + }, |
| 297 | + ArrowFunctionExpression(node: AstNode) { |
| 298 | + recordParams(node.params, record) |
| 299 | + }, |
| 300 | + |
| 301 | + ForStatement(node: AstNode) { |
| 302 | + const iterName = matchCachedForInit(node.init) |
| 303 | + if (!iterName) { |
| 304 | + return |
| 305 | + } |
| 306 | + const kind = kinds.get(iterName) ?? 'unknown' |
| 307 | + if (!FLAGGED_KINDS.has(kind)) { |
| 308 | + return |
| 309 | + } |
| 310 | + context.report({ |
| 311 | + node: node.init, |
| 312 | + messageId: 'noCachedForOnIterable', |
| 313 | + data: { name: iterName, kind }, |
| 314 | + }) |
| 315 | + }, |
| 316 | + } |
| 317 | + }, |
| 318 | +} |
| 319 | + |
| 320 | +function recordParams( |
| 321 | + params: AstNode[] | undefined, |
| 322 | + record: (name: string | undefined, kind: Kind) => void, |
| 323 | +): void { |
| 324 | + if (!params) { |
| 325 | + return |
| 326 | + } |
| 327 | + for (let i = 0, { length } = params; i < length; i += 1) { |
| 328 | + const p = params[i] |
| 329 | + if (!p || p.type !== 'Identifier') { |
| 330 | + continue |
| 331 | + } |
| 332 | + const name = p.name as string |
| 333 | + const annotated = classifyTypeAnnotation(p.typeAnnotation) |
| 334 | + record(name, annotated) |
| 335 | + } |
| 336 | +} |
| 337 | + |
| 338 | +export default rule |
0 commit comments