[compiler] Allow ref access in callbacks passed to event handler props

[kolvian]

Summary

Fixes #35040. The React compiler incorrectly flags ref access within event handlers as ref access at render time. For example, this code would fail to compile with error "Cannot access refs during render":

  const onSubmit = async (data) => {
    const file = ref.current?.toFile(); // Incorrectly flagged as error
  };

  <form onSubmit={handleSubmit(onSubmit)}>

This is a false positive because any built-in DOM event handler is guaranteed not to run at render time. This PR only supports built-in event handlers because there are no guarantees that user-made event handlers will not run at render time.

How did you test this change?

I created 4 test fixtures which validate this change:

• allow-ref-access-in-event-handler-wrapper.tsx - Sync handler test input
• allow-ref-access-in-event-handler-wrapper.expect.md - Sync handler expected output
• allow-ref-access-in-async-event-handler-wrapper.tsx - Async handler test input
• allow-ref-access-in-async-event-handler-wrapper.expect.md - Async handler expected output

All linters and test suites also pass.

[react-sizebot]

The size diff is too large to display in a single comment. The GitHub action for this pull request contains an artifact called 'sizebot-message.md' with the full message.

Generated by 🚫 dangerJS against afef137

[josephsavona]

Thanks for the PR! The approach here works in some cases, but it's encoding some important information — which values can be inferred as event handlers — in an-hoc way. The way we would typically handle this is by adding types to represent a concept, inferring the types appropriately in InferTypes, and then using the type information in later passes.

So the way to go here would be to add a BuiltinEventHandlerId shapeId (see ObjectShape.ts with all the BuiltIn...Id constants), then update InferTypes to infer the types of JsxExpression props as {kind: 'Function', shapeId: BuiltinEventHandlerId, ...} using similar logic to what you have here. Ie, for primitive tags where the prop is a named prop starting with "on".

Then, ValidateNoRefAccessInRender can allow ref-accessing functions to flow into expressions if the result is known to be an event handler. Then you can add an extra case into the logic at https://github.com/facebook/react/blob/main/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccessInRender.ts#L521C21-L553, along the lines of

  const isRefLValue = isUseRefType(instr.lvalue.identifier);
+ const isEventHandlerLValue = isEventHandlerType(instr.lvalue.identifier);
...
  if (
    isRevLValue ||
+   isEventHandlerLValue ||
...

Finally, all of this should only be enabled behind a feature flag, which you can add in Environment.ts: enableInferEventHandlers, false by default. Put the logic in InferTypes behind this feature flag (the ValidateNoRefAccess changes will only take effect if the type is inferred, which it won't be w/o the feature).

Make sure to add // @ enableInferEventHandlers as the first line of each of your new test fixtures to enable the feature.

[kolvian]

Thanks for the detailed review! I made some changes, was just wondering if the current string-based check is robust enough for the React compiler or if I should find some other checking method. Some alternatives I explored were type annotation-based checks (but these aren't available by the time the InferTypes runs) and hardcoded lists of supported event handlers, but these aren't extensible and would need to be maintained. I also added a check that makes sure that the component isn't a web component (would have a hyphen), which is the only edge case I can think of. If there are any others I can definitely include them.

[josephsavona]

just noticed this - all the @ pragmas need to be on the first line (also the same fix in other files). I think the reason you're getting errors is that validateRefAccessDuringRender is on by default

[josephsavona]

We reviewed as a team and are aligned on the direction here, i actually came here to merge and noticed one small thing to fix first, see above comment

[josephsavona]

accepting but please address the comment so that we can merge

[kolvian]

accepting but please address the comment so that we can merge

should be fixed now @josephsavona

[josephsavona]

Thanks again!