ronantakizawa/codereview-bench huggingface dataset:
Repository: denoland/deno
File: cli/tools/test/mod.rs (Rust)

Code before:
  pub sanitize_ops: bool,
  pub sanitize_resources: bool,
}

/// May represent a failure of a test or test step.
#[derive(Debug, Clone, PartialEq, Deserialize, Eq, Hash)]
#[serde(rename_all = "camelCase")]
pub struct TestFailureDescription {
  pub id: usize,
  pub name: String,
  pub origin: String,
  pub location: TestLocation,
}

impl From<&TestDescription> for TestFailureDescription {
  fn from(value: &TestDescription) -> Self {
    Self {
      id: value.id,
      name: value.name.clone(),
      origin: value.origin.clone(),
      location: value.location.clone(),
    }
  }
}

#[derive(Debug, Clone, PartialEq, Deserialize)]
pub struct TestFailureFormatOptions {
  pub hide_traces: bool,
}

#[allow(clippy::derive_partial_eq_without_eq)]
#[derive(Debug, Clone, PartialEq, Deserialize)]
#[serde(rename_all = "camelCase")]
pub enum TestFailure {
  JsError(Box<JsError>),
  FailedSteps(usize),
  IncompleteSteps,
  Leaked(Vec<String>, Vec<String>), // Details, trailer notes
  // The rest are for steps only.
  Incomplete,
  OverlapsWithSanitizers(IndexSet<String>), // Long names of overlapped tests
  HasSanitizersAndOverlaps(IndexSet<String>), // Long names of overlapped tests
}

impl TestFailure {
  pub fn format(&self, options: Option<&TestFailureFormatOptions>) -> String {
    let mut f = String::new();
    let result = match self {
      TestFailure::JsError(js_error) => {
        write!(f, "{}", format_test_error(js_error, options))
      }

Review comment (bug):
```suggestion
#[derive(Debug, Clone, PartialEq)]
```

Not needed?

Code after:
  pub sanitize_resources: bool,
}

/// May represent a failure of a test or test step.
#[derive(Debug, Clone, PartialEq, Deserialize, Eq, Hash)]
#[serde(rename_all = "camelCase")]
pub struct TestFailureDescription {
  pub id: usize,
  pub name: String,
  pub origin: String,
  pub location: TestLocation,
}

impl From<&TestDescription> for TestFailureDescription {
  fn from(value: &TestDescription) -> Self {
    Self {
      id: value.id,
      name: value.name.clone(),
      origin: value.origin.clone(),
      location: value.location.clone(),
    }
  }
}

#[derive(Debug, Default, Clone, PartialEq)]
pub struct TestFailureFormatOptions {
  pub hide_stacktraces: bool,
}

#[allow(clippy::derive_partial_eq_without_eq)]
#[derive(Debug, Clone, PartialEq, Deserialize)]
#[serde(rename_all = "camelCase")]
pub enum TestFailure {
  JsError(Box<JsError>),
  FailedSteps(usize),
  IncompleteSteps,
  Leaked(Vec<String>, Vec<String>), // Details, trailer notes
  // The rest are for steps only.
  Incomplete,
  OverlapsWithSanitizers(IndexSet<String>), // Long names of overlapped tests
  HasSanitizersAndOverlaps(IndexSet<String>), // Long names of overlapped tests
}

impl TestFailure {
  pub fn format(
    &self,
    options: &TestFailureFormatOptions,
  ) -> Cow<'static, str> {
    match self {
      TestFailure::JsError(js_error) => {
        Cow::Owned(format_test_error(js_error, options))
Repository: denoland/deno
File: ext/fs/std_fs.rs (Rust)

Code before:
      != 0
    {
      fsstat.is_symlink = true;
    }

    if file_info.BasicInformation.FileAttributes
      & winapi::um::winnt::FILE_ATTRIBUTE_DIRECTORY
      != 0
    {
      fsstat.mode |= libc::S_IFDIR as u32;
      fsstat.size = 0;
    } else {
      fsstat.mode |= libc::S_IFREG as u32;
      fsstat.size = *file_info.StandardInformation.EndOfFile.QuadPart() as _;
    }

    if file_info.BasicInformation.FileAttributes
      & winapi::um::winnt::FILE_ATTRIBUTE_READONLY
      != 0
    {
      fsstat.mode |=
        (libc::S_IREAD | (libc::S_IREAD >> 3) | (libc::S_IREAD >> 6)) as u32;
    } else {
      fsstat.mode |= ((libc::S_IREAD | libc::S_IWRITE)
        | ((libc::S_IREAD | libc::S_IWRITE) >> 3)
        | ((libc::S_IREAD | libc::S_IWRITE) >> 6)) as u32;
    }

    Ok(())
  }
}

fn realpath(path: &Path) -> FsResult<PathBuf> {
  Ok(deno_core::strip_unc_prefix(path.canonicalize()?))
}

fn read_dir(path: &Path) -> FsResult<Vec<FsDirEntry>> {
  let entries = fs::read_dir(path)?
    .filter_map(|entry| {
      let entry = entry.ok()?;
      let name = entry.file_name().into_string().ok()?;
      let metadata = entry.file_type();
      macro_rules! method_or_false {
        ($method:ident) => {
          if let Ok(metadata) = &metadata {
            metadata.$method()
          } else {
            false
          }
        };
      }

Review comment (performance):
Does all this make everything that uses stat slower? (ex. `Deno.statSync`?) We should probably optimize stuff like `fs.existsSync` to not bother with all this stuff in case it's using it.

Code after:

    Ok(info.dwVolumeSerialNumber as u64)
  }

  use windows_sys::Wdk::Storage::FileSystem::FILE_ALL_INFORMATION;

  unsafe fn query_file_information(
    handle: winapi::shared::ntdef::HANDLE,
  ) -> std::io::Result<FILE_ALL_INFORMATION> {
    use windows_sys::Wdk::Storage::FileSystem::NtQueryInformationFile;

    let mut info = std::mem::MaybeUninit::<FILE_ALL_INFORMATION>::zeroed();
    let status = NtQueryInformationFile(
      handle as _,
      std::ptr::null_mut(),
      info.as_mut_ptr() as *mut _,
      std::mem::size_of::<FILE_ALL_INFORMATION>() as _,
      18, /* FileAllInformation */
    );

    if status < 0 {
      return Err(std::io::Error::last_os_error());
    }

    Ok(info.assume_init())
  }

  // SAFETY: winapi calls
  unsafe {
    let mut path: Vec<_> = path.as_os_str().encode_wide().collect();
    path.push(0);
    let file_handle = CreateFileW(
      path.as_ptr(),
      0,
      FILE_SHARE_READ | FILE_SHARE_DELETE | FILE_SHARE_WRITE,
      std::ptr::null_mut(),
      OPEN_EXISTING,
      file_flags,
      std::ptr::null_mut(),
    );
    if file_handle == INVALID_HANDLE_VALUE {
      return Err(std::io::Error::last_os_error().into());
    }

    let result = get_dev(file_handle);
    CloseHandle(file_handle);
    fsstat.dev = result?;

    if let Ok(file_info) = query_file_information(file_handle) {
      if file_info.BasicInformation.FileAttributes
        & winapi::um::winnt::FILE_ATTRIBUTE_REPARSE_POINT
Repository: denoland/deno
File: cli/tools/serve.rs (Rust)

Code before:
  }

  let factory = CliFactory::from_flags(flags);
  let cli_options = factory.cli_options()?;
  let deno_dir = factory.deno_dir()?;
  let http_client = factory.http_client_provider();

  // Run a background task that checks for available upgrades or output
  // if an earlier run of this background task found a new version of Deno.
  #[cfg(feature = "upgrade")]
  super::upgrade::check_for_upgrades(
    http_client.clone(),
    deno_dir.upgrade_check_file_path(),
  );

  let main_module = cli_options.resolve_main_module()?;

  maybe_npm_install(&factory).await?;

  let worker_factory = factory.create_cli_main_worker_factory().await?;
  let hmr = serve_flags
    .watch
    .unwrap_or(WatchFlagsWithPaths {
      hmr: false,
      ..Default::default()
    })
    .hmr;
  do_serve(
    worker_factory,
    main_module.clone(),
    serve_flags.worker_count,
    hmr,
  )
  .await
}

async fn do_serve(
  worker_factory: CliMainWorkerFactory,
  main_module: ModuleSpecifier,
  worker_count: Option<usize>,
  hmr: bool,
) -> Result<i32, AnyError> {
  let mut worker = worker_factory
    .create_main_worker(
      deno_runtime::WorkerExecutionMode::Serve {
        is_main: true,
        worker_count,
      },
      main_module.clone(),
    )
    .await?;

Review comment (suggestion):
```suggestion
.map(|watch_flags| watch_flags.hmr).unwrap_or(false)
```

Code after:
  }

  let factory = CliFactory::from_flags(flags);
  let cli_options = factory.cli_options()?;
  let deno_dir = factory.deno_dir()?;
  let http_client = factory.http_client_provider();

  // Run a background task that checks for available upgrades or output
  // if an earlier run of this background task found a new version of Deno.
  #[cfg(feature = "upgrade")]
  super::upgrade::check_for_upgrades(
    http_client.clone(),
    deno_dir.upgrade_check_file_path(),
  );

  let main_module = cli_options.resolve_main_module()?;

  maybe_npm_install(&factory).await?;

  let worker_factory = factory.create_cli_main_worker_factory().await?;
  let hmr = serve_flags
    .watch
    .map(|watch_flags| watch_flags.hmr)
    .unwrap_or(false);
  do_serve(
    worker_factory,
    main_module.clone(),
    serve_flags.worker_count,
    hmr,
  )
  .await
}

async fn do_serve(
  worker_factory: CliMainWorkerFactory,
  main_module: ModuleSpecifier,
  worker_count: Option<usize>,
  hmr: bool,
) -> Result<i32, AnyError> {
  let mut worker = worker_factory
    .create_main_worker(
      deno_runtime::WorkerExecutionMode::Serve {
        is_main: true,
        worker_count,
      },
      main_module.clone(),
    )
    .await?;
  let worker_count = match worker_count {
    None | Some(1) => return worker.run().await,
    Some(c) => c,
Repository: denoland/deno
File: cli/download_deno_binary.rs (Rust)

Code before:
// Copyright 2018-2024 the Deno authors. All rights reserved. MIT license.

use std::path::Path;
use std::path::PathBuf;

use crate::http_util::HttpClient;
use crate::http_util::HttpClientProvider;
use crate::util::progress_bar::ProgressBar;
use crate::util::progress_bar::ProgressBarStyle;
use deno_core::anyhow::bail;
use deno_core::error::AnyError;

use crate::cache::DenoDir;
use crate::shared::ReleaseChannel;

pub async fn download_deno_binary(
  http_client_provider: &HttpClientProvider,
  deno_dir: &DenoDir,
  binary_kind: &str,
  target: &str,
  version_or_git_hash: &str,
  release_channel: ReleaseChannel,
) -> Result<PathBuf, AnyError> {
  let binary_name = format!("{}-{}.zip", binary_kind, target);
  let binary_path_suffix = match release_channel {
    ReleaseChannel::Canary => {
      format!("canary/{}/{}", version_or_git_hash, binary_name,)
    }
    _ => {
      format!("release/v{}/{}", version_or_git_hash, binary_name)
    }
  };

  let download_directory = deno_dir.dl_folder_path();
  let binary_path = download_directory.join(&binary_path_suffix);

  if !binary_path.exists() {
    let http_client = http_client_provider.get_or_create()?;
    download_base_binary(
      &http_client,
      &download_directory,
      &binary_path_suffix,
    )
    .await?;
  }

Review comment (suggestion):
Consider making these two enums that have `FromStr` impl

Code after:
// Copyright 2018-2024 the Deno authors. All rights reserved. MIT license.

use std::path::Path;
use std::path::PathBuf;

use crate::http_util::HttpClient;
use crate::http_util::HttpClientProvider;
use crate::util::progress_bar::ProgressBar;
use crate::util::progress_bar::ProgressBarStyle;
use deno_core::anyhow::bail;
use deno_core::error::AnyError;

use crate::cache::DenoDir;
use crate::shared::ReleaseChannel;

#[derive(Clone, Copy, Debug)]
pub enum BinaryKind {
  Deno,
  Denort,
}

impl BinaryKind {
  pub fn name(&self) -> &str {
    match self {
      BinaryKind::Deno => "deno",
      BinaryKind::Denort => "denort",
    }
  }
}

pub async fn download_deno_binary(
  http_client_provider: &HttpClientProvider,
  deno_dir: &DenoDir,
  binary_kind: BinaryKind,
  target: &str,
  version_or_git_hash: &str,
  release_channel: ReleaseChannel,
) -> Result<PathBuf, AnyError> {
  let binary_name = archive_name(binary_kind, target);
  let binary_path_suffix = match release_channel {
    ReleaseChannel::Canary => {
      format!("canary/{}/{}", version_or_git_hash, binary_name,)
    }
    _ => {
      format!("release/v{}/{}", version_or_git_hash, binary_name)
Repository: denoland/deno
File: cli/standalone/binary.rs (Rust)

Code before:
      eszip,
      root_dir_url,
      entrypoint,
      cli_options,
      compile_flags,
    )
  }

  async fn get_base_binary(
    &self,
    compile_flags: &CompileFlags,
  ) -> Result<Vec<u8>, AnyError> {
    // Used for testing.
    //
    // Phase 2 of the 'min sized' deno compile RFC talks
    // about adding this as a flag.
    if let Some(path) = std::env::var_os("DENORT_BIN") {
      return std::fs::read(&path).with_context(|| {
        format!("Could not find denort at '{}'", path.to_string_lossy())
      });
    }

    let target = compile_flags.resolve_target();

    let exe_name = "denort";
    let archive_name = format!("{exe_name}-{target}.zip");

    let binary_path = download_deno_binary(
      self.http_client_provider,
      self.deno_dir,
      exe_name,
      &target,
      crate::version::DENO_VERSION_INFO.version_or_git_hash(),
      crate::version::DENO_VERSION_INFO.release_channel,
    )
    .await?;

    let archive_data = std::fs::read(binary_path)?;
    let temp_dir = tempfile::TempDir::new()?;
    let base_binary_path = archive::unpack_into_dir(archive::UnpackArgs {
      exe_name,
      archive_name: &archive_name,
      archive_data: &archive_data,
      is_windows: target.contains("windows"),
      dest_path: temp_dir.path(),
    })?;
    let base_binary = std::fs::read(base_binary_path)?;
    drop(temp_dir); // delete the temp dir
    Ok(base_binary)
  }


Review comment (refactor):
Maybe this could be returned from a helper function in `download_deno_binary` module so we don't have this computed all over the place?

Code after:
  }

  pub async fn write_bin(
    &self,
    writer: File,
    graph: &ModuleGraph,
    root_dir_url: StandaloneRelativeFileBaseUrl<'_>,
    entrypoint: &ModuleSpecifier,
    compile_flags: &CompileFlags,
    cli_options: &CliOptions,
  ) -> Result<(), AnyError> {
    // Select base binary based on target
    let mut original_binary = self.get_base_binary(compile_flags).await?;

    if compile_flags.no_terminal {
      let target = compile_flags.resolve_target();
      if !target.contains("windows") {
        bail!(
          "The `--no-terminal` flag is only available when targeting Windows (current: {})",
          target,
        )
      }
      set_windows_binary_to_gui(&mut original_binary)?;
    }
    if compile_flags.icon.is_some() {
      let target = compile_flags.resolve_target();
      if !target.contains("windows") {
        bail!(
          "The `--icon` flag is only available when targeting Windows (current: {})",
          target,
        )
      }
    }
    self
      .write_standalone_binary(
        writer,
        original_binary,
        graph,
        root_dir_url,
        entrypoint,
        cli_options,
        compile_flags,
      )
      .await
  }

  async fn get_base_binary(
    &self,
    compile_flags: &CompileFlags,
  ) -> Result<Vec<u8>, AnyError> {
    // Used for testing.
Repository: denoland/deno
File: cli/tools/info.rs (Rust)

Code before:
      colors::bold("Remote modules cache:"),
      modules_cache.display()
    );
    println!(
      "{} {}",
      colors::bold("npm modules cache:"),
      npm_cache.display()
    );
    println!(
      "{} {}",
      colors::bold("Emitted modules cache:"),
      typescript_cache.display()
    );
    println!(
      "{} {}",
      colors::bold("Language server registries cache:"),
      registry_cache.display(),
    );
    println!(
      "{} {}",
      colors::bold("Origin storage:"),
      origin_dir.display()
    );
    println!(
      "{} {}",
      colors::bold("Web Cache storage:"),
      web_cache_dir.display()
    );
    if location.is_some() {
      println!(
        "{} {}",
        colors::bold("Local Storage:"),
        local_storage_dir.display(),
      );
    }
    Ok(())
  }
}

fn add_npm_packages_to_json(
  json: &mut serde_json::Value,
  npm_resolver: &dyn CliNpmResolver,
  npmrc: &ResolvedNpmRc,
) {
  let Some(npm_resolver) = npm_resolver.as_managed() else {
    return; // does not include byonm to deno info's output
  };

  // ideally deno_graph could handle this, but for now we just modify the json here
  let snapshot = npm_resolver.snapshot();
  let json = json.as_object_mut().unwrap();

Review comment (performance):
```suggestion
      colors::bold("Web cache storage:"),
```

To be like the others?

Code after:
      colors::bold("Remote modules cache:"),
      modules_cache.display()
    );
    println!(
      "{} {}",
      colors::bold("npm modules cache:"),
      npm_cache.display()
    );
    println!(
      "{} {}",
      colors::bold("Emitted modules cache:"),
      typescript_cache.display()
    );
    println!(
      "{} {}",
      colors::bold("Language server registries cache:"),
      registry_cache.display(),
    );
    println!(
      "{} {}",
      colors::bold("Origin storage:"),
      origin_dir.display()
    );
    println!(
      "{} {}",
      colors::bold("Web cache storage:"),
      web_cache_dir.display()
    );
    if location.is_some() {
      println!(
        "{} {}",
        colors::bold("Local Storage:"),
        local_storage_dir.display(),
      );
    }
    Ok(())
  }
}

fn add_npm_packages_to_json(
  json: &mut serde_json::Value,
  npm_resolver: &dyn CliNpmResolver,
  npmrc: &ResolvedNpmRc,
) {
  let Some(npm_resolver) = npm_resolver.as_managed() else {
    return; // does not include byonm to deno info's output
  };

  // ideally deno_graph could handle this, but for now we just modify the json here
  let snapshot = npm_resolver.snapshot();
  let json = json.as_object_mut().unwrap();
Repository: denoland/deno
File: runtime/worker.rs (Rust)

Code before:
use crate::BootstrapOptions;

pub type FormatJsErrorFn = dyn Fn(&JsError) -> String + Sync + Send;

pub fn import_meta_resolve_callback(
  loader: &dyn deno_core::ModuleLoader,
  specifier: String,
  referrer: String,
) -> Result<ModuleSpecifier, AnyError> {
  loader.resolve(
    &specifier,
    &referrer,
    deno_core::ResolutionKind::DynamicImport,
  )
}

// TODO(bartlomieju): temporary measurement until we start supporting more
// module types
pub fn validate_import_attributes_callback(
  scope: &mut v8::HandleScope,
  attributes: &HashMap<String, String>,
) {
  for (key, value) in attributes {
    let msg = if key != "type" {
      Some(format!("\"{key}\" attribute is not supported."))
    } else if value != "json" {
      Some(format!("\"{value}\" is not a valid module type."))
    } else {
      None
    };

    let Some(msg) = msg else {
      continue;
    };

    let message = v8::String::new(scope, &msg).unwrap();
    let exception = v8::Exception::type_error(scope, message);
    scope.throw_exception(exception);
    return;
  }
}

#[derive(Clone, Default)]
pub struct ExitCode(Arc<AtomicI32>);

impl ExitCode {
  pub fn get(&self) -> i32 {
    self.0.load(Relaxed)
  }

  pub fn set(&mut self, code: i32) {

Review comment (suggestion):
I probably should have exported that string in `deno_core` or handled the validation there. 

Code after:
use crate::BootstrapOptions;

pub type FormatJsErrorFn = dyn Fn(&JsError) -> String + Sync + Send;

pub fn import_meta_resolve_callback(
  loader: &dyn deno_core::ModuleLoader,
  specifier: String,
  referrer: String,
) -> Result<ModuleSpecifier, AnyError> {
  loader.resolve(
    &specifier,
    &referrer,
    deno_core::ResolutionKind::DynamicImport,
  )
}

// TODO(bartlomieju): temporary measurement until we start supporting more
// module types
pub fn validate_import_attributes_callback(
  scope: &mut v8::HandleScope,
  attributes: &HashMap<String, String>,
) {
  for (key, value) in attributes {
    let msg = if key != "type" {
      Some(format!("\"{key}\" attribute is not supported."))
    } else if value != "json" && value != "$$deno-core-internal-wasm-module" {
      Some(format!("\"{value}\" is not a valid module type."))
    } else {
      None
    };

    let Some(msg) = msg else {
      continue;
    };

    let message = v8::String::new(scope, &msg).unwrap();
    let exception = v8::Exception::type_error(scope, message);
    scope.throw_exception(exception);
    return;
  }
}

#[derive(Clone, Default)]
pub struct ExitCode(Arc<AtomicI32>);

impl ExitCode {
  pub fn get(&self) -> i32 {
    self.0.load(Relaxed)
  }

  pub fn set(&mut self, code: i32) {
Repository: denoland/deno
File: ext/node/polyfills/timers.ts (TypeScript)

Code before:
  if (timer) {
    timeout._destroyed = true;
    MapPrototypeDelete(activeTimers, id);
  }
  clearInterval_(id);
}
export function setImmediate(
  cb: (...args: unknown[]) => void,
  ...args: unknown[]
): Timeout {
  return new Immediate(cb, ...new SafeArrayIterator(args));
}
export function clearImmediate(immediate: Immediate) {
  if (immediate == null) {
    return;
  }

  // FIXME(nathanwhit): will probably change once
  //  deno_core has proper support for immediates
  clearTimeout_(immediate._immediateId);
}

async function* setIntervalAsync(
  after: number,
  value: number,
  options: { signal?: AbortSignal; ref?: boolean } = {},
) {
  validateObject(options, "options");

  if (typeof options?.signal !== "undefined") {
    validateAbortSignal(options.signal, "options.signal");
  }

  if (typeof options?.ref !== "undefined") {
    validateBoolean(options.ref, "options.ref");
  }

  const { signal, ref = true } = options;

  if (signal?.aborted) {
    throw new AbortError(undefined, { cause: signal?.reason });
  }

  let onCancel: (() => void) | undefined = undefined;
  let interval: Timeout | undefined = undefined;
  try {
    let notYielded = 0;
    let callback: ((value?: object) => void) | undefined = undefined;
    let rejectCallback: ((message?: string) => void) | undefined = undefined;
    interval = new Timeout(
      () => {

Review comment (suggestion):
```suggestion
  options: { signal?: AbortSignal; ref?: boolean } = { __proto__: null },
```

Code after:
  if (timer) {
    timeout._destroyed = true;
    MapPrototypeDelete(activeTimers, id);
  }
  clearInterval_(id);
}
export function setImmediate(
  cb: (...args: unknown[]) => void,
  ...args: unknown[]
): Timeout {
  return new Immediate(cb, ...new SafeArrayIterator(args));
}
export function clearImmediate(immediate: Immediate) {
  if (immediate == null) {
    return;
  }

  // FIXME(nathanwhit): will probably change once
  //  deno_core has proper support for immediates
  clearTimeout_(immediate._immediateId);
}

async function* setIntervalAsync(
  after: number,
  value: number,
  options: { signal?: AbortSignal; ref?: boolean } = { __proto__: null },
) {
  validateObject(options, "options");

  if (typeof options?.signal !== "undefined") {
    validateAbortSignal(options.signal, "options.signal");
  }

  if (typeof options?.ref !== "undefined") {
    validateBoolean(options.ref, "options.ref");
  }

  const { signal, ref = true } = options;

  if (signal?.aborted) {
    throw new AbortError(undefined, { cause: signal?.reason });
  }

  let onCancel: (() => void) | undefined = undefined;
  let interval: Timeout | undefined = undefined;
  try {
    let notYielded = 0;
    let callback: ((value?: object) => void) | undefined = undefined;
    let rejectCallback: ((message?: string) => void) | undefined = undefined;
    interval = new Timeout(
      () => {