From d673efebd2fccfc3500f66ebf0c6887b85ca6b47 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Wed, 1 Nov 2023 11:53:00 +0200 Subject: [PATCH] Add prettier workspace resolution test --- Cargo.lock | 2 +- crates/prettier/src/prettier.rs | 396 +++++++++++++++++- crates/project/src/project_tests.rs | 4 +- crates/project/src/search.rs | 26 +- crates/search/Cargo.toml | 1 - crates/search/src/project_search.rs | 4 +- crates/semantic_index/src/db.rs | 4 +- crates/semantic_index/src/semantic_index.rs | 3 +- .../src/semantic_index_tests.rs | 4 +- crates/util/Cargo.toml | 1 + crates/util/src/paths.rs | 26 ++ 11 files changed, 426 insertions(+), 45 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 64bfe4edf8..17d19258e4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7580,7 +7580,6 @@ dependencies = [ "collections", "editor", "futures 0.3.28", - "globset", "gpui", "language", "log", @@ -9887,6 +9886,7 @@ dependencies = [ "dirs 3.0.2", "futures 0.3.28", "git2", + "globset", "isahc", "lazy_static", "log", diff --git a/crates/prettier/src/prettier.rs b/crates/prettier/src/prettier.rs index 7517b4ee43..58de2fc913 100644 --- a/crates/prettier/src/prettier.rs +++ b/crates/prettier/src/prettier.rs @@ -3,7 +3,7 @@ use std::path::{Path, PathBuf}; use std::sync::Arc; use anyhow::Context; -use collections::HashMap; +use collections::{HashMap, HashSet}; use fs::Fs; use gpui::{AsyncAppContext, ModelHandle}; use language::language_settings::language_settings; @@ -11,7 +11,7 @@ use language::{Buffer, Diff}; use lsp::{LanguageServer, LanguageServerId}; use node_runtime::NodeRuntime; use serde::{Deserialize, Serialize}; -use util::paths::DEFAULT_PRETTIER_DIR; +use util::paths::{PathMatcher, DEFAULT_PRETTIER_DIR}; pub enum Prettier { Real(RealPrettier), @@ -63,14 +63,77 @@ impl Prettier { ".editorconfig", ]; + pub async fn locate_prettier_installation( + fs: &dyn Fs, + installed_prettiers: &HashSet, + locate_from: &Path, + ) -> anyhow::Result> { + let mut path_to_check = locate_from + .components() + .take_while(|component| !is_node_modules(component)) + .collect::(); + let mut project_path_with_prettier_dependency = None; + loop { + if installed_prettiers.contains(&path_to_check) { + return Ok(Some(path_to_check)); + } else if let Some(package_json_contents) = + read_package_json(fs, &path_to_check).await? + { + if has_prettier_in_package_json(&package_json_contents) { + if has_prettier_in_node_modules(fs, &path_to_check).await? { + return Ok(Some(path_to_check)); + } else if project_path_with_prettier_dependency.is_none() { + project_path_with_prettier_dependency = Some(path_to_check.clone()); + } + } else { + match package_json_contents.get("workspaces") { + Some(serde_json::Value::Array(workspaces)) => { + match &project_path_with_prettier_dependency { + Some(project_path_with_prettier_dependency) => { + let subproject_path = project_path_with_prettier_dependency.strip_prefix(&path_to_check).expect("traversing path parents, should be able to strip prefix"); + if workspaces.iter().filter_map(|value| { + if let serde_json::Value::String(s) = value { + Some(s.clone()) + } else { + log::warn!("Skipping non-string 'workspaces' value: {value:?}"); + None + } + }).any(|workspace_definition| { + if let Some(path_matcher) = PathMatcher::new(&workspace_definition).ok() { + path_matcher.is_match(subproject_path) + } else { + workspace_definition == subproject_path.to_string_lossy() + } + }) { + return Ok(Some(path_to_check)); + } else { + log::warn!("Skipping path {path_to_check:?} that has prettier in its 'node_modules' subdirectory, but is not included in its package.json workspaces {workspaces:?}"); + } + } + None => { + log::warn!("Skipping path {path_to_check:?} that has prettier in its 'node_modules' subdirectory, but has no prettier in its package.json"); + } + } + }, + Some(unknown) => log::error!("Failed to parse workspaces for {path_to_check:?} from package.json, got {unknown:?}. Skipping."), + None => log::warn!("Skipping path {path_to_check:?} that has no prettier dependency and no workspaces section in its package.json"), + } + } + } + + if !path_to_check.pop() { + match project_path_with_prettier_dependency { + Some(closest_prettier_discovered) => anyhow::bail!("No prettier found in ancestors of {locate_from:?}, but discovered prettier package.json dependency in {closest_prettier_discovered:?}"), + None => return Ok(None), + } + } + } + } + pub async fn locate( starting_path: Option, fs: Arc, ) -> anyhow::Result { - fn is_node_modules(path_component: &std::path::Component<'_>) -> bool { - path_component.as_os_str().to_string_lossy() == "node_modules" - } - let paths_to_check = match starting_path.as_ref() { Some(starting_path) => { let worktree_root = starting_path @@ -106,7 +169,7 @@ impl Prettier { None => Vec::new(), }; - match find_closest_prettier_dir(paths_to_check, fs.as_ref()) + match find_closest_prettier_dir(fs.as_ref(), paths_to_check) .await .with_context(|| format!("finding prettier starting with {starting_path:?}"))? { @@ -350,9 +413,62 @@ impl Prettier { } } -async fn find_closest_prettier_dir( - paths_to_check: Vec, +async fn has_prettier_in_node_modules(fs: &dyn Fs, path: &Path) -> anyhow::Result { + let possible_node_modules_location = path.join("node_modules").join(PRETTIER_PACKAGE_NAME); + if let Some(node_modules_location_metadata) = fs + .metadata(&possible_node_modules_location) + .await + .with_context(|| format!("fetching metadata for {possible_node_modules_location:?}"))? + { + return Ok(node_modules_location_metadata.is_dir); + } + Ok(false) +} + +async fn read_package_json( fs: &dyn Fs, + path: &Path, +) -> anyhow::Result>> { + let possible_package_json = path.join("package.json"); + if let Some(package_json_metadata) = fs + .metadata(&possible_package_json) + .await + .with_context(|| format!("Fetching metadata for {possible_package_json:?}"))? + { + if !package_json_metadata.is_dir && !package_json_metadata.is_symlink { + let package_json_contents = fs + .load(&possible_package_json) + .await + .with_context(|| format!("reading {possible_package_json:?} file contents"))?; + return serde_json::from_str::>( + &package_json_contents, + ) + .map(Some) + .with_context(|| format!("parsing {possible_package_json:?} file contents")); + } + } + Ok(None) +} + +fn has_prettier_in_package_json( + package_json_contents: &HashMap, +) -> bool { + if let Some(serde_json::Value::Object(o)) = package_json_contents.get("dependencies") { + if o.contains_key(PRETTIER_PACKAGE_NAME) { + return true; + } + } + if let Some(serde_json::Value::Object(o)) = package_json_contents.get("devDependencies") { + if o.contains_key(PRETTIER_PACKAGE_NAME) { + return true; + } + } + false +} + +async fn find_closest_prettier_dir( + fs: &dyn Fs, + paths_to_check: Vec, ) -> anyhow::Result> { for path in paths_to_check { let possible_package_json = path.join("package.json"); @@ -436,3 +552,265 @@ impl lsp::request::Request for ClearCache { type Result = (); const METHOD: &'static str = "prettier/clear_cache"; } + +#[cfg(test)] +mod tests { + use fs::FakeFs; + use serde_json::json; + + use super::*; + + #[gpui::test] + async fn test_prettier_lookup_finds_nothing(cx: &mut gpui::TestAppContext) { + let fs = FakeFs::new(cx.background()); + fs.insert_tree( + "/root", + json!({ + ".config": { + "zed": { + "settings.json": r#"{ "formatter": "auto" }"#, + }, + }, + "work": { + "project": { + "src": { + "index.js": "// index.js file contents", + }, + "node_modules": { + "expect": { + "build": { + "print.js": "// print.js file contents", + }, + "package.json": r#"{ + "devDependencies": { + "prettier": "2.5.1" + } + }"#, + }, + "prettier": { + "index.js": "// Dummy prettier package file", + }, + }, + "package.json": r#"{}"# + }, + } + }), + ) + .await; + + assert!( + Prettier::locate_prettier_installation( + fs.as_ref(), + &HashSet::default(), + Path::new("/root/.config/zed/settings.json"), + ) + .await + .unwrap() + .is_none(), + "Should successfully find no prettier for path hierarchy without it" + ); + assert!( + Prettier::locate_prettier_installation( + fs.as_ref(), + &HashSet::default(), + Path::new("/root/work/project/src/index.js") + ) + .await + .unwrap() + .is_none(), + "Should successfully find no prettier for path hierarchy that has node_modules with prettier, but no package.json mentions of it" + ); + assert!( + Prettier::locate_prettier_installation( + fs.as_ref(), + &HashSet::default(), + Path::new("/root/work/project/node_modules/expect/build/print.js") + ) + .await + .unwrap() + .is_none(), + "Even though it has package.json with prettier in it and no prettier on node_modules along the path, nothing should fail since declared inside node_modules" + ); + } + + #[gpui::test] + async fn test_prettier_lookup_in_simple_npm_projects(cx: &mut gpui::TestAppContext) { + let fs = FakeFs::new(cx.background()); + fs.insert_tree( + "/root", + json!({ + "web_blog": { + "node_modules": { + "prettier": { + "index.js": "// Dummy prettier package file", + }, + "expect": { + "build": { + "print.js": "// print.js file contents", + }, + "package.json": r#"{ + "devDependencies": { + "prettier": "2.5.1" + } + }"#, + }, + }, + "pages": { + "[slug].tsx": "// [slug].tsx file contents", + }, + "package.json": r#"{ + "devDependencies": { + "prettier": "2.3.0" + }, + "prettier": { + "semi": false, + "printWidth": 80, + "htmlWhitespaceSensitivity": "strict", + "tabWidth": 4 + } + }"# + } + }), + ) + .await; + + assert_eq!( + Prettier::locate_prettier_installation( + fs.as_ref(), + &HashSet::default(), + Path::new("/root/web_blog/pages/[slug].tsx") + ) + .await + .unwrap(), + Some(PathBuf::from("/root/web_blog")), + "Should find a preinstalled prettier in the project root" + ); + assert_eq!( + Prettier::locate_prettier_installation( + fs.as_ref(), + &HashSet::default(), + Path::new("/root/web_blog/node_modules/expect/build/print.js") + ) + .await + .unwrap(), + Some(PathBuf::from("/root/web_blog")), + "Should find a preinstalled prettier in the project root even for node_modules files" + ); + } + + #[gpui::test] + async fn test_prettier_lookup_for_not_installed(cx: &mut gpui::TestAppContext) { + let fs = FakeFs::new(cx.background()); + fs.insert_tree( + "/root", + json!({ + "work": { + "web_blog": { + "pages": { + "[slug].tsx": "// [slug].tsx file contents", + }, + "package.json": r#"{ + "devDependencies": { + "prettier": "2.3.0" + }, + "prettier": { + "semi": false, + "printWidth": 80, + "htmlWhitespaceSensitivity": "strict", + "tabWidth": 4 + } + }"# + } + } + }), + ) + .await; + + let path = "/root/work/web_blog/node_modules/pages/[slug].tsx"; + match Prettier::locate_prettier_installation( + fs.as_ref(), + &HashSet::default(), + Path::new(path) + ) + .await { + Ok(path) => panic!("Expected to fail for prettier in package.json but not in node_modules found, but got path {path:?}"), + Err(e) => { + let message = e.to_string(); + assert!(message.contains(path), "Error message should mention which start file was used for location"); + assert!(message.contains("/root/work/web_blog"), "Error message should mention potential candidates without prettier node_modules contents"); + }, + }; + + assert_eq!( + Prettier::locate_prettier_installation( + fs.as_ref(), + &HashSet::from_iter( + [PathBuf::from("/root"), PathBuf::from("/root/work")].into_iter() + ), + Path::new("/root/work/web_blog/node_modules/pages/[slug].tsx") + ) + .await + .unwrap(), + Some(PathBuf::from("/root/work")), + "Should return first cached value found without path checks" + ); + } + + #[gpui::test] + async fn test_prettier_lookup_in_npm_workspaces(cx: &mut gpui::TestAppContext) { + let fs = FakeFs::new(cx.background()); + fs.insert_tree( + "/root", + json!({ + "work": { + "full-stack-foundations": { + "exercises": { + "03.loading": { + "01.problem.loader": { + "app": { + "routes": { + "users+": { + "$username_+": { + "notes.tsx": "// notes.tsx file contents", + }, + }, + }, + }, + "node_modules": {}, + "package.json": r#"{ + "devDependencies": { + "prettier": "^3.0.3" + } + }"# + }, + }, + }, + "package.json": r#"{ + "workspaces": ["exercises/*/*", "examples/*"] + }"#, + "node_modules": { + "prettier": { + "index.js": "// Dummy prettier package file", + }, + }, + }, + } + }), + ) + .await; + + assert_eq!( + Prettier::locate_prettier_installation( + fs.as_ref(), + &HashSet::default(), + Path::new("/root/work/full-stack-foundations/exercises/03.loading/01.problem.loader/app/routes/users+/$username_+/notes.tsx"), + ).await.unwrap(), + Some(PathBuf::from("/root/work/full-stack-foundations")), + "Should ascend to the multi-workspace root and find the prettier there", + ); + } +} + +fn is_node_modules(path_component: &std::path::Component<'_>) -> bool { + path_component.as_os_str().to_string_lossy() == "node_modules" +} diff --git a/crates/project/src/project_tests.rs b/crates/project/src/project_tests.rs index d5ff1e08d7..32dc542c20 100644 --- a/crates/project/src/project_tests.rs +++ b/crates/project/src/project_tests.rs @@ -1,4 +1,4 @@ -use crate::{search::PathMatcher, worktree::WorktreeModelHandle, Event, *}; +use crate::{worktree::WorktreeModelHandle, Event, *}; use fs::{FakeFs, RealFs}; use futures::{future, StreamExt}; use gpui::{executor::Deterministic, test::subscribe, AppContext}; @@ -13,7 +13,7 @@ use pretty_assertions::assert_eq; use serde_json::json; use std::{cell::RefCell, os::unix, rc::Rc, task::Poll}; use unindent::Unindent as _; -use util::{assert_set_eq, test::temp_tree}; +use util::{assert_set_eq, test::temp_tree, paths::PathMatcher}; #[cfg(test)] #[ctor::ctor] diff --git a/crates/project/src/search.rs b/crates/project/src/search.rs index 46dd30c8a0..f626f15d12 100644 --- a/crates/project/src/search.rs +++ b/crates/project/src/search.rs @@ -13,6 +13,7 @@ use std::{ path::{Path, PathBuf}, sync::Arc, }; +use util::paths::PathMatcher; #[derive(Clone, Debug)] pub struct SearchInputs { @@ -52,31 +53,6 @@ pub enum SearchQuery { }, } -#[derive(Clone, Debug)] -pub struct PathMatcher { - maybe_path: PathBuf, - glob: GlobMatcher, -} - -impl std::fmt::Display for PathMatcher { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - self.maybe_path.to_string_lossy().fmt(f) - } -} - -impl PathMatcher { - pub fn new(maybe_glob: &str) -> Result { - Ok(PathMatcher { - glob: Glob::new(&maybe_glob)?.compile_matcher(), - maybe_path: PathBuf::from(maybe_glob), - }) - } - - pub fn is_match>(&self, other: P) -> bool { - other.as_ref().starts_with(&self.maybe_path) || self.glob.is_match(other) - } -} - impl SearchQuery { pub fn text( query: impl ToString, diff --git a/crates/search/Cargo.toml b/crates/search/Cargo.toml index 64421f5431..4ebd31a2bc 100644 --- a/crates/search/Cargo.toml +++ b/crates/search/Cargo.toml @@ -29,7 +29,6 @@ serde.workspace = true serde_derive.workspace = true smallvec.workspace = true smol.workspace = true -globset.workspace = true serde_json.workspace = true [dev-dependencies] client = { path = "../client", features = ["test-support"] } diff --git a/crates/search/src/project_search.rs b/crates/search/src/project_search.rs index 55e3f6babd..f6e17bbee5 100644 --- a/crates/search/src/project_search.rs +++ b/crates/search/src/project_search.rs @@ -22,7 +22,7 @@ use gpui::{ }; use menu::Confirm; use project::{ - search::{PathMatcher, SearchInputs, SearchQuery}, + search::{SearchInputs, SearchQuery}, Entry, Project, }; use semantic_index::{SemanticIndex, SemanticIndexStatus}; @@ -37,7 +37,7 @@ use std::{ sync::Arc, time::{Duration, Instant}, }; -use util::ResultExt as _; +use util::{paths::PathMatcher, ResultExt as _}; use workspace::{ item::{BreadcrumbText, Item, ItemEvent, ItemHandle}, searchable::{Direction, SearchableItem, SearchableItemHandle}, diff --git a/crates/semantic_index/src/db.rs b/crates/semantic_index/src/db.rs index 63527cea1c..5b416f7a64 100644 --- a/crates/semantic_index/src/db.rs +++ b/crates/semantic_index/src/db.rs @@ -9,7 +9,7 @@ use futures::channel::oneshot; use gpui::executor; use ndarray::{Array1, Array2}; use ordered_float::OrderedFloat; -use project::{search::PathMatcher, Fs}; +use project::Fs; use rpc::proto::Timestamp; use rusqlite::params; use rusqlite::types::Value; @@ -21,7 +21,7 @@ use std::{ sync::Arc, time::SystemTime, }; -use util::TryFutureExt; +use util::{paths::PathMatcher, TryFutureExt}; pub fn argsort(data: &[T]) -> Vec { let mut indices = (0..data.len()).collect::>(); diff --git a/crates/semantic_index/src/semantic_index.rs b/crates/semantic_index/src/semantic_index.rs index 818faa0444..7d1eacd7fa 100644 --- a/crates/semantic_index/src/semantic_index.rs +++ b/crates/semantic_index/src/semantic_index.rs @@ -21,7 +21,7 @@ use ordered_float::OrderedFloat; use parking_lot::Mutex; use parsing::{CodeContextRetriever, Span, SpanDigest, PARSEABLE_ENTIRE_FILE_TYPES}; use postage::watch; -use project::{search::PathMatcher, Fs, PathChange, Project, ProjectEntryId, Worktree, WorktreeId}; +use project::{Fs, PathChange, Project, ProjectEntryId, Worktree, WorktreeId}; use smol::channel; use std::{ cmp::Reverse, @@ -33,6 +33,7 @@ use std::{ sync::{Arc, Weak}, time::{Duration, Instant, SystemTime}, }; +use util::paths::PathMatcher; use util::{channel::RELEASE_CHANNEL_NAME, http::HttpClient, paths::EMBEDDINGS_DIR, ResultExt}; use workspace::WorkspaceCreated; diff --git a/crates/semantic_index/src/semantic_index_tests.rs b/crates/semantic_index/src/semantic_index_tests.rs index 7a91d1e100..044ded2682 100644 --- a/crates/semantic_index/src/semantic_index_tests.rs +++ b/crates/semantic_index/src/semantic_index_tests.rs @@ -10,13 +10,13 @@ use gpui::{executor::Deterministic, Task, TestAppContext}; use language::{Language, LanguageConfig, LanguageRegistry, ToOffset}; use parking_lot::Mutex; use pretty_assertions::assert_eq; -use project::{project_settings::ProjectSettings, search::PathMatcher, FakeFs, Fs, Project}; +use project::{project_settings::ProjectSettings, FakeFs, Fs, Project}; use rand::{rngs::StdRng, Rng}; use serde_json::json; use settings::SettingsStore; use std::{path::Path, sync::Arc, time::SystemTime}; use unindent::Unindent; -use util::RandomCharIter; +use util::{paths::PathMatcher, RandomCharIter}; #[ctor::ctor] fn init_logger() { diff --git a/crates/util/Cargo.toml b/crates/util/Cargo.toml index 6ab76b0850..cfbd7551f9 100644 --- a/crates/util/Cargo.toml +++ b/crates/util/Cargo.toml @@ -14,6 +14,7 @@ test-support = ["tempdir", "git2"] [dependencies] anyhow.workspace = true backtrace = "0.3" +globset.workspace = true log.workspace = true lazy_static.workspace = true futures.workspace = true diff --git a/crates/util/src/paths.rs b/crates/util/src/paths.rs index 96d77236a9..d54e0b1cd6 100644 --- a/crates/util/src/paths.rs +++ b/crates/util/src/paths.rs @@ -1,5 +1,6 @@ use std::path::{Path, PathBuf}; +use globset::{Glob, GlobMatcher}; use serde::{Deserialize, Serialize}; lazy_static::lazy_static! { @@ -189,6 +190,31 @@ impl

PathLikeWithPosition

{ } } +#[derive(Clone, Debug)] +pub struct PathMatcher { + maybe_path: PathBuf, + glob: GlobMatcher, +} + +impl std::fmt::Display for PathMatcher { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.maybe_path.to_string_lossy().fmt(f) + } +} + +impl PathMatcher { + pub fn new(maybe_glob: &str) -> Result { + Ok(PathMatcher { + glob: Glob::new(&maybe_glob)?.compile_matcher(), + maybe_path: PathBuf::from(maybe_glob), + }) + } + + pub fn is_match>(&self, other: P) -> bool { + other.as_ref().starts_with(&self.maybe_path) || self.glob.is_match(other) + } +} + #[cfg(test)] mod tests { use super::*;