From 66500199350fd9e7a32e6fc10855654ffac01c49 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Mon, 4 Mar 2024 11:10:39 -0500 Subject: [PATCH] fix(lsp): ignore code errors when type passes for non-`@deno-types` reolution (#22682) --- cli/lsp/diagnostics.rs | 30 ++++++++++------- tests/integration/lsp_tests.rs | 33 +++++++++++++++++++ .../@denotest/monaco-editor/1.0.0/main.js | 4 +++ .../monaco-editor/1.0.0/main.types.d.ts | 1 + .../monaco-editor/1.0.0/package.json | 6 ++++ 5 files changed, 63 insertions(+), 11 deletions(-) create mode 100644 tests/testdata/npm/registry/@denotest/monaco-editor/1.0.0/main.js create mode 100644 tests/testdata/npm/registry/@denotest/monaco-editor/1.0.0/main.types.d.ts create mode 100644 tests/testdata/npm/registry/@denotest/monaco-editor/1.0.0/package.json diff --git a/cli/lsp/diagnostics.rs b/cli/lsp/diagnostics.rs index 25cfd94e2a..6deafde4c0 100644 --- a/cli/lsp/diagnostics.rs +++ b/cli/lsp/diagnostics.rs @@ -1456,12 +1456,28 @@ fn diagnose_dependency( .iter() .map(|i| documents::to_lsp_range(&i.range)) .collect(); + // TODO(nayeemrmn): This is a crude way of detecting `@deno-types` which has + // a different specifier and therefore needs a separate call to + // `diagnose_resolution()`. It would be much cleaner if that were modelled as + // a separate dependency: https://github.com/denoland/deno_graph/issues/247. + let is_types_deno_types = !dependency.maybe_type.is_none() + && !dependency + .imports + .iter() + .any(|i| dependency.maybe_type.includes(&i.range.start).is_some()); diagnostics.extend( diagnose_resolution( snapshot, dependency_key, - if dependency.maybe_code.is_none() { + if dependency.maybe_code.is_none() + // If not @deno-types, diagnose the types if the code errored because + // it's likely resolving into the node_modules folder, which might be + // erroring correctly due to resolution only being for bundlers. Let this + // fail at runtime if necesarry, but don't bother erroring in the editor + || !is_types_deno_types && matches!(dependency.maybe_type, Resolution::Ok(_)) + && matches!(dependency.maybe_code, Resolution::Err(_)) + { &dependency.maybe_type } else { &dependency.maybe_code @@ -1476,16 +1492,8 @@ fn diagnose_dependency( .map(|range| diag.to_lsp_diagnostic(range)) }), ); - // TODO(nayeemrmn): This is a crude way of detecting `@deno-types` which has - // a different specifier and therefore needs a separate call to - // `diagnose_resolution()`. It would be much cleaner if that were modelled as - // a separate dependency: https://github.com/denoland/deno_graph/issues/247. - if !dependency.maybe_type.is_none() - && !dependency - .imports - .iter() - .any(|i| dependency.maybe_type.includes(&i.range.start).is_some()) - { + + if is_types_deno_types { let range = match &dependency.maybe_type { Resolution::Ok(resolved) => documents::to_lsp_range(&resolved.range), Resolution::Err(error) => documents::to_lsp_range(error.range()), diff --git a/tests/integration/lsp_tests.rs b/tests/integration/lsp_tests.rs index 4ea4336a51..0c4c1544f2 100644 --- a/tests/integration/lsp_tests.rs +++ b/tests/integration/lsp_tests.rs @@ -8659,6 +8659,39 @@ fn lsp_ts_diagnostics_refresh_on_lsp_version_reset() { client.shutdown(); } +#[test] +fn lsp_diagnostics_none_for_resolving_types() { + let context = TestContextBuilder::for_npm().use_temp_cwd().build(); + context + .temp_dir() + .write("deno.json", r#"{ "unstable": ["byonm"] }"#); + context.temp_dir().write( + "package.json", + r#"{ "dependencies": { "@denotest/monaco-editor": "*" } }"#, + ); + context.run_npm("install"); + + let mut client = context.new_lsp_command().build(); + client.initialize_default(); + // The types for this package will succeed, but the code will fail + // because the package is only made for bundling and not meant to + // run in Deno or Node. + let diagnostics = client.did_open(json!({ + "textDocument": { + "uri": context.temp_dir().path().join("file.ts").uri_file(), + "languageId": "typescript", + "version": 1, + "text": concat!( + "import * as a from \"@denotest/monaco-editor\";\n", + "console.log(new a.Editor())\n", + ) + }, + })); + let diagnostics = diagnostics.all(); + assert!(diagnostics.is_empty(), "{:?}", diagnostics); + client.shutdown(); +} + #[test] fn lsp_jupyter_diagnostics() { let context = TestContextBuilder::new().use_temp_cwd().build(); diff --git a/tests/testdata/npm/registry/@denotest/monaco-editor/1.0.0/main.js b/tests/testdata/npm/registry/@denotest/monaco-editor/1.0.0/main.js new file mode 100644 index 0000000000..403806c6ba --- /dev/null +++ b/tests/testdata/npm/registry/@denotest/monaco-editor/1.0.0/main.js @@ -0,0 +1,4 @@ +// The monaco-editor package uses an entry in the package.json +// where it has no "type": "module" and then only specifies a +// "module": "./main.js"-like entry that points at an ESM file. +export class Editor {} \ No newline at end of file diff --git a/tests/testdata/npm/registry/@denotest/monaco-editor/1.0.0/main.types.d.ts b/tests/testdata/npm/registry/@denotest/monaco-editor/1.0.0/main.types.d.ts new file mode 100644 index 0000000000..d978fa1597 --- /dev/null +++ b/tests/testdata/npm/registry/@denotest/monaco-editor/1.0.0/main.types.d.ts @@ -0,0 +1 @@ +export class Editor {} diff --git a/tests/testdata/npm/registry/@denotest/monaco-editor/1.0.0/package.json b/tests/testdata/npm/registry/@denotest/monaco-editor/1.0.0/package.json new file mode 100644 index 0000000000..eb0428b490 --- /dev/null +++ b/tests/testdata/npm/registry/@denotest/monaco-editor/1.0.0/package.json @@ -0,0 +1,6 @@ +{ + "name": "@denotest/monaco-editor", + "version": "1.0.0", + "module": "./main.js", + "types": "./main.types.d.ts" +}