[compiler] Run compiler pipeline on 'use no forget' #30720

poteto · 2024-08-16T15:57:00Z

Stack from ghstack (oldest at bottom):

This PR updates the babel plugin to continue the compilation pipeline as
normal on components/hooks that have been opted out using a directive.
Instead, we no longer emit the compiled function when the directive is
present.

Previously, we would skip over the entire pipeline. By continuing to
enter the pipeline, we'll be able to detect if there are unused
directives.

The end result is:

(no change) 'use forget' will always opt into compilation
(new) 'use no forget' will opt out of compilation but continue to log
errors without throwing them. This means that a Program containing
multiple functions (some of which are opted out) will continue to
compile correctly

[ghstack-poisoned]

vercel · 2024-08-16T15:57:03Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
react-compiler-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 16, 2024 9:42pm

[ghstack-poisoned]

poteto · 2024-08-16T19:11:28Z

compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts

@@ -102,7 +103,7 @@ export type CompileResult = {
  compiledFn: CodegenFunction;
 };

-function handleError(
+function logError(


Split this up so we can decide to only log in some cases instead of throwing (eg a file contains at least one function that was compiled although there were opt outs + errors in others)

[ghstack-poisoned]

poteto · 2024-08-16T19:31:48Z

compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts

-    // Opt-outs disable compilation regardless of mode
-    const useNoForget = findDirectiveDisablingMemoization(
-      fn.node.body.directives,
-      pass.opts,
-    );
-    if (useNoForget != null) {
-      pass.opts.logger?.logEvent(pass.filename, {
-        kind: 'CompileError',
-        fnLoc: fn.node.body.loc ?? null,
-        detail: {
-          severity: ErrorSeverity.Todo,
-          reason: 'Skipped due to "use no forget" directive.',
-          loc: useNoForget.loc ?? null,
-          suggestions: null,
-        },
-      });
-      return null;
-    }
-    // Otherwise opt-ins enable compilation regardless of mode
-    if (findDirectiveEnablingMemoization(fn.node.body.directives) != null) {
+    if (findDirectiveEnablingMemoization(fn.node.body.directives).length > 0)
      return getComponentOrHookLike(fn, hookPattern) ?? 'Other';
-    }


This logic really shouldn't have been in this function which is responsible for categorization of the function (eg "Component" or "Hook". This has been moved into processFn instead.

poteto · 2024-08-16T19:34:31Z

compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts

-  // Top level "use no forget", skip this file entirely
-  const useNoForget = findDirectiveDisablingMemoization(
-    program.node.directives,
-    pass.opts,
-  );
-  if (useNoForget != null) {
-    pass.opts.logger?.logEvent(pass.filename, {
-      kind: 'CompileError',
-      fnLoc: null,
-      detail: {
-        severity: ErrorSeverity.Todo,
-        reason: 'Skipped due to "use no forget" directive.',
-        loc: useNoForget.loc ?? null,
-        suggestions: null,
-      },
-    });
-    return true;
-  }


Previously this would skip visiting the function at all, so no validations could be checked.

josephsavona · 2024-08-16T20:32:08Z

compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts

+      const optInDirectives = findDirectiveEnablingMemoization(
+        fn.node.body.directives,
+      );
+      const optOutDirectives = findDirectiveDisablingMemoization(
+        fn.node.body.directives,
+      );


we could probably just calculate these once at the top of processFn, then reference the result here and above in the error case?

josephsavona · 2024-08-16T20:34:53Z

...c/__tests__/fixtures/compiler/error.use-no-forget-multiple-with-eslint-suppression.expect.md

+```
+   7 |   'use no forget';
+   8 |   const ref = useRef(null);
+>  9 |   // eslint-disable-next-line react-hooks/rules-of-hooks
+     |   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ InvalidReact: React Compiler has skipped optimizing this component because one or more React ESLint rules were disabled. React Compiler only works when your components follow all the rules of React, disabling them may result in unexpected or incorrect behavior. eslint-disable-next-line react-hooks/rules-of-hooks (9:9)
+  10 |   ref.current = 'bad';
+  11 |   return <button ref={ref} />;
+  12 | }
+```


hmm, shouldn't this not be reported since the component is opted out?

josephsavona · 2024-08-16T20:35:13Z

...mpiler/src/__tests__/fixtures/compiler/error.use-no-forget-with-eslint-suppression.expect.md

+    |   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ InvalidReact: React Compiler has skipped optimizing this component because one or more React ESLint rules were disabled. React Compiler only works when your components follow all the rules of React, disabling them may result in unexpected or incorrect behavior. eslint-disable-next-line react-hooks/rules-of-hooks (6:6)
+  7 |   ref.current = 'bad';
+  8 |   return <button ref={ref} />;
+  9 | }


josephsavona · 2024-08-16T20:36:05Z

compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts

+      if (fn.node.body.type === 'BlockStatement') {
+        const optOutDirectives = findDirectiveDisablingMemoization(
+          fn.node.body.directives,
+        );
+        if (optOutDirectives.length > 0) {
+          logError(err, pass, fn.node.loc ?? null);
+          return null;
+        }
+      }


i think you need similar logic to this above on https://github.com/facebook/react/pull/30720/files#diff-def62b77d903a117d9a493b69a884f79317eb8c8beac025622265c99f1c7189fR403 (ugh, why doesn't GH let you comment on lines that weren't changed by a PR)

Ahh yeah, thanks for catching this!

[ghstack-poisoned]

josephsavona

nice nice

This PR updates the babel plugin to continue the compilation pipeline as normal on components/hooks that have been opted out using a directive. Instead, we no longer emit the compiled function when the directive is present. Previously, we would skip over the entire pipeline. By continuing to enter the pipeline, we'll be able to detect if there are unused directives. The end result is: - (no change) 'use forget' will always opt into compilation - (new) 'use no forget' will opt out of compilation but continue to log errors without throwing them. This means that a Program containing multiple functions (some of which are opted out) will continue to compile correctly ghstack-source-id: 5bd85df2f81350cb2c1998a8761b8ed3fec32a40 Pull Request resolved: #30720

Update

4099769

[ghstack-poisoned]

poteto mentioned this pull request Aug 16, 2024

[compiler] Surface unused opt out directives in eslint #30721

Merged

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Aug 16, 2024

Update

96d979b

[ghstack-poisoned]

vercel bot deployed to Preview August 16, 2024 16:09 View deployment

josephsavona approved these changes Aug 16, 2024

View reviewed changes

Update

49165e5

[ghstack-poisoned]

vercel bot deployed to Preview August 16, 2024 16:24 View deployment

Update

6d61238

[ghstack-poisoned]

vercel bot deployed to Preview August 16, 2024 19:10 View deployment

poteto commented Aug 16, 2024

View reviewed changes

Update

706d791

[ghstack-poisoned]

vercel bot deployed to Preview August 16, 2024 19:30 View deployment

Update

bebf000

[ghstack-poisoned]

poteto commented Aug 16, 2024

View reviewed changes

vercel bot deployed to Preview August 16, 2024 19:36 View deployment

josephsavona reviewed Aug 16, 2024

View reviewed changes

Update

4817909

[ghstack-poisoned]

poteto mentioned this pull request Aug 16, 2024

[playground] Allow (Arrow)FunctionExpressions #30729

Merged

Update

a1a6803

[ghstack-poisoned]

josephsavona approved these changes Aug 16, 2024

View reviewed changes

vercel bot deployed to Preview August 16, 2024 21:42 View deployment

poteto merged commit a1a6803 into gh/poteto/4/base Aug 16, 2024
19 checks passed

poteto deleted the gh/poteto/4/head branch August 16, 2024 22:12

eps1lon mentioned this pull request Aug 22, 2024

Update React from 1eaccd82-20240816 to eb3ad065-20240822 vercel/next.js#69196

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[compiler] Run compiler pipeline on 'use no forget' #30720

[compiler] Run compiler pipeline on 'use no forget' #30720

poteto commented Aug 16, 2024 •

edited

Loading

vercel bot commented Aug 16, 2024 •

edited

Loading

poteto Aug 16, 2024

poteto Aug 16, 2024

poteto Aug 16, 2024

josephsavona Aug 16, 2024

josephsavona Aug 16, 2024

poteto Aug 16, 2024

josephsavona Aug 16, 2024

josephsavona Aug 16, 2024

poteto Aug 16, 2024

josephsavona left a comment

[compiler] Run compiler pipeline on 'use no forget' #30720

[compiler] Run compiler pipeline on 'use no forget' #30720

Conversation

poteto commented Aug 16, 2024 • edited Loading

vercel bot commented Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephsavona left a comment

Choose a reason for hiding this comment

poteto commented Aug 16, 2024 •

edited

Loading

vercel bot commented Aug 16, 2024 •

edited

Loading