Skip to content

Commit d1a8249

Browse files
authored
fix(lsp): panic on goto std files (nushell#16694)
Fixes a new issue introduced by nushell#16568 ```nushell use std/assert ``` Goto def on `assert` will cause panic, since std files are cached with relative paths as names. This PR simply skips those files. Perhaps a better way would be to find them in the vendor std-lib directory, which I'm not sure how to get correctly. ## Release notes summary - What our users need to know ## Tasks after submitting
1 parent 4062cbd commit d1a8249

File tree

6 files changed

+79
-32
lines changed

6 files changed

+79
-32
lines changed

crates/nu-lsp/src/goto.rs

Lines changed: 39 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,17 @@ impl LanguageServer {
1515
for cached_file in files.into_iter() {
1616
if cached_file.covered_span.contains(span.start) {
1717
let path = Path::new(&*cached_file.name);
18+
// skip nu-std files
19+
// TODO: maybe find them in vendor directories?
20+
if path.is_relative() {
21+
let _ = self.send_log_message(
22+
lsp_types::MessageType::WARNING,
23+
format!(
24+
"Location found in file {path:?}, but absolute path is expected. Skipping..."
25+
),
26+
);
27+
continue;
28+
}
1829
let target_uri = path_to_uri(path);
1930
if let Some(file) = self.docs.lock().ok()?.get_document(&target_uri) {
2031
return Some(Location {
@@ -99,7 +110,7 @@ mod tests {
99110
use crate::path_to_uri;
100111
use crate::tests::{initialize_language_server, open, open_unchecked, result_from_message};
101112
use assert_json_diff::assert_json_eq;
102-
use lsp_server::{Connection, Message};
113+
use lsp_server::{Connection, Message, Notification};
103114
use lsp_types::{
104115
GotoDefinitionParams, PartialResultParams, Position, TextDocumentIdentifier,
105116
TextDocumentPositionParams, Uri, WorkDoneProgressParams,
@@ -146,7 +157,7 @@ mod tests {
146157
let script = path_to_uri(&none_existent_path);
147158
let resp = send_goto_definition_request(&client_connection, script, 0, 0);
148159

149-
assert_json_eq!(result_from_message(resp), serde_json::json!(null));
160+
assert_json_eq!(result_from_message(resp), serde_json::Value::Null);
150161
}
151162

152163
#[rstest]
@@ -189,24 +200,21 @@ mod tests {
189200
if let Some(name) = expected_file {
190201
target_uri = target_uri.replace(filename, name);
191202
}
192-
assert_json_eq!(
193-
result.pointer("/uri").unwrap(),
194-
serde_json::json!(target_uri)
195-
);
203+
assert_json_eq!(result["uri"], serde_json::json!(target_uri));
196204
let (line, character) = expected_start;
197205
assert_json_eq!(
198-
result.pointer("/range/start").unwrap(),
206+
result["range"]["start"],
199207
serde_json::json!({ "line": line, "character": character })
200208
);
201209
if let Some((line, character)) = expected_end {
202210
assert_json_eq!(
203-
result.pointer("/range/end").unwrap(),
211+
result["range"]["end"],
204212
serde_json::json!({ "line": line, "character": character })
205213
);
206214
}
207215
}
208216

209-
#[rstest]
217+
#[test]
210218
// https://github.com/nushell/nushell/issues/16539
211219
fn goto_definition_in_new_file() {
212220
let (client_connection, _recv) = initialize_language_server(None, None);
@@ -240,4 +248,26 @@ mod tests {
240248
})
241249
);
242250
}
251+
252+
#[test]
253+
fn goto_definition_on_stdlib_should_not_panic() {
254+
let (client_connection, _recv) = initialize_language_server(None, None);
255+
256+
let mut script = fixtures();
257+
script.push("lsp/goto/use_module.nu");
258+
let script = path_to_uri(&script);
259+
260+
open_unchecked(&client_connection, script.clone());
261+
let resp = send_goto_definition_request(&client_connection, script, 7, 19);
262+
match resp {
263+
Message::Notification(Notification { params, .. }) => {
264+
assert!(
265+
params["message"]
266+
.to_string()
267+
.contains("absolute path is expected")
268+
);
269+
}
270+
_ => panic!("Unexpected message!"),
271+
}
272+
}
243273
}

crates/nu-lsp/src/hover.rs

Lines changed: 3 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -263,9 +263,7 @@ mod hover_tests {
263263
let resp = send_hover_request(&client_connection, script, line, character);
264264

265265
assert_json_eq!(
266-
result_from_message(resp)
267-
.pointer("/contents/value")
268-
.unwrap(),
266+
result_from_message(resp)["contents"]["value"],
269267
serde_json::json!(expected)
270268
);
271269
}
@@ -281,12 +279,7 @@ mod hover_tests {
281279
open_unchecked(&client_connection, script.clone());
282280
let resp = send_hover_request(&client_connection, script, 6, 2);
283281

284-
let hover_text = result_from_message(resp)
285-
.pointer("/contents/value")
286-
.unwrap()
287-
.as_str()
288-
.unwrap()
289-
.to_string();
282+
let hover_text = result_from_message(resp)["contents"]["value"].to_string();
290283

291284
#[cfg(not(windows))]
292285
assert!(hover_text.contains("SLEEP"));
@@ -321,11 +314,7 @@ mod hover_tests {
321314
let resp = send_hover_request(&client_connection, script_uri, line, character);
322315
let result = result_from_message(resp);
323316

324-
let actual = result
325-
.pointer("/contents/value")
326-
.unwrap()
327-
.to_string()
328-
.replace("\\r", "");
317+
let actual = result["contents"]["value"].to_string().replace("\\r", "");
329318

330319
assert!(actual.starts_with(expected_prefix));
331320
}

crates/nu-lsp/src/lib.rs

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,8 @@
22
use lsp_server::{Connection, IoThreads, Message, Response, ResponseError};
33
use lsp_textdocument::{FullTextDocument, TextDocuments};
44
use lsp_types::{
5-
InlayHint, OneOf, Position, Range, ReferencesOptions, RenameOptions, SemanticToken,
6-
SemanticTokenType, SemanticTokensLegend, SemanticTokensOptions,
5+
InlayHint, MessageType, OneOf, Position, Range, ReferencesOptions, RenameOptions,
6+
SemanticToken, SemanticTokenType, SemanticTokensLegend, SemanticTokensOptions,
77
SemanticTokensServerCapabilities, ServerCapabilities, SignatureHelpOptions,
88
TextDocumentSyncKind, Uri, WorkDoneProgressOptions, WorkspaceFolder,
99
WorkspaceFoldersServerCapabilities, WorkspaceServerCapabilities,
@@ -291,6 +291,10 @@ impl LanguageServer {
291291
pub(crate) fn cancel_background_thread(&mut self) {
292292
if let Some((sender, _)) = &self.channels {
293293
sender.send(true).ok();
294+
let _ = self.send_log_message(
295+
MessageType::WARNING,
296+
"Workspace-wide search took too long!".into(),
297+
);
294298
}
295299
}
296300

@@ -510,7 +514,7 @@ mod tests {
510514

511515
let _initialize_response = client_connection
512516
.receiver
513-
.recv_timeout(Duration::from_secs(2))
517+
.recv_timeout(Duration::from_secs(5))
514518
.unwrap();
515519

516520
(client_connection, recv)

crates/nu-lsp/src/notification.rs

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
11
use crate::LanguageServer;
22
use lsp_types::{
33
DidChangeTextDocumentParams, DidChangeWorkspaceFoldersParams, DidCloseTextDocumentParams,
4-
DidOpenTextDocumentParams, ProgressParams, ProgressParamsValue, ProgressToken, Uri,
5-
WorkDoneProgress, WorkDoneProgressBegin, WorkDoneProgressEnd, WorkDoneProgressReport,
4+
DidOpenTextDocumentParams, LogMessageParams, MessageType, ProgressParams, ProgressParamsValue,
5+
ProgressToken, Uri, WorkDoneProgress, WorkDoneProgressBegin, WorkDoneProgressEnd,
6+
WorkDoneProgressReport,
67
notification::{
78
DidChangeTextDocument, DidChangeWorkspaceFolders, DidCloseTextDocument,
89
DidOpenTextDocument, Notification, Progress,
@@ -71,6 +72,18 @@ impl LanguageServer {
7172
.into_diagnostic()
7273
}
7374

75+
pub(crate) fn send_log_message(&self, typ: MessageType, message: String) -> Result<()> {
76+
let log_params = LogMessageParams { typ, message };
77+
let notification = lsp_server::Notification::new(
78+
lsp_types::notification::LogMessage::METHOD.to_string(),
79+
log_params,
80+
);
81+
self.connection
82+
.sender
83+
.send(lsp_server::Message::Notification(notification))
84+
.into_diagnostic()
85+
}
86+
7487
pub(crate) fn send_progress_begin(&self, token: ProgressToken, title: String) -> Result<()> {
7588
self.send_progress_notification(
7689
token,

crates/nu-lsp/src/workspace.rs

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -544,6 +544,7 @@ mod tests {
544544
use crate::tests::{initialize_language_server, open, open_unchecked, send_hover_request};
545545
use assert_json_diff::assert_json_eq;
546546
use lsp_server::{Connection, Message};
547+
use lsp_types::notification::{LogMessage, Notification, Progress};
547548
use lsp_types::{
548549
DocumentHighlightParams, InitializeParams, PartialResultParams, Position, ReferenceContext,
549550
ReferenceParams, RenameParams, TextDocumentIdentifier, TextDocumentPositionParams, Uri,
@@ -832,7 +833,7 @@ mod tests {
832833
let mut has_response = false;
833834
for message in messages {
834835
match message {
835-
Message::Notification(n) => assert_eq!(n.method, "$/progress"),
836+
Message::Notification(n) => assert_eq!(n.method, Progress::METHOD),
836837
Message::Response(r) => {
837838
has_response = true;
838839
let result = r.result.unwrap();
@@ -921,7 +922,7 @@ mod tests {
921922
let mut has_response = false;
922923
for message in messages {
923924
match message {
924-
Message::Notification(n) => assert_eq!(n.method, "$/progress"),
925+
Message::Notification(n) => assert_eq!(n.method, Progress::METHOD),
925926
Message::Response(r) => {
926927
has_response = true;
927928
assert_json_eq!(r.result, expected_prepare)
@@ -979,7 +980,7 @@ mod tests {
979980

980981
open_unchecked(&client_connection, script.clone());
981982

982-
let message_num = 3;
983+
let message_num = 4;
983984
let messages = send_rename_prepare_request(
984985
&client_connection,
985986
script.clone(),
@@ -1000,7 +1001,13 @@ mod tests {
10001001
let mut has_response = false;
10011002
for message in messages {
10021003
match message {
1003-
Message::Notification(n) => assert_eq!(n.method, "$/progress"),
1004+
Message::Notification(n) => {
1005+
if n.method == LogMessage::METHOD {
1006+
assert_json_eq!(n.params["message"], "Workspace-wide search took too long!")
1007+
} else {
1008+
assert_eq!(n.method, Progress::METHOD)
1009+
};
1010+
}
10041011
// the response of the preempting hover request
10051012
Message::Response(r) => {
10061013
has_response = true;

tests/fixtures/lsp/goto/use_module.nu

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,3 +2,7 @@ use module.nu "module name"
22
overlay use module.nu as new_name
33
overlay hide --keep-env [PWD] new_name
44
hide "module name"
5+
6+
# stdlib files are loaded virtually
7+
# goto_def on them should just fail without panic
8+
use std/log log-level

0 commit comments

Comments
 (0)