ronantakizawa/codereview-bench huggingface dataset:
Repository: awslabs/llrt
File: src/fs/mkdir.rs (Rust)

Code before:
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0
use std::os::unix::prelude::PermissionsExt;

use ring::rand::{SecureRandom, SystemRandom};
use rquickjs::{function::Opt, Ctx, Object, Result};
use tokio::fs;

use crate::utils::result::ResultExt;

pub async fn mkdir<'js>(ctx: Ctx<'js>, path: String, options: Opt<Object<'js>>) -> Result<String> {
    let (recursive, mode) = get_mkdir_params(options);

    if recursive {
        fs::create_dir_all(&path).await
    } else {
        fs::create_dir(&path).await
    }
    .or_throw_msg(
        &ctx,
        &format!(
            "EEXIST: file already exists, mkdir. Can't create dir \"{}\"",
            &path
        ),
    )?;

    fs::set_permissions(&path, PermissionsExt::from_mode(mode))
        .await
        .or_throw_msg(&ctx, &format!("Can't set permissions of \"{}\"", &path))?;

    Ok(path)
}

pub fn mkdirsync<'js>(ctx: Ctx<'js>, path: String, options: Opt<Object<'js>>) -> Result<String> {
    let (recursive, mode) = get_mkdir_params(options);

    if recursive {
        std::fs::create_dir_all(&path)
    } else {
        std::fs::create_dir(&path)
    }
    .or_throw_msg(
        &ctx,
        &format!(
            "EEXIST: file already exists, mkdir. Can't create dir \"{}\"",
            &path
        ),

Review comment (suggestion):
`or_throw_msg` captures the original error string and appends message, so no need to add EEXISTS (besides it can be other errors)

Code after:
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0
use std::os::unix::prelude::PermissionsExt;

use ring::rand::{SecureRandom, SystemRandom};
use rquickjs::{function::Opt, Ctx, Object, Result};
use tokio::fs;

use crate::utils::result::ResultExt;

pub async fn mkdir<'js>(ctx: Ctx<'js>, path: String, options: Opt<Object<'js>>) -> Result<String> {
    let (recursive, mode) = get_mkdir_params(options);

    if recursive {
        fs::create_dir_all(&path).await
    } else {
        fs::create_dir(&path).await
    }
    .or_throw_msg(&ctx, &format!("Can't create dir \"{}\"", &path))?;

    fs::set_permissions(&path, PermissionsExt::from_mode(mode))
        .await
        .or_throw_msg(&ctx, &format!("Can't set permissions of \"{}\"", &path))?;

    Ok(path)
}

pub fn mkdirsync<'js>(ctx: Ctx<'js>, path: String, options: Opt<Object<'js>>) -> Result<String> {
    let (recursive, mode) = get_mkdir_params(options);

    if recursive {
        std::fs::create_dir_all(&path)
    } else {
        std::fs::create_dir(&path)
    }
    .or_throw_msg(&ctx, &format!("Can't create dir \"{}\"", &path))?;

    std::fs::set_permissions(&path, PermissionsExt::from_mode(mode))
        .or_throw_msg(&ctx, &format!("Can't set permissions of \"{}\"", &path))?;

    Ok(path)
}

fn get_mkdir_params(options: Opt<Object>) -> (bool, u32) {
    let mut recursive = false;
    let mut mode = 0o777;
Repository: awslabs/llrt
File: src/utils/io.rs (Rust)

Code before:
        let mut stream = fs::read_dir(dir).await?;
        while let Some(entry) = stream.next_entry().await? {
            let entry_path = entry.path();

            let name = entry.file_name().to_string_lossy().to_string();
            if !(self.filter)(&name) {
                continue;
            }

            let metadata = fs::metadata(&entry_path).await?;
            self.stack.push((entry_path, Some(metadata)));
        }
        Ok(())
    }

    fn append_stack_sync(&mut self, dir: &PathBuf) -> io::Result<()> {
        let dir = std::fs::read_dir(dir)?;

        for entry in dir.flatten() {
            let name = entry.file_name().to_string_lossy().to_string();
            if !(self.filter)(&name) {
                continue;
            }
            let entry_path = entry.path();
            let metadata = entry.metadata();
            match metadata {
                Ok(metadata) => self.stack.push((entry_path, Some(metadata))),
                Err(..) => self.stack.push((entry_path, None)),
            }
        }

        Ok(())
    }
}


Review comment (bug):
Why match here? Can we use `metadata()?;`? Or is it so that we check the rest of the dirs if we have a failture? If that's the case, it should be reflected in async version as well and we can simply do `continue` instead of pushing to stack right?

