From a7460b1242b725d7b9c0d4015f3d27755c9a5fa9 Mon Sep 17 00:00:00 2001 From: Joe Neeman Date: Thu, 1 Feb 2024 17:27:27 -0600 Subject: [PATCH 1/4] Add rename support (Cross-file renaming is only working in one direction) --- lsp/lsp-harness/src/lib.rs | 6 ++-- lsp/lsp-harness/src/output.rs | 22 +++++++++++- lsp/nls/src/requests/mod.rs | 1 + lsp/nls/src/requests/rename.rs | 59 +++++++++++++++++++++++++++++++++ lsp/nls/src/server.rs | 11 ++++-- lsp/nls/tests/inputs/rename.ncl | 55 ++++++++++++++++++++++++++++++ 6 files changed, 149 insertions(+), 5 deletions(-) create mode 100644 lsp/nls/src/requests/rename.rs create mode 100644 lsp/nls/tests/inputs/rename.ncl diff --git a/lsp/lsp-harness/src/lib.rs b/lsp/lsp-harness/src/lib.rs index 67fcddf8e..c917b1a9a 100644 --- a/lsp/lsp-harness/src/lib.rs +++ b/lsp/lsp-harness/src/lib.rs @@ -10,10 +10,10 @@ use lsp_types::{ notification::{Notification, PublishDiagnostics}, request::{ Completion, DocumentSymbolRequest, Formatting, GotoDefinition, HoverRequest, References, - Request as LspRequest, + Rename, Request as LspRequest, }, CompletionParams, DocumentFormattingParams, DocumentSymbolParams, GotoDefinitionParams, - HoverParams, PublishDiagnosticsParams, ReferenceParams, Url, + HoverParams, PublishDiagnosticsParams, ReferenceParams, RenameParams, Url, }; pub use output::LspDebug; use serde::Deserialize; @@ -43,6 +43,7 @@ pub enum Request { Completion(CompletionParams), Formatting(DocumentFormattingParams), Hover(HoverParams), + Rename(RenameParams), Symbols(DocumentSymbolParams), } @@ -142,6 +143,7 @@ impl TestHarness { Request::Formatting(f) => self.request::(f), Request::Hover(h) => self.request::(h), Request::References(r) => self.request::(r), + Request::Rename(r) => self.request::(r), Request::Symbols(s) => self.request::(s), } } diff --git a/lsp/lsp-harness/src/output.rs b/lsp/lsp-harness/src/output.rs index 013cb358d..212199dfe 100644 --- a/lsp/lsp-harness/src/output.rs +++ b/lsp/lsp-harness/src/output.rs @@ -4,7 +4,7 @@ use std::io::Write; -use lsp_types::{DocumentSymbolResponse, GotoDefinitionResponse}; +use lsp_types::{DocumentSymbolResponse, GotoDefinitionResponse, WorkspaceEdit}; pub trait LspDebug { fn debug(&self, w: impl Write) -> std::io::Result<()>; @@ -54,6 +54,12 @@ impl LspDebug for Vec { } } +impl LspDebug for (S, T) { + fn debug(&self, mut w: impl Write) -> std::io::Result<()> { + write!(w, "({}, {})", self.0.debug_str(), self.1.debug_str()) + } +} + impl LspDebug for lsp_types::Range { fn debug(&self, mut w: impl Write) -> std::io::Result<()> { write!( @@ -64,6 +70,12 @@ impl LspDebug for lsp_types::Range { } } +impl LspDebug for lsp_types::Url { + fn debug(&self, mut w: impl Write) -> std::io::Result<()> { + write!(w, "{}", self.as_str()) + } +} + impl LspDebug for lsp_types::Location { fn debug(&self, mut w: impl Write) -> std::io::Result<()> { write!(w, "{}:{}", self.uri.as_str(), self.range.debug_str()) @@ -188,3 +200,11 @@ impl LspDebug for DocumentSymbolResponse { } } } + +impl LspDebug for WorkspaceEdit { + fn debug(&self, w: impl Write) -> std::io::Result<()> { + let changes = self.changes.clone(); + let changes = changes.into_iter().flatten().collect::>(); + changes.debug(w) + } +} diff --git a/lsp/nls/src/requests/mod.rs b/lsp/nls/src/requests/mod.rs index 5a6e851f2..552619fc3 100644 --- a/lsp/nls/src/requests/mod.rs +++ b/lsp/nls/src/requests/mod.rs @@ -1,6 +1,7 @@ pub mod completion; pub mod goto; pub mod hover; +pub mod rename; pub mod symbols; #[cfg(feature = "format")] diff --git a/lsp/nls/src/requests/rename.rs b/lsp/nls/src/requests/rename.rs new file mode 100644 index 000000000..910e9a325 --- /dev/null +++ b/lsp/nls/src/requests/rename.rs @@ -0,0 +1,59 @@ +use std::collections::HashMap; + +use lsp_server::{RequestId, Response, ResponseError}; +use lsp_types::{Range, RenameParams, TextEdit, Url, WorkspaceEdit}; +use nickel_lang_core::term::{RichTerm, Term, UnaryOp}; + +use crate::cache::CacheExt as _; +use crate::diagnostic::LocationCompat; +use crate::requests::goto::get_defs; +use crate::server::Server; + +pub fn handle_rename( + params: RenameParams, + id: RequestId, + server: &mut Server, +) -> Result<(), ResponseError> { + let pos = server.cache.position(¶ms.text_document_position)?; + + let ident = server.lookup_ident_by_position(pos)?; + let term = server.lookup_term_by_position(pos)?; + let mut def_locs = term + .and_then(|term| get_defs(term, ident, server)) + .unwrap_or_default(); + def_locs.extend(ident.and_then(|id| id.pos.into_opt())); + if let Some(Term::Op1(UnaryOp::StaticAccess(id), _)) = term.map(RichTerm::as_ref) { + def_locs.extend(id.pos.into_opt()); + } + + let mut all_positions: Vec<_> = def_locs + .iter() + .flat_map(|id| server.analysis.get_usages(id)) + .filter_map(|id| id.pos.into_opt()) + .chain(def_locs.iter().copied()) + .collect(); + + // Sort in some arbitrary order, for determinism and deduplication. + all_positions.sort_by_key(|span| (span.src_id, span.start, span.end)); + all_positions.dedup(); + + // Group edits by file + let mut changes = HashMap::>::new(); + for pos in all_positions { + let url = Url::from_file_path(server.cache.files().name(pos.src_id)).unwrap(); + changes.entry(url).or_default().push(TextEdit { + range: Range::from_span(&pos, server.cache.files()), + new_text: params.new_name.clone(), + }); + } + + server.reply(Response::new_ok( + id, + WorkspaceEdit { + changes: Some(changes), + document_changes: None, + change_annotations: None, + }, + )); + Ok(()) +} diff --git a/lsp/nls/src/server.rs b/lsp/nls/src/server.rs index e21b198be..cb30ce8ec 100644 --- a/lsp/nls/src/server.rs +++ b/lsp/nls/src/server.rs @@ -17,7 +17,7 @@ use lsp_types::{ CodeActionParams, CompletionOptions, CompletionParams, DidChangeTextDocumentParams, DidOpenTextDocumentParams, DocumentFormattingParams, DocumentSymbolParams, ExecuteCommandParams, GotoDefinitionParams, HoverOptions, HoverParams, HoverProviderCapability, - OneOf, PublishDiagnosticsParams, ReferenceParams, ServerCapabilities, + OneOf, PublishDiagnosticsParams, ReferenceParams, RenameParams, ServerCapabilities, TextDocumentSyncCapability, TextDocumentSyncKind, TextDocumentSyncOptions, Url, WorkDoneProgressOptions, }; @@ -39,7 +39,7 @@ use crate::{ field_walker::{Def, FieldResolver}, identifier::LocIdent, pattern::Bindings, - requests::{completion, formatting, goto, hover, symbols}, + requests::{completion, formatting, goto, hover, rename, symbols}, trace::Trace, }; @@ -93,6 +93,7 @@ impl Server { commands: vec!["eval".to_owned()], ..Default::default() }), + rename_provider: Some(OneOf::Left(true)), ..ServerCapabilities::default() } } @@ -288,6 +289,12 @@ impl Server { command::handle_command(params, req.id.clone(), self) } + Rename::METHOD => { + debug!("rename"); + let params: RenameParams = serde_json::from_value(req.params).unwrap(); + rename::handle_rename(params, req.id.clone(), self) + } + _ => Ok(()), }; diff --git a/lsp/nls/tests/inputs/rename.ncl b/lsp/nls/tests/inputs/rename.ncl new file mode 100644 index 000000000..60ac3b468 --- /dev/null +++ b/lsp/nls/tests/inputs/rename.ncl @@ -0,0 +1,55 @@ +### /base.ncl +let foo = import "dep.ncl" in +{ + x = foo.included, + y = { foo = x }, +} +### /dep.ncl +{ + included = 5 +} +### # Rename foo to a bar (but not the "foo" in "foo = x") +### [[request]] +### type = "Rename" +### textDocument.uri = "file:///base.ncl" +### position = { line = 0, character = 5 } +### newName = "bar" +### +### [[request]] +### type = "Rename" +### textDocument.uri = "file:///base.ncl" +### position = { line = 2, character = 9 } +### newName = "bar" +### +### # Rename "foo" in "foo = x" (shouldn't rename the others) +### [[request]] +### type = "Rename" +### textDocument.uri = "file:///base.ncl" +### position = { line = 3, character = 11 } +### newName = "baz" +### +### # Rename x to y +### [[request]] +### type = "Rename" +### textDocument.uri = "file:///base.ncl" +### position = { line = 2, character = 4 } +### newName = "y" +### +### [[request]] +### type = "Rename" +### textDocument.uri = "file:///base.ncl" +### position = { line = 3, character = 16 } +### newName = "y" +### +### # Cross-file rename +### [[request]] +### type = "Rename" +### textDocument.uri = "file:///base.ncl" +### position = { line = 2, character = 15 } +### newName = "dependency" +### +### [[request]] +### type = "Rename" +### textDocument.uri = "file:///dep.ncl" +### position = { line = 1, character = 5 } +### newName = "dependency" From 3816b5cdd90ec22e35431c32504d9c9d37dbca49 Mon Sep 17 00:00:00 2001 From: Joe Neeman Date: Thu, 8 Feb 2024 12:10:23 -0600 Subject: [PATCH 2/4] Fix some rebase issues --- lsp/nls/src/requests/rename.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/lsp/nls/src/requests/rename.rs b/lsp/nls/src/requests/rename.rs index 910e9a325..7b1af0f8c 100644 --- a/lsp/nls/src/requests/rename.rs +++ b/lsp/nls/src/requests/rename.rs @@ -2,11 +2,9 @@ use std::collections::HashMap; use lsp_server::{RequestId, Response, ResponseError}; use lsp_types::{Range, RenameParams, TextEdit, Url, WorkspaceEdit}; -use nickel_lang_core::term::{RichTerm, Term, UnaryOp}; use crate::cache::CacheExt as _; use crate::diagnostic::LocationCompat; -use crate::requests::goto::get_defs; use crate::server::Server; pub fn handle_rename( @@ -19,18 +17,17 @@ pub fn handle_rename( let ident = server.lookup_ident_by_position(pos)?; let term = server.lookup_term_by_position(pos)?; let mut def_locs = term - .and_then(|term| get_defs(term, ident, server)) + .map(|term| server.get_defs(term, ident)) .unwrap_or_default(); + def_locs.extend(ident.and_then(|id| id.pos.into_opt())); - if let Some(Term::Op1(UnaryOp::StaticAccess(id), _)) = term.map(RichTerm::as_ref) { - def_locs.extend(id.pos.into_opt()); - } let mut all_positions: Vec<_> = def_locs .iter() .flat_map(|id| server.analysis.get_usages(id)) .filter_map(|id| id.pos.into_opt()) .chain(def_locs.iter().copied()) + .chain(def_locs.iter().flat_map(|def| server.get_field_refs(*def))) .collect(); // Sort in some arbitrary order, for determinism and deduplication. From 364b9c9510a44a9fc448e95481d01376e9f585df Mon Sep 17 00:00:00 2001 From: Joe Neeman Date: Thu, 8 Feb 2024 12:10:48 -0600 Subject: [PATCH 3/4] Insta review --- .../main__lsp__nls__tests__inputs__rename.ncl.snap | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__rename.ncl.snap diff --git a/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__rename.ncl.snap b/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__rename.ncl.snap new file mode 100644 index 000000000..22d5642d7 --- /dev/null +++ b/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__rename.ncl.snap @@ -0,0 +1,12 @@ +--- +source: lsp/nls/tests/main.rs +expression: output +--- +[(file:///base.ncl, [<0:4-0:7> bar, <2:8-2:11> bar])] +[(file:///base.ncl, [<0:4-0:7> bar, <2:8-2:11> bar])] +[(file:///base.ncl, [<3:10-3:13> baz])] +[(file:///base.ncl, [<2:4-2:5> y, <3:16-3:17> y])] +[(file:///base.ncl, [<2:4-2:5> y, <3:16-3:17> y])] +[(file:///dep.ncl, [<1:2-1:10> dependency]), (file:///base.ncl, [<2:12-2:20> dependency])] +[(file:///dep.ncl, [<1:2-1:10> dependency]), (file:///base.ncl, [<2:12-2:20> dependency])] + From 4160c029b551a7c4965929a3b9e6b600dd11b8e0 Mon Sep 17 00:00:00 2001 From: Joe Neeman Date: Thu, 8 Feb 2024 12:54:16 -0600 Subject: [PATCH 4/4] Make the test deterministic --- lsp/lsp-harness/src/output.rs | 4 +++- .../snapshots/main__lsp__nls__tests__inputs__rename.ncl.snap | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/lsp/lsp-harness/src/output.rs b/lsp/lsp-harness/src/output.rs index 212199dfe..3a76a0589 100644 --- a/lsp/lsp-harness/src/output.rs +++ b/lsp/lsp-harness/src/output.rs @@ -204,7 +204,9 @@ impl LspDebug for DocumentSymbolResponse { impl LspDebug for WorkspaceEdit { fn debug(&self, w: impl Write) -> std::io::Result<()> { let changes = self.changes.clone(); - let changes = changes.into_iter().flatten().collect::>(); + let mut changes = changes.into_iter().flatten().collect::>(); + // Sort the keys, for determinism + changes.sort_by_key(|(url, _)| url.clone()); changes.debug(w) } } diff --git a/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__rename.ncl.snap b/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__rename.ncl.snap index 22d5642d7..df1b79e06 100644 --- a/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__rename.ncl.snap +++ b/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__rename.ncl.snap @@ -7,6 +7,6 @@ expression: output [(file:///base.ncl, [<3:10-3:13> baz])] [(file:///base.ncl, [<2:4-2:5> y, <3:16-3:17> y])] [(file:///base.ncl, [<2:4-2:5> y, <3:16-3:17> y])] -[(file:///dep.ncl, [<1:2-1:10> dependency]), (file:///base.ncl, [<2:12-2:20> dependency])] -[(file:///dep.ncl, [<1:2-1:10> dependency]), (file:///base.ncl, [<2:12-2:20> dependency])] +[(file:///base.ncl, [<2:12-2:20> dependency]), (file:///dep.ncl, [<1:2-1:10> dependency])] +[(file:///base.ncl, [<2:12-2:20> dependency]), (file:///dep.ncl, [<1:2-1:10> dependency])]