ronantakizawa/codereview-bench huggingface dataset:
Repository: nodejs/node
File: lib/internal/process/permission.js (JavaScript)

Code before:
      const { getOptionValue } = require('internal/options');
      _permission = getOptionValue('--permission');
    }
    return _permission;
  },
  has(scope, reference) {
    validateString(scope, 'scope');
    if (reference != null) {
      // TODO: add support for WHATWG URLs and Uint8Arrays.
      if (isBuffer(reference)) {
        validateBuffer(reference, 'reference');
      } else {
        validateString(reference, 'reference');
      }
    }

    return permission.has(scope, reference);
  },
  availableFlags() {
    return [
      '--allow-fs-read',
      '--allow-fs-write',
      '--allow-addons',
      '--allow-child-process',
      '--allow-net',
      '--allow-wasi',
      '--allow-worker',
    ];
  },
});


Review comment (suggestion):
Use alphabetical order here?

```suggestion
      '--allow-inspector',
      '--allow-net',
```

Code after:
      const { getOptionValue } = require('internal/options');
      _permission = getOptionValue('--permission');
    }
    return _permission;
  },
  has(scope, reference) {
    validateString(scope, 'scope');
    if (reference != null) {
      // TODO: add support for WHATWG URLs and Uint8Arrays.
      if (isBuffer(reference)) {
        validateBuffer(reference, 'reference');
      } else {
        validateString(reference, 'reference');
      }
    }

    return permission.has(scope, reference);
  },
  availableFlags() {
    return [
      '--allow-fs-read',
      '--allow-fs-write',
      '--allow-addons',
      '--allow-child-process',
      '--allow-net',
      '--allow-inspector',
      '--allow-wasi',
      '--allow-worker',
    ];
  },
});
Repository: nodejs/node
File: src/js_native_api.h (C/C++)

Code before:

#if NAPI_VERSION >= 7
// ArrayBuffer detaching
NAPI_EXTERN napi_status NAPI_CDECL
napi_detach_arraybuffer(napi_env env, napi_value arraybuffer);

NAPI_EXTERN napi_status NAPI_CDECL
napi_is_detached_arraybuffer(napi_env env, napi_value value, bool* result);
#endif  // NAPI_VERSION >= 7

#if NAPI_VERSION >= 8
// Type tagging
NAPI_EXTERN napi_status NAPI_CDECL napi_type_tag_object(
    napi_env env, napi_value value, const napi_type_tag* type_tag);

NAPI_EXTERN napi_status NAPI_CDECL
napi_check_object_type_tag(napi_env env,
                           napi_value value,
                           const napi_type_tag* type_tag,
                           bool* result);
NAPI_EXTERN napi_status NAPI_CDECL napi_object_freeze(napi_env env,
                                                      napi_value object);
NAPI_EXTERN napi_status NAPI_CDECL napi_object_seal(napi_env env,
                                                    napi_value object);
NAPI_EXTERN napi_status NAPI_CDECL
napi_create_object_with_properties(napi_env env,
                                   napi_value prototype_or_null,
                                   napi_value* property_names,
                                   napi_value* property_values,
                                   size_t property_count,
                                   napi_value* result);
#endif  // NAPI_VERSION >= 8

EXTERN_C_END

#endif  // SRC_JS_NATIVE_API_H_


Review comment (suggestion):
A new node-api function should be nested in `NAPI_EXPERIMENTAL`. Example:

https://github.com/nodejs/node/blob/d2ff9daf589af059cacd58972640c8dbc6c7ba88/src/js_native_api.h#L489-L495

Code after:

// Instance data
NAPI_EXTERN napi_status NAPI_CDECL
napi_set_instance_data(node_api_basic_env env,
                       void* data,
                       napi_finalize finalize_cb,
                       void* finalize_hint);

NAPI_EXTERN napi_status NAPI_CDECL
napi_get_instance_data(node_api_basic_env env, void** data);
#endif  // NAPI_VERSION >= 6

#if NAPI_VERSION >= 7
// ArrayBuffer detaching
NAPI_EXTERN napi_status NAPI_CDECL
napi_detach_arraybuffer(napi_env env, napi_value arraybuffer);