Code after:
        let mut stream = fs::read_dir(dir).await?;
        while let Some(entry) = stream.next_entry().await? {
            let entry_path = entry.path();

            let name = entry.file_name().to_string_lossy().to_string();
            if !(self.filter)(&name) {
                continue;
            }

            let metadata = fs::metadata(&entry_path).await?;
            self.stack.push((entry_path, Some(metadata)));
        }
        Ok(())
    }

    fn append_stack_sync(&mut self, dir: &PathBuf) -> io::Result<()> {
        let dir = std::fs::read_dir(dir)?;

        for entry in dir.flatten() {
            let name = entry.file_name().to_string_lossy().to_string();
            if !(self.filter)(&name) {
                continue;
            }
            let entry_path = entry.path();
            let metadata = entry.metadata()?;
            self.stack.push((entry_path, Some(metadata)))
        }

        Ok(())
    }
}
Repository: awslabs/llrt
File: example/infrastructure/src/index.ts (TypeScript)

Code before:
      code: aws_lambda.Code.fromAsset(sourceDirs["hello.mjs"]),
      ...props,
      environment: {
        AWS_LAMBDA_EXEC_WRAPPER: '/opt/bootstrap'
      },
      runtime: aws_lambda.Runtime.NODEJS_20_X,
      layers: [llrtLayer],
    }
  );

  // Node hello
  const helloNode20Function = new aws_lambda.Function(
    stack,
    "HelloNode20Function",
    {
      functionName: "example-hello-node20",
      code: aws_lambda.Code.fromAsset(sourceDirs["hello.mjs"]),
      ...props,
      environment: {},
    }
  );

  const helloNode18Function = new aws_lambda.Function(
    stack,
    "HelloNode18Function",
    {
      functionName: "example-hello-node18",
      code: aws_lambda.Code.fromAsset(sourceDirs["hello.mjs"]),
      ...props,
      runtime: aws_lambda.Runtime.NODEJS_18_X,
      environment: {},
    }
  );

  const helloNode16Function = new aws_lambda.Function(
    stack,
    "HelloNode16Function",
    {
      functionName: "example-hello-node16",
      code: aws_lambda.Code.fromAsset(sourceDirs["hello.mjs"]),
      ...props,
      runtime: aws_lambda.Runtime.NODEJS_16_X,
      environment: {},
    }
  );

  // Node 20, provided "aws-sdk"
  const v2Function = new aws_lambda_nodejs.NodejsFunction(stack, "V2", {
    functionName: "example-v2",
    entry: "../functions/src/v2.mjs",
    ...props,

Review comment (bug):
This handler is also currently broken. Downgrading it to using the Node 16 runtime is not sufficient, as the v2 `aws-sdk` doesn't play nice with ESM. We can either update the runtime version and import statement in `v2.mjs` to something like `'aws-sdk/lib/aws.js'`, or just remove this example

Code after:
  const helloNode18Function = new aws_lambda.Function(
    stack,
    "HelloNode18Function",
    {
      functionName: "example-hello-node18",
      code: aws_lambda.Code.fromAsset(sourceDirs["hello.mjs"]),
      ...props,
      runtime: aws_lambda.Runtime.NODEJS_18_X,
      environment: {},
    }
  );

  const helloNode16Function = new aws_lambda.Function(
    stack,
    "HelloNode16Function",
    {
      functionName: "example-hello-node16",
      code: aws_lambda.Code.fromAsset(sourceDirs["hello.mjs"]),
      ...props,
      runtime: aws_lambda.Runtime.NODEJS_16_X,
      environment: {},
    }
  );

  // Node 16, provided "aws-sdk"
  const v2Function = new aws_lambda_nodejs.NodejsFunction(stack, "V2", {
    functionName: "example-v2",
    entry: "../functions/src/v2.js",
    ...props,
    runtime: aws_lambda.Runtime.NODEJS_16_X,
    bundling: {
      ...props.bundling,
      externalModules: ["aws-sdk"],
    },
  });

  // Node 20, aws-sdk-v3, DynamoDBClient.send API, bundled in
  const v3BundledFunction = new aws_lambda_nodejs.NodejsFunction(
    stack,
    "V3Bundled",
    {
      functionName: "example-v3-bundled",
      entry: "../functions/src/v3.mjs",
      ...props,
      bundling: {
        ...props.bundling,
        externalModules: [],
      },
    }
  );
Repository: awslabs/llrt
File: src/fs/read_file.rs (Rust)

Code before:
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0
use rquickjs::{function::Opt, Ctx, Object, Result};
use tokio::fs;

use crate::{buffer::Buffer, utils::result::ResultExt};

//TODO implement options
pub async fn read_file<'js>(
    ctx: Ctx<'js>,
    path: String,
    _options: Opt<Object<'js>>,
) -> Result<Buffer> {
    let bytes = fs::read(path.clone())
        .await
        .or_throw_msg(&ctx, &format!("Can't read \"{}\"", &path))?;
    Ok(Buffer(bytes))
}


Review comment (suggestion):
I could use `Result::ok` here, but I prefer to use `Result::map` to raise any error instead of ignoring it..

Code after:
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0
use rquickjs::{function::Opt, Ctx, IntoJs, Result, Value};
use tokio::fs;

use crate::{
    buffer::Buffer,
    utils::{object::ObjectExt, result::ResultExt},
};

//TODO implement options
pub async fn read_file<'js>(
    ctx: Ctx<'js>,
    path: String,
    options: Opt<Value<'js>>,
) -> Result<Value<'js>> {
    let bytes = fs::read(path.clone())
        .await
        .or_throw_msg(&ctx, &format!("Can't read \"{}\"", &path))?;
    let buffer = Buffer(bytes);

    if let Some(options) = options.0 {
        let encoding = if options.is_string() {
            options.as_string().unwrap().to_string().map(Some)?
        } else {
            options.get_optional::<_, String>("encoding")?
        };

        if let Some(encoding) = encoding {
            return buffer
                .to_string(&ctx, &encoding)
                .and_then(|s| s.into_js(&ctx));
        }
    }

    buffer.into_js(&ctx)
}
Repository: awslabs/llrt
File: src/url.rs (Rust)

