ronantakizawa/codereview-bench huggingface dataset:
Repository: awslabs/llrt
File: llrt_core/src/modules/http/request.rs (Rust)

Code before:
    body: Option<Value<'js>>,
    content_type: Option<String>,
    signal: Option<Class<'js, AbortSignal<'js>>>,
}

impl<'js> Trace<'js> for Request<'js> {
    fn trace<'a>(&self, tracer: rquickjs::class::Tracer<'a, 'js>) {
        if let Some(headers) = &self.headers {
            headers.trace(tracer);
        }
        if let Some(body) = &self.body {
            body.trace(tracer);
        }
    }
}

#[rquickjs::methods(rename_all = "camelCase")]
impl<'js> Request<'js> {
    #[qjs(constructor)]
    pub fn new(ctx: Ctx<'js>, input: Value<'js>, options: Opt<Object<'js>>) -> Result<Self> {
        let mut request = Self {
            url: String::from(""),
            method: "GET".to_string(),
            headers: None,
            body: None,
            content_type: None,
            signal: None,
        };

        if input.is_string() {
            request.url = input.get()?;
        } else if input.is_object() {
            assign_request(&mut request, ctx.clone(), input.as_object().unwrap())?;
        }
        if let Some(options) = options.0 {
            assign_request(&mut request, ctx.clone(), &options)?;
        }
        if request.headers.is_none() {
            let headers = Class::instance(ctx, Headers::default())?;
            request.headers = Some(headers);
        }

        Ok(request)
    }

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

    #[qjs(get)]

Review comment (suggestion):
Content-type needs to be used when making the fetch call. Make sure it sets the content type header on request before headers are used as headers should take precedence if specified

Code after:
    body: Option<Value<'js>>,
    signal: Option<Class<'js, AbortSignal<'js>>>,
}

impl<'js> Trace<'js> for Request<'js> {
    fn trace<'a>(&self, tracer: rquickjs::class::Tracer<'a, 'js>) {
        if let Some(headers) = &self.headers {
            headers.trace(tracer);
        }
        if let Some(body) = &self.body {
            body.trace(tracer);
        }
    }
}

#[rquickjs::methods(rename_all = "camelCase")]
impl<'js> Request<'js> {
    #[qjs(constructor)]
    pub fn new(ctx: Ctx<'js>, input: Value<'js>, options: Opt<Object<'js>>) -> Result<Self> {
        let mut request = Self {
            url: String::from(""),
            method: "GET".to_string(),
            headers: None,
            body: None,
            signal: None,
        };

        if input.is_string() {
            request.url = input.get()?;
        } else if input.is_object() {
            assign_request(&mut request, ctx.clone(), input.as_object().unwrap())?;
        }
        if let Some(options) = options.0 {
            assign_request(&mut request, ctx.clone(), &options)?;
        }
        if request.headers.is_none() {
            let headers = Class::instance(ctx, Headers::default())?;
            request.headers = Some(headers);
        }

        Ok(request)
    }

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

    #[qjs(get)]
    fn method(&self) -> String {
        self.method.clone()
Repository: awslabs/llrt
File: llrt_core/src/modules/http/request.rs (Rust)

Code before:
    }

    if obj.contains_key("body").unwrap_or_default() {
        let body: Value = obj.get("body").unwrap();
        if !body.is_undefined() && !body.is_null() {
            if let "GET" | "HEAD" = request.method.as_str() {
                return Err(Exception::throw_type(
                    &ctx,
                    "Failed to construct 'Request': Request with GET/HEAD method cannot have body.",
                ));
            }

            match get_class::<Blob>(&body)? {
                Some(blob) => {
                    let blob = blob.borrow();
                    request.body =
                        Some(TypedArray::<u8>::new(ctx.clone(), blob.get_bytes())?.into_value());
                    request.content_type = match blob.mime_type().len() {
                        0 => None,
                        _ => Some(blob.mime_type()),
                    };
                },
                None => {
                    request.body = Some(body);
                    request.content_type = None;
                },
            }
        }
    }

    if obj.contains_key("headers").unwrap() {
        let headers: Value = obj.get("headers")?;
        let headers = Headers::from_value(&ctx, headers)?;
        if request.content_type.is_none() {
            request.content_type = headers
                .iter()
                .find(|(k, _)| k == &"content-type")
                .map(|(_, v)| v.clone());
        }
        let headers = Class::instance(ctx, headers)?;
        request.headers = Some(headers);
    }

    Ok(())
}


Review comment (suggestion):
This can be implemented like this instead:
```rust
if let Some(blob) = get_class::<Blob>(&body)? {
    let blob = blob.borrow();
    request.body = Some(TypedArray::<u8>::new(ctx.clone(), blob.get_bytes())?.into_value());
    request.content_type = if blob.mime_type().is_empty() {
        None
    } else {
        Some(blob.mime_type())
    };
} else {
    request.body = Some(body);
    request.content_type = None;
}
```

Code after:
        }
    }

    if let Some(body) = obj.get_optional::<_, Value>("body")? {
        if !body.is_undefined() && !body.is_null() {
            if let "GET" | "HEAD" = request.method.as_str() {
                return Err(Exception::throw_type(
                    &ctx,
                    "Failed to construct 'Request': Request with GET/HEAD method cannot have body.",
                ));
            }

            request.body = if let Some(blob) = get_class::<Blob>(&body)? {
                let blob = blob.borrow();
                Some(TypedArray::<u8>::new(ctx.clone(), blob.get_bytes())?.into_value())
            } else {
                Some(body)
            }
        }
    }

    if let Some(headers) = obj.get_optional("headers")? {
        let headers = Headers::from_value(&ctx, headers)?;
        let headers = Class::instance(ctx, headers)?;
        request.headers = Some(headers);
    }

    Ok(())
}
Repository: awslabs/llrt
File: llrt_core/src/modules/encoding/text_decoder.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, Exception, FromJs, Object, Result, Value};

use std::borrow::Cow;

use crate::utils::object::get_bytes;
use encoding_rs::Encoding;

#[rquickjs::class]
#[derive(rquickjs::class::Trace)]
pub struct TextDecoder {
    #[qjs(skip_trace)]
    encoding: String,
    fatal: bool,
    ignore_bom: bool,
}

#[rquickjs::methods]
impl<'js> TextDecoder {
    #[qjs(constructor)]
    pub fn new(ctx: Ctx<'js>, label: Opt<String>, options: Opt<Object<'js>>) -> Result<Self> {
        let label = match label.0 {
            Some(lbl) if !lbl.is_empty() => lbl,
            Some(_) | None => String::from("utf-8"),
        };
        let mut fatal = false;
        let mut ignore_bom = false;

        let encoding = match Encoding::for_label(label.as_bytes()) {
            Some(enc) => enc.name().to_string(),
            None => return Err(Exception::throw_type(&ctx, "Unsupported encoding label")),
        };

        if let Some(opt) = get_option::<bool>("fatal", &options)? {
            fatal = opt;
        }
        if let Some(opt) = get_option::<bool>("ignoreBOM", &options)? {
            ignore_bom = opt;
        }

        Ok(TextDecoder {
            encoding,
            fatal,
            ignore_bom,
        })
    }


Review comment (suggestion):
```rust
let label = label.0
    .filter(|lbl| !lbl.is_empty())
    .unwrap_or_else(|| String::from("utf-8"));
```

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

use std::borrow::Cow;

use crate::utils::{object::get_bytes, object::ObjectExt, result::ResultExt};
use encoding_rs::Encoding;

#[rquickjs::class]
#[derive(rquickjs::class::Trace)]
pub struct TextDecoder {
    #[qjs(skip_trace)]
    encoding: String,
    fatal: bool,
    ignore_bom: bool,
}

#[rquickjs::methods]
impl<'js> TextDecoder {
    #[qjs(constructor)]
    pub fn new(ctx: Ctx<'js>, label: Opt<String>, options: Opt<Object<'js>>) -> Result<Self> {
        let label = label
            .0
            .filter(|lbl| !lbl.is_empty())
            .unwrap_or_else(|| String::from("utf-8"));
        let mut fatal = false;
        let mut ignore_bom = false;

        let encoding = Encoding::for_label(label.as_bytes())
            .map(|enc| enc.name().to_string())
            .or_throw_msg(&ctx, "Unsupported encoding label")?;

        if let Some(options) = options.0 {
            if let Some(opt) = options.get_optional("fatal")? {
                fatal = opt;
            }
            if let Some(opt) = options.get_optional("ignoreBOM")? {
                ignore_bom = opt;
            }
        }

        Ok(TextDecoder {
            encoding,
            fatal,
            ignore_bom,
        })
    }
Repository: awslabs/llrt
File: llrt_core/src/modules/encoding/text_decoder.rs (Rust)

Code before:
use std::borrow::Cow;

use crate::utils::object::get_bytes;
use encoding_rs::Encoding;

#[rquickjs::class]
#[derive(rquickjs::class::Trace)]
pub struct TextDecoder {
    #[qjs(skip_trace)]
    encoding: String,
    fatal: bool,
    ignore_bom: bool,
}

#[rquickjs::methods]
impl<'js> TextDecoder {
    #[qjs(constructor)]
    pub fn new(ctx: Ctx<'js>, label: Opt<String>, options: Opt<Object<'js>>) -> Result<Self> {
        let label = match label.0 {
            Some(lbl) if !lbl.is_empty() => lbl,
            Some(_) | None => String::from("utf-8"),
        };
        let mut fatal = false;
        let mut ignore_bom = false;

        let encoding = match Encoding::for_label(label.as_bytes()) {
            Some(enc) => enc.name().to_string(),
            None => return Err(Exception::throw_type(&ctx, "Unsupported encoding label")),
        };

        if let Some(opt) = get_option::<bool>("fatal", &options)? {
            fatal = opt;
        }
        if let Some(opt) = get_option::<bool>("ignoreBOM", &options)? {
            ignore_bom = opt;
        }

        Ok(TextDecoder {
            encoding,
            fatal,
            ignore_bom,
        })
    }

    #[qjs(get)]
    fn encoding(&self) -> String {
        let s = self.encoding.clone();
        s.replace('_', "-").to_ascii_lowercase()
    }

    #[qjs(get)]

Review comment (refactor):
Simplify:
```rust
let encoding = Encoding::for_label(label.as_bytes())
    .map(|enc| enc.name().to_string()).or_throw_message(&ctx,"Unsupported encoding label")
```

Code after:
use std::borrow::Cow;

use crate::utils::{object::get_bytes, object::ObjectExt, result::ResultExt};
use encoding_rs::Encoding;

#[rquickjs::class]
#[derive(rquickjs::class::Trace)]
pub struct TextDecoder {
    #[qjs(skip_trace)]
    encoding: String,
    fatal: bool,
    ignore_bom: bool,
}

#[rquickjs::methods]
impl<'js> TextDecoder {
    #[qjs(constructor)]
    pub fn new(ctx: Ctx<'js>, label: Opt<String>, options: Opt<Object<'js>>) -> Result<Self> {
        let label = label
            .0
            .filter(|lbl| !lbl.is_empty())
            .unwrap_or_else(|| String::from("utf-8"));
        let mut fatal = false;
        let mut ignore_bom = false;

        let encoding = Encoding::for_label(label.as_bytes())
            .map(|enc| enc.name().to_string())
            .or_throw_msg(&ctx, "Unsupported encoding label")?;

        if let Some(options) = options.0 {
            if let Some(opt) = options.get_optional("fatal")? {
                fatal = opt;
            }
            if let Some(opt) = options.get_optional("ignoreBOM")? {
                ignore_bom = opt;
            }
        }

        Ok(TextDecoder {
            encoding,
            fatal,
            ignore_bom,
        })
    }

    #[qjs(get)]
    fn encoding(&self) -> String {
        let s = self.encoding.clone();
        s.replace('_', "-").to_ascii_lowercase()
    }
Repository: awslabs/llrt
File: llrt_core/src/modules/encoding/text_encoder.rs (Rust)

Code before:
#[rquickjs::methods(rename_all = "camelCase")]
impl TextEncoder {
    #[qjs(constructor)]
    pub fn new() -> Self {
        Self {}
    }

    #[qjs(get)]
    fn encoding(&self) -> String {
        "utf-8".to_string()
    }

    pub fn encode<'js>(&self, ctx: Ctx<'js>, string: String) -> Result<Value<'js>> {
        TypedArray::new(ctx, string.as_bytes()).map(|m| m.into_value())
    }

    pub fn encode_into<'js>(
        &self,
        ctx: Ctx<'js>,
        string: String,
        obj: Object<'js>,
    ) -> Result<Object<'js>> {
        let mut read = 0;
        let mut written = 0;

        if let Some((array_buffer, source_length, source_offset)) = obj_to_array_buffer(&obj)? {
            let raw = array_buffer
                .as_raw()
                .ok_or("ArrayBuffer is detached")
                .or_throw(&ctx)?;

            let bytes = unsafe {
                std::slice::from_raw_parts_mut(raw.ptr.as_ptr().add(source_offset), source_length)
            };

            let mut enc = encoding_rs::UTF_8.new_encoder();
            (_, _, written, _) = enc.encode_from_utf8(string.as_str(), bytes, false);
            read = from_utf8(&bytes[..written]).unwrap().encode_utf16().count();
        }

        let obj = Object::new(ctx)?;
        obj.set("read", read)?;
        obj.set("written", written)?;
        Ok(obj)
    }
}


Review comment (suggestion):
I don't think you need to access the raw bytes like this.
You can do `let bytes: &[u8] = array_buffer.as_ref()` instead and use offsets on slice instead.

Code after:
impl TextEncoder {
    #[qjs(constructor)]
    pub fn new() -> Self {
        Self {}
    }

    #[qjs(get)]
    fn encoding(&self) -> String {
        "utf-8".to_string()
    }

    pub fn encode<'js>(&self, ctx: Ctx<'js>, string: String) -> Result<Value<'js>> {
        TypedArray::new(ctx, string.as_bytes()).map(|m| m.into_value())
    }

    pub fn encode_into<'js>(
        &self,
        ctx: Ctx<'js>,
        string: String,
        obj: Object<'js>,
    ) -> Result<Object<'js>> {
        let mut read = 0;
        let mut written = 0;

        if let Some((array_buffer, source_length, source_offset)) = obj_to_array_buffer(&obj)? {
            let raw = array_buffer
                .as_raw()
                .ok_or("ArrayBuffer is detached")
                .or_throw(&ctx)?;

            let bytes = unsafe {
                std::slice::from_raw_parts_mut(raw.ptr.as_ptr().add(source_offset), source_length)
            };

            let mut enc = encoding_rs::UTF_8.new_encoder();
            (_, _, written, _) = enc.encode_from_utf8(string.as_str(), bytes, false);
            read = string[..written]
                .chars()
                .fold(0, |acc, ch| acc + ch.len_utf16());
        }

        let obj = Object::new(ctx)?;
        obj.set("read", read)?;
        obj.set("written", written)?;
        Ok(obj)
    }
}
Repository: awslabs/llrt
File: llrt_modules/src/modules/fs/write_file.rs (Rust)

Code before:
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0

use llrt_utils::{bytes::ObjectBytes, result::ResultExt};
use rquickjs::{Ctx, Result};
use tokio::fs;
use tokio::io::AsyncWriteExt;

pub async fn write_file<'js>(ctx: Ctx<'js>, path: String, bytes: ObjectBytes<'js>) -> Result<()> {
    let mut file = fs::File::create(&path)
        .await
        .or_throw_msg(&ctx, &["Can't create file \"", &path, "\""].concat())?;

    let write_error_message = &["Can't write file \"", &path, "\""].concat();

    file.write_all(bytes.as_bytes())
        .await
        .or_throw_msg(&ctx, write_error_message)?;
    file.flush().await.or_throw_msg(&ctx, write_error_message)?;

    Ok(())
}

pub fn write_file_sync<'js>(ctx: Ctx<'js>, path: String, bytes: ObjectBytes<'js>) -> Result<()> {
    std::fs::write(&path, bytes.as_bytes())
        .or_throw_msg(&ctx, &["Can't write \"{}\"", &path].concat())?;

    Ok(())
}


Review comment (suggestion):
Here is an optimization not to accept ObjectBytes<'js> as we check if we can create the file first and then do the ObjectBytes calculation.  Also I saw now that we can reuse:
`let write_error_message = &["Can't write file \"", &path, "\""].concat();`

Code after:
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0

use llrt_utils::{bytes::ObjectBytes, result::ResultExt};
use rquickjs::{Ctx, Result, Value};
use tokio::fs;
use tokio::io::AsyncWriteExt;

pub async fn write_file<'js>(ctx: Ctx<'js>, path: String, data: Value<'js>) -> Result<()> {
    let write_error_message = &["Can't write file \"", &path, "\""].concat();

    let mut file = fs::File::create(&path)
        .await
        .or_throw_msg(&ctx, write_error_message)?;

    let bytes = ObjectBytes::from(&ctx, &data)?;
    file.write_all(bytes.as_bytes())
        .await
        .or_throw_msg(&ctx, write_error_message)?;
    file.flush().await.or_throw_msg(&ctx, write_error_message)?;

    Ok(())
}

pub fn write_file_sync<'js>(ctx: Ctx<'js>, path: String, bytes: ObjectBytes<'js>) -> Result<()> {
    std::fs::write(&path, bytes.as_bytes())
        .or_throw_msg(&ctx, &["Can't write \"{}\"", &path].concat())?;

    Ok(())
}
Repository: awslabs/llrt
File: modules/llrt_assert/src/lib.rs (Rust)

Code before:
        if let Some(err) = obj.as_exception() {
            return Err(err.clone().throw());
        }
    }

    Err(Exception::throw_message(
        &ctx,
        "The expression was evaluated to a falsy value",
    ))
}

pub struct AssertModule;

impl ModuleDef for AssertModule {
    fn declare(declare: &Declarations) -> Result<()> {
        declare.declare("ok")?;

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

    fn evaluate<'js>(ctx: &Ctx<'js>, exports: &Exports<'js>) -> Result<()> {
        export_default(ctx, exports, |default| {
            default.set("ok", Func::from(ok))?;

            Ok(())
        })
    }
}

impl From<AssertModule> for ModuleInfo<AssertModule> {
    fn from(val: AssertModule) -> Self {
        ModuleInfo {
            name: "assert",
            module: val,
        }
    }
}


Review comment (suggestion):
A more performant way would be to match on `value.type_of();` That way we don't have to cast and recheck for all types.

Code after:
        | Type::Constructor
        | Type::Exception
        | Type::Function
        | Type::Symbol
        | Type::Object => {
            return Ok(());
        },
        _ => {},
    }

    if let Some(obj) = message.0 {
        match obj.type_of() {
            Type::String => {
                let msg = obj.as_string().unwrap().to_string().unwrap();
                return Err(Exception::throw_message(&ctx, &msg));
            },
            Type::Exception => return Err(obj.as_exception().cloned().unwrap().throw()),
            _ => {},
        };
    }

    Err(Exception::throw_message(
        &ctx,
        "AssertionError: The expression was evaluated to a falsy value",
    ))
}

pub struct AssertModule;

impl ModuleDef for AssertModule {
    fn declare(declare: &Declarations) -> Result<()> {
        declare.declare("ok")?;

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

    fn evaluate<'js>(ctx: &Ctx<'js>, exports: &Exports<'js>) -> Result<()> {
        let ok_function = Function::new(ctx.clone(), ok)?.with_name("ok")?;
        ok_function.set("ok", ok_function.clone())?;

        exports.export("ok", ok_function.clone())?;
        exports.export("default", ok_function)?;
        Ok(())
    }
}

impl From<AssertModule> for ModuleInfo<AssertModule> {
    fn from(val: AssertModule) -> Self {
        ModuleInfo {
            name: "assert",
Repository: awslabs/llrt
File: modules/llrt_assert/src/lib.rs (Rust)

Code before:
}

pub struct AssertModule;

impl ModuleDef for AssertModule {
    fn declare(declare: &Declarations) -> Result<()> {
        declare.declare("ok")?;

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

    fn evaluate<'js>(ctx: &Ctx<'js>, exports: &Exports<'js>) -> Result<()> {
        export_default(ctx, exports, |default| {
            default.set("ok", Func::from(ok))?;

            Ok(())
        })
    }
}

impl From<AssertModule> for ModuleInfo<AssertModule> {
    fn from(val: AssertModule) -> Self {
        ModuleInfo {
            name: "assert",
            module: val,
        }
    }
}


Review comment (suggestion):
I think `assert` is also a function. So should basically export a function that also has additional properties, similar to this javascript code:
```javascript
function assert(...){
//body
}
assert.ok = assert
```
etc

Code after:

    if let Some(obj) = message.0 {
        match obj.type_of() {
            Type::String => {
                let msg = obj.as_string().unwrap().to_string().unwrap();
                return Err(Exception::throw_message(&ctx, &msg));
            },
            Type::Exception => return Err(obj.as_exception().cloned().unwrap().throw()),
            _ => {},
        };
    }

    Err(Exception::throw_message(
        &ctx,
        "AssertionError: The expression was evaluated to a falsy value",
    ))
}

pub struct AssertModule;

impl ModuleDef for AssertModule {
    fn declare(declare: &Declarations) -> Result<()> {
        declare.declare("ok")?;

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

    fn evaluate<'js>(ctx: &Ctx<'js>, exports: &Exports<'js>) -> Result<()> {
        let ok_function = Function::new(ctx.clone(), ok)?.with_name("ok")?;
        ok_function.set("ok", ok_function.clone())?;

        exports.export("ok", ok_function.clone())?;
        exports.export("default", ok_function)?;
        Ok(())
    }
}

impl From<AssertModule> for ModuleInfo<AssertModule> {
    fn from(val: AssertModule) -> Self {
        ModuleInfo {
            name: "assert",
            module: val,
        }
    }
}