feat(lsp): provide quick fixes for specifiers that could be resolved sloppily (#21506)

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'"
-    );
-  }
 }

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;
@@ -938,6 +940,13 @@ struct DiagnosticDataRedirect {
   pub redirect: ModuleSpecifier,
 }
 
+#[derive(Debug, Deserialize)]
+#[serde(rename_all = "camelCase")]
+struct DiagnosticDataNoLocal {
+  pub to: ModuleSpecifier,
+  pub message: String,
+}
+
 #[derive(Debug, Deserialize)]
 #[serde(rename_all = "camelCase")]
 struct DiagnosticDataImportMapRemap {
@@ -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!(
+      match code.as_str() {
-        code.as_str(),
         "import-map-remap"
-          | "no-cache"
+        | "no-cache"
-          | "no-cache-npm"
+        | "no-cache-npm"
-          | "no-attribute-type"
+        | "no-attribute-type"
-          | "redirect"
+        | "redirect"
-          | "import-node-prefix-missing"
+        | "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 {
+    fn no_local_message(
-      let fs: Arc<dyn deno_fs::FileSystem> = Arc::new(deno_fs::RealFs);
+      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) {
+    relative_specifier(redirect, referrer)
-      Some(relative) => {
-        if relative.starts_with('.') {
-          relative
-        } else {
-          format!("./{}", relative)
-        }
-      }
-      None => redirect.to_string(),
-    }
   } 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,

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)

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,

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 {

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| {
+    let entry = self
-      if stat.is_file {
+      .fs
-        Some(SloppyImportsFsEntry::File)
+      .stat_sync(path)
-      } else if stat.is_directory {
+      .ok()
-        Some(SloppyImportsFsEntry::Dir)
+      .and_then(|stat| SloppyImportsFsEntry::from_fs_stat(&stat));
-      } else {
-        None
-      }
-    });
     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(),
+      SloppyImportsResolution::None(_) => None,
-      Self::JsToTs(specifier) => specifier,
+      SloppyImportsResolution::JsToTs(specifier) => {
-      Self::NoExtension(specifier) => specifier,
+        let media_type = MediaType::from_specifier(specifier);
-      Self::Directory(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'"
+    );
+  }
 }

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();
 }

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,
         )?;

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,
         )?;

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,