Code before:
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0
use rquickjs::{
    module::{Declarations, Exports, ModuleDef},
    Class, Ctx, Result,
};

use crate::{
    http::{url::URL, url_search_params::URLSearchParams},
    module::export_default,
};
pub struct UrlModule;

impl ModuleDef for UrlModule {
    fn declare(declare: &mut Declarations) -> Result<()> {
        declare.declare("URL")?;
        declare.declare("URLSearchParams")?;
        declare.declare("default")?;
        Ok(())
    }

    fn evaluate<'js>(ctx: &Ctx<'js>, exports: &mut Exports<'js>) -> Result<()> {
        let url_constructor =
            Class::<URL>::create_constructor(ctx)?.expect("Could note create URL constructor");

        let url_search_params_constructor = Class::<URLSearchParams>::create_constructor(ctx)?
            .expect("Could note create URLSearchParams constructor");

        export_default(ctx, exports, |default| {
            default.set("URL", url_constructor.clone())?;
            default.set("URLSearchParams", url_search_params_constructor.clone())?;
            Ok(())
        })
    }
}


Review comment (suggestion):
Use  `Class::<URL>::define(default)?;` and ` Class::<URLSearchParams>::define(default)?;` inside export default instead of manually creating constructors.

Code after:
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0
use rquickjs::{
    function::Constructor,
    module::{Declarations, Exports, ModuleDef},
    Ctx, Result,
};

use crate::module::export_default;
pub struct UrlModule;

impl ModuleDef for UrlModule {
    fn declare(declare: &mut Declarations) -> Result<()> {
        declare.declare(stringify!(URL))?;
        declare.declare(stringify!(URLSearchParams))?;

        declare.declare("default")?;
        Ok(())
    }

    fn evaluate<'js>(ctx: &Ctx<'js>, exports: &mut Exports<'js>) -> Result<()> {
        let globals = ctx.globals();
        let url: Constructor = globals.get(stringify!(URL))?;
        let url_search_params: Constructor = globals.get(stringify!(URLSearchParams))?;

        export_default(ctx, exports, |default| {
            default.set(stringify!(URL), url)?;
            default.set(stringify!(URLSearchParams), url_search_params)?;
            Ok(())
        })?;

        Ok(())
    }
}
Repository: awslabs/llrt
File: src/performance.rs (Rust)

Code before:
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0
use rquickjs::{
    module::{Declarations, Exports, ModuleDef},
    prelude::Func,
    Ctx, Object, Result, Value,
};

use crate::module::export_default;

use crate::STARTED;

use once_cell::sync::Lazy;

use chrono::Utc;

static TIME_ORIGIN: Lazy<f64> = Lazy::new(|| (Utc::now().timestamp_micros() as f64) / 1e3);

fn get_time_origin() -> f64 {
    *TIME_ORIGIN
}

fn now() -> f64 {
    let started = unsafe { STARTED.assume_init() };
    let elapsed = started.elapsed();

    elapsed.as_micros() as f64 / 1e3
}

