From c4f82cab31d1ec09b2bce1f0155f92c7d7bd50e0 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Thu, 30 Mar 2023 12:15:21 -0400 Subject: [PATCH] fix(lsp): `textDocument/references` should respect `includeDeclaration` (#18496) --- cli/lsp/code_lens.rs | 104 ++++++------- cli/lsp/language_server.rs | 31 ++-- cli/lsp/tsc.rs | 62 +++++++- cli/tests/integration/lsp_tests.rs | 226 +++++++++++++++++++++++++--- cli/tests/integration/repl_tests.rs | 4 +- cli/tsc/99_main_compiler.js | 4 +- cli/tsc/compiler.d.ts | 6 +- 7 files changed, 339 insertions(+), 98 deletions(-) diff --git a/cli/lsp/code_lens.rs b/cli/lsp/code_lens.rs index 1253fe5aa1..650e5e2416 100644 --- a/cli/lsp/code_lens.rs +++ b/cli/lsp/code_lens.rs @@ -297,70 +297,72 @@ async fn resolve_references_code_lens( data: CodeLensData, language_server: &language_server::Inner, ) -> Result { - let asset_or_document = - language_server.get_asset_or_document(&data.specifier)?; - let line_index = asset_or_document.line_index(); - let req = tsc::RequestMethod::GetReferences(( - data.specifier.clone(), - line_index.offset_tsc(code_lens.range.start)?, - )); - let snapshot = language_server.snapshot(); - let maybe_references: Option> = - language_server.ts_server.request(snapshot, req).await?; - if let Some(references) = maybe_references { + fn get_locations( + maybe_referenced_symbols: Option>, + language_server: &language_server::Inner, + ) -> Result, AnyError> { + let symbols = match maybe_referenced_symbols { + Some(symbols) => symbols, + None => return Ok(Vec::new()), + }; let mut locations = Vec::new(); - for reference in references { + for reference in symbols.iter().flat_map(|s| &s.references) { if reference.is_definition { continue; } let reference_specifier = - resolve_url(&reference.document_span.file_name)?; + resolve_url(&reference.entry.document_span.file_name)?; let asset_or_doc = language_server.get_asset_or_document(&reference_specifier)?; locations.push( reference + .entry .to_location(asset_or_doc.line_index(), &language_server.url_map), ); } - let command = if !locations.is_empty() { - let title = if locations.len() > 1 { - format!("{} references", locations.len()) - } else { - "1 reference".to_string() - }; - lsp::Command { - title, - command: "deno.showReferences".to_string(), - arguments: Some(vec![ - json!(data.specifier), - json!(code_lens.range.start), - json!(locations), - ]), - } - } else { - lsp::Command { - title: "0 references".to_string(), - command: "".to_string(), - arguments: None, - } - }; - Ok(lsp::CodeLens { - range: code_lens.range, - command: Some(command), - data: None, - }) - } else { - let command = lsp::Command { - title: "0 references".to_string(), - command: "".to_string(), - arguments: None, - }; - Ok(lsp::CodeLens { - range: code_lens.range, - command: Some(command), - data: None, - }) + Ok(locations) } + + let asset_or_document = + language_server.get_asset_or_document(&data.specifier)?; + let line_index = asset_or_document.line_index(); + let snapshot = language_server.snapshot(); + let maybe_referenced_symbols = language_server + .ts_server + .find_references( + snapshot, + &data.specifier, + line_index.offset_tsc(code_lens.range.start)?, + ) + .await?; + let locations = get_locations(maybe_referenced_symbols, language_server)?; + let title = if locations.len() == 1 { + "1 reference".to_string() + } else { + format!("{} references", locations.len()) + }; + let command = if locations.is_empty() { + lsp::Command { + title, + command: String::new(), + arguments: None, + } + } else { + lsp::Command { + title, + command: "deno.showReferences".to_string(), + arguments: Some(vec![ + json!(data.specifier), + json!(code_lens.range.start), + json!(locations), + ]), + } + }; + Ok(lsp::CodeLens { + range: code_lens.range, + command: Some(command), + data: None, + }) } pub async fn resolve_code_lens( diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index 2d0bbd1400..164c9734f8 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -1953,27 +1953,23 @@ impl Inner { let mark = self.performance.mark("references", Some(¶ms)); let asset_or_doc = self.get_asset_or_document(&specifier)?; let line_index = asset_or_doc.line_index(); - let req = tsc::RequestMethod::GetReferences(( - specifier.clone(), - line_index.offset_tsc(params.text_document_position.position)?, - )); - let maybe_references: Option> = self + let maybe_referenced_symbols = self .ts_server - .request(self.snapshot(), req) - .await - .map_err(|err| { - error!("Unable to get references from TypeScript: {}", err); - LspError::internal_error() - })?; + .find_references( + self.snapshot(), + &specifier, + line_index.offset_tsc(params.text_document_position.position)?, + ) + .await?; - if let Some(references) = maybe_references { + if let Some(symbols) = maybe_referenced_symbols { let mut results = Vec::new(); - for reference in references { + for reference in symbols.iter().flat_map(|s| &s.references) { if !params.context.include_declaration && reference.is_definition { continue; } let reference_specifier = - resolve_url(&reference.document_span.file_name).unwrap(); + resolve_url(&reference.entry.document_span.file_name).unwrap(); let reference_line_index = if reference_specifier == specifier { line_index.clone() } else { @@ -1981,8 +1977,11 @@ impl Inner { self.get_asset_or_document(&reference_specifier)?; asset_or_doc.line_index() }; - results - .push(reference.to_location(reference_line_index, &self.url_map)); + results.push( + reference + .entry + .to_location(reference_line_index, &self.url_map), + ); } self.performance.measure(mark); diff --git a/cli/lsp/tsc.rs b/cli/lsp/tsc.rs index 3164369880..e846cc4963 100644 --- a/cli/lsp/tsc.rs +++ b/cli/lsp/tsc.rs @@ -149,7 +149,28 @@ impl TsServer { if self.0.send((req, snapshot, tx, token)).is_err() { return Err(anyhow!("failed to send request to tsc thread")); } - rx.await?.map(|v| serde_json::from_value::(v).unwrap()) + let value = rx.await??; + Ok(serde_json::from_value::(value)?) + } + + // todo(dsherret): refactor the rest of the request methods to have + // methods to call on this struct, then make `RequestMethod` and + // friends internal + + pub async fn find_references( + &self, + snapshot: Arc, + specifier: &ModuleSpecifier, + position: u32, + ) -> Result>, LspError> { + let req = RequestMethod::FindReferences { + specifier: specifier.clone(), + position, + }; + self.request(snapshot, req).await.map_err(|err| { + log::error!("Unable to get references from TypeScript: {}", err); + LspError::internal_error() + }) } } @@ -1688,10 +1709,31 @@ pub struct CombinedCodeActions { #[derive(Debug, Deserialize)] #[serde(rename_all = "camelCase")] -pub struct ReferenceEntry { - // is_write_access: bool, +pub struct ReferencedSymbol { + pub definition: ReferencedSymbolDefinitionInfo, + pub references: Vec, +} + +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct ReferencedSymbolDefinitionInfo { + #[serde(flatten)] + pub definition_info: DefinitionInfo, +} + +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct ReferencedSymbolEntry { #[serde(default)] pub is_definition: bool, + #[serde(flatten)] + pub entry: ReferenceEntry, +} + +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct ReferenceEntry { + // is_write_access: bool, // is_in_string: Option, #[serde(flatten)] pub document_span: DocumentSpan, @@ -3178,8 +3220,11 @@ pub enum RequestMethod { GetOutliningSpans(ModuleSpecifier), /// Return quick info at position (hover information). GetQuickInfo((ModuleSpecifier, u32)), - /// Get document references for a specific position. - GetReferences((ModuleSpecifier, u32)), + /// Finds the document references for a specific position. + FindReferences { + specifier: ModuleSpecifier, + position: u32, + }, /// Get signature help items for a specific position. GetSignatureHelpItems((ModuleSpecifier, u32, SignatureHelpItemsOptions)), /// Get a selection range for a specific position. @@ -3349,9 +3394,12 @@ impl RequestMethod { "specifier": state.denormalize_specifier(specifier), "position": position, }), - RequestMethod::GetReferences((specifier, position)) => json!({ + RequestMethod::FindReferences { + specifier, + position, + } => json!({ "id": id, - "method": "getReferences", + "method": "findReferences", "specifier": state.denormalize_specifier(specifier), "position": position, }), diff --git a/cli/tests/integration/lsp_tests.rs b/cli/tests/integration/lsp_tests.rs index 11800c2f76..35c115d564 100644 --- a/cli/tests/integration/lsp_tests.rs +++ b/cli/tests/integration/lsp_tests.rs @@ -2367,16 +2367,32 @@ fn lsp_semantic_tokens() { fn lsp_code_lens() { let mut client = LspClientBuilder::new().build(); client.initialize_default(); - client.did_open( - json!({ - "textDocument": { - "uri": "file:///a/file.ts", - "languageId": "typescript", - "version": 1, - "text": "class A {\n a = \"a\";\n\n b() {\n console.log(this.a);\n }\n\n c() {\n this.a = \"c\";\n }\n}\n\nconst a = new A();\na.b();\n" - } - }), - ); + client.did_open(json!({ + "textDocument": { + "uri": "file:///a/file.ts", + "languageId": "typescript", + "version": 1, + "text": concat!( + "class A {\n", + " a = \"a\";\n", + "\n", + " b() {\n", + " console.log(this.a);\n", + " }\n", + "\n", + " c() {\n", + " this.a = \"c\";\n", + " }\n", + "}\n", + "\n", + "const a = new A();\n", + "a.b();\n", + "const b = 2;\n", + "const c = 3;\n", + "c; c;", + ), + } + })); let res = client.write_request( "textDocument/codeLens", json!({ @@ -2428,18 +2444,12 @@ fn lsp_code_lens() { "end": { "line": 0, "character": 7 } }, "command": { - "title": "2 references", + "title": "1 reference", "command": "deno.showReferences", "arguments": [ "file:///a/file.ts", { "line": 0, "character": 6 }, [{ - "uri": "file:///a/file.ts", - "range": { - "start": { "line": 0, "character": 6 }, - "end": { "line": 0, "character": 7 } - } - }, { "uri": "file:///a/file.ts", "range": { "start": { "line": 12, "character": 14 }, @@ -2450,6 +2460,80 @@ fn lsp_code_lens() { } }) ); + + // 0 references + let res = client.write_request( + "codeLens/resolve", + json!({ + "range": { + "start": { "line": 14, "character": 6 }, + "end": { "line": 14, "character": 7 } + }, + "data": { + "specifier": "file:///a/file.ts", + "source": "references" + } + }), + ); + assert_eq!( + res, + json!({ + "range": { + "start": { "line": 14, "character": 6 }, + "end": { "line": 14, "character": 7 } + }, + "command": { + "title": "0 references", + "command": "", + } + }) + ); + + // 2 references + let res = client.write_request( + "codeLens/resolve", + json!({ + "range": { + "start": { "line": 15, "character": 6 }, + "end": { "line": 15, "character": 7 } + }, + "data": { + "specifier": "file:///a/file.ts", + "source": "references" + } + }), + ); + assert_eq!( + res, + json!({ + "range": { + "start": { "line": 15, "character": 6 }, + "end": { "line": 15, "character": 7 } + }, + "command": { + "title": "2 references", + "command": "deno.showReferences", + "arguments": [ + "file:///a/file.ts", + { "line": 15, "character": 6 }, + [{ + "uri": "file:///a/file.ts", + "range": { + "start": { "line": 16, "character": 0 }, + "end": { "line": 16, "character": 1 } + } + },{ + "uri": "file:///a/file.ts", + "range": { + "start": { "line": 16, "character": 3 }, + "end": { "line": 16, "character": 4 } + } + }] + ] + } + }) + ); + client.shutdown(); } @@ -3091,6 +3175,114 @@ fn lsp_nav_tree_updates() { client.shutdown(); } +#[test] +fn lsp_find_references() { + let mut client = LspClientBuilder::new().build(); + client.initialize_default(); + client.did_open(json!({ + "textDocument": { + "uri": "file:///a/mod.ts", + "languageId": "typescript", + "version": 1, + "text": r#"export const a = 1;\nconst b = 2;"# + } + })); + client.did_open(json!({ + "textDocument": { + "uri": "file:///a/mod.test.ts", + "languageId": "typescript", + "version": 1, + "text": r#"import { a } from './mod.ts'; console.log(a);"# + } + })); + + // test without including the declaration + let res = client.write_request( + "textDocument/references", + json!({ + "textDocument": { + "uri": "file:///a/mod.ts", + }, + "position": { "line": 0, "character": 13 }, + "context": { + "includeDeclaration": false + } + }), + ); + + assert_eq!( + res, + json!([{ + "uri": "file:///a/mod.test.ts", + "range": { + "start": { "line": 0, "character": 9 }, + "end": { "line": 0, "character": 10 } + } + }, { + "uri": "file:///a/mod.test.ts", + "range": { + "start": { "line": 0, "character": 42 }, + "end": { "line": 0, "character": 43 } + } + }]) + ); + + // test with including the declaration + let res = client.write_request( + "textDocument/references", + json!({ + "textDocument": { + "uri": "file:///a/mod.ts", + }, + "position": { "line": 0, "character": 13 }, + "context": { + "includeDeclaration": true + } + }), + ); + + assert_eq!( + res, + json!([{ + "uri": "file:///a/mod.ts", + "range": { + "start": { "line": 0, "character": 13 }, + "end": { "line": 0, "character": 14 } + } + }, { + "uri": "file:///a/mod.test.ts", + "range": { + "start": { "line": 0, "character": 9 }, + "end": { "line": 0, "character": 10 } + } + }, { + "uri": "file:///a/mod.test.ts", + "range": { + "start": { "line": 0, "character": 42 }, + "end": { "line": 0, "character": 43 } + } + }]) + ); + + // test 0 references + let res = client.write_request( + "textDocument/references", + json!({ + "textDocument": { + "uri": "file:///a/mod.ts", + }, + "position": { "line": 1, "character": 6 }, + "context": { + "includeDeclaration": false + } + }), + ); + + assert_eq!(res, json!(null)); // seems it always returns null for this, which is ok + + client.shutdown(); +} + #[test] fn lsp_signature_help() { let mut client = LspClientBuilder::new().build(); diff --git a/cli/tests/integration/repl_tests.rs b/cli/tests/integration/repl_tests.rs index 27a9b716c5..73c8918f9d 100644 --- a/cli/tests/integration/repl_tests.rs +++ b/cli/tests/integration/repl_tests.rs @@ -905,7 +905,7 @@ fn package_json_uncached_no_error() { ); test_context.new_command().with_pty(|mut console| { console.write_line("console.log(123 + 456);"); - console.expect("579"); + console.expect_all(&["579", "undefined"]); assert_not_contains!( console.all_output(), "Could not set npm package requirements", @@ -914,7 +914,7 @@ fn package_json_uncached_no_error() { // should support getting the package now though console .write_line("import { getValue, setValue } from '@denotest/esm-basic';"); - console.expect("undefined"); + console.expect_all(&["undefined", "Initialize"]); console.write_line("setValue(12 + 30);"); console.expect("undefined"); console.write_line("getValue()"); diff --git a/cli/tsc/99_main_compiler.js b/cli/tsc/99_main_compiler.js index a00b946e22..b8189278c0 100644 --- a/cli/tsc/99_main_compiler.js +++ b/cli/tsc/99_main_compiler.js @@ -1121,10 +1121,10 @@ delete Object.prototype.__proto__; ), ); } - case "getReferences": { + case "findReferences": { return respond( id, - languageService.getReferencesAtPosition( + languageService.findReferences( request.specifier, request.position, ), diff --git a/cli/tsc/compiler.d.ts b/cli/tsc/compiler.d.ts index a1ee457971..b59f6dca81 100644 --- a/cli/tsc/compiler.d.ts +++ b/cli/tsc/compiler.d.ts @@ -75,7 +75,7 @@ declare global { | GetNavigationTree | GetOutliningSpans | GetQuickInfoRequest - | GetReferencesRequest + | FindReferencesRequest | GetSignatureHelpItemsRequest | GetSmartSelectionRange | GetSupportedCodeFixes @@ -212,8 +212,8 @@ declare global { position: number; } - interface GetReferencesRequest extends BaseLanguageServerRequest { - method: "getReferences"; + interface FindReferencesRequest extends BaseLanguageServerRequest { + method: "findReferences"; specifier: string; position: number; }