[compiler] Cleanup: consistent tryRecord() wrapping and error recording

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

@@ -728,19 +728,21 @@ function tryCompileFunction(
   }
 
   try {
-    return {
-      kind: 'compile',
-      compiledFn: compileFn(
-        fn,
-        programContext.opts.environment,
-        fnType,
-        outputMode,
-        programContext,
-        programContext.opts.logger,
-        programContext.filename,
-        programContext.code,
-      ),
-    };
+    const result = compileFn(
+      fn,
+      programContext.opts.environment,
+      fnType,
+      outputMode,
+      programContext,
+      programContext.opts.logger,
+      programContext.filename,
+      programContext.code,
+    );
+    if (result.isOk()) {
+      return {kind: 'compile', compiledFn: result.unwrap()};
+    } else {
+      return {kind: 'error', error: result.unwrapErr()};
+    }
   } catch (err) {
     return {kind: 'error', error: err};
   }
@@ -779,10 +781,16 @@ function retryCompileFunction(
       programContext.code,
     );
 
-    if (!retryResult.hasFireRewrite && !retryResult.hasInferredEffect) {
+    if (retryResult.isErr()) {
+      const err = retryResult.unwrapErr();
+      programContext.retryErrors.push({fn, error: err});
+      return null;
+    }
+    const compiledFn = retryResult.unwrap();
+    if (!compiledFn.hasFireRewrite && !compiledFn.hasInferredEffect) {
       return null;
     }
-    return retryResult;
+    return compiledFn;
   } catch (err) {
     // TODO: we might want to log error here, but this will also result in duplicate logging
     if (err instanceof CompilerError) {

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

@@ -14,7 +14,6 @@ import {
   CompilerSuggestionOperation,
   ErrorCategory,
 } from '../CompilerError';
-import {Err, Ok, Result} from '../Utils/Result';
 import {assertExhaustive, hasNode} from '../Utils/utils';
 import {Environment} from './Environment';
 import {
@@ -75,7 +74,7 @@ export function lower(
   // Bindings captured from the outer function, in case lower() is called recursively (for lambdas)
   bindings: Bindings | null = null,
   capturedRefs: Map<t.Identifier, SourceLocation> = new Map(),
-): Result<HIRFunction, CompilerError> {
+): HIRFunction {
   const builder = new HIRBuilder(env, {
     bindings,
     context: capturedRefs,
@@ -186,32 +185,47 @@ export function lower(
 
   let directives: Array<string> = [];
   const body = func.get('body');
-  if (body.isExpression()) {
-    const fallthrough = builder.reserve('block');
-    const terminal: ReturnTerminal = {
-      kind: 'return',
-      returnVariant: 'Implicit',
-      loc: GeneratedSource,
-      value: lowerExpressionToTemporary(builder, body),
-      id: makeInstructionId(0),
-      effects: null,
-    };
-    builder.terminateWithContinuation(terminal, fallthrough);
-  } else if (body.isBlockStatement()) {
-    lowerStatement(builder, body);
-    directives = body.get('directives').map(d => d.node.value.value);
-  } else {
-    builder.errors.pushDiagnostic(
-      CompilerDiagnostic.create({
-        category: ErrorCategory.Syntax,
-        reason: `Unexpected function body kind`,
-        description: `Expected function body to be an expression or a block statement, got \`${body.type}\``,
-      }).withDetails({
-        kind: 'error',
-        loc: body.node.loc ?? null,
-        message: 'Expected a block statement or expression',
-      }),
-    );
+  try {
+    if (body.isExpression()) {
+      const fallthrough = builder.reserve('block');
+      const terminal: ReturnTerminal = {
+        kind: 'return',
+        returnVariant: 'Implicit',
+        loc: GeneratedSource,
+        value: lowerExpressionToTemporary(builder, body),
+        id: makeInstructionId(0),
+        effects: null,
+      };
+      builder.terminateWithContinuation(terminal, fallthrough);
+    } else if (body.isBlockStatement()) {
+      lowerStatement(builder, body);
+      directives = body.get('directives').map(d => d.node.value.value);
+    } else {
+      builder.errors.pushDiagnostic(
+        CompilerDiagnostic.create({
+          category: ErrorCategory.Syntax,
+          reason: `Unexpected function body kind`,
+          description: `Expected function body to be an expression or a block statement, got \`${body.type}\``,
+        }).withDetails({
+          kind: 'error',
+          loc: body.node.loc ?? null,
+          message: 'Expected a block statement or expression',
+        }),
+      );
+    }
+  } catch (err) {
+    if (err instanceof CompilerError) {
+      // Re-throw invariant errors immediately
+      for (const detail of err.details) {
+        if (detail.category === ErrorCategory.Invariant) {
+          throw err;
+        }
+      }
+      // Record non-invariant errors and continue to produce partial HIR
+      builder.errors.merge(err);
+    } else {
+      throw err;
+    }
   }
 
   let validatedId: HIRFunction['id'] = null;
@@ -224,10 +238,6 @@ export function lower(
     }
   }
 
-  if (builder.errors.hasAnyErrors()) {
-    return Err(builder.errors);
-  }
-
   builder.terminate(
     {
       kind: 'return',
@@ -244,23 +254,29 @@ export function lower(
     null,
   );
 
-  return Ok({
+  const hirBody = builder.build();
+
+  // Record all accumulated errors (including any from build()) on env
+  if (builder.errors.hasAnyErrors()) {
+    env.recordErrors(builder.errors);
+  }
+
+  return {
     id: validatedId,
     nameHint: null,
     params,
     fnType: bindings == null ? env.fnType : 'Other',
     returnTypeAnnotation: null, // TODO: extract the actual return type node if present
     returns: createTemporaryPlace(env, func.node.loc ?? GeneratedSource),
-    body: builder.build(),
+    body: hirBody,
     context,
     generator: func.node.generator === true,
     async: func.node.async === true,
     loc: func.node.loc ?? GeneratedSource,
     env,
-    effects: null,
     aliasingEffects: null,
     directives,
-  });
+  };
 }
 
 // Helper to lower a statement
@@ -555,6 +571,22 @@ function lowerStatement(
 
       const initBlock = builder.enter('loop', _blockId => {
         const init = stmt.get('init');
+        if (init.node == null) {
+          // No init expression (e.g., `for (; ...)`), add a placeholder to avoid
+          // invariant about empty blocks
+          lowerValueToTemporary(builder, {
+            kind: 'Primitive',
+            value: undefined,
+            loc: stmt.node.loc ?? GeneratedSource,
+          });
+          return {
+            kind: 'goto',
+            block: testBlock.id,
+            variant: GotoVariant.Break,
+            id: makeInstructionId(0),
+            loc: stmt.node.loc ?? GeneratedSource,
+          };
+        }
         if (!init.isVariableDeclaration()) {
           builder.errors.push({
             reason:
@@ -563,8 +595,14 @@ function lowerStatement(
             loc: stmt.node.loc ?? null,
             suggestions: null,
           });
+          // Lower the init expression as best-effort and continue
+          if (init.isExpression()) {
+            lowerExpressionToTemporary(builder, init as NodePath<t.Expression>);
+          }
           return {
-            kind: 'unsupported',
+            kind: 'goto',
+            block: testBlock.id,
+            variant: GotoVariant.Break,
             id: makeInstructionId(0),
             loc: init.node?.loc ?? GeneratedSource,
           };
@@ -635,6 +673,23 @@ function lowerStatement(
           loc: stmt.node.loc ?? null,
           suggestions: null,
         });
+        // Treat `for(;;)` as `while(true)` to keep the builder state consistent
+        builder.terminateWithContinuation(
+          {
+            kind: 'branch',
+            test: lowerValueToTemporary(builder, {
+              kind: 'Primitive',
+              value: true,
+              loc: stmt.node.loc ?? GeneratedSource,
+            }),
+            consequent: bodyBlock,
+            alternate: continuationBlock.id,
+            fallthrough: continuationBlock.id,
+            id: makeInstructionId(0),
+            loc: stmt.node.loc ?? GeneratedSource,
+          },
+          continuationBlock,
+        );
       } else {
         builder.terminateWithContinuation(
           {
@@ -858,10 +913,12 @@ function lowerStatement(
           loc: stmt.node.loc ?? null,
           suggestions: null,
         });
-        return;
+        // Treat `var` as `let` so references to the variable don't break
       }
       const kind =
-        nodeKind === 'let' ? InstructionKind.Let : InstructionKind.Const;
+        nodeKind === 'let' || nodeKind === 'var'
+          ? InstructionKind.Let
+          : InstructionKind.Const;
       for (const declaration of stmt.get('declarations')) {
         const id = declaration.get('id');
         const init = declaration.get('init');
@@ -1494,9 +1551,6 @@ function lowerObjectMethod(
 ): InstructionValue {
   const loc = property.node.loc ?? GeneratedSource;
   const loweredFunc = lowerFunction(builder, property);
-  if (!loweredFunc) {
-    return {kind: 'UnsupportedNode', node: property.node, loc: loc};
-  }
 
   return {
     kind: 'ObjectMethod',
@@ -2276,18 +2330,20 @@ function lowerExpression(
         });
         for (const [name, locations] of Object.entries(fbtLocations)) {
           if (locations.length > 1) {
-            CompilerError.throwDiagnostic({
-              category: ErrorCategory.Todo,
-              reason: 'Support duplicate fbt tags',
-              description: `Support \`<${tagName}>\` tags with multiple \`<${tagName}:${name}>\` values`,
-              details: locations.map(loc => {
-                return {
-                  kind: 'error',
-                  message: `Multiple \`<${tagName}:${name}>\` tags found`,
-                  loc,
-                };
+            builder.errors.pushDiagnostic(
+              new CompilerDiagnostic({
+                category: ErrorCategory.Todo,
+                reason: 'Support duplicate fbt tags',
+                description: `Support \`<${tagName}>\` tags with multiple \`<${tagName}:${name}>\` values`,
+                details: locations.map(loc => {
+                  return {
+                    kind: 'error' as const,
+                    message: `Multiple \`<${tagName}:${name}>\` tags found`,
+                    loc,
+                  };
+                }),
               }),
-            });
+            );
           }
         }
       }
@@ -3468,9 +3524,6 @@ function lowerFunctionToValue(
   const exprNode = expr.node;
   const exprLoc = exprNode.loc ?? GeneratedSource;
   const loweredFunc = lowerFunction(builder, expr);
-  if (!loweredFunc) {
-    return {kind: 'UnsupportedNode', node: exprNode, loc: exprLoc};
-  }
   return {
     kind: 'FunctionExpression',
     name: loweredFunc.func.id,
@@ -3489,7 +3542,7 @@ function lowerFunction(
     | t.FunctionDeclaration
     | t.ObjectMethod
   >,
-): LoweredFunction | null {
+): LoweredFunction {
   const componentScope: Scope = builder.environment.parentFunction.scope;
   const capturedContext = gatherCapturedContext(expr, componentScope);
 
@@ -3501,19 +3554,12 @@ function lowerFunction(
    * This isn't a problem in practice because use Babel's scope analysis to
    * identify the correct references.
    */
-  const lowering = lower(
+  const loweredFunc = lower(
     expr,
     builder.environment,
     builder.bindings,
     new Map([...builder.context, ...capturedContext]),
   );
-  let loweredFunc: HIRFunction;
-  if (lowering.isErr()) {
-    const functionErrors = lowering.unwrapErr();
-    builder.errors.merge(functionErrors);
-    return null;
-  }
-  loweredFunc = lowering.unwrap();
   return {
     func: loweredFunc,
   };