From 672216d65a7e737256133615b0bf56092b57b87f Mon Sep 17 00:00:00 2001 From: Nathan Whitaker <17734409+nathanwhit@users.noreply.github.com> Date: Mon, 6 May 2024 16:54:52 -0700 Subject: [PATCH] fix(lsp): Pass diagnostic codes to TSC as numbers (#23720) Fixes the `Debug Failure` errors described in https://github.com/denoland/deno/issues/23643#issuecomment-2094552765 . The issue here was that we were passing diagnostic codes as strings but TSC expects the codes to be numbers. This resulted in some quick fixes not working (as illustrated by the test added here which fails before this PR). The first commit is the actual fix. The rest are just test related. --- cli/lsp/language_server.rs | 10 ++- cli/lsp/tsc.rs | 4 +- tests/integration/jupyter_tests.rs | 36 ++------ tests/integration/lsp_tests.rs | 129 ++++++++++++++++++++++++++++ tests/util/server/src/assertions.rs | 41 +++++++++ 5 files changed, 186 insertions(+), 34 deletions(-) diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index 367be2c3b3..5879a74916 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -1550,8 +1550,14 @@ impl Inner { match diagnostic.source.as_deref() { Some("deno-ts") => { let code = match diagnostic.code.as_ref().unwrap() { - NumberOrString::String(code) => code.to_string(), - NumberOrString::Number(code) => code.to_string(), + NumberOrString::String(code) => match code.parse() { + Ok(c) => c, + Err(e) => { + lsp_warn!("Invalid diagnostic code {code}: {e}"); + continue; + } + }, + NumberOrString::Number(code) => *code, }; let codes = vec![code]; let actions = self diff --git a/cli/lsp/tsc.rs b/cli/lsp/tsc.rs index 0f7ec2b6cc..61308092ec 100644 --- a/cli/lsp/tsc.rs +++ b/cli/lsp/tsc.rs @@ -511,7 +511,7 @@ impl TsServer { snapshot: Arc, specifier: ModuleSpecifier, range: Range, - codes: Vec, + codes: Vec, format_code_settings: FormatCodeSettings, preferences: UserPreferences, ) -> Vec { @@ -4817,7 +4817,7 @@ pub enum TscRequest { String, u32, u32, - Vec, + Vec, FormatCodeSettings, UserPreferences, )>, diff --git a/tests/integration/jupyter_tests.rs b/tests/integration/jupyter_tests.rs index 75b1da0854..3c4efbdacb 100644 --- a/tests/integration/jupyter_tests.rs +++ b/tests/integration/jupyter_tests.rs @@ -5,6 +5,7 @@ use std::sync::Arc; use std::time::Duration; use bytes::Bytes; +use test_util::assertions::assert_json_subset; use test_util::DenoChild; use test_util::TestContext; use test_util::TestContextBuilder; @@ -488,31 +489,6 @@ async fn setup() -> (TestContext, JupyterClient, JupyterServerProcess) { (context, client, process) } -/// Asserts that the actual value is equal to the expected value, but -/// only for the keys present in the expected value. -/// In other words, `assert_eq_subset(json!({"a": 1, "b": 2}), json!({"a": 1}))` would pass. -#[track_caller] -fn assert_eq_subset(actual: Value, expected: Value) { - match (actual, expected) { - (Value::Object(actual), Value::Object(expected)) => { - for (k, v) in expected.iter() { - let Some(actual_v) = actual.get(k) else { - panic!("Key {k:?} not found in actual value ({actual:#?})"); - }; - assert_eq_subset(actual_v.clone(), v.clone()); - } - } - (Value::Array(actual), Value::Array(expected)) => { - for (i, v) in expected.iter().enumerate() { - assert_eq_subset(actual[i].clone(), v.clone()); - } - } - (actual, expected) => { - assert_eq!(actual, expected); - } - } -} - #[tokio::test] async fn jupyter_heartbeat_echoes() -> Result<()> { let (_ctx, client, _process) = setup().await; @@ -531,7 +507,7 @@ async fn jupyter_kernel_info() -> Result<()> { .await?; let msg = client.recv(Control).await?; assert_eq!(msg.header.msg_type, "kernel_info_reply"); - assert_eq_subset( + assert_json_subset( msg.content, json!({ "status": "ok", @@ -568,7 +544,7 @@ async fn jupyter_execute_request() -> Result<()> { .await?; let reply = client.recv(Shell).await?; assert_eq!(reply.header.msg_type, "execute_reply"); - assert_eq_subset( + assert_json_subset( reply.content, json!({ "status": "ok", @@ -602,7 +578,7 @@ async fn jupyter_execute_request() -> Result<()> { }) .expect("execution_state idle not found"); assert_eq!(execution_idle.parent_header, request.header.to_json()); - assert_eq_subset( + assert_json_subset( execution_idle.content.clone(), json!({ "execution_state": "idle", @@ -615,7 +591,7 @@ async fn jupyter_execute_request() -> Result<()> { .expect("stream not found"); assert_eq!(execution_result.header.msg_type, "stream"); assert_eq!(execution_result.parent_header, request.header.to_json()); - assert_eq_subset( + assert_json_subset( execution_result.content.clone(), json!({ "name": "stdout", @@ -643,7 +619,7 @@ async fn jupyter_store_history_false() -> Result<()> { let reply = client.recv(Shell).await?; assert_eq!(reply.header.msg_type, "execute_reply"); - assert_eq_subset( + assert_json_subset( reply.content, json!({ "status": "ok", diff --git a/tests/integration/lsp_tests.rs b/tests/integration/lsp_tests.rs index f5d09bc77f..9d82e0afd1 100644 --- a/tests/integration/lsp_tests.rs +++ b/tests/integration/lsp_tests.rs @@ -9,6 +9,7 @@ use deno_core::url::Url; use pretty_assertions::assert_eq; use std::fs; use test_util::assert_starts_with; +use test_util::assertions::assert_json_subset; use test_util::deno_cmd_with_deno_dir; use test_util::env_vars_for_npm_tests; use test_util::lsp::LspClient; @@ -16,6 +17,65 @@ use test_util::testdata_path; use test_util::TestContextBuilder; use tower_lsp::lsp_types as lsp; +/// Helper to get the `lsp::Range` of the `n`th occurrence of +/// `text` in `src`. `n` is zero-based, like most indexes. +fn range_of_nth( + n: usize, + text: impl AsRef, + src: impl AsRef, +) -> lsp::Range { + let text = text.as_ref(); + + let src = src.as_ref(); + + let start = src + .match_indices(text) + .nth(n) + .map(|(i, _)| i) + .unwrap_or_else(|| panic!("couldn't find text {text} in source {src}")); + let end = start + text.len(); + let mut line = 0; + let mut col = 0; + let mut byte_idx = 0; + + let pos = |line, col| lsp::Position { + line, + character: col, + }; + + let mut start_pos = None; + let mut end_pos = None; + for c in src.chars() { + if byte_idx == start { + start_pos = Some(pos(line, col)); + } + if byte_idx == end { + end_pos = Some(pos(line, col)); + break; + } + if c == '\n' { + line += 1; + col = 0; + } else { + col += c.len_utf16() as u32; + } + byte_idx += c.len_utf8(); + } + if start_pos.is_some() && end_pos.is_none() { + // range extends to end of string + end_pos = Some(pos(line, col)); + } + + let (start, end) = (start_pos.unwrap(), end_pos.unwrap()); + lsp::Range { start, end } +} + +/// Helper to get the `lsp::Range` of the first occurrence of +/// `text` in `src`. Equivalent to `range_of_nth(0, text, src)`. +fn range_of(text: impl AsRef, src: impl AsRef) -> lsp::Range { + range_of_nth(0, text, src) +} + #[test] fn lsp_startup_shutdown() { let context = TestContextBuilder::new().use_temp_cwd().build(); @@ -12548,3 +12608,72 @@ fn lsp_cjs_internal_types_default_export() { // previously, diagnostic about `add` not being callable assert_eq!(json!(diagnostics.all()), json!([])); } + +#[test] +fn lsp_ts_code_fix_any_param() { + let context = TestContextBuilder::new().use_temp_cwd().build(); + let temp_dir = context.temp_dir(); + + let mut client = context.new_lsp_command().build(); + client.initialize_default(); + + let src = "export function foo(param) { console.log(param); }"; + + let param_def = range_of("param", src); + + let main_url = temp_dir.path().join("main.ts").uri_file(); + let diagnostics = client.did_open(json!({ + "textDocument": { + "uri": main_url, + "languageId": "typescript", + "version": 1, + "text": src, + } + })); + // make sure the "implicit any type" diagnostic is there for "param" + assert_json_subset( + json!(diagnostics.all()), + json!([{ + "range": param_def, + "code": 7006, + "message": "Parameter 'param' implicitly has an 'any' type." + }]), + ); + + // response is array of fixes + let response = client.write_request( + "textDocument/codeAction", + json!({ + "textDocument": { + "uri": main_url, + }, + "range": lsp::Range { + start: param_def.end, + ..param_def + }, + "context": { + "diagnostics": diagnostics.all(), + } + }), + ); + let fixes = response.as_array().unwrap(); + + // we're looking for the quick fix that pertains to our diagnostic, + // specifically the "Infer parameter types from usage" fix + for fix in fixes { + let Some(diags) = fix.get("diagnostics") else { + continue; + }; + let Some(fix_title) = fix.get("title").and_then(|s| s.as_str()) else { + continue; + }; + if diags == &json!(diagnostics.all()) + && fix_title == "Infer parameter types from usage" + { + // found it! + return; + } + } + + panic!("failed to find 'Infer parameter types from usage' fix in fixes: {fixes:#?}"); +} diff --git a/tests/util/server/src/assertions.rs b/tests/util/server/src/assertions.rs index f964e56e99..c8b8845f4c 100644 --- a/tests/util/server/src/assertions.rs +++ b/tests/util/server/src/assertions.rs @@ -108,3 +108,44 @@ pub fn assert_wildcard_match_with_logger( } } } + +/// Asserts that the actual `serde_json::Value` is equal to the expected `serde_json::Value`, but +/// only for the keys present in the expected value. +/// +/// # Example +/// +/// ``` +/// # use serde_json::json; +/// # use test_server::assertions::assert_json_subset; +/// assert_json_subset(json!({"a": 1, "b": 2}), json!({"a": 1})); +/// +/// // Arrays are compared element by element +/// assert_json_subset(json!([{ "a": 1, "b": 2 }, {}]), json!([{"a": 1}, {}])); +/// ``` +#[track_caller] +pub fn assert_json_subset( + actual: serde_json::Value, + expected: serde_json::Value, +) { + match (actual, expected) { + ( + serde_json::Value::Object(actual), + serde_json::Value::Object(expected), + ) => { + for (k, v) in expected.iter() { + let Some(actual_v) = actual.get(k) else { + panic!("Key {k:?} not found in actual value ({actual:#?})"); + }; + assert_json_subset(actual_v.clone(), v.clone()); + } + } + (serde_json::Value::Array(actual), serde_json::Value::Array(expected)) => { + for (i, v) in expected.iter().enumerate() { + assert_json_subset(actual[i].clone(), v.clone()); + } + } + (actual, expected) => { + assert_eq!(actual, expected); + } + } +}