diff options
| -rw-r--r-- | cli/graph_util.rs | 97 | ||||
| -rw-r--r-- | cli/lsp/diagnostics.rs | 96 | ||||
| -rw-r--r-- | cli/lsp/documents.rs | 3 | ||||
| -rw-r--r-- | cli/lsp/language_server.rs | 2 | ||||
| -rw-r--r-- | cli/module_loader.rs | 7 | ||||
| -rw-r--r-- | cli/resolver.rs | 133 | ||||
| -rw-r--r-- | cli/tests/integration/lsp_tests.rs | 76 | ||||
| -rw-r--r-- | cli/tools/bench/mod.rs | 2 | ||||
| -rw-r--r-- | cli/tools/test/mod.rs | 2 | ||||
| -rw-r--r-- | cli/tools/vendor/build.rs | 3 |
10 files changed, 285 insertions, 136 deletions
diff --git a/cli/graph_util.rs b/cli/graph_util.rs index eba88e4d0..95351ba86 100644 --- a/cli/graph_util.rs +++ b/cli/graph_util.rs @@ -12,7 +12,6 @@ use crate::errors::get_error_class_name; use crate::file_fetcher::FileFetcher; use crate::npm::CliNpmResolver; use crate::resolver::CliGraphResolver; -use crate::resolver::SloppyImportsResolution; use crate::resolver::SloppyImportsResolver; use crate::tools::check; use crate::tools::check::TypeChecker; @@ -20,7 +19,6 @@ use crate::util::file_watcher::WatcherCommunicator; use crate::util::sync::TaskQueue; use crate::util::sync::TaskQueuePermit; -use deno_ast::MediaType; use deno_core::anyhow::bail; use deno_core::anyhow::Context; use deno_core::error::custom_error; @@ -61,7 +59,7 @@ pub struct GraphValidOptions { /// error statically reachable from `roots` and not a dynamic import. pub fn graph_valid_with_cli_options( graph: &ModuleGraph, - fs: &Arc<dyn FileSystem>, + fs: &dyn FileSystem, roots: &[ModuleSpecifier], options: &CliOptions, ) -> Result<(), AnyError> { @@ -86,7 +84,7 @@ pub fn graph_valid_with_cli_options( /// for the CLI. pub fn graph_valid( graph: &ModuleGraph, - fs: &Arc<dyn FileSystem>, + fs: &dyn FileSystem, roots: &[ModuleSpecifier], options: GraphValidOptions, ) -> Result<(), AnyError> { @@ -366,7 +364,7 @@ impl ModuleGraphBuilder { let graph = Arc::new(graph); graph_valid_with_cli_options( &graph, - &self.fs, + self.fs.as_ref(), &graph.roots, &self.options, )?; @@ -538,12 +536,13 @@ pub fn enhanced_resolution_error_message(error: &ResolutionError) -> String { } pub fn enhanced_module_error_message( - fs: &Arc<dyn FileSystem>, + fs: &dyn FileSystem, error: &ModuleError, ) -> String { let additional_message = match error { ModuleError::Missing(specifier, _) => { - maybe_sloppy_imports_suggestion_message(fs, specifier) + SloppyImportsResolver::resolve_with_fs(fs, specifier) + .as_suggestion_message() } _ => None, }; @@ -557,48 +556,6 @@ pub fn enhanced_module_error_message( } } -pub fn maybe_sloppy_imports_suggestion_message( - fs: &Arc<dyn FileSystem>, - original_specifier: &ModuleSpecifier, -) -> Option<String> { - let sloppy_imports_resolver = SloppyImportsResolver::new(fs.clone()); - let resolution = sloppy_imports_resolver.resolve(original_specifier); - sloppy_import_resolution_to_suggestion_message(&resolution) -} - -fn sloppy_import_resolution_to_suggestion_message( - resolution: &SloppyImportsResolution, -) -> Option<String> { - match resolution { - SloppyImportsResolution::None(_) => None, - SloppyImportsResolution::JsToTs(specifier) => { - let media_type = MediaType::from_specifier(specifier); - Some(format!( - "Maybe change the extension to '{}'", - media_type.as_ts_extension() - )) - } - SloppyImportsResolution::NoExtension(specifier) => { - let media_type = MediaType::from_specifier(specifier); - Some(format!( - "Maybe add a '{}' extension", - media_type.as_ts_extension() - )) - } - SloppyImportsResolution::Directory(specifier) => { - let file_name = specifier - .path() - .rsplit_once('/') - .map(|(_, file_name)| file_name) - .unwrap_or(specifier.path()); - Some(format!( - "Maybe specify path to '{}' file in directory instead", - file_name - )) - } - } -} - pub fn get_resolution_error_bare_node_specifier( error: &ResolutionError, ) -> Option<&str> { @@ -972,46 +929,4 @@ mod test { assert_eq!(get_resolution_error_bare_node_specifier(&err), output,); } } - - #[test] - fn test_sloppy_import_resolution_to_message() { - // none - let url = ModuleSpecifier::parse("file:///dir/index.js").unwrap(); - assert_eq!( - sloppy_import_resolution_to_suggestion_message( - &SloppyImportsResolution::None(&url) - ), - None, - ); - // directory - assert_eq!( - sloppy_import_resolution_to_suggestion_message( - &SloppyImportsResolution::Directory( - ModuleSpecifier::parse("file:///dir/index.js").unwrap() - ) - ) - .unwrap(), - "Maybe specify path to 'index.js' file in directory instead" - ); - // no ext - assert_eq!( - sloppy_import_resolution_to_suggestion_message( - &SloppyImportsResolution::NoExtension( - ModuleSpecifier::parse("file:///dir/index.mjs").unwrap() - ) - ) - .unwrap(), - "Maybe add a '.mjs' extension" - ); - // js to ts - assert_eq!( - sloppy_import_resolution_to_suggestion_message( - &SloppyImportsResolution::JsToTs( - ModuleSpecifier::parse("file:///dir/index.mts").unwrap() - ) - ) - .unwrap(), - "Maybe change the extension to '.mts'" - ); - } } diff --git a/cli/lsp/diagnostics.rs b/cli/lsp/diagnostics.rs index 4dbb4e1dd..8034127e9 100644 --- a/cli/lsp/diagnostics.rs +++ b/cli/lsp/diagnostics.rs @@ -19,6 +19,8 @@ use crate::args::LintOptions; use crate::graph_util; use crate::graph_util::enhanced_resolution_error_message; use crate::lsp::lsp_custom::DiagnosticBatchNotificationParams; +use crate::resolver::SloppyImportsResolution; +use crate::resolver::SloppyImportsResolver; use crate::tools::lint::get_configured_rules; use deno_ast::MediaType; @@ -940,6 +942,13 @@ struct DiagnosticDataRedirect { #[derive(Debug, Deserialize)] #[serde(rename_all = "camelCase")] +struct DiagnosticDataNoLocal { + pub to: ModuleSpecifier, + pub message: String, +} + +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] struct DiagnosticDataImportMapRemap { pub from: String, pub to: String, @@ -1084,6 +1093,32 @@ impl DenoDiagnostic { ..Default::default() } } + "no-local" => { + let data = diagnostic + .data + .clone() + .ok_or_else(|| anyhow!("Diagnostic is missing data"))?; + let data: DiagnosticDataNoLocal = serde_json::from_value(data)?; + lsp::CodeAction { + title: data.message, + kind: Some(lsp::CodeActionKind::QUICKFIX), + diagnostics: Some(vec![diagnostic.clone()]), + edit: Some(lsp::WorkspaceEdit { + changes: Some(HashMap::from([( + specifier.clone(), + vec![lsp::TextEdit { + new_text: format!( + "\"{}\"", + relative_specifier(&data.to, specifier) + ), + range: diagnostic.range, + }], + )])), + ..Default::default() + }), + ..Default::default() + } + } "redirect" => { let data = diagnostic .data @@ -1150,15 +1185,16 @@ impl DenoDiagnostic { /// diagnostic is fixable or not pub fn is_fixable(diagnostic: &lsp_types::Diagnostic) -> bool { if let Some(lsp::NumberOrString::String(code)) = &diagnostic.code { - matches!( - code.as_str(), + match code.as_str() { "import-map-remap" - | "no-cache" - | "no-cache-npm" - | "no-attribute-type" - | "redirect" - | "import-node-prefix-missing" - ) + | "no-cache" + | "no-cache-npm" + | "no-attribute-type" + | "redirect" + | "import-node-prefix-missing" => true, + "no-local" => diagnostic.data.is_some(), + _ => false, + } } else { false } @@ -1167,12 +1203,14 @@ impl DenoDiagnostic { /// Convert to an lsp Diagnostic when the range the diagnostic applies to is /// provided. pub fn to_lsp_diagnostic(&self, range: &lsp::Range) -> lsp::Diagnostic { - fn no_local_message(specifier: &ModuleSpecifier) -> String { - let fs: Arc<dyn deno_fs::FileSystem> = Arc::new(deno_fs::RealFs); + fn no_local_message( + specifier: &ModuleSpecifier, + sloppy_resolution: SloppyImportsResolution, + ) -> String { let mut message = format!("Unable to load a local module: {}\n", specifier); if let Some(additional_message) = - graph_util::maybe_sloppy_imports_suggestion_message(&fs, specifier) + sloppy_resolution.as_suggestion_message() { message.push_str(&additional_message); message.push('.'); @@ -1189,7 +1227,17 @@ impl DenoDiagnostic { Self::NoAttributeType => (lsp::DiagnosticSeverity::ERROR, "The module is a JSON module and not being imported with an import attribute. Consider adding `with { type: \"json\" }` to the import statement.".to_string(), None), Self::NoCache(specifier) => (lsp::DiagnosticSeverity::ERROR, format!("Uncached or missing remote URL: {specifier}"), Some(json!({ "specifier": specifier }))), Self::NoCacheNpm(pkg_req, specifier) => (lsp::DiagnosticSeverity::ERROR, format!("Uncached or missing npm package: {}", pkg_req), Some(json!({ "specifier": specifier }))), - Self::NoLocal(specifier) => (lsp::DiagnosticSeverity::ERROR, no_local_message(specifier), None), + Self::NoLocal(specifier) => { + let sloppy_resolution = SloppyImportsResolver::resolve_with_fs(&deno_fs::RealFs, specifier); + let data = sloppy_resolution.as_lsp_quick_fix_message().map(|message| { + json!({ + "specifier": specifier, + "to": sloppy_resolution.as_specifier(), + "message": message, + }) + }); + (lsp::DiagnosticSeverity::ERROR, no_local_message(specifier, sloppy_resolution), data) + }, Self::Redirect { from, to} => (lsp::DiagnosticSeverity::INFORMATION, format!("The import of \"{from}\" was redirected to \"{to}\"."), Some(json!({ "specifier": from, "redirect": to }))), Self::ResolutionError(err) => ( lsp::DiagnosticSeverity::ERROR, @@ -1218,21 +1266,25 @@ fn specifier_text_for_redirected( ) -> 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(), - } + relative_specifier(redirect, referrer) } else { redirect.to_string() } } +fn relative_specifier(specifier: &lsp::Url, referrer: &lsp::Url) -> String { + match referrer.make_relative(specifier) { + Some(relative) => { + if relative.starts_with('.') { + relative + } else { + format!("./{}", relative) + } + } + None => specifier.to_string(), + } +} + fn diagnose_resolution( snapshot: &language_server::StateSnapshot, dependency_key: &str, diff --git a/cli/lsp/documents.rs b/cli/lsp/documents.rs index a341ae207..535f32d3f 100644 --- a/cli/lsp/documents.rs +++ b/cli/lsp/documents.rs @@ -1055,7 +1055,8 @@ impl Documents { Some( self .resolve_unstable_sloppy_import(specifier) - .into_owned_specifier(), + .into_specifier() + .into_owned(), ) } else { self.redirect_resolver.resolve(specifier) diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index bc8a10235..a893ab3a8 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -263,7 +263,7 @@ impl LanguageServer { .await?; graph_util::graph_valid( &graph, - factory.fs(), + factory.fs().as_ref(), &roots, graph_util::GraphValidOptions { is_vendoring: false, diff --git a/cli/module_loader.rs b/cli/module_loader.rs index afd2d1999..b10b2f627 100644 --- a/cli/module_loader.rs +++ b/cli/module_loader.rs @@ -169,7 +169,12 @@ impl ModuleLoadPreparer { ) .await?; - graph_valid_with_cli_options(graph, &self.fs, &roots, &self.options)?; + graph_valid_with_cli_options( + graph, + self.fs.as_ref(), + &roots, + &self.options, + )?; // If there is a lockfile... if let Some(lockfile) = &self.lockfile { diff --git a/cli/resolver.rs b/cli/resolver.rs index 6fd034979..45a7e865b 100644 --- a/cli/resolver.rs +++ b/cli/resolver.rs @@ -441,7 +441,7 @@ fn sloppy_imports_resolve( format_range_with_colors(referrer_range) }, ); - resolution.into_owned_specifier() + resolution.into_specifier().into_owned() } fn resolve_package_json_dep( @@ -562,15 +562,11 @@ impl SloppyImportsStatCache { return *entry; } - let entry = self.fs.stat_sync(path).ok().and_then(|stat| { - if stat.is_file { - Some(SloppyImportsFsEntry::File) - } else if stat.is_directory { - Some(SloppyImportsFsEntry::Dir) - } else { - None - } - }); + let entry = self + .fs + .stat_sync(path) + .ok() + .and_then(|stat| SloppyImportsFsEntry::from_fs_stat(&stat)); cache.insert(path.to_owned(), entry); entry } @@ -582,6 +578,20 @@ pub enum SloppyImportsFsEntry { Dir, } +impl SloppyImportsFsEntry { + pub fn from_fs_stat( + stat: &deno_runtime::deno_io::fs::FsStat, + ) -> Option<SloppyImportsFsEntry> { + if stat.is_file { + Some(SloppyImportsFsEntry::File) + } else if stat.is_directory { + Some(SloppyImportsFsEntry::Dir) + } else { + None + } + } +} + #[derive(Debug, PartialEq, Eq)] pub enum SloppyImportsResolution<'a> { /// No sloppy resolution was found. @@ -595,6 +605,15 @@ pub enum SloppyImportsResolution<'a> { } impl<'a> SloppyImportsResolution<'a> { + pub fn as_specifier(&self) -> &ModuleSpecifier { + match self { + Self::None(specifier) => specifier, + Self::JsToTs(specifier) => specifier, + Self::NoExtension(specifier) => specifier, + Self::Directory(specifier) => specifier, + } + } + pub fn into_specifier(self) -> Cow<'a, ModuleSpecifier> { match self { Self::None(specifier) => Cow::Borrowed(specifier), @@ -604,12 +623,48 @@ impl<'a> SloppyImportsResolution<'a> { } } - pub fn into_owned_specifier(self) -> ModuleSpecifier { + pub fn as_suggestion_message(&self) -> Option<String> { + Some(format!("Maybe {}", self.as_base_message()?)) + } + + pub fn as_lsp_quick_fix_message(&self) -> Option<String> { + let message = self.as_base_message()?; + let mut chars = message.chars(); + Some(format!( + "{}{}.", + chars.next().unwrap().to_uppercase(), + chars.as_str() + )) + } + + fn as_base_message(&self) -> Option<String> { match self { - Self::None(specifier) => specifier.clone(), - Self::JsToTs(specifier) => specifier, - Self::NoExtension(specifier) => specifier, - Self::Directory(specifier) => specifier, + SloppyImportsResolution::None(_) => None, + SloppyImportsResolution::JsToTs(specifier) => { + let media_type = MediaType::from_specifier(specifier); + Some(format!( + "change the extension to '{}'", + media_type.as_ts_extension() + )) + } + SloppyImportsResolution::NoExtension(specifier) => { + let media_type = MediaType::from_specifier(specifier); + Some(format!( + "add a '{}' extension", + media_type.as_ts_extension() + )) + } + SloppyImportsResolution::Directory(specifier) => { + let file_name = specifier + .path() + .rsplit_once('/') + .map(|(_, file_name)| file_name) + .unwrap_or(specifier.path()); + Some(format!( + "specify path to '{}' file in directory instead", + file_name + )) + } } } } @@ -626,6 +681,17 @@ impl SloppyImportsResolver { } } + pub fn resolve_with_fs<'a>( + fs: &dyn FileSystem, + specifier: &'a ModuleSpecifier, + ) -> SloppyImportsResolution<'a> { + Self::resolve_with_stat_sync(specifier, |path| { + fs.stat_sync(path) + .ok() + .and_then(|stat| SloppyImportsFsEntry::from_fs_stat(&stat)) + }) + } + pub fn resolve_with_stat_sync( specifier: &ModuleSpecifier, stat_sync: impl Fn(&Path) -> Option<SloppyImportsFsEntry>, @@ -885,4 +951,41 @@ mod test { ); } } + + #[test] + fn test_sloppy_import_resolution_suggestion_message() { + // none + let url = ModuleSpecifier::parse("file:///dir/index.js").unwrap(); + assert_eq!( + SloppyImportsResolution::None(&url).as_suggestion_message(), + None, + ); + // directory + assert_eq!( + SloppyImportsResolution::Directory( + ModuleSpecifier::parse("file:///dir/index.js").unwrap() + ) + .as_suggestion_message() + .unwrap(), + "Maybe specify path to 'index.js' file in directory instead" + ); + // no ext + assert_eq!( + SloppyImportsResolution::NoExtension( + ModuleSpecifier::parse("file:///dir/index.mjs").unwrap() + ) + .as_suggestion_message() + .unwrap(), + "Maybe add a '.mjs' extension" + ); + // js to ts + assert_eq!( + SloppyImportsResolution::JsToTs( + ModuleSpecifier::parse("file:///dir/index.mts").unwrap() + ) + .as_suggestion_message() + .unwrap(), + "Maybe change the extension to '.mts'" + ); + } } diff --git a/cli/tests/integration/lsp_tests.rs b/cli/tests/integration/lsp_tests.rs index 9283b4021..98aaaebb4 100644 --- a/cli/tests/integration/lsp_tests.rs +++ b/cli/tests/integration/lsp_tests.rs @@ -10622,7 +10622,7 @@ fn lsp_sloppy_imports_warn() { ), "data": { "specifier": temp_dir.join("a").uri_file(), - "redirect": temp_dir.join("a.ts").uri_file() + "redirect": temp_dir.join("a.ts").uri_file(), }, }], "only": ["quickfix"] @@ -10713,10 +10713,84 @@ fn sloppy_imports_not_enabled() { "Unable to load a local module: {}\nMaybe add a '.ts' extension.", temp_dir.join("a").uri_file(), ), + data: Some(json!({ + "specifier": temp_dir.join("a").uri_file(), + "to": temp_dir.join("a.ts").uri_file(), + "message": "Add a '.ts' extension.", + })), ..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": "no-local", + "source": "deno", + "message": format!( + "Unable to load a local module: {}\nMaybe add a '.ts' extension.", + temp_dir.join("a").uri_file(), + ), + "data": { + "specifier": temp_dir.join("a").uri_file(), + "to": temp_dir.join("a.ts").uri_file(), + "message": "Add a '.ts' extension.", + }, + }], + "only": ["quickfix"] + } + }), + ); + assert_eq!( + res, + json!([{ + "title": "Add a '.ts' extension.", + "kind": "quickfix", + "diagnostics": [{ + "range": { + "start": { "line": 0, "character": 19 }, + "end": { "line": 0, "character": 24 } + }, + "severity": 3, + "code": "no-local", + "source": "deno", + "message": format!( + "Unable to load a local module: {}\nMaybe add a '.ts' extension.", + temp_dir.join("a").uri_file(), + ), + "data": { + "specifier": temp_dir.join("a").uri_file(), + "to": temp_dir.join("a.ts").uri_file(), + "message": "Add a '.ts' extension.", + }, + }], + "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/tools/bench/mod.rs b/cli/tools/bench/mod.rs index b04aa757d..57d148463 100644 --- a/cli/tools/bench/mod.rs +++ b/cli/tools/bench/mod.rs @@ -497,7 +497,7 @@ pub async fn run_benchmarks_with_watch( .await?; graph_valid_with_cli_options( &graph, - factory.fs(), + factory.fs().as_ref(), &bench_modules, cli_options, )?; diff --git a/cli/tools/test/mod.rs b/cli/tools/test/mod.rs index 5d943d716..840c5ac87 100644 --- a/cli/tools/test/mod.rs +++ b/cli/tools/test/mod.rs @@ -1282,7 +1282,7 @@ pub async fn run_tests_with_watch( .await?; graph_valid_with_cli_options( &graph, - factory.fs(), + factory.fs().as_ref(), &test_modules, &cli_options, )?; diff --git a/cli/tools/vendor/build.rs b/cli/tools/vendor/build.rs index 62fc0aa9a..4cfadb901 100644 --- a/cli/tools/vendor/build.rs +++ b/cli/tools/vendor/build.rs @@ -135,10 +135,9 @@ pub async fn build< } // surface any errors - let fs: Arc<dyn deno_fs::FileSystem> = Arc::new(deno_fs::RealFs); graph_util::graph_valid( &graph, - &fs, + &deno_fs::RealFs, &graph.roots, graph_util::GraphValidOptions { is_vendoring: true, |