ronantakizawa/codereview-bench huggingface dataset:
Repository: denoland/deno
File: ext/node/polyfills/internal_binding/tcp_wrap.ts (TypeScript)

Code before:

        break;
      }
      default: {
        unreachable();
      }
    }

    super(provider, conn);

    // TODO(cmorten): the handling of new connections and construction feels
    // a little off. Suspect duplicating in some fashion.
    if (conn && provider === providerType.TCPWRAP) {
      const localAddr = conn.localAddr as Deno.NetAddr;
      this.#address = localAddr.hostname;
      this.#port = localAddr.port;

      const remoteAddr = conn.remoteAddr as Deno.NetAddr;
      this.#remoteAddress = remoteAddr.hostname;
      this.#remotePort = remoteAddr.port;
      const ipVersion = isIP(remoteAddr.hostname);
      this.#remoteFamily = ipVersion === 6
        ? "IPv6"
        : ipVersion === 4
        ? "IPv4"
        : undefined;
    }
  }

  get fd() {
    return this[kStreamBaseField]?.[internalFdSymbol];
  }

  /**
   * Opens a file descriptor.
   * @param fd The file descriptor to open.
   * @return An error status code.
   */
  open(_fd: number): number {
    // REF: https://github.com/denoland/deno/issues/6529
    notImplemented("TCP.prototype.open");
  }

  /**
   * Bind to an IPv4 address.
   * @param address The hostname to bind to.
   * @param port The port to bind to
   * @return An error status code.
   */
  bind(address: string, port: number): number {
    return this.#bind(address, port, 0);

Review comment (refactor):
I suggest to move this logic into a function inside `ext/node/polyfills/internal/net.ts` and we can export it from there

Code after:

        break;
      }
      default: {
        unreachable();
      }
    }

    super(provider, conn);

    // TODO(cmorten): the handling of new connections and construction feels
    // a little off. Suspect duplicating in some fashion.
    if (conn && provider === providerType.TCPWRAP) {
      const localAddr = conn.localAddr as Deno.NetAddr;
      this.#address = localAddr.hostname;
      this.#port = localAddr.port;

      const remoteAddr = conn.remoteAddr as Deno.NetAddr;
      this.#remoteAddress = remoteAddr.hostname;
      this.#remotePort = remoteAddr.port;
      this.#remoteFamily = getIPFamily(remoteAddr.hostname);
    }
  }

  get fd() {
    return this[kStreamBaseField]?.[internalFdSymbol];
  }

  /**
   * Opens a file descriptor.
   * @param fd The file descriptor to open.
   * @return An error status code.
   */
  open(_fd: number): number {
    // REF: https://github.com/denoland/deno/issues/6529
    notImplemented("TCP.prototype.open");
  }

  /**
   * Bind to an IPv4 address.
   * @param address The hostname to bind to.
   * @param port The port to bind to
   * @return An error status code.
   */
  bind(address: string, port: number): number {
    return this.#bind(address, port, 0);
  }

  /**
   * Bind to an IPv6 address.
   * @param address The hostname to bind to.
Repository: denoland/deno
File: cli/module_loader.rs (Rust)

Code before:
  fn load_external_source_map(
    &self,
    source_map_url: &str,
  ) -> Option<Cow<'_, [u8]>> {
    let specifier = resolve_url(source_map_url).ok()?;

    if let Ok(Some(file)) = self
      .0
      .shared
      .file_fetcher
      .get_cached_source_or_local(&specifier)
    {
      return Some(Cow::Owned(file.source.to_vec()));
    }

    None
  }

  fn source_map_source_exists(&self, source_url: &str) -> Option<bool> {
    let specifier = resolve_url(source_url).ok()?;

    // For file:// URLs, check if the file exists on disk
    if specifier.scheme() == "file"
      && let Ok(path) = specifier.to_file_path()
    {
      return Some(path.exists());
    }

    // For other URLs, check if it's in the module graph
    let graph = self.0.graph_container.graph();
    Some(graph.get(&specifier).is_some())
  }

  fn get_source_mapped_source_line(
    &self,
    file_name: &str,
    line_number: usize,
  ) -> Option<String> {
    let specifier = resolve_url(file_name).ok()?;
    let graph = self.0.graph_container.graph();

    let code = match graph.get(&specifier) {
      Some(deno_graph::Module::Js(module)) => &module.source.text,
      Some(deno_graph::Module::Json(module)) => &module.source.text,
      Some(
        deno_graph::Module::Wasm(_)
        | deno_graph::Module::Npm(_)
        | deno_graph::Module::Node(_)
        | deno_graph::Module::External(_),
      ) => {
        return None;

Review comment (security):
I'm somewhat lost here... I thought in the original PR we did permission checks for loading source maps? This unconditionally checks disk for a file path, @dsherret what do you think should happen here?

Code after:
  fn load_external_source_map(
    &self,
    source_map_url: &str,
  ) -> Option<Cow<'_, [u8]>> {
    let specifier = resolve_url(source_map_url).ok()?;

    if let Ok(Some(file)) = self
      .0
      .shared
      .file_fetcher
      .get_cached_source_or_local(&specifier)
    {
      return Some(Cow::Owned(file.source.to_vec()));
    }

    None
  }

  fn source_map_source_exists(&self, source_url: &str) -> Option<bool> {
    let specifier = resolve_url(source_url).ok()?;

    // For npm packages, we can check the file system directly
    if self.0.shared.in_npm_pkg_checker.in_npm_package(&specifier)
      && let Ok(path) = deno_path_util::url_to_file_path(&specifier)
    {
      return Some(path.is_file());
    }

    None
  }

  fn get_source_mapped_source_line(
    &self,
    file_name: &str,
    line_number: usize,
  ) -> Option<String> {
    let specifier = resolve_url(file_name).ok()?;
    let graph = self.0.graph_container.graph();

    let code = match graph.get(&specifier) {
      Some(deno_graph::Module::Js(module)) => &module.source.text,
      Some(deno_graph::Module::Json(module)) => &module.source.text,
      Some(
        deno_graph::Module::Wasm(_)
        | deno_graph::Module::Npm(_)
        | deno_graph::Module::Node(_)
        | deno_graph::Module::External(_),
      ) => {
        return None;
      }
      None => {
Repository: denoland/deno
File: cli/module_loader.rs (Rust)

Code before:
  }

  fn load_external_source_map(
    &self,
    source_map_url: &str,
  ) -> Option<Cow<'_, [u8]>> {
    let specifier = resolve_url(source_map_url).ok()?;

    if let Ok(Some(file)) = self
      .0
      .shared
      .file_fetcher
      .get_cached_source_or_local(&specifier)
    {
      return Some(Cow::Owned(file.source.to_vec()));
    }

    None
  }

  fn source_map_source_exists(&self, source_url: &str) -> Option<bool> {
    let specifier = resolve_url(source_url).ok()?;

    // For file:// URLs, check if the file exists on disk
    if specifier.scheme() == "file"
      && let Ok(path) = specifier.to_file_path()
    {
      return Some(path.exists());
    }

    // For other URLs, check if it's in the module graph
    let graph = self.0.graph_container.graph();
    Some(graph.get(&specifier).is_some())
  }

  fn get_source_mapped_source_line(
    &self,
    file_name: &str,
    line_number: usize,
  ) -> Option<String> {
    let specifier = resolve_url(file_name).ok()?;
    let graph = self.0.graph_container.graph();

    let code = match graph.get(&specifier) {
      Some(deno_graph::Module::Js(module)) => &module.source.text,
      Some(deno_graph::Module::Json(module)) => &module.source.text,
      Some(
        deno_graph::Module::Wasm(_)
        | deno_graph::Module::Npm(_)
        | deno_graph::Module::Node(_)
        | deno_graph::Module::External(_),

Review comment (suggestion):
```suggestion
    if let Ok(path) = deno_path_util::url_to_file_path(specifier)
```

Code after:
  }

  fn load_external_source_map(
    &self,
    source_map_url: &str,
  ) -> Option<Cow<'_, [u8]>> {
    let specifier = resolve_url(source_map_url).ok()?;

    if let Ok(Some(file)) = self
      .0
      .shared
      .file_fetcher
      .get_cached_source_or_local(&specifier)
    {
      return Some(Cow::Owned(file.source.to_vec()));
    }

    None
  }

  fn source_map_source_exists(&self, source_url: &str) -> Option<bool> {
    let specifier = resolve_url(source_url).ok()?;

    // For npm packages, we can check the file system directly
    if self.0.shared.in_npm_pkg_checker.in_npm_package(&specifier)
      && let Ok(path) = deno_path_util::url_to_file_path(&specifier)
    {
      return Some(path.is_file());
    }

    None
  }

  fn get_source_mapped_source_line(
    &self,
    file_name: &str,
    line_number: usize,
  ) -> Option<String> {
    let specifier = resolve_url(file_name).ok()?;
    let graph = self.0.graph_container.graph();

    let code = match graph.get(&specifier) {
      Some(deno_graph::Module::Js(module)) => &module.source.text,
      Some(deno_graph::Module::Json(module)) => &module.source.text,
      Some(
        deno_graph::Module::Wasm(_)
        | deno_graph::Module::Npm(_)
        | deno_graph::Module::Node(_)
        | deno_graph::Module::External(_),
      ) => {
        return None;
Repository: denoland/deno
File: ext/node/polyfills/tls.ts (TypeScript)

Code before:
    ensureLazyRootCertificates(target);
    return ReflectPreventExtensions(target);
  },
  setPrototypeOf() {
    return false;
  },
});

export const DEFAULT_ECDH_CURVE = "auto";
export const DEFAULT_MAX_VERSION = "TLSv1.3";
export const DEFAULT_MIN_VERSION = "TLSv1.2";
export const CLIENT_RENEG_LIMIT = 3;
export const CLIENT_RENEG_WINDOW = 600;

export class CryptoStream {}
export class SecurePair {}
export const Server = tlsWrap.Server;

export function setDefaultCACertificates(certs: string[]) {
  if (!ArrayIsArray(certs)) {
    throw new TypeError(
      "The argument 'certs' must be an array of strings",
    );
  }

  for (const cert of new SafeArrayIterator(certs)) {
    if (typeof cert !== "string") {
      throw new TypeError(
        "Each certificate in 'certs' must be a string",
      );
    }
  }

  op_set_default_ca_certificates(certs);

  lazyRootCertificates = null;
}

export function createSecurePair() {
  notImplemented("tls.createSecurePair");
}

export default {
  CryptoStream,
  SecurePair,
  Server,
  TLSSocket: tlsWrap.TLSSocket,
  checkServerIdentity: tlsWrap.checkServerIdentity,
  connect: tlsWrap.connect,
  createSecureContext: tlsCommon.createSecureContext,
  createSecurePair,

Review comment (performance):
A regular for loop has better performance.

```suggestion
  for (let i = 0; i < certs.length; ++i) {
    const cert = certs[i];
```


Code after:
    return ReflectPreventExtensions(target);
  },
  setPrototypeOf() {
    return false;
  },
});

export const DEFAULT_ECDH_CURVE = "auto";
export const DEFAULT_MAX_VERSION = "TLSv1.3";
export const DEFAULT_MIN_VERSION = "TLSv1.2";
export const CLIENT_RENEG_LIMIT = 3;
export const CLIENT_RENEG_WINDOW = 600;

export class CryptoStream {}
export class SecurePair {}
export const Server = tlsWrap.Server;

export function setDefaultCACertificates(certs: string[]) {
  if (!ArrayIsArray(certs)) {
    throw new TypeError(
      "The argument 'certs' must be an array of strings",
    );
  }

  for (let i = 0; i < certs.length; ++i) {
    const cert = certs[i];
    if (typeof cert !== "string") {
      throw new TypeError(
        "Each certificate in 'certs' must be a string",
      );
    }
  }

  op_set_default_ca_certificates(certs);

  lazyRootCertificates = null;
}

export function createSecurePair() {
  notImplemented("tls.createSecurePair");
}

export default {
  CryptoStream,
  SecurePair,
  Server,
  TLSSocket: tlsWrap.TLSSocket,
  checkServerIdentity: tlsWrap.checkServerIdentity,
  connect: tlsWrap.connect,
  createSecureContext: tlsCommon.createSecureContext,
  createSecurePair,
Repository: denoland/deno
File: ext/tls/keylog.rs (Rust)

Code before:
//! are written to the specified file in NSS Key Log format, which can be
//! used by tools like Wireshark to decrypt TLS traffic.

use std::env;
use std::fs::OpenOptions;
use std::sync::Arc;
use std::sync::OnceLock;

use rustls::KeyLog;
use rustls::KeyLogFile;

static SSL_KEY_LOG: OnceLock<Arc<dyn KeyLog + Send + Sync>> = OnceLock::new();

pub fn get_ssl_key_log() -> Arc<dyn KeyLog> {
  SSL_KEY_LOG
    .get_or_init(|| {
      if let Some(path) = env::var_os("SSLKEYLOGFILE")
        && let Err(e) = OpenOptions::new().append(true).create(true).open(&path)
      {
        log::warn!(
          "SSLKEYLOGFILE is set but the file could not be opened: {e}"
        );
      }
      Arc::new(KeyLogFile::new())
    })
    .clone()
}


Review comment (suggestion):
This env var should be resolved on startup, otherwise someone might use `Deno.env.set(...)` before starting a TLS connection to capture the key

Code after:
//! are written to the specified file in NSS Key Log format, which can be
//! used by tools like Wireshark to decrypt TLS traffic.

use std::env;
use std::fs::OpenOptions;
use std::sync::Arc;
use std::sync::OnceLock;

use rustls::KeyLog;
use rustls::KeyLogFile;

static SSL_KEY_LOG: OnceLock<Arc<dyn KeyLog + Send + Sync>> = OnceLock::new();

/// Eagerly initializes the global TLS key logger.
///
/// `KeyLogFile::new()` reads `SSLKEYLOGFILE` from the process environment.
/// Callers can use this to snapshot that value early, instead of on first TLS
/// config construction.
pub(super) fn init_ssl_key_log() {
  let _ = get_ssl_key_log();
}

pub fn get_ssl_key_log() -> Arc<dyn KeyLog> {
  SSL_KEY_LOG
    .get_or_init(|| {
      if let Some(path) = env::var_os("SSLKEYLOGFILE")
        && let Err(e) = OpenOptions::new().append(true).create(true).open(&path)
      {
        log::warn!(
          "SSLKEYLOGFILE is set but '{}' could not be opened: {e}",
          path.to_string_lossy()
        );
      }
      Arc::new(KeyLogFile::new())
    })
    .clone()
}
Repository: denoland/deno
File: cli/tools/x.rs (Rust)

Code before:
          all_bins.insert(command.clone(), bin_value.clone());
        }
      }
      Ok(all_bins)
    }
  }
}

fn run_js_file(
  main_module: &deno_core::url::Url,
  argv: &[String],
  npm_process_state: Option<String>,
  npm: bool,
) -> Result<i32, AnyError> {
  use deno_runtime::deno_io::FromRawIoHandle;

  let deno_exe = std::env::current_exe()
    .and_then(|p| p.canonicalize())
    .context("Failed to get current executable path")?;

  let mut args: Vec<std::ffi::OsString> =
    vec!["run".into(), "-A".into(), main_module.as_str().into()];
  args.extend(argv.iter().map(|s| s.into()));

  let mut command = std::process::Command::new(deno_exe);
  command.args(&args);

  let _temp_file = if let Some(state) = &npm_process_state {
    let fd =
      deno_runtime::deno_process::npm_process_state_tempfile(state.as_bytes())
        .context("Failed to create npm process state tempfile")?;
    command.env(
      deno_runtime::deno_process::NPM_RESOLUTION_STATE_FD_ENV_VAR_NAME,
      (fd as usize).to_string(),
    );
    // Keep the file alive until the subprocess completes
    // SAFETY: fd is valid
    Some(unsafe { std::fs::File::from_raw_io_handle(fd) })
  } else {
    None
  };

  if npm {
    crate::tools::run::set_npm_user_agent();
    if let Some(agent) = std::env::var_os("NPM_CONFIG_USER_AGENT") {
      command.env("NPM_CONFIG_USER_AGENT", agent);
    }
  }

  let mut child = command.spawn().context("Failed to spawn deno subprocess")?;
  let status = child.wait().context("Failed to wait for deno subprocess")?;

Review comment (suggestion):
Is it on purpose using `-A` here? What is someone passes different arg set? If I'm misunderstanding a comment here would be helpful

Code after:
          all_bins.insert(command.clone(), bin_value.clone());
        }
      }
      Ok(all_bins)
    }
  }
}

fn run_js_file(
  main_module: &deno_core::url::Url,
  permission_args: &[String],
  argv: &[String],
  npm_process_state: Option<String>,
  npm: bool,
) -> Result<i32, AnyError> {
  use deno_runtime::deno_io::FromRawIoHandle;

  let deno_exe = std::env::current_exe()
    .and_then(|p| p.canonicalize())
    .context("Failed to get current executable path")?;

  let mut args: Vec<std::ffi::OsString> = vec!["run".into()];
  args.extend(permission_args.iter().map(|s| s.into()));
  args.push(main_module.as_str().into());
  args.extend(argv.iter().map(|s| s.into()));

  let mut command = std::process::Command::new(deno_exe);
  command.args(&args);

  let _temp_file = if let Some(state) = &npm_process_state {
    let fd =
      deno_runtime::deno_process::npm_process_state_tempfile(state.as_bytes())
        .context("Failed to create npm process state tempfile")?;
    command.env(
      deno_runtime::deno_process::NPM_RESOLUTION_STATE_FD_ENV_VAR_NAME,
      (fd as usize).to_string(),
    );
    // Keep the file alive until the subprocess completes
    // SAFETY: fd is valid
    Some(unsafe { std::fs::File::from_raw_io_handle(fd) })
  } else {
    None
  };

  if npm {
    crate::tools::run::set_npm_user_agent();
  }

  #[cfg(unix)]
  {
    use std::os::unix::process::CommandExt;
Repository: denoland/deno
File: tests/unit_node/_fs/_fs_readFile_test.ts (TypeScript)

Code before:

Deno.test(
  "fs.readFile throws ERR_INVALID_ARG_TYPE when path is undefined (e.g. fd.name)",
  async () => {
    const { open } = await import("node:fs/promises");
    const fh = await open(testData, "r");

    try {
      // Simulate user code that passes `fd.name` (which is undefined) into readFile
      const name = (fh as { name?: unknown }).name;

      let threw = false;
      try {
        // readFile should throw synchronously for invalid path types
        // we cast through unknown to avoid using `any` while still passing
        // an invalid value (undefined) into the API
        readFile(name as unknown as string, () => {});
      } catch (err) {
        threw = true;
        // Ensure it's the node-style error
        assert(err instanceof Error);
        // The internal error class sets the code to ERR_INVALID_ARG_TYPE
        // deno-lint-ignore no-explicit-any
        assertEquals((err as any).code, "ERR_INVALID_ARG_TYPE");
      }
      assert(threw, "readFile did not throw for undefined path");

      // Also check sync variant
      assertThrows(() => readFileSync(name as unknown as string), TypeError);
    } finally {
      await fh.close();
    }
  },
);

Deno.test("fs.readFileSync creates new file when passed 'w+' flag", () => {
  const tmpDir = Deno.makeTempDirSync();
  const filePath = path.join(tmpDir, "newfile.txt");
  readFileSync(filePath, { flag: "w+" });

  assert(existsSync(filePath));
  Deno.removeSync(tmpDir, { recursive: true });
});


Review comment (suggestion):
I'd say to replace this and use `readFile` from `node:fs/promises` instead. Then you could check the error thrown using `assertRejects()` similar to what you did on the sync version.

Code after:
      res(data);
    });
  });

  assert(existsSync(filePath));
  Deno.removeSync(tmpDir, { recursive: true });
});

Deno.test(
  "fs.readFile throws ERR_INVALID_ARG_TYPE when path is undefined (e.g. fd.name)",
  async () => {
    const fh = await open(testData, "r");

    try {
      // Simulate user code that passes `fd.name` (which is undefined) into readFile
      const name = (fh as { name?: unknown }).name;

      // Check async (promises) variant rejects with ERR_INVALID_ARG_TYPE
      const err = await assertRejects(async () => {
        await readFilePromise(name as unknown as string);
      });
      const errObj = err as { code?: string };
      assertEquals(errObj.code, "ERR_INVALID_ARG_TYPE");

      // Also check sync variant throws the proper TypeError
      assertThrows(() => readFileSync(name as unknown as string), TypeError);
    } finally {
      await fh.close();
    }
  },
);

Deno.test("fs.readFileSync creates new file when passed 'w+' flag", () => {
  const tmpDir = Deno.makeTempDirSync();
  const filePath = path.join(tmpDir, "newfile.txt");
  readFileSync(filePath, { flag: "w+" });

  assert(existsSync(filePath));
  Deno.removeSync(tmpDir, { recursive: true });
});
Repository: denoland/deno
File: cli/tools/installer/mod.rs (Rust)

Code before:
    return Err(anyhow!(
      "Existing installation found. Aborting (Use -f to overwrite).",
    ));
  }

  let compile_flags = CompileFlags {
    source_file,
    output: Some(output.clone()),
    args: install_flags_global.args,
    target: None,
    no_terminal: false,
    icon: None,
    include: vec![],
    exclude: vec![],
    eszip: false,
  };

  let mut new_flags = flags.as_ref().clone();
  new_flags.subcommand =
    DenoSubcommand::Compile(compile_flags.clone());
  // `is_package_manager_subcommand` forces managed resolver for deno install
  // but we loose that with DenoSubcommand::Command. Explicitly set that here.
  if new_flags.node_modules_dir.is_none() {
    new_flags.node_modules_dir =
      Some(deno_config::deno_json::NodeModulesDirMode::Auto);
  }

  crate::tools::compile::compile(Arc::new(new_flags), compile_flags).await?;

  log::info!("Successfully installed {}", output);

  if !is_in_path(&install_dir) {
    let installation_dir_str = install_dir.to_string_lossy();
    log::info!("Add {} to PATH", installation_dir_str);
    if cfg!(windows) {
      log::info!("    set PATH=%PATH%;{}", installation_dir_str);
    } else {
      log::info!("    export PATH=\"{}:$PATH\"", installation_dir_str);
    }
  }

  Ok(())
}

async fn create_install_shim(
  bin_name_resolver: &BinNameResolver<'_>,
  cwd: &Path,
  flags: &Flags,
  install_flags_global: &InstallFlagsGlobal,
  module_url: &str,
) -> Result<(), AnyError> {

Review comment (suggestion):
cc @dsherret Is this correct?

I get `error: Resolving npm specifier entrypoints this way is currently not supported with "nodeModules": "manual". In the meantime, try with --node-modules-dir=auto instead` before this change

Code after:
  if output_path.is_file() && !install_flags_global.force {
    return Err(anyhow!(
      "Existing installation found. Aborting (Use -f to overwrite).",
    ));
  }

  let compile_flags = CompileFlags {
    source_file,
    output: Some(output.clone()),
    args: install_flags_global.args,
    target: None,
    no_terminal: false,
    icon: None,
    include: vec![],
    exclude: vec![],
    eszip: false,
  };

  let mut new_flags = flags.as_ref().clone();
  new_flags.subcommand = DenoSubcommand::Compile(compile_flags.clone());

  crate::tools::compile::compile(new_flags, compile_flags).await?;

  log::info!("Successfully installed {}", output);

  if !is_in_path(&install_dir) {
    let installation_dir_str = install_dir.to_string_lossy();
    log::info!("Add {} to PATH", installation_dir_str);
    if cfg!(windows) {
      log::info!("    set PATH=%PATH%;{}", installation_dir_str);
    } else {
      log::info!("    export PATH=\"{}:$PATH\"", installation_dir_str);
    }
  }

  Ok(())
}

async fn create_install_shim(
  bin_name_resolver: &BinNameResolver<'_>,
  cwd: &Path,
  flags: &Flags,
  install_flags_global: &InstallFlagsGlobal,
  module_url: &str,
) -> Result<(), AnyError> {
  let shim_data = resolve_shim_data(
    bin_name_resolver,
    cwd,
    flags,
    install_flags_global,
    module_url,