ronantakizawa/codereview-bench huggingface dataset:
Repository: microsoft/TypeChat
File: examples/healthData/src/translator.ts (TypeScript)

Code before:

        const prompt = `
    user: You are a service that translates user requests into JSON objects of type "${typename}" according to the following TypeScript definitions:
    '''
    ${schema}
    '''

    user:
    Use precise date and times RELATIVE TO CURRENT DATE: ${now.toLocaleDateString()} CURRENT TIME: ${now.toTimeString().split(' ')[0]}
    Also turn ranges like next week and next month into precise dates
    
    user:
    ${_additionalAgentInstructions}
    
    system:
    IMPORTANT CONTEXT for the user request:
    ${historyStr}

    user:
    The following is a user request:
    '''
    ${intent}
    '''
    The following is the user request translated into a JSON object with 2 spaces of indentation and no properties with the value undefined:
    """
    `;
        return prompt;
    }
}


Review comment (suggestion):
```suggestion
user: You are a service that translates user requests into JSON objects of type "${typename}" according to the following TypeScript definitions:
'''
${schema}
'''

user:
Use precise date and times RELATIVE TO CURRENT DATE: ${now.toLocaleDateString()} CURRENT TIME: ${now.toTimeString().split(' ')[0]}
Also turn ranges like next week and next month into precise dates

user:
${_additionalAgentInstructions}

system:
IMPORTANT CONTEXT for the user request:
${historyStr}

user:
The following is a user request:
'''
${intent}
'''
The following is the user request translated into a JSON object with 2 spaces of indentation and no properties with the value undefined:
"""
`;
```

Code after:
        const now = new Date();

        const prompt = `
user: You are a service that translates user requests into JSON objects of type "${typename}" according to the following TypeScript definitions:
'''
${schema}
'''

user:
Use precise date and times RELATIVE TO CURRENT DATE: ${now.toLocaleDateString()} CURRENT TIME: ${now.toTimeString().split(' ')[0]}
Also turn ranges like next week and next month into precise dates

user:
${_additionalAgentInstructions}

system:
IMPORTANT CONTEXT for the user request:
${historyStr}

user:
The following is a user request:
'''
${intent}
'''
The following is the user request translated into a JSON object with 2 spaces of indentation and no properties with the value undefined:
"""
`;
        return prompt;
    }
}
Repository: microsoft/TypeChat
File: python/tests/generic_alias_1.py (Python)

Code before:
from typing import Literal, TypedDict, TypeVar, Generic
from typing_extensions import TypeAliasType

from typechat import python_type_to_typescript_schema

T = TypeVar("T", covariant=True)


class First(Generic[T], TypedDict):
    kind: Literal["first"]
    first_attr: T


class Second(Generic[T], TypedDict):
    kind: Literal["second"]
    second_attr: T


FirstOrSecond = TypeAliasType("FirstOrSecond", First[T] | Second[T], type_params=(T,))

result = python_type_to_typescript_schema(FirstOrSecond)

print("// Entry point is: '{result.typescript_type_reference}'")
print("// TypeScript Schema:\n")
print(result.typescript_schema_str)
if result.errors:
    print("// Errors:")
    for err in result.errors:
        print(f"// - {err}\n")


Review comment (suggestion):
Currently TypeChat only understands generics using 3.12 syntax. It doesn't consider types that extend `Generic[...]` or generic protocols as adding type parameters. So I think we either find a way to make some of these tests only run in 3.12, or temporarily disable them.

Code after:
from typing import Literal, TypedDict, TypeVar, Generic
from typing_extensions import TypeAliasType

from typechat import python_type_to_typescript_schema

T = TypeVar("T", covariant=True)


class First(Generic[T], TypedDict):
    kind: Literal["first"]
    first_attr: T


class Second(Generic[T], TypedDict):
    kind: Literal["second"]
    second_attr: T


FirstOrSecond = TypeAliasType("FirstOrSecond", First[T] | Second[T], type_params=(T,))

result = python_type_to_typescript_schema(FirstOrSecond)

print(f"// Entry point is: '{result.typescript_type_reference}'")
print("// TypeScript Schema:\n")
print(result.typescript_schema_str)
if result.errors:
    print("// Errors:")
    for err in result.errors:
        print(f"// - {err}\n")
Repository: microsoft/TypeChat
File: python/tests/test_coffeeshop.py (Python)

Code before:
from typechat import python_type_to_typescript_schema
from tests.coffeeshop import Cart

def test_python_to_typescript_schema():
    result = python_type_to_typescript_schema(Cart)
    assert result is not None
    assert result.typescript_type_reference == "Cart"
    assert result.typescript_schema_str is not None
    assert not result.errors


Review comment (suggestion):
So I'm not entirely familiar with it, but if we add syrupy, it looks like you can add a parameter called `snapshot` and you can just write

```suggestion
def test_coffeeshop(snapshot):
    result = python_type_to_typescript_schema(Cart)
    assert result == snapshot
```

Let me know how that works


Code after:
from typing_extensions import Literal, NotRequired, TypedDict, Annotated, Doc, Any
from typechat import python_type_to_typescript_schema

class UnknownText(TypedDict):
    """
    Represents any text that could not be understood.
    """

    type: Literal["UnknownText"]
    text: Annotated[str, Doc("The text that wasn't understood")]


class Caffeine(TypedDict):
    type: Literal["Caffeine"]
    name: Literal["regular", "two thirds caf", "half caf", "one third caf", "decaf"]


class Milk(TypedDict):
    type: Literal["Milk"]
    name: Literal[
        "whole milk", "two percent milk", "nonfat milk", "coconut milk", "soy milk", "almond milk", "oat milk"
    ]


class Creamer(TypedDict):
    type: Literal["Creamer"]
    name: Literal[
        "whole milk creamer",
        "two percent milk creamer",
        "one percent milk creamer",
        "nonfat milk creamer",
        "coconut milk creamer",
        "soy milk creamer",
        "almond milk creamer",
Repository: microsoft/edit
File: src/sys/unix.rs (Rust)

Code before:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

//! Unix-specific platform code.
//!
//! Read the `windows` module for reference.
//! TODO: This reminds me that the sys API should probably be a trait.

use std::ffi::{CStr, c_int, c_void};
use std::fs::{self, File};
use std::mem::{self, MaybeUninit};
use std::os::fd::{AsRawFd as _, FromRawFd as _};
use std::ptr::{self, NonNull, null_mut};
use std::{thread, time};

use crate::arena::{Arena, ArenaString, scratch_arena};
use crate::helpers::*;
use crate::{apperr, arena_format};

unsafe extern "C" {
    fn __error() -> *mut libc::c_int;
}

struct State {
    stdin: libc::c_int,
    stdin_flags: libc::c_int,
    stdout: libc::c_int,
    stdout_initial_termios: Option<libc::termios>,
    inject_resize: bool,
    // Buffer for incomplete UTF-8 sequences (max 4 bytes needed)
    utf8_buf: [u8; 4],
    utf8_len: usize,
}

static mut STATE: State = State {
    stdin: libc::STDIN_FILENO,
    stdin_flags: 0,
    stdout: libc::STDOUT_FILENO,
    stdout_initial_termios: None,
    inject_resize: false,
    utf8_buf: [0; 4],
    utf8_len: 0,
};

extern "C" fn sigwinch_handler(_: libc::c_int) {
    unsafe {
        STATE.inject_resize = true;

Review comment (refactor):
You can use `libc::__error()` instead, I believe. Also, this should be similarly abstracted away:

```rs
#[cfg(target_os = "macos")]
pub fn get_errno() -> libc::c_int {
    unsafe { *libc::__error() };
}

#[cfg(not(target_os = "macos"))]
pub fn get_errno() -> libc::c_int {
    unsafe { *libc::__errno_location() };
}
```

Then you can use it in place of the other calls in this file. This function should go to the end of the file, near the other errno related functions.

Code after:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

//! Unix-specific platform code.
//!
//! Read the `windows` module for reference.
//! TODO: This reminds me that the sys API should probably be a trait.

use std::ffi::{CStr, c_int, c_void};
use std::fs::{self, File};
use std::mem::{self, ManuallyDrop, MaybeUninit};
use std::os::fd::{AsRawFd as _, FromRawFd as _};
use std::ptr::{self, NonNull, null_mut};
use std::{thread, time};

use crate::arena::{Arena, ArenaString, scratch_arena};
use crate::helpers::*;
use crate::{apperr, arena_format};

struct State {
    stdin: libc::c_int,
    stdin_flags: libc::c_int,
    stdout: libc::c_int,
    stdout_initial_termios: Option<libc::termios>,
    inject_resize: bool,
    // Buffer for incomplete UTF-8 sequences (max 4 bytes needed)
    utf8_buf: [u8; 4],
    utf8_len: usize,
}

static mut STATE: State = State {
    stdin: libc::STDIN_FILENO,
    stdin_flags: 0,
    stdout: libc::STDOUT_FILENO,
    stdout_initial_termios: None,
    inject_resize: false,
    utf8_buf: [0; 4],
    utf8_len: 0,
};

extern "C" fn sigwinch_handler(_: libc::c_int) {
    unsafe {
        STATE.inject_resize = true;
    }
}

pub fn init() -> apperr::Result<Deinit> {
Repository: microsoft/edit
File: src/bin/edit/localization.rs (Rust)

Code before:
        /* ru      */ "Файл уже существует. Перезаписать?",
        /* zh_hans */ "文件已存在。要覆盖它吗？",
        /* zh_hant */ "檔案已存在。要覆蓋它嗎？",
    ],
];

static mut S_LANG: LangId = LangId::en;

pub fn init() {
    let scratch = scratch_arena(None);
    let langs = sys::preferred_languages(&scratch);
    let mut lang = LangId::en;

    for l in langs {
        lang = match l.as_str() {
            l if l.starts_with("en") => LangId::en,
            l if l.starts_with("de") => LangId::de,
            l if l.starts_with("es") => LangId::es,
            l if l.starts_with("fr") => LangId::fr,
            l if l.starts_with("it") => LangId::it,
            l if l.starts_with("ja") => LangId::ja,
            l if l.starts_with("ko") => LangId::ko,
            l if l.starts_with("pt-br") => LangId::pt_br,
            l if l.starts_with("ru") => LangId::ru,
            l if l.starts_with("zh-hant") => LangId::zh_hant,
            l if l.starts_with("zh-hans") => LangId::zh_hans,
            _ => continue,
        };
        break;
    }

    unsafe {
        S_LANG = lang;
    }
}

pub fn loc(id: LocId) -> &'static str {
    S_LANG_LUT[id as usize][unsafe { S_LANG as usize }]
}


Review comment (security):
There's another issue with this code: It should ideally perform ASCII case-insensitive prefix matching (similar to [eq_ignore_ascii_case](https://doc.rust-lang.org/std/primitive.str.html#method.eq_ignore_ascii_case) but as `starts_with`). If you're up for it, it'd be great if you could add such a method to `helpers.rs` and use it here.

Otherwise, if you're short on time, we can of course also just merge this PR and do this some time later.

Code after:
        /* ko      */ "파일이 이미 존재합니다. 덮어쓰시겠습니까?",
        /* pt_br   */ "O arquivo já existe. Deseja sobrescrevê-lo?",
        /* ru      */ "Файл уже существует. Перезаписать?",
        /* zh_hans */ "文件已存在。要覆盖它吗？",
        /* zh_hant */ "檔案已存在。要覆蓋它嗎？",
    ],
];

static mut S_LANG: LangId = LangId::en;

pub fn init() {
    const LANG_MAP: &[(&str, LangId)] = &[
        ("en", LangId::en),
        // ----------------
        ("de", LangId::de),
        ("es", LangId::es),
        ("fr", LangId::fr),
        ("it", LangId::it),
        ("ja", LangId::ja),
        ("ko", LangId::ko),
        ("pt-br", LangId::pt_br),
        ("ru", LangId::ru),
        ("zh-hant", LangId::zh_hant),
        ("zh-tw", LangId::zh_hant),
        ("zh", LangId::zh_hans),
    ];

    let scratch = scratch_arena(None);
    let langs = sys::preferred_languages(&scratch);
    let mut lang = LangId::en;

    for l in langs {
        for (prefix, id) in LANG_MAP {
            if l.starts_with_ignore_ascii_case(prefix) {
                lang = *id;
                break;
            }
        }
    }

    unsafe {
        S_LANG = lang;
    }
}

pub fn loc(id: LocId) -> &'static str {
    S_LANG_LUT[id as usize][unsafe { S_LANG as usize }]
}
Repository: microsoft/edit
File: src/helpers.rs (Rust)

Code before:

/// Turns a [`&mut [T]`] into a [`&mut [MaybeUninit<T>]`].
#[inline(always)]
pub const fn slice_as_uninit_mut<T>(slice: &mut [T]) -> &mut [MaybeUninit<T>] {
    unsafe { slice::from_raw_parts_mut(slice.as_mut_ptr() as *mut MaybeUninit<T>, slice.len()) }
}

/// Tests if a string starts with a given ASCII prefix.
pub fn starts_with_ascii(text: &str, prefix: &str) -> bool {
    // If the prefix is longer than the text, it cannot be a prefix.
    if prefix.len() > text.len() {
        return false;
    }

    let text_bytes = text.as_bytes();
    let prefix_bytes = prefix.as_bytes();

    // Iterate over the bytes of both string slices
    // We only need to compare up to the length of the prefix
    for i in 0..prefix.len() {
        // Get the ASCII byte from both text and prefix
        let text_byte = text_bytes[i];
        let prefix_byte = prefix_bytes[i];

        // If the bytes don't match, it's not a prefix
        if text_byte != prefix_byte {
            return false;
        }
    }

    // If we've gone through all prefix bytes and they all matched, then it's a prefix
    true
}


Review comment (security):
What I meant is that we need a case-insensitive `starts_with` function. Since the region codes are expected to be ASCII-only we could write an ASCII-only `starts_with` function without relying on ICU to do the comparison (since ICU may be unavailable).

Code after:

/// Turns a [`&mut [T]`] into a [`&mut [MaybeUninit<T>]`].
#[inline(always)]
pub const fn slice_as_uninit_mut<T>(slice: &mut [T]) -> &mut [MaybeUninit<T>] {
    unsafe { slice::from_raw_parts_mut(slice.as_mut_ptr() as *mut MaybeUninit<T>, slice.len()) }
}

/// Helpers for ASCII string comparisons.
pub trait AsciiStringHelpers {
    /// Tests if a string starts with a given ASCII prefix.
    ///
    /// This function name really is a mouthful, but it's a combination
    /// of [`str::starts_with`] and [`str::eq_ignore_ascii_case`].
    fn starts_with_ignore_ascii_case(&self, prefix: &str) -> bool;
}

impl AsciiStringHelpers for str {
    fn starts_with_ignore_ascii_case(&self, prefix: &str) -> bool {
        // Casting to bytes first ensures we skip any UTF8 boundary checks.
        // Since the comparison is ASCII, we don't need to worry about that.
        let s = self.as_bytes();
        let p = prefix.as_bytes();
        p.len() <= s.len() && s[..p.len()].eq_ignore_ascii_case(p)
    }
}
Repository: microsoft/edit
File: src/helpers.rs (Rust)

Code before:
pub const fn slice_as_uninit_ref<T>(slice: &[T]) -> &[MaybeUninit<T>] {
    unsafe { slice::from_raw_parts(slice.as_ptr() as *const MaybeUninit<T>, slice.len()) }
}

/// Turns a [`&mut [T]`] into a [`&mut [MaybeUninit<T>]`].
#[inline(always)]
pub const fn slice_as_uninit_mut<T>(slice: &mut [T]) -> &mut [MaybeUninit<T>] {
    unsafe { slice::from_raw_parts_mut(slice.as_mut_ptr() as *mut MaybeUninit<T>, slice.len()) }
}

/// Tests if a string starts with a given ASCII prefix.
pub fn starts_with_ascii(text: &str, prefix: &str) -> bool {
    // If the prefix is longer than the text, it cannot be a prefix.
    if prefix.len() > text.len() {
        return false;
    }

    let text_bytes = text.as_bytes();
    let prefix_bytes = prefix.as_bytes();

    // Iterate over the bytes of both string slices
    // We only need to compare up to the length of the prefix
    for i in 0..prefix.len() {
        // Get the ASCII byte from both text and prefix
        let text_byte = text_bytes[i];
        let prefix_byte = prefix_bytes[i];

        // If the bytes don't match, it's not a prefix
        if text_byte != prefix_byte {
            return false;
        }
    }

    // If we've gone through all prefix bytes and they all matched, then it's a prefix
    true
}


Review comment (suggestion):
I forgot to mention that you can use `text_bytes.iter().zip(prefix_bytes.iter())` for such patterns. See here: https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.zip

Code after:
pub const fn slice_as_uninit_ref<T>(slice: &[T]) -> &[MaybeUninit<T>] {
    unsafe { slice::from_raw_parts(slice.as_ptr() as *const MaybeUninit<T>, slice.len()) }
}

/// Turns a [`&mut [T]`] into a [`&mut [MaybeUninit<T>]`].
#[inline(always)]
pub const fn slice_as_uninit_mut<T>(slice: &mut [T]) -> &mut [MaybeUninit<T>] {
    unsafe { slice::from_raw_parts_mut(slice.as_mut_ptr() as *mut MaybeUninit<T>, slice.len()) }
}

/// Helpers for ASCII string comparisons.
pub trait AsciiStringHelpers {
    /// Tests if a string starts with a given ASCII prefix.
    ///
    /// This function name really is a mouthful, but it's a combination
    /// of [`str::starts_with`] and [`str::eq_ignore_ascii_case`].
    fn starts_with_ignore_ascii_case(&self, prefix: &str) -> bool;
}

impl AsciiStringHelpers for str {
    fn starts_with_ignore_ascii_case(&self, prefix: &str) -> bool {
        // Casting to bytes first ensures we skip any UTF8 boundary checks.
        // Since the comparison is ASCII, we don't need to worry about that.
        let s = self.as_bytes();
        let p = prefix.as_bytes();
        p.len() <= s.len() && s[..p.len()].eq_ignore_ascii_case(p)
    }
}
Repository: microsoft/edit
File: src/bin/edit/main.rs (Rust)

Code before:
        .filter_map(|arg| {
            if arg == "-" {
                // We ignore it
                return None;
            }
            let p = cwd.join(Path::new(arg));
            let p = path::normalize(&p);
            if p.is_dir() {
                return None;
            }
            Some(p)
        })
        .collect::<Vec<PathBuf>>();

    if paths.len() > 1 {
        for p in paths {
            state.documents.add_file_path(&p)?;
        }
    } else if paths.len() == 1 {
        let p = paths[0].clone();
        if let Some(parent) = p.parent() {
            cwd = parent.to_path_buf();
        }
        state.documents.add_file_path(&p)?;
    } else {
        if let Some(mut file) = sys::open_stdin_if_redirected() {
            let doc = state.documents.add_untitled()?;
            let mut tb = doc.buffer.borrow_mut();
            tb.read_file(&mut file, None)?;
            tb.mark_as_dirty();
        } else {
            state.documents.add_untitled()?;
        }
    }

    state.file_picker_pending_dir = DisplayablePathBuf::new(cwd);
    Ok(false)
}

fn print_help() {
    sys::write_stdout(concat!(
        "Usage: edit [OPTIONS] [FILE]\r\n",
        "Options:\r\n",
        "    -h, --help       Print this help message\r\n",
        "    -v, --version    Print the version number\r\n",
    ));
}

fn print_version() {
    sys::write_stdout(concat!("edit version ", env!("CARGO_PKG_VERSION"), "\r\n"));
}

Review comment (suggestion):
We also need to handle this situation properly. If stdin is redirected, we _must_ read that input, even if multiple file paths are given.

Code after:
            print_version();
            return Ok(true);
        } else if arg == "-" {
            paths.clear();
            break;
        }
        let p = cwd.join(Path::new(&arg));
        let p = path::normalize(&p);
        if !p.is_dir() {
            paths.push(p);
        }
    }

    for p in &paths {
        state.documents.add_file_path(p)?;
    }
    if let Some(parent) = paths.first().and_then(|p| p.parent()) {
        cwd = parent.to_path_buf();
    }

    if let Some(mut file) = sys::open_stdin_if_redirected() {
        let doc = state.documents.add_untitled()?;
        let mut tb = doc.buffer.borrow_mut();
        tb.read_file(&mut file, None)?;
        tb.mark_as_dirty();
    } else if paths.is_empty() {
        // No files were passed, and stdin is not redirected.
        state.documents.add_untitled()?;
    }

    state.file_picker_pending_dir = DisplayablePathBuf::new(cwd);
    Ok(false)
}

fn print_help() {
    sys::write_stdout(concat!(
        "Usage: edit [OPTIONS] [FILE]\r\n",
        "Options:\r\n",
        "    -h, --help       Print this help message\r\n",
        "    -v, --version    Print the version number\r\n",
    ));
}

fn print_version() {
    sys::write_stdout(concat!("edit version ", env!("CARGO_PKG_VERSION"), "\r\n"));
}

fn draw(ctx: &mut Context, state: &mut State) {
    draw_menubar(ctx, state);
    draw_editor(ctx, state);
    draw_statusbar(ctx, state);
Repository: microsoft/edit
File: src/bin/edit/main.rs (Rust)

Code before:
        }
        let p = cwd.join(Path::new(&arg));
        let p = path::normalize(&p);
        if !p.is_dir() {
            paths.push(p);
        }
    }

    if paths.len() > 1 {
        for p in &paths {
            state.documents.add_file_path(p)?;
        }
    } else if paths.len() == 1 {
        let p = &paths[0];
        if let Some(parent) = p.parent() {
            cwd = parent.to_path_buf();
        }
        state.documents.add_file_path(p)?;
    }

    if let Some(mut file) = sys::open_stdin_if_redirected() {
        let doc = state.documents.add_untitled()?;
        let mut tb = doc.buffer.borrow_mut();
        tb.read_file(&mut file, None)?;
        tb.mark_as_dirty();
    } else if paths.len() == 0 {
        // No files were passed, and stdin is not redirected.
        state.documents.add_untitled()?;
    }

    state.file_picker_pending_dir = DisplayablePathBuf::new(cwd);
    Ok(false)
}

fn print_help() {
    sys::write_stdout(concat!(
        "Usage: edit [OPTIONS] [FILE]\r\n",
        "Options:\r\n",
        "    -h, --help       Print this help message\r\n",
        "    -v, --version    Print the version number\r\n",
    ));
}

fn print_version() {
    sys::write_stdout(concat!("edit version ", env!("CARGO_PKG_VERSION"), "\r\n"));
}

fn draw(ctx: &mut Context, state: &mut State) {
    draw_menubar(ctx, state);
    draw_editor(ctx, state);
    draw_statusbar(ctx, state);

Review comment (suggestion):
`cargo clippy` complains about this line. Could you change it to this?
```suggestion
    } else if paths.is_empty() {
```

Code after:
        }
        let p = cwd.join(Path::new(&arg));
        let p = path::normalize(&p);
        if !p.is_dir() {
            paths.push(p);
        }
    }

    for p in &paths {
        state.documents.add_file_path(p)?;
    }
    if let Some(parent) = paths.first().and_then(|p| p.parent()) {
        cwd = parent.to_path_buf();
    }

    if let Some(mut file) = sys::open_stdin_if_redirected() {
        let doc = state.documents.add_untitled()?;
        let mut tb = doc.buffer.borrow_mut();
        tb.read_file(&mut file, None)?;
        tb.mark_as_dirty();
    } else if paths.is_empty() {
        // No files were passed, and stdin is not redirected.
        state.documents.add_untitled()?;
    }

    state.file_picker_pending_dir = DisplayablePathBuf::new(cwd);
    Ok(false)
}

fn print_help() {
    sys::write_stdout(concat!(
        "Usage: edit [OPTIONS] [FILE]\r\n",
        "Options:\r\n",
        "    -h, --help       Print this help message\r\n",
        "    -v, --version    Print the version number\r\n",
    ));
}

fn print_version() {
    sys::write_stdout(concat!("edit version ", env!("CARGO_PKG_VERSION"), "\r\n"));
}

fn draw(ctx: &mut Context, state: &mut State) {
    draw_menubar(ctx, state);
    draw_editor(ctx, state);
    draw_statusbar(ctx, state);

    if state.wants_close {
        draw_handle_wants_close(ctx, state);
    }
    if state.wants_exit {