From 70c822bfe2180a2c98f52f02ded6aa1f19bc5b89 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker <17734409+nathanwhit@users.noreply.github.com> Date: Fri, 10 Jan 2025 19:26:01 -0800 Subject: [PATCH] fix(lsp/check): don't resolve unknown media types to a `.js` extension (#27631) Fixes https://github.com/denoland/deno/issues/25762. Note that some of the things in that issue are not resolved (vite/client types not working properly which has other root causes), but the wildcard module augmentation specifically is fixed by this. We were telling TSC that files with unknown media types had an extension of `.js`, so the ambient module declarations weren't applying. Instead, just don't resolve them, so the ambient declaration applies. --- cli/lsp/tsc.rs | 14 ++++++++++---- cli/tsc/99_main_compiler.js | 8 ++++---- cli/tsc/mod.rs | 27 ++++++++++++++++---------- tests/integration/lsp_tests.rs | 35 ++++++++++++++++++++++++++++++++++ 4 files changed, 66 insertions(+), 18 deletions(-) diff --git a/cli/lsp/tsc.rs b/cli/lsp/tsc.rs index cd1a724f5e..8d9a5a46a1 100644 --- a/cli/lsp/tsc.rs +++ b/cli/lsp/tsc.rs @@ -4509,11 +4509,12 @@ fn op_release( #[op2] #[serde] +#[allow(clippy::type_complexity)] fn op_resolve( state: &mut OpState, #[string] base: String, #[serde] specifiers: Vec<(bool, String)>, -) -> Result>, deno_core::url::ParseError> { +) -> Result)>>, deno_core::url::ParseError> { op_resolve_inner(state, ResolveArgs { base, specifiers }) } @@ -4595,10 +4596,11 @@ async fn op_poll_requests( } #[inline] +#[allow(clippy::type_complexity)] fn op_resolve_inner( state: &mut OpState, args: ResolveArgs, -) -> Result>, deno_core::url::ParseError> { +) -> Result)>>, deno_core::url::ParseError> { let state = state.borrow_mut::(); let mark = state.performance.mark_with_args("tsc.op.op_resolve", &args); let referrer = state.specifier_map.normalize(&args.base)?; @@ -4611,7 +4613,11 @@ fn op_resolve_inner( o.map(|(s, mt)| { ( state.specifier_map.denormalize(&s), - mt.as_ts_extension().to_string(), + if matches!(mt, MediaType::Unknown) { + None + } else { + Some(mt.as_ts_extension().to_string()) + }, ) }) }) @@ -6461,7 +6467,7 @@ mod tests { resolved, vec![Some(( temp_dir.url().join("b.ts").unwrap().to_string(), - MediaType::TypeScript.as_ts_extension().to_string() + Some(MediaType::TypeScript.as_ts_extension().to_string()) ))] ); } diff --git a/cli/tsc/99_main_compiler.js b/cli/tsc/99_main_compiler.js index 25813c3f9d..b3279f54ac 100644 --- a/cli/tsc/99_main_compiler.js +++ b/cli/tsc/99_main_compiler.js @@ -723,7 +723,7 @@ delete Object.prototype.__proto__; } : arg; if (fileReference.fileName.startsWith("npm:")) { - /** @type {[string, ts.Extension] | undefined} */ + /** @type {[string, ts.Extension | null] | undefined} */ const resolved = ops.op_resolve( containingFilePath, [ @@ -735,7 +735,7 @@ delete Object.prototype.__proto__; ], ], )?.[0]; - if (resolved) { + if (resolved && resolved[1]) { return { resolvedTypeReferenceDirective: { primary: true, @@ -785,7 +785,7 @@ delete Object.prototype.__proto__; debug(` base: ${base}`); debug(` specifiers: ${specifiers.map((s) => s[1]).join(", ")}`); } - /** @type {Array<[string, ts.Extension] | undefined>} */ + /** @type {Array<[string, ts.Extension | null] | undefined>} */ const resolved = ops.op_resolve( base, specifiers, @@ -793,7 +793,7 @@ delete Object.prototype.__proto__; if (resolved) { /** @type {Array} */ const result = resolved.map((item) => { - if (item) { + if (item && item[1]) { const [resolvedFileName, extension] = item; return { resolvedModule: { diff --git a/cli/tsc/mod.rs b/cli/tsc/mod.rs index 1473b8a8d9..f645a5f6b8 100644 --- a/cli/tsc/mod.rs +++ b/cli/tsc/mod.rs @@ -746,7 +746,7 @@ fn op_resolve( state: &mut OpState, #[string] base: String, #[serde] specifiers: Vec<(bool, String)>, -) -> Result, ResolveError> { +) -> Result)>, ResolveError> { op_resolve_inner(state, ResolveArgs { base, specifiers }) } @@ -754,9 +754,9 @@ fn op_resolve( fn op_resolve_inner( state: &mut OpState, args: ResolveArgs, -) -> Result, ResolveError> { +) -> Result)>, ResolveError> { let state = state.borrow_mut::(); - let mut resolved: Vec<(String, &'static str)> = + let mut resolved: Vec<(String, Option<&'static str>)> = Vec::with_capacity(args.specifiers.len()); let referrer = if let Some(remapped_specifier) = state.maybe_remapped_specifier(&args.base) @@ -770,14 +770,14 @@ fn op_resolve_inner( if specifier.starts_with("node:") { resolved.push(( MISSING_DEPENDENCY_SPECIFIER.to_string(), - MediaType::Dts.as_ts_extension(), + Some(MediaType::Dts.as_ts_extension()), )); continue; } if specifier.starts_with("asset:///") { let ext = MediaType::from_str(&specifier).as_ts_extension(); - resolved.push((specifier, ext)); + resolved.push((specifier, Some(ext))); continue; } @@ -857,14 +857,15 @@ fn op_resolve_inner( ( specifier_str, match media_type { - MediaType::Css => ".js", // surface these as .js for typescript - media_type => media_type.as_ts_extension(), + MediaType::Css => Some(".js"), // surface these as .js for typescript + MediaType::Unknown => None, + media_type => Some(media_type.as_ts_extension()), }, ) } None => ( MISSING_DEPENDENCY_SPECIFIER.to_string(), - MediaType::Dts.as_ts_extension(), + Some(MediaType::Dts.as_ts_extension()), ), }; log::debug!("Resolved {} from {} to {:?}", specifier, referrer, result); @@ -1441,7 +1442,10 @@ mod tests { }, ) .expect("should have invoked op"); - assert_eq!(actual, vec![("https://deno.land/x/b.ts".into(), ".ts")]); + assert_eq!( + actual, + vec![("https://deno.land/x/b.ts".into(), Some(".ts"))] + ); } #[tokio::test] @@ -1460,7 +1464,10 @@ mod tests { }, ) .expect("should have not errored"); - assert_eq!(actual, vec![(MISSING_DEPENDENCY_SPECIFIER.into(), ".d.ts")]); + assert_eq!( + actual, + vec![(MISSING_DEPENDENCY_SPECIFIER.into(), Some(".d.ts"))] + ); } #[tokio::test] diff --git a/tests/integration/lsp_tests.rs b/tests/integration/lsp_tests.rs index 247851da9c..a25710b2b1 100644 --- a/tests/integration/lsp_tests.rs +++ b/tests/integration/lsp_tests.rs @@ -17221,3 +17221,38 @@ fn lsp_wasm_module() { ); client.shutdown(); } + +#[test] +fn wildcard_augment() { + let context = TestContextBuilder::new().use_temp_cwd().build(); + let mut client = context.new_lsp_command().build(); + let temp_dir = context.temp_dir().path(); + let source = source_file( + temp_dir.join("index.ts"), + r#" + import styles from "./hello_world.scss"; + + function bar(v: string): string { + return v; + } + + bar(styles); + "#, + ); + temp_dir.join("index.d.ts").write( + r#" + declare module '*.scss' { + const content: string; + export default content; + } + "#, + ); + temp_dir + .join("hello_world.scss") + .write("body { color: red; }"); + + client.initialize_default(); + + let diagnostics = client.did_open_file(&source); + assert_eq!(diagnostics.all().len(), 0); +}