ronantakizawa/codereview-bench huggingface dataset:
Repository: denoland/deno
File: ext/fs/ops.rs (Rust)

Code before:
}

fn sync_permission_check<'a, P: FsPermissions + 'static>(
  permissions: &'a mut P,
  api_name: &'static str,
) -> Option<impl AccessCheckFn + 'a> {
  if permissions.allows_all() {
    None
  } else {
    Some(cast(move |resolved, path, options| {
      permissions.check(resolved, options, path, api_name)
    }))
  }
}

fn async_permission_check<P: FsPermissions + 'static>(
  state: Rc<RefCell<OpState>>,
  api_name: &'static str,
) -> Option<impl AccessCheckFn> {
  let allows_all = {
    let mut permissions = state.borrow_mut();
    let permissions = permissions.borrow_mut::<P>();
    permissions.allows_all()
  };
  if allows_all {
    None
  } else {
    Some(cast(move |resolved, path, options| {
      let mut state = state.borrow_mut();
      let permissions = state.borrow_mut::<P>();
      permissions.check(resolved, options, path, api_name)
    }))
  }
}

fn map_permission_error(
  operation: &'static str,
  error: FsError,
  path: &Path,
) -> FsOpsError {
  match error {
    FsError::NotCapable(err) => {
      let path = format!("{path:?}");
      let (path, truncated) = if path.len() > 1024 {
        (&path[0..1024], "...(truncated)")
      } else {
        (path.as_str(), "")
      };

      FsOpsErrorKind::NotCapableAccess {
        standalone: deno_permissions::is_standalone(),

Review comment (security):
How hard is it to move the canonicalization stuff into `FsPermissions::check_open`? Ideally we'd only be accessing `Permissions` once from the OpState and then aquiring its lock once instead of now twice when not running with `-A`.

Code after:
  state: Rc<RefCell<OpState>>,
  api_name: &'static str,
) -> impl AccessCheckFn + 'static {
  move |path, options, resolve| {
    let mut state = state.borrow_mut();
    let permissions = state.borrow_mut::<P>();
    permissions.check(options, path, api_name, resolve)
  }
}

fn map_permission_error(
  operation: &'static str,
  error: FsError,
  path: &Path,
) -> FsOpsError {
  match error {
    FsError::NotCapable(err) => {
      let path = format!("{path:?}");
      let (path, truncated) = if path.len() > 1024 {
        (&path[0..1024], "...(truncated)")
      } else {
        (path.as_str(), "")
      };

      FsOpsErrorKind::NotCapableAccess {
        standalone: deno_permissions::is_standalone(),
        err,
        path: format!("{path}{truncated}"),
      }
      .into_box()
    }
    err => Err::<(), _>(err)
      .context_path(operation, path)
      .err()
      .unwrap(),
  }
}

#[op2(stack_trace)]
#[string]
pub fn op_fs_cwd<P>(state: &mut OpState) -> Result<String, FsOpsError>
where
  P: FsPermissions + 'static,
{
  let fs = state.borrow::<FileSystemRc>();
  let path = fs.cwd()?;
  let path_str = path_into_string(path.into_os_string())?;
  Ok(path_str)
}

#[op2(fast, stack_trace)]
Repository: denoland/deno
File: ext/node/ops/handle_wrap.rs (Rust)

Code before:

#[cfg(test)]
mod tests {
  use std::future::poll_fn;
  use std::task::Poll;

  use deno_core::JsRuntime;
  use deno_core::RuntimeOptions;

  #[tokio::test]
  async fn test_handle_wrap() {
    deno_core::extension!(
      test_ext,
      objects = [super::AsyncWrap, super::HandleWrap,],
      state = |state| {
        state.put::<super::AsyncId>(super::AsyncId::default());
      }
    );

    let mut runtime = JsRuntime::new(RuntimeOptions {
      extensions: vec![test_ext::init_ops()],
      ..Default::default()
    });

    let source_code = r#"
          const { HandleWrap } = Deno.core.ops;

          let called = false;
          class MyHandleWrap extends HandleWrap {
            constructor() {
              super(0, null);
            }

            _onClose() {
              called = true;
            }
          }

          const handleWrap = new MyHandleWrap();
          handleWrap.close();

          if (!called) {
            throw new Error("HandleWrap._onClose was not called");
          }
        "#;

    poll_fn(move |cx| {
      runtime
        .execute_script("file://handle_wrap_test.js", source_code)
        .unwrap();


Review comment (suggestion):
Add 
```
// Ported from https://github.com/nodejs/node/blob/038d82980ab26cd79abe4409adc2fecad94d7c93/src/handle_wrap.cc#L49-L55
```

Code after:
  fn has_ref(&self, state: &mut OpState) -> bool {
    if let Some(handle) = self.handle {
      return state.has_ref(handle);
    }

    true
  }

  // Ported from Node.js
  //
  // https://github.com/nodejs/node/blob/038d82980ab26cd79abe4409adc2fecad94d7c93/src/handle_wrap.cc#L40-L46
  #[fast]
  #[rename("r#ref")]
  fn ref_(&self, state: &mut OpState) {
    if self.is_alive() {
      if let Some(handle) = self.handle {
        state.uv_ref(handle);
      }
    }
  }

  // Ported from Node.js
  //
  // https://github.com/nodejs/node/blob/038d82980ab26cd79abe4409adc2fecad94d7c93/src/handle_wrap.cc#L49-L55
  #[fast]
  fn unref(&self, state: &mut OpState) {
    if self.is_alive() {
      if let Some(handle) = self.handle {
        state.uv_unref(handle);
      }
    }
  }
}

fn uv_close<F>(
  scope: &mut v8::HandleScope,
  op_state: Rc<RefCell<OpState>>,
  this: v8::Global<v8::Object>,
  on_close: F,
) where
  F: FnOnce(&mut v8::HandleScope) + 'static,
{
  // Call _onClose() on the JS handles. Not needed for Rust handles.
  let this = v8::Local::new(scope, this);
  let on_close_str = ON_CLOSE_STR.v8_string(scope).unwrap();
  let onclose = this.get(scope, on_close_str.into());

  if let Some(onclose) = onclose {
    let fn_: v8::Local<v8::Function> = onclose.try_into().unwrap();
    fn_.call(scope, this.into(), &[]);
  }
Repository: denoland/deno
File: ext/node/ops/handle_wrap.rs (Rust)

Code before:
    if let Some(handle) = self.handle {
      state.uv_ref(handle);
    }
  }

  #[fast]
  fn unref(&self, state: &mut OpState) {
    if let Some(handle) = self.handle {
      state.uv_unref(handle);
    }
  }
}

#[cfg(test)]
mod tests {
  use std::future::poll_fn;
  use std::task::Poll;

  use deno_core::JsRuntime;
  use deno_core::RuntimeOptions;

  #[tokio::test]
  async fn test_handle_wrap() {
    deno_core::extension!(
      test_ext,
      objects = [super::AsyncWrap, super::HandleWrap,],
      state = |state| {
        state.put::<super::AsyncId>(super::AsyncId::default());
      }
    );

    let mut runtime = JsRuntime::new(RuntimeOptions {
      extensions: vec![test_ext::init_ops()],
      ..Default::default()
    });

    let source_code = r#"
          const { HandleWrap } = Deno.core.ops;

          let called = false;
          class MyHandleWrap extends HandleWrap {
            constructor() {
              super(0, null);
            }

            _onClose() {
              called = true;
            }
          }

          const handleWrap = new MyHandleWrap();

Review comment (suggestion):
Add
```
https://github.com/nodejs/node/blob/038d82980ab26cd79abe4409adc2fecad94d7c93/src/handle_wrap.cc#L40-L46
```

Code after:
    };

    uv_close(scope, op_state, this, on_close);
    self.state.set(State::Closing);

    Ok(())
  }

  // Ported from Node.js
  //
  // https://github.com/nodejs/node/blob/038d82980ab26cd79abe4409adc2fecad94d7c93/src/handle_wrap.cc#L58-L62
  #[fast]
  fn has_ref(&self, state: &mut OpState) -> bool {
    if let Some(handle) = self.handle {
      return state.has_ref(handle);
    }

    true
  }

  // Ported from Node.js
  //
  // https://github.com/nodejs/node/blob/038d82980ab26cd79abe4409adc2fecad94d7c93/src/handle_wrap.cc#L40-L46
  #[fast]
  #[rename("r#ref")]
  fn ref_(&self, state: &mut OpState) {
    if self.is_alive() {
      if let Some(handle) = self.handle {
        state.uv_ref(handle);
      }
    }
  }

  // Ported from Node.js
  //
  // https://github.com/nodejs/node/blob/038d82980ab26cd79abe4409adc2fecad94d7c93/src/handle_wrap.cc#L49-L55
  #[fast]
  fn unref(&self, state: &mut OpState) {
    if self.is_alive() {
      if let Some(handle) = self.handle {
        state.uv_unref(handle);
      }
    }
  }
}

fn uv_close<F>(
  scope: &mut v8::HandleScope,
  op_state: Rc<RefCell<OpState>>,
  this: v8::Global<v8::Object>,
  on_close: F,
Repository: denoland/deno
File: ext/node/polyfills/testing.ts (TypeScript)

Code before:
suite.skip = function skip(name, options, fn) {
  return suite(name, options, fn, { skip: true });
};
suite.todo = function todo(name, options, fn) {
  return suite(name, options, fn, { todo: true });
};
suite.only = function only(name, options, fn) {
  return suite(name, options, fn, { only: true });
};

export function it(name, options, fn) {
  return test(name, options, fn, {});
}

it.skip = function skip(name, options, fn) {
  return test.skip(name, options, fn);
};

it.todo = function todo(name, options, fn) {
  return test.todo(name, options, fn);
};

it.only = function only(name, options, fn) {
  return test.only(name, options, fn);
};

export function before() {
  notImplemented("test.before");
}

export function after() {
  notImplemented("test.after");
}

export function beforeEach() {
  notImplemented("test.beforeEach");
}

export function afterEach() {
  notImplemented("test.afterEach");
}

export const mock = {
  fn: () => {
    notImplemented("test.mock.fn");
  },
  getter: () => {
    notImplemented("test.mock.getter");
  },
  method: () => {
    notImplemented("test.mock.method");

Review comment (suggestion):
Can you add these lines?

```
test.test = test;
test.describe = describe;
test.it = it;
```
This will make this module more in-line with Node's implementation

Code after:
suite.skip = function skip(name, options, fn) {
  return suite(name, options, fn, { skip: true });
};
suite.todo = function todo(name, options, fn) {
  return suite(name, options, fn, { todo: true });
};
suite.only = function only(name, options, fn) {
  return suite(name, options, fn, { only: true });
};

export function it(name, options, fn) {
  return test(name, options, fn, {});
}

it.skip = function skip(name, options, fn) {
  return test.skip(name, options, fn);
};

it.todo = function todo(name, options, fn) {
  return test.todo(name, options, fn);
};

it.only = function only(name, options, fn) {
  return test.only(name, options, fn);
};

export function before() {
  notImplemented("test.before");
}

export function after() {
  notImplemented("test.after");
}

export function beforeEach() {
  notImplemented("test.beforeEach");
}

export function afterEach() {
  notImplemented("test.afterEach");
}

test.it = it;
test.describe = describe;
test.suite = suite;

export const mock = {
  fn: () => {
    notImplemented("test.mock.fn");
  },
  getter: () => {
Repository: denoland/deno
File: runtime/js/41_prompt.js (JavaScript)

Code before:

  readLineFromStdinSync();
}

function confirm(message = "Confirm") {
  if (!stdin.isTerminal()) {
    return false;
  }

  core.print(`${message} [y/N] `, false);

  const answer = readLineFromStdinSync();

  return answer === "Y" || answer === "y";
}

function prompt(message = "Prompt", defaultValue) {
  defaultValue ??= "";

  if (!stdin.isTerminal()) {
    return null;
  }

  const formattedMessage = message === "" ? "" : `${message} `;

  return op_read_line_prompt(formattedMessage, `${defaultValue}`);
}

function readLineFromStdinSync() {
  const c = new Uint8Array(1);
  const buf = [];

  while (true) {
    const n = stdin.readSync(c);
    if (n === null || n === 0) {
      break;
    }
    if (c[0] === CR) {
      const n = stdin.readSync(c);
      if (c[0] === LF) {
        break;
      }
      ArrayPrototypePush(buf, CR);
      if (n === null || n === 0) {
        break;
      }
    }
    if (c[0] === LF) {
      break;
    }
    ArrayPrototypePush(buf, c[0]);

Review comment (suggestion):
```suggestion
  const formattedMessage = message.length === 0 ? "" : `${message} `;
  return op_read_line_prompt(formattedMessage, `${defaultValue}`);
```

Code after:

  readLineFromStdinSync();
}

function confirm(message = "Confirm") {
  if (!stdin.isTerminal()) {
    return false;
  }

  core.print(`${message} [y/N] `, false);

  const answer = readLineFromStdinSync();

  return answer === "Y" || answer === "y";
}

function prompt(message = "Prompt", defaultValue) {
  defaultValue ??= "";

  if (!stdin.isTerminal()) {
    return null;
  }

  const formattedMessage = message.length === 0 ? "" : `${message} `;
  return op_read_line_prompt(formattedMessage, `${defaultValue}`);
}

function readLineFromStdinSync() {
  const c = new Uint8Array(1);
  const buf = [];

  while (true) {
    const n = stdin.readSync(c);
    if (n === null || n === 0) {
      break;
    }
    if (c[0] === CR) {
      const n = stdin.readSync(c);
      if (c[0] === LF) {
        break;
      }
      ArrayPrototypePush(buf, CR);
      if (n === null || n === 0) {
        break;
      }
    }
    if (c[0] === LF) {
      break;
    }
    ArrayPrototypePush(buf, c[0]);
  }
Repository: denoland/deno
File: ext/node/polyfills/testing.ts (TypeScript)

Code before:

const methodsToCopy = [
  "deepEqual",
  "deepStrictEqual",
  "doesNotMatch",
  "doesNotReject",
  "doesNotThrow",
  "equal",
  "fail",
  "ifError",
  "match",
  "notDeepEqual",
  "notDeepStrictEqual",
  "notEqual",
  "notStrictEqual",
  "partialDeepStrictEqual",
  "rejects",
  "strictEqual",
  "throws",
];

/** `assert` object available via t.assert */
let assertObject = undefined;
function getAssertObject() {
  if (assertObject === undefined) {
    const assert = process.getBuiltinModule("node:assert");
    assertObject = { __proto__: null };
    ArrayPrototypeForEach(methodsToCopy, (method) => {
      assertObject[method] = assert[method];
    });
  }
  return assertObject;
}

export function run() {
  notImplemented("test.run");
}

function noop() {}

class NodeTestContext {
  #denoContext: Deno.TestContext;

  constructor(t: Deno.TestContext) {
    this.#denoContext = t;
  }

  get assert() {
    return getAssertObject();
  }


Review comment (suggestion):
Shouldn't this be a direct import of `assert` module - since we use ESM instead of CJS I don't think there's a need for going through `process.getBuiltinModule`

Code after:
import assert from "node:assert";

const methodsToCopy = [
  "deepEqual",
  "deepStrictEqual",
  "doesNotMatch",
  "doesNotReject",
  "doesNotThrow",
  "equal",
  "fail",
  "ifError",
  "match",
  "notDeepEqual",
  "notDeepStrictEqual",
  "notEqual",
  "notStrictEqual",
  "partialDeepStrictEqual",
  "rejects",
  "strictEqual",
  "throws",
];

/** `assert` object available via t.assert */
let assertObject = undefined;
function getAssertObject() {
  if (assertObject === undefined) {
    assertObject = { __proto__: null };
    ArrayPrototypeForEach(methodsToCopy, (method) => {
      assertObject[method] = assert[method];
    });
  }
  return assertObject;
}

export function run() {
  notImplemented("test.run");
}

function noop() {}

class NodeTestContext {
  #denoContext: Deno.TestContext;

  constructor(t: Deno.TestContext) {
    this.#denoContext = t;
  }

  get assert() {
    return getAssertObject();
  }
Repository: denoland/deno
File: ext/node/polyfills/testing.ts (TypeScript)

Code before:
    // deno-lint-ignore no-console
    console.log("DIAGNOSTIC:", message);
  }

  get mock() {
    notImplemented("test.TestContext.mock");
    return null;
  }

  runOnly() {
    notImplemented("test.TestContext.runOnly");
    return null;
  }

  skip() {
    warnNotImplemented("test.TestContext.skip");
    return null;
  }

  todo() {
    warnNotImplemented("test.TestContext.todo");
    return null;
  }

  test(name, options, fn) {
    const prepared = prepareOptions(name, options, fn, {});
    return PromisePrototypeThen(
      this.#denoContext.step({
        name: prepared.name,
        fn: async (denoTestContext) => {
          const newNodeTextContext = new NodeTestContext(denoTestContext);
          await prepared.fn(newNodeTextContext);
        },
        ignore: prepared.options.todo || prepared.options.skip,
        sanitizeExit: false,
        sanitizeOps: false,
        sanitizeResources: false,
      }),
      () => undefined,
    );
  }

  before(_fn, _options) {
    notImplemented("test.TestContext.before");
  }

  after(_fn, _options) {
    notImplemented("test.TestContext.after");
  }

  beforeEach(_fn, _options) {

Review comment (suggestion):
Q: Why do we need to clone the assert object in the first place? Does Node only expose a subset here? Otherwise we could skip the object creation:

```ts
  get assert() {
    return assert
  }
```

Code after:
function getAssertObject() {
  if (assertObject === undefined) {
    assertObject = { __proto__: null };
    ArrayPrototypeForEach(methodsToCopy, (method) => {
      assertObject[method] = assert[method];
    });
  }
  return assertObject;
}

export function run() {
  notImplemented("test.run");
}

function noop() {}

class NodeTestContext {
  #denoContext: Deno.TestContext;

  constructor(t: Deno.TestContext) {
    this.#denoContext = t;
  }

  get assert() {
    return getAssertObject();
  }

  get signal() {
    notImplemented("test.TestContext.signal");
    return null;
  }

  get name() {
    notImplemented("test.TestContext.name");
    return null;
  }

  diagnostic(message) {
    // deno-lint-ignore no-console
    console.log("DIAGNOSTIC:", message);
  }

  get mock() {
    notImplemented("test.TestContext.mock");
    return null;
  }

  runOnly() {
    notImplemented("test.TestContext.runOnly");
    return null;
  }
Repository: denoland/deno
File: ext/net/02_tls.js (JavaScript)

Code before:
    );
  }
  const keyPair = loadTlsKeyPair("Deno.listenTls", arguments[0]);
  const { 0: rid, 1: localAddr } = op_net_listen_tls(
    { hostname, port },
    { alpnProtocols, reusePort },
    keyPair,
  );
  return new TlsListener(rid, localAddr);
}

// deno-lint-ignore require-await
async function startTls(
  conn,
  {
    hostname = "127.0.0.1",
    caCerts = [],
    cert = undefined,
    key = undefined,
    alpnProtocols = undefined,
  } = { __proto__: null },
) {
  const keyPair = loadTlsKeyPair("Deno.startTls", { cert, key });
  const { 0: rid, 1: localAddr, 2: remoteAddr } = op_tls_start({
    rid: conn[internalRidSymbol],
    hostname,
    caCerts,
    alpnProtocols,
  }, keyPair);
  return new TlsConn(rid, remoteAddr, localAddr);
}

const resolverSymbol = SymbolFor("unstableSniResolver");
const serverNameSymbol = SymbolFor("unstableServerName");

function createTlsKeyResolver(callback) {
  const { 0: resolver, 1: lookup } = op_tls_cert_resolver_create();
  (async () => {
    while (true) {
      const sni = await op_tls_cert_resolver_poll(lookup);
      if (typeof sni !== "string") {
        break;
      }
      try {
        const key = await callback(sni);
        if (!hasTlsKeyPairOptions(key)) {
          op_tls_cert_resolver_resolve_error(lookup, sni, "Invalid key");
        } else {
          const resolved = loadTlsKeyPair("Deno.listenTls", key);
          op_tls_cert_resolver_resolve(lookup, sni, resolved);
        }

Review comment (suggestion):
I don't think this should change the stable `Deno.startTls`. Instead, maybe add a private `startTls` function in this extension that is used by `ext/node` code internally.

Code after:
    );
  }
  const keyPair = loadTlsKeyPair("Deno.listenTls", arguments[0]);
  const { 0: rid, 1: localAddr } = op_net_listen_tls(
    { hostname, port },
    { alpnProtocols, reusePort },
    keyPair,
  );
  return new TlsListener(rid, localAddr);
}

// deno-lint-ignore require-await
async function startTls(
  conn,
  {
    hostname = "127.0.0.1",
    caCerts = [],
    alpnProtocols = undefined,
  } = { __proto__: null },
) {
  const { 0: rid, 1: localAddr, 2: remoteAddr } = op_tls_start({
    rid: conn[internalRidSymbol],
    hostname,
    caCerts,
    alpnProtocols,
  }, null);
  return new TlsConn(rid, remoteAddr, localAddr);
}

const resolverSymbol = SymbolFor("unstableSniResolver");
const serverNameSymbol = SymbolFor("unstableServerName");

function createTlsKeyResolver(callback) {
  const { 0: resolver, 1: lookup } = op_tls_cert_resolver_create();
  (async () => {
    while (true) {
      const sni = await op_tls_cert_resolver_poll(lookup);
      if (typeof sni !== "string") {
        break;
      }
      try {
        const key = await callback(sni);
        if (!hasTlsKeyPairOptions(key)) {
          op_tls_cert_resolver_resolve_error(lookup, sni, "Invalid key");
        } else {
          const resolved = loadTlsKeyPair("Deno.listenTls", key);
          op_tls_cert_resolver_resolve(lookup, sni, resolved);
        }
      } catch (e) {
        op_tls_cert_resolver_resolve_error(lookup, sni, e.message);
      }
Repository: denoland/deno
File: ext/node/polyfills/_fs/_fs_opendir.ts (TypeScript)

Code before:
import { promisify } from "ext:deno_node/internal/util.mjs";
import { primordials } from "ext:core/mod.js";

const {
  StringPrototypeToString,
  ObjectPrototypeIsPrototypeOf,
} = primordials;

/** These options aren't functionally used right now, as `Dir` doesn't yet support them.
 * However, these values are still validated.
 */
type Options = {
  encoding?: string;
  bufferSize?: number;
};
type Callback = (err?: Error | null, dir?: Dir) => void;

function _validateFunction(callback: unknown): asserts callback is Callback {
  validateFunction(callback, "callback");
}

function getPathString(
  path: string | Buffer | URL,
): string {
  if (ObjectPrototypeIsPrototypeOf(Buffer.prototype, path)) {
    // deno-lint-ignore prefer-primordials
    return path.toString();
  }

  return StringPrototypeToString(path);
}

/** @link https://nodejs.org/api/fs.html#fsopendirsyncpath-options */
export function opendir(
  path: string | Buffer | URL,
  options: Options | Callback,
  callback?: Callback,
) {
  callback = typeof options === "function" ? options : callback;
  _validateFunction(callback);

  path = getPathString(getValidatedPath(path));

  let err, dir;
  try {
    const { bufferSize } = getOptions(options, {
      encoding: "utf8",
      bufferSize: 32,
    });
    validateInteger(bufferSize, "options.bufferSize", 1, 4294967295);


Review comment (suggestion):
Buffers have their own `.toString` behavior so `StringPrototypeToString` can't be used for them

Code after:
import { promisify } from "ext:deno_node/internal/util.mjs";
import { primordials } from "ext:core/mod.js";

const {
  StringPrototypeToString,
} = primordials;

/** These options aren't functionally used right now, as `Dir` doesn't yet support them.
 * However, these values are still validated.
 */
type Options = {
  encoding?: string;
  bufferSize?: number;
};
type Callback = (err?: Error | null, dir?: Dir) => void;

function _validateFunction(callback: unknown): asserts callback is Callback {
  validateFunction(callback, "callback");
}

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

  return StringPrototypeToString(path);
}

/** @link https://nodejs.org/api/fs.html#fsopendirsyncpath-options */
export function opendir(
  path: string | Buffer | URL,
  options: Options | Callback,
  callback?: Callback,
) {
  callback = typeof options === "function" ? options : callback;
  _validateFunction(callback);

  path = getPathString(getValidatedPath(path));

  let err, dir;
  try {
    const { bufferSize } = getOptions(options, {
      encoding: "utf8",
      bufferSize: 32,
    });
    validateInteger(bufferSize, "options.bufferSize", 1, 4294967295);

    /** Throws if path is invalid */