NAPI_EXTERN napi_status NAPI_CDECL
napi_is_detached_arraybuffer(napi_env env, napi_value value, bool* result);
#endif  // NAPI_VERSION >= 7

#if NAPI_VERSION >= 8
// Type tagging
NAPI_EXTERN napi_status NAPI_CDECL napi_type_tag_object(
    napi_env env, napi_value value, const napi_type_tag* type_tag);

NAPI_EXTERN napi_status NAPI_CDECL
napi_check_object_type_tag(napi_env env,
                           napi_value value,
                           const napi_type_tag* type_tag,
                           bool* result);
NAPI_EXTERN napi_status NAPI_CDECL napi_object_freeze(napi_env env,
                                                      napi_value object);
NAPI_EXTERN napi_status NAPI_CDECL napi_object_seal(napi_env env,
                                                    napi_value object);
#endif  // NAPI_VERSION >= 8

EXTERN_C_END

#endif  // SRC_JS_NATIVE_API_H_
Repository: nodejs/node
File: test/js-native-api/test_object/test.js (JavaScript)

Code before:
  }, /Cannot assign to read only property 'x' of object '#<Object>/);

  assert.throws(() => {
    obj.w = 15;
  }, /Cannot add property w, object is not extensible/);

  assert.throws(() => {
    delete obj.x;
  }, /Cannot delete property 'x' of #<Object>/);
}

{
  const objectWithProperties = test_object.TestCreateObjectWithProperties();
  assert.strictEqual(typeof objectWithProperties, 'object');
  assert.strictEqual(objectWithProperties.name, 'Foo');
  assert.strictEqual(objectWithProperties.age, 42);
  assert.strictEqual(objectWithProperties.active, true);

  const emptyObject = test_object.TestCreateObjectWithPropertiesEmpty();
  assert.strictEqual(typeof emptyObject, 'object');
  assert.strictEqual(Object.keys(emptyObject).length, 0);

  const objectWithCustomPrototype = test_object.TestCreateObjectWithCustomPrototype();
  assert.strictEqual(typeof objectWithCustomPrototype, 'object');
  assert.strictEqual(objectWithCustomPrototype.value, 42);
  assert.strictEqual(typeof objectWithCustomPrototype.test, 'function');
}


Review comment (suggestion):
It's worth asserting that this object is created with the specified own property names:

```suggestion
  assert.deepStrictEqual(Object.getOwnPropertyNames(objectWithCustomPrototype), ['value']);
  assert.strictEqual(objectWithCustomPrototype.value, 42);
```

Code after:
  }, /Cannot assign to read only property 'x' of object '#<Object>/);

  assert.throws(() => {
    obj.w = 15;
  }, /Cannot add property w, object is not extensible/);

  assert.throws(() => {
    delete obj.x;
  }, /Cannot delete property 'x' of #<Object>/);
}

{
  const objectWithProperties = test_object.TestCreateObjectWithProperties();
  assert.strictEqual(typeof objectWithProperties, 'object');
  assert.strictEqual(objectWithProperties.name, 'Foo');
  assert.strictEqual(objectWithProperties.age, 42);
  assert.strictEqual(objectWithProperties.active, true);

  const emptyObject = test_object.TestCreateObjectWithPropertiesEmpty();
  assert.strictEqual(typeof emptyObject, 'object');
  assert.strictEqual(Object.keys(emptyObject).length, 0);

  const objectWithCustomPrototype = test_object.TestCreateObjectWithCustomPrototype();
  assert.strictEqual(typeof objectWithCustomPrototype, 'object');
  assert.deepStrictEqual(Object.getOwnPropertyNames(objectWithCustomPrototype), ['value']);
  assert.strictEqual(objectWithCustomPrototype.value, 42);
  assert.strictEqual(typeof objectWithCustomPrototype.test, 'function');
}
Repository: nodejs/node
File: lib/internal/modules/esm/module_job.js (JavaScript)

