ronantakizawa/codereview-bench huggingface dataset:
Repository: helix-editor/helix
File: helix-stdx/src/env.rs (Rust)

Code before:
use std::{
    borrow::Cow,
    ffi::{OsStr, OsString},
    path::{Path, PathBuf},
    sync::RwLock,
};

use once_cell::sync::Lazy;

static CWD: RwLock<Option<PathBuf>> = RwLock::new(None);
// previous working directory, stored to allow `:cd -`
static PREV_CWD: RwLock<Option<PathBuf>> = RwLock::new(None);

fn change_cwd(new_dir: PathBuf) {
    let mut cwd = CWD.write().unwrap();
    let mut prev_cwd = PREV_CWD.write().unwrap();
    *prev_cwd = cwd.clone();
    *cwd = Some(new_dir);
}

// Get the current working directory.
// This information is managed internally as the call to std::env::current_dir
// might fail if the cwd has been deleted.
pub fn current_working_dir() -> PathBuf {
    if let Some(path) = &*CWD.read().unwrap() {
        return path.clone();
    }

    // implementation of crossplatform pwd -L
    // we want pwd -L so that symlinked directories are handled correctly
    let mut cwd = std::env::current_dir().expect("Couldn't determine current working directory");

    let pwd = std::env::var_os("PWD");
    #[cfg(windows)]
    let pwd = pwd.or_else(|| std::env::var_os("CD"));

    if let Some(pwd) = pwd.map(PathBuf::from) {

Review comment (suggestion):
We keep the `CWD` as a static so that we can access it in places where we don't have access to the `Editor`. For this feature though we only need to access the previous cwd within the `:cd` command (where we have an exclusive borrow of the Editor) so we can store a `last_cwd: Option<PathBuf>` on the `Editor` rather than dealing with all of the static and locking business here

Code after:
use std::{
    borrow::Cow,
    ffi::{OsStr, OsString},
    path::{Path, PathBuf},
    sync::RwLock,
};

use once_cell::sync::Lazy;

// We keep the CWD as a static so that we can access it in places where we don't have access to the Editor
static CWD: RwLock<Option<PathBuf>> = RwLock::new(None);

// Get the current working directory.
// This information is managed internally as the call to std::env::current_dir
// might fail if the cwd has been deleted.
pub fn current_working_dir() -> PathBuf {
    if let Some(path) = &*CWD.read().unwrap() {
        return path.clone();
    }

    // implementation of crossplatform pwd -L
    // we want pwd -L so that symlinked directories are handled correctly
    let mut cwd = std::env::current_dir().expect("Couldn't determine current working directory");

    let pwd = std::env::var_os("PWD");
    #[cfg(windows)]
    let pwd = pwd.or_else(|| std::env::var_os("CD"));

    if let Some(pwd) = pwd.map(PathBuf::from) {
        if pwd.canonicalize().ok().as_ref() == Some(&cwd) {
            cwd = pwd;
        }
    }
    let mut dst = CWD.write().unwrap();
    *dst = Some(cwd.clone());

    cwd
Repository: helix-editor/helix
File: helix-term/src/commands/typed.rs (Rust)

Code before:
    cx.editor
        .set_status(cx.editor.registers.clipboard_provider_name());
    Ok(())
}

fn change_current_directory(
    cx: &mut compositor::Context,
    args: &[Cow<str>],
    event: PromptEvent,
) -> anyhow::Result<()> {
    if event != PromptEvent::Validate {
        return Ok(());
    }

    let dir = match args.first() {
        Some(Cow::Borrowed("-")) => cx.editor.last_cwd.clone(),
        Some(input_path) => Some(
            helix_stdx::path::expand_tilde(Path::new(input_path.as_ref()).to_owned())
                .deref()
                .to_path_buf(),
        ),
        None => Some(home_dir()?.as_path().to_owned()),
    };

    if let Some(dir) = dir {
        helix_stdx::env::set_current_working_dir(&mut cx.editor.last_cwd, dir)?;

        cx.editor.set_status(format!(
            "Current working directory is now {}",
            helix_stdx::env::current_working_dir().display()
        ));
    } else {
        // dir is None only if there is not a previous working directory, in which case we won't change it
        cx.editor.set_status(format!(
            "Current working directory is still {}",
            helix_stdx::env::current_working_dir().display()
        ));
    };

    Ok(())
}

fn show_current_directory(
    cx: &mut compositor::Context,
    _args: &[Cow<str>],
    event: PromptEvent,
) -> anyhow::Result<()> {
    if event != PromptEvent::Validate {
        return Ok(());
    }


Review comment (suggestion):
Rather than passing this in you can set it on another line:

```rust
helix_stdx::env::set_current_working_dir(dir)?;
cx.editor.last_cwd = Some(dir);
```

Code after:
    cx.editor
        .set_status(cx.editor.registers.clipboard_provider_name());
    Ok(())
}

fn change_current_directory(
    cx: &mut compositor::Context,
    args: &[Cow<str>],
    event: PromptEvent,
) -> anyhow::Result<()> {
    if event != PromptEvent::Validate {
        return Ok(());
    }

    let dir = match args.first() {
        Some(Cow::Borrowed("-")) => cx
            .editor
            .last_cwd
            .clone()
            .ok_or(anyhow!("No previous working directory"))?,
        Some(input_path) => {
            helix_stdx::path::expand_tilde(Path::new(input_path.as_ref()).to_owned())
                .deref()
                .to_path_buf()
        }
        None => home_dir()?.as_path().to_owned(),
    };

    cx.editor.last_cwd = helix_stdx::env::set_current_working_dir(dir)?;

    cx.editor.set_status(format!(
        "Current working directory is now {}",
        helix_stdx::env::current_working_dir().display()
    ));

    Ok(())
}

fn show_current_directory(
    cx: &mut compositor::Context,
    _args: &[Cow<str>],
    event: PromptEvent,
) -> anyhow::Result<()> {
    if event != PromptEvent::Validate {
        return Ok(());
    }

    let cwd = helix_stdx::env::current_working_dir();
    let message = format!("Current working directory is {}", cwd.display());

    if cwd.exists() {
Repository: helix-editor/helix
File: helix-term/src/commands/typed.rs (Rust)

Code before:

fn show_clipboard_provider(
    cx: &mut compositor::Context,
    _args: &[Cow<str>],
    event: PromptEvent,
) -> anyhow::Result<()> {
    if event != PromptEvent::Validate {
        return Ok(());
    }

    cx.editor
        .set_status(cx.editor.registers.clipboard_provider_name());
    Ok(())
}

fn change_current_directory(
    cx: &mut compositor::Context,
    args: &[Cow<str>],
    event: PromptEvent,
) -> anyhow::Result<()> {
    if event != PromptEvent::Validate {
        return Ok(());
    }

    let dir = match args.first() {
        Some(Cow::Borrowed("-")) => cx.editor.last_cwd.clone(),
        Some(input_path) => Some(
            helix_stdx::path::expand_tilde(Path::new(input_path.as_ref()).to_owned())
                .deref()
                .to_path_buf(),
        ),
        None => Some(home_dir()?.as_path().to_owned()),
    };

    if let Some(dir) = dir {
        helix_stdx::env::set_current_working_dir(&mut cx.editor.last_cwd, dir)?;

        cx.editor.set_status(format!(
            "Current working directory is now {}",
            helix_stdx::env::current_working_dir().display()
        ));
    } else {
        // dir is None only if there is not a previous working directory, in which case we won't change it
        cx.editor.set_status(format!(
            "Current working directory is still {}",
            helix_stdx::env::current_working_dir().display()
        ));
    };

    Ok(())
}

Review comment (suggestion):
We can early return here to avoid the extra option handling:

```rust
Some(Cow::Borrowed("-")) => cx.editor.last_cwd.clone().ok_or(anyhow!("No previous working directory"))?,
```

Code after:

fn show_clipboard_provider(
    cx: &mut compositor::Context,
    _args: &[Cow<str>],
    event: PromptEvent,
) -> anyhow::Result<()> {
    if event != PromptEvent::Validate {
        return Ok(());
    }

    cx.editor
        .set_status(cx.editor.registers.clipboard_provider_name());
    Ok(())
}

fn change_current_directory(
    cx: &mut compositor::Context,
    args: &[Cow<str>],
    event: PromptEvent,
) -> anyhow::Result<()> {
    if event != PromptEvent::Validate {
        return Ok(());
    }

    let dir = match args.first() {
        Some(Cow::Borrowed("-")) => cx
            .editor
            .last_cwd
            .clone()
            .ok_or(anyhow!("No previous working directory"))?,
        Some(input_path) => {
            helix_stdx::path::expand_tilde(Path::new(input_path.as_ref()).to_owned())
                .deref()
                .to_path_buf()
        }
        None => home_dir()?.as_path().to_owned(),
    };

    cx.editor.last_cwd = helix_stdx::env::set_current_working_dir(dir)?;

    cx.editor.set_status(format!(
        "Current working directory is now {}",
        helix_stdx::env::current_working_dir().display()
    ));

    Ok(())
}

fn show_current_directory(
    cx: &mut compositor::Context,
    _args: &[Cow<str>],
Repository: helix-editor/helix
File: helix-term/src/ui/text_decorations.rs (Rust)

Code before:
                    Ordering::Greater => break,
                }
            }
        }
    }

    pub fn decorate_line(&mut self, renderer: &mut TextRenderer, pos: LinePos) {
        for (decoration, _) in &mut self.decorations {
            decoration.decorate_line(renderer, pos);
        }
    }

    pub fn render_virtual_lines(
        &mut self,
        renderer: &mut TextRenderer,
        pos: LinePos,
        line_width: usize,
    ) {
        let mut virt_off = Position::new(1, line_width); // start at 1 so the line is never overwritten
        for (decoration, _) in &mut self.decorations {
            virt_off += decoration.render_virt_lines(renderer, pos, virt_off);
        }
    }
}

pub struct ColorSwatch {
    color: Color,
    pos: usize,
}

impl ColorSwatch {
    pub fn new(color: Color, pos: usize) -> Self {
        ColorSwatch { color, pos }
    }
}

impl Decoration for ColorSwatch {
    fn decorate_grapheme(
        &mut self,
        _renderer: &mut TextRenderer,
        _grapheme: &FormattedGrapheme,
        style: &mut Style,
    ) -> usize {
        style.fg = Some(self.color);
        usize::MAX
    }

    fn reset_pos(&mut self, pos: usize) -> usize {
        if self.pos >= pos {
            self.pos
        } else {

Review comment (suggestion):
looking at this closer, this should act more like a state machine and we should only have one per document see this commit for how it can be implemented this way https://github.com/gabydd/helix/commit/3bfcbb555169c1c8b2c668abd70aa9f8acd198ee

Code after:
        }
    }

    pub fn render_virtual_lines(
        &mut self,
        renderer: &mut TextRenderer,
        pos: LinePos,
        line_width: usize,
    ) {
        let mut virt_off = Position::new(1, line_width); // start at 1 so the line is never overwritten
        for (decoration, _) in &mut self.decorations {
            virt_off += decoration.render_virt_lines(renderer, pos, virt_off);
        }
    }
}

/// Cursor rendering is done externally so all the cursor decoration
/// does is save the position of primary cursor
pub struct Cursor<'a> {
    pub cache: &'a CursorCache,
    pub primary_cursor: usize,
}
impl Decoration for Cursor<'_> {
    fn reset_pos(&mut self, pos: usize) -> usize {
        if pos <= self.primary_cursor {
            self.primary_cursor
        } else {
            usize::MAX
        }
    }

    fn decorate_grapheme(
        &mut self,
        renderer: &mut TextRenderer,
        grapheme: &FormattedGrapheme,
    ) -> usize {
        if renderer.column_in_bounds(grapheme.visual_pos.col, grapheme.width())
            && renderer.offset.row < grapheme.visual_pos.row
        {
            let position = grapheme.visual_pos - renderer.offset;
            self.cache.set(Some(position));
        }
        usize::MAX
    }
}
Repository: helix-editor/helix
File: helix-term/src/commands/typed.rs (Rust)

Code before:

fn change_current_directory(
    cx: &mut compositor::Context,
    args: &[Cow<str>],
    event: PromptEvent,
) -> anyhow::Result<()> {
    if event != PromptEvent::Validate {
        return Ok(());
    }

    let dir = match args.first().map(AsRef::as_ref) {
        Some("-") => cx
            .editor
            .last_cwd
            .clone()
            .ok_or_else(|| anyhow!("No previous working directory"))?,
        Some(input_path) => helix_stdx::path::expand_tilde(Path::new(input_path)).to_path_buf(),
        None => home_dir()?,
    };

    cx.editor.last_cwd = helix_stdx::env::set_current_working_dir(dir)?;

    let (_, doc) = current!(cx.editor);

    // Clear the relative path as this will need to be recalculated.
    doc.relative_path.take();

    cx.editor.set_status(format!(
        "Current working directory is now {}",
        helix_stdx::env::current_working_dir().display()
    ));

    Ok(())
}

fn show_current_directory(
    cx: &mut compositor::Context,
    _args: &[Cow<str>],
    event: PromptEvent,
) -> anyhow::Result<()> {
    if event != PromptEvent::Validate {
        return Ok(());
    }

    let cwd = helix_stdx::env::current_working_dir();
    let message = format!("Current working directory is {}", cwd.display());

    if cwd.exists() {
        cx.editor.set_status(message);
    } else {
        cx.editor.set_error(format!("{} (deleted)", message));

Review comment (suggestion):
All relative paths will need to be reset so this will need to go through `cx.editor.documents_mut()`

Code after:
        cx.editor.set_status(encoding);
        Ok(())
    }
}

/// Shows info about the character under the primary cursor.
fn get_character_info(
    cx: &mut compositor::Context,
    _args: Args,
    event: PromptEvent,
) -> anyhow::Result<()> {
    if event != PromptEvent::Validate {
        return Ok(());
    }

    let (view, doc) = current_ref!(cx.editor);
    let text = doc.text().slice(..);

    let grapheme_start = doc.selection(view.id).primary().cursor(text);
    let grapheme_end = graphemes::next_grapheme_boundary(text, grapheme_start);

    if grapheme_start == grapheme_end {
        return Ok(());
    }

    let grapheme = text.slice(grapheme_start..grapheme_end).to_string();
    let encoding = doc.encoding();

    let printable = grapheme.chars().fold(String::new(), |mut s, c| {
        match c {
            '\0' => s.push_str("\\0"),
            '\t' => s.push_str("\\t"),
            '\n' => s.push_str("\\n"),
            '\r' => s.push_str("\\r"),
            _ => s.push(c),
        }

        s
    });

    // Convert to Unicode codepoints if in UTF-8
    let unicode = if encoding == encoding::UTF_8 {
        let mut unicode = " (".to_owned();

        for (i, char) in grapheme.chars().enumerate() {
            if i != 0 {
                unicode.push(' ');
            }

            unicode.push_str("U+");
Repository: helix-editor/helix
File: helix-lsp-types/src/lib.rs (Rust)

Code before:
        version: Option<i32>,
    ) -> PublishDiagnosticsParams {
        PublishDiagnosticsParams {
            uri,
            diagnostics,
            version,
        }
    }
}

#[derive(Debug, Eq, PartialEq, Deserialize, Serialize, Clone)]
#[serde(untagged)]
pub enum Documentation {
    String(String),
    MarkupContent(MarkupContent),
}

/// MarkedString can be used to render human readable text. It is either a
/// markdown string or a code-block that provides a language and a code snippet.
/// The language identifier is semantically equal to the optional language
/// identifier in fenced code blocks in GitHub issues.
///
/// The pair of a language and a value is an equivalent to markdown:
///
/// ```language
/// value
/// ```
#[derive(Debug, Eq, PartialEq, Clone, Deserialize, Serialize)]
#[serde(untagged)]
pub enum MarkedString {
    String(String),
    LanguageString(LanguageString),
}

#[derive(Debug, Eq, PartialEq, Clone, Deserialize, Serialize)]
pub struct LanguageString {
    pub language: String,
    pub value: String,
}

impl MarkedString {
    pub fn from_markdown(markdown: String) -> MarkedString {
        MarkedString::String(markdown)
    }

    pub fn from_language_code(language: String, code_block: String) -> MarkedString {
        MarkedString::LanguageString(LanguageString {
            language,
            value: code_block,
        })
    }

Review comment (suggestion):
Both the rendering of the doc and clippy dont seem to like any way this feels like it should be handled. These changes get the CI to pass, but still has rendering issues. I couldn't get nested code fences to work for either case.

Code after:
        version: Option<i32>,
    ) -> PublishDiagnosticsParams {
        PublishDiagnosticsParams {
            uri,
            diagnostics,
            version,
        }
    }
}

#[derive(Debug, Eq, PartialEq, Deserialize, Serialize, Clone)]
#[serde(untagged)]
pub enum Documentation {
    String(String),
    MarkupContent(MarkupContent),
}

/// MarkedString can be used to render human readable text. It is either a
/// markdown string or a code-block that provides a language and a code snippet.
/// The language identifier is semantically equal to the optional language
/// identifier in fenced code blocks in GitHub issues.
///
/// The pair of a language and a value is an equivalent to markdown:
///
/// <pre><code>```${language}
/// ${value}
/// ```</code></pre>
#[derive(Debug, Eq, PartialEq, Clone, Deserialize, Serialize)]
#[serde(untagged)]
pub enum MarkedString {
    String(String),
    LanguageString(LanguageString),
}

#[derive(Debug, Eq, PartialEq, Clone, Deserialize, Serialize)]
pub struct LanguageString {
    pub language: String,
    pub value: String,
}

impl MarkedString {
    pub fn from_markdown(markdown: String) -> MarkedString {
        MarkedString::String(markdown)
    }

    pub fn from_language_code(language: String, code_block: String) -> MarkedString {
        MarkedString::LanguageString(LanguageString {
            language,
            value: code_block,
        })
    }
Repository: helix-editor/helix
File: helix-view/src/expansion.rs (Rust)

Code before:
    let (view, doc) = current_ref!(editor);
    let text = doc.text().slice(..);

    match variable {
        Variable::CursorLine => {
            let cursor_line = doc.selection(view.id).primary().cursor_line(text);
            Ok(Cow::Owned((cursor_line + 1).to_string()))
        }
        Variable::CursorColumn => {
            let cursor = doc.selection(view.id).primary().cursor(text);
            let position = helix_core::coords_at_pos(text, cursor);
            Ok(Cow::Owned((position.col + 1).to_string()))
        }
        Variable::BufferName => {
            // Note: usually we would use `Document::display_name` but we can statically borrow
            // the scratch buffer name by partially reimplementing `display_name`.
            if let Some(path) = doc.relative_path() {
                Ok(Cow::Owned(path.to_string_lossy().into_owned()))
            } else {
                Ok(Cow::Borrowed(crate::document::SCRATCH_BUFFER_NAME))
            }
        }
        Variable::LineEnding => Ok(Cow::Borrowed(doc.line_ending.as_str())),
        Variable::Language => Ok(Cow::Owned(
            doc.language_name().unwrap_or("text").to_string(),
        )),
    }
}


Review comment (suggestion):
```suggestion
        Variable::Language => Ok(match doc.language_name() {
            Some(lang) => Cow::Owned(lang.to_owned()),
            None => Cow::Borrowed("text"),
        }),
```

might as well avoid that extra allocation when there is no lang, even though it's small

Code after:
    let (view, doc) = current_ref!(editor);
    let text = doc.text().slice(..);

    match variable {
        Variable::CursorLine => {
            let cursor_line = doc.selection(view.id).primary().cursor_line(text);
            Ok(Cow::Owned((cursor_line + 1).to_string()))
        }
        Variable::CursorColumn => {
            let cursor = doc.selection(view.id).primary().cursor(text);
            let position = helix_core::coords_at_pos(text, cursor);
            Ok(Cow::Owned((position.col + 1).to_string()))
        }
        Variable::BufferName => {
            // Note: usually we would use `Document::display_name` but we can statically borrow
            // the scratch buffer name by partially reimplementing `display_name`.
            if let Some(path) = doc.relative_path() {
                Ok(Cow::Owned(path.to_string_lossy().into_owned()))
            } else {
                Ok(Cow::Borrowed(crate::document::SCRATCH_BUFFER_NAME))
            }
        }
        Variable::LineEnding => Ok(Cow::Borrowed(doc.line_ending.as_str())),
        Variable::Language => Ok(match doc.language_name() {
            Some(lang) => Cow::Owned(lang.to_owned()),
            None => Cow::Borrowed("text"),
        }),
    }
}
Repository: helix-editor/helix
File: helix-term/src/ui/lsp/signature_help.rs (Rust)

Code before:

        let signature_text = crate::ui::markdown::highlighted_code_block(
            signature.signature.as_str(),
            &self.language,
            None,
            &self.config_loader.load(),
            None,
        );
        let (sig_width, sig_height) =
            crate::ui::text::required_size(&signature_text, max_text_width);

        let (width, height) = match signature.signature_doc {
            Some(ref doc) => {
                let doc_md = Markdown::new(doc.clone(), Arc::clone(&self.config_loader));
                let doc_text = doc_md.parse(None);
                let (doc_width, doc_height) =
                    crate::ui::text::required_size(&doc_text, max_text_width);
                (
                    sig_width.max(doc_width),
                    sig_height + SEPARATOR_HEIGHT + doc_height,
                )
            }
            None => (sig_width, sig_height),
        };
        let sig_text_para = Paragraph::new(&signature_text).wrap(Wrap { trim: false });
        let height = height.max(sig_text_para.line_count(max_text_width));

        let sig_index_width = if self.signatures.len() > 1 {
            self.signature_index().len() + 1
        } else {
            0
        };

        Some((width + PADDING + sig_index_width as u16, height + PADDING))
    }
}


Review comment (suggestion):
Counting lines in the `Paragraph` widget according to its wrapping is more accurate and we should probably be doing this anywhere we render as a `Paragraph`. Instead of a `line_count` method on `Paragraph` let's have a `required_size` method that also calculates the width - that would replace calls to `helix_term::ui::text::required_size`.

Code after:

        let signature_text = crate::ui::markdown::highlighted_code_block(
            signature.signature.as_str(),
            &self.language,
            None,
            &self.config_loader.load(),
            None,
        );
        let sig_text_para = Paragraph::new(&signature_text).wrap(Wrap { trim: false });
        let (sig_width, sig_height) = sig_text_para.required_size(max_text_width);

        let (width, height) = match signature.signature_doc {
            Some(ref doc) => {
                let doc_md = Markdown::new(doc.clone(), Arc::clone(&self.config_loader));
                let doc_text = doc_md.parse(None);
                let (doc_width, doc_height) =
                    crate::ui::text::required_size(&doc_text, max_text_width);
                (
                    sig_width.max(doc_width),
                    sig_height + SEPARATOR_HEIGHT + doc_height,
                )
            }
            None => (sig_width, sig_height),
        };

        let sig_index_width = if self.signatures.len() > 1 {
            self.signature_index().len() + 1
        } else {
            0
        };

        Some((width + PADDING + sig_index_width as u16, height + PADDING))
    }
}