Skip to content

fix(react-doctor): honor empty-frame barriers in no-prop-callback-in-effect / no-derived-useState prop-stack lookups#163

Merged
aidenybai merged 1 commit intomainfrom
cursor/fix-prop-stack-barrier-7144
May 8, 2026
Merged

fix(react-doctor): honor empty-frame barriers in no-prop-callback-in-effect / no-derived-useState prop-stack lookups#163
aidenybai merged 1 commit intomainfrom
cursor/fix-prop-stack-barrier-7144

Conversation

@aidenybai
Copy link
Copy Markdown
Member

Surfaced during a self-review of the in-flight useEffect rule PRs (#153, #154, #155, #156, #157, #162). The bug is on main and predates that work, but the same shape needed fixing on the new branches too — fixing here keeps main honest and avoids forking the convention.

The bug

The visitor scaffolding shared by no-prop-callback-in-effect and no-derived-useState pushes an empty Set when entering a non-component FunctionDeclaration / ArrowFunctionExpression. The intent (per the existing comment) is to act as a barrier: identifiers inside a helper shouldn't resolve against an outer component's props because a closed-over value is not a prop of the helper.

The implementation walked the entire stack anyway:

const isPropName = (name: string): boolean => {
  for (let stackIndex = componentPropParamStack.length - 1; stackIndex >= 0; stackIndex--) {
    if (componentPropParamStack[stackIndex].has(name)) return true;
  }
  return false;
};

Top frame is empty → keep walking → next frame matches → return true. Barrier intent broken.

False positives this produced

Both rules incorrectly fired on this shape:

function Outer({ value, onChange }) {
  function inner() {
    const [draft, setDraft] = useState(value);          // FP: no-derived-useState
    useEffect(() => {
      onChange(isOn);
    }, [isOn, onChange]);                               // FP: no-prop-callback-in-effect
  }
  inner();
}

value and onChange are lexically captured from Outer, not props of inner.

Fix

Single-line change in each rule's isPropName: stop the walk at the first empty frame.

const isPropName = (name: string): boolean => {
  for (let stackIndex = componentPropParamStack.length - 1; stackIndex >= 0; stackIndex--) {
    const frame = componentPropParamStack[stackIndex];
    if (frame.size === 0) return false;   // ← barrier
    if (frame.has(name)) return true;
  }
  return false;
};

The empty frame the visitor pushes when entering a non-component function now acts as the barrier the comment claims it does.

Tests

New file tests/regressions/prop-stack-barrier.test.ts with 4 cases:

  • no-derived-useState real prop → still flags ✅
  • no-derived-useState nested helper closing over outer prop → no longer flags ❌
  • no-prop-callback-in-effect real prop → still flags ✅
  • no-prop-callback-in-effect nested helper closing over outer prop → no longer flags ❌

484/484 tests passing. Lint, typecheck, format clean. No changeset.

Related

The same fix has been applied to the equivalent helpers introduced by the in-flight rule PRs:

Both of those branches additionally switched from walkAst to a direct for (statement of componentBody.body) iteration so they only visit useEffects that are direct top-level statements — that's a separate fix for a related FP path that only affected the new rules.

Open in Web Open in Cursor 

The visitor scaffolding shared by no-prop-callback-in-effect and
no-derived-useState pushes an empty Set when entering a non-component
FunctionDeclaration / ArrowFunctionExpression. The intent (per the
existing comment) is to act as a barrier so identifiers inside the
helper don't resolve against an outer component's props.

The implementation walked the entire stack regardless, so a helper
nested inside a component would inherit the component's prop set and
produce false positives:

  function Outer({ value, onChange }) {       // outer props
    function inner() {
      const [draft, setDraft] = useState(value);
      useEffect(() => {
        onChange(isOn);
      }, [isOn, onChange]);
    }
    inner();
  }

The closed-over `value` and `onChange` are NOT props of `inner`;
they're lexically captured from `Outer`. Both rules incorrectly
flagged this shape.

Fix: stop the lookup walk at the first empty frame so the barrier
the visitor pushes actually behaves as one.

Two regression cases per rule (real prop → still flags; nested helper
→ no longer flags) live in tests/regressions/prop-stack-barrier.test.ts.

Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
@vercel
Copy link
Copy Markdown

vercel Bot commented May 7, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
react-doctor-website Ready Ready Preview, Comment May 7, 2026 9:59pm

@aidenybai aidenybai marked this pull request as ready for review May 8, 2026 01:20
@aidenybai aidenybai changed the title fix: honor empty-frame barriers in no-prop-callback-in-effect / no-derived-useState prop-stack lookups fix(react-doctor): honor empty-frame barriers in no-prop-callback-in-effect / no-derived-useState prop-stack lookups May 8, 2026
@aidenybai aidenybai merged commit c20857e into main May 8, 2026
6 checks passed
cursor Bot pushed a commit that referenced this pull request May 8, 2026
Rebased onto main after PR #153 (no-effect-event-handler /
no-derived-state-effect) and PR #163 (prop-stack-barrier) merged.
Reconstructed the test file by appending the new describe block to
main's version (the textual conflict was purely interleaved test
sections — both rules' tests now coexist cleanly).

Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
cursor Bot pushed a commit that referenced this pull request May 8, 2026
…ndler defer

Rebased onto main after #153 / #163 merged. Test file reconstructed
by appending PR #155's describe block to main's version. The
noEffectEventHandler rule now combines main's widened test predicate
(uses getRootIdentifierName so MemberExpression guards like
`if (product.isInCart)` match) with PR #155's deference logic
(skip when noEventTriggerState would fire — state-typed dep +
recognized side-effect callee). All 552 tests pass; lint, typecheck,
format clean.

Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
cursor Bot pushed a commit that referenced this pull request May 8, 2026
Rebased onto main after #153 / #163 merged. Test file reconstructed
by appending the new describe block to main's version. All 549
tests pass; lint, typecheck, format clean.

Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
cursor Bot pushed a commit that referenced this pull request May 8, 2026
…eps, no-mirror-prop-effect, effect-needs-cleanup)

Rebased onto main after #153 / #163 merged. Test file reconstructed
by appending PR's describe blocks to main's version. All 571 tests
pass.

Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
cursor Bot pushed a commit that referenced this pull request May 8, 2026
Rebased onto main after #153 / #163 merged. Test file reconstructed
by appending PR's describe block to main's version + the
reactMajorVersion helper-arg modification. All 555 tests pass.

Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
aidenybai added a commit that referenced this pull request May 8, 2026
Rebased onto main after #153 / #163 merged. Test file reconstructed
by appending PR's describe block to main's version + the
reactMajorVersion helper-arg modification. All 555 tests pass.

Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
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