Code before:
} = require('internal/source_map/source_map_cache');
const assert = require('internal/assert');
const resolvedPromise = PromiseResolve();
const {
  setHasStartedUserESMExecution,
  urlToFilename,
} = require('internal/modules/helpers');
const { getOptionValue } = require('internal/options');
const noop = FunctionPrototype;
const {
  ERR_REQUIRE_ASYNC_MODULE,
} = require('internal/errors').codes;
let hasPausedEntry = false;

const CJSGlobalLike = [
  'require',
  'module',
  'exports',
  '__filename',
  '__dirname',
];
const getUndefinedCJSGlobalLike = (errorMessage) =>
  ArrayPrototypeFind(
    CJSGlobalLike,
    (globalLike) => errorMessage === `${globalLike} is not defined`,
  ) ?? null;


/**
 *
 * @param {Error} e
 * @param {string} url
 * @returns {void}
 */
const explainCommonJSGlobalLikeNotDefinedError = (e, url, hasTopLevelAwait) => {
  const undefinedGlobal = getUndefinedCJSGlobalLike(e?.message);
  if (e?.name === 'ReferenceError' && undefinedGlobal !== null) {

    if (hasTopLevelAwait) {
      let advice;
      switch (undefinedGlobal) {
        case 'require':
          advice = 'replace require() with import';
          break;
        case 'module':
        case 'exports':
          advice = 'use export instead of module.exports/exports';
          break;
        case '__filename':
          advice = 'use import.meta.filename instead';
          break;

Review comment (suggestion):
That's not useful, it it?
```suggestion
  );
```

Code after:
const { decorateErrorStack } = require('internal/util');
const { isPromise } = require('internal/util/types');
const {
  getSourceMapsSupport,
} = require('internal/source_map/source_map_cache');
const assert = require('internal/assert');
const resolvedPromise = PromiseResolve();
const {
  setHasStartedUserESMExecution,
  urlToFilename,
} = require('internal/modules/helpers');
const { getOptionValue } = require('internal/options');
const noop = FunctionPrototype;
const {
  ERR_REQUIRE_ASYNC_MODULE,
} = require('internal/errors').codes;
let hasPausedEntry = false;

const CJSGlobalLike = [
  'require',
  'module',
  'exports',
  '__filename',
  '__dirname',
];
const findCommonJSGlobalLikeNotDefinedError = (errorMessage) =>
  ArrayPrototypeFind(
    CJSGlobalLike,
    (globalLike) => errorMessage === `${globalLike} is not defined`,
  );


/**
 *
 * @param {Error} e
 * @param {string} url
 * @returns {void}
 */
const explainCommonJSGlobalLikeNotDefinedError = (e, url, hasTopLevelAwait) => {
  const notDefinedGlobalLike = e?.name === 'ReferenceError' && findCommonJSGlobalLikeNotDefinedError(e.message);

  if (notDefinedGlobalLike) {
    if (hasTopLevelAwait) {
      let advice;
      switch (notDefinedGlobalLike) {
        case 'require':
          advice = 'replace require() with import';
          break;
        case 'module':
        case 'exports':
          advice = 'use export instead of module.exports/exports';
Repository: nodejs/node
File: doc/api_assets/api.js (JavaScript)

Code before:
          } else {
            code = parentNode.querySelector('.cjs').textContent;
          }
        } else {
          code = parentNode.querySelector('code').textContent;
        }

        button.textContent = 'Copied';
        navigator.clipboard.writeText(code);

        setTimeout(() => {
          button.textContent = 'Copy';
        }, 2500);
      });
    });
  }

  function setupSidebarScroll() {
    const sidebar = document.querySelector('#column2');
    if (!sidebar) return;

    const currentModule = window.location.pathname.split('/').pop();
    if (!currentModule) return;

    const link = sidebar.querySelector(`a[href="${currentModule}"]`);
    if (!link) return;
    
    link.scrollIntoView({ block: 'center' });
  }


  function bootstrap() {
    // Check if we have JavaScript support.
    document.documentElement.classList.add('has-js');

    // Restore user mode preferences.
    setupTheme();

    // Handle pickers with click/taps rather than hovers.
    setupPickers();

    // Track when the header is in sticky position.
    setupStickyHeaders();

    // Make link to other versions of the doc open to the same hash target (if it exists).
    setupAltDocsLink();

    setupFlavorToggles();

    setupCopyButton();


Review comment (suggestion):
`a[href="${currentModule}"]` does a string comparison of the value of the `href` attribute, instead we can directly compare with the actual resolved value, which should be less work for the browser engine:
```suggestion
    const sidebarLinks = document.querySelector('#column2 a');
    let link;
    for (link of sidebarLinks) {
      if (link.href === window.location.href) break;
    }
```

Code after:
          } else {
            code = parentNode.querySelector('.cjs').textContent;
          }
        } else {
          code = parentNode.querySelector('code').textContent;
        }

        button.textContent = 'Copied';
        navigator.clipboard.writeText(code);

        setTimeout(() => {
          button.textContent = 'Copy';
        }, 2500);
      });
    });
  }

  function setupSidebarScroll() {
    const sidebarLinks = document.querySelectorAll('#column2 a');

    let link;
    for (link of sidebarLinks) {
      if (link.pathname === window.location.pathname) break;
    }

    if (!link) return;

    link.scrollIntoView({ block: 'center' });
  }


  function bootstrap() {
    // Check if we have JavaScript support.
    document.documentElement.classList.add('has-js');

    // Restore user mode preferences.
    setupTheme();

    // Handle pickers with click/taps rather than hovers.
    setupPickers();

    // Track when the header is in sticky position.
    setupStickyHeaders();

    // Make link to other versions of the doc open to the same hash target (if it exists).
    setupAltDocsLink();

    setupFlavorToggles();

    setupCopyButton();
Repository: nodejs/node
File: src/node_modules.cc (C++)

Code before:
    // to walk upwards
    if (is_permissions_enabled &&
        !env->permission()->is_granted(
            env,
            permission::PermissionScope::kFileSystemRead,
            current_path.generic_string())) [[unlikely]] {
      return nullptr;
    }

    // Check if the path ends with `/node_modules`
    if (current_path.generic_string().ends_with("/node_modules")) {
      return nullptr;
    }

    auto package_json_path = current_path / "package.json";
    auto package_json =
        GetPackageJSON(realm, package_json_path.string(), nullptr);
    if (package_json != nullptr) {
      return package_json;
    }
  } while (true);

  return nullptr;
}

