From 890780a9e905f506e897f632d9cfbe153e66b931 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Wed, 6 Dec 2023 19:03:18 -0500 Subject: [PATCH] feat(unstable): ability to resolve specifiers with no extension, specifiers for a directory, and TS files from JS extensions (#21464) Adds an `--unstable-sloppy-imports` flag which supports the following for `file:` specifiers: * Allows writing `./mod` in a specifier to do extension probing. - ex. `import { Example } from "./example"` instead of `import { Example } from "./example.ts"` * Allows writing `./routes` to do directory extension probing for files like `./routes/index.ts` * Allows writing `./mod.js` for *mod.ts* files. This functionality is **NOT RECOMMENDED** for general use with Deno: 1. It's not as optimal for perf: https://marvinh.dev/blog/speeding-up-javascript-ecosystem-part-2/ 1. It makes tooling in the ecosystem more complex in order to have to understand this. 1. The "Deno way" is to be explicit about what you're doing. It's better in the long run. 1. It doesn't work if published to the Deno registry because doing stuff like extension probing with remote specifiers would be incredibly slow. This is instead only recommended to help with migrating existing projects to Deno. For example, it's very useful for getting CJS projects written with import/export declaration working in Deno without modifying module specifiers and for supporting TS ESM projects written with `./mod.js` specifiers. This feature will output warnings to guide the user towards correcting their specifiers. Additionally, quick fixes are provided in the LSP to update these specifiers: --- Cargo.lock | 16 +- cli/Cargo.toml | 8 +- cli/args/flags.rs | 13 ++ cli/args/mod.rs | 13 +- cli/factory.rs | 6 + cli/graph_util.rs | 2 +- cli/lsp/diagnostics.rs | 51 +++- cli/lsp/documents.rs | 130 +++++++++-- cli/lsp/language_server.rs | 3 +- cli/lsp/tsc.rs | 2 +- cli/module_loader.rs | 6 +- cli/resolver.rs | 359 ++++++++++++++++++++++++++++- cli/tests/integration/lsp_tests.rs | 166 ++++++++++++- cli/tests/integration/run_tests.rs | 68 ++++++ cli/tools/repl/session.rs | 7 +- cli/tools/vendor/build.rs | 6 +- cli/tools/vendor/import_map.rs | 6 +- cli/tools/vendor/test.rs | 1 + 18 files changed, 816 insertions(+), 47 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5e0e2bb42e..1f17b5fabe 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1137,9 +1137,9 @@ dependencies = [ [[package]] name = "deno_doc" -version = "0.73.5" +version = "0.74.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "76d4d2960ce5c7af64e57ec4e7fd99023bd1ab664cedc4ec267db1d294b6002a" +checksum = "347a6288b9fa4f1ce2d56f04326bc39e1dec243545a89240efeef402d5fbde6b" dependencies = [ "anyhow", "cfg-if", @@ -1160,9 +1160,9 @@ dependencies = [ [[package]] name = "deno_emit" -version = "0.31.5" +version = "0.32.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5b98917905e9be9740722f89c3b2d3a963beaed8a05dce58e642947d0f6aa17d" +checksum = "bba6c66922c3953ba465fac215e42c637cc91119618911c0cd84cb994b25d5b7" dependencies = [ "anyhow", "base64 0.13.1", @@ -1228,9 +1228,9 @@ dependencies = [ [[package]] name = "deno_graph" -version = "0.61.5" +version = "0.62.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "332e6a1930b3266dc848edeaf1426bbbd3ddca25c5e107e70823efb1b3ce68be" +checksum = "2b0c15558cd72b534064bf7ae07e0fc99e0693915a4552f67558cc0b2b09c287" dependencies = [ "anyhow", "async-trait", @@ -2204,9 +2204,9 @@ dependencies = [ [[package]] name = "eszip" -version = "0.55.5" +version = "0.56.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "455c055f28756fc7ba0d64506e6167b91878f3a39854271d7d63b577deb8b45d" +checksum = "f50abfc246ed6d7a5e34c9658cac93f959788058c8455c528d69471ef72d8ebf" dependencies = [ "anyhow", "base64 0.21.4", diff --git a/cli/Cargo.toml b/cli/Cargo.toml index 0c9d228c3c..8fc6431de0 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -60,16 +60,16 @@ deno_ast = { workspace = true, features = ["bundler", "cjs", "codegen", "dep_gra deno_cache_dir = "=0.6.1" deno_config = "=0.6.5" deno_core = { workspace = true, features = ["include_js_files_for_snapshotting"] } -deno_doc = { version = "=0.73.5", features = ["html"] } -deno_emit = "=0.31.5" -deno_graph = "=0.61.5" +deno_doc = { version = "=0.74.0", features = ["html"] } +deno_emit = "=0.32.0" +deno_graph = "=0.62.0" deno_lint = { version = "=0.52.2", features = ["docs"] } deno_lockfile.workspace = true deno_npm = "0.15.3" deno_runtime = { workspace = true, features = ["dont_create_runtime_snapshot", "include_js_files_for_snapshotting"] } deno_semver = "0.5.1" deno_task_shell = "=0.14.0" -eszip = "=0.55.5" +eszip = "=0.56.0" napi_sym.workspace = true async-trait.workspace = true diff --git a/cli/args/flags.rs b/cli/args/flags.rs index 9bd3a62d44..1acef2058a 100644 --- a/cli/args/flags.rs +++ b/cli/args/flags.rs @@ -455,6 +455,7 @@ pub struct Flags { pub unstable: bool, pub unstable_bare_node_builtins: bool, pub unstable_byonm: bool, + pub unstable_sloppy_imports: bool, pub unstable_workspaces: bool, pub unstable_features: Vec, pub unsafely_ignore_certificate_errors: Option>, @@ -862,6 +863,7 @@ pub fn flags_from_vec(args: Vec) -> clap::error::Result { flags.unstable_bare_node_builtins = matches.get_flag("unstable-bare-node-builtins"); flags.unstable_byonm = matches.get_flag("unstable-byonm"); + flags.unstable_sloppy_imports = matches.get_flag("unstable-sloppy-imports"); flags.unstable_workspaces = matches.get_flag("unstable-workspaces"); if matches.get_flag("quiet") { @@ -981,6 +983,17 @@ fn clap_root() -> Command { .action(ArgAction::SetTrue) .global(true), ) + .arg( + Arg::new("unstable-sloppy-imports") + .long("unstable-sloppy-imports") + .help( + "Enable unstable resolving of specifiers by extension probing, .js to .ts, and directory probing.", + ) + .env("DENO_UNSTABLE_SLOPPY_IMPORTS") + .value_parser(FalseyValueParser::new()) + .action(ArgAction::SetTrue) + .global(true), + ) .arg( Arg::new("unstable-workspaces") .long("unstable-workspaces") diff --git a/cli/args/mod.rs b/cli/args/mod.rs index 3e61b50bdf..033b30d979 100644 --- a/cli/args/mod.rs +++ b/cli/args/mod.rs @@ -1342,7 +1342,7 @@ impl CliOptions { || self .maybe_config_file() .as_ref() - .map(|c| c.json.unstable.contains(&"bare-node-builtins".to_string())) + .map(|c| c.has_unstable("bare-node-builtins")) .unwrap_or(false) } @@ -1355,7 +1355,16 @@ impl CliOptions { || self .maybe_config_file() .as_ref() - .map(|c| c.json.unstable.iter().any(|c| c == "byonm")) + .map(|c| c.has_unstable("byonm")) + .unwrap_or(false) + } + + pub fn unstable_sloppy_imports(&self) -> bool { + self.flags.unstable_sloppy_imports + || self + .maybe_config_file() + .as_ref() + .map(|c| c.has_unstable("sloppy-imports")) .unwrap_or(false) } diff --git a/cli/factory.rs b/cli/factory.rs index 78f8f5b866..e4b9c1da85 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -39,6 +39,7 @@ use crate::npm::CliNpmResolverManagedPackageJsonInstallerOption; use crate::npm::CliNpmResolverManagedSnapshotOption; use crate::resolver::CliGraphResolver; use crate::resolver::CliGraphResolverOptions; +use crate::resolver::UnstableSloppyImportsResolver; use crate::standalone::DenoCompileBinaryWriter; use crate::tools::check::TypeChecker; use crate::util::file_watcher::WatcherCommunicator; @@ -381,6 +382,11 @@ impl CliFactory { Ok(Arc::new(CliGraphResolver::new(CliGraphResolverOptions { fs: self.fs().clone(), cjs_resolutions: Some(self.cjs_resolutions().clone()), + sloppy_imports_resolver: if self.options.unstable_sloppy_imports() { + Some(UnstableSloppyImportsResolver::new(self.fs().clone())) + } else { + None + }, node_resolver: Some(self.node_resolver().await?.clone()), npm_resolver: if self.options.no_npm() { None diff --git a/cli/graph_util.rs b/cli/graph_util.rs index 445f269e31..2e7766b9f0 100644 --- a/cli/graph_util.rs +++ b/cli/graph_util.rs @@ -857,7 +857,7 @@ mod test { use deno_graph::ResolutionError; use deno_graph::SpecifierError; - use crate::graph_util::get_resolution_error_bare_node_specifier; + use super::*; #[test] fn import_map_node_resolution_error() { diff --git a/cli/lsp/diagnostics.rs b/cli/lsp/diagnostics.rs index 1349029c32..4e4e9b3bb5 100644 --- a/cli/lsp/diagnostics.rs +++ b/cli/lsp/diagnostics.rs @@ -1097,7 +1097,10 @@ impl DenoDiagnostic { changes: Some(HashMap::from([( specifier.clone(), vec![lsp::TextEdit { - new_text: format!("\"{}\"", data.redirect), + new_text: format!( + "\"{}\"", + specifier_text_for_redirected(&data.redirect, specifier) + ), range: diagnostic.range, }], )])), @@ -1193,6 +1196,27 @@ impl DenoDiagnostic { } } +fn specifier_text_for_redirected( + redirect: &lsp::Url, + referrer: &lsp::Url, +) -> String { + if redirect.scheme() == "file" && referrer.scheme() == "file" { + // use a relative specifier when it's going to a file url + match referrer.make_relative(redirect) { + Some(relative) => { + if relative.starts_with('.') { + relative + } else { + format!("./{}", relative) + } + } + None => redirect.to_string(), + } + } else { + redirect.to_string() + } +} + fn diagnose_resolution( snapshot: &language_server::StateSnapshot, dependency_key: &str, @@ -1833,4 +1857,29 @@ let c: number = "a"; ]) ); } + + #[test] + fn test_specifier_text_for_redirected() { + #[track_caller] + fn run_test(specifier: &str, referrer: &str, expected: &str) { + let result = specifier_text_for_redirected( + &ModuleSpecifier::parse(specifier).unwrap(), + &ModuleSpecifier::parse(referrer).unwrap(), + ); + assert_eq!(result, expected); + } + + run_test("file:///a/a.ts", "file:///a/mod.ts", "./a.ts"); + run_test("file:///a/a.ts", "file:///a/sub_dir/mod.ts", "../a.ts"); + run_test( + "file:///a/sub_dir/a.ts", + "file:///a/mod.ts", + "./sub_dir/a.ts", + ); + run_test( + "https://deno.land/x/example/mod.ts", + "file:///a/sub_dir/a.ts", + "https://deno.land/x/example/mod.ts", + ); + } } diff --git a/cli/lsp/documents.rs b/cli/lsp/documents.rs index 95e8df917b..6687d2208d 100644 --- a/cli/lsp/documents.rs +++ b/cli/lsp/documents.rs @@ -20,6 +20,9 @@ use crate::lsp::logging::lsp_warn; use crate::npm::CliNpmResolver; use crate::resolver::CliGraphResolver; use crate::resolver::CliGraphResolverOptions; +use crate::resolver::UnstableSloppyImportsFsEntry; +use crate::resolver::UnstableSloppyImportsResolution; +use crate::resolver::UnstableSloppyImportsResolver; use crate::util::glob; use crate::util::path::specifier_to_file_path; use crate::util::text_encoding; @@ -50,6 +53,7 @@ use indexmap::IndexMap; use lsp::Url; use once_cell::sync::Lazy; use package_json::PackageJsonDepsProvider; +use std::borrow::Cow; use std::collections::HashMap; use std::collections::HashSet; use std::collections::VecDeque; @@ -688,12 +692,12 @@ fn recurse_dependents( } #[derive(Debug)] -struct SpecifierResolver { +struct RedirectResolver { cache: Arc, redirects: Mutex>, } -impl SpecifierResolver { +impl RedirectResolver { pub fn new(cache: Arc) -> Self { Self { cache, @@ -887,7 +891,9 @@ pub struct Documents { /// should be injected. has_injected_types_node_package: bool, /// Resolves a specifier to its final redirected to specifier. - specifier_resolver: Arc, + redirect_resolver: Arc, + /// If --unstable-sloppy-imports is enabled. + unstable_sloppy_imports: bool, } impl Documents { @@ -910,10 +916,12 @@ impl Documents { maybe_import_map: None, maybe_vendor_dir: None, bare_node_builtins_enabled: false, + sloppy_imports_resolver: None, })), npm_specifier_reqs: Default::default(), has_injected_types_node_package: false, - specifier_resolver: Arc::new(SpecifierResolver::new(cache)), + redirect_resolver: Arc::new(RedirectResolver::new(cache)), + unstable_sloppy_imports: false, } } @@ -1022,7 +1030,15 @@ impl Documents { ) -> bool { let maybe_specifier = self .get_resolver() - .resolve(specifier, referrer, ResolutionMode::Types) + .resolve( + specifier, + &deno_graph::Range { + specifier: referrer.clone(), + start: deno_graph::Position::zeroed(), + end: deno_graph::Position::zeroed(), + }, + ResolutionMode::Types, + ) .ok(); if let Some(import_specifier) = maybe_specifier { self.exists(&import_specifier) @@ -1031,16 +1047,48 @@ impl Documents { } } - pub fn resolve_redirected( + pub fn resolve_specifier( &self, specifier: &ModuleSpecifier, ) -> Option { - self.specifier_resolver.resolve(specifier) + if self.unstable_sloppy_imports && specifier.scheme() == "file" { + Some( + self + .resolve_unstable_sloppy_import(specifier) + .into_owned_specifier(), + ) + } else { + self.redirect_resolver.resolve(specifier) + } + } + + fn resolve_unstable_sloppy_import<'a>( + &self, + specifier: &'a ModuleSpecifier, + ) -> UnstableSloppyImportsResolution<'a> { + UnstableSloppyImportsResolver::resolve_with_stat_sync(specifier, |path| { + if let Ok(specifier) = ModuleSpecifier::from_file_path(path) { + if self.open_docs.contains_key(&specifier) + || self.cache.contains(&specifier) + { + return Some(UnstableSloppyImportsFsEntry::File); + } + } + path.metadata().ok().and_then(|m| { + if m.is_file() { + Some(UnstableSloppyImportsFsEntry::File) + } else if m.is_dir() { + Some(UnstableSloppyImportsFsEntry::Dir) + } else { + None + } + }) + }) } /// Return `true` if the specifier can be resolved to a document. pub fn exists(&self, specifier: &ModuleSpecifier) -> bool { - let specifier = self.specifier_resolver.resolve(specifier); + let specifier = self.resolve_specifier(specifier); if let Some(specifier) = specifier { if self.open_docs.contains_key(&specifier) { return true; @@ -1069,7 +1117,7 @@ impl Documents { ) -> Vec { self.calculate_dependents_if_dirty(); let mut dependents = HashSet::new(); - if let Some(specifier) = self.specifier_resolver.resolve(specifier) { + if let Some(specifier) = self.resolve_specifier(specifier) { recurse_dependents(&specifier, &self.dependents_map, &mut dependents); dependents.into_iter().collect() } else { @@ -1091,7 +1139,7 @@ impl Documents { /// Return a document for the specifier. pub fn get(&self, original_specifier: &ModuleSpecifier) -> Option { - let specifier = self.specifier_resolver.resolve(original_specifier)?; + let specifier = self.resolve_specifier(original_specifier)?; if let Some(document) = self.open_docs.get(&specifier) { Some(document.clone()) } else { @@ -1221,7 +1269,7 @@ impl Documents { pub fn set_cache(&mut self, cache: Arc) { // TODO update resolved dependencies? self.cache = cache.clone(); - self.specifier_resolver = Arc::new(SpecifierResolver::new(cache)); + self.redirect_resolver = Arc::new(RedirectResolver::new(cache)); self.dirty = true; } @@ -1257,6 +1305,7 @@ impl Documents { maybe_jsx_config: Option<&JsxImportSourceConfig>, maybe_vendor_dir: Option, maybe_package_json_deps: Option<&PackageJsonDeps>, + maybe_unstable_flags: Option<&Vec>, ) -> u64 { let mut hasher = FastInsecureHasher::default(); hasher.write_hashable(document_preload_limit); @@ -1272,6 +1321,7 @@ impl Documents { } hasher.write_hashable(maybe_vendor_dir); hasher.write_hashable(maybe_jsx_config); + hasher.write_hashable(maybe_unstable_flags); if let Some(package_json_deps) = &maybe_package_json_deps { // We need to ensure the hashing is deterministic so explicitly type // this in order to catch if the type of package_json_deps ever changes @@ -1307,11 +1357,13 @@ impl Documents { maybe_jsx_config.as_ref(), options.maybe_config_file.and_then(|c| c.vendor_dir_flag()), maybe_package_json_deps.as_ref(), + options.maybe_config_file.map(|c| &c.json.unstable), ); let deps_provider = Arc::new(PackageJsonDepsProvider::new(maybe_package_json_deps)); + let fs = Arc::new(RealFs); self.resolver = Arc::new(CliGraphResolver::new(CliGraphResolverOptions { - fs: Arc::new(RealFs), + fs: fs.clone(), node_resolver: options.node_resolver, npm_resolver: options.npm_resolver, cjs_resolutions: None, // only used for runtime @@ -1324,14 +1376,15 @@ impl Documents { .as_ref(), bare_node_builtins_enabled: options .maybe_config_file - .map(|config| { - config - .json - .unstable - .contains(&"bare-node-builtins".to_string()) - }) + .map(|config| config.has_unstable("bare-node-builtins")) .unwrap_or(false), + // Don't set this for the LSP because instead we'll use the OpenDocumentsLoader + // because it's much easier and we get diagnostics/quick fixes about a redirected + // specifier for free. + sloppy_imports_resolver: None, })); + self.redirect_resolver = + Arc::new(RedirectResolver::new(self.cache.clone())); self.imports = Arc::new( if let Some(Ok(imports)) = options.maybe_config_file.map(|cf| cf.to_maybe_imports()) @@ -1352,6 +1405,10 @@ impl Documents { IndexMap::new() }, ); + self.unstable_sloppy_imports = options + .maybe_config_file + .map(|c| c.has_unstable("sloppy-imports")) + .unwrap_or(false); // only refresh the dependencies if the underlying configuration has changed if self.resolver_config_hash != new_resolver_config_hash { @@ -1649,6 +1706,7 @@ fn node_resolve_npm_req_ref( pub struct OpenDocumentsGraphLoader<'a> { pub inner_loader: &'a mut dyn deno_graph::source::Loader, pub open_docs: &'a HashMap, + pub unstable_sloppy_imports: bool, } impl<'a> OpenDocumentsGraphLoader<'a> { @@ -1670,6 +1728,28 @@ impl<'a> OpenDocumentsGraphLoader<'a> { } None } + + fn resolve_unstable_sloppy_import<'b>( + &self, + specifier: &'b ModuleSpecifier, + ) -> UnstableSloppyImportsResolution<'b> { + UnstableSloppyImportsResolver::resolve_with_stat_sync(specifier, |path| { + if let Ok(specifier) = ModuleSpecifier::from_file_path(path) { + if self.open_docs.contains_key(&specifier) { + return Some(UnstableSloppyImportsFsEntry::File); + } + } + path.metadata().ok().and_then(|m| { + if m.is_file() { + Some(UnstableSloppyImportsFsEntry::File) + } else if m.is_dir() { + Some(UnstableSloppyImportsFsEntry::Dir) + } else { + None + } + }) + }) + } } impl<'a> deno_graph::source::Loader for OpenDocumentsGraphLoader<'a> { @@ -1683,9 +1763,19 @@ impl<'a> deno_graph::source::Loader for OpenDocumentsGraphLoader<'a> { is_dynamic: bool, cache_setting: deno_graph::source::CacheSetting, ) -> deno_graph::source::LoadFuture { - match self.load_from_docs(specifier) { + let specifier = if self.unstable_sloppy_imports { + self + .resolve_unstable_sloppy_import(specifier) + .into_specifier() + } else { + Cow::Borrowed(specifier) + }; + + match self.load_from_docs(&specifier) { Some(fut) => fut, - None => self.inner_loader.load(specifier, is_dynamic, cache_setting), + None => self + .inner_loader + .load(&specifier, is_dynamic, cache_setting), } } diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index 1104acc9c7..92173b8ad2 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -256,6 +256,7 @@ impl LanguageServer { let mut loader = crate::lsp::documents::OpenDocumentsGraphLoader { inner_loader: &mut inner_loader, open_docs: &open_docs, + unstable_sloppy_imports: cli_options.unstable_sloppy_imports(), }; let graph = module_graph_builder .create_graph_with_loader(GraphKind::All, roots.clone(), &mut loader) @@ -1080,7 +1081,7 @@ async fn create_npm_resolver( let is_byonm = std::env::var("DENO_UNSTABLE_BYONM").as_deref() == Ok("1") || maybe_config_file .as_ref() - .map(|c| c.json.unstable.iter().any(|c| c == "byonm")) + .map(|c| c.has_unstable("byonm")) .unwrap_or(false); create_cli_npm_resolver_for_lsp(if is_byonm { CliNpmResolverCreateOptions::Byonm(CliNpmResolverByonmCreateOptions { diff --git a/cli/lsp/tsc.rs b/cli/lsp/tsc.rs index 72dbcc3a8f..b4f4d52c76 100644 --- a/cli/lsp/tsc.rs +++ b/cli/lsp/tsc.rs @@ -3977,7 +3977,7 @@ fn op_script_names(state: &mut OpState) -> Vec { ); for specifier in specifiers { if seen.insert(specifier.as_str()) { - if let Some(specifier) = documents.resolve_redirected(specifier) { + if let Some(specifier) = documents.resolve_specifier(specifier) { // only include dependencies we know to exist otherwise typescript will error if documents.exists(&specifier) { result.push(specifier.to_string()); diff --git a/cli/module_loader.rs b/cli/module_loader.rs index 8332ea4f99..92116dc7b9 100644 --- a/cli/module_loader.rs +++ b/cli/module_loader.rs @@ -568,7 +568,11 @@ impl ModuleLoader for CliModuleLoader { // support in REPL. This should be fixed. let resolution = self.shared.resolver.resolve( specifier, - &referrer, + &deno_graph::Range { + specifier: referrer.clone(), + start: deno_graph::Position::zeroed(), + end: deno_graph::Position::zeroed(), + }, ResolutionMode::Execution, ); diff --git a/cli/resolver.rs b/cli/resolver.rs index 44de3aa3f8..12b86632ff 100644 --- a/cli/resolver.rs +++ b/cli/resolver.rs @@ -1,10 +1,12 @@ // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license. +use deno_ast::MediaType; use deno_core::anyhow::anyhow; use deno_core::error::AnyError; use deno_core::futures::future; use deno_core::futures::future::LocalBoxFuture; use deno_core::futures::FutureExt; +use deno_core::parking_lot::Mutex; use deno_core::ModuleSpecifier; use deno_graph::source::NpmPackageReqResolution; use deno_graph::source::NpmResolver; @@ -24,16 +26,21 @@ use deno_runtime::permissions::PermissionsContainer; use deno_semver::npm::NpmPackageReqReference; use deno_semver::package::PackageReq; use import_map::ImportMap; +use std::borrow::Cow; +use std::collections::HashMap; +use std::path::Path; use std::path::PathBuf; use std::sync::Arc; use crate::args::package_json::PackageJsonDeps; use crate::args::JsxImportSourceConfig; use crate::args::PackageJsonDepsProvider; +use crate::graph_util::format_range_with_colors; use crate::module_loader::CjsResolutionStore; use crate::npm::ByonmCliNpmResolver; use crate::npm::CliNpmResolver; use crate::npm::InnerCliNpmResolverRef; +use crate::util::path::specifier_to_file_path; use crate::util::sync::AtomicFlag; /// Result of checking if a specifier is mapped via @@ -110,6 +117,7 @@ impl MappedSpecifierResolver { #[derive(Debug)] pub struct CliGraphResolver { fs: Arc, + sloppy_imports_resolver: Option, mapped_specifier_resolver: MappedSpecifierResolver, maybe_default_jsx_import_source: Option, maybe_jsx_import_source_module: Option, @@ -124,6 +132,7 @@ pub struct CliGraphResolver { pub struct CliGraphResolverOptions<'a> { pub fs: Arc, pub cjs_resolutions: Option>, + pub sloppy_imports_resolver: Option, pub node_resolver: Option>, pub npm_resolver: Option>, pub package_json_deps_provider: Arc, @@ -143,6 +152,7 @@ impl CliGraphResolver { Self { fs: options.fs, cjs_resolutions: options.cjs_resolutions, + sloppy_imports_resolver: options.sloppy_imports_resolver, mapped_specifier_resolver: MappedSpecifierResolver::new( options.maybe_import_map, if is_byonm { @@ -232,7 +242,7 @@ impl Resolver for CliGraphResolver { fn resolve( &self, specifier: &str, - referrer: &ModuleSpecifier, + referrer_range: &deno_graph::Range, mode: ResolutionMode, ) -> Result { fn to_node_mode(mode: ResolutionMode) -> NodeResolutionMode { @@ -242,6 +252,7 @@ impl Resolver for CliGraphResolver { } } + let referrer = &referrer_range.specifier; let result = match self .mapped_specifier_resolver .resolve(specifier, referrer)? @@ -253,10 +264,26 @@ impl Resolver for CliGraphResolver { self.found_package_json_dep_flag.raise(); Ok(specifier) } - MappedResolution::None => deno_graph::resolve_import(specifier, referrer) - .map_err(|err| err.into()), + MappedResolution::None => { + deno_graph::resolve_import(specifier, &referrer_range.specifier) + .map_err(|err| err.into()) + } }; + // do sloppy imports resolution if enabled + let result = + if let Some(sloppy_imports_resolver) = &self.sloppy_imports_resolver { + result.map(|specifier| { + sloppy_imports_resolve( + sloppy_imports_resolver, + specifier, + referrer_range, + ) + }) + } else { + result + }; + // When the user is vendoring, don't allow them to import directly from the vendor/ directory // as it might cause them confusion or duplicate dependencies. Additionally, this folder has // special treatment in the language server so it will definitely cause issues/confusion there @@ -371,6 +398,52 @@ impl Resolver for CliGraphResolver { } } +fn sloppy_imports_resolve( + resolver: &UnstableSloppyImportsResolver, + specifier: ModuleSpecifier, + referrer_range: &deno_graph::Range, +) -> ModuleSpecifier { + let resolution = resolver.resolve(&specifier); + let hint_message = match &resolution { + UnstableSloppyImportsResolution::JsToTs(to_specifier) => { + let from_media_type = MediaType::from_specifier(&specifier); + let to_media_type = MediaType::from_specifier(to_specifier); + format!( + "update {} extension to {}", + from_media_type.as_ts_extension(), + to_media_type.as_ts_extension() + ) + } + UnstableSloppyImportsResolution::NoExtension(to_specifier) => { + let to_media_type = MediaType::from_specifier(to_specifier); + format!("add {} extension", to_media_type.as_ts_extension()) + } + UnstableSloppyImportsResolution::Directory(to_specifier) => { + let file_name = to_specifier + .path() + .rsplit_once('/') + .map(|(_, file_name)| file_name) + .unwrap_or(to_specifier.path()); + format!("specify path to {} file in directory instead", file_name) + } + UnstableSloppyImportsResolution::None(_) => return specifier, + }; + // show a warning when this happens in order to drive + // the user towards correcting these specifiers + log::warn!( + "{} Sloppy import resolution {}\n at {}", + crate::colors::yellow("Warning"), + crate::colors::gray(format!("(hint: {})", hint_message)), + if referrer_range.end == deno_graph::Position::zeroed() { + // not worth showing the range in this case + crate::colors::cyan(referrer_range.specifier.as_str()).to_string() + } else { + format_range_with_colors(referrer_range) + }, + ); + resolution.into_owned_specifier() +} + fn resolve_package_json_dep( specifier: &str, deps: &PackageJsonDeps, @@ -467,10 +540,208 @@ impl NpmResolver for CliGraphResolver { } } +#[derive(Debug)] +struct UnstableSloppyImportsStatCache { + fs: Arc, + cache: Mutex>>, +} + +impl UnstableSloppyImportsStatCache { + pub fn new(fs: Arc) -> Self { + Self { + fs, + cache: Default::default(), + } + } + + pub fn stat_sync(&self, path: &Path) -> Option { + // there will only ever be one thread in here at a + // time, so it's ok to hold the lock for so long + let mut cache = self.cache.lock(); + if let Some(entry) = cache.get(path) { + return *entry; + } + + let entry = self.fs.stat_sync(path).ok().and_then(|stat| { + if stat.is_file { + Some(UnstableSloppyImportsFsEntry::File) + } else if stat.is_directory { + Some(UnstableSloppyImportsFsEntry::Dir) + } else { + None + } + }); + cache.insert(path.to_owned(), entry); + entry + } +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum UnstableSloppyImportsFsEntry { + File, + Dir, +} + +#[derive(Debug, PartialEq, Eq)] +pub enum UnstableSloppyImportsResolution<'a> { + /// No sloppy resolution was found. + None(&'a ModuleSpecifier), + /// Ex. `./file.js` to `./file.ts` + JsToTs(ModuleSpecifier), + /// Ex. `./file` to `./file.ts` + NoExtension(ModuleSpecifier), + /// Ex. `./dir` to `./dir/index.ts` + Directory(ModuleSpecifier), +} + +impl<'a> UnstableSloppyImportsResolution<'a> { + pub fn into_specifier(self) -> Cow<'a, ModuleSpecifier> { + match self { + Self::None(specifier) => Cow::Borrowed(specifier), + Self::JsToTs(specifier) => Cow::Owned(specifier), + Self::NoExtension(specifier) => Cow::Owned(specifier), + Self::Directory(specifier) => Cow::Owned(specifier), + } + } + + pub fn into_owned_specifier(self) -> ModuleSpecifier { + match self { + Self::None(specifier) => specifier.clone(), + Self::JsToTs(specifier) => specifier, + Self::NoExtension(specifier) => specifier, + Self::Directory(specifier) => specifier, + } + } +} + +#[derive(Debug)] +pub struct UnstableSloppyImportsResolver { + stat_cache: UnstableSloppyImportsStatCache, +} + +impl UnstableSloppyImportsResolver { + pub fn new(fs: Arc) -> Self { + Self { + stat_cache: UnstableSloppyImportsStatCache::new(fs), + } + } + + pub fn resolve_with_stat_sync( + specifier: &ModuleSpecifier, + stat_sync: impl Fn(&Path) -> Option, + ) -> UnstableSloppyImportsResolution { + if specifier.scheme() != "file" { + return UnstableSloppyImportsResolution::None(specifier); + } + + let Ok(path) = specifier_to_file_path(specifier) else { + return UnstableSloppyImportsResolution::None(specifier); + }; + let mut is_dir_resolution = false; + let mut is_no_ext_resolution = false; + let probe_paths = match (stat_sync)(&path) { + Some(UnstableSloppyImportsFsEntry::File) => { + return UnstableSloppyImportsResolution::None(specifier); + } + Some(UnstableSloppyImportsFsEntry::Dir) => { + is_dir_resolution = true; + // try to resolve at the index file + vec![ + path.join("index.ts"), + path.join("index.js"), + path.join("index.mts"), + path.join("index.mjs"), + path.join("index.tsx"), + path.join("index.jsx"), + ] + } + None => { + let media_type = MediaType::from_specifier(specifier); + let probe_media_type_types = match media_type { + MediaType::JavaScript => vec![MediaType::TypeScript, MediaType::Tsx], + MediaType::Jsx => vec![MediaType::Tsx], + MediaType::Mjs => vec![MediaType::Mts], + MediaType::Cjs => vec![MediaType::Cts], + MediaType::TypeScript + | MediaType::Mts + | MediaType::Cts + | MediaType::Dts + | MediaType::Dmts + | MediaType::Dcts + | MediaType::Tsx + | MediaType::Json + | MediaType::Wasm + | MediaType::TsBuildInfo + | MediaType::SourceMap => { + return UnstableSloppyImportsResolution::None(specifier) + } + MediaType::Unknown => { + is_no_ext_resolution = true; + vec![ + MediaType::TypeScript, + MediaType::JavaScript, + MediaType::Tsx, + MediaType::Jsx, + MediaType::Mts, + MediaType::Mjs, + ] + } + }; + let old_path_str = path.to_string_lossy(); + let old_path_str = match media_type { + MediaType::Unknown => old_path_str, + _ => match old_path_str.strip_suffix(media_type.as_ts_extension()) { + Some(s) => Cow::Borrowed(s), + None => return UnstableSloppyImportsResolution::None(specifier), + }, + }; + probe_media_type_types + .into_iter() + .map(|media_type| { + PathBuf::from(format!( + "{}{}", + old_path_str, + media_type.as_ts_extension() + )) + }) + .collect::>() + } + }; + + for probe_path in probe_paths { + if (stat_sync)(&probe_path) == Some(UnstableSloppyImportsFsEntry::File) { + if let Ok(specifier) = ModuleSpecifier::from_file_path(probe_path) { + if is_dir_resolution { + return UnstableSloppyImportsResolution::Directory(specifier); + } else if is_no_ext_resolution { + return UnstableSloppyImportsResolution::NoExtension(specifier); + } else { + return UnstableSloppyImportsResolution::JsToTs(specifier); + } + } + } + } + + UnstableSloppyImportsResolution::None(specifier) + } + + pub fn resolve<'a>( + &self, + specifier: &'a ModuleSpecifier, + ) -> UnstableSloppyImportsResolution<'a> { + Self::resolve_with_stat_sync(specifier, |path| { + self.stat_cache.stat_sync(path) + }) + } +} + #[cfg(test)] mod test { use std::collections::BTreeMap; + use deno_runtime::deno_fs::RealFs; + use test_util::TestContext; + use super::*; #[test] @@ -532,4 +803,86 @@ mod test { // non-existent bare specifier assert_eq!(resolve("non-existent", &deps).unwrap(), None); } + + #[test] + fn test_unstable_sloppy_imports() { + fn resolve(specifier: &ModuleSpecifier) -> UnstableSloppyImportsResolution { + UnstableSloppyImportsResolver::resolve_with_stat_sync(specifier, |path| { + RealFs.stat_sync(path).ok().and_then(|stat| { + if stat.is_file { + Some(UnstableSloppyImportsFsEntry::File) + } else if stat.is_directory { + Some(UnstableSloppyImportsFsEntry::Dir) + } else { + None + } + }) + }) + } + + let context = TestContext::default(); + let temp_dir = context.temp_dir().path(); + + // scenarios like resolving ./example.js to ./example.ts + for (ext_from, ext_to) in [("js", "ts"), ("js", "tsx"), ("mjs", "mts")] { + let ts_file = temp_dir.join(format!("file.{}", ext_to)); + ts_file.write(""); + let ts_file_uri = ts_file.uri_file(); + assert_eq!( + resolve(&ts_file.uri_file()), + UnstableSloppyImportsResolution::None(&ts_file_uri), + ); + assert_eq!( + resolve( + &temp_dir + .uri_dir() + .join(&format!("file.{}", ext_from)) + .unwrap() + ), + UnstableSloppyImportsResolution::JsToTs(ts_file.uri_file()), + ); + ts_file.remove_file(); + } + + // no extension scenarios + for ext in ["js", "ts", "js", "tsx", "jsx", "mjs", "mts"] { + let file = temp_dir.join(format!("file.{}", ext)); + file.write(""); + assert_eq!( + resolve( + &temp_dir + .uri_dir() + .join("file") // no ext + .unwrap() + ), + UnstableSloppyImportsResolution::NoExtension(file.uri_file()), + ); + file.remove_file(); + } + + // .ts and .js exists, .js specified (goes to specified) + { + let ts_file = temp_dir.join("file.ts"); + ts_file.write(""); + let js_file = temp_dir.join("file.js"); + js_file.write(""); + let js_file_uri = js_file.uri_file(); + assert_eq!( + resolve(&js_file.uri_file()), + UnstableSloppyImportsResolution::None(&js_file_uri), + ); + } + + // resolving a directory to an index file + { + let routes_dir = temp_dir.join("routes"); + routes_dir.create_dir_all(); + let index_file = routes_dir.join("index.ts"); + index_file.write(""); + assert_eq!( + resolve(&routes_dir.uri_file()), + UnstableSloppyImportsResolution::Directory(index_file.uri_file()), + ); + } + } } diff --git a/cli/tests/integration/lsp_tests.rs b/cli/tests/integration/lsp_tests.rs index 6dfb0a29a4..711d0bcd88 100644 --- a/cli/tests/integration/lsp_tests.rs +++ b/cli/tests/integration/lsp_tests.rs @@ -1095,8 +1095,7 @@ fn lsp_import_attributes() { "only": ["quickfix"] } }), - ) - ; + ); assert_eq!( res, json!([{ @@ -10506,3 +10505,166 @@ fn lsp_jupyter_byonm_diagnostics() { ); client.shutdown(); } + +#[test] +fn lsp_sloppy_imports_warn() { + let context = TestContextBuilder::new().use_temp_cwd().build(); + let temp_dir = context.temp_dir(); + let temp_dir = temp_dir.path(); + temp_dir + .join("deno.json") + .write(r#"{ "unstable": ["sloppy-imports"] }"#); + // should work when exists on the fs and when doesn't + temp_dir.join("a.ts").write("export class A {}"); + let mut client = context.new_lsp_command().build(); + client.initialize(|builder| { + builder.set_root_uri(temp_dir.uri_dir()); + }); + client.did_open(json!({ + "textDocument": { + "uri": temp_dir.join("b.ts").uri_file(), + "languageId": "typescript", + "version": 1, + "text": "export class B {}", + }, + })); + let diagnostics = client.did_open(json!({ + "textDocument": { + "uri": temp_dir.join("file.ts").uri_file(), + "languageId": "typescript", + "version": 1, + "text": "import * as a from './a';\nimport * as b from './b.js';\nconsole.log(a)\nconsole.log(b);\n", + }, + })); + assert_eq!( + diagnostics.messages_with_source("deno"), + lsp::PublishDiagnosticsParams { + uri: temp_dir.join("file.ts").uri_file(), + diagnostics: vec![ + lsp::Diagnostic { + range: lsp::Range { + start: lsp::Position { + line: 0, + character: 19 + }, + end: lsp::Position { + line: 0, + character: 24 + } + }, + severity: Some(lsp::DiagnosticSeverity::INFORMATION), + code: Some(lsp::NumberOrString::String("redirect".to_string())), + source: Some("deno".to_string()), + message: format!( + "The import of \"{}\" was redirected to \"{}\".", + temp_dir.join("a").uri_file(), + temp_dir.join("a.ts").uri_file() + ), + data: Some(json!({ + "specifier": temp_dir.join("a").uri_file(), + "redirect": temp_dir.join("a.ts").uri_file() + })), + ..Default::default() + }, + lsp::Diagnostic { + range: lsp::Range { + start: lsp::Position { + line: 1, + character: 19 + }, + end: lsp::Position { + line: 1, + character: 27 + } + }, + severity: Some(lsp::DiagnosticSeverity::INFORMATION), + code: Some(lsp::NumberOrString::String("redirect".to_string())), + source: Some("deno".to_string()), + message: format!( + "The import of \"{}\" was redirected to \"{}\".", + temp_dir.join("b.js").uri_file(), + temp_dir.join("b.ts").uri_file() + ), + data: Some(json!({ + "specifier": temp_dir.join("b.js").uri_file(), + "redirect": temp_dir.join("b.ts").uri_file() + })), + ..Default::default() + } + ], + version: Some(1), + } + ); + + let res = client.write_request( + "textDocument/codeAction", + json!({ + "textDocument": { + "uri": temp_dir.join("file.ts").uri_file() + }, + "range": { + "start": { "line": 0, "character": 19 }, + "end": { "line": 0, "character": 24 } + }, + "context": { + "diagnostics": [{ + "range": { + "start": { "line": 0, "character": 19 }, + "end": { "line": 0, "character": 24 } + }, + "severity": 3, + "code": "redirect", + "source": "deno", + "message": format!( + "The import of \"{}\" was redirected to \"{}\".", + temp_dir.join("a").uri_file(), + temp_dir.join("a.ts").uri_file() + ), + "data": { + "specifier": temp_dir.join("a").uri_file(), + "redirect": temp_dir.join("a.ts").uri_file() + }, + }], + "only": ["quickfix"] + } + }), + ); + assert_eq!( + res, + json!([{ + "title": "Update specifier to its redirected specifier.", + "kind": "quickfix", + "diagnostics": [{ + "range": { + "start": { "line": 0, "character": 19 }, + "end": { "line": 0, "character": 24 } + }, + "severity": 3, + "code": "redirect", + "source": "deno", + "message": format!( + "The import of \"{}\" was redirected to \"{}\".", + temp_dir.join("a").uri_file(), + temp_dir.join("a.ts").uri_file() + ), + "data": { + "specifier": temp_dir.join("a").uri_file(), + "redirect": temp_dir.join("a.ts").uri_file() + }, + }], + "edit": { + "changes": { + temp_dir.join("file.ts").uri_file(): [{ + "range": { + "start": { "line": 0, "character": 19 }, + "end": { "line": 0, "character": 24 } + }, + "newText": "\"./a.ts\"" + }] + } + } + }]) + ); + + client.shutdown(); +} diff --git a/cli/tests/integration/run_tests.rs b/cli/tests/integration/run_tests.rs index b3bd98098c..4e0bbdfd20 100644 --- a/cli/tests/integration/run_tests.rs +++ b/cli/tests/integration/run_tests.rs @@ -4735,3 +4735,71 @@ itest!(unsafe_proto_flag { http_server: false, exit_code: 0, }); + +#[test] +fn test_unstable_sloppy_imports() { + let context = TestContextBuilder::new().use_temp_cwd().build(); + let temp_dir = context.temp_dir(); + temp_dir.write("deno.json", r#"{ "unstable": ["sloppy-imports"] }"#); + temp_dir.write("a.ts", "export class A {}"); + temp_dir.write("b.js", "export class B {}"); + temp_dir.write("c.mts", "export class C {}"); + temp_dir.write("d.mjs", "export class D {}"); + temp_dir.write("e.tsx", "export class E {}"); + temp_dir.write("f.jsx", "export class F {}"); + let dir = temp_dir.path().join("dir"); + dir.create_dir_all(); + dir.join("index.tsx").write("export class G {}"); + temp_dir.write( + "main.ts", + r#"import * as a from "./a.js"; +import * as b from "./b"; +import * as c from "./c"; +import * as d from "./d"; +import * as e from "./e"; +import * as e2 from "./e.js"; +import * as f from "./f"; +import * as g from "./dir"; +console.log(a.A); +console.log(b.B); +console.log(c.C); +console.log(d.D); +console.log(e.E); +console.log(e2.E); +console.log(f.F); +console.log(g.G); +"#, + ); + + context + .new_command() + .args("run main.ts") + .run() + .assert_matches_text( + "Warning Sloppy import resolution (hint: update .js extension to .ts) + at file:///[WILDCARD]/main.ts:1:20 +Warning Sloppy import resolution (hint: add .js extension) + at file:///[WILDCARD]/main.ts:2:20 +Warning Sloppy import resolution (hint: add .mts extension) + at file:///[WILDCARD]/main.ts:3:20 +Warning Sloppy import resolution (hint: add .mjs extension) + at file:///[WILDCARD]/main.ts:4:20 +Warning Sloppy import resolution (hint: add .tsx extension) + at file:///[WILDCARD]/main.ts:5:20 +Warning Sloppy import resolution (hint: update .js extension to .tsx) + at file:///[WILDCARD]/main.ts:6:21 +Warning Sloppy import resolution (hint: add .jsx extension) + at file:///[WILDCARD]/main.ts:7:20 +Warning Sloppy import resolution (hint: specify path to index.tsx file in directory instead) + at file:///[WILDCARD]/main.ts:8:20 +[class A] +[class B] +[class C] +[class D] +[class E] +[class E] +[class F] +[class G] +", + ); +} diff --git a/cli/tools/repl/session.rs b/cli/tools/repl/session.rs index 0a0dd66484..22dd30caf5 100644 --- a/cli/tools/repl/session.rs +++ b/cli/tools/repl/session.rs @@ -657,13 +657,18 @@ impl ReplSession { let mut collector = ImportCollector::new(); program.visit_with(&mut collector); + let referrer_range = deno_graph::Range { + specifier: self.referrer.clone(), + start: deno_graph::Position::zeroed(), + end: deno_graph::Position::zeroed(), + }; let resolved_imports = collector .imports .iter() .flat_map(|i| { self .resolver - .resolve(i, &self.referrer, ResolutionMode::Execution) + .resolve(i, &referrer_range, ResolutionMode::Execution) .ok() .or_else(|| ModuleSpecifier::parse(i).ok()) }) diff --git a/cli/tools/vendor/build.rs b/cli/tools/vendor/build.rs index dd7c053c17..80dda86cc3 100644 --- a/cli/tools/vendor/build.rs +++ b/cli/tools/vendor/build.rs @@ -114,7 +114,11 @@ pub async fn build< if let Some(specifier_text) = jsx_import_source.maybe_specifier_text() { if let Ok(specifier) = resolver.resolve( &specifier_text, - &jsx_import_source.base_url, + &deno_graph::Range { + specifier: jsx_import_source.base_url.clone(), + start: deno_graph::Position::zeroed(), + end: deno_graph::Position::zeroed(), + }, ResolutionMode::Execution, ) { entry_points.push(specifier); diff --git a/cli/tools/vendor/import_map.rs b/cli/tools/vendor/import_map.rs index b5893e9f54..5d565014a7 100644 --- a/cli/tools/vendor/import_map.rs +++ b/cli/tools/vendor/import_map.rs @@ -216,7 +216,11 @@ pub fn build_import_map( if let Some(specifier_text) = jsx_import_source.maybe_specifier_text() { if let Ok(resolved_url) = resolver.resolve( &specifier_text, - &jsx_import_source.base_url, + &deno_graph::Range { + specifier: jsx_import_source.base_url.clone(), + start: deno_graph::Position::zeroed(), + end: deno_graph::Position::zeroed(), + }, ResolutionMode::Execution, ) { builder.imports.add(specifier_text, &resolved_url); diff --git a/cli/tools/vendor/test.rs b/cli/tools/vendor/test.rs index 94e692a928..f013ad5835 100644 --- a/cli/tools/vendor/test.rs +++ b/cli/tools/vendor/test.rs @@ -300,6 +300,7 @@ fn build_resolver( node_resolver: None, npm_resolver: None, cjs_resolutions: None, + sloppy_imports_resolver: None, package_json_deps_provider: Default::default(), maybe_jsx_import_source_config, maybe_import_map: original_import_map.map(Arc::new),