Skip to content

feat: add prefer-promise-all lint rule#57

Merged
danielchen0 merged 4 commits intomainfrom
danielchen0/prefer-promise-all
May 4, 2026
Merged

feat: add prefer-promise-all lint rule#57
danielchen0 merged 4 commits intomainfrom
danielchen0/prefer-promise-all

Conversation

@danielchen0
Copy link
Copy Markdown
Collaborator

Summary

  • Adds prefer-promise-all rule that detects sequential await in for...of loops that could be parallelized with Promise.all(items.map(async ...))
  • Uses scope analysis to detect cross-iteration variable dependencies (variables declared outside the loop that are both read and written inside)
  • Skips order-dependent patterns: .push(), .unshift(), .splice() to arrays
  • Skips loops with break/return (early exit depends on sequential evaluation)
  • Skips for await...of (async iterator pattern)
  • Skips await inside nested functions (not actually sequential in the loop)
  • Platform: universal (no platform tag — useful on web + backend)

Heuristics

The rule flags when all of these are true:

  1. Loop is for...of (not for, while, or for await...of)
  2. Loop body contains a direct await (not inside a nested function)
  3. No .push() / .unshift() / .splice() calls (order-dependent accumulation)
  4. No break or return targeting the loop (sequential early-exit)
  5. No variable declared outside the loop that is reassigned inside it (cross-iteration dependency)

Test plan

Scenario Expected
for (const x of items) { await doThing(x); } Warning: use Promise.all
Result added to Set/Map Warning: use Promise.all
Result .push()ed to array No warning (order matters)
Loop has break on condition No warning (early exit)
Loop has return on condition No warning (early exit)
Cross-iteration variable (let prev = ...; prev = await ...) No warning (dependency)
Accumulator (total += await ...) No warning (dependency)
for await...of No warning (async iterator)
No await in loop No warning
await inside nested .map() callback No warning (not sequential in loop)
break inside switch (not targeting loop) Still warns (break doesn't exit loop)

@daniel-chen-1
Copy link
Copy Markdown

daniel-chen-1 Bot commented Apr 28, 2026

This PR has been open since early March with all CI green (7/7 checks passing) but mergeable_state=blocked — awaiting a required code review. @danielchen0 — could you flag a reviewer on this?

Comment thread src/rules/prefer-promise-all.ts Outdated
Comment thread src/rules/prefer-promise-all.ts Outdated
Copy link
Copy Markdown
Contributor

@lainterr lainterr Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed as steward. Approving — this is a careful, conservative implementation.

Things I particularly like:

  • Skipping nested functions/arrows so awaits inside items.forEach(async ...) (already concurrent) don't trip the rule.
  • Skipping for await...of and break/return paths.
  • The cross-iteration dependency check using loopPath.scope.getBinding(...) is the right approach — catches reductions/accumulators bound outside the loop.
  • Severity is warning, which is correct (perf hint, not correctness bug).

Minor coverage gaps, all non-blocking:

  1. arr[i] = await foo() and obj[key] = await foo() patterns. The AssignmentExpression visitor in hasCrossIterationDependency only checks t.isIdentifier(left), so member-assignment to outer-scope objects is not tracked. In practice, indexed/keyed writes to a pre-allocated container across iterations are usually parallelizable, so the rule will (correctly) still flag them. Not a bug, just noting.

  2. .set() on a Map / .add() on a Set are not in the order-dependent allowlist. They are usually order-independent, so the rule should NOT skip them — current behavior is correct. (Just confirming, not asking for a change.)

  3. try/catch with sequential semantics — if the user is intentionally awaiting in sequence so a thrown error short-circuits the rest, the rule will still warn. That's acceptable as a warning severity.

  4. No platform tag — universal makes sense (perf advice applies on web/backend; expo less so but harmless).

Ready to merge.

@daniel-chen-1
Copy link
Copy Markdown

daniel-chen-1 Bot commented May 1, 2026

@danielchen0 — both reviewers approved this PR (dx5v at 00:03:21Z and lainterr[bot] at 00:09:46Z, both on head c2b778c). dx5v left two non-blocking comments worth a look before merge:

  1. Real false-positive case: hasCrossIterationDependency only inspects Identifier lefts on AssignmentExpression. A MemberExpression like state.prev = await process(state.prev, item) or an ObjectPattern left like ({ next } = await process(next, item)) is a genuine cross-iter dep but is currently flagged anyway. dx5v suggests walking member-expression to the root identifier and binding-pattern names. Lainterr also called this out as a coverage gap (item 1) but rated it non-blocking.

  2. Type hygiene: visitor params are typed : any throughout (this signature L121/162 + visitors at L21/27/32/36/61/81/126/130/137). Per CLAUDE.md ("No unnecessary type assertions — use Babel's type system directly"), they should be NodePath<t.CallExpression>, etc. — see src/rules/prefer-named-params.ts for the established pattern.

Both are mergeable-as-is per the approvals; flagging in case you want to address the false-positive case before merge.

mergeable_state is still "blocked" — likely the unresolved review comments. Up to you whether to mark them resolved or address before merging.

— danielchen0-pr-monitor

@daniel-chen-1
Copy link
Copy Markdown

daniel-chen-1 Bot commented May 3, 2026

Auto-fix: Lint & Format CI was failing on this rebase because README.md needed prettier formatting. Pushed prettier --write README.md as a follow-up commit. CI should go green on the next run.

Rebased onto main to resolve conflicts after #51 merge.
@daniel-chen-1 daniel-chen-1 Bot force-pushed the danielchen0/prefer-promise-all branch from 3a50582 to e1042dc Compare May 4, 2026 16:24
@daniel-chen-1
Copy link
Copy Markdown

daniel-chen-1 Bot commented May 4, 2026

Rebased onto main after #51 merged — the conflicts were mechanical (rule-registration list, README rule table, rule-count assertion, meta.ts platform map), no logic touched. New head is e1042dc, all 7 CI checks green, mergeable=true. Ready to merge when you are.

@danielchen0 danielchen0 enabled auto-merge (squash) May 4, 2026 17:51
@danielchen0
Copy link
Copy Markdown
Collaborator Author

@daniel-chen-1 please address the PR comments.

lainterr Bot and others added 3 commits May 4, 2026 18:00
Auto-resolved trivial conflicts in shared files. Resolved by Lainter (steward).
…S in cross-iter dep check

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<T>` 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.
@danielchen0 danielchen0 merged commit ccbeabf into main May 4, 2026
7 checks passed
@daniel-chen-1
Copy link
Copy Markdown

daniel-chen-1 Bot commented May 4, 2026

@danielchen0 — addressed dx5v's two review comments in commit a974487 (replied + resolved both threads):

  1. Cross-iter dep via member/destructure LHS: hasCrossIterationDependency now walks the LHS via a new collectAssignedNames helper that handles MemberExpression (root identifier — state.prev = ... resolves to state), ObjectPattern (each bound name, including aliased { next: prev } and RestElement), ArrayPattern (each element + rest), and AssignmentPattern (default-value destructure). Both example false-positive cases from the review are now covered by tests.
  2. : any on visitors and helpers: Replaced with NodePath<T> from @babel/traverse plus the matching node type from @babel/types, matching the pattern in prefer-named-params.ts. Helpers narrow with t.isMemberExpression / t.isObjectPattern / etc. instead of ad-hoc casts.

Also pushed an earlier auto-fix commit (62077ec) running prettier --write README.md to clear the table-alignment churn from this morning's main-merge — both fixes are on top of head 8039ea8.

Test count for prefer-promise-all: 14 → 20 (the 6 new cases verify both directions of the fix, including a positive case where the root identifier is loop-local and the rule should still fire). bunx tsc --noEmit and bunx eslint both green on the changed files. Full test suite re-runs once CI lands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants