Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

LSP: Key diagnostics off file path instead of URI #7367

Merged
merged 2 commits into from
Feb 25, 2024
Merged

LSP: Key diagnostics off file path instead of URI #7367

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7ea16ad
LSP: Key diagnostics off file path instead of URI
the-mikedavis Aug 27, 2023
fbd31db
LSP: Normalize diagnostic file paths
the-mikedavis Jun 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions helix-term/src/application.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -729,7 +729,7 @@ impl Application {
}
Notification::PublishDiagnostics(mut params) => {
let path = match params.uri.to_file_path() {
Ok(path) => path,
Ok(path) => helix_stdx::path::normalize(&path),
Err(_) => {
log::error!("Unsupported file URI: {}", params.uri);
return;
Expand Down Expand Up @@ -758,9 +758,7 @@ impl Application {
let lang_conf = doc.language.clone();

if let Some(lang_conf) = &lang_conf {
if let Some(old_diagnostics) =
self.editor.diagnostics.get(&params.uri)
{
if let Some(old_diagnostics) = self.editor.diagnostics.get(&path) {
if !lang_conf.persistent_diagnostic_sources.is_empty() {
// Sort diagnostics first by severity and then by line numbers.
// Note: The `lsp::DiagnosticSeverity` enum is already defined in decreasing order
Expand Down Expand Up @@ -793,7 +791,7 @@ impl Application {
// Insert the original lsp::Diagnostics here because we may have no open document
// for diagnosic message and so we can't calculate the exact position.
// When using them later in the diagnostics picker, we calculate them on-demand.
let diagnostics = match self.editor.diagnostics.entry(params.uri) {
let diagnostics = match self.editor.diagnostics.entry(path) {
Entry::Occupied(o) => {
let current_diagnostics = o.into_mut();
// there may entries of other language servers, which is why we can't overwrite the whole entry
Expand Down
72 changes: 33 additions & 39 deletions helix-term/src/commands/lsp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ use std::{
collections::{BTreeMap, HashSet},
fmt::Write,
future::Future,
path::PathBuf,
path::{Path, PathBuf},
};

/// Gets the first language server that is attached to a document which supports a specific feature.
Expand Down Expand Up @@ -134,7 +134,7 @@ struct DiagnosticStyles {
}

struct PickerDiagnostic {
url: lsp::Url,
path: PathBuf,
diag: lsp::Diagnostic,
offset_encoding: OffsetEncoding,
}
Expand Down Expand Up @@ -167,8 +167,7 @@ impl ui::menu::Item for PickerDiagnostic {
let path = match format {
DiagnosticsFormat::HideSourcePath => String::new(),
DiagnosticsFormat::ShowSourcePath => {
let file_path = self.url.to_file_path().unwrap();
let path = path::get_truncated_path(file_path);
let path = path::get_truncated_path(&self.path);
format!("{}: ", path.to_string_lossy())
}
};
Expand Down Expand Up @@ -208,24 +207,33 @@ fn jump_to_location(
return;
}
};
jump_to_position(editor, &path, location.range, offset_encoding, action);
}

let doc = match editor.open(&path, action) {
fn jump_to_position(
editor: &mut Editor,
path: &Path,
range: lsp::Range,
offset_encoding: OffsetEncoding,
action: Action,
) {
let doc = match editor.open(path, action) {
Ok(id) => doc_mut!(editor, &id),
Err(err) => {
let err = format!("failed to open path: {:?}: {:?}", location.uri, err);
let err = format!("failed to open path: {:?}: {:?}", path, err);
editor.set_error(err);
return;
}
};
let view = view_mut!(editor);
// TODO: convert inside server
let new_range =
if let Some(new_range) = lsp_range_to_range(doc.text(), location.range, offset_encoding) {
new_range
} else {
log::warn!("lsp position out of bounds - {:?}", location.range);
return;
};
let new_range = if let Some(new_range) = lsp_range_to_range(doc.text(), range, offset_encoding)
{
new_range
} else {
log::warn!("lsp position out of bounds - {:?}", range);
return;
};
// we flip the range so that the cursor sits on the start of the symbol
// (for example start of the function).
doc.set_selection(view.id, Selection::single(new_range.head, new_range.anchor));
Expand Down Expand Up @@ -258,21 +266,20 @@ enum DiagnosticsFormat {

fn diag_picker(
cx: &Context,
diagnostics: BTreeMap<lsp::Url, Vec<(lsp::Diagnostic, usize)>>,
_current_path: Option<lsp::Url>,
diagnostics: BTreeMap<PathBuf, Vec<(lsp::Diagnostic, usize)>>,
format: DiagnosticsFormat,
) -> Picker<PickerDiagnostic> {
// TODO: drop current_path comparison and instead use workspace: bool flag?

// flatten the map to a vec of (url, diag) pairs
let mut flat_diag = Vec::new();
for (url, diags) in diagnostics {
for (path, diags) in diagnostics {
flat_diag.reserve(diags.len());

for (diag, ls) in diags {
if let Some(ls) = cx.editor.language_server_by_id(ls) {
flat_diag.push(PickerDiagnostic {
url: url.clone(),
path: path.clone(),
diag,
offset_encoding: ls.offset_encoding(),
});
Expand All @@ -292,22 +299,17 @@ fn diag_picker(
(styles, format),
move |cx,
PickerDiagnostic {
url,
path,
diag,
offset_encoding,
},
action| {
jump_to_location(
cx.editor,
&lsp::Location::new(url.clone(), diag.range),
*offset_encoding,
action,
)
jump_to_position(cx.editor, path, diag.range, *offset_encoding, action)
},
)
.with_preview(move |_editor, PickerDiagnostic { url, diag, .. }| {
let location = lsp::Location::new(url.clone(), diag.range);
Some(location_to_file_location(&location))
.with_preview(move |_editor, PickerDiagnostic { path, diag, .. }| {
let line = Some((diag.range.start.line as usize, diag.range.end.line as usize));
Some((path.clone().into(), line))
})
.truncate_start(false)
}
Expand Down Expand Up @@ -470,34 +472,26 @@ pub fn workspace_symbol_picker(cx: &mut Context) {

pub fn diagnostics_picker(cx: &mut Context) {
let doc = doc!(cx.editor);
if let Some(current_url) = doc.url() {
if let Some(current_path) = doc.path() {
let diagnostics = cx
.editor
.diagnostics
.get(&current_url)
.get(current_path)
.cloned()
.unwrap_or_default();
let picker = diag_picker(
cx,
[(current_url.clone(), diagnostics)].into(),
Some(current_url),
[(current_path.clone(), diagnostics)].into(),
DiagnosticsFormat::HideSourcePath,
);
cx.push_layer(Box::new(overlaid(picker)));
}
}

pub fn workspace_diagnostics_picker(cx: &mut Context) {
let doc = doc!(cx.editor);
let current_url = doc.url();
// TODO not yet filtered by LanguageServerFeature, need to do something similar as Document::shown_diagnostics here for all open documents
let diagnostics = cx.editor.diagnostics.clone();
let picker = diag_picker(
cx,
diagnostics,
current_url,
DiagnosticsFormat::ShowSourcePath,
);
let picker = diag_picker(cx, diagnostics, DiagnosticsFormat::ShowSourcePath);
cx.push_layer(Box::new(overlaid(picker)));
}

Expand Down
9 changes: 4 additions & 5 deletions helix-view/src/editor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -911,7 +911,7 @@ pub struct Editor {
pub macro_recording: Option<(char, Vec<KeyEvent>)>,
pub macro_replaying: Vec<char>,
pub language_servers: helix_lsp::Registry,
pub diagnostics: BTreeMap<lsp::Url, Vec<(lsp::Diagnostic, usize)>>,
pub diagnostics: BTreeMap<PathBuf, Vec<(lsp::Diagnostic, usize)>>,
pub diff_providers: DiffProviderRegistry,

pub debugger: Option<dap::Client>,
Expand Down Expand Up @@ -1809,7 +1809,7 @@ impl Editor {
/// Returns all supported diagnostics for the document
pub fn doc_diagnostics<'a>(
language_servers: &'a helix_lsp::Registry,
diagnostics: &'a BTreeMap<lsp::Url, Vec<(lsp::Diagnostic, usize)>>,
diagnostics: &'a BTreeMap<PathBuf, Vec<(lsp::Diagnostic, usize)>>,
document: &Document,
) -> impl Iterator<Item = helix_core::Diagnostic> + 'a {
Editor::doc_diagnostics_with_filter(language_servers, diagnostics, document, |_, _| true)
Expand All @@ -1819,7 +1819,7 @@ impl Editor {
/// filtered by `filter` which is invocated with the raw `lsp::Diagnostic` and the language server id it came from
pub fn doc_diagnostics_with_filter<'a>(
language_servers: &'a helix_lsp::Registry,
diagnostics: &'a BTreeMap<lsp::Url, Vec<(lsp::Diagnostic, usize)>>,
diagnostics: &'a BTreeMap<PathBuf, Vec<(lsp::Diagnostic, usize)>>,

document: &Document,
filter: impl Fn(&lsp::Diagnostic, usize) -> bool + 'a,
Expand All @@ -1828,8 +1828,7 @@ impl Editor {
let language_config = document.language.clone();
document
.path()
.and_then(|path| url::Url::from_file_path(path).ok()) // TODO log error?
.and_then(|uri| diagnostics.get(&uri))
.and_then(|path| diagnostics.get(path))
.map(|diags| {
diags.iter().filter_map(move |(diagnostic, lsp_id)| {
let ls = language_servers.get_by_id(*lsp_id)?;
Expand Down
Loading