void BindingData::GetNearestParentPackageJSON(
    const v8::FunctionCallbackInfo<v8::Value>& args) {
  CHECK_GE(args.Length(), 1);
  CHECK(args[0]->IsString());

  Realm* realm = Realm::GetCurrent(args);
  BufferValue path_value(realm->isolate(), args[0]);
  // Check if the path has a trailing slash. If so, add it after
  // ToNamespacedPath() as it will be deleted by ToNamespacedPath()
  bool slashCheck = path_value.ToStringView().ends_with(kPathSeparator);

  ToNamespacedPath(realm->env(), &path_value);

  std::string path_value_str = path_value.ToString();
  if (slashCheck) {
    path_value_str.push_back(kPathSeparator);
  }

  std::filesystem::path path;

#ifdef _WIN32
  std::wstring wide_path = ConvertToWideString(path_value_str, GetACP());
  path = std::filesystem::path(wide_path);
#else
  path = std::filesystem::path(path_value_str);
#endif

Review comment (security):
1. Can we use existing methods rather then adding a new method?
2. I believe `THROW_IF_INSUFFICIENT_PERMISSIONS` call is missing in this method.

Code after:
    if (is_permissions_enabled) {
      if (!env->permission()->is_granted(
              env,
              permission::PermissionScope::kFileSystemRead,
              ConvertGenericPathToUTF8(current_path))) [[unlikely]] {
        return nullptr;
      }
    }

    // Check if the path ends with `/node_modules`
    if (current_path.filename() == "node_modules") {
      return nullptr;
    }

    auto package_json_path = current_path / "package.json";

    auto package_json =
        GetPackageJSON(realm, ConvertPathToUTF8(package_json_path), nullptr);
    if (package_json != nullptr) {
      return package_json;
    }
  } while (true);

  return nullptr;
}

const std::filesystem::path BindingData::NormalizePath(
    Realm* realm, BufferValue* path_value) {
  // Check if the path has a trailing slash. If so, add it after
  // ToNamespacedPath() as it will be deleted by ToNamespacedPath()
  bool slashCheck = path_value->ToStringView().ends_with(kPathSeparator);

  ToNamespacedPath(realm->env(), path_value);

  auto path = path_value->ToPath();

  if (slashCheck) {
    path /= "";
  }

  return path;
}

void BindingData::GetNearestParentPackageJSON(
    const v8::FunctionCallbackInfo<v8::Value>& args) {
  CHECK_GE(args.Length(), 1);
  CHECK(args[0]->IsString());

  Realm* realm = Realm::GetCurrent(args);
  BufferValue path_value(realm->isolate(), args[0]);
Repository: nodejs/node
File: lib/stream/consumers.js (JavaScript)

Code before:
  return new Blob(chunks);
}

/**
 * @param {AsyncIterable|ReadableStream|Readable} stream
 * @returns {Promise<ArrayBuffer>}
 */
async function arrayBuffer(stream) {
  const ret = await blob(stream);
  return ret.arrayBuffer();
}

/**
 * @param {AsyncIterable|ReadableStream|Readable} stream
 * @returns {Promise<Buffer>}
 */
async function buffer(stream) {
  return Buffer.from(await arrayBuffer(stream));
}

/**
 * @param {AsyncIterable|ReadableStream|Readable} stream
 * @returns {Promise<Uint8Array>}
 */
async function bytes(stream) {
  return new Uint8Array(await buffer(stream));
}

/**
 * @param {AsyncIterable|ReadableStream|Readable} stream
 * @returns {Promise<string>}
 */
async function text(stream) {
  const dec = new TextDecoder();
  let str = '';
  for await (const chunk of stream) {
    if (typeof chunk === 'string')
      str += chunk;
    else
      str += dec.decode(chunk, { stream: true });
  }
  // Flush the streaming TextDecoder so that any pending
  // incomplete multibyte characters are handled.
  str += dec.decode(undefined, { stream: false });
  return str;
}

/**
 * @param {AsyncIterable|ReadableStream|Readable} stream
 * @returns {Promise<any>}
 */

Review comment (suggestion):
```suggestion
  return new Uint8Array(await arrayBuffer(stream));
```

There's no need to create an intermediate `Buffer` just to then copy its contents into the new `Uint8Array`.

Code after:
  return new Blob(chunks);
}

/**
 * @param {AsyncIterable|ReadableStream|Readable} stream
 * @returns {Promise<ArrayBuffer>}
 */
async function arrayBuffer(stream) {
  const ret = await blob(stream);
  return ret.arrayBuffer();
}

/**
 * @param {AsyncIterable|ReadableStream|Readable} stream
 * @returns {Promise<Buffer>}
 */
async function buffer(stream) {
  return Buffer.from(await arrayBuffer(stream));
}

/**
 * @param {AsyncIterable|ReadableStream|Readable} stream
 * @returns {Promise<Uint8Array>}
 */
async function bytes(stream) {
  return new Uint8Array(await arrayBuffer(stream));
}

/**
 * @param {AsyncIterable|ReadableStream|Readable} stream
 * @returns {Promise<string>}
 */
async function text(stream) {
  const dec = new TextDecoder();
  let str = '';
  for await (const chunk of stream) {
    if (typeof chunk === 'string')
      str += chunk;
    else
      str += dec.decode(chunk, { stream: true });
  }
  // Flush the streaming TextDecoder so that any pending
  // incomplete multibyte characters are handled.
  str += dec.decode(undefined, { stream: false });
  return str;
}

/**
 * @param {AsyncIterable|ReadableStream|Readable} stream
 * @returns {Promise<any>}
 */
Repository: nodejs/node
File: test/parallel/test-stream-consumers.js (JavaScript)

Code before:

  arrayBuffer(passthrough).then(common.mustCall(async (ab) => {
    assert.strictEqual(ab.byteLength, 10);
    assert.deepStrictEqual(ab, kArrayBuffer);
  }));

  passthrough.write('hello');
  setTimeout(() => passthrough.end('there'), 10);
}

{
  const passthrough = new PassThrough();

  buffer(passthrough).then(common.mustCall(async (buf) => {
    assert.strictEqual(buf.byteLength, 10);
    assert.deepStrictEqual(buf.buffer, kArrayBuffer);
  }));

  passthrough.write('hello');
  setTimeout(() => passthrough.end('there'), 10);
}

{
  const passthrough = new PassThrough();

  bytes(passthrough).then(common.mustCall(async (uint8arr) => {
    assert(uint8arr instanceof Uint8Array);
    assert.strictEqual(uint8arr.byteLength, 10);
    assert.deepStrictEqual(Buffer.from(uint8arr), buf);
  }));

  passthrough.write('hello');
  setTimeout(() => passthrough.end('there'), 10);
}

{
  const passthrough = new PassThrough();

  text(passthrough).then(common.mustCall(async (str) => {
    assert.strictEqual(str.length, 10);
    assert.strictEqual(str, 'hellothere');
  }));

  passthrough.write('hello');
  setTimeout(() => passthrough.end('there'), 10);
}

{
  const readable = new Readable({
    read() {}
  });

Review comment (suggestion):
```suggestion
  bytes(passthrough).then(common.mustCall((uint8arr) => {
```

I hope you don't mind my applying this trivial change, which is needed to satisfy lint-ci after e50cbc1abd41d4a47d60bfabd12cf72833096cbc.

Code after:

  arrayBuffer(passthrough).then(common.mustCall(async (ab) => {
    assert.strictEqual(ab.byteLength, 10);
    assert.deepStrictEqual(ab, kArrayBuffer);
  }));

  passthrough.write('hello');
  setTimeout(() => passthrough.end('there'), 10);
}

{
  const passthrough = new PassThrough();

  buffer(passthrough).then(common.mustCall(async (buf) => {
    assert.strictEqual(buf.byteLength, 10);
    assert.deepStrictEqual(buf.buffer, kArrayBuffer);
  }));

  passthrough.write('hello');
  setTimeout(() => passthrough.end('there'), 10);
}

{
  const passthrough = new PassThrough();

  bytes(passthrough).then(common.mustCall((uint8arr) => {
    assert(uint8arr instanceof Uint8Array);
    assert.strictEqual(uint8arr.byteLength, 10);
    assert.deepStrictEqual(Buffer.from(uint8arr), buf);
  }));

  passthrough.write('hello');
  setTimeout(() => passthrough.end('there'), 10);
}

{
  const passthrough = new PassThrough();

  text(passthrough).then(common.mustCall(async (str) => {
    assert.strictEqual(str.length, 10);
    assert.strictEqual(str, 'hellothere');
  }));

  passthrough.write('hello');
  setTimeout(() => passthrough.end('there'), 10);
}

{
  const readable = new Readable({
    read() {}
  });
Repository: nodejs/node
File: test/parallel/test-stream-readable-readable-one.js (JavaScript)

Code before:
'use strict';
const common = require('../common');
const assert = require('assert');

const { Readable } = require('stream');

// Read one buffer at a time and don't waste cycles allocating
// and copying into a new larger buffer.
{
  const r = new Readable({
    read() {}
  });
  const buffers = [Buffer.allocUnsafe(5), Buffer.allocUnsafe(10)];
  for (const buf of buffers) {
    r.push(buf);
  }
  for (const buf of buffers) {
    assert.strictEqual(r.read(), buf);
  }
}


Review comment (suggestion):
```suggestion
require('../common');
```

I would also rename the file to something like `test-stream-readable-read-one.js`

Code after:
'use strict';
require('../common');
const assert = require('assert');

const { Readable } = require('stream');

// Read one buffer at a time and don't waste cycles allocating
// and copying into a new larger buffer.
{
  const r = new Readable({
    read() {}
  });
  const buffers = [Buffer.allocUnsafe(5), Buffer.allocUnsafe(10)];
  for (const buf of buffers) {
    r.push(buf);
  }
  for (const buf of buffers) {
    assert.strictEqual(r.read(), buf);
  }
}