fix(cli): config file should resolve paths relative to the config file (#12867)

* Add `specifier_to_file_path` to support converting a ModuleSpecifier with a unix-style path to a PathBuf on Windows.

cli/config_file.rs

@@ -286,13 +286,52 @@ pub struct LintRulesConfig {
 
 #[derive(Clone, Debug, Default, Deserialize)]
 #[serde(default, deny_unknown_fields)]
-pub struct FilesConfig {
+struct SerializedFilesConfig {
   pub include: Vec<String>,
   pub exclude: Vec<String>,
 }
 
+impl SerializedFilesConfig {
+  pub fn into_resolved(self, config_file_path: &Path) -> FilesConfig {
+    let config_dir = config_file_path.parent().unwrap();
+    FilesConfig {
+      include: self
+        .include
+        .into_iter()
+        .map(|p| config_dir.join(p))
+        .collect(),
+      exclude: self
+        .exclude
+        .into_iter()
+        .map(|p| config_dir.join(p))
+        .collect(),
+    }
+  }
+}
+
+#[derive(Clone, Debug, Default)]
+pub struct FilesConfig {
+  pub include: Vec<PathBuf>,
+  pub exclude: Vec<PathBuf>,
+}
+
 #[derive(Clone, Debug, Default, Deserialize)]
 #[serde(default, deny_unknown_fields)]
+struct SerializedLintConfig {
+  pub rules: LintRulesConfig,
+  pub files: SerializedFilesConfig,
+}
+
+impl SerializedLintConfig {
+  pub fn into_resolved(self, config_file_path: &Path) -> LintConfig {
+    LintConfig {
+      rules: self.rules,
+      files: self.files.into_resolved(config_file_path),
+    }
+  }
+}
+
+#[derive(Clone, Debug, Default)]
 pub struct LintConfig {
   pub rules: LintRulesConfig,
   pub files: FilesConfig,
@@ -318,6 +357,21 @@ pub struct FmtOptionsConfig {
 
 #[derive(Clone, Debug, Default, Deserialize)]
 #[serde(default, deny_unknown_fields)]
+struct SerializedFmtConfig {
+  pub options: FmtOptionsConfig,
+  pub files: SerializedFilesConfig,
+}
+
+impl SerializedFmtConfig {
+  pub fn into_resolved(self, config_file_path: &Path) -> FmtConfig {
+    FmtConfig {
+      options: self.options,
+      files: self.files.into_resolved(config_file_path),
+    }
+  }
+}
+
+#[derive(Clone, Debug, Default)]
 pub struct FmtConfig {
   pub options: FmtOptionsConfig,
   pub files: FilesConfig,
@@ -402,9 +456,9 @@ impl ConfigFile {
 
   pub fn to_lint_config(&self) -> Result<Option<LintConfig>, AnyError> {
     if let Some(config) = self.json.lint.clone() {
-      let lint_config: LintConfig = serde_json::from_value(config)
+      let lint_config: SerializedLintConfig = serde_json::from_value(config)
         .context("Failed to parse \"lint\" configuration")?;
-      Ok(Some(lint_config))
+      Ok(Some(lint_config.into_resolved(&self.path)))
     } else {
       Ok(None)
     }
@@ -460,9 +514,9 @@ impl ConfigFile {
 
   pub fn to_fmt_config(&self) -> Result<Option<FmtConfig>, AnyError> {
     if let Some(config) = self.json.fmt.clone() {
-      let fmt_config: FmtConfig = serde_json::from_value(config)
+      let fmt_config: SerializedFmtConfig = serde_json::from_value(config)
         .context("Failed to parse \"fmt\" configuration")?;
-      Ok(Some(fmt_config))
+      Ok(Some(fmt_config.into_resolved(&self.path)))
     } else {
       Ok(None)
     }
@@ -549,7 +603,8 @@ mod tests {
         }
       }
     }"#;
-    let config_path = PathBuf::from("/deno/tsconfig.json");
+    let config_dir = PathBuf::from("/deno");
+    let config_path = config_dir.join("tsconfig.json");
     let config_file = ConfigFile::new(config_text, &config_path).unwrap();
     let (options_value, ignored) =
       config_file.to_compiler_options().expect("error parsing");
@@ -569,8 +624,11 @@ mod tests {
       .to_lint_config()
       .expect("error parsing lint object")
       .expect("lint object should be defined");
-    assert_eq!(lint_config.files.include, vec!["src/"]);
+    assert_eq!(lint_config.files.include, vec![config_dir.join("src")]);
-    assert_eq!(lint_config.files.exclude, vec!["src/testdata/"]);
+    assert_eq!(
+      lint_config.files.exclude,
+      vec![config_dir.join("src/testdata/")]
+    );
     assert_eq!(
       lint_config.rules.include,
       Some(vec!["ban-untagged-todo".to_string()])
@@ -585,8 +643,11 @@ mod tests {
       .to_fmt_config()
       .expect("error parsing fmt object")
       .expect("fmt object should be defined");
-    assert_eq!(fmt_config.files.include, vec!["src/"]);
+    assert_eq!(fmt_config.files.include, vec![config_dir.join("src")]);
-    assert_eq!(fmt_config.files.exclude, vec!["src/testdata/"]);
+    assert_eq!(
+      fmt_config.files.exclude,
+      vec![config_dir.join("src/testdata/")]
+    );
     assert_eq!(fmt_config.options.use_tabs, Some(true));
     assert_eq!(fmt_config.options.line_width, Some(80));
     assert_eq!(fmt_config.options.indent_width, Some(4));

cli/fs_util.rs

@@ -1,7 +1,7 @@
 // Copyright 2018-2021 the Deno authors. All rights reserved. MIT license.
 
 use deno_core::anyhow::Context;
-use deno_core::error::AnyError;
+use deno_core::error::{uri_error, AnyError};
 pub use deno_core::normalize_path;
 use deno_core::ModuleSpecifier;
 use deno_runtime::deno_crypto::rand;
@@ -298,8 +298,8 @@ where
   Ok(prepared)
 }
 
-// Asynchronously removes a directory and all its descendants, but does not error
+/// Asynchronously removes a directory and all its descendants, but does not error
-// when the directory does not exist.
+/// when the directory does not exist.
 pub async fn remove_dir_all_if_exists(path: &Path) -> std::io::Result<()> {
   let result = tokio::fs::remove_dir_all(path).await;
   match result {
@@ -308,6 +308,48 @@ pub async fn remove_dir_all_if_exists(path: &Path) -> std::io::Result<()> {
   }
 }
 
+/// Attempts to convert a specifier to a file path. By default, uses the Url
+/// crate's `to_file_path()` method, but falls back to try and resolve unix-style
+/// paths on Windows.
+pub fn specifier_to_file_path(
+  specifier: &ModuleSpecifier,
+) -> Result<PathBuf, AnyError> {
+  let result = if cfg!(windows) {
+    match specifier.to_file_path() {
+      Ok(path) => Ok(path),
+      Err(()) => {
+        // This might be a unix-style path which is used in the tests even on Windows.
+        // Attempt to see if we can convert it to a `PathBuf`. This code should be removed
+        // once/if https://github.com/servo/rust-url/issues/730 is implemented.
+        if specifier.scheme() == "file"
+          && specifier.host().is_none()
+          && specifier.port().is_none()
+          && specifier.path_segments().is_some()
+        {
+          let path_str = specifier.path();
+          match String::from_utf8(
+            percent_encoding::percent_decode(path_str.as_bytes()).collect(),
+          ) {
+            Ok(path_str) => Ok(PathBuf::from(path_str)),
+            Err(_) => Err(()),
+          }
+        } else {
+          Err(())
+        }
+      }
+    }
+  } else {
+    specifier.to_file_path()
+  };
+  match result {
+    Ok(path) => Ok(path),
+    Err(()) => Err(uri_error(format!(
+      "Invalid file path.\n  Specifier: {}",
+      specifier
+    ))),
+  }
+}
+
 #[cfg(test)]
 mod tests {
   use super::*;
@@ -628,4 +670,22 @@ mod tests {
       );
     }
   }
+
+  #[test]
+  fn test_specifier_to_file_path() {
+    run_success_test("file:///", "/");
+    run_success_test("file:///test", "/test");
+    run_success_test("file:///dir/test/test.txt", "/dir/test/test.txt");
+    run_success_test(
+      "file:///dir/test%20test/test.txt",
+      "/dir/test test/test.txt",
+    );
+
+    fn run_success_test(specifier: &str, expected_path: &str) {
+      let result =
+        specifier_to_file_path(&ModuleSpecifier::parse(specifier).unwrap())
+          .unwrap();
+      assert_eq!(result, PathBuf::from(expected_path));
+    }
+  }
 }

cli/lsp/completions.rs

@@ -5,6 +5,7 @@ use super::lsp_custom;
 use super::tsc;
 
 use crate::fs_util::is_supported_ext;
+use crate::fs_util::specifier_to_file_path;
 
 use deno_core::normalize_path;
 use deno_core::resolve_path;
@@ -180,7 +181,7 @@ fn get_local_completions(
     return None;
   }
 
-  let mut base_path = base.to_file_path().ok()?;
+  let mut base_path = specifier_to_file_path(base).ok()?;
   base_path.pop();
   let mut current_path = normalize_path(base_path.join(current));
   // if the current text does not end in a `/` then we are still selecting on
@@ -340,7 +341,10 @@ fn relative_specifier(
     || specifier.port_or_known_default() != base.port_or_known_default()
   {
     if specifier.scheme() == "file" {
-      specifier.to_file_path().unwrap().to_string_lossy().into()
+      specifier_to_file_path(specifier)
+        .unwrap()
+        .to_string_lossy()
+        .into()
     } else {
       specifier.as_str().into()
     }

cli/lsp/diagnostics.rs

@@ -8,6 +8,7 @@ use super::language_server;
 use super::tsc;
 
 use crate::diagnostics;
+use crate::fs_util::specifier_to_file_path;
 
 use deno_core::anyhow::anyhow;
 use deno_core::error::AnyError;
@@ -301,68 +302,46 @@ fn ts_json_to_diagnostics(
     .collect()
 }
 
-// Returns `ConfigSnapshot::root_uri` in the correct format.
-fn get_root_specifier(
-  snapshot: &language_server::StateSnapshot,
-) -> Option<ModuleSpecifier> {
-  let root = snapshot.config.root_uri.as_ref()?;
-  let root = root.to_file_path().ok()?;
-
-  // FIXME: `root_uri` from `ConfigSnapshot` are without a trailing slash,
-  // so `Url::join` treats the root as a file, not a directory, and erases the folder name.
-  // To fix that behaviour we just parsing `root_uri` again.
-  ModuleSpecifier::from_directory_path(root).ok()
-}
-
 // Filters documents according to the `include` and the `exclude` lists (from `StateSnapshot::maybe_lint_config`).
 // If a document is in the `exclude` list - then it be removed.
 // If the `include` list is not empty, and a document is not in - then it be removed too.
-fn filter_documents(
+fn filter_lint_documents(
   snapshot: &language_server::StateSnapshot,
   documents: &mut Vec<Document>,
 ) {
-  let root_uri = match get_root_specifier(snapshot) {
+  let lint_config = match &snapshot.maybe_lint_config {
-    Some(uri) => uri,
-    None => return,
-  };
-
-  let linter_config = match &snapshot.maybe_lint_config {
     Some(config) => config,
     None => return,
   };
 
-  let join_specifiers = |specifiers: &Vec<String>| -> Vec<ModuleSpecifier> {
-    specifiers
-      .iter()
-      .filter_map(|i| root_uri.join(i).ok())
-      .collect()
-  };
-
-  let ignore_specifiers = join_specifiers(&linter_config.files.exclude);
-  let include_specifiers = join_specifiers(&linter_config.files.include);
-
   documents.retain(|doc| {
-    let path = doc.specifier().path();
+    let path = if let Ok(file_path) = specifier_to_file_path(doc.specifier()) {
+      file_path
+    } else {
+      return false;
+    };
 
     // Skip files which is in the exclude list.
-    if ignore_specifiers.iter().any(|i| path.starts_with(i.path())) {
+    if lint_config
-      return false;
+      .files
-    }
+      .exclude
-
-    // Early return if the include list is empty.
-    if include_specifiers.is_empty() {
-      return true;
-    }
-
-    // Ignore files which is not in the include list.
-    if !include_specifiers
       .iter()
-      .any(|i| path.starts_with(i.path()))
+      .any(|i| path.starts_with(i))
     {
       return false;
     }
 
-    true
+    // Early return if the include list is empty.
+    if lint_config.files.include.is_empty() {
+      return true;
+    }
+
+    // Ignore files not in the include list.
+    lint_config
+      .files
+      .include
+      .iter()
+      .any(|i| path.starts_with(i))
   });
 }
 
@@ -374,7 +353,7 @@ async fn generate_lint_diagnostics(
   let workspace_settings = snapshot.config.settings.workspace.clone();
   let maybe_lint_config = snapshot.maybe_lint_config.clone();
 
-  filter_documents(snapshot, &mut documents);
+  filter_lint_documents(snapshot, &mut documents);
 
   tokio::task::spawn(async move {
     let mut diagnostics_vec = Vec::new();

cli/lsp/documents.rs

@@ -8,6 +8,7 @@ use crate::config_file::ConfigFile;
 use crate::file_fetcher::get_source_from_bytes;
 use crate::file_fetcher::map_content_type;
 use crate::file_fetcher::SUPPORTED_SCHEMES;
+use crate::fs_util::specifier_to_file_path;
 use crate::http_cache;
 use crate::http_cache::HttpCache;
 use crate::resolver::ImportMapResolver;
@@ -741,7 +742,7 @@ fn get_document_path(
   specifier: &ModuleSpecifier,
 ) -> Option<PathBuf> {
   if specifier.scheme() == "file" {
-    specifier.to_file_path().ok()
+    specifier_to_file_path(specifier).ok()
   } else {
     let path = cache.get_cache_filename(specifier)?;
     if path.is_file() {

cli/lsp/language_server.rs

@@ -60,6 +60,7 @@ use crate::config_file::TsConfig;
 use crate::deno_dir;
 use crate::file_fetcher::get_source_from_data_url;
 use crate::fs_util;
+use crate::fs_util::specifier_to_file_path;
 use crate::logger;
 use crate::tools::fmt::format_file;
 use crate::tools::fmt::format_parsed_source;
@@ -303,9 +304,7 @@ impl Inner {
         let config_url = if let Ok(url) = Url::from_file_path(config_str) {
           Ok(url)
         } else if let Some(root_uri) = maybe_root_uri {
-          let root_path = root_uri
+          let root_path = specifier_to_file_path(&root_uri)?;
-            .to_file_path()
-            .map_err(|_| anyhow!("Bad root_uri: {}", root_uri))?;
           let config_path = root_path.join(config_str);
           Url::from_file_path(config_path).map_err(|_| {
             anyhow!("Bad file path for configuration file: \"{}\"", config_str)
@@ -319,9 +318,7 @@ impl Inner {
         info!("  Resolved configuration file: \"{}\"", config_url);
 
         let config_file = {
-          let buffer = config_url
+          let buffer = specifier_to_file_path(&config_url)?;
-            .to_file_path()
-            .map_err(|_| anyhow!("Bad uri: \"{}\"", config_url))?;
           let path = buffer
             .to_str()
             .ok_or_else(|| anyhow!("Bad uri: \"{}\"", config_url))?;
@@ -404,9 +401,7 @@ impl Inner {
       let cache_url = if let Ok(url) = Url::from_file_path(cache_str) {
         Ok(url)
       } else if let Some(root_uri) = &maybe_root_uri {
-        let root_path = root_uri
+        let root_path = specifier_to_file_path(root_uri)?;
-          .to_file_path()
-          .map_err(|_| anyhow!("Bad root_uri: {}", root_uri))?;
         let cache_path = root_path.join(cache_str);
         Url::from_file_path(cache_path).map_err(|_| {
           anyhow!("Bad file path for import path: {:?}", cache_str)
@@ -417,9 +412,7 @@ impl Inner {
           cache_str
         ))
       }?;
-      let cache_path = cache_url.to_file_path().map_err(|_| {
+      let cache_path = specifier_to_file_path(&cache_url)?;
-        anyhow!("Cannot convert \"{}\" into a file path.", cache_url)
-      })?;
       info!(
         "  Resolved cache path: \"{}\"",
         cache_path.to_string_lossy()
@@ -464,9 +457,7 @@ impl Inner {
           anyhow!("Bad data url for import map: {:?}", import_map_str)
         })
       } else if let Some(root_uri) = &maybe_root_uri {
-        let root_path = root_uri
+        let root_path = specifier_to_file_path(root_uri)?;
-          .to_file_path()
-          .map_err(|_| anyhow!("Bad root_uri: {}", root_uri))?;
         let import_map_path = root_path.join(import_map_str);
         Url::from_file_path(import_map_path).map_err(|_| {
           anyhow!("Bad file path for import map: {:?}", import_map_str)
@@ -481,9 +472,7 @@ impl Inner {
       let import_map_json = if import_map_url.scheme() == "data" {
         get_source_from_data_url(&import_map_url)?.0
       } else {
-        let import_map_path = import_map_url.to_file_path().map_err(|_| {
+        let import_map_path = specifier_to_file_path(&import_map_url)?;
-          anyhow!("Cannot convert \"{}\" into a file path.", import_map_url)
-        })?;
         info!(
           "  Resolved import map: \"{}\"",
           import_map_path.to_string_lossy()
@@ -1024,11 +1013,10 @@ impl Inner {
     };
     let mark = self.performance.mark("formatting", Some(&params));
     let file_path =
-      if let Ok(file_path) = params.text_document.uri.to_file_path() {
+      specifier_to_file_path(&params.text_document.uri).map_err(|err| {
-        file_path
+        error!("{}", err);
-      } else {
+        LspError::invalid_request()
-        PathBuf::from(params.text_document.uri.path())
+      })?;
-      };
 
     let fmt_options = if let Some(fmt_config) = self.maybe_fmt_config.as_ref() {
       fmt_config.options.clone()
@@ -1913,7 +1901,7 @@ impl Inner {
       .config
       .root_uri
       .as_ref()
-      .and_then(|uri| uri.to_file_path().ok());
+      .and_then(|uri| specifier_to_file_path(uri).ok());
     let mut resolved_items = Vec::<CallHierarchyIncomingCall>::new();
     for item in incoming_calls.iter() {
       if let Some(resolved) = item
@@ -1962,7 +1950,7 @@ impl Inner {
       .config
       .root_uri
       .as_ref()
-      .and_then(|uri| uri.to_file_path().ok());
+      .and_then(|uri| specifier_to_file_path(uri).ok());
     let mut resolved_items = Vec::<CallHierarchyOutgoingCall>::new();
     for item in outgoing_calls.iter() {
       if let Some(resolved) = item
@@ -2018,7 +2006,7 @@ impl Inner {
         .config
         .root_uri
         .as_ref()
-        .and_then(|uri| uri.to_file_path().ok());
+        .and_then(|uri| specifier_to_file_path(uri).ok());
       let mut resolved_items = Vec::<CallHierarchyItem>::new();
       match one_or_many {
         tsc::OneOrMany::One(item) => {

cli/lsp/tsc.rs

@@ -16,6 +16,7 @@ use super::text::LineIndex;
 use super::urls::INVALID_SPECIFIER;
 
 use crate::config_file::TsConfig;
+use crate::fs_util::specifier_to_file_path;
 use crate::tsc;
 use crate::tsc::ResolveArgs;
 
@@ -1512,7 +1513,7 @@ impl CallHierarchyItem {
 
     let use_file_name = self.is_source_file_item();
     let maybe_file_path = if uri.scheme() == "file" {
-      uri.to_file_path().ok()
+      specifier_to_file_path(&uri).ok()
     } else {
       None
     };

cli/tests/testdata/fmt/deno.jsonc

@@ -1,8 +1,8 @@
 {
   "fmt": {
     "files": {
-      "include": ["fmt/fmt_with_config/"],
+      "include": ["fmt_with_config/"],
-      "exclude": ["fmt/fmt_with_config/b.ts"]
+      "exclude": ["fmt_with_config/b.ts"]
     },
     "options": {
       "useTabs": true,

cli/tests/testdata/fmt/deno.malformed.jsonc

@@ -1,8 +1,8 @@
 {
   "fmt": {
     "files": {
-      "include": ["fmt/fmt_with_config/"],
+      "include": ["fmt_with_config/"],
-      "exclude": ["fmt/fmt_with_config/b.ts"]
+      "exclude": ["fmt_with_config/b.ts"]
     },
     "dont_know_this_field": {},
     "options": {

cli/tests/testdata/fmt/deno.malformed2.jsonc

@@ -1,8 +1,8 @@
 {
   "fmt": {
     "files": {
-      "include": ["fmt/fmt_with_config/"],
+      "include": ["fmt_with_config/"],
-      "exclude": ["fmt/fmt_with_config/b.ts"],
+      "exclude": ["fmt_with_config/b.ts"],
       "dont_know_this_field": {}
     },
     "options": {

cli/tests/testdata/lint/Deno.jsonc

@@ -1,8 +1,8 @@
 {
   "lint": {
     "files": {
-      "include": ["lint/with_config/"],
+      "include": ["with_config/"],
-      "exclude": ["lint/with_config/b.ts"]
+      "exclude": ["with_config/b.ts"]
     },
     "rules": {
       "tags": ["recommended"],

cli/tests/testdata/lint/Deno.malformed.jsonc

@@ -1,8 +1,8 @@
 {
   "lint": {
     "files": {
-      "include": ["lint/with_config/"],
+      "include": ["with_config/"],
-      "exclude": ["lint/with_config/b.ts"]
+      "exclude": ["with_config/b.ts"]
     },
     "dont_know_this_field": {},
     "rules": {

cli/tests/testdata/lint/Deno.malformed2.jsonc

@@ -1,8 +1,8 @@
 {
   "lint": {
     "files": {
-      "include": ["lint/with_config/"],
+      "include": ["with_config/"],
-      "exclude": ["lint/with_config/b.ts"],
+      "exclude": ["with_config/b.ts"],
       "dont_know_this_field": {}
     },
     "rules": {

cli/tests/testdata/lint/Deno.no_tags.jsonc

@@ -2,10 +2,10 @@
   "lint": {
     "files": {
       "include": [
-        "lint/with_config/"
+        "with_config/"
       ],
       "exclude": [
-        "lint/with_config/b.ts"
+        "with_config/b.ts"
       ]
     },
     "rules": {

cli/tools/fmt.rs

@@ -55,21 +55,11 @@ pub async fn format(
 
   if let Some(fmt_config) = maybe_fmt_config.as_ref() {
     if include_files.is_empty() {
-      include_files = fmt_config
+      include_files = fmt_config.files.include.clone();
-        .files
-        .include
-        .iter()
-        .map(PathBuf::from)
-        .collect::<Vec<PathBuf>>();
     }
 
     if exclude_files.is_empty() {
-      exclude_files = fmt_config
+      exclude_files = fmt_config.files.exclude.clone();
-        .files
-        .exclude
-        .iter()
-        .map(PathBuf::from)
-        .collect::<Vec<PathBuf>>();
     }
   }
 

cli/tools/lint.rs

@@ -71,21 +71,11 @@ pub async fn lint(
 
   if let Some(lint_config) = maybe_lint_config.as_ref() {
     if include_files.is_empty() {
-      include_files = lint_config
+      include_files = lint_config.files.include.clone();
-        .files
-        .include
-        .iter()
-        .map(PathBuf::from)
-        .collect::<Vec<PathBuf>>();
     }
 
     if exclude_files.is_empty() {
-      exclude_files = lint_config
+      exclude_files = lint_config.files.exclude.clone();
-        .files
-        .exclude
-        .iter()
-        .map(PathBuf::from)
-        .collect::<Vec<PathBuf>>();
     }
   }