pub fn init(ctx: &Ctx<'_>) -> Result<()> {
    let globals = ctx.globals();

    let performance = Object::new(ctx.clone())?;

    performance.set("timeOrigin", get_time_origin())?;
    performance.set("now", Func::from(now))?;

    globals.set("performance", performance)?;

    Ok(())
}


Review comment (refactor):
Can we reuse the same global variable we init in https://github.com/awslabs/llrt/blob/589900a7430cd496e6a96857ed91a598d6495de1/src/main.rs#L72C16-L72C23 and use in https://github.com/awslabs/llrt/blob/589900a7430cd496e6a96857ed91a598d6495de1/src/process.rs#L45

Maybe we should move this variable to `vm.rs ` as well

Code after:
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0
use std::sync::atomic::Ordering;

use rquickjs::{
    module::{Declarations, Exports, ModuleDef},
    prelude::Func,
    Ctx, Object, Result, Value,
};

use crate::module::export_default;

use chrono::Utc;

use crate::vm::TIME_ORIGIN;

fn get_time_origin() -> f64 {
    let time_origin = TIME_ORIGIN.load(Ordering::Relaxed) as f64;

    time_origin / 1e6
}

fn now() -> f64 {
    let now = Utc::now().timestamp_nanos_opt().unwrap_or_default() as f64;
    let started = TIME_ORIGIN.load(Ordering::Relaxed) as f64;

    (now - started) / 1e6
}

pub fn init(ctx: &Ctx<'_>) -> Result<()> {
    let globals = ctx.globals();

    let performance = Object::new(ctx.clone())?;

    performance.set("timeOrigin", get_time_origin())?;
    performance.set("now", Func::from(now))?;

    globals.set("performance", performance)?;

    Ok(())
}
Repository: awslabs/llrt
File: src/http/url.rs (Rust)

Code before:
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0
use rquickjs::{
    atom::PredefinedAtom,
    class::{Trace, Tracer},
    function::Opt,
    prelude::This,
    Class, Coerced, Ctx, Exception, FromJs, Function, Result, Value,
};
use url::Url;

use crate::utils::result::ResultExt;

use super::url_search_params::URLSearchParams;

#[derive(Clone)]
#[rquickjs::class]
pub struct URL<'js> {
    protocol: String,
    host: String,
    hostname: String,
    port: String,
    pathname: String,
    hash: String,
    search_params: Class<'js, URLSearchParams>,
    username: String,
    password: String,
}

impl<'js> Trace<'js> for URL<'js> {
    fn trace<'a>(&self, tracer: Tracer<'a, 'js>) {
        self.search_params.trace(tracer);
    }
}

#[rquickjs::methods(rename_all = "camelCase")]
impl<'js> URL<'js> {
    #[qjs(constructor)]
    pub fn new(ctx: Ctx<'js>, input: Value<'js>, base: Opt<Value<'js>>) -> Result<Self> {
        if let Some(base) = base.0 {
            let base_string = get_string(&ctx, base)?;
            let path_string = get_string(&ctx, input)?;

Review comment (suggestion):
Using the url crate and parsing the URLs shouldn't be required. :thinking: 

Reference:
https://github.com/servo/rust-url/blob/73803fa780c8890e0b40cd01fbbd5362fbe4c9e0/url/tests/setters_tests.json#L764

Code after:
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0
use rquickjs::{
    atom::PredefinedAtom,
    class::{Trace, Tracer},
    function::Opt,
    prelude::This,
    Class, Coerced, Ctx, Exception, FromJs, Function, Result, Value,
};
use url::Url;

use crate::utils::result::ResultExt;

use super::url_search_params::URLSearchParams;

static DEFAULT_PORTS: &[&str] = &["21", "80", "443"];
static DEFAULT_PROTOCOLS: &[&str] = &["ftp", "http", "https"];

#[derive(Clone)]
#[rquickjs::class]
pub struct URL<'js> {
    protocol: String,
    host: String,
    hostname: String,
    port: String,
    pathname: String,
    hash: String,
    search_params: Class<'js, URLSearchParams>,
    username: String,
    password: String,
}

impl<'js> Trace<'js> for URL<'js> {
    fn trace<'a>(&self, tracer: Tracer<'a, 'js>) {
        self.search_params.trace(tracer);
    }
}

#[rquickjs::methods(rename_all = "camelCase")]
impl<'js> URL<'js> {
    #[qjs(constructor)]
    pub fn new(ctx: Ctx<'js>, input: Value<'js>, base: Opt<Value<'js>>) -> Result<Self> {
Repository: awslabs/llrt
File: src/exceptions.rs (Rust)

Code before:
        self.name.clone()
    }

    #[qjs(rename = PredefinedAtom::ToString)]
    pub fn to_string(&self) -> String {
        let mut message = String::new();

        if !self.message.is_empty() {
            message = format!(": {}", &self.message)
        }

        format!("{}{}", &self.name, message)
    }
}

pub fn init(ctx: &Ctx<'_>) -> Result<()> {
    let globals = ctx.globals();

    Class::<DOMException>::define(&globals)?;

    Ok(())
}

pub struct ExceptionsModule;

impl ModuleDef for ExceptionsModule {
    fn declare(declare: &mut Declarations) -> Result<()> {
        declare.declare("default")?;

        Ok(())
    }

    fn evaluate<'js>(ctx: &Ctx<'js>, exports: &mut Exports<'js>) -> Result<()> {
        Class::<DOMException>::register(ctx)?;

        export_default(ctx, exports, |default| {
            Class::<DOMException>::define(default)?;

            Ok(())
        })
    }
}


Review comment (suggestion):
I don't think `DomException` should be an importable module? It would be enough to have it at global scope.

Code after:
    }

    #[qjs(get)]
    fn name(&self) -> String {
        self.name.clone()
    }

    #[qjs(get)]
    fn stack(&self) -> String {
        self.stack.clone()
    }

    #[qjs(rename = PredefinedAtom::ToString)]
    pub fn to_string(&self) -> String {
        if self.message.is_empty() {
            return self.name.clone();
        }

        format!("{}: {}", &self.name, &self.message)
    }
}

pub fn init(ctx: &Ctx<'_>) -> Result<()> {
    let globals = ctx.globals();
    Class::<DOMException>::define(&globals)?;

    let dom_ex_proto = Class::<DOMException>::prototype(ctx.clone()).unwrap();
    let error_ctor: Object = globals.get(PredefinedAtom::Error)?;
    let error_proto = error_ctor.get_prototype();
    dom_ex_proto.set_prototype(error_proto.as_ref())?;

    Ok(())
}
Repository: awslabs/llrt
File: src/exceptions.rs (Rust)

Code before:
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0
use rquickjs::{
    atom::PredefinedAtom,
    function::Opt,
    module::{Declarations, Exports, ModuleDef},
    Class, Ctx, Result,
};

use crate::module::export_default;

#[derive(Clone, Default)]
#[rquickjs::class]
#[derive(rquickjs::class::Trace)]
pub struct DOMException {
    message: String,
    name: String,
}

#[rquickjs::methods]
impl<'js> DOMException {
    #[qjs(constructor)]
    pub fn new(_ctx: Ctx<'js>, message: Opt<String>, name: Opt<String>) -> Result<Self> {
        Ok(Self {
            message: message.0.unwrap_or(String::from("")),
            name: name.0.unwrap_or(String::from("Error")),
        })
    }

    #[qjs(get, rename = "message")]
    fn get_message(&self) -> String {
        self.message.clone()
    }

    #[qjs(get, rename = "name")]
    pub fn get_name(&self) -> String {
        self.name.clone()
    }

    #[qjs(rename = PredefinedAtom::ToString)]
    pub fn to_string(&self) -> String {
        let mut message = String::new();

        if !self.message.is_empty() {
            message = format!(": {}", &self.message)
        }

Review comment (suggestion):
I think we might need to inherit from `Error` in order for stack traces to be populated. Please verify that stacks are populated if throwing.

Code after:
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0
use rquickjs::{
    atom::PredefinedAtom,
    function::{Constructor, Opt},
    Class, Ctx, Object, Result,
};

#[rquickjs::class]
#[derive(rquickjs::class::Trace)]
struct DOMException {
    message: String,
    name: String,
    stack: String,
}

#[rquickjs::methods]
impl DOMException {
    #[qjs(constructor)]
    fn new(ctx: Ctx<'_>, message: Opt<String>, name: Opt<String>) -> Result<Self> {
        let error_ctor: Constructor = ctx.globals().get(PredefinedAtom::Error)?;
        let new: Object = error_ctor.construct((message.clone(),))?;

        let var_message = message.0.unwrap_or(String::from(""));
        let var_name = name.0.unwrap_or(String::from("Error"));

        Ok(Self {
            message: var_message,
            name: var_name,
            stack: new.get::<_, String>(PredefinedAtom::Stack)?,
        })
    }

    #[qjs(get)]
    fn message(&self) -> String {
        self.message.clone()
    }

    #[qjs(get)]
    fn name(&self) -> String {
        self.name.clone()
    }

    #[qjs(get)]
    fn stack(&self) -> String {
        self.stack.clone()