From e1042dc34ccc9e7db63c615b605acfbc36a1dfce Mon Sep 17 00:00:00 2001 From: Daniel Chen Date: Mon, 4 May 2026 16:24:47 +0000 Subject: [PATCH 1/3] feat: add prefer-promise-all lint rule Rebased onto main to resolve conflicts after #51 merge. --- README.md | 1 + src/rules/index.ts | 2 + src/rules/prefer-promise-all.ts | 169 +++++++++++++++++++++++++ tests/config-modes.test.ts | 2 +- tests/prefer-promise-all.test.ts | 206 +++++++++++++++++++++++++++++++ 5 files changed, 379 insertions(+), 1 deletion(-) create mode 100644 src/rules/prefer-promise-all.ts create mode 100644 tests/prefer-promise-all.test.ts diff --git a/README.md b/README.md index 2c32f34..0742cb5 100644 --- a/README.md +++ b/README.md @@ -237,6 +237,7 @@ const backendRules = getRulesForPlatform('backend'); | `no-react-native-in-web` | error | web | Don't import react-native in web modules (causes ESM failures) | | `prefer-lucide-icons` | warning | expo, web | Prefer lucide-react/lucide-react-native icons | | `no-module-level-new` | error | web | Don't use `new` at module scope (crashes during SSR) | +| `prefer-promise-all` | warning | universal | Use Promise.all instead of sequential await in for...of loops | --- diff --git a/src/rules/index.ts b/src/rules/index.ts index bd58375..fbc8e16 100644 --- a/src/rules/index.ts +++ b/src/rules/index.ts @@ -52,6 +52,7 @@ import { noServerImportInClient } from './no-server-import-in-client'; import { ssrBrowserApiGuard } from './ssr-browser-api-guard'; import { noReactNativeInWeb } from './no-react-native-in-web'; import { noModuleLevelNew } from './no-module-level-new'; +import { preferPromiseAll } from './prefer-promise-all'; export const rules: Record = { 'no-relative-paths': noRelativePaths, @@ -107,4 +108,5 @@ export const rules: Record = { 'ssr-browser-api-guard': ssrBrowserApiGuard, 'no-react-native-in-web': noReactNativeInWeb, 'no-module-level-new': noModuleLevelNew, + 'prefer-promise-all': preferPromiseAll, }; diff --git a/src/rules/prefer-promise-all.ts b/src/rules/prefer-promise-all.ts new file mode 100644 index 0000000..8a28fa2 --- /dev/null +++ b/src/rules/prefer-promise-all.ts @@ -0,0 +1,169 @@ +import traverse from '@babel/traverse'; +import * as t from '@babel/types'; +import type { File } from '@babel/types'; +import type { LintResult } from '../types'; + +const RULE_NAME = 'prefer-promise-all'; + +export function preferPromiseAll(ast: File, _code: string): LintResult[] { + const results: LintResult[] = []; + + traverse(ast, { + ForOfStatement(loopPath) { + // Skip `for await...of` — already an async iterator pattern + if (loopPath.node.await) return; + + let hasAwait = false; + let hasOrderDependentPattern = false; + + loopPath.traverse({ + // Don't descend into nested functions — their awaits are independent + 'FunctionDeclaration|FunctionExpression|ArrowFunctionExpression'(innerPath: any) { + innerPath.skip(); + }, + + // Don't descend into nested loops + 'ForStatement|ForOfStatement|ForInStatement|WhileStatement|DoWhileStatement'( + innerPath: any, + ) { + innerPath.skip(); + }, + + AwaitExpression(_awaitPath: any) { + hasAwait = true; + }, + + CallExpression(callPath: any) { + const { callee } = callPath.node; + + // Detect .push() calls — order-dependent array accumulation + if ( + t.isMemberExpression(callee) && + t.isIdentifier(callee.property) && + callee.property.name === 'push' + ) { + hasOrderDependentPattern = true; + callPath.stop(); + } + + // Detect .splice() / .unshift() — order-dependent + if ( + t.isMemberExpression(callee) && + t.isIdentifier(callee.property) && + (callee.property.name === 'splice' || callee.property.name === 'unshift') + ) { + hasOrderDependentPattern = true; + callPath.stop(); + } + }, + + // Detect break/return conditioned on await result — sequential early-exit + BreakStatement(breakPath: any) { + // Check this break targets the outer for...of (not a nested switch/loop) + let parent = breakPath.parentPath; + while (parent && parent !== loopPath) { + const type = parent.node.type; + if ( + type === 'ForStatement' || + type === 'WhileStatement' || + type === 'DoWhileStatement' || + type === 'ForOfStatement' || + type === 'ForInStatement' || + type === 'SwitchStatement' + ) { + return; // break belongs to nested construct + } + parent = parent.parentPath; + } + hasOrderDependentPattern = true; + }, + + ReturnStatement(returnPath: any) { + // Check this return is in the function containing the loop, not a nested fn + let parent = returnPath.parentPath; + while (parent && parent !== loopPath) { + if ( + parent.node.type === 'FunctionDeclaration' || + parent.node.type === 'FunctionExpression' || + parent.node.type === 'ArrowFunctionExpression' + ) { + return; // return belongs to nested function + } + parent = parent.parentPath; + } + hasOrderDependentPattern = true; + }, + }); + + if (hasOrderDependentPattern) return; + + // Check for cross-iteration data dependencies: + // A variable declared BEFORE the loop that is both read and written inside the loop body + if (hasCrossIterationDependency(loopPath)) return; + + if (hasAwait) { + const { loc } = loopPath.node; + results.push({ + rule: RULE_NAME, + message: + 'Sequential await in for...of loop can likely be parallelized with Promise.all(items.map(async (item) => ...)) for better performance', + line: loc?.start.line ?? 0, + column: loc?.start.column ?? 0, + severity: 'warning', + }); + } + }, + }); + + return results; +} + +function hasCrossIterationDependency(loopPath: any): boolean { + // Collect identifiers that are ASSIGNED inside the loop body + const assignedInLoop = new Set(); + + loopPath.traverse({ + 'FunctionDeclaration|FunctionExpression|ArrowFunctionExpression'(innerPath: any) { + innerPath.skip(); + }, + + AssignmentExpression(assignPath: any) { + const left = assignPath.node.left; + if (t.isIdentifier(left)) { + assignedInLoop.add(left.name); + } + }, + + UpdateExpression(updatePath: any) { + const arg = updatePath.node.argument; + if (t.isIdentifier(arg)) { + assignedInLoop.add(arg.name); + } + }, + }); + + if (assignedInLoop.size === 0) return false; + + // Check if any of those assigned variables are bound OUTSIDE the loop + for (const name of assignedInLoop) { + const binding = loopPath.scope.getBinding(name); + if (!binding) continue; // global or unresolved — assume dependency + + // If the variable is declared outside the loop, it's a cross-iteration dependency + const declPath = binding.path; + if (!isInsidePath(declPath, loopPath)) { + return true; + } + } + + return false; +} + +function isInsidePath(inner: any, outer: any): boolean { + let current = inner; + while (current) { + if (current === outer) return true; + current = current.parentPath; + } + return false; +} diff --git a/tests/config-modes.test.ts b/tests/config-modes.test.ts index c18404f..d8bbfed 100644 --- a/tests/config-modes.test.ts +++ b/tests/config-modes.test.ts @@ -123,7 +123,7 @@ describe('config modes', () => { expect(ruleNames).toContain('no-relative-paths'); expect(ruleNames).toContain('expo-image-import'); expect(ruleNames).toContain('no-stylesheet-create'); - expect(ruleNames.length).toBe(53); + expect(ruleNames.length).toBe(54); }); }); }); diff --git a/tests/prefer-promise-all.test.ts b/tests/prefer-promise-all.test.ts new file mode 100644 index 0000000..2647dbc --- /dev/null +++ b/tests/prefer-promise-all.test.ts @@ -0,0 +1,206 @@ +import { describe, it, expect } from 'vitest'; +import { lintJsxCode } from '../src'; + +const config = { rules: ['prefer-promise-all'] }; + +describe('prefer-promise-all rule', () => { + it('should flag sequential await in for...of with bare await', () => { + const code = ` + async function processAll(items) { + for (const item of items) { + await processItem(item); + } + } + `; + const results = lintJsxCode(code, config); + expect(results).toHaveLength(1); + expect(results[0].rule).toBe('prefer-promise-all'); + expect(results[0].message).toContain('Promise.all'); + expect(results[0].severity).toBe('warning'); + }); + + it('should flag when result is added to a Set', () => { + const code = ` + async function fetchAll(urls) { + const results = new Set(); + for (const url of urls) { + const data = await fetch(url); + results.add(data); + } + return results; + } + `; + const results = lintJsxCode(code, config); + expect(results).toHaveLength(1); + }); + + it('should flag when result is stored in a Map', () => { + const code = ` + async function fetchAll(items) { + const cache = new Map(); + for (const item of items) { + const result = await fetchItem(item.id); + cache.set(item.id, result); + } + return cache; + } + `; + const results = lintJsxCode(code, config); + expect(results).toHaveLength(1); + }); + + it('should flag when await result is assigned to a local variable only', () => { + const code = ` + async function processAll(items) { + for (const item of items) { + const result = await doWork(item); + console.log(result); + } + } + `; + const results = lintJsxCode(code, config); + expect(results).toHaveLength(1); + }); + + it('should NOT flag when result is pushed to an array', () => { + const code = ` + async function fetchAll(urls) { + const results = []; + for (const url of urls) { + const data = await fetch(url); + results.push(data); + } + return results; + } + `; + const results = lintJsxCode(code, config); + expect(results).toHaveLength(0); + }); + + it('should NOT flag when loop has break', () => { + const code = ` + async function findFirst(items) { + for (const item of items) { + const result = await check(item); + if (result.found) break; + } + } + `; + const results = lintJsxCode(code, config); + expect(results).toHaveLength(0); + }); + + it('should NOT flag when loop has return', () => { + const code = ` + async function findFirst(items) { + for (const item of items) { + const result = await check(item); + if (result.found) return result; + } + } + `; + const results = lintJsxCode(code, config); + expect(results).toHaveLength(0); + }); + + it('should NOT flag cross-iteration dependency', () => { + const code = ` + async function chain(items) { + let prev = null; + for (const item of items) { + prev = await process(prev, item); + } + return prev; + } + `; + const results = lintJsxCode(code, config); + expect(results).toHaveLength(0); + }); + + it('should NOT flag cross-iteration accumulator', () => { + const code = ` + async function sum(items) { + let total = 0; + for (const item of items) { + const value = await getValue(item); + total += value; + } + return total; + } + `; + const results = lintJsxCode(code, config); + expect(results).toHaveLength(0); + }); + + it('should NOT flag for await...of', () => { + const code = ` + async function consume(stream) { + for await (const chunk of stream) { + await processChunk(chunk); + } + } + `; + const results = lintJsxCode(code, config); + expect(results).toHaveLength(0); + }); + + it('should NOT flag loops without await', () => { + const code = ` + function processAll(items) { + for (const item of items) { + processItem(item); + } + } + `; + const results = lintJsxCode(code, config); + expect(results).toHaveLength(0); + }); + + it('should NOT flag when await is inside a nested function (e.g. .map callback)', () => { + const code = ` + function processAll(items) { + for (const group of groups) { + const results = group.items.map(async (item) => { + return await fetchItem(item); + }); + } + } + `; + const results = lintJsxCode(code, config); + expect(results).toHaveLength(0); + }); + + it('should NOT flag break inside a switch statement (not targeting outer loop)', () => { + const code = ` + async function processAll(items) { + for (const item of items) { + const result = await fetchItem(item); + switch (result.type) { + case 'a': + handleA(result); + break; + default: + handleDefault(result); + break; + } + } + } + `; + const results = lintJsxCode(code, config); + expect(results).toHaveLength(1); + }); + + it('should NOT flag unshift to array', () => { + const code = ` + async function fetchAll(urls) { + const results = []; + for (const url of urls) { + const data = await fetch(url); + results.unshift(data); + } + } + `; + const results = lintJsxCode(code, config); + expect(results).toHaveLength(0); + }); +}); From 62077ec86d2539932022201f00807e6bc16b3362 Mon Sep 17 00:00:00 2001 From: Daniel Chen Date: Mon, 4 May 2026 18:03:58 +0000 Subject: [PATCH 2/3] chore: prettier --write README.md --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index b86be06..93891fa 100644 --- a/README.md +++ b/README.md @@ -241,7 +241,7 @@ const backendRules = getRulesForPlatform('backend'); | `sql-no-nested-calls` | error | backend | Don't nest sql template tags | | `no-sync-fs` | error | backend | Use fs.promises or fs/promises instead of sync fs methods | | `no-unrestricted-loop-in-serverless` | error | backend | Unbounded loops (while(true), for(;;)) cause serverless timeouts | -| `prefer-promise-all` | warning | universal | Use Promise.all instead of sequential await in for...of loops | +| `prefer-promise-all` | warning | universal | Use Promise.all instead of sequential await in for...of loops | --- From a974487eccde78251a82f34cfe444407764da859 Mon Sep 17 00:00:00 2001 From: Daniel Chen Date: Mon, 4 May 2026 18:07:58 +0000 Subject: [PATCH 3/3] fix(prefer-promise-all): handle MemberExpression and destructuring LHS in cross-iter dep check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses dx5v's review on PR #57: - AssignmentExpression LHS now collects names from MemberExpression (root identifier — e.g. `state.prev = ...` → `state`) and ObjectPattern / ArrayPattern (each bound name, including aliased `{ next: prev }`, RestElement, AssignmentPattern defaults). - UpdateExpression on a MemberExpression also resolves to the root. - Replaced ad-hoc `: any` annotations on visitor params and helpers with `NodePath` from @babel/traverse and the corresponding node types from @babel/types, matching the pattern in prefer-named-params. - Added 6 tests covering the false-positive cases from the review: member-expression LHS (incl. nested), object-pattern LHS (incl. aliased `{ next: prev }`), array-pattern LHS, plus a positive case where the root is loop-local and the rule should still fire. --- src/rules/prefer-promise-all.ts | 117 +++++++++++++++++++++++++------ tests/prefer-promise-all.test.ts | 90 ++++++++++++++++++++++++ 2 files changed, 186 insertions(+), 21 deletions(-) diff --git a/src/rules/prefer-promise-all.ts b/src/rules/prefer-promise-all.ts index 8a28fa2..4a9f6cf 100644 --- a/src/rules/prefer-promise-all.ts +++ b/src/rules/prefer-promise-all.ts @@ -1,6 +1,16 @@ import traverse from '@babel/traverse'; +import type { NodePath } from '@babel/traverse'; import * as t from '@babel/types'; -import type { File } from '@babel/types'; +import type { + AssignmentExpression, + AwaitExpression, + BreakStatement, + CallExpression, + File, + ForOfStatement, + ReturnStatement, + UpdateExpression, +} from '@babel/types'; import type { LintResult } from '../types'; const RULE_NAME = 'prefer-promise-all'; @@ -9,7 +19,7 @@ export function preferPromiseAll(ast: File, _code: string): LintResult[] { const results: LintResult[] = []; traverse(ast, { - ForOfStatement(loopPath) { + ForOfStatement(loopPath: NodePath) { // Skip `for await...of` — already an async iterator pattern if (loopPath.node.await) return; @@ -18,22 +28,22 @@ export function preferPromiseAll(ast: File, _code: string): LintResult[] { loopPath.traverse({ // Don't descend into nested functions — their awaits are independent - 'FunctionDeclaration|FunctionExpression|ArrowFunctionExpression'(innerPath: any) { + 'FunctionDeclaration|FunctionExpression|ArrowFunctionExpression'(innerPath: NodePath) { innerPath.skip(); }, // Don't descend into nested loops 'ForStatement|ForOfStatement|ForInStatement|WhileStatement|DoWhileStatement'( - innerPath: any, + innerPath: NodePath, ) { innerPath.skip(); }, - AwaitExpression(_awaitPath: any) { + AwaitExpression(_awaitPath: NodePath) { hasAwait = true; }, - CallExpression(callPath: any) { + CallExpression(callPath: NodePath) { const { callee } = callPath.node; // Detect .push() calls — order-dependent array accumulation @@ -58,9 +68,9 @@ export function preferPromiseAll(ast: File, _code: string): LintResult[] { }, // Detect break/return conditioned on await result — sequential early-exit - BreakStatement(breakPath: any) { + BreakStatement(breakPath: NodePath) { // Check this break targets the outer for...of (not a nested switch/loop) - let parent = breakPath.parentPath; + let parent: NodePath | null = breakPath.parentPath; while (parent && parent !== loopPath) { const type = parent.node.type; if ( @@ -78,9 +88,9 @@ export function preferPromiseAll(ast: File, _code: string): LintResult[] { hasOrderDependentPattern = true; }, - ReturnStatement(returnPath: any) { + ReturnStatement(returnPath: NodePath) { // Check this return is in the function containing the loop, not a nested fn - let parent = returnPath.parentPath; + let parent: NodePath | null = returnPath.parentPath; while (parent && parent !== loopPath) { if ( parent.node.type === 'FunctionDeclaration' || @@ -118,26 +128,26 @@ export function preferPromiseAll(ast: File, _code: string): LintResult[] { return results; } -function hasCrossIterationDependency(loopPath: any): boolean { +function hasCrossIterationDependency(loopPath: NodePath): boolean { // Collect identifiers that are ASSIGNED inside the loop body const assignedInLoop = new Set(); loopPath.traverse({ - 'FunctionDeclaration|FunctionExpression|ArrowFunctionExpression'(innerPath: any) { + 'FunctionDeclaration|FunctionExpression|ArrowFunctionExpression'(innerPath: NodePath) { innerPath.skip(); }, - AssignmentExpression(assignPath: any) { - const left = assignPath.node.left; - if (t.isIdentifier(left)) { - assignedInLoop.add(left.name); - } + AssignmentExpression(assignPath: NodePath) { + collectAssignedNames(assignPath.node.left, assignedInLoop); }, - UpdateExpression(updatePath: any) { + UpdateExpression(updatePath: NodePath) { const arg = updatePath.node.argument; if (t.isIdentifier(arg)) { assignedInLoop.add(arg.name); + } else if (t.isMemberExpression(arg)) { + const root = getMemberRootName(arg); + if (root) assignedInLoop.add(root); } }, }); @@ -147,7 +157,7 @@ function hasCrossIterationDependency(loopPath: any): boolean { // Check if any of those assigned variables are bound OUTSIDE the loop for (const name of assignedInLoop) { const binding = loopPath.scope.getBinding(name); - if (!binding) continue; // global or unresolved — assume dependency + if (!binding) continue; // global or unresolved — assume no dependency // If the variable is declared outside the loop, it's a cross-iteration dependency const declPath = binding.path; @@ -159,8 +169,73 @@ function hasCrossIterationDependency(loopPath: any): boolean { return false; } -function isInsidePath(inner: any, outer: any): boolean { - let current = inner; +// Walk a left-hand side and collect every identifier name it ultimately writes +// to. Handles Identifier, MemberExpression (root object), ObjectPattern, +// ArrayPattern, RestElement, AssignmentPattern (default-value destructure), +// and TS-only wrappers like `as` / `!` casts. +function collectAssignedNames(node: t.Node, out: Set): void { + if (t.isIdentifier(node)) { + out.add(node.name); + return; + } + + if (t.isMemberExpression(node) || t.isOptionalMemberExpression(node)) { + // For `state.prev = ...` or `obj.a.b = ...`, treat the root identifier + // (e.g. `state`, `obj`) as the dependency surface. If that root is bound + // outside the loop, mutations to it carry across iterations. + const root = getMemberRootName(node); + if (root) out.add(root); + return; + } + + if (t.isObjectPattern(node)) { + for (const prop of node.properties) { + if (t.isObjectProperty(prop)) { + // value is the binding target (e.g. `{ a: x }` → `x`; `{ a }` → `a`) + collectAssignedNames(prop.value, out); + } else if (t.isRestElement(prop)) { + collectAssignedNames(prop.argument, out); + } + } + return; + } + + if (t.isArrayPattern(node)) { + for (const el of node.elements) { + if (el === null) continue; + collectAssignedNames(el, out); + } + return; + } + + if (t.isRestElement(node)) { + collectAssignedNames(node.argument, out); + return; + } + + if (t.isAssignmentPattern(node)) { + collectAssignedNames(node.left, out); + return; + } + + if (t.isTSNonNullExpression(node) || t.isTSAsExpression(node)) { + collectAssignedNames(node.expression, out); + return; + } +} + +function getMemberRootName(node: t.MemberExpression | t.OptionalMemberExpression): string | null { + let current: t.Expression | t.Super = node.object; + while (t.isMemberExpression(current) || t.isOptionalMemberExpression(current)) { + current = current.object; + } + if (t.isIdentifier(current)) return current.name; + if (t.isThisExpression(current)) return 'this'; + return null; +} + +function isInsidePath(inner: NodePath, outer: NodePath): boolean { + let current: NodePath | null = inner; while (current) { if (current === outer) return true; current = current.parentPath; diff --git a/tests/prefer-promise-all.test.ts b/tests/prefer-promise-all.test.ts index 2647dbc..ada5462 100644 --- a/tests/prefer-promise-all.test.ts +++ b/tests/prefer-promise-all.test.ts @@ -203,4 +203,94 @@ describe('prefer-promise-all rule', () => { const results = lintJsxCode(code, config); expect(results).toHaveLength(0); }); + + // Cross-iteration dependency via member-expression assignment. + // Pre-fix this would false-positive because hasCrossIterationDependency only + // looked at Identifier lefts. dx5v review on PR #57. + it('should NOT flag cross-iteration dependency through member expression LHS', () => { + const code = ` + async function chain(items) { + const state = { prev: null }; + for (const item of items) { + state.prev = await process(state.prev, item); + } + return state.prev; + } + `; + const results = lintJsxCode(code, config); + expect(results).toHaveLength(0); + }); + + it('should NOT flag cross-iteration dependency through nested member expression LHS', () => { + const code = ` + async function chain(items) { + const state = { meta: { prev: null } }; + for (const item of items) { + state.meta.prev = await process(state.meta.prev, item); + } + } + `; + const results = lintJsxCode(code, config); + expect(results).toHaveLength(0); + }); + + // Cross-iteration dependency via object-pattern assignment (re-bind). + // dx5v review on PR #57. + it('should NOT flag cross-iteration dependency through object-pattern LHS', () => { + const code = ` + async function chain(items) { + let next = null; + for (const item of items) { + ({ next } = await process(next, item)); + } + return next; + } + `; + const results = lintJsxCode(code, config); + expect(results).toHaveLength(0); + }); + + // Cross-iteration dependency via aliased object-pattern (e.g. `{ a: x }`). + it('should NOT flag cross-iteration dependency through aliased object-pattern LHS', () => { + const code = ` + async function chain(items) { + let prev = null; + for (const item of items) { + ({ next: prev } = await process(prev, item)); + } + } + `; + const results = lintJsxCode(code, config); + expect(results).toHaveLength(0); + }); + + // Cross-iteration dependency via array-pattern assignment. + it('should NOT flag cross-iteration dependency through array-pattern LHS', () => { + const code = ` + async function chain(items) { + let prev = null; + let count = 0; + for (const item of items) { + [prev, count] = await process(prev, item); + } + } + `; + const results = lintJsxCode(code, config); + expect(results).toHaveLength(0); + }); + + // Member writes targeting a fresh object declared inside the loop should + // still flag — the root binding is loop-local, not cross-iteration. + it('should still flag member-expression assignment to a loop-local object', () => { + const code = ` + async function processAll(items) { + for (const item of items) { + const local = {}; + local.value = await fetchItem(item); + } + } + `; + const results = lintJsxCode(code, config); + expect(results).toHaveLength(1); + }); });