Skip to content

feat(compiler): clarify preserve-manual-memoization error for forward…#188

Closed
everettbu wants to merge 2 commits into
mainfrom
fix/preserve-manual-memoization-error-message
Closed

feat(compiler): clarify preserve-manual-memoization error for forward…#188
everettbu wants to merge 2 commits into
mainfrom
fix/preserve-manual-memoization-error-message

Conversation

@everettbu

@everettbu everettbu commented Dec 12, 2025

Copy link
Copy Markdown

Mirror of facebook/react#35086
Original author: Rithvik914


… references

Improve diagnostic when a callback (useCallback/useMemo) depends on a memoized value declared later in the component. Adds explicit guidance to declare memoized values before callbacks that capture them and includes the dependency identifier when available.

Changes:

  • Enhance message in ValidatePreservedManualMemoization.ts (StartMemoize visitor) to add declaration-order hint and actionable fix suggestion.
  • Update existing snapshot: error.false-positive-useMemo-overlap-scopes.expect.md to reflect new wording.
  • Add new failing fixture & snapshot: error.useCallback-references-later-useMemo.* (demonstrates forward reference error)
  • Add new passing fixture & snapshot: useCallback-after-useMemo-declared.* (proves correct order is accepted)

Testing:

  • Snap runner executed; new fixtures behave as expected.
  • Overall compiler tests previously at 1786 total with unrelated failures retained; targeted fixtures pass.

Rationale:
Clearer messaging reduces confusion around declaration order vs dependency inference, speeding developer remediation.

Summary

How did you test this change?

…d CHANGELOG entry under Unreleased for enhanced forward reference guidance in babel-plugin-react-compiler.
@greptile-apps

greptile-apps Bot commented Dec 12, 2025

Copy link
Copy Markdown

Greptile Overview

Greptile Summary

Improves the diagnostic message in ValidatePreservedManualMemoization when a callback depends on a later-declared memoized value. The enhanced error now includes the dependency name and suggests moving declarations to resolve the issue.

Key changes:

  • Enhanced error message extracts dependency name and provides specific guidance about declaration order
  • Updated existing snapshot for error.false-positive-useMemo-overlap-scopes test
  • Added passing test case demonstrating correct declaration order

Issue found:

  • The new failing test fixture error.useCallback-references-later-useMemo.ts has a misleading comment claiming this is a "false positive preserve-manual-memoization error" and "valid pattern in React", but the actual error is from a different validator (InferMutationAliasingEffects) catching legitimate forward references that would cause ReferenceErrors at runtime. The test doesn't actually demonstrate the improved error message from this PR.

Confidence Score: 3/5

  • Safe to merge with clarification needed on test fixture intent
  • The core code change is solid and improves error messaging, but one of the new test fixtures has a misleading comment that mischaracterizes the error being tested, which could confuse future developers
  • Pay attention to error.useCallback-references-later-useMemo.ts - verify the comment accurately describes what is being tested

Important Files Changed

File Analysis

Filename Score Overview
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts 5/5 Improves error message by adding dependency name and declaration order guidance when detecting forward references
compiler/packages/babel-plugin-react-compiler/src/tests/fixtures/compiler/preserve-memo-validation/error.useCallback-references-later-useMemo.ts 3/5 Test fixture with misleading comment - triggers Immutability error, not PreserveManualMemo error as claimed
compiler/packages/babel-plugin-react-compiler/src/tests/fixtures/compiler/preserve-memo-validation/error.useCallback-references-later-useMemo.expect.md 3/5 Snapshot shows Immutability error for forward reference, not the PreserveManualMemo error this PR improves

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

7 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment on lines +4 to +16
/**
* Issue: When a useCallback references a value from a useMemo that is
* declared later in the component, the compiler triggers a false positive
* preserve-manual-memoization error.
*
* The error occurs because the validation checks that dependencies have
* completed their scope before the manual memo block starts. However,
* when the callback is declared before the useMemo, the useMemo's scope
* hasn't completed yet.
*
* This is a valid pattern in React - declaration order doesn't matter
* for the runtime behavior since both are memoized.
*/

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

logic: Comment describes "false positive preserve-manual-memoization error" but the actual error (see .expect.md) is "Cannot access variable before it is declared" from the Immutability category (ErrorCategory.Immutability in InferMutationAliasingEffects.ts:1278), not from PreserveManualMemo validation.

The comment also claims "This is a valid pattern in React - declaration order doesn't matter" but forward references in JavaScript do cause ReferenceErrors at runtime. The compiler error is correct.

Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.useCallback-references-later-useMemo.ts
Line: 4:16

Comment:
**logic:** Comment describes "false positive preserve-manual-memoization error" but the actual error (see `.expect.md`) is "Cannot access variable before it is declared" from the Immutability category (ErrorCategory.Immutability in `InferMutationAliasingEffects.ts:1278`), not from PreserveManualMemo validation.

The comment also claims "This is a valid pattern in React - declaration order doesn't matter" but forward references in JavaScript **do** cause ReferenceErrors at runtime. The compiler error is correct.

How can I resolve this? If you propose a fix, please make it concise.

@everettbu everettbu added the Resolution: Stale Automatically closed due to inactivity label Feb 7, 2026
@everettbu

Copy link
Copy Markdown
Author

Upstream PR was closed or merged. Code is synced via branch mirror.

@everettbu everettbu closed this Feb 7, 2026
@everettbu
everettbu deleted the fix/preserve-manual-memoization-error-message branch February 7, 2026 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed Resolution: Stale Automatically closed due to inactivity

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants