ronantakizawa/codereview-bench huggingface dataset:
Repository: facebook/react
File: packages/react-dom/src/shared/ensureCorrectIsomorphicReactVersion.js (JavaScript)

Code before:
/**
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 *
 * @flow
 */

import reactDOMPackageVersion from 'shared/ReactVersion';
import * as IsomorphicReactPackage from 'react';

export function ensureCorrectIsomorphicReactVersion() {
  const isomorphicReactPackageVersion = IsomorphicReactPackage.version;
  if (isomorphicReactPackageVersion !== reactDOMPackageVersion) {
    // TODO: Do we have a canoncical documentation page for this?
    throw new Error(
      'Incompatible React versions: The "react" and "react-dom" packages must ' +
        'have the exact same version. Instead got:\n' +
        `  - react:      ${isomorphicReactPackageVersion}\n` +
        `  - react-dom:  ${reactDOMPackageVersion}`,
    );
  }
}


Review comment (refactor):
Closest is https://legacy.reactjs.org/warnings/invalid-hook-call-warning.html#mismatching-versions-of-react-and-react-dom and https://react.dev/warnings/invalid-hook-call-warning#mismatching-versions-of-react-and-react-dom. But these are just about a React version that's too low or duplicate React modules not explicitly about version mismatch. For example, package managers that auto-install peer dependencies will result in multiple versions of React if the specified version of `react` and `react-dom` don't match.

So maybe we include version mismatch in that section and link to it?

Code after:
/**
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 *
 * @flow
 */

import reactDOMPackageVersion from 'shared/ReactVersion';
import * as IsomorphicReactPackage from 'react';

export function ensureCorrectIsomorphicReactVersion() {
  const isomorphicReactPackageVersion = IsomorphicReactPackage.version;
  if (isomorphicReactPackageVersion !== reactDOMPackageVersion) {
    throw new Error(
      'Incompatible React versions: The "react" and "react-dom" packages must ' +
        'have the exact same version. Instead got:\n' +
        `  - react:      ${isomorphicReactPackageVersion}\n` +
        `  - react-dom:  ${reactDOMPackageVersion}\n` +
        'Learn more: https://react.dev/warnings/version-mismatch',
    );
  }
}
Repository: facebook/react
File: packages/react-dom-bindings/src/server/ReactDOMLegacyServerStreamConfig.js (JavaScript)

Code before:
/**
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 *
 * @flow
 */

export interface Destination {
  push(chunk: string | null): boolean;
  destroy(error: Error): mixed;
}

export opaque type PrecomputedChunk = string;
export opaque type Chunk = string;
export opaque type BinaryChunk = string;

export function scheduleMicrotask(callback: () => void) {
  throw new Error(
    'Legacy React Server builds should not be using scheduleMicrotask. This is a bug in React.',
  );
}

export function scheduleWork(callback: () => void) {
  callback();
}

export function flushBuffered(destination: Destination) {}

export function beginWriting(destination: Destination) {}

export function writeChunk(
  destination: Destination,
  chunk: Chunk | PrecomputedChunk | BinaryChunk,
): void {
  writeChunkAndReturn(destination, chunk);
}

export function writeChunkAndReturn(
  destination: Destination,
  chunk: Chunk | PrecomputedChunk | BinaryChunk,
): boolean {
  return destination.push(chunk);
}

export function completeWriting(destination: Destination) {}

Review comment (suggestion):
Should environments that previously scheduled all work sync just continue to do sync work? I Should probably rename to `scheduleWorkInCurrentTask` since it otherwise lies about what it does.

I made this error because we have no legacy builds of flight which is currently the only place this new API is used but this feels pretty inconsistent to me.

Code after:
/**
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 *
 * @flow
 */

export interface Destination {
  push(chunk: string | null): boolean;
  destroy(error: Error): mixed;
}

export opaque type PrecomputedChunk = string;
export opaque type Chunk = string;
export opaque type BinaryChunk = string;

export function scheduleWork(callback: () => void) {
  callback();
}

export function scheduleMicrotask(callback: () => void) {
  // While this defies the method name the legacy builds have special
  // overrides that make work scheduling sync. At the moment scheduleMicrotask
  // isn't used by any legacy APIs so this is somewhat academic but if they
  // did in the future we'd probably want to have this be in sync with scheduleWork
  callback();
}

export function flushBuffered(destination: Destination) {}

export function beginWriting(destination: Destination) {}

export function writeChunk(
  destination: Destination,
  chunk: Chunk | PrecomputedChunk | BinaryChunk,
): void {
  writeChunkAndReturn(destination, chunk);
}

export function writeChunkAndReturn(
  destination: Destination,
  chunk: Chunk | PrecomputedChunk | BinaryChunk,
): boolean {
  return destination.push(chunk);
}
Repository: facebook/react
File: packages/react-server/src/ReactServerStreamConfigBrowser.js (JavaScript)

Code before:
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 *
 * @flow
 */

export type Destination = ReadableStreamController;

export type PrecomputedChunk = Uint8Array;
export opaque type Chunk = Uint8Array;
export type BinaryChunk = Uint8Array;

function handleErrorInNextTick(error: any) {
  setTimeout(() => {
    throw error;
  });
}

export const scheduleMicrotask: (callback: () => void) => void =
  typeof queueMicrotask === 'function'
    ? queueMicrotask
    : callback => {
        Promise.resolve(null).then(callback).catch(handleErrorInNextTick);
      };

export function scheduleWork(callback: () => void) {
  callback();
}

export function flushBuffered(destination: Destination) {
  // WHATWG Streams do not yet have a way to flush the underlying
  // transform streams. https://github.com/whatwg/streams/issues/960
}

const VIEW_SIZE = 2048;
let currentView = null;
let writtenBytes = 0;

export function beginWriting(destination: Destination) {
  currentView = new Uint8Array(VIEW_SIZE);
  writtenBytes = 0;
}

export function writeChunk(
  destination: Destination,
  chunk: PrecomputedChunk | Chunk | BinaryChunk,
): void {
  if (chunk.byteLength === 0) {
    return;

Review comment (suggestion):
unlike in Fiber where there is a feature test for Promise I assume here that you must be using an environment with Promise globals. At this point I think we can safely just say React requires promises and since 19 is about to ship maybe we make this breaking change in Fiber too

Code after:
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 *
 * @flow
 */

export type Destination = ReadableStreamController;

export type PrecomputedChunk = Uint8Array;
export opaque type Chunk = Uint8Array;
export type BinaryChunk = Uint8Array;

const channel = new MessageChannel();
const taskQueue = [];
channel.port1.onmessage = () => {
  const task = taskQueue.shift();
  if (task) {
    task();
  }
};

export function scheduleWork(callback: () => void) {
  taskQueue.push(callback);
  channel.port2.postMessage(null);
}

function handleErrorInNextTick(error: any) {
  setTimeout(() => {
    throw error;
  });
}

const LocalPromise = Promise;

export const scheduleMicrotask: (callback: () => void) => void =
  typeof queueMicrotask === 'function'
    ? queueMicrotask
    : callback => {
        LocalPromise.resolve(null).then(callback).catch(handleErrorInNextTick);
      };

export function flushBuffered(destination: Destination) {
  // WHATWG Streams do not yet have a way to flush the underlying
  // transform streams. https://github.com/whatwg/streams/issues/960
}

const VIEW_SIZE = 2048;
let currentView = null;
let writtenBytes = 0;
Repository: facebook/react
File: compiler/packages/babel-plugin-react-compiler/src/HIR/Globals.ts (TypeScript)

Code before:
      returnValueKind: ValueKind.Frozen,
      returnValueReason: ValueReason.Context,
    }),
  ],
  [
    "useState",
    addHook(DEFAULT_SHAPES, {
      positionalParams: [],
      restParam: Effect.Freeze,
      returnType: { kind: "Object", shapeId: BuiltInUseStateId },
      calleeEffect: Effect.Read,
      hookKind: "useState",
      returnValueKind: ValueKind.Frozen,
      returnValueReason: ValueReason.State,
    }),
  ],
  [
    "useReducer",
    addHook(DEFAULT_SHAPES, {
      positionalParams: [],
      restParam: Effect.Freeze,
      returnType: { kind: "Object", shapeId: BuiltInUseReducerId },
      calleeEffect: Effect.Read,
      hookKind: "useReducer",
      returnValueKind: ValueKind.Frozen,
      returnValueReason: ValueReason.State,
    }),
  ],
  [
    "useRef",
    addHook(DEFAULT_SHAPES, {
      positionalParams: [],
      restParam: Effect.Capture,
      returnType: { kind: "Object", shapeId: BuiltInUseRefId },
      calleeEffect: Effect.Read,
      hookKind: "useRef",
      returnValueKind: ValueKind.Mutable,
    }),
  ],
  [
    "useMemo",
    addHook(DEFAULT_SHAPES, {
      positionalParams: [],
      restParam: Effect.Freeze,
      returnType: { kind: "Poly" },
      calleeEffect: Effect.Read,
      hookKind: "useMemo",
      returnValueKind: ValueKind.Frozen,
    }),
  ],
  [

Review comment (suggestion):
Let’s add a new ValueReason.Reducer enum variant and use it here, so that we can customize the error message for reducers

Code after:
      returnValueKind: ValueKind.Frozen,
      returnValueReason: ValueReason.Context,
    }),
  ],
  [
    "useState",
    addHook(DEFAULT_SHAPES, {
      positionalParams: [],
      restParam: Effect.Freeze,
      returnType: { kind: "Object", shapeId: BuiltInUseStateId },
      calleeEffect: Effect.Read,
      hookKind: "useState",
      returnValueKind: ValueKind.Frozen,
      returnValueReason: ValueReason.State,
    }),
  ],
  [
    "useReducer",
    addHook(DEFAULT_SHAPES, {
      positionalParams: [],
      restParam: Effect.Freeze,
      returnType: { kind: "Object", shapeId: BuiltInUseReducerId },
      calleeEffect: Effect.Read,
      hookKind: "useReducer",
      returnValueKind: ValueKind.Frozen,
      returnValueReason: ValueReason.ReducerState,
    }),
  ],
  [
    "useRef",
    addHook(DEFAULT_SHAPES, {
      positionalParams: [],
      restParam: Effect.Capture,
      returnType: { kind: "Object", shapeId: BuiltInUseRefId },
      calleeEffect: Effect.Read,
      hookKind: "useRef",
      returnValueKind: ValueKind.Mutable,
    }),
  ],
  [
    "useMemo",
    addHook(DEFAULT_SHAPES, {
      positionalParams: [],
      restParam: Effect.Freeze,
      returnType: { kind: "Poly" },
      calleeEffect: Effect.Read,
      hookKind: "useMemo",
      returnValueKind: ValueKind.Frozen,
    }),
  ],
  [
Repository: facebook/react
File: compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts (TypeScript)

Code before:
  });
}

class CountMemoBlockVisitor extends ReactiveFunctionVisitor<void> {
  env: Environment;
  memoBlocks: number = 0;
  memoValues: number = 0;
  prunedMemoBlocks: number = 0;
  prunedMemoValues: number = 0;

  constructor(env: Environment) {
    super();
    this.env = env;
  }

  override visitScope(scopeBlock: ReactiveScopeBlock, state: void): void {
    this.memoBlocks += 1;
    this.memoValues += scopeBlock.scope.declarations.size;
    this.traverseScope(scopeBlock, state);
  }

  override visitPrunedScope(
    scopeBlock: PrunedReactiveScopeBlock,
    state: void
  ): void {
    let isHookOnlyMemoBlock = false;
    if (
      scopeBlock.instructions.length === 1 &&
      scopeBlock.instructions[0].kind === "instruction"
    ) {
      const instr = scopeBlock.instructions[0]!.instruction;
      if (
        instr.value.kind === "MethodCall" ||
        instr.value.kind === "CallExpression"
      ) {
        const callee =
          instr.value.kind === "MethodCall"
            ? instr.value.property
            : instr.value.callee;
        if (getHookKind(this.env, callee.identifier) != null) {
          isHookOnlyMemoBlock = true;
        }
      }
    }
    if (!isHookOnlyMemoBlock) {
      this.prunedMemoBlocks += 1;
      this.prunedMemoValues += scopeBlock.scope.declarations.size;
    }
    this.traversePrunedScope(scopeBlock, state);
  }
}

Review comment (suggestion):
this logic is now in FlattenScopesWithHooksOrUse, and also takes account of `use()` and not just hooks

Code after:
  });
}

class CountMemoBlockVisitor extends ReactiveFunctionVisitor<void> {
  env: Environment;
  memoBlocks: number = 0;
  memoValues: number = 0;
  prunedMemoBlocks: number = 0;
  prunedMemoValues: number = 0;

  constructor(env: Environment) {
    super();
    this.env = env;
  }

  override visitScope(scopeBlock: ReactiveScopeBlock, state: void): void {
    this.memoBlocks += 1;
    this.memoValues += scopeBlock.scope.declarations.size;
    this.traverseScope(scopeBlock, state);
  }

  override visitPrunedScope(
    scopeBlock: PrunedReactiveScopeBlock,
    state: void
  ): void {
    this.prunedMemoBlocks += 1;
    this.prunedMemoValues += scopeBlock.scope.declarations.size;
    this.traversePrunedScope(scopeBlock, state);
  }
}

function convertParameter(
  param: Place | SpreadPattern
): t.Identifier | t.RestElement {
  if (param.kind === "Identifier") {
    return convertIdentifier(param.identifier);
  } else {
    return t.restElement(convertIdentifier(param.place.identifier));
  }
}

class Context {
  env: Environment;
  fnName: string;
  #nextCacheIndex: number = 0;
  #declarations: Set<IdentifierId> = new Set();
  temp: Temporaries;
  errors: CompilerError = new CompilerError();
  objectMethods: Map<IdentifierId, ObjectMethod> = new Map();
  uniqueIdentifiers: Set<string>;
  synthesizedNames: Map<string, ValidIdentifierName> = new Map();
Repository: facebook/react
File: compiler/packages/babel-plugin-react-compiler/src/HIR/visitors.ts (TypeScript)

Code before:
        kind: "maybe-throw",
        continuation,
        handler,
        id: makeInstructionId(0),
        loc: terminal.loc,
      };
    }
    case "try": {
      const block = fn(terminal.block);
      const handler = fn(terminal.handler);
      const fallthrough = fn(terminal.fallthrough);
      return {
        kind: "try",
        block,
        handlerBinding: terminal.handlerBinding,
        handler,
        fallthrough,
        id: makeInstructionId(0),
        loc: terminal.loc,
      };
    }
    case "scope": {
      const block = fn(terminal.block);
      const fallthrough = fn(terminal.fallthrough);
      return {
        kind: "scope",
        scope: terminal.scope,
        block,
        fallthrough,
        id: makeInstructionId(0),
        loc: terminal.loc,
      };
    }
    case "unreachable":
    case "unsupported": {
      return terminal;
    }
    default: {
      assertExhaustive(
        terminal,
        `Unexpected terminal kind \`${(terminal as any as Terminal).kind}\``
      );
    }
  }
}

export function terminalHasFallthrough<
  T extends Terminal,
  U extends T & { fallthrough: BlockId },
>(terminal: T): terminal is U {
  switch (terminal.kind) {

Review comment (suggestion):
Hi @josephsavona, would it be appropriate to change the value of the return value's `kind` to `terminal.kind`?
https://github.com/facebook/react/pull/29837/files/2e39ef7c24a5155110929c19f918842972c58989#diff-8a37300261fbcd405210aeb6f21ea36db52abdc192b1215262ce2358f9bcc762L858

Code after:
        kind: "maybe-throw",
        continuation,
        handler,
        id: makeInstructionId(0),
        loc: terminal.loc,
      };
    }
    case "try": {
      const block = fn(terminal.block);
      const handler = fn(terminal.handler);
      const fallthrough = fn(terminal.fallthrough);
      return {
        kind: "try",
        block,
        handlerBinding: terminal.handlerBinding,
        handler,
        fallthrough,
        id: makeInstructionId(0),
        loc: terminal.loc,
      };
    }
    case "scope":
    case "pruned-scope": {
      const block = fn(terminal.block);
      const fallthrough = fn(terminal.fallthrough);
      return {
        kind: "scope",
        scope: terminal.scope,
        block,
        fallthrough,
        id: makeInstructionId(0),
        loc: terminal.loc,
      };
    }
    case "unreachable":
    case "unsupported": {
      return terminal;
    }
    default: {
      assertExhaustive(
        terminal,
        `Unexpected terminal kind \`${(terminal as any as Terminal).kind}\``
      );
    }
  }
}

export function terminalHasFallthrough<
  T extends Terminal,
  U extends T & { fallthrough: BlockId },
>(terminal: T): terminal is U {
Repository: facebook/react
File: packages/react-server/src/ReactFizzServer.js (JavaScript)

Code before:
  owner: null | ReactComponentInfo | ComponentStackNode, // DEV only
  stack: null | Error, // DEV only
): ComponentStackNode {
  if (__DEV__) {
    return {
      tag: 2,
      parent: task.componentStack,
      type,
      owner,
      stack,
    };
  }
  return {
    tag: 2,
    parent: task.componentStack,
    type,
  };
}
function pushServerComponentStack(
  task: Task,
  debugInfo: void | null | ReactDebugInfo,
): void {
  if (!__DEV__) {
    // eslint-disable-next-line react-internal/prod-error-codes
    throw new Error(
      'createServerComponentStack should never be called in production. This is a bug in React.',
    );
  }
  // Build a Server Component parent stack from the debugInfo.
  if (debugInfo != null) {
    const stack: ReactDebugInfo = debugInfo;
    for (let i = 0; i < stack.length; i++) {
      const componentInfo: ReactComponentInfo = (stack[i]: any);
      if (typeof componentInfo.name !== 'string') {
        continue;
      }
      if (enableOwnerStacks && componentInfo.stack === undefined) {
        continue;
      }
      let name = componentInfo.name;
      const env = componentInfo.env;
      if (env) {
        name += ' (' + env + ')';
      }
      task.componentStack = {
        tag: 3,
        parent: task.componentStack,
        type: name,
        owner: componentInfo.owner,
        stack: componentInfo.stack,
      };

Review comment (bug):
```suggestion
      'pushServerComponentStack should never be called in production. This is a bug in React.',
```

Code after:
  owner: null | ReactComponentInfo | ComponentStackNode, // DEV only
  stack: null | Error, // DEV only
): ComponentStackNode {
  if (__DEV__) {
    return {
      tag: 2,
      parent: task.componentStack,
      type,
      owner,
      stack,
    };
  }
  return {
    tag: 2,
    parent: task.componentStack,
    type,
  };
}
function pushServerComponentStack(
  task: Task,
  debugInfo: void | null | ReactDebugInfo,
): void {
  if (!__DEV__) {
    // eslint-disable-next-line react-internal/prod-error-codes
    throw new Error(
      'pushServerComponentStack should never be called in production. This is a bug in React.',
    );
  }
  // Build a Server Component parent stack from the debugInfo.
  if (debugInfo != null) {
    const stack: ReactDebugInfo = debugInfo;
    for (let i = 0; i < stack.length; i++) {
      const componentInfo: ReactComponentInfo = (stack[i]: any);
      if (typeof componentInfo.name !== 'string') {
        continue;
      }
      if (enableOwnerStacks && componentInfo.stack === undefined) {
        continue;
      }
      let name = componentInfo.name;
      const env = componentInfo.env;
      if (env) {
        name += ' (' + env + ')';
      }
      task.componentStack = {
        tag: 3,
        parent: task.componentStack,
        type: name,
        owner: componentInfo.owner,
        stack: componentInfo.stack,
      };
Repository: facebook/react
File: packages/react-devtools-shared/src/backend/DevToolsFiberComponentStack.js (JavaScript)

Code before:

    while (owner) {
      if (typeof owner.tag === 'number') {
        const fiber: Fiber = (owner: any);
        owner = fiber._debugOwner;
        let debugStack = fiber._debugStack;
        // If we don't actually print the stack if there is no owner of this JSX element.
        // In a real app it's typically not useful since the root app is always controlled
        // by the framework. These also tend to have noisy stacks because they're not rooted
        // in a React render but in some imperative bootstrapping code. It could be useful
        // if the element was created in module scope. E.g. hoisted. We could add a a single
        // stack frame for context for example but it doesn't say much if that's a wrapper.
        if (owner && debugStack) {
          if (typeof debugStack !== 'string') {
            debugStack = formatOwnerStack(debugStack);
          }
          if (debugStack !== '') {
            info += '\n' + debugStack;
          }
        }
      } else if (owner.debugStack != null) {
        // Server Component
        const ownerStack: Error = owner.debugStack;
        owner = owner.owner;
        if (owner && ownerStack) {
          info += '\n' + formatOwnerStack(ownerStack);
        }
      } else {
        break;
      }
    }
    return info;
  } catch (x) {
    return '\nError generating stack: ' + x.message + '\n' + x.stack;
  }
}


Review comment (suggestion):
Since the actual stack usually has a frame inside the current component, we don't have to add it even if there's no owner since otherwise it'll end up show up twice. I'll do a separate PR to align this with captureOwnerStack() which should also exclude these.

Code after:
      if (typeof owner.tag === 'number') {
        const fiber: Fiber = (owner: any);
        owner = fiber._debugOwner;
        let debugStack = fiber._debugStack;
        // If we don't actually print the stack if there is no owner of this JSX element.
        // In a real app it's typically not useful since the root app is always controlled
        // by the framework. These also tend to have noisy stacks because they're not rooted
        // in a React render but in some imperative bootstrapping code. It could be useful
        // if the element was created in module scope. E.g. hoisted. We could add a a single
        // stack frame for context for example but it doesn't say much if that's a wrapper.
        if (owner && debugStack) {
          if (typeof debugStack !== 'string') {
            debugStack = formatOwnerStack(debugStack);
          }
          if (debugStack !== '') {
            info += '\n' + debugStack;
          }
        }
      } else if (owner.debugStack != null) {
        // Server Component
        const ownerStack: Error = owner.debugStack;
        owner = owner.owner;
        if (owner && ownerStack) {
          info += '\n' + formatOwnerStack(ownerStack);
        }
      } else {
        break;
      }
    }
    return info;
  } catch (x) {
    return '\nError generating stack: ' + x.message + '\n' + x.stack;
  }
}