Skip to content

[compiler] Fix module-level 'use memo' not compiling in annotation mode#587

Open
everettbu wants to merge 3 commits into
mainfrom
fix-35868
Open

[compiler] Fix module-level 'use memo' not compiling in annotation mode#587
everettbu wants to merge 3 commits into
mainfrom
fix-35868

Conversation

@everettbu

Copy link
Copy Markdown

Mirror of facebook/react#35870
Original author: sleitor


Summary

When compilationMode is set to 'annotation', module-level opt-in directives such as 'use memo' placed at the top of a file were silently ignored. Only function-level directives (inside the function body) triggered compilation, contrary to the documented behavior:

"Place directives at the top of a file to affect all functions in that module."

Root cause

Both getReactFunctionType() and processFn() only inspected the function body's own directives (fn.node.body.directives) when deciding whether to compile in annotation mode. The program-level directives (program.node.directives) were never consulted.

Fix

  • Add hasModuleScopeOptIn to ProgramContext (computed once from tryFindDirectiveEnablingMemoization(program.node.directives)).
  • In getReactFunctionType(): when compilationMode is 'annotation' and no function-level opt-in is found, fall back to hasModuleScopeOptIn before returning null. This ensures functions in the module are queued for compilation.
  • In processFn(): add !programContext.hasModuleScopeOptIn to the annotation-mode skip guard so that the compiled function is actually emitted.
  • Add a test fixture use-memo-module-level that verifies 'use memo' at module scope in annotation mode compiles the component.

Behaviour table

Directive compilationMode Before After
function-level 'use memo' annotation ✅ compiled ✅ compiled
module-level 'use memo' annotation ❌ not compiled ✅ compiled
no directive annotation ✅ skipped ✅ skipped
module-level 'use memo' infer ✅ compiled (infer compiles all) ✅ compiled

Fixes #35868

When compilationMode is set to 'annotation', module-level opt-in directives
such as 'use memo' (placed at the top of a file) were silently ignored. Only
function-level directives triggered compilation, contrary to the documented
behavior that "place directives at the top of a file to affect all functions
in that module".

Root cause: both getReactFunctionType() and processFn() only inspected the
function body's own directives (fn.node.body.directives) when deciding
whether to compile in annotation mode. The program-level directives
(program.node.directives) were never consulted.

Fix:
- Add hasModuleScopeOptIn to ProgramContext, computed once from
  tryFindDirectiveEnablingMemoization(program.node.directives).
- In getReactFunctionType(), when compilationMode is 'annotation' and no
  function-level opt-in is found, fall back to hasModuleScopeOptIn before
  returning null.  This ensures functions in the module are queued for
  compilation.
- In processFn(), add !programContext.hasModuleScopeOptIn to the annotation
  mode skip guard so that the compiled function is emitted.

Fixes react/react#35868
@greptile-apps

greptile-apps Bot commented Feb 23, 2026

Copy link
Copy Markdown

Greptile Summary

Fixes module-level 'use memo' directives being silently ignored in annotation compilation mode. The root cause was that both getReactFunctionType() and processFn() only checked function-body directives, never consulting program-level directives. The fix adds a hasModuleScopeOptIn flag to ProgramContext, computed once from program directives, and threads it through both the function-queuing logic (getReactFunctionType) and the compiled-output emission logic (processFn).

  • The fix is minimal and well-targeted — only the two code paths that need to consult module-level directives are updated
  • Precedence is correct: module-level opt-out (hasModuleScopeOptOut) is checked before the opt-in, so 'use no memo' still takes priority
  • A test fixture validates the core scenario (module-level 'use memo' + compilationMode:"annotation")

Confidence Score: 4/5

  • This PR is safe to merge — it's a focused bug fix with correct logic and a supporting test.
  • The changes are minimal, logically sound, and consistent across both entry and exit points. The fix correctly handles the four directive combinations described in the PR. One minor concern (silent error handling for malformed module-level directives) was already raised in a previous review thread. The test fixture covers the primary scenario but doesn't cover edge cases like multiple functions, non-component functions, or interaction with function-level opt-out.
  • compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts — contains all the logic changes

Important Files Changed

Filename Overview
compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Imports.ts Adds hasModuleScopeOptIn boolean to ProgramContext class and its constructor options type. Clean, straightforward plumbing change.
compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts Core fix: computes hasModuleScopeOptIn from program directives, threads it through getReactFunctionType to queue functions for compilation, and updates the processFn skip guard to emit compiled output when module-level opt-in is present. Logic is correct and consistent across both entry and exit points.
compiler/packages/babel-plugin-react-compiler/src/tests/fixtures/compiler/use-memo-module-level.expect.md Expected test output showing that module-level 'use memo' in annotation mode correctly triggers compilation with memoization cache slots.
compiler/packages/babel-plugin-react-compiler/src/tests/fixtures/compiler/use-memo-module-level.js New test fixture validating module-level 'use memo' directive with compilationMode:"annotation".

Last reviewed commit: f7d2aa7

@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.

4 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment on lines +417 to +421
hasModuleScopeOptIn:
tryFindDirectiveEnablingMemoization(
program.node.directives,
pass.opts,
).unwrapOr(null) != null,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Silent error handling for module-level directives

When tryFindDirectiveEnablingMemoization returns an Err (e.g., a malformed dynamic gating directive at module scope), .unwrapOr(null) silently treats it as "no opt-in." This differs from the function-level handling in processFn (line 607-614), where errors are surfaced via handleError. A malformed module-level directive would be silently ignored rather than reported to the developer.

Consider handling the error case explicitly, similar to how function-level errors are handled:

const moduleScopeOptIn = tryFindDirectiveEnablingMemoization(
  program.node.directives,
  pass.opts,
);
if (moduleScopeOptIn.isErr()) {
  handleError(moduleScopeOptIn.unwrapErr(), pass, null);
}

Then use moduleScopeOptIn.unwrapOr(null) != null for hasModuleScopeOptIn.

Prompt To Fix With AI
This is a comment left during a code review.
Path: compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts
Line: 417-421

Comment:
**Silent error handling for module-level directives**

When `tryFindDirectiveEnablingMemoization` returns an `Err` (e.g., a malformed dynamic gating directive at module scope), `.unwrapOr(null)` silently treats it as "no opt-in." This differs from the function-level handling in `processFn` (line 607-614), where errors are surfaced via `handleError`. A malformed module-level directive would be silently ignored rather than reported to the developer.

Consider handling the error case explicitly, similar to how function-level errors are handled:
```
const moduleScopeOptIn = tryFindDirectiveEnablingMemoization(
  program.node.directives,
  pass.opts,
);
if (moduleScopeOptIn.isErr()) {
  handleError(moduleScopeOptIn.unwrapErr(), pass, null);
}
```
Then use `moduleScopeOptIn.unwrapOr(null) != null` for `hasModuleScopeOptIn`.

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

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants