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

Code before:

import { denoErrorToNodeError } from "ext:deno_node/internal/errors.ts";
import {
  BigIntStats,
  CFISBIS,
  statCallback,
  statCallbackBigInt,
  statOptions,
  Stats,
} from "ext:deno_node/_fs/_fs_stat.ts";
import { promisify } from "ext:deno_node/internal/util.mjs";
import { primordials } from "ext:core/mod.js";
import { Buffer } from "ext:deno_node/internal/buffer.mjs";

const {
  Error,
  PromisePrototypeThen,
  ObjectPrototypeIsPrototypeOf,
} = primordials;

function toPathString(path: string | URL | Buffer): string | URL {
  // deno-lint-ignore prefer-primordials
  if (Buffer.isBuffer(path)) return path.toString();
  return path;
}

export function lstat(path: string | URL, callback: statCallback): void;
export function lstat(
  path: string | URL,
  options: { bigint: false },
  callback: statCallback,
): void;
export function lstat(
  path: string | URL,
  options: { bigint: true },
  callback: statCallbackBigInt,
): void;
export function lstat(
  path: string | URL,
  optionsOrCallback: statCallback | statCallbackBigInt | statOptions,
  maybeCallback?: statCallback | statCallbackBigInt,
) {
  const callback =
    (typeof optionsOrCallback === "function"
      ? optionsOrCallback
      : maybeCallback) as (
        ...args: [Error] | [null, BigIntStats | Stats]
      ) => void;
  const options = typeof optionsOrCallback === "object"
    ? optionsOrCallback
    : { bigint: false };

Review comment (suggestion):
We should use `getValidatedPathToString` from `ext:deno_node/internal/fs/utils.mjs` instead. It will handle invalid path types by giving the same error as node

Code after:

import { denoErrorToNodeError } from "ext:deno_node/internal/errors.ts";
import {
  BigIntStats,
  CFISBIS,
  statCallback,
  statCallbackBigInt,
  statOptions,
  Stats,
} from "ext:deno_node/_fs/_fs_stat.ts";
import { getValidatedPathToString } from "ext:deno_node/internal/fs/utils.mjs";
import { promisify } from "ext:deno_node/internal/util.mjs";
import { primordials } from "ext:core/mod.js";

const {
  Error,
  PromisePrototypeThen,
  ObjectPrototypeIsPrototypeOf,
} = primordials;

export function lstat(path: string | URL, callback: statCallback): void;
export function lstat(
  path: string | URL,
  options: { bigint: false },
  callback: statCallback,
): void;
export function lstat(
  path: string | URL,
  options: { bigint: true },
  callback: statCallbackBigInt,
): void;
export function lstat(
  path: string | URL,
  optionsOrCallback: statCallback | statCallbackBigInt | statOptions,
  maybeCallback?: statCallback | statCallbackBigInt,
) {
  const callback =
    (typeof optionsOrCallback === "function"
      ? optionsOrCallback
      : maybeCallback) as (
        ...args: [Error] | [null, BigIntStats | Stats]
      ) => void;
  const options = typeof optionsOrCallback === "object"
    ? optionsOrCallback
    : { bigint: false };

  if (!callback) throw new Error("No callback function supplied");

  PromisePrototypeThen(
    Deno.lstat(getValidatedPathToString(path)),
    (stat) => callback(null, CFISBIS(stat, options.bigint)),
Repository: denoland/deno
File: cli/tools/upgrade.rs (Rust)

Code before:
  };

  let banner_handle = spawn_banner_task(
    &selected_version_to_upgrade.version_or_hash,
    selected_version_to_upgrade.release_channel,
    http_client_provider.get_or_create()?,
  );

  let dl_dir = factory.deno_dir()?.dl_folder_path();
  let cache_path = get_binary_cache_path(
    &dl_dir,
    &selected_version_to_upgrade.version_or_hash,
    requested_version.release_channel(),
  );

  let archive_data = if cache_path.exists() {
    log::info!(
      "{}",
      colors::gray(format!(
        "Using cached binary from {}",
        cache_path.display()
      ))
    );
    std::fs::read(&cache_path).with_context(|| {
      format!("Failed to read cached binary at '{}'", cache_path.display())
    })?
  } else {
    let download_url = get_download_url(
      &selected_version_to_upgrade.version_or_hash,
      requested_version.release_channel(),
    )?;
    log::info!("{}", colors::gray(format!("Downloading {}", &download_url)));
    let Some(data) = download_package(&client, download_url).await? else {
      log::error!("Download could not be found, aborting");
      deno_runtime::exit(1)
    };

    // Cache the downloaded archive
    if let Some(parent) = cache_path.parent() {
      fs::create_dir_all(parent).with_context(|| {
        format!("Failed to create cache directory '{}'", parent.display())
      })?;
    }
    if let Err(err) = fs::write(&cache_path, &data) {
      log::debug!("Failed to cache binary: {}", err);
    }

    data
  };

  // verify checksum if provided

Review comment (bug):
Instead of `cache_path.exists()`, probably it should always try to read from this and on failure then it goes the the next step.

Code after:

  let force_selection_of_new_version =
    upgrade_flags.force || full_path_output_flag.is_some();

  let requested_version =
    RequestedVersion::from_upgrade_flags(upgrade_flags.clone())?;

  log::info!("Current Deno version: v{}", version::DENO_VERSION_INFO.deno);

  let maybe_selected_version_to_upgrade = match &requested_version {
    RequestedVersion::Latest(channel) => {
      find_latest_version_to_upgrade(
        http_client_provider.clone(),
        *channel,
        force_selection_of_new_version,
      )
      .await?
    }
    RequestedVersion::SpecificVersion(channel, version) => {
      select_specific_version_for_upgrade(
        *channel,
        version.clone(),
        force_selection_of_new_version,
      )?
    }
  };

  let Some(selected_version_to_upgrade) = maybe_selected_version_to_upgrade
  else {
    return Ok(());
  };

  let banner_handle = spawn_banner_task(
    &selected_version_to_upgrade.version_or_hash,
    selected_version_to_upgrade.release_channel,
    http_client_provider.get_or_create()?,
  );

  let dl_dir = factory.deno_dir()?.dl_folder_path();
  let cache_path = get_binary_cache_path(
    &dl_dir,
    &selected_version_to_upgrade.version_or_hash,
    requested_version.release_channel(),
  );

  let sys = &sys_traits::impls::RealSys;
  let archive_data =
    if let Some(data) = try_read_cached_binary(sys, &cache_path) {
      log::info!(
        "{}",
        colors::gray(format!(
Repository: denoland/deno
File: cli/tools/upgrade.rs (Rust)

Code before:
        cache_path.display()
      ))
    );
    std::fs::read(&cache_path).with_context(|| {
      format!("Failed to read cached binary at '{}'", cache_path.display())
    })?
  } else {
    let download_url = get_download_url(
      &selected_version_to_upgrade.version_or_hash,
      requested_version.release_channel(),
    )?;
    log::info!("{}", colors::gray(format!("Downloading {}", &download_url)));
    let Some(data) = download_package(&client, download_url).await? else {
      log::error!("Download could not be found, aborting");
      deno_runtime::exit(1)
    };

    // Cache the downloaded archive
    if let Some(parent) = cache_path.parent() {
      fs::create_dir_all(parent).with_context(|| {
        format!("Failed to create cache directory '{}'", parent.display())
      })?;
    }
    if let Err(err) = fs::write(&cache_path, &data) {
      log::debug!("Failed to cache binary: {}", err);
    }

    data
  };

  // verify checksum if provided
  if let Some(expected_checksum) = &upgrade_flags.checksum {
    verify_checksum(&archive_data, expected_checksum)?;
  }

  log::info!(
    "{}",
    colors::gray(format!(
      "Deno is upgrading to version {}",
      &selected_version_to_upgrade.version_or_hash
    ))
  );

  let temp_dir =
    tempfile::TempDir::new().context("failed to create temporary directory")?;
  let new_exe_path = archive::unpack_into_dir(archive::UnpackArgs {
    exe_name: "deno",
    archive_name: &ARCHIVE_NAME,
    archive_data: &archive_data,
    is_windows: cfg!(windows),
    dest_path: temp_dir.path(),

Review comment (performance):
This should probably use `atomic_write`. Otherwise if the process is killed on this line it will corrupt the cache.

Code after:
        *channel,
        version.clone(),
        force_selection_of_new_version,
      )?
    }
  };

  let Some(selected_version_to_upgrade) = maybe_selected_version_to_upgrade
  else {
    return Ok(());
  };

  let banner_handle = spawn_banner_task(
    &selected_version_to_upgrade.version_or_hash,
    selected_version_to_upgrade.release_channel,
    http_client_provider.get_or_create()?,
  );

  let dl_dir = factory.deno_dir()?.dl_folder_path();
  let cache_path = get_binary_cache_path(
    &dl_dir,
    &selected_version_to_upgrade.version_or_hash,
    requested_version.release_channel(),
  );

  let sys = &sys_traits::impls::RealSys;
  let archive_data =
    if let Some(data) = try_read_cached_binary(sys, &cache_path) {
      log::info!(
        "{}",
        colors::gray(format!(
          "Using cached binary from {}",
          cache_path.display()
        ))
      );
      data
    } else {
      let download_url = get_download_url(
        &selected_version_to_upgrade.version_or_hash,
        requested_version.release_channel(),
      )?;
      log::info!("{}", colors::gray(format!("Downloading {}", &download_url)));
      let Some(data) = download_package(&client, download_url).await? else {
        log::error!("Download could not be found, aborting");
        deno_runtime::exit(1)
      };

      store_cached_binary(sys, &cache_path, &data);

      data
    };
Repository: denoland/deno
File: cli/args/flags.rs (Rust)

Code before:
  #[test]
  fn create() {
    let r = flags_from_vec(svec!["deno", "create"]);
    assert!(r.is_err());

    let r = flags_from_vec(svec!["deno", "create", "vite"]);
    assert_eq!(
      r.unwrap(),
      Flags {
        subcommand: DenoSubcommand::Init(InitFlags {
          package: Some("npm:vite".to_string()),
          package_args: vec![],
          dir: None,
          lib: false,
          serve: false,
          empty: false,
          yes: false,
        }),
        ..Flags::default()
      }
    );

    let r = flags_from_vec(svec!["deno", "create", "npm:vite", "my-project"]);
    assert_eq!(
      r.unwrap(),
      Flags {
        subcommand: DenoSubcommand::Init(InitFlags {
          package: Some("npm:vite".to_string()),
          package_args: svec!["my-project"],
          dir: None,
          lib: false,
          serve: false,
          empty: false,
          yes: false,
        }),
        ..Flags::default()
      }
    );

    let r = flags_from_vec(svec!["deno", "create", "--npm", "vite"]);
    assert_eq!(
      r.unwrap(),
      Flags {
        subcommand: DenoSubcommand::Init(InitFlags {
          package: Some("npm:vite".to_string()),
          package_args: vec![],
          dir: None,
          lib: false,
          serve: false,
          empty: false,
          yes: false,

Review comment (suggestion):
This should require a double hyphen. For example: `deno create npm:vite -- my-project`

It's better and I believe npm requires the same.

Code after:
    assert_eq!(
      r.unwrap(),
      Flags {
        subcommand: DenoSubcommand::Init(InitFlags {
          package: Some("vite".to_string()),
          package_args: vec![],
          dir: None,
          lib: false,
          serve: false,
          empty: false,
          yes: true,
        }),
        ..Flags::default()
      }
    );
  }

  #[test]
  fn create() {
    let r = flags_from_vec(svec!["deno", "create"]);
    assert!(r.is_err());

    let r = flags_from_vec(svec!["deno", "create", "vite"]);
    assert!(r.is_err());

    let r = flags_from_vec(svec!["deno", "create", "npm:vite", "my-project"]);
    assert!(r.is_err());

    let r =
      flags_from_vec(svec!["deno", "create", "npm:vite", "--", "my-project"]);
    assert_eq!(
      r.unwrap(),
      Flags {
        subcommand: DenoSubcommand::Init(InitFlags {
          package: Some("npm:vite".to_string()),
          package_args: svec!["my-project"],
          dir: None,
          lib: false,
          serve: false,
          empty: false,
          yes: false,
        }),
        ..Flags::default()
      }
    );

    let r = flags_from_vec(svec!["deno", "create", "--npm", "vite"]);
    assert_eq!(
      r.unwrap(),
      Flags {
        subcommand: DenoSubcommand::Init(InitFlags {
Repository: denoland/deno
File: cli/args/flags.rs (Rust)

Code before:
    empty: false,
    yes: matches.get_flag("yes"),
  });

  Ok(())
}

fn normalize_create_package_name(
  package: String,
) -> Result<String, clap::Error> {
  if package.starts_with("jsr:") {
    return Err(clap::Error::raw(
      clap::error::ErrorKind::InvalidValue,
      "JSR packages are not supported by `deno create` yet. Use npm: or --npm.",
    ));
  }

  let name = package.strip_prefix("npm:").unwrap_or(&package);
  if name.is_empty() {
    return Err(clap::Error::raw(
      clap::error::ErrorKind::InvalidValue,
      "Missing package name after 'npm:' prefix.",
    ));
  }

  Ok(format!("npm:{name}"))
}

fn info_parse(
  flags: &mut Flags,
  matches: &mut ArgMatches,
) -> clap::error::Result<()> {
  unstable_args_parse(flags, matches, UnstableArgsConfig::ResolutionOnly);
  reload_arg_parse(flags, matches)?;
  config_args_parse(flags, matches);
  import_map_arg_parse(flags, matches);
  location_arg_parse(flags, matches);
  ca_file_arg_parse(flags, matches);
  unsafely_ignore_certificate_errors_parse(flags, matches);
  node_modules_and_vendor_dir_arg_parse(flags, matches);
  lock_args_parse(flags, matches);
  no_remote_arg_parse(flags, matches);
  no_npm_arg_parse(flags, matches);
  allow_and_deny_import_parse(flags, matches)?;
  let json = matches.get_flag("json");
  flags.subcommand = DenoSubcommand::Info(InfoFlags {
    file: matches.remove_one::<String>("file"),
    json,
  });

  Ok(())

Review comment (suggestion):
I know it was said in the issue to not require `npm:`, but can you change this to require `npm:` for now? I think not requiring `npm:` is inconsistent with other subcommands and requires a bigger discussion internally.

Code after:
  // `create` is a thin alias to init's package-generator path, not template mode.
  flags.subcommand = DenoSubcommand::Init(InitFlags {
    package: Some(package),
    package_args,
    dir: None,
    lib: false,
    serve: false,
    empty: false,
    yes: matches.get_flag("yes"),
  });

  Ok(())
}

fn normalize_create_package_name(
  package: String,
  allow_unprefixed: bool,
) -> Result<String, clap::Error> {
  if package.starts_with("jsr:") {
    return Err(clap::Error::raw(
      clap::error::ErrorKind::InvalidValue,
      "JSR packages are not supported by `deno create` yet. Use npm: or --npm.",
    ));
  }

  let name = package.strip_prefix("npm:").unwrap_or(&package);
  if name.is_empty() {
    return Err(clap::Error::raw(
      clap::error::ErrorKind::InvalidValue,
      "Missing package name after 'npm:' prefix.",
    ));
  }

  if package.starts_with("npm:") {
    Ok(package)
  } else if allow_unprefixed {
    Ok(format!("npm:{package}"))
  } else {
    Err(clap::Error::raw(
      clap::error::ErrorKind::InvalidValue,
      "Missing `npm:` prefix. For example: `deno create npm:vite`.",
    ))
  }
}

fn info_parse(
  flags: &mut Flags,
  matches: &mut ArgMatches,
) -> clap::error::Result<()> {
  unstable_args_parse(flags, matches, UnstableArgsConfig::ResolutionOnly);
  reload_arg_parse(flags, matches)?;
Repository: diegomura/react-pdf
File: src/utils/image.js (JavaScript)

Code before:
import fetch from 'isomorphic-fetch';
import PNG from '@react-pdf/png-js';
import JPEG from './jpeg';

const isValidFormat = format => {
  const lower = format.toLowerCase();
  return lower === 'jpg' || lower === 'jpeg' || lower === 'png';
};

const isBase64 = src => /data:image\/[a-zA-Z]*;base64,[^\"]*/g.test(src);

function getImage(body, extension) {
  switch (extension.toLowerCase()) {
    case 'jpg':
    case 'jpeg':
      return new JPEG(body);
    case 'png':
      return new PNG(body);
    default:
      return null;
  }
}

const resolveBase64Image = src => {
  const match = /data:image\/([a-zA-Z]*);base64,([^\"]*)/g.exec(src);
  const format = match[1];
  const data = match[2];

  if (!isValidFormat(format)) {
    throw new Error(`Base64 image invalid format: ${format}`);
  }

  return new Promise(resolve => {
    return resolve(getImage(Buffer.from(data, 'base64'), format));
  });

Review comment (suggestion):
I'm no expert on this, but reading up on matching for base64 it does seem a bit more complex than this suggests. I understand that separating data and filetype is the objective here, so it makes sense to keep it simple. I would suggest renaming the function to `isCompatibleBase64`, just to make this distinction clear.

Code after:
import fetch from 'isomorphic-fetch';
import PNG from '@react-pdf/png-js';
import JPEG from './jpeg';

const isValidFormat = format => {
  const lower = format.toLowerCase();
  return lower === 'jpg' || lower === 'jpeg' || lower === 'png';
};

const isCompatibleBase64 = src =>
  /data:image\/[a-zA-Z]*;base64,[^"]*/g.test(src);

function getImage(body, extension) {
  switch (extension.toLowerCase()) {
    case 'jpg':
    case 'jpeg':
      return new JPEG(body);
    case 'png':
      return new PNG(body);
    default:
      return null;
  }
}

const resolveBase64Image = src => {
  const match = /data:image\/([a-zA-Z]*);base64,([^"]*)/g.exec(src);
  const format = match[1];
  const data = match[2];

  if (!isValidFormat(format)) {
    throw new Error(`Base64 image invalid format: ${format}`);
  }

  return new Promise(resolve => {
    return resolve(getImage(Buffer.from(data, 'base64'), format));
Repository: diegomura/react-pdf
File: src/utils/image.js (JavaScript)

Code before:
import fetch from 'isomorphic-fetch';
import PNG from '@react-pdf/png-js';
import JPEG from './jpeg';

const isValidFormat = format => {
  const lower = format.toLowerCase();
  return lower === 'jpg' || lower === 'jpeg' || lower === 'png';
};

const isBase64 = src => /data:image\/[a-zA-Z]*;base64,[^\"]*/g.test(src);

function getImage(body, extension) {
  switch (extension.toLowerCase()) {
    case 'jpg':
    case 'jpeg':
      return new JPEG(body);
    case 'png':
      return new PNG(body);
    default:
      return null;
  }
}

const resolveBase64Image = src => {
  const match = /data:image\/([a-zA-Z]*);base64,([^\"]*)/g.exec(src);
  const format = match[1];
  const data = match[2];

  if (!isValidFormat(format)) {
    throw new Error(`Base64 image invalid format: ${format}`);
  }

  return new Promise(resolve => {
    return resolve(getImage(Buffer.from(data, 'base64'), format));
  });
};

const resolveLocalImage = src => {
  if (src.data && src.format) {
    return new Promise(resolve => resolve(getImage(src.data, src.format)));
  }

  throw new Error(`Invalid data given for local file: ${JSON.stringify(src)}`);
};

const resolveRemoteImage = src => {
  return fetch(src)
    .then(response => {
      if (response.buffer) {
        return response.buffer();

Review comment (refactor):
This regex is identical to the `isBase64` regex besides the catching groups, right? If so, they should be able to use the same, extracted to a single `const`, just for simplicity.

Code after:
import fetch from 'isomorphic-fetch';
import PNG from '@react-pdf/png-js';
import JPEG from './jpeg';

const isValidFormat = format => {
  const lower = format.toLowerCase();
  return lower === 'jpg' || lower === 'jpeg' || lower === 'png';
};

const isCompatibleBase64 = src =>
  /data:image\/[a-zA-Z]*;base64,[^"]*/g.test(src);

function getImage(body, extension) {
  switch (extension.toLowerCase()) {
    case 'jpg':
    case 'jpeg':
      return new JPEG(body);
    case 'png':
      return new PNG(body);
    default:
      return null;
  }
}

const resolveBase64Image = src => {
  const match = /data:image\/([a-zA-Z]*);base64,([^"]*)/g.exec(src);
  const format = match[1];
  const data = match[2];

  if (!isValidFormat(format)) {
    throw new Error(`Base64 image invalid format: ${format}`);
  }

  return new Promise(resolve => {
    return resolve(getImage(Buffer.from(data, 'base64'), format));
  });
};

const resolveLocalImage = src => {
  if (src.data && src.format) {
    return new Promise(resolve => resolve(getImage(src.data, src.format)));
  }

  throw new Error(`Invalid data given for local file: ${JSON.stringify(src)}`);
};

const resolveRemoteImage = src => {
  return fetch(src)
    .then(response => {
      if (response.buffer) {
Repository: diegomura/react-pdf
File: src/utils/image.js (JavaScript)

Code before:
import { isNode } from 'browser-or-node';
import fetch from 'isomorphic-fetch';
import PNG from './png';
import JPEG from './jpeg';
import createCache from './cache';

export const isDangerousLocalPath = (
  filename,
  { safePath = './public' } = {},
) => {
  const path = require('path');
  const absoluteSafePath = path.resolve(safePath);
  const absoluteFilePath = path.resolve(filename);
  return !absoluteFilePath.startsWith(absoluteSafePath);
};

const fetchLocalFile = (src, { safePath, allowDangerousPaths = false } = {}) =>
  new Promise((resolve, reject) => {
    try {
      const {
        protocol,
        auth,
        host,
        port,
        hostname,
        path,
      } = require('url').parse(src);
      const absolutePath = require('path').resolve(path);
      const fs = require('fs');
      if (
        (protocol && protocol !== 'file:') ||
        auth ||
        host ||
        port ||
        hostname
      ) {

Review comment (suggestion):
No need to require it here. You can use `import path from 'path'` as all other imports in this file. Rollup will ignore it for web build. Of course you should use `path` only inside a `!BROWSER` conditional

Code after:
import fs from 'fs';
import path from 'path';
import url from 'url';
import fetch from 'isomorphic-fetch';
import PNG from './png';
import JPEG from './jpeg';
import createCache from './cache';

export const getAbsoluteLocalPath = src => {
  if (BROWSER) {
    throw new Error('Cannot check local paths in client-side environment');
  }

  const { protocol, auth, host, port, hostname, path: pathname } = url.parse(
    src,
  );
  const absolutePath = path.resolve(pathname);
  if ((protocol && protocol !== 'file:') || auth || host || port || hostname) {
    return undefined;
  }
  return absolutePath;
};

export const isDangerousLocalPath = (
  filename,
  { safePath = './public' } = {},
) => {
  if (BROWSER) {
    throw new Error(
      'Cannot check dangerous local path in client-side environemnt',
    );
  }
  const absoluteSafePath = path.resolve(safePath);
  const absoluteFilePath = path.resolve(filename);
  return !absoluteFilePath.startsWith(absoluteSafePath);
};