ronantakizawa/codereview-bench huggingface dataset:
Repository: helix-editor/helix
File: helix-view/src/handlers/lsp.rs (Rust)

Code before:
}

#[derive(Debug, PartialEq, Eq, Clone, Copy)]
pub enum SignatureHelpInvoked {
    Automatic,
    Manual,
}

pub enum SignatureHelpEvent {
    Invoked,
    Trigger,
    ReTrigger,
    Cancel,
    RequestComplete { open: bool },
}

#[derive(Debug, PartialEq, Eq, Clone, Copy)]
pub enum AutoSaveInvoked {
    Automatic,
}

pub enum AutoSaveEvent {
    // contains delay in milli seconds
    Trigger(u64),
    Cancel,
}

#[derive(Debug)]
pub struct ApplyEditError {
    pub kind: ApplyEditErrorKind,
    pub failed_change_idx: usize,
}

#[derive(Debug)]
pub enum ApplyEditErrorKind {
    DocumentChanged,
    FileNotFound,
    UnknownURISchema,
    IoError(std::io::Error),
    // TODO: check edits before applying and propagate failure
    // InvalidEdit,
}

impl ToString for ApplyEditErrorKind {
    fn to_string(&self) -> String {
        match self {
            ApplyEditErrorKind::DocumentChanged => "document has changed".to_string(),
            ApplyEditErrorKind::FileNotFound => "file not found".to_string(),
            ApplyEditErrorKind::UnknownURISchema => "URI schema not supported".to_string(),
            ApplyEditErrorKind::IoError(err) => err.to_string(),
        }

Review comment (suggestion):
I don't think you need all of these types.

As far as I can tell the `AutoSaveEvent::Cancel` is never constructed and `AutoSaveInvoked` is also never constructed (and only has a single variant). I think the original PR had these types because it took the completions/signature help as reference.

You can simply set `type Event = u64;`.

Code after:
}

#[derive(Debug, PartialEq, Eq, Clone, Copy)]
pub enum SignatureHelpInvoked {
    Automatic,
    Manual,
}

pub enum SignatureHelpEvent {
    Invoked,
    Trigger,
    ReTrigger,
    Cancel,
    RequestComplete { open: bool },
}

#[derive(Debug)]
pub struct ApplyEditError {
    pub kind: ApplyEditErrorKind,
    pub failed_change_idx: usize,
}

#[derive(Debug)]
pub enum ApplyEditErrorKind {
    DocumentChanged,
    FileNotFound,
    UnknownURISchema,
    IoError(std::io::Error),
    // TODO: check edits before applying and propagate failure
    // InvalidEdit,
}

impl ToString for ApplyEditErrorKind {
    fn to_string(&self) -> String {
        match self {
            ApplyEditErrorKind::DocumentChanged => "document has changed".to_string(),
            ApplyEditErrorKind::FileNotFound => "file not found".to_string(),
            ApplyEditErrorKind::UnknownURISchema => "URI schema not supported".to_string(),
            ApplyEditErrorKind::IoError(err) => err.to_string(),
        }
    }
}

impl Editor {
    fn apply_text_edits(
        &mut self,
        uri: &helix_lsp::Url,
        version: Option<i32>,
        text_edits: Vec<lsp::TextEdit>,
        offset_encoding: OffsetEncoding,
    ) -> Result<(), ApplyEditErrorKind> {
Repository: helix-editor/helix
File: helix-term/src/handlers/auto_save.rs (Rust)

Code before:
use std::time::Duration;

use anyhow::Ok;
use arc_swap::access::Access;

use helix_event::{register_hook, send_blocking};
use helix_view::{
    editor::DEFAULT_AUTO_SAVE_DELAY,
    events::DocumentDidChange,
    handlers::{lsp::AutoSaveEvent, Handlers},
    Editor,
};
use tokio::time::Instant;

use crate::{
    commands, compositor,
    job::{self, Jobs},
};

#[derive(Debug)]
enum State {
    Closed,
}

#[derive(Debug)]
pub(super) struct AutoSaveHandler {
    state: State,
}

impl AutoSaveHandler {
    pub fn new() -> AutoSaveHandler {
        AutoSaveHandler {
            state: State::Closed,
        }
    }
}

impl helix_event::AsyncHook for AutoSaveHandler {
    type Event = AutoSaveEvent;

    fn handle_event(
        &mut self,
        event: Self::Event,
        _: Option<tokio::time::Instant>,
    ) -> Option<Instant> {
        match event {

Review comment (refactor):
This type is also useless/unneded the AutoSaveHandler does not have any state. This was again copied from signature help. And you can just remove this and make `AutoSaveHandler` a unit struct/zst

Code after:
use std::time::Duration;

use anyhow::Ok;
use arc_swap::access::Access;

use helix_event::{register_hook, send_blocking};
use helix_view::{events::DocumentDidChange, handlers::Handlers, Editor};
use tokio::time::Instant;

use crate::{
    commands, compositor,
    job::{self, Jobs},
};

#[derive(Debug)]
pub(super) struct AutoSaveHandler;

impl AutoSaveHandler {
    pub fn new() -> AutoSaveHandler {
        AutoSaveHandler
    }
}

impl helix_event::AsyncHook for AutoSaveHandler {
    type Event = u64;

    fn handle_event(
        &mut self,
        timeout: Self::Event,
        _: Option<tokio::time::Instant>,
    ) -> Option<Instant> {
        Some(Instant::now() + Duration::from_millis(timeout))
    }

    fn finish_debounce(&mut self) {
        job::dispatch_blocking(move |editor, _| request_auto_save(editor))
    }
}

fn request_auto_save(editor: &mut Editor) {
    let context = &mut compositor::Context {
        editor,
        scroll: Some(0),
        jobs: &mut Jobs::new(),
    };
Repository: helix-editor/helix
File: helix-view/src/view.rs (Rust)

Code before:
    VisualOffsetError::{PosAfterMaxRow, PosBeforeAnchorRow},
};

use std::{
    collections::{HashMap, VecDeque},
    fmt,
};

const JUMP_LIST_CAPACITY: usize = 30;

type Jump = (DocumentId, Selection);

#[derive(Debug, Clone)]
pub struct JumpList {
    jumps: VecDeque<Jump>,
    current: usize,
}

impl JumpList {
    pub fn new(initial: Jump) -> Self {
        let mut jumps = VecDeque::with_capacity(JUMP_LIST_CAPACITY);
        jumps.push_back(initial);
        Self { jumps, current: 0 }
    }

    pub fn push(&mut self, jump: Jump) -> usize {
        let mut num_removed_from_front = 0;
        self.jumps.truncate(self.current);
        // don't push duplicates
        if self.jumps.back() != Some(&jump) {
            // If the jumplist is full, drop the oldest items.
            while self.jumps.len() >= JUMP_LIST_CAPACITY {
                if self.jumps.pop_front().is_some() {
                    num_removed_from_front += 1;
                };
            }

            self.jumps.push_back(jump);
            self.current = self.jumps.len();
        }
        num_removed_from_front
    }

    pub fn forward(&mut self, count: usize) -> Option<&Jump> {
        if self.current + count < self.jumps.len() {
            self.current += count;
            self.jumps.get(self.current)
        } else {
            None
        }
    }

Review comment (suggestion):
Returning the number from `push` is ok but I don't want to let this leak out into the public interface. Let's rename this `push` to `push_impl` and make it private and then introduce a new `pub fn push` that has no return and just calls `self.push_impl(jump);`

Code after:
    VisualOffsetError::{PosAfterMaxRow, PosBeforeAnchorRow},
};

use std::{
    collections::{HashMap, VecDeque},
    fmt,
};

const JUMP_LIST_CAPACITY: usize = 30;

type Jump = (DocumentId, Selection);

#[derive(Debug, Clone)]
pub struct JumpList {
    jumps: VecDeque<Jump>,
    current: usize,
}

impl JumpList {
    pub fn new(initial: Jump) -> Self {
        let mut jumps = VecDeque::with_capacity(JUMP_LIST_CAPACITY);
        jumps.push_back(initial);
        Self { jumps, current: 0 }
    }

    fn push_impl(&mut self, jump: Jump) -> usize {
        let mut num_removed_from_front = 0;
        self.jumps.truncate(self.current);
        // don't push duplicates
        if self.jumps.back() != Some(&jump) {
            // If the jumplist is full, drop the oldest item.
            while self.jumps.len() >= JUMP_LIST_CAPACITY {
                self.jumps.pop_front();
                num_removed_from_front += 1;
            }

            self.jumps.push_back(jump);
            self.current = self.jumps.len();
        }
        num_removed_from_front
    }

    pub fn push(&mut self, jump: Jump) {
        self.push_impl(jump);
    }

    pub fn forward(&mut self, count: usize) -> Option<&Jump> {
        if self.current + count < self.jumps.len() {
            self.current += count;
            self.jumps.get(self.current)
        } else {
Repository: helix-editor/helix
File: helix-view/src/handlers/lsp.rs (Rust)

Code before:
        doc: DocumentId,
        view: ViewId,
    },
    /// Some text was deleted and the cursor is now at `pos`
    DeleteText { cursor: usize },
    /// Invalidate the current auto completion trigger
    Cancel,
}

#[derive(Debug, PartialEq, Eq, Clone, Copy)]
pub enum SignatureHelpInvoked {
    Automatic,
    Manual,
}

pub enum SignatureHelpEvent {
    Invoked,
    Trigger,
    ReTrigger,
    Cancel,
    RequestComplete { open: bool },
}

pub enum PullDiagnosticsEvent {
    Trigger,
}

#[derive(Debug)]
pub struct ApplyEditError {
    pub kind: ApplyEditErrorKind,
    pub failed_change_idx: usize,
}

#[derive(Debug)]
pub enum ApplyEditErrorKind {
    DocumentChanged,
    FileNotFound,
    InvalidUrl(helix_core::uri::UrlConversionError),
    IoError(std::io::Error),
    // TODO: check edits before applying and propagate failure
    // InvalidEdit,
}

impl From<std::io::Error> for ApplyEditErrorKind {
    fn from(err: std::io::Error) -> Self {
        ApplyEditErrorKind::IoError(err)
    }
}

impl From<helix_core::uri::UrlConversionError> for ApplyEditErrorKind {
    fn from(err: helix_core::uri::UrlConversionError) -> Self {

Review comment (refactor):
Instead let's make this a struct (for now - we can always refactor as an enum later if we want to support multiple kinds of events) which carries the `DocumentId` from `event.doc.id()` on the `DocumentDidChange` event. Then in the hook you can get the document with `doc!(editor, &doc_id)` or `doc_mut!(editor, &doc_id)` for the mutable version

Code after:
    Invoked,
    Trigger,
    ReTrigger,
    Cancel,
    RequestComplete { open: bool },
}

pub struct PullDiagnosticsEvent {
    pub document_id: DocumentId,
}

pub struct PullAllDocumentsDiagnosticsEvent {
    pub language_servers: HashSet<LanguageServerId>,
}

#[derive(Debug)]
pub struct ApplyEditError {
    pub kind: ApplyEditErrorKind,
    pub failed_change_idx: usize,
}

#[derive(Debug)]
pub enum ApplyEditErrorKind {
    DocumentChanged,
    FileNotFound,
    InvalidUrl(helix_core::uri::UrlConversionError),
    IoError(std::io::Error),
    // TODO: check edits before applying and propagate failure
    // InvalidEdit,
}

impl From<std::io::Error> for ApplyEditErrorKind {
    fn from(err: std::io::Error) -> Self {
        ApplyEditErrorKind::IoError(err)
    }
}

impl From<helix_core::uri::UrlConversionError> for ApplyEditErrorKind {
    fn from(err: helix_core::uri::UrlConversionError) -> Self {
        ApplyEditErrorKind::InvalidUrl(err)
    }
}

impl Display for ApplyEditErrorKind {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        match self {
            ApplyEditErrorKind::DocumentChanged => f.write_str("document has changed"),
            ApplyEditErrorKind::FileNotFound => f.write_str("file not found"),
            ApplyEditErrorKind::InvalidUrl(err) => f.write_str(&format!("{err}")),
            ApplyEditErrorKind::IoError(err) => f.write_str(&format!("{err}")),
        }
Repository: helix-editor/helix
File: helix-term/src/handlers/diagnostics.rs (Rust)

Code before:
                ),
            };

            if let Some(doc) = editor.document_mut(document_id) {
                doc.previous_diagnostic_id = result_id;
            };

            related_documents
        }
        lsp::DocumentDiagnosticReportResult::Partial(report) => report.related_documents,
    };

    for (url, report) in related_documents.into_iter().flatten() {
        let result_id = match report {
            lsp::DocumentDiagnosticReportKind::Full(report) => {
                let Ok(uri) = Uri::try_from(url) else {
                    continue;
                };

                editor.handle_lsp_diagnostics(&provider, uri, None, report.items);
                report.result_id
            }
            lsp::DocumentDiagnosticReportKind::Unchanged(report) => Some(report.result_id),
        };

        if let Some(doc) = editor.document_mut(document_id) {
            doc.previous_diagnostic_id = result_id;
        }
    }
}


Review comment (suggestion):
It seems there might be an issue here. The code updates `doc.previous_diagnostic_id` for the `document_id`-associated document, but this section is iterating over `related_documents`, which are identified by `url`. Instead of using `document_id`, it may be more appropriate to fetch the document corresponding to the `url` and update its `previous_diagnostic_id`. 

Code after:
fn handle_pull_diagnostics_response(
    editor: &mut Editor,
    result: lsp::DocumentDiagnosticReportResult,
    provider: DiagnosticProvider,
    uri: Uri,
    document_id: DocumentId,
) {
    match result {
        lsp::DocumentDiagnosticReportResult::Report(report) => {
            let result_id = match report {
                lsp::DocumentDiagnosticReport::Full(report) => {
                    editor.handle_lsp_diagnostics(
                        &provider,
                        uri,
                        None,
                        report.full_document_diagnostic_report.items,
                    );

                    report.full_document_diagnostic_report.result_id
                }
                lsp::DocumentDiagnosticReport::Unchanged(report) => {
                    Some(report.unchanged_document_diagnostic_report.result_id)
                }
            };

            if let Some(doc) = editor.document_mut(document_id) {
                doc.previous_diagnostic_id = result_id;
            };
        }
        lsp::DocumentDiagnosticReportResult::Partial(_) => {}
    };
}
Repository: helix-editor/helix
File: helix-view/src/handlers/lsp.rs (Rust)

Code before:
use helix_lsp::util::generate_transaction_from_edits;
use helix_lsp::{lsp, LanguageServerId, OffsetEncoding};

use super::Handlers;

pub struct DocumentColorsEvent(pub DocumentId);

#[derive(Debug, PartialEq, Eq, Clone, Copy)]
pub enum SignatureHelpInvoked {
    Automatic,
    Manual,
}

pub enum SignatureHelpEvent {
    Invoked,
    Trigger,
    ReTrigger,
    Cancel,
    RequestComplete { open: bool },
}

pub struct PullDiagnosticsEvent {
    pub document_id: DocumentId,
}

pub struct PullAllDocumentsDiagnosticsEvent {}

#[derive(Debug)]
pub struct ApplyEditError {
    pub kind: ApplyEditErrorKind,
    pub failed_change_idx: usize,
}

#[derive(Debug)]
pub enum ApplyEditErrorKind {
    DocumentChanged,
    FileNotFound,
    InvalidUrl(helix_core::uri::UrlConversionError),
    IoError(std::io::Error),
    // TODO: check edits before applying and propagate failure
    // InvalidEdit,
}

impl From<std::io::Error> for ApplyEditErrorKind {
    fn from(err: std::io::Error) -> Self {
        ApplyEditErrorKind::IoError(err)
    }
}

impl From<helix_core::uri::UrlConversionError> for ApplyEditErrorKind {
    fn from(err: helix_core::uri::UrlConversionError) -> Self {

Review comment (suggestion):
For this hook I think we should try to pass in the `LanguageServerId`s of language servers that declare inter-file dependencies. You might have a simpler language server (for example one checking for typos) and requesting to refresh all of its documents would be a bit wasteful. It's a bit of a hypothetical scenario though - I wouldn't consider this to be a merge blocker.

Code after:
use helix_event::register_hook;
use helix_lsp::util::generate_transaction_from_edits;
use helix_lsp::{lsp, LanguageServerId, OffsetEncoding};

use super::Handlers;

pub struct DocumentColorsEvent(pub DocumentId);

#[derive(Debug, PartialEq, Eq, Clone, Copy)]
pub enum SignatureHelpInvoked {
    Automatic,
    Manual,
}

pub enum SignatureHelpEvent {
    Invoked,
    Trigger,
    ReTrigger,
    Cancel,
    RequestComplete { open: bool },
}

pub struct PullDiagnosticsEvent {
    pub document_id: DocumentId,
}

pub struct PullAllDocumentsDiagnosticsEvent {
    pub language_servers: HashSet<LanguageServerId>,
}

#[derive(Debug)]
pub struct ApplyEditError {
    pub kind: ApplyEditErrorKind,
    pub failed_change_idx: usize,
}

#[derive(Debug)]
pub enum ApplyEditErrorKind {
    DocumentChanged,
    FileNotFound,
    InvalidUrl(helix_core::uri::UrlConversionError),
    IoError(std::io::Error),
    // TODO: check edits before applying and propagate failure
    // InvalidEdit,
}

impl From<std::io::Error> for ApplyEditErrorKind {
    fn from(err: std::io::Error) -> Self {
        ApplyEditErrorKind::IoError(err)
    }
}
Repository: helix-editor/helix
File: helix-term/src/commands/lsp.rs (Rust)

Code before:
        let language_server = $doc.language_servers_with_feature($feature).next();
        match language_server {
            Some(language_server) => language_server,
            None => {
                $editor.set_status(format!(
                    "No configured language server supports {}",
                    $feature
                ));
                return;
            }
        }
    }};
}

/// A wrapper around `lsp::Location` that swaps out the LSP URI for `helix_core::Uri`.
#[derive(Debug, Clone, PartialEq, Eq)]
struct Location {
    uri: Uri,
    range: lsp::Range,
}

fn lsp_location_to_location(location: lsp::Location) -> Option<Location> {
    let uri = match location.uri.try_into() {
        Ok(uri) => uri,
        Err(err) => {
            log::warn!(
                "discarding LSP location which could not be converted to a core location: {err}"
            );
            return None;
        }
    };
    Some(Location {
        uri,
        range: location.range,
    })
}

struct SymbolInformationItem {
    location: Location,
    symbol: lsp::SymbolInformation,
    offset_encoding: OffsetEncoding,
}

struct DiagnosticStyles {
    hint: Style,
    info: Style,
    warning: Style,
    error: Style,
}

struct PickerDiagnostic {

Review comment (suggestion):
I think this one could use a better error message.

For example `"discarding invalid or unsupported URI {uri}: {err}"`.

The current message is very focused on the implementation but what matters to users is that `foo:://` uris are not supported by helix. 

Code after:
        let language_server = $doc.language_servers_with_feature($feature).next();
        match language_server {
            Some(language_server) => language_server,
            None => {
                $editor.set_status(format!(
                    "No configured language server supports {}",
                    $feature
                ));
                return;
            }
        }
    }};
}

/// A wrapper around `lsp::Location` that swaps out the LSP URI for `helix_core::Uri`.
#[derive(Debug, Clone, PartialEq, Eq)]
struct Location {
    uri: Uri,
    range: lsp::Range,
}

fn lsp_location_to_location(location: lsp::Location) -> Option<Location> {
    let uri = match location.uri.try_into() {
        Ok(uri) => uri,
        Err(err) => {
            log::warn!("discarding invalid or unsupported URI: {err}");
            return None;
        }
    };
    Some(Location {
        uri,
        range: location.range,
    })
}

struct SymbolInformationItem {
    location: Location,
    symbol: lsp::SymbolInformation,
    offset_encoding: OffsetEncoding,
}

struct DiagnosticStyles {
    hint: Style,
    info: Style,
    warning: Style,
    error: Style,
}

struct PickerDiagnostic {
    location: Location,
    diag: lsp::Diagnostic,
Repository: helix-editor/helix
File: helix-term/src/commands/typed.rs (Rust)

Code before:
) -> anyhow::Result<()> {
    if event != PromptEvent::Validate {
        return Ok(());
    }

    cx.editor.new_file(Action::Replace);

    Ok(())
}

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

    let (view, doc) = current!(cx.editor);
    if let Some(format) = doc.format() {
        let callback = make_format_callback(doc.id(), doc.version(), view.id, format, None);
        cx.jobs.callback(callback);
    } else {
        cx.editor
            .set_error("Formater not available and no LSP has formatting capabilities");
    }

    Ok(())
}

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

    use IndentStyle::*;

    // If no argument, report current indent style.
    if args.is_empty() {
        let style = doc!(cx.editor).indent_style;
        cx.editor.set_status(match style {
            Tabs => "tabs".to_owned(),
            Spaces(1) => "1 space".to_owned(),
            Spaces(n) => format!("{} spaces", n),
        });
        return Ok(());

Review comment (suggestion):
For typable commands you can return an `Err` for the same effect as `cx.editor.set_error`:

```suggestion
        bail!("Formatter not available and no language server has formatting capabilities");
```

Code after:
) -> anyhow::Result<()> {
    if event != PromptEvent::Validate {
        return Ok(());
    }

    cx.editor.new_file(Action::Replace);

    Ok(())
}

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

    let (view, doc) = current!(cx.editor);
    let format = doc.format().context(
        "A formatter isn't available, and no language server provides formatting capabilities",
    )?;
    let callback = make_format_callback(doc.id(), doc.version(), view.id, format, None);
    cx.jobs.callback(callback);

    Ok(())
}

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

    use IndentStyle::*;

    // If no argument, report current indent style.
    if args.is_empty() {
        let style = doc!(cx.editor).indent_style;
        cx.editor.set_status(match style {
            Tabs => "tabs".to_owned(),
            Spaces(1) => "1 space".to_owned(),
            Spaces(n) => format!("{} spaces", n),
        });
        return Ok(());
    }