Update on "[compiler] Validate environment config while parsing plugin opts"

Addresses a todo from a while back. We now validate environment options when parsing the plugin options, which means we can stop re-parsing/validating in later phases.

[ghstack-poisoned]

Author: josephsavona

ReactVersions.js

@@ -52,7 +52,7 @@ const stablePackages = {
 // These packages do not exist in the @canary or @latest channel, only
 // @experimental. We don't use semver, just the commit sha, so this is just a
 // list of package names instead of a map.
-const experimentalPackages = [];
+const experimentalPackages = ['react-markup'];
 
 module.exports = {
   ReactVersion,

compiler/apps/playground/components/Editor/EditorImpl.tsx

@@ -66,14 +66,14 @@ function parseFunctions(
   source: string,
   language: 'flow' | 'typescript',
 ): Array<
-  NodePath<
-    t.FunctionDeclaration | t.ArrowFunctionExpression | t.FunctionExpression
-  >
+  | NodePath<t.FunctionDeclaration>
+  | NodePath<t.ArrowFunctionExpression>
+  | NodePath<t.FunctionExpression>
 > {
   const items: Array<
-    NodePath<
-      t.FunctionDeclaration | t.ArrowFunctionExpression | t.FunctionExpression
-    >
+    | NodePath<t.FunctionDeclaration>
+    | NodePath<t.ArrowFunctionExpression>
+    | NodePath<t.FunctionExpression>
   > = [];
   try {
     const ast = parseInput(source, language);
@@ -155,22 +155,33 @@ function isHookName(s: string): boolean {
   return /^use[A-Z0-9]/.test(s);
 }
 
-function getReactFunctionType(
-  id: NodePath<t.Identifier | null | undefined>,
-): ReactFunctionType {
-  if (id && id.node && id.isIdentifier()) {
-    if (isHookName(id.node.name)) {
+function getReactFunctionType(id: t.Identifier | null): ReactFunctionType {
+  if (id != null) {
+    if (isHookName(id.name)) {
       return 'Hook';
     }
 
     const isPascalCaseNameSpace = /^[A-Z].*/;
-    if (isPascalCaseNameSpace.test(id.node.name)) {
+    if (isPascalCaseNameSpace.test(id.name)) {
       return 'Component';
     }
   }
   return 'Other';
 }
 
+function getFunctionIdentifier(
+  fn:
+    | NodePath<t.FunctionDeclaration>
+    | NodePath<t.ArrowFunctionExpression>
+    | NodePath<t.FunctionExpression>,
+): t.Identifier | null {
+  if (fn.isArrowFunctionExpression()) {
+    return null;
+  }
+  const id = fn.get('id');
+  return Array.isArray(id) === false && id.isIdentifier() ? id.node : null;
+}
+
 function compile(source: string): [CompilerOutput, 'flow' | 'typescript'] {
   const results = new Map<string, PrintedCompilerPipelineValue[]>();
   const error = new CompilerError();
@@ -188,27 +199,21 @@ function compile(source: string): [CompilerOutput, 'flow' | 'typescript'] {
   } else {
     language = 'typescript';
   }
+  let count = 0;
+  const withIdentifier = (id: t.Identifier | null): t.Identifier => {
+    if (id != null && id.name != null) {
+      return id;
+    } else {
+      return t.identifier(`anonymous_${count++}`);
+    }
+  };
   try {
     // Extract the first line to quickly check for custom test directives
     const pragma = source.substring(0, source.indexOf('\n'));
     const config = parseConfigPragma(pragma);
 
     for (const fn of parseFunctions(source, language)) {
-      if (!fn.isFunctionDeclaration()) {
-        error.pushErrorDetail(
-          new CompilerErrorDetail({
-            reason: `Unexpected function type ${fn.node.type}`,
-            description:
-              'Playground only supports parsing function declarations',
-            severity: ErrorSeverity.Todo,
-            loc: fn.node.loc ?? null,
-            suggestions: null,
-          }),
-        );
-        continue;
-      }
-
-      const id = fn.get('id');
+      const id = withIdentifier(getFunctionIdentifier(fn));
       for (const result of run(
         fn,
         {
@@ -221,7 +226,7 @@ function compile(source: string): [CompilerOutput, 'flow' | 'typescript'] {
         null,
         null,
       )) {
-        const fnName = fn.node.id?.name ?? null;
+        const fnName = id.name;
         switch (result.kind) {
           case 'ast': {
             upsert({
@@ -230,7 +235,7 @@ function compile(source: string): [CompilerOutput, 'flow' | 'typescript'] {
               name: result.name,
               value: {
                 type: 'FunctionDeclaration',
-                id: result.value.id,
+                id,
                 async: result.value.async,
                 generator: result.value.generator,
                 body: result.value.body,

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

@@ -169,6 +169,12 @@ export type LoggerEvent =
       fnLoc: t.SourceLocation | null;
       detail: Omit<Omit<CompilerErrorDetailOptions, 'severity'>, 'suggestions'>;
     }
+  | {
+      kind: 'CompileSkip';
+      fnLoc: t.SourceLocation | null;
+      reason: string;
+      loc: t.SourceLocation | null;
+    }
   | {
       kind: 'CompileSuccess';
       fnLoc: t.SourceLocation | null;

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

@@ -105,7 +105,6 @@ import {outlineFunctions} from '../Optimization/OutlineFunctions';
 import {propagatePhiTypes} from '../TypeInference/PropagatePhiTypes';
 import {lowerContextAccess} from '../Optimization/LowerContextAccess';
 import {validateNoSetStateInPassiveEffects} from '../Validation/ValidateNoSetStateInPassiveEffects';
-import {validateNoJSXInTryStatement} from '../Validation/ValidateNoJSXInTryStatement';
 
 export type CompilerPipelineValue =
   | {kind: 'ast'; name: string; value: CodegenFunction}
@@ -250,10 +249,6 @@ function* runWithEnvironment(
     validateNoSetStateInPassiveEffects(hir);
   }
 
-  if (env.config.validateNoJSXInTryStatements) {
-    validateNoJSXInTryStatement(hir);
-  }
-
   inferReactivePlaces(hir);
   yield log({kind: 'hir', name: 'InferReactivePlaces', value: hir});
 

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

@@ -42,34 +42,23 @@ export type CompilerPass = {
   comments: Array<t.CommentBlock | t.CommentLine>;
   code: string | null;
 };
+const OPT_IN_DIRECTIVES = new Set(['use forget', 'use memo']);
+export const OPT_OUT_DIRECTIVES = new Set(['use no forget', 'use no memo']);
 
 function findDirectiveEnablingMemoization(
   directives: Array<t.Directive>,
-): t.Directive | null {
-  for (const directive of directives) {
-    const directiveValue = directive.value.value;
-    if (directiveValue === 'use forget' || directiveValue === 'use memo') {
-      return directive;
-    }
-  }
-  return null;
+): Array<t.Directive> {
+  return directives.filter(directive =>
+    OPT_IN_DIRECTIVES.has(directive.value.value),
+  );
 }
 
 function findDirectiveDisablingMemoization(
   directives: Array<t.Directive>,
-  options: PluginOptions,
-): t.Directive | null {
-  for (const directive of directives) {
-    const directiveValue = directive.value.value;
-    if (
-      (directiveValue === 'use no forget' ||
-        directiveValue === 'use no memo') &&
-      !options.ignoreUseNoForget
-    ) {
-      return directive;
-    }
-  }
-  return null;
+): Array<t.Directive> {
+  return directives.filter(directive =>
+    OPT_OUT_DIRECTIVES.has(directive.value.value),
+  );
 }
 
 function isCriticalError(err: unknown): boolean {
@@ -101,7 +90,7 @@ export type CompileResult = {
   compiledFn: CodegenFunction;
 };
 
-function handleError(
+function logError(
   err: unknown,
   pass: CompilerPass,
   fnLoc: t.SourceLocation | null,
@@ -130,6 +119,13 @@ function handleError(
       });
     }
   }
+}
+function handleError(
+  err: unknown,
+  pass: CompilerPass,
+  fnLoc: t.SourceLocation | null,
+): void {
+  logError(err, pass, fnLoc);
   if (
     pass.opts.panicThreshold === 'all_errors' ||
     (pass.opts.panicThreshold === 'critical_errors' && isCriticalError(err)) ||
@@ -378,6 +374,17 @@ export function compileProgram(
     fn: BabelFn,
     fnType: ReactFunctionType,
   ): null | CodegenFunction => {
+    let optInDirectives: Array<t.Directive> = [];
+    let optOutDirectives: Array<t.Directive> = [];
+    if (fn.node.body.type === 'BlockStatement') {
+      optInDirectives = findDirectiveEnablingMemoization(
+        fn.node.body.directives,
+      );
+      optOutDirectives = findDirectiveDisablingMemoization(
+        fn.node.body.directives,
+      );
+    }
+
     if (lintError != null) {
       /**
        * Note that Babel does not attach comment nodes to nodes; they are dangling off of the
@@ -389,7 +396,11 @@ export function compileProgram(
         fn,
       );
       if (suppressionsInFunction.length > 0) {
-        handleError(lintError, pass, fn.node.loc ?? null);
+        if (optOutDirectives.length > 0) {
+          logError(lintError, pass, fn.node.loc ?? null);
+        } else {
+          handleError(lintError, pass, fn.node.loc ?? null);
+        }
       }
     }
 
@@ -415,11 +426,50 @@ export function compileProgram(
         prunedMemoValues: compiledFn.prunedMemoValues,
       });
     } catch (err) {
+      /**
+       * If an opt out directive is present, log only instead of throwing and don't mark as
+       * containing a critical error.
+       */
+      if (fn.node.body.type === 'BlockStatement') {
+        if (optOutDirectives.length > 0) {
+          logError(err, pass, fn.node.loc ?? null);
+          return null;
+        }
+      }
       hasCriticalError ||= isCriticalError(err);
       handleError(err, pass, fn.node.loc ?? null);
       return null;
     }
 
+    /**
+     * Always compile functions with opt in directives.
+     */
+    if (optInDirectives.length > 0) {
+      return compiledFn;
+    } else if (pass.opts.compilationMode === 'annotation') {
+      /**
+       * No opt-in directive in annotation mode, so don't insert the compiled function.
+       */
+      return null;
+    }
+
+    /**
+     * Otherwise if 'use no forget/memo' is present, we still run the code through the compiler
+     * for validation but we don't mutate the babel AST. This allows us to flag if there is an
+     * unused 'use no forget/memo' directive.
+     */
+    if (pass.opts.ignoreUseNoForget === false && optOutDirectives.length > 0) {
+      for (const directive of optOutDirectives) {
+        pass.opts.logger?.logEvent(pass.filename, {
+          kind: 'CompileSkip',
+          fnLoc: fn.node.body.loc ?? null,
+          reason: `Skipped due to '${directive.value.value}' directive.`,
+          loc: directive.loc ?? null,
+        });
+      }
+      return null;
+    }
+
     if (!pass.opts.noEmit && !hasCriticalError) {
       return compiledFn;
     }
@@ -466,6 +516,16 @@ export function compileProgram(
     });
   }
 
+  /**
+   * Do not modify source if there is a module scope level opt out directive.
+   */
+  const moduleScopeOptOutDirectives = findDirectiveDisablingMemoization(
+    program.node.directives,
+  );
+  if (moduleScopeOptOutDirectives.length > 0) {
+    return;
+  }
+
   if (pass.opts.gating != null) {
     const error = checkFunctionReferencedBeforeDeclarationAtTopLevel(
       program,
@@ -581,24 +641,6 @@ function shouldSkipCompilation(
     }
   }
 
-  // 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;
-  }
   const moduleName = pass.opts.runtimeModule ?? 'react/compiler-runtime';
   if (hasMemoCacheFunctionImport(program, moduleName)) {
     return true;
@@ -616,28 +658,8 @@ function getReactFunctionType(
 ): ReactFunctionType | null {
   const hookPattern = environment.hookPattern;
   if (fn.node.body.type === 'BlockStatement') {
-    // 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';
-    }
   }
 
   // Component and hook declarations are known components/hooks

compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts

@@ -237,12 +237,6 @@ const EnvironmentConfigSchema = z.object({
    */
   validateNoSetStateInPassiveEffects: z.boolean().default(false),
 
-  /**
-   * Validates against creating JSX within a try block and recommends using an error boundary
-   * instead.
-   */
-  validateNoJSXInTryStatements: z.boolean().default(false),
-
   /**
    * Validates that the dependencies of all effect hooks are memoized. This helps ensure
    * that Forget does not introduce infinite renders caused by a dependency changing,