From 70b970b2c8b2af8b946229a865808e5df109ff43 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Mon, 16 Dec 2024 17:24:00 -0500 Subject: [PATCH 01/23] Basic first pass at boundaries --- Cargo.lock | 8 +- Cargo.toml | 1 + crates/turbo-trace/Cargo.toml | 2 +- crates/turbo-trace/src/import_finder.rs | 6 + crates/turbo-trace/src/lib.rs | 1 + crates/turbo-trace/src/tracer.rs | 8 +- crates/turborepo-lib/Cargo.toml | 2 + crates/turborepo-lib/src/boundaries.rs | 191 ++++++++++++++++++ crates/turborepo-lib/src/cli/error.rs | 2 + crates/turborepo-lib/src/cli/mod.rs | 14 +- .../turborepo-lib/src/commands/boundaries.rs | 22 ++ crates/turborepo-lib/src/commands/mod.rs | 1 + crates/turborepo-lib/src/lib.rs | 1 + crates/turborepo-lib/src/run/summary/page.rs | 62 ++++++ 14 files changed, 311 insertions(+), 10 deletions(-) create mode 100644 crates/turborepo-lib/src/boundaries.rs create mode 100644 crates/turborepo-lib/src/commands/boundaries.rs create mode 100644 crates/turborepo-lib/src/run/summary/page.rs diff --git a/Cargo.lock b/Cargo.lock index 7a72650696612..69ec602dfc040 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3617,9 +3617,9 @@ dependencies = [ [[package]] name = "oxc_resolver" -version = "2.1.0" +version = "3.0.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ff67bf7c8cb5f4ea7537faa849892d8e13a408d50d0903aa40889fd395c66993" +checksum = "bed381b6ab4bbfebfc7a011ad43b110ace8d201d02a39c0e09855f16b8f3f741" dependencies = [ "cfg-if", "dashmap 6.1.0", @@ -6116,7 +6116,7 @@ dependencies = [ "futures", "globwalk", "miette", - "oxc_resolver 2.1.0", + "oxc_resolver 3.0.3", "swc_common", "swc_ecma_ast", "swc_ecma_parser", @@ -6426,6 +6426,7 @@ dependencies = [ "notify", "num_cpus", "owo-colors", + "oxc_resolver 3.0.3", "path-clean", "petgraph", "pidlock", @@ -6450,6 +6451,7 @@ dependencies = [ "swc_common", "swc_ecma_ast", "swc_ecma_parser", + "swc_ecma_visit", "sysinfo", "tabwriter", "tempfile", diff --git a/Cargo.toml b/Cargo.toml index 0467dbb11a642..8682a60d66d34 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -117,6 +117,7 @@ notify = "6.1.1" num_cpus = "1.15.0" once_cell = "1.17.1" owo-colors = "3.5.0" +oxc_resolver = { version = "3.0.3" } parking_lot = "0.12.1" path-clean = "1.0.1" pathdiff = "0.2.1" diff --git a/crates/turbo-trace/Cargo.toml b/crates/turbo-trace/Cargo.toml index 9d710c271d0b7..6cde76bece2f8 100644 --- a/crates/turbo-trace/Cargo.toml +++ b/crates/turbo-trace/Cargo.toml @@ -10,7 +10,7 @@ clap = { version = "4.5.17", features = ["derive"] } futures = { workspace = true } globwalk = { version = "0.1.0", path = "../turborepo-globwalk" } miette = { workspace = true, features = ["fancy"] } -oxc_resolver = { version = "2.1.0" } +oxc_resolver = { workspace = true } swc_common = { workspace = true, features = ["concurrent", "tty-emitter"] } swc_ecma_ast = { workspace = true } swc_ecma_parser = { workspace = true } diff --git a/crates/turbo-trace/src/import_finder.rs b/crates/turbo-trace/src/import_finder.rs index 79543c7185417..587e143445a48 100644 --- a/crates/turbo-trace/src/import_finder.rs +++ b/crates/turbo-trace/src/import_finder.rs @@ -9,6 +9,12 @@ pub struct ImportFinder { imports: Vec<(String, Span)>, } +impl Default for ImportFinder { + fn default() -> Self { + Self::new(ImportType::All) + } +} + impl ImportFinder { pub fn new(import_type: ImportType) -> Self { Self { diff --git a/crates/turbo-trace/src/lib.rs b/crates/turbo-trace/src/lib.rs index e0b38cd1bd02b..f0e469078ce25 100644 --- a/crates/turbo-trace/src/lib.rs +++ b/crates/turbo-trace/src/lib.rs @@ -2,4 +2,5 @@ mod import_finder; mod tracer; +pub use import_finder::ImportFinder; pub use tracer::{ImportType, TraceError, TraceResult, Tracer}; diff --git a/crates/turbo-trace/src/tracer.rs b/crates/turbo-trace/src/tracer.rs index 9a15c22996f68..8e3ed79086154 100644 --- a/crates/turbo-trace/src/tracer.rs +++ b/crates/turbo-trace/src/tracer.rs @@ -344,7 +344,7 @@ impl Tracer { } } - pub fn create_resolver(&mut self) -> Resolver { + pub fn create_resolver(ts_config: Option) -> Resolver { let mut options = ResolveOptions::default() .with_builtin_modules(true) .with_force_extension(EnforceExtension::Disabled) @@ -362,7 +362,7 @@ impl Tracer { // We add a bunch so oxc_resolver can resolve all kinds of imports. .with_condition_names(&["import", "require", "node", "types", "default"]); - if let Some(ts_config) = self.ts_config.take() { + if let Some(ts_config) = ts_config { options.tsconfig = Some(TsconfigOptions { config_file: ts_config.into(), references: TsconfigReferences::Auto, @@ -374,7 +374,7 @@ impl Tracer { pub async fn trace(mut self, max_depth: Option) -> TraceResult { let mut seen: HashMap = HashMap::new(); - let resolver = self.create_resolver(); + let resolver = Self::create_resolver(self.ts_config.take()); while let Some((file_path, file_depth)) = self.files.pop() { if let Some(max_depth) = max_depth { @@ -420,7 +420,7 @@ impl Tracer { let mut futures = JoinSet::new(); - let resolver = Arc::new(self.create_resolver()); + let resolver = Arc::new(Self::create_resolver(self.ts_config.take())); let source_map = self.source_map.clone(); let shared_self = Arc::new(self); diff --git a/crates/turborepo-lib/Cargo.toml b/crates/turborepo-lib/Cargo.toml index dd7d7a55470ed..07e7d422ce385 100644 --- a/crates/turborepo-lib/Cargo.toml +++ b/crates/turborepo-lib/Cargo.toml @@ -78,6 +78,7 @@ nix = "0.26.2" notify = { workspace = true } num_cpus = { workspace = true } owo-colors = { workspace = true } +oxc_resolver = { workspace = true } path-clean = "1.0.1" petgraph = { workspace = true } pidlock = { path = "../turborepo-pidlock" } @@ -103,6 +104,7 @@ svix-ksuid = { version = "0.7.0", features = ["serde"] } swc_common = { workspace = true } swc_ecma_ast = { workspace = true, features = ["serde-impl"] } swc_ecma_parser = { workspace = true } +swc_ecma_visit = { workspace = true } sysinfo = "0.27.7" tabwriter = "1.3.0" thiserror = "1.0.38" diff --git a/crates/turborepo-lib/src/boundaries.rs b/crates/turborepo-lib/src/boundaries.rs new file mode 100644 index 0000000000000..7950de03614f1 --- /dev/null +++ b/crates/turborepo-lib/src/boundaries.rs @@ -0,0 +1,191 @@ +use std::collections::HashSet; + +use itertools::Itertools; +use miette::{Diagnostic, NamedSource, Report, SourceSpan}; +use oxc_resolver::{ResolveError, Resolver}; +use swc_common::{comments::SingleThreadedComments, input::StringInput, FileName, SourceMap}; +use swc_ecma_ast::EsVersion; +use swc_ecma_parser::{lexer::Lexer, Capturing, EsSyntax, Parser, Syntax, TsSyntax}; +use swc_ecma_visit::VisitWith; +use thiserror::Error; +use turbo_trace::{ImportFinder, Tracer}; +use turbopath::{AbsoluteSystemPath, AbsoluteSystemPathBuf}; +use turborepo_repository::package_graph::{PackageName, PackageNode}; + +use crate::run::Run; + +#[derive(Debug, Error, Diagnostic)] +pub enum Error { + #[error("cannot import package `{name}` because it is not a dependency")] + PackageNotFound { + name: String, + #[label("package imported here")] + span: SourceSpan, + #[source_code] + text: NamedSource, + }, + #[error(transparent)] + GlobWalk(#[from] globwalk::WalkError), + #[error("failed to read file: {0}")] + FileNotFound(AbsoluteSystemPathBuf), + #[error("failed to parse file {0}")] + ParseError(AbsoluteSystemPathBuf, swc_ecma_parser::error::Error), +} + +impl Run { + pub async fn check_boundaries(&self) -> Result<(), Error> { + let packages = self.pkg_dep_graph().packages(); + for (package_name, package_info) in packages { + let package_root = self.repo_root().resolve(package_info.package_path()); + let dependencies = self + .pkg_dep_graph() + .immediate_dependencies(&PackageNode::Workspace(package_name.to_owned())) + .unwrap_or_default(); + self.check_package(&package_root, dependencies).await?; + } + Ok(()) + } + async fn check_package( + &self, + package_root: &AbsoluteSystemPath, + dependencies: HashSet<&PackageNode>, + ) -> Result<(), Error> { + let files = globwalk::globwalk( + package_root, + &[ + "**/*.js".parse().unwrap(), + "**/*.jsx".parse().unwrap(), + "**/*.ts".parse().unwrap(), + "**/*.tsx".parse().unwrap(), + ], + &["**/node_modules/**".parse().unwrap()], + globwalk::WalkType::Files, + )?; + + let source_map = SourceMap::default(); + let mut errors = Vec::new(); + // TODO: Load tsconfig.json + let resolver = Tracer::create_resolver(None); + + for file_path in files { + // Read the file content + let Ok(file_content) = tokio::fs::read_to_string(&file_path).await else { + errors.push(Error::FileNotFound(file_path.to_owned())); + continue; + }; + + let comments = SingleThreadedComments::default(); + + let source_file = source_map.new_source_file( + FileName::Custom(file_path.to_string()).into(), + file_content.clone(), + ); + + let syntax = if matches!(file_path.extension(), Some("ts") | Some("tsx")) { + Syntax::Typescript(TsSyntax { + tsx: file_path.extension() == Some("tsx"), + decorators: true, + ..Default::default() + }) + } else { + Syntax::Es(EsSyntax { + jsx: true, + ..Default::default() + }) + }; + + let lexer = Lexer::new( + syntax, + EsVersion::EsNext, + StringInput::from(&*source_file), + Some(&comments), + ); + + let mut parser = Parser::new_from(Capturing::new(lexer)); + + // Parse the file as a module + let module = match parser.parse_module() { + Ok(module) => module, + Err(err) => { + errors.push(Error::ParseError(file_path.to_owned(), err)); + continue; + } + }; + + // Visit the AST and find imports + let mut finder = ImportFinder::default(); + module.visit_with(&mut finder); + for (import, span) in finder.imports() { + let (start, end) = source_map.span_to_char_offset(&source_file, *span); + let start = start as usize; + let end = end as usize; + let span = SourceSpan::new(start.into(), (end - start).into()); + + // We have a file import + let check_result = if import.starts_with(".") { + self.check_file_import(&import) + } else { + self.check_package_import( + &import, + span, + &file_path, + &file_content, + &dependencies, + &resolver, + ) + }; + + if let Err(err) = check_result { + errors.push(err); + } + } + } + + for error in errors { + println!("{:?}", Report::new(error)); + } + + Ok(()) + } + + fn check_file_import(&self, import: &str) -> Result<(), Error> { + Ok(()) + } + + fn check_package_import( + &self, + import: &str, + span: SourceSpan, + file_path: &AbsoluteSystemPath, + file_content: &str, + dependencies: &HashSet<&PackageNode>, + resolver: &Resolver, + ) -> Result<(), Error> { + let package_name = if import.starts_with("@") { + import.split('/').take(2).join("/") + } else { + import + .split_once("/") + .map(|(import, _)| import) + .unwrap_or(import) + .to_string() + }; + let package_name = PackageNode::Workspace(PackageName::Other(package_name)); + let folder = file_path.parent().expect("file_path should have a parent"); + + if !dependencies.contains(&package_name) + && !matches!( + resolver.resolve(folder, import), + Err(ResolveError::Builtin { .. }) + ) + { + return Err(Error::PackageNotFound { + name: package_name.to_string(), + span, + text: NamedSource::new(file_path.as_str(), file_content.to_string()), + }); + } + + Ok(()) + } +} diff --git a/crates/turborepo-lib/src/cli/error.rs b/crates/turborepo-lib/src/cli/error.rs index 99785fd5371fd..4247804c511a2 100644 --- a/crates/turborepo-lib/src/cli/error.rs +++ b/crates/turborepo-lib/src/cli/error.rs @@ -24,6 +24,8 @@ pub enum Error { #[error("{0}")] Bin(#[from] bin::Error, #[backtrace] backtrace::Backtrace), #[error(transparent)] + Boundaries(#[from] crate::boundaries::Error), + #[error(transparent)] Path(#[from] turbopath::PathError), #[error(transparent)] #[diagnostic(transparent)] diff --git a/crates/turborepo-lib/src/cli/mod.rs b/crates/turborepo-lib/src/cli/mod.rs index 2c2831dab588d..fecc0125cf444 100644 --- a/crates/turborepo-lib/src/cli/mod.rs +++ b/crates/turborepo-lib/src/cli/mod.rs @@ -28,8 +28,8 @@ use turborepo_ui::{ColorConfig, GREY}; use crate::{ cli::error::print_potential_tasks, commands::{ - bin, config, daemon, generate, info, link, login, logout, ls, prune, query, run, scan, - telemetry, unlink, CommandBase, + bin, boundaries, config, daemon, generate, info, link, login, logout, ls, prune, query, + run, scan, telemetry, unlink, CommandBase, }, get_version, run::watch::WatchClient, @@ -552,6 +552,7 @@ impl Args { pub enum Command { /// Get the path to the Turbo binary Bin, + Boundaries, /// Generate the autocompletion script for the specified shell Completion { shell: Shell, @@ -1271,6 +1272,15 @@ pub async fn run( Ok(0) } + Command::Boundaries => { + let event = CommandEventBuilder::new("boundaries").with_parent(&root_telemetry); + + event.track_call(); + let base = CommandBase::new(cli_args.clone(), repo_root, version, color_config)?; + + boundaries::run(base, event).await?; + Ok(0) + } #[allow(unused_variables)] Command::Daemon { command, idle_time } => { let event = CommandEventBuilder::new("daemon").with_parent(&root_telemetry); diff --git a/crates/turborepo-lib/src/commands/boundaries.rs b/crates/turborepo-lib/src/commands/boundaries.rs new file mode 100644 index 0000000000000..53e325edacbdf --- /dev/null +++ b/crates/turborepo-lib/src/commands/boundaries.rs @@ -0,0 +1,22 @@ +use turborepo_telemetry::events::command::CommandEventBuilder; + +use crate::{ + cli, + commands::{run::get_signal, CommandBase}, + run::builder::RunBuilder, + signal::SignalHandler, +}; + +pub async fn run(base: CommandBase, telemetry: CommandEventBuilder) -> Result<(), cli::Error> { + let signal = get_signal()?; + let handler = SignalHandler::new(signal); + + let run = RunBuilder::new(base)? + .do_not_validate_engine() + .build(&handler, telemetry) + .await?; + + run.check_boundaries().await?; + + Ok(()) +} diff --git a/crates/turborepo-lib/src/commands/mod.rs b/crates/turborepo-lib/src/commands/mod.rs index eebe1bfdd55c5..4acee04c53f0b 100644 --- a/crates/turborepo-lib/src/commands/mod.rs +++ b/crates/turborepo-lib/src/commands/mod.rs @@ -14,6 +14,7 @@ use crate::{ }; pub(crate) mod bin; +pub(crate) mod boundaries; pub(crate) mod config; pub(crate) mod daemon; pub(crate) mod generate; diff --git a/crates/turborepo-lib/src/lib.rs b/crates/turborepo-lib/src/lib.rs index f27e511c404d3..17448af06168b 100644 --- a/crates/turborepo-lib/src/lib.rs +++ b/crates/turborepo-lib/src/lib.rs @@ -20,6 +20,7 @@ mod daemon; mod diagnostics; mod engine; +mod boundaries; mod framework; mod gitignore; pub(crate) mod globwatcher; diff --git a/crates/turborepo-lib/src/run/summary/page.rs b/crates/turborepo-lib/src/run/summary/page.rs new file mode 100644 index 0000000000000..ec89617b2886b --- /dev/null +++ b/crates/turborepo-lib/src/run/summary/page.rs @@ -0,0 +1,62 @@ +use maud::{html, Markup, DOCTYPE}; +use tailwind_css::TailwindBuilder; + +use crate::run::summary::{execution::ExecutionSummary, Error, RunSummary}; + +impl<'a> RunSummary<'a> { + pub fn render_html(&self) -> Result { + let mut tailwind = TailwindBuilder::default(); + let body = html! { + body class=(trace(&mut tailwind, "flex flex-col")?) { + h3 class=(trace(&mut tailwind, "text-md text-gray-200")?) { "turbo " (self.turbo_version) } + (self.execution + .as_ref() + .map(|e| e.render_html(&mut tailwind, self.packages.len())) + .transpose()?.unwrap_or_default()) + } + }; + + Ok(html! { + html lang="en" { + (DOCTYPE) + style { + (tailwind.bundle()?) + } + head { + meta charset="utf-8"; + meta name="viewport" content="width=device-width, initial-scale=1.0"; + title { "Turborepo" } + } + (body) + } + } + .into_string()) + } +} + +fn trace(tailwind: &mut TailwindBuilder, class: &'static str) -> Result<&'static str, Error> { + tailwind.trace(class, false)?; + Ok(class) +} + +impl<'a> ExecutionSummary<'a> { + pub fn render_html( + &self, + tailwind: &mut TailwindBuilder, + packages: usize, + ) -> Result { + Ok(html! { + div { + h1 class=(trace(tailwind, "text-2xl")?) { + "Ran " + code { + (self.command) + } + " in " + (packages) + " packages" + } + } + }) + } +} From bc36549e35e25d6a71640a20830847326ea62ffd Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Tue, 17 Dec 2024 13:17:36 -0500 Subject: [PATCH 02/23] Filter by git status --- Cargo.lock | 11 ++++++----- Cargo.toml | 1 + crates/turborepo-filewatch/Cargo.toml | 2 +- crates/turborepo-lib/Cargo.toml | 1 + crates/turborepo-lib/src/boundaries.rs | 13 ++++++++++++- crates/turborepo-scm/Cargo.toml | 2 +- 6 files changed, 22 insertions(+), 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 69ec602dfc040..4ce54e4c94904 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2076,11 +2076,11 @@ checksum = "b6c80984affa11d98d1b88b66ac8853f143217b399d3c74116778ff8fdb4ed2e" [[package]] name = "git2" -version = "0.16.1" +version = "0.19.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ccf7f68c2995f392c49fffb4f95ae2c873297830eb25c6bc4c114ce8f4562acc" +checksum = "b903b73e45dc0c6c596f2d37eccece7c1c8bb6e4407b001096387c63d0d93724" dependencies = [ - "bitflags 1.3.2", + "bitflags 2.5.0", "libc", "libgit2-sys", "log", @@ -2909,9 +2909,9 @@ checksum = "d8adc4bb1803a324070e64a98ae98f38934d91957a99cfb3a43dcbc01bc56439" [[package]] name = "libgit2-sys" -version = "0.14.2+1.5.1" +version = "0.17.0+1.8.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7f3d95f6b51075fe9810a7ae22c7095f12b98005ab364d8544797a825ce946a4" +checksum = "10472326a8a6477c3c20a64547b0059e4b0d086869eee31e6d7da728a8eb7224" dependencies = [ "cc", "libc", @@ -6407,6 +6407,7 @@ dependencies = [ "either", "futures", "futures-core", + "git2", "globwalk", "globwatch", "go-parse-duration", diff --git a/Cargo.toml b/Cargo.toml index 8682a60d66d34..93b1480790bd3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -103,6 +103,7 @@ dunce = "1.0.3" either = "1.9.0" futures = "0.3.26" futures-retry = "0.6.0" +git2 = { version = "0.19.0", default-features = false } hex = "0.4.3" httpmock = { version = "0.6.8", default-features = false } indexmap = "1.9.2" diff --git a/crates/turborepo-filewatch/Cargo.toml b/crates/turborepo-filewatch/Cargo.toml index 5c908ad2a31db..3002513ea49b1 100644 --- a/crates/turborepo-filewatch/Cargo.toml +++ b/crates/turborepo-filewatch/Cargo.toml @@ -36,7 +36,7 @@ version = "1.0.4" version = "0.2.4" [dev-dependencies] -git2 = { version = "0.16.1", default-features = false } +git2 = { version = "0.19.0", default-features = false } tempfile = { workspace = true } tokio-scoped = "0.2.0" tracing-test = "0.2.4" diff --git a/crates/turborepo-lib/Cargo.toml b/crates/turborepo-lib/Cargo.toml index 07e7d422ce385..5e7efa28fe6e4 100644 --- a/crates/turborepo-lib/Cargo.toml +++ b/crates/turborepo-lib/Cargo.toml @@ -60,6 +60,7 @@ dunce = { workspace = true } either = { workspace = true } futures = "0.3.30" futures-core = "0.3.30" +git2 = { workspace = true, default-features = false } globwalk = { version = "0.1.0", path = "../turborepo-globwalk" } globwatch = { path = "../turborepo-globwatch" } go-parse-duration = "0.1.1" diff --git a/crates/turborepo-lib/src/boundaries.rs b/crates/turborepo-lib/src/boundaries.rs index 7950de03614f1..fdb697e6e074d 100644 --- a/crates/turborepo-lib/src/boundaries.rs +++ b/crates/turborepo-lib/src/boundaries.rs @@ -1,5 +1,6 @@ use std::collections::HashSet; +use git2::Repository; use itertools::Itertools; use miette::{Diagnostic, NamedSource, Report, SourceSpan}; use oxc_resolver::{ResolveError, Resolver}; @@ -35,18 +36,21 @@ pub enum Error { impl Run { pub async fn check_boundaries(&self) -> Result<(), Error> { let packages = self.pkg_dep_graph().packages(); + let repo = Repository::discover(&self.repo_root()).ok(); for (package_name, package_info) in packages { let package_root = self.repo_root().resolve(package_info.package_path()); let dependencies = self .pkg_dep_graph() .immediate_dependencies(&PackageNode::Workspace(package_name.to_owned())) .unwrap_or_default(); - self.check_package(&package_root, dependencies).await?; + self.check_package(&repo, &package_root, dependencies) + .await?; } Ok(()) } async fn check_package( &self, + repo: &Option, package_root: &AbsoluteSystemPath, dependencies: HashSet<&PackageNode>, ) -> Result<(), Error> { @@ -57,6 +61,8 @@ impl Run { "**/*.jsx".parse().unwrap(), "**/*.ts".parse().unwrap(), "**/*.tsx".parse().unwrap(), + "**/*.vue".parse().unwrap(), + "**/*.svelte".parse().unwrap(), ], &["**/node_modules/**".parse().unwrap()], globwalk::WalkType::Files, @@ -68,6 +74,11 @@ impl Run { let resolver = Tracer::create_resolver(None); for file_path in files { + if let Some(repo) = repo { + if matches!(repo.status_should_ignore(file_path.as_std_path()), Ok(true)) { + continue; + } + }; // Read the file content let Ok(file_content) = tokio::fs::read_to_string(&file_path).await else { errors.push(Error::FileNotFound(file_path.to_owned())); diff --git a/crates/turborepo-scm/Cargo.toml b/crates/turborepo-scm/Cargo.toml index 4be21216a1409..dd854ee236c58 100644 --- a/crates/turborepo-scm/Cargo.toml +++ b/crates/turborepo-scm/Cargo.toml @@ -11,7 +11,7 @@ workspace = true [dependencies] bstr = "1.4.0" -git2 = { version = "0.16.1", default-features = false } +git2 = { workspace = true, default-features = false } globwalk = { path = "../turborepo-globwalk" } hex = { workspace = true } ignore = "0.4.20" From ffd7c03a31b730d43e3610da44edbcfd8d9d8499 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Thu, 19 Dec 2024 12:04:40 -0500 Subject: [PATCH 03/23] Add external dependencies --- crates/turborepo-lib/src/boundaries.rs | 43 ++++++++++++++++++++------ 1 file changed, 33 insertions(+), 10 deletions(-) diff --git a/crates/turborepo-lib/src/boundaries.rs b/crates/turborepo-lib/src/boundaries.rs index fdb697e6e074d..dbc1b10259864 100644 --- a/crates/turborepo-lib/src/boundaries.rs +++ b/crates/turborepo-lib/src/boundaries.rs @@ -1,4 +1,4 @@ -use std::collections::HashSet; +use std::collections::{BTreeMap, HashSet}; use git2::Repository; use itertools::Itertools; @@ -37,14 +37,27 @@ impl Run { pub async fn check_boundaries(&self) -> Result<(), Error> { let packages = self.pkg_dep_graph().packages(); let repo = Repository::discover(&self.repo_root()).ok(); + println!( + "{:?}", + self.pkg_dep_graph() + .package_info(&PackageName::Other("@turbo/gen".to_string())) + .unwrap() + .unresolved_external_dependencies + ); for (package_name, package_info) in packages { let package_root = self.repo_root().resolve(package_info.package_path()); - let dependencies = self + let internal_dependencies = self .pkg_dep_graph() .immediate_dependencies(&PackageNode::Workspace(package_name.to_owned())) .unwrap_or_default(); - self.check_package(&repo, &package_root, dependencies) - .await?; + let external_dependencies = package_info.unresolved_external_dependencies.as_ref(); + self.check_package( + &repo, + &package_root, + internal_dependencies, + external_dependencies, + ) + .await?; } Ok(()) } @@ -52,7 +65,8 @@ impl Run { &self, repo: &Option, package_root: &AbsoluteSystemPath, - dependencies: HashSet<&PackageNode>, + internal_dependencies: HashSet<&PackageNode>, + external_dependencies: Option<&BTreeMap>, ) -> Result<(), Error> { let files = globwalk::globwalk( package_root, @@ -64,12 +78,15 @@ impl Run { "**/*.vue".parse().unwrap(), "**/*.svelte".parse().unwrap(), ], - &["**/node_modules/**".parse().unwrap()], + &[ + "**/node_modules/**".parse().unwrap(), + "**/examples/**".parse().unwrap(), + ], globwalk::WalkType::Files, )?; let source_map = SourceMap::default(); - let mut errors = Vec::new(); + let mut errors: Vec = Vec::new(); // TODO: Load tsconfig.json let resolver = Tracer::create_resolver(None); @@ -141,7 +158,8 @@ impl Run { span, &file_path, &file_content, - &dependencies, + &internal_dependencies, + external_dependencies, &resolver, ) }; @@ -169,7 +187,8 @@ impl Run { span: SourceSpan, file_path: &AbsoluteSystemPath, file_content: &str, - dependencies: &HashSet<&PackageNode>, + internal_dependencies: &HashSet<&PackageNode>, + external_dependencies: Option<&BTreeMap>, resolver: &Resolver, ) -> Result<(), Error> { let package_name = if import.starts_with("@") { @@ -183,8 +202,12 @@ impl Run { }; let package_name = PackageNode::Workspace(PackageName::Other(package_name)); let folder = file_path.parent().expect("file_path should have a parent"); + let contained_in_dependencies = internal_dependencies.contains(&package_name) + || external_dependencies.map_or(false, |external_dependencies| { + external_dependencies.contains_key(&package_name.to_string()) + }); - if !dependencies.contains(&package_name) + if !contained_in_dependencies && !matches!( resolver.resolve(folder, import), Err(ResolveError::Builtin { .. }) From 07c53e5ccfcfd6d5c27e12d9e2cf403afdc819a7 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Fri, 20 Dec 2024 13:10:23 -0500 Subject: [PATCH 04/23] - Add filter to boundaries - Add lint for file imports --- crates/turborepo-lib/src/boundaries.rs | 104 ++++++++++++++++---- crates/turborepo-lib/src/cli/mod.rs | 7 +- crates/turborepo-lib/src/opts.rs | 8 ++ crates/turborepo-lockfiles/src/pnpm/data.rs | 3 + 4 files changed, 101 insertions(+), 21 deletions(-) diff --git a/crates/turborepo-lib/src/boundaries.rs b/crates/turborepo-lib/src/boundaries.rs index dbc1b10259864..d5bef8f7ebfe9 100644 --- a/crates/turborepo-lib/src/boundaries.rs +++ b/crates/turborepo-lib/src/boundaries.rs @@ -1,4 +1,4 @@ -use std::collections::{BTreeMap, HashSet}; +use std::collections::{BTreeMap, HashMap, HashSet}; use git2::Repository; use itertools::Itertools; @@ -10,7 +10,7 @@ use swc_ecma_parser::{lexer::Lexer, Capturing, EsSyntax, Parser, Syntax, TsSynta use swc_ecma_visit::VisitWith; use thiserror::Error; use turbo_trace::{ImportFinder, Tracer}; -use turbopath::{AbsoluteSystemPath, AbsoluteSystemPathBuf}; +use turbopath::{AbsoluteSystemPath, AbsoluteSystemPathBuf, AnchoredSystemPath, PathRelation}; use turborepo_repository::package_graph::{PackageName, PackageNode}; use crate::run::Run; @@ -25,6 +25,20 @@ pub enum Error { #[source_code] text: NamedSource, }, + #[error("cannot import file `{import}` because it leaves the package")] + ImportLeavesPackage { + import: String, + #[label("file imported here")] + span: SourceSpan, + #[source_code] + text: NamedSource, + }, + #[error("file `{0}` does not have a parent directory")] + NoParentDir(AbsoluteSystemPathBuf), + #[error(transparent)] + Path(#[from] turbopath::PathError), + #[error(transparent)] + Lockfiles(#[from] turborepo_lockfiles::Error), #[error(transparent)] GlobWalk(#[from] globwalk::WalkError), #[error("failed to read file: {0}")] @@ -37,25 +51,38 @@ impl Run { pub async fn check_boundaries(&self) -> Result<(), Error> { let packages = self.pkg_dep_graph().packages(); let repo = Repository::discover(&self.repo_root()).ok(); - println!( - "{:?}", - self.pkg_dep_graph() - .package_info(&PackageName::Other("@turbo/gen".to_string())) - .unwrap() - .unresolved_external_dependencies - ); + for (package_name, package_info) in packages { + if !self.filtered_pkgs().contains(package_name) + || matches!(package_name, PackageName::Root) + { + continue; + } + let package_root = self.repo_root().resolve(package_info.package_path()); let internal_dependencies = self .pkg_dep_graph() .immediate_dependencies(&PackageNode::Workspace(package_name.to_owned())) .unwrap_or_default(); - let external_dependencies = package_info.unresolved_external_dependencies.as_ref(); + let unresolved_external_dependencies = + package_info.unresolved_external_dependencies.as_ref(); + + let resolved_external_dependencies = self + .pkg_dep_graph() + .lockfile() + .map(|l| l.all_dependencies(&format!("{}@*", package_name))) + .transpose()? + .flatten(); + + println!("internal: {:?}", internal_dependencies); + println!("resolved: {:#?}", resolved_external_dependencies); + println!("unresolved: {:#?}", unresolved_external_dependencies); self.check_package( &repo, &package_root, internal_dependencies, - external_dependencies, + resolved_external_dependencies, + unresolved_external_dependencies, ) .await?; } @@ -66,7 +93,8 @@ impl Run { repo: &Option, package_root: &AbsoluteSystemPath, internal_dependencies: HashSet<&PackageNode>, - external_dependencies: Option<&BTreeMap>, + resolved_external_dependencies: Option>, + unresolved_external_dependencies: Option<&BTreeMap>, ) -> Result<(), Error> { let files = globwalk::globwalk( package_root, @@ -151,7 +179,7 @@ impl Run { // We have a file import let check_result = if import.starts_with(".") { - self.check_file_import(&import) + self.check_file_import(&file_path, &package_root, &import, span, &file_content) } else { self.check_package_import( &import, @@ -159,7 +187,8 @@ impl Run { &file_path, &file_content, &internal_dependencies, - external_dependencies, + &resolved_external_dependencies, + unresolved_external_dependencies, &resolver, ) }; @@ -177,8 +206,39 @@ impl Run { Ok(()) } - fn check_file_import(&self, import: &str) -> Result<(), Error> { - Ok(()) + fn check_file_import( + &self, + file_path: &AbsoluteSystemPath, + package_path: &AbsoluteSystemPath, + import: &str, + source_span: SourceSpan, + file_content: &str, + ) -> Result<(), Error> { + let import_path = AnchoredSystemPath::new(import)?; + let dir_path = file_path + .parent() + .ok_or_else(|| Error::NoParentDir(file_path.to_owned()))?; + let resolved_import_path = dir_path.resolve(&import_path).clean()?; + // We have to check for this case because `relation_to_path` returns `Parent` if + // the paths are equal and there's nothing wrong with importing the + // package you're in. + if resolved_import_path.as_str() == package_path.as_str() { + return Ok(()); + } + // We use `relation_to_path` and not `contains` because `contains` + // panics on invalid paths with too many `..` components + if matches!( + package_path.relation_to_path(&resolved_import_path), + PathRelation::Divergent | PathRelation::Child + ) { + Err(Error::ImportLeavesPackage { + import: import.to_string(), + span: source_span, + text: NamedSource::new(file_path.as_str(), file_content.to_string()), + }) + } else { + Ok(()) + } } fn check_package_import( @@ -188,7 +248,8 @@ impl Run { file_path: &AbsoluteSystemPath, file_content: &str, internal_dependencies: &HashSet<&PackageNode>, - external_dependencies: Option<&BTreeMap>, + resolved_external_dependencies: &Option>, + unresolved_external_dependencies: Option<&BTreeMap>, resolver: &Resolver, ) -> Result<(), Error> { let package_name = if import.starts_with("@") { @@ -203,8 +264,13 @@ impl Run { let package_name = PackageNode::Workspace(PackageName::Other(package_name)); let folder = file_path.parent().expect("file_path should have a parent"); let contained_in_dependencies = internal_dependencies.contains(&package_name) - || external_dependencies.map_or(false, |external_dependencies| { - external_dependencies.contains_key(&package_name.to_string()) + || resolved_external_dependencies + .as_ref() + .map_or(false, |external_dependencies| { + external_dependencies.contains_key(package_name.as_package_name().as_str()) + }) + || unresolved_external_dependencies.map_or(false, |external_dependencies| { + external_dependencies.contains_key(package_name.as_package_name().as_str()) }); if !contained_in_dependencies diff --git a/crates/turborepo-lib/src/cli/mod.rs b/crates/turborepo-lib/src/cli/mod.rs index fecc0125cf444..0ba6413a5c354 100644 --- a/crates/turborepo-lib/src/cli/mod.rs +++ b/crates/turborepo-lib/src/cli/mod.rs @@ -552,7 +552,10 @@ impl Args { pub enum Command { /// Get the path to the Turbo binary Bin, - Boundaries, + Boundaries { + #[clap(short = 'F', long, group = "scope-filter-group")] + filter: Vec, + }, /// Generate the autocompletion script for the specified shell Completion { shell: Shell, @@ -1272,7 +1275,7 @@ pub async fn run( Ok(0) } - Command::Boundaries => { + Command::Boundaries { .. } => { let event = CommandEventBuilder::new("boundaries").with_parent(&root_telemetry); event.track_call(); diff --git a/crates/turborepo-lib/src/opts.rs b/crates/turborepo-lib/src/opts.rs index d0e665889be09..2b628d600d125 100644 --- a/crates/turborepo-lib/src/opts.rs +++ b/crates/turborepo-lib/src/opts.rs @@ -146,6 +146,14 @@ impl Opts { (&Box::new(execution_args), &Box::default()) } + Some(Command::Boundaries { filter }) => { + let execution_args = ExecutionArgs { + filter: filter.clone(), + ..Default::default() + }; + + (&Box::new(execution_args), &Box::default()) + } _ => (&Box::default(), &Box::default()), }; diff --git a/crates/turborepo-lockfiles/src/pnpm/data.rs b/crates/turborepo-lockfiles/src/pnpm/data.rs index 324f3b3c9016a..177c64daa942d 100644 --- a/crates/turborepo-lockfiles/src/pnpm/data.rs +++ b/crates/turborepo-lockfiles/src/pnpm/data.rs @@ -414,17 +414,20 @@ impl crate::Lockfile for PnpmLockfile { &self, key: &str, ) -> Result>, crate::Error> { + println!("all_dependencies: {}", key); // Check snapshots for v7 if let Some(snapshot) = self .snapshots .as_ref() .and_then(|snapshots| snapshots.get(key)) { + println!("snapshot for {}: {:#?}", key, snapshot); return Ok(Some(snapshot.dependencies())); } let Some(entry) = self.packages.as_ref().and_then(|pkgs| pkgs.get(key)) else { return Ok(None); }; + println!("snapshot: {:#?}", entry.snapshot); Ok(Some(entry.snapshot.dependencies())) } From dc402f0ef84776c2b6e75f90dfdb6fed3f7597f1 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Fri, 20 Dec 2024 16:55:27 -0500 Subject: [PATCH 05/23] Look for type declaration package if regular package is not found, but error if import is not properly declared as a type import --- crates/turbo-trace/src/import_finder.rs | 42 ++++-- crates/turbo-trace/src/lib.rs | 4 +- crates/turbo-trace/src/tracer.rs | 22 +-- crates/turborepo-lib/src/boundaries.rs | 144 +++++++++++++++++--- crates/turborepo-lib/src/query/file.rs | 8 +- crates/turborepo-lockfiles/src/pnpm/data.rs | 3 - 6 files changed, 170 insertions(+), 53 deletions(-) diff --git a/crates/turbo-trace/src/import_finder.rs b/crates/turbo-trace/src/import_finder.rs index 587e143445a48..d20c7ee561bef 100644 --- a/crates/turbo-trace/src/import_finder.rs +++ b/crates/turbo-trace/src/import_finder.rs @@ -2,28 +2,34 @@ use swc_common::{Span, Spanned}; use swc_ecma_ast::{Decl, ModuleDecl, Stmt}; use swc_ecma_visit::{Visit, VisitWith}; -use crate::tracer::ImportType; +use crate::tracer::ImportTraceType; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ImportType { + Type, + Value, +} pub struct ImportFinder { - import_type: ImportType, - imports: Vec<(String, Span)>, + import_type: ImportTraceType, + imports: Vec<(String, Span, ImportType)>, } impl Default for ImportFinder { fn default() -> Self { - Self::new(ImportType::All) + Self::new(ImportTraceType::All) } } impl ImportFinder { - pub fn new(import_type: ImportType) -> Self { + pub fn new(import_type: ImportTraceType) -> Self { Self { import_type, imports: Vec::new(), } } - pub fn imports(&self) -> &[(String, Span)] { + pub fn imports(&self) -> &[(String, Span, ImportType)] { &self.imports } } @@ -31,18 +37,23 @@ impl ImportFinder { impl Visit for ImportFinder { fn visit_module_decl(&mut self, decl: &ModuleDecl) { if let ModuleDecl::Import(import) = decl { + let import_type = if import.type_only { + ImportType::Type + } else { + ImportType::Value + }; match self.import_type { - ImportType::All => { + ImportTraceType::All => { self.imports - .push((import.src.value.to_string(), import.span)); + .push((import.src.value.to_string(), import.span, import_type)); } - ImportType::Types if import.type_only => { + ImportTraceType::Types if import.type_only => { self.imports - .push((import.src.value.to_string(), import.span)); + .push((import.src.value.to_string(), import.span, import_type)); } - ImportType::Values if !import.type_only => { + ImportTraceType::Values if !import.type_only => { self.imports - .push((import.src.value.to_string(), import.span)); + .push((import.src.value.to_string(), import.span, import_type)); } _ => {} } @@ -62,8 +73,11 @@ impl Visit for ImportFinder { lit_str, )) = &*arg.expr { - self.imports - .push((lit_str.value.to_string(), expr.span())); + self.imports.push(( + lit_str.value.to_string(), + expr.span(), + ImportType::Value, + )); } } } diff --git a/crates/turbo-trace/src/lib.rs b/crates/turbo-trace/src/lib.rs index f0e469078ce25..f6bc4c0efabeb 100644 --- a/crates/turbo-trace/src/lib.rs +++ b/crates/turbo-trace/src/lib.rs @@ -2,5 +2,5 @@ mod import_finder; mod tracer; -pub use import_finder::ImportFinder; -pub use tracer::{ImportType, TraceError, TraceResult, Tracer}; +pub use import_finder::{ImportFinder, ImportType}; +pub use tracer::{ImportTraceType, TraceError, TraceResult, Tracer}; diff --git a/crates/turbo-trace/src/tracer.rs b/crates/turbo-trace/src/tracer.rs index 8e3ed79086154..0ae9770e01869 100644 --- a/crates/turbo-trace/src/tracer.rs +++ b/crates/turbo-trace/src/tracer.rs @@ -38,7 +38,7 @@ pub struct Tracer { source_map: Arc, cwd: AbsoluteSystemPathBuf, errors: Vec, - import_type: ImportType, + import_type: ImportTraceType, } #[derive(Clone, Debug, Error, Diagnostic)] @@ -98,7 +98,7 @@ pub struct TraceResult { /// The type of imports to trace. #[derive(Debug, Clone, Copy, PartialEq, Eq)] #[allow(dead_code)] -pub enum ImportType { +pub enum ImportTraceType { /// Trace all imports. All, /// Trace only `import type` imports @@ -122,14 +122,14 @@ impl Tracer { files, ts_config, cwd, - import_type: ImportType::All, + import_type: ImportTraceType::All, errors: Vec::new(), source_map: Arc::new(SourceMap::default()), } } #[allow(dead_code)] - pub fn set_import_type(&mut self, import_type: ImportType) { + pub fn set_import_type(&mut self, import_type: ImportTraceType) { self.import_type = import_type; } @@ -139,7 +139,7 @@ impl Tracer { errors: &mut Vec, resolver: &Resolver, file_path: &AbsoluteSystemPath, - import_type: ImportType, + import_type: ImportTraceType, ) -> Option<(Vec, SeenFile)> { // Read the file content let Ok(file_content) = tokio::fs::read_to_string(&file_path).await else { @@ -191,7 +191,7 @@ impl Tracer { // Convert found imports/requires to absolute paths and add them to files to // visit let mut files = Vec::new(); - for (import, span) in finder.imports() { + for (import, span, _) in finder.imports() { debug!("processing {} in {}", import, file_path); let Some(file_dir) = file_path.parent() else { errors.push(TraceError::RootFile(file_path.to_owned())); @@ -344,7 +344,7 @@ impl Tracer { } } - pub fn create_resolver(ts_config: Option) -> Resolver { + pub fn create_resolver(ts_config: Option<&AbsoluteSystemPath>) -> Resolver { let mut options = ResolveOptions::default() .with_builtin_modules(true) .with_force_extension(EnforceExtension::Disabled) @@ -364,7 +364,7 @@ impl Tracer { if let Some(ts_config) = ts_config { options.tsconfig = Some(TsconfigOptions { - config_file: ts_config.into(), + config_file: ts_config.as_std_path().into(), references: TsconfigReferences::Auto, }); } @@ -374,7 +374,7 @@ impl Tracer { pub async fn trace(mut self, max_depth: Option) -> TraceResult { let mut seen: HashMap = HashMap::new(); - let resolver = Self::create_resolver(self.ts_config.take()); + let resolver = Self::create_resolver(self.ts_config.as_deref()); while let Some((file_path, file_depth)) = self.files.pop() { if let Some(max_depth) = max_depth { @@ -393,7 +393,7 @@ impl Tracer { } } - pub async fn reverse_trace(mut self) -> TraceResult { + pub async fn reverse_trace(self) -> TraceResult { let files = match globwalk::globwalk( &self.cwd, &[ @@ -420,7 +420,7 @@ impl Tracer { let mut futures = JoinSet::new(); - let resolver = Arc::new(Self::create_resolver(self.ts_config.take())); + let resolver = Arc::new(Self::create_resolver(self.ts_config.as_deref())); let source_map = self.source_map.clone(); let shared_self = Arc::new(self); diff --git a/crates/turborepo-lib/src/boundaries.rs b/crates/turborepo-lib/src/boundaries.rs index d5bef8f7ebfe9..efe0657d4699a 100644 --- a/crates/turborepo-lib/src/boundaries.rs +++ b/crates/turborepo-lib/src/boundaries.rs @@ -1,22 +1,40 @@ -use std::collections::{BTreeMap, HashMap, HashSet}; +use std::{ + collections::{BTreeMap, HashMap, HashSet}, + sync::LazyLock, +}; use git2::Repository; use itertools::Itertools; use miette::{Diagnostic, NamedSource, Report, SourceSpan}; use oxc_resolver::{ResolveError, Resolver}; +use regex::Regex; use swc_common::{comments::SingleThreadedComments, input::StringInput, FileName, SourceMap}; use swc_ecma_ast::EsVersion; use swc_ecma_parser::{lexer::Lexer, Capturing, EsSyntax, Parser, Syntax, TsSyntax}; use swc_ecma_visit::VisitWith; use thiserror::Error; -use turbo_trace::{ImportFinder, Tracer}; +use turbo_trace::{ImportFinder, ImportType, Tracer}; use turbopath::{AbsoluteSystemPath, AbsoluteSystemPathBuf, AnchoredSystemPath, PathRelation}; -use turborepo_repository::package_graph::{PackageName, PackageNode}; +use turborepo_repository::{ + package_graph::{PackageName, PackageNode}, + package_json::PackageJson, +}; use crate::run::Run; #[derive(Debug, Error, Diagnostic)] pub enum Error { + #[error( + "importing from a type declaration package, but import is not declared as a type-only \ + import" + )] + #[help("add `type` to the import declaration")] + NotTypeOnlyImport { + #[label("package imported here")] + span: SourceSpan, + #[source_code] + text: NamedSource, + }, #[error("cannot import package `{name}` because it is not a dependency")] PackageNotFound { name: String, @@ -47,6 +65,10 @@ pub enum Error { ParseError(AbsoluteSystemPathBuf, swc_ecma_parser::error::Error), } +static PACKAGE_NAME_REGEX: LazyLock = LazyLock::new(|| { + Regex::new(r"^(@[a-z0-9-~][a-z0-9-._~]*\/)?[a-z0-9-~][a-z0-9-._~]*$").unwrap() +}); + impl Run { pub async fn check_boundaries(&self) -> Result<(), Error> { let packages = self.pkg_dep_graph().packages(); @@ -60,6 +82,7 @@ impl Run { } let package_root = self.repo_root().resolve(package_info.package_path()); + let internal_dependencies = self .pkg_dep_graph() .immediate_dependencies(&PackageNode::Workspace(package_name.to_owned())) @@ -74,12 +97,13 @@ impl Run { .transpose()? .flatten(); - println!("internal: {:?}", internal_dependencies); - println!("resolved: {:#?}", resolved_external_dependencies); - println!("unresolved: {:#?}", unresolved_external_dependencies); + // println!("internal: {:?}", internal_dependencies); + // println!("resolved: {:#?}", resolved_external_dependencies); + // println!("unresolved: {:#?}", unresolved_external_dependencies); self.check_package( &repo, &package_root, + &package_info.package_json, internal_dependencies, resolved_external_dependencies, unresolved_external_dependencies, @@ -88,10 +112,16 @@ impl Run { } Ok(()) } + + fn is_potential_package_name(import: &str) -> bool { + PACKAGE_NAME_REGEX.is_match(import) + } + async fn check_package( &self, repo: &Option, package_root: &AbsoluteSystemPath, + package_json: &PackageJson, internal_dependencies: HashSet<&PackageNode>, resolved_external_dependencies: Option>, unresolved_external_dependencies: Option<&BTreeMap>, @@ -115,8 +145,12 @@ impl Run { let source_map = SourceMap::default(); let mut errors: Vec = Vec::new(); + // We assume the tsconfig.json is at the root of the package + let tsconfig_path = package_root.join_component("tsconfig.json"); + // TODO: Load tsconfig.json - let resolver = Tracer::create_resolver(None); + let resolver = + Tracer::create_resolver(tsconfig_path.exists().then(|| tsconfig_path.as_ref())); for file_path in files { if let Some(repo) = repo { @@ -171,7 +205,7 @@ impl Run { // Visit the AST and find imports let mut finder = ImportFinder::default(); module.visit_with(&mut finder); - for (import, span) in finder.imports() { + for (import, span, import_type) in finder.imports() { let (start, end) = source_map.span_to_char_offset(&source_file, *span); let start = start as usize; let end = end as usize; @@ -180,17 +214,21 @@ impl Run { // We have a file import let check_result = if import.starts_with(".") { self.check_file_import(&file_path, &package_root, &import, span, &file_content) - } else { + } else if Self::is_potential_package_name(&import) { self.check_package_import( &import, + *import_type, span, &file_path, &file_content, + package_json, &internal_dependencies, &resolved_external_dependencies, unresolved_external_dependencies, &resolver, ) + } else { + Ok(()) }; if let Err(err) = check_result { @@ -241,12 +279,59 @@ impl Run { } } + /// Go through all the possible places a package could be declared to see if + /// it's a valid import. We don't use `oxc_resolver` because there are some + /// cases where you can resolve a package that isn't declared properly. + fn is_dependency( + internal_dependencies: &HashSet<&PackageNode>, + package_json: &PackageJson, + resolved_external_dependencies: Option<&BTreeMap>, + unresolved_external_dependencies: Option<&HashMap>, + package_name: &PackageNode, + ) -> bool { + internal_dependencies.contains(&package_name) + || resolved_external_dependencies + .as_ref() + .map_or(false, |external_dependencies| { + external_dependencies.contains_key(package_name.as_package_name().as_str()) + }) + || unresolved_external_dependencies.map_or(false, |external_dependencies| { + external_dependencies.contains_key(package_name.as_package_name().as_str()) + }) + || package_json + .dependencies + .as_ref() + .map_or(false, |dependencies| { + dependencies.contains_key(package_name.as_package_name().as_str()) + }) + || package_json + .dev_dependencies + .as_ref() + .map_or(false, |dev_dependencies| { + dev_dependencies.contains_key(package_name.as_package_name().as_str()) + }) + || package_json + .peer_dependencies + .as_ref() + .map_or(false, |peer_dependencies| { + peer_dependencies.contains_key(package_name.as_package_name().as_str()) + }) + || package_json + .optional_dependencies + .as_ref() + .map_or(false, |optional_dependencies| { + optional_dependencies.contains_key(package_name.as_package_name().as_str()) + }) + } + fn check_package_import( &self, import: &str, + import_type: ImportType, span: SourceSpan, file_path: &AbsoluteSystemPath, file_content: &str, + package_json: &PackageJson, internal_dependencies: &HashSet<&PackageNode>, resolved_external_dependencies: &Option>, unresolved_external_dependencies: Option<&BTreeMap>, @@ -263,22 +348,43 @@ impl Run { }; let package_name = PackageNode::Workspace(PackageName::Other(package_name)); let folder = file_path.parent().expect("file_path should have a parent"); - let contained_in_dependencies = internal_dependencies.contains(&package_name) - || resolved_external_dependencies - .as_ref() - .map_or(false, |external_dependencies| { - external_dependencies.contains_key(package_name.as_package_name().as_str()) - }) - || unresolved_external_dependencies.map_or(false, |external_dependencies| { - external_dependencies.contains_key(package_name.as_package_name().as_str()) - }); + let is_valid_dependency = Self::is_dependency( + internal_dependencies, + package_json, + unresolved_external_dependencies, + resolved_external_dependencies.as_ref(), + &package_name, + ); - if !contained_in_dependencies + if !is_valid_dependency && !matches!( resolver.resolve(folder, import), Err(ResolveError::Builtin { .. }) ) { + // Check the @types package + let types_package_name = PackageNode::Workspace(PackageName::Other(format!( + "@types/{}", + package_name.as_package_name().as_str() + ))); + let is_types_dependency = Self::is_dependency( + internal_dependencies, + package_json, + unresolved_external_dependencies, + resolved_external_dependencies.as_ref(), + &types_package_name, + ); + + if is_types_dependency { + return match import_type { + ImportType::Type => Ok(()), + ImportType::Value => Err(Error::NotTypeOnlyImport { + span, + text: NamedSource::new(file_path.as_str(), file_content.to_string()), + }), + }; + } + return Err(Error::PackageNotFound { name: package_name.to_string(), span, diff --git a/crates/turborepo-lib/src/query/file.rs b/crates/turborepo-lib/src/query/file.rs index 77e78cf54fb21..080d6fc7a4347 100644 --- a/crates/turborepo-lib/src/query/file.rs +++ b/crates/turborepo-lib/src/query/file.rs @@ -166,12 +166,12 @@ pub enum ImportType { Values, } -impl From for turbo_trace::ImportType { +impl From for turbo_trace::ImportTraceType { fn from(import_type: ImportType) -> Self { match import_type { - ImportType::All => turbo_trace::ImportType::All, - ImportType::Types => turbo_trace::ImportType::Types, - ImportType::Values => turbo_trace::ImportType::Values, + ImportType::All => turbo_trace::ImportTraceType::All, + ImportType::Types => turbo_trace::ImportTraceType::Types, + ImportType::Values => turbo_trace::ImportTraceType::Values, } } } diff --git a/crates/turborepo-lockfiles/src/pnpm/data.rs b/crates/turborepo-lockfiles/src/pnpm/data.rs index 177c64daa942d..324f3b3c9016a 100644 --- a/crates/turborepo-lockfiles/src/pnpm/data.rs +++ b/crates/turborepo-lockfiles/src/pnpm/data.rs @@ -414,20 +414,17 @@ impl crate::Lockfile for PnpmLockfile { &self, key: &str, ) -> Result>, crate::Error> { - println!("all_dependencies: {}", key); // Check snapshots for v7 if let Some(snapshot) = self .snapshots .as_ref() .and_then(|snapshots| snapshots.get(key)) { - println!("snapshot for {}: {:#?}", key, snapshot); return Ok(Some(snapshot.dependencies())); } let Some(entry) = self.packages.as_ref().and_then(|pkgs| pkgs.get(key)) else { return Ok(None); }; - println!("snapshot: {:#?}", entry.snapshot); Ok(Some(entry.snapshot.dependencies())) } From 78b6fcac4e94c4b20cdfce8f978bcba391ef8e64 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Thu, 2 Jan 2025 17:03:47 -0500 Subject: [PATCH 06/23] - Connected boundaries to query - Added tests for boundaries --- crates/turborepo-lib/src/boundaries.rs | 190 +++++++++++------- .../turborepo-lib/src/commands/boundaries.rs | 4 +- crates/turborepo-lib/src/query/boundaries.rs | 42 ++++ crates/turborepo-lib/src/query/file.rs | 30 +-- crates/turborepo-lib/src/query/mod.rs | 31 ++- crates/turborepo/tests/boundaries.rs | 13 ++ ...ies_get_boundaries_lints_(npm@10.5.0).snap | 28 +++ .../fixtures/boundaries/.gitignore | 3 + .../boundaries/apps/my-app/.env.local | 0 .../fixtures/boundaries/apps/my-app/index.ts | 7 + .../boundaries/apps/my-app/package.json | 12 ++ .../boundaries/apps/my-app/tsconfig.json | 9 + .../fixtures/boundaries/apps/my-app/types.ts | 5 + .../fixtures/boundaries/package.json | 14 ++ .../boundaries/packages/another/index.jsx | 3 + .../boundaries/packages/another/package.json | 9 + .../packages/module-package/my-module.mjs | 1 + .../packages/module-package/package.json | 4 + .../boundaries/packages/ship-types/index.ts | 1 + .../packages/ship-types/package.json | 4 + .../fixtures/boundaries/turbo.json | 1 + 21 files changed, 314 insertions(+), 97 deletions(-) create mode 100644 crates/turborepo-lib/src/query/boundaries.rs create mode 100644 crates/turborepo/tests/boundaries.rs create mode 100644 crates/turborepo/tests/snapshots/boundaries__boundaries_get_boundaries_lints_(npm@10.5.0).snap create mode 100644 turborepo-tests/integration/fixtures/boundaries/.gitignore create mode 100644 turborepo-tests/integration/fixtures/boundaries/apps/my-app/.env.local create mode 100644 turborepo-tests/integration/fixtures/boundaries/apps/my-app/index.ts create mode 100644 turborepo-tests/integration/fixtures/boundaries/apps/my-app/package.json create mode 100644 turborepo-tests/integration/fixtures/boundaries/apps/my-app/tsconfig.json create mode 100644 turborepo-tests/integration/fixtures/boundaries/apps/my-app/types.ts create mode 100644 turborepo-tests/integration/fixtures/boundaries/package.json create mode 100644 turborepo-tests/integration/fixtures/boundaries/packages/another/index.jsx create mode 100644 turborepo-tests/integration/fixtures/boundaries/packages/another/package.json create mode 100644 turborepo-tests/integration/fixtures/boundaries/packages/module-package/my-module.mjs create mode 100644 turborepo-tests/integration/fixtures/boundaries/packages/module-package/package.json create mode 100644 turborepo-tests/integration/fixtures/boundaries/packages/ship-types/index.ts create mode 100644 turborepo-tests/integration/fixtures/boundaries/packages/ship-types/package.json create mode 100644 turborepo-tests/integration/fixtures/boundaries/turbo.json diff --git a/crates/turborepo-lib/src/boundaries.rs b/crates/turborepo-lib/src/boundaries.rs index efe0657d4699a..88d0b78c11eb4 100644 --- a/crates/turborepo-lib/src/boundaries.rs +++ b/crates/turborepo-lib/src/boundaries.rs @@ -1,6 +1,6 @@ use std::{ - collections::{BTreeMap, HashMap, HashSet}, - sync::LazyLock, + collections::{BTreeMap, HashSet}, + sync::{Arc, LazyLock, Mutex}, }; use git2::Repository; @@ -8,7 +8,12 @@ use itertools::Itertools; use miette::{Diagnostic, NamedSource, Report, SourceSpan}; use oxc_resolver::{ResolveError, Resolver}; use regex::Regex; -use swc_common::{comments::SingleThreadedComments, input::StringInput, FileName, SourceMap}; +use swc_common::{ + comments::SingleThreadedComments, + errors::{ColorConfig, Handler}, + input::StringInput, + FileName, SourceMap, +}; use swc_ecma_ast::EsVersion; use swc_ecma_parser::{lexer::Lexer, Capturing, EsSyntax, Parser, Syntax, TsSyntax}; use swc_ecma_visit::VisitWith; @@ -22,18 +27,19 @@ use turborepo_repository::{ use crate::run::Run; -#[derive(Debug, Error, Diagnostic)] -pub enum Error { +#[derive(Clone, Debug, Error, Diagnostic)] +pub enum BoundariesDiagnostic { #[error( "importing from a type declaration package, but import is not declared as a type-only \ import" )] #[help("add `type` to the import declaration")] NotTypeOnlyImport { + import: String, #[label("package imported here")] span: SourceSpan, #[source_code] - text: NamedSource, + text: Arc, }, #[error("cannot import package `{name}` because it is not a dependency")] PackageNotFound { @@ -41,7 +47,7 @@ pub enum Error { #[label("package imported here")] span: SourceSpan, #[source_code] - text: NamedSource, + text: Arc, }, #[error("cannot import file `{import}` because it leaves the package")] ImportLeavesPackage { @@ -49,8 +55,14 @@ pub enum Error { #[label("file imported here")] span: SourceSpan, #[source_code] - text: NamedSource, + text: Arc, }, + #[error("failed to parse file {0}")] + ParseError(AbsoluteSystemPathBuf, swc_ecma_parser::error::Error), +} + +#[derive(Debug, Error, Diagnostic)] +pub enum Error { #[error("file `{0}` does not have a parent directory")] NoParentDir(AbsoluteSystemPathBuf), #[error(transparent)] @@ -61,19 +73,45 @@ pub enum Error { GlobWalk(#[from] globwalk::WalkError), #[error("failed to read file: {0}")] FileNotFound(AbsoluteSystemPathBuf), - #[error("failed to parse file {0}")] - ParseError(AbsoluteSystemPathBuf, swc_ecma_parser::error::Error), } static PACKAGE_NAME_REGEX: LazyLock = LazyLock::new(|| { Regex::new(r"^(@[a-z0-9-~][a-z0-9-._~]*\/)?[a-z0-9-~][a-z0-9-._~]*$").unwrap() }); +pub struct BoundariesResult { + pub source_map: Arc, + pub diagnostics: Vec, +} + +impl BoundariesResult { + pub fn emit(&self) { + let handler = Handler::with_tty_emitter( + ColorConfig::Auto, + true, + false, + Some(self.source_map.clone()), + ); + + for diagnostic in self.diagnostics.clone() { + match diagnostic { + BoundariesDiagnostic::ParseError(_, e) => { + e.clone().into_diagnostic(&handler).emit(); + } + e => { + eprintln!("{:?}", Report::new(e.to_owned())); + } + } + } + } +} + impl Run { - pub async fn check_boundaries(&self) -> Result<(), Error> { + pub async fn check_boundaries(&self) -> Result { let packages = self.pkg_dep_graph().packages(); - let repo = Repository::discover(&self.repo_root()).ok(); - + let repo = Repository::discover(&self.repo_root()).ok().map(Mutex::new); + let mut diagnostics = vec![]; + let source_map = SourceMap::default(); for (package_name, package_info) in packages { if !self.filtered_pkgs().contains(package_name) || matches!(package_name, PackageName::Root) @@ -90,42 +128,40 @@ impl Run { let unresolved_external_dependencies = package_info.unresolved_external_dependencies.as_ref(); - let resolved_external_dependencies = self - .pkg_dep_graph() - .lockfile() - .map(|l| l.all_dependencies(&format!("{}@*", package_name))) - .transpose()? - .flatten(); - - // println!("internal: {:?}", internal_dependencies); - // println!("resolved: {:#?}", resolved_external_dependencies); - // println!("unresolved: {:#?}", unresolved_external_dependencies); - self.check_package( - &repo, - &package_root, - &package_info.package_json, - internal_dependencies, - resolved_external_dependencies, - unresolved_external_dependencies, - ) - .await?; + let package_diagnostics = self + .check_package( + &repo, + &package_root, + &package_info.package_json, + internal_dependencies, + unresolved_external_dependencies, + &source_map, + ) + .await?; + + diagnostics.extend(package_diagnostics); } - Ok(()) + + Ok(BoundariesResult { + source_map: Arc::new(source_map), + diagnostics, + }) } fn is_potential_package_name(import: &str) -> bool { PACKAGE_NAME_REGEX.is_match(import) } + /// Either returns a list of errors or a single, fatal error async fn check_package( &self, - repo: &Option, + repo: &Option>, package_root: &AbsoluteSystemPath, package_json: &PackageJson, internal_dependencies: HashSet<&PackageNode>, - resolved_external_dependencies: Option>, unresolved_external_dependencies: Option<&BTreeMap>, - ) -> Result<(), Error> { + source_map: &SourceMap, + ) -> Result, Error> { let files = globwalk::globwalk( package_root, &[ @@ -143,8 +179,7 @@ impl Run { globwalk::WalkType::Files, )?; - let source_map = SourceMap::default(); - let mut errors: Vec = Vec::new(); + let mut diagnostics: Vec = Vec::new(); // We assume the tsconfig.json is at the root of the package let tsconfig_path = package_root.join_component("tsconfig.json"); @@ -154,14 +189,14 @@ impl Run { for file_path in files { if let Some(repo) = repo { + let repo = repo.lock().expect("lock poisoned"); if matches!(repo.status_should_ignore(file_path.as_std_path()), Ok(true)) { continue; } }; // Read the file content let Ok(file_content) = tokio::fs::read_to_string(&file_path).await else { - errors.push(Error::FileNotFound(file_path.to_owned())); - continue; + return Err(Error::FileNotFound(file_path.to_owned())); }; let comments = SingleThreadedComments::default(); @@ -197,7 +232,7 @@ impl Run { let module = match parser.parse_module() { Ok(module) => module, Err(err) => { - errors.push(Error::ParseError(file_path.to_owned(), err)); + diagnostics.push(BoundariesDiagnostic::ParseError(file_path.to_owned(), err)); continue; } }; @@ -213,7 +248,7 @@ impl Run { // We have a file import let check_result = if import.starts_with(".") { - self.check_file_import(&file_path, &package_root, &import, span, &file_content) + self.check_file_import(&file_path, &package_root, &import, span, &file_content)? } else if Self::is_potential_package_name(&import) { self.check_package_import( &import, @@ -223,25 +258,20 @@ impl Run { &file_content, package_json, &internal_dependencies, - &resolved_external_dependencies, unresolved_external_dependencies, &resolver, ) } else { - Ok(()) + None }; - if let Err(err) = check_result { - errors.push(err); + if let Some(diagnostic) = check_result { + diagnostics.push(diagnostic); } } } - for error in errors { - println!("{:?}", Report::new(error)); - } - - Ok(()) + Ok(diagnostics) } fn check_file_import( @@ -251,7 +281,7 @@ impl Run { import: &str, source_span: SourceSpan, file_content: &str, - ) -> Result<(), Error> { + ) -> Result, Error> { let import_path = AnchoredSystemPath::new(import)?; let dir_path = file_path .parent() @@ -261,7 +291,7 @@ impl Run { // the paths are equal and there's nothing wrong with importing the // package you're in. if resolved_import_path.as_str() == package_path.as_str() { - return Ok(()); + return Ok(None); } // We use `relation_to_path` and not `contains` because `contains` // panics on invalid paths with too many `..` components @@ -269,13 +299,16 @@ impl Run { package_path.relation_to_path(&resolved_import_path), PathRelation::Divergent | PathRelation::Child ) { - Err(Error::ImportLeavesPackage { + Ok(Some(BoundariesDiagnostic::ImportLeavesPackage { import: import.to_string(), span: source_span, - text: NamedSource::new(file_path.as_str(), file_content.to_string()), - }) + text: Arc::new(NamedSource::new( + file_path.as_str(), + file_content.to_string(), + )), + })) } else { - Ok(()) + Ok(None) } } @@ -285,16 +318,10 @@ impl Run { fn is_dependency( internal_dependencies: &HashSet<&PackageNode>, package_json: &PackageJson, - resolved_external_dependencies: Option<&BTreeMap>, - unresolved_external_dependencies: Option<&HashMap>, + unresolved_external_dependencies: Option<&BTreeMap>, package_name: &PackageNode, ) -> bool { internal_dependencies.contains(&package_name) - || resolved_external_dependencies - .as_ref() - .map_or(false, |external_dependencies| { - external_dependencies.contains_key(package_name.as_package_name().as_str()) - }) || unresolved_external_dependencies.map_or(false, |external_dependencies| { external_dependencies.contains_key(package_name.as_package_name().as_str()) }) @@ -333,10 +360,9 @@ impl Run { file_content: &str, package_json: &PackageJson, internal_dependencies: &HashSet<&PackageNode>, - resolved_external_dependencies: &Option>, unresolved_external_dependencies: Option<&BTreeMap>, resolver: &Resolver, - ) -> Result<(), Error> { + ) -> Option { let package_name = if import.starts_with("@") { import.split('/').take(2).join("/") } else { @@ -346,13 +372,23 @@ impl Run { .unwrap_or(import) .to_string() }; + + if package_name.starts_with("@types/") && matches!(import_type, ImportType::Value) { + return Some(BoundariesDiagnostic::NotTypeOnlyImport { + import: import.to_string(), + span, + text: Arc::new(NamedSource::new( + file_path.as_str(), + file_content.to_string(), + )), + }); + } let package_name = PackageNode::Workspace(PackageName::Other(package_name)); let folder = file_path.parent().expect("file_path should have a parent"); let is_valid_dependency = Self::is_dependency( internal_dependencies, package_json, unresolved_external_dependencies, - resolved_external_dependencies.as_ref(), &package_name, ); @@ -371,27 +407,33 @@ impl Run { internal_dependencies, package_json, unresolved_external_dependencies, - resolved_external_dependencies.as_ref(), &types_package_name, ); if is_types_dependency { return match import_type { - ImportType::Type => Ok(()), - ImportType::Value => Err(Error::NotTypeOnlyImport { + ImportType::Type => None, + ImportType::Value => Some(BoundariesDiagnostic::NotTypeOnlyImport { + import: import.to_string(), span, - text: NamedSource::new(file_path.as_str(), file_content.to_string()), + text: Arc::new(NamedSource::new( + file_path.as_str(), + file_content.to_string(), + )), }), }; } - return Err(Error::PackageNotFound { + return Some(BoundariesDiagnostic::PackageNotFound { name: package_name.to_string(), span, - text: NamedSource::new(file_path.as_str(), file_content.to_string()), + text: Arc::new(NamedSource::new( + file_path.as_str(), + file_content.to_string(), + )), }); } - Ok(()) + None } } diff --git a/crates/turborepo-lib/src/commands/boundaries.rs b/crates/turborepo-lib/src/commands/boundaries.rs index 53e325edacbdf..d5d4e83b238d4 100644 --- a/crates/turborepo-lib/src/commands/boundaries.rs +++ b/crates/turborepo-lib/src/commands/boundaries.rs @@ -16,7 +16,9 @@ pub async fn run(base: CommandBase, telemetry: CommandEventBuilder) -> Result<() .build(&handler, telemetry) .await?; - run.check_boundaries().await?; + let result = run.check_boundaries().await?; + + result.emit(); Ok(()) } diff --git a/crates/turborepo-lib/src/query/boundaries.rs b/crates/turborepo-lib/src/query/boundaries.rs new file mode 100644 index 0000000000000..7ce65808f0d76 --- /dev/null +++ b/crates/turborepo-lib/src/query/boundaries.rs @@ -0,0 +1,42 @@ +use super::Diagnostic; +use crate::boundaries::BoundariesDiagnostic; + +impl From for Diagnostic { + fn from(diagnostic: BoundariesDiagnostic) -> Self { + let message = diagnostic.to_string(); + match diagnostic { + BoundariesDiagnostic::NotTypeOnlyImport { import, span, text } => Diagnostic { + message, + path: Some(text.name().to_string()), + start: Some(span.offset()), + end: Some(span.offset() + span.len()), + import: Some(import), + reason: None, + }, + BoundariesDiagnostic::PackageNotFound { name, span, text } => Diagnostic { + message, + path: Some(text.name().to_string()), + start: Some(span.offset()), + end: Some(span.offset() + span.len()), + import: Some(name.to_string()), + reason: None, + }, + BoundariesDiagnostic::ImportLeavesPackage { import, span, text } => Diagnostic { + message, + path: Some(text.name().to_string()), + start: Some(span.offset()), + end: Some(span.offset() + span.len()), + import: Some(import), + reason: None, + }, + BoundariesDiagnostic::ParseError(_, _) => Diagnostic { + message, + start: None, + end: None, + import: None, + path: None, + reason: None, + }, + } + } +} diff --git a/crates/turborepo-lib/src/query/file.rs b/crates/turborepo-lib/src/query/file.rs index 080d6fc7a4347..713cd372d3970 100644 --- a/crates/turborepo-lib/src/query/file.rs +++ b/crates/turborepo-lib/src/query/file.rs @@ -10,7 +10,7 @@ use turbo_trace::Tracer; use turbopath::AbsoluteSystemPathBuf; use crate::{ - query::{Array, Error}, + query::{Array, Diagnostic, Error}, run::Run, }; @@ -73,40 +73,30 @@ impl File { } } -#[derive(SimpleObject, Debug, Default)] -pub struct TraceError { - message: String, - reason: String, - path: Option, - import: Option, - start: Option, - end: Option, -} - -impl From for TraceError { +impl From for Diagnostic { fn from(error: turbo_trace::TraceError) -> Self { let message = error.to_string(); match error { - turbo_trace::TraceError::FileNotFound(file) => TraceError { + turbo_trace::TraceError::FileNotFound(file) => Diagnostic { message, path: Some(file.to_string()), ..Default::default() }, - turbo_trace::TraceError::PathEncoding(_) => TraceError { + turbo_trace::TraceError::PathEncoding(_) => Diagnostic { message, ..Default::default() }, - turbo_trace::TraceError::RootFile(path) => TraceError { + turbo_trace::TraceError::RootFile(path) => Diagnostic { message, path: Some(path.to_string()), ..Default::default() }, - turbo_trace::TraceError::ParseError(path, e) => TraceError { + turbo_trace::TraceError::ParseError(path, e) => Diagnostic { message: format!("failed to parse file: {:?}", e), path: Some(path.to_string()), ..Default::default() }, - turbo_trace::TraceError::GlobError(err) => TraceError { + turbo_trace::TraceError::GlobError(err) => Diagnostic { message: format!("failed to glob files: {}", err), ..Default::default() }, @@ -122,10 +112,10 @@ impl From for TraceError { .ok() .map(|s| String::from_utf8_lossy(s.data()).to_string()); - TraceError { + Diagnostic { message, import, - reason, + reason: Some(reason.to_string()), path: Some(file_path), start: Some(span.offset()), end: Some(span.offset() + span.len()), @@ -138,7 +128,7 @@ impl From for TraceError { #[derive(SimpleObject)] struct TraceResult { files: Array, - errors: Array, + errors: Array, } impl TraceResult { diff --git a/crates/turborepo-lib/src/query/mod.rs b/crates/turborepo-lib/src/query/mod.rs index 044abe1848291..905ba422ef316 100644 --- a/crates/turborepo-lib/src/query/mod.rs +++ b/crates/turborepo-lib/src/query/mod.rs @@ -1,4 +1,6 @@ + mod external_package; +mod boundaries; mod file; mod package; mod server; @@ -29,9 +31,11 @@ use crate::{ signal::SignalHandler, }; -#[derive(Error, Debug, Diagnostic)] +#[derive(Error, Debug, miette::Diagnostic)] pub enum Error { - #[error("Failed to get file dependencies.")] + #[error(transparent)] + Boundaries(#[from] crate::boundaries::Error), + #[error("Failed to get file dependencies")] Trace(#[related] Vec), #[error("No signal handler.")] NoSignalHandler, @@ -151,6 +155,7 @@ impl RepositoryQuery { #[graphql(concrete(name = "Files", params(File)))] #[graphql(concrete(name = "TraceErrors", params(file::TraceError)))] #[graphql(concrete(name = "ExternalPackages", params(ExternalPackage)))] +#[graphql(concrete(name = "Diagnostics", params(Diagnostic)))] pub struct Array { items: Vec, length: usize, @@ -551,6 +556,18 @@ impl RepositoryQuery { get_version() } + /// Check boundaries for all packages. + async fn boundaries(&self) -> Result, Error> { + match self.run.check_boundaries().await { + Ok(result) => { + result.emit(); + + Ok(result.diagnostics.into_iter().map(|b| b.into()).collect()) + } + Err(err) => Err(Error::Boundaries(err)), + } + } + async fn file(&self, path: String) -> Result { let abs_path = AbsoluteSystemPathBuf::from_unknown(self.run.repo_root(), path); @@ -608,3 +625,13 @@ pub async fn run_query_server(run: Run, signal: SignalHandler) -> Result<(), Err Ok(()) } + +#[derive(SimpleObject, Debug, Default)] +pub struct Diagnostic { + pub message: String, + pub reason: Option, + pub path: Option, + pub import: Option, + pub start: Option, + pub end: Option, +} diff --git a/crates/turborepo/tests/boundaries.rs b/crates/turborepo/tests/boundaries.rs new file mode 100644 index 0000000000000..69a1cedf96ef8 --- /dev/null +++ b/crates/turborepo/tests/boundaries.rs @@ -0,0 +1,13 @@ +mod common; + +#[test] +fn test_boundaries() -> Result<(), anyhow::Error> { + check_json!( + "boundaries", + "npm@10.5.0", + "query", + "get boundaries lints" => "query { boundaries { items { message import } } }", + ); + + Ok(()) +} diff --git a/crates/turborepo/tests/snapshots/boundaries__boundaries_get_boundaries_lints_(npm@10.5.0).snap b/crates/turborepo/tests/snapshots/boundaries__boundaries_get_boundaries_lints_(npm@10.5.0).snap new file mode 100644 index 0000000000000..d1dc57842e99e --- /dev/null +++ b/crates/turborepo/tests/snapshots/boundaries__boundaries_get_boundaries_lints_(npm@10.5.0).snap @@ -0,0 +1,28 @@ +--- +source: crates/turborepo/tests/boundaries.rs +expression: query_output +--- +{ + "data": { + "boundaries": { + "items": [ + { + "message": "cannot import file `../../packages/another/index.jsx` because it leaves the package", + "import": "../../packages/another/index.jsx" + }, + { + "message": "importing from a type declaration package, but import is not declared as a type-only import", + "import": "ship" + }, + { + "message": "importing from a type declaration package, but import is not declared as a type-only import", + "import": "@types/ship" + }, + { + "message": "cannot import package `module-package` because it is not a dependency", + "import": "module-package" + } + ] + } + } +} diff --git a/turborepo-tests/integration/fixtures/boundaries/.gitignore b/turborepo-tests/integration/fixtures/boundaries/.gitignore new file mode 100644 index 0000000000000..77af9fc60321d --- /dev/null +++ b/turborepo-tests/integration/fixtures/boundaries/.gitignore @@ -0,0 +1,3 @@ +node_modules/ +.turbo +.npmrc diff --git a/turborepo-tests/integration/fixtures/boundaries/apps/my-app/.env.local b/turborepo-tests/integration/fixtures/boundaries/apps/my-app/.env.local new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/turborepo-tests/integration/fixtures/boundaries/apps/my-app/index.ts b/turborepo-tests/integration/fixtures/boundaries/apps/my-app/index.ts new file mode 100644 index 0000000000000..2575ef917a4ef --- /dev/null +++ b/turborepo-tests/integration/fixtures/boundaries/apps/my-app/index.ts @@ -0,0 +1,7 @@ +// Import directly from a file instead of via package name +import { blackbeard } from "../../packages/another/index.jsx"; +// Import type without "type" specifier in import +import { Ship } from "ship"; +import { Ship } from "@types/ship"; +// Import package that is not specified +import { walkThePlank } from "module-package"; diff --git a/turborepo-tests/integration/fixtures/boundaries/apps/my-app/package.json b/turborepo-tests/integration/fixtures/boundaries/apps/my-app/package.json new file mode 100644 index 0000000000000..521d6f8e04c18 --- /dev/null +++ b/turborepo-tests/integration/fixtures/boundaries/apps/my-app/package.json @@ -0,0 +1,12 @@ +{ + "name": "my-app", + "scripts": { + "build": "echo building", + "maybefails": "exit 4" + }, + "dependencies": { + "utils": "*", + "@types/ship": "*", + "package-with-conditions": "*" + } +} diff --git a/turborepo-tests/integration/fixtures/boundaries/apps/my-app/tsconfig.json b/turborepo-tests/integration/fixtures/boundaries/apps/my-app/tsconfig.json new file mode 100644 index 0000000000000..c199498e2ce9f --- /dev/null +++ b/turborepo-tests/integration/fixtures/boundaries/apps/my-app/tsconfig.json @@ -0,0 +1,9 @@ +{ + "compilerOptions": { + "paths": { + "@/*": [ + "./*" + ] + } + } +} \ No newline at end of file diff --git a/turborepo-tests/integration/fixtures/boundaries/apps/my-app/types.ts b/turborepo-tests/integration/fixtures/boundaries/apps/my-app/types.ts new file mode 100644 index 0000000000000..8617784efc920 --- /dev/null +++ b/turborepo-tests/integration/fixtures/boundaries/apps/my-app/types.ts @@ -0,0 +1,5 @@ +import { blackbeard } from "@/../../packages/another/index.jsx"; + +export interface Pirate { + ship: string; +} diff --git a/turborepo-tests/integration/fixtures/boundaries/package.json b/turborepo-tests/integration/fixtures/boundaries/package.json new file mode 100644 index 0000000000000..83520cd0fc7cb --- /dev/null +++ b/turborepo-tests/integration/fixtures/boundaries/package.json @@ -0,0 +1,14 @@ +{ + "name": "monorepo", + "scripts": { + "something": "turbo run build" + }, + "dependencies": { + "module-package": "*" + }, + "packageManager": "npm@10.5.0", + "workspaces": [ + "apps/**", + "packages/**" + ] +} diff --git a/turborepo-tests/integration/fixtures/boundaries/packages/another/index.jsx b/turborepo-tests/integration/fixtures/boundaries/packages/another/index.jsx new file mode 100644 index 0000000000000..94639c6347160 --- /dev/null +++ b/turborepo-tests/integration/fixtures/boundaries/packages/another/index.jsx @@ -0,0 +1,3 @@ +import ship from "utils"; + +export const blackbeard = "Edward Teach on " + ship; diff --git a/turborepo-tests/integration/fixtures/boundaries/packages/another/package.json b/turborepo-tests/integration/fixtures/boundaries/packages/another/package.json new file mode 100644 index 0000000000000..bb796c8455b16 --- /dev/null +++ b/turborepo-tests/integration/fixtures/boundaries/packages/another/package.json @@ -0,0 +1,9 @@ +{ + "name": "another", + "scripts": { + "dev": "echo building" + }, + "dependencies": { + "utils": "*" + } +} diff --git a/turborepo-tests/integration/fixtures/boundaries/packages/module-package/my-module.mjs b/turborepo-tests/integration/fixtures/boundaries/packages/module-package/my-module.mjs new file mode 100644 index 0000000000000..5300c6eb7abe2 --- /dev/null +++ b/turborepo-tests/integration/fixtures/boundaries/packages/module-package/my-module.mjs @@ -0,0 +1 @@ +export const walkThePlank = "walk the plank matey"; diff --git a/turborepo-tests/integration/fixtures/boundaries/packages/module-package/package.json b/turborepo-tests/integration/fixtures/boundaries/packages/module-package/package.json new file mode 100644 index 0000000000000..79b674031cb6b --- /dev/null +++ b/turborepo-tests/integration/fixtures/boundaries/packages/module-package/package.json @@ -0,0 +1,4 @@ +{ + "name": "module-package", + "module": "my-module.mjs" +} \ No newline at end of file diff --git a/turborepo-tests/integration/fixtures/boundaries/packages/ship-types/index.ts b/turborepo-tests/integration/fixtures/boundaries/packages/ship-types/index.ts new file mode 100644 index 0000000000000..c91206a0e0056 --- /dev/null +++ b/turborepo-tests/integration/fixtures/boundaries/packages/ship-types/index.ts @@ -0,0 +1 @@ +export type Ship = "pirate" | "privateer" | "corvette"; diff --git a/turborepo-tests/integration/fixtures/boundaries/packages/ship-types/package.json b/turborepo-tests/integration/fixtures/boundaries/packages/ship-types/package.json new file mode 100644 index 0000000000000..c19c2c74a2b30 --- /dev/null +++ b/turborepo-tests/integration/fixtures/boundaries/packages/ship-types/package.json @@ -0,0 +1,4 @@ +{ + "name": "@types/ship", + "types": "index.ts" +} \ No newline at end of file diff --git a/turborepo-tests/integration/fixtures/boundaries/turbo.json b/turborepo-tests/integration/fixtures/boundaries/turbo.json new file mode 100644 index 0000000000000..9e26dfeeb6e64 --- /dev/null +++ b/turborepo-tests/integration/fixtures/boundaries/turbo.json @@ -0,0 +1 @@ +{} \ No newline at end of file From a80f4ae1e503648c0ee9735dec2332bb2979800d Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Thu, 2 Jan 2025 17:20:33 -0500 Subject: [PATCH 07/23] Return a correct exit code --- crates/turborepo-lib/src/boundaries.rs | 4 ++++ crates/turborepo-lib/src/cli/mod.rs | 3 +-- crates/turborepo-lib/src/commands/boundaries.rs | 8 ++++++-- 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/crates/turborepo-lib/src/boundaries.rs b/crates/turborepo-lib/src/boundaries.rs index 88d0b78c11eb4..b40428a96fd23 100644 --- a/crates/turborepo-lib/src/boundaries.rs +++ b/crates/turborepo-lib/src/boundaries.rs @@ -85,6 +85,10 @@ pub struct BoundariesResult { } impl BoundariesResult { + pub fn is_ok(&self) -> bool { + self.diagnostics.is_empty() + } + pub fn emit(&self) { let handler = Handler::with_tty_emitter( ColorConfig::Auto, diff --git a/crates/turborepo-lib/src/cli/mod.rs b/crates/turborepo-lib/src/cli/mod.rs index 0ba6413a5c354..bc3ce098fc4ea 100644 --- a/crates/turborepo-lib/src/cli/mod.rs +++ b/crates/turborepo-lib/src/cli/mod.rs @@ -1281,8 +1281,7 @@ pub async fn run( event.track_call(); let base = CommandBase::new(cli_args.clone(), repo_root, version, color_config)?; - boundaries::run(base, event).await?; - Ok(0) + Ok(boundaries::run(base, event).await?) } #[allow(unused_variables)] Command::Daemon { command, idle_time } => { diff --git a/crates/turborepo-lib/src/commands/boundaries.rs b/crates/turborepo-lib/src/commands/boundaries.rs index d5d4e83b238d4..07c6faef34972 100644 --- a/crates/turborepo-lib/src/commands/boundaries.rs +++ b/crates/turborepo-lib/src/commands/boundaries.rs @@ -7,7 +7,7 @@ use crate::{ signal::SignalHandler, }; -pub async fn run(base: CommandBase, telemetry: CommandEventBuilder) -> Result<(), cli::Error> { +pub async fn run(base: CommandBase, telemetry: CommandEventBuilder) -> Result { let signal = get_signal()?; let handler = SignalHandler::new(signal); @@ -20,5 +20,9 @@ pub async fn run(base: CommandBase, telemetry: CommandEventBuilder) -> Result<() result.emit(); - Ok(()) + if result.is_ok() { + Ok(0) + } else { + Ok(1) + } } From c867764cdf9ac3c3ef9681412640b25e0cc6b092 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Thu, 2 Jan 2025 17:23:41 -0500 Subject: [PATCH 08/23] Remove examples in globs --- crates/turborepo-lib/src/boundaries.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/crates/turborepo-lib/src/boundaries.rs b/crates/turborepo-lib/src/boundaries.rs index b40428a96fd23..295636740c8de 100644 --- a/crates/turborepo-lib/src/boundaries.rs +++ b/crates/turborepo-lib/src/boundaries.rs @@ -176,10 +176,7 @@ impl Run { "**/*.vue".parse().unwrap(), "**/*.svelte".parse().unwrap(), ], - &[ - "**/node_modules/**".parse().unwrap(), - "**/examples/**".parse().unwrap(), - ], + &["**/node_modules/**".parse().unwrap()], globwalk::WalkType::Files, )?; From 55a62094b2f12ba74b83fbf46134f9443888ddd5 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Thu, 2 Jan 2025 17:31:11 -0500 Subject: [PATCH 09/23] Hide it --- crates/turborepo-lib/src/cli/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/turborepo-lib/src/cli/mod.rs b/crates/turborepo-lib/src/cli/mod.rs index bc3ce098fc4ea..b8e453a71f114 100644 --- a/crates/turborepo-lib/src/cli/mod.rs +++ b/crates/turborepo-lib/src/cli/mod.rs @@ -552,6 +552,7 @@ impl Args { pub enum Command { /// Get the path to the Turbo binary Bin, + #[clap(hide = true)] Boundaries { #[clap(short = 'F', long, group = "scope-filter-group")] filter: Vec, From 17b2eb25eae3027a4f08503b6c57e3e31a15654c Mon Sep 17 00:00:00 2001 From: Nicholas Yang Date: Fri, 3 Jan 2025 11:19:36 -0500 Subject: [PATCH 10/23] Update crates/turborepo-filewatch/Cargo.toml Co-authored-by: Chris Olszewski --- crates/turborepo-filewatch/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/turborepo-filewatch/Cargo.toml b/crates/turborepo-filewatch/Cargo.toml index 3002513ea49b1..fa8af6944e4a2 100644 --- a/crates/turborepo-filewatch/Cargo.toml +++ b/crates/turborepo-filewatch/Cargo.toml @@ -36,7 +36,7 @@ version = "1.0.4" version = "0.2.4" [dev-dependencies] -git2 = { version = "0.19.0", default-features = false } +git2 = { workspace = true, default-features = false } tempfile = { workspace = true } tokio-scoped = "0.2.0" tracing-test = "0.2.4" From 45fe56557518241c8ceb3605e9044102683988fa Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Fri, 3 Jan 2025 11:20:57 -0500 Subject: [PATCH 11/23] Removing unnecessary dependencies --- .../integration/fixtures/boundaries/apps/my-app/package.json | 2 -- 1 file changed, 2 deletions(-) diff --git a/turborepo-tests/integration/fixtures/boundaries/apps/my-app/package.json b/turborepo-tests/integration/fixtures/boundaries/apps/my-app/package.json index 521d6f8e04c18..7030751d54895 100644 --- a/turborepo-tests/integration/fixtures/boundaries/apps/my-app/package.json +++ b/turborepo-tests/integration/fixtures/boundaries/apps/my-app/package.json @@ -5,8 +5,6 @@ "maybefails": "exit 4" }, "dependencies": { - "utils": "*", "@types/ship": "*", - "package-with-conditions": "*" } } From cfb86afcf53ad3a78c558541e20a3845ebf23f54 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Fri, 3 Jan 2025 11:21:24 -0500 Subject: [PATCH 12/23] Removing dependencies --- .../integration/fixtures/boundaries/apps/my-app/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/turborepo-tests/integration/fixtures/boundaries/apps/my-app/package.json b/turborepo-tests/integration/fixtures/boundaries/apps/my-app/package.json index 7030751d54895..1597bcae5865f 100644 --- a/turborepo-tests/integration/fixtures/boundaries/apps/my-app/package.json +++ b/turborepo-tests/integration/fixtures/boundaries/apps/my-app/package.json @@ -5,6 +5,6 @@ "maybefails": "exit 4" }, "dependencies": { - "@types/ship": "*", + "@types/ship": "*" } } From 3ba4e65c12b8d4e4c2aafeaa62e4fe3089db7042 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Fri, 3 Jan 2025 11:22:38 -0500 Subject: [PATCH 13/23] Removing unnecessary file --- crates/turborepo-lib/src/run/summary/page.rs | 62 -------------------- 1 file changed, 62 deletions(-) delete mode 100644 crates/turborepo-lib/src/run/summary/page.rs diff --git a/crates/turborepo-lib/src/run/summary/page.rs b/crates/turborepo-lib/src/run/summary/page.rs deleted file mode 100644 index ec89617b2886b..0000000000000 --- a/crates/turborepo-lib/src/run/summary/page.rs +++ /dev/null @@ -1,62 +0,0 @@ -use maud::{html, Markup, DOCTYPE}; -use tailwind_css::TailwindBuilder; - -use crate::run::summary::{execution::ExecutionSummary, Error, RunSummary}; - -impl<'a> RunSummary<'a> { - pub fn render_html(&self) -> Result { - let mut tailwind = TailwindBuilder::default(); - let body = html! { - body class=(trace(&mut tailwind, "flex flex-col")?) { - h3 class=(trace(&mut tailwind, "text-md text-gray-200")?) { "turbo " (self.turbo_version) } - (self.execution - .as_ref() - .map(|e| e.render_html(&mut tailwind, self.packages.len())) - .transpose()?.unwrap_or_default()) - } - }; - - Ok(html! { - html lang="en" { - (DOCTYPE) - style { - (tailwind.bundle()?) - } - head { - meta charset="utf-8"; - meta name="viewport" content="width=device-width, initial-scale=1.0"; - title { "Turborepo" } - } - (body) - } - } - .into_string()) - } -} - -fn trace(tailwind: &mut TailwindBuilder, class: &'static str) -> Result<&'static str, Error> { - tailwind.trace(class, false)?; - Ok(class) -} - -impl<'a> ExecutionSummary<'a> { - pub fn render_html( - &self, - tailwind: &mut TailwindBuilder, - packages: usize, - ) -> Result { - Ok(html! { - div { - h1 class=(trace(tailwind, "text-2xl")?) { - "Ran " - code { - (self.command) - } - " in " - (packages) - " packages" - } - } - }) - } -} From 997af2e95c02c144e6e3f14aa035a4ac848ce100 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Fri, 3 Jan 2025 11:34:08 -0500 Subject: [PATCH 14/23] More tests --- crates/turbo-trace/src/import_finder.rs | 3 +++ crates/turborepo-lib/src/boundaries.rs | 6 ++--- crates/turborepo-lib/src/cli/mod.rs | 25 ++++++++++++++++++- ...__test__turbo---filter=foo-boundaries.snap | 9 +++++++ ...i__test__turbo---no-daemon-boundaries.snap | 9 +++++++ 5 files changed, 48 insertions(+), 4 deletions(-) create mode 100644 crates/turborepo-lib/src/cli/snapshots/turborepo_lib__cli__test__turbo---filter=foo-boundaries.snap create mode 100644 crates/turborepo-lib/src/cli/snapshots/turborepo_lib__cli__test__turbo---no-daemon-boundaries.snap diff --git a/crates/turbo-trace/src/import_finder.rs b/crates/turbo-trace/src/import_finder.rs index d20c7ee561bef..6d15244042d29 100644 --- a/crates/turbo-trace/src/import_finder.rs +++ b/crates/turbo-trace/src/import_finder.rs @@ -4,6 +4,9 @@ use swc_ecma_visit::{Visit, VisitWith}; use crate::tracer::ImportTraceType; +/// The type of import that we find, either an import with a `type` keyword +/// (indicating that it is importing only types) or an import without the `type` +/// keyword (indicating that it is importing values and possibly types). #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum ImportType { Type, diff --git a/crates/turborepo-lib/src/boundaries.rs b/crates/turborepo-lib/src/boundaries.rs index 295636740c8de..92d7db9ace2c4 100644 --- a/crates/turborepo-lib/src/boundaries.rs +++ b/crates/turborepo-lib/src/boundaries.rs @@ -19,7 +19,7 @@ use swc_ecma_parser::{lexer::Lexer, Capturing, EsSyntax, Parser, Syntax, TsSynta use swc_ecma_visit::VisitWith; use thiserror::Error; use turbo_trace::{ImportFinder, ImportType, Tracer}; -use turbopath::{AbsoluteSystemPath, AbsoluteSystemPathBuf, AnchoredSystemPath, PathRelation}; +use turbopath::{AbsoluteSystemPath, AbsoluteSystemPathBuf, PathRelation, RelativeUnixPath}; use turborepo_repository::{ package_graph::{PackageName, PackageNode}, package_json::PackageJson, @@ -283,11 +283,11 @@ impl Run { source_span: SourceSpan, file_content: &str, ) -> Result, Error> { - let import_path = AnchoredSystemPath::new(import)?; + let import_path = RelativeUnixPath::new(import)?; let dir_path = file_path .parent() .ok_or_else(|| Error::NoParentDir(file_path.to_owned()))?; - let resolved_import_path = dir_path.resolve(&import_path).clean()?; + let resolved_import_path = dir_path.join_unix_path(&import_path).clean()?; // We have to check for this case because `relation_to_path` returns `Parent` if // the paths are equal and there's nothing wrong with importing the // package you're in. diff --git a/crates/turborepo-lib/src/cli/mod.rs b/crates/turborepo-lib/src/cli/mod.rs index b8e453a71f114..5860dd407e11a 100644 --- a/crates/turborepo-lib/src/cli/mod.rs +++ b/crates/turborepo-lib/src/cli/mod.rs @@ -519,7 +519,9 @@ impl Args { if self.run_args.is_some() && !matches!( self.command, - None | Some(Command::Run { .. }) | Some(Command::Config) + None | Some(Command::Run { .. }) + | Some(Command::Config) + | Some(Command::Boundaries { .. }) ) { let mut cmd = Self::command(); @@ -541,6 +543,14 @@ impl Args { clap::error::ErrorKind::ArgumentConflict, "Cannot use run arguments before `run` subcommand", )) + } else if matches!(self.command, Some(Command::Boundaries { .. })) + && (self.run_args.is_some() || self.execution_args.is_some()) + { + let mut cmd = Self::command(); + Err(cmd.error( + clap::error::ErrorKind::ArgumentConflict, + "Cannot use run arguments before `boundaries` subcommand", + )) } else { Ok(()) } @@ -3024,4 +3034,17 @@ mod test { assert_snapshot!(args.join("-").as_str(), err); } } + + #[test_case::test_case(&["turbo", "--filter=foo", "boundaries"], false; "execution args")] + #[test_case::test_case(&["turbo", "--no-daemon", "boundaries"], false; "run args")] + fn test_no_run_args_before_boundaries(args: &[&str], is_okay: bool) { + let os_args = args.iter().map(|s| OsString::from(*s)).collect(); + let cli = Args::parse(os_args); + if is_okay { + cli.unwrap(); + } else { + let err = cli.unwrap_err(); + assert_snapshot!(args.join("-").as_str(), err); + } + } } diff --git a/crates/turborepo-lib/src/cli/snapshots/turborepo_lib__cli__test__turbo---filter=foo-boundaries.snap b/crates/turborepo-lib/src/cli/snapshots/turborepo_lib__cli__test__turbo---filter=foo-boundaries.snap new file mode 100644 index 0000000000000..089d332391f20 --- /dev/null +++ b/crates/turborepo-lib/src/cli/snapshots/turborepo_lib__cli__test__turbo---filter=foo-boundaries.snap @@ -0,0 +1,9 @@ +--- +source: crates/turborepo-lib/src/cli/mod.rs +expression: err +--- +error: Cannot use run arguments before `boundaries` subcommand + +Usage: turbo [OPTIONS] [COMMAND] + +For more information, try '--help'. diff --git a/crates/turborepo-lib/src/cli/snapshots/turborepo_lib__cli__test__turbo---no-daemon-boundaries.snap b/crates/turborepo-lib/src/cli/snapshots/turborepo_lib__cli__test__turbo---no-daemon-boundaries.snap new file mode 100644 index 0000000000000..089d332391f20 --- /dev/null +++ b/crates/turborepo-lib/src/cli/snapshots/turborepo_lib__cli__test__turbo---no-daemon-boundaries.snap @@ -0,0 +1,9 @@ +--- +source: crates/turborepo-lib/src/cli/mod.rs +expression: err +--- +error: Cannot use run arguments before `boundaries` subcommand + +Usage: turbo [OPTIONS] [COMMAND] + +For more information, try '--help'. From 6eb9286ea9d63858b40ddd012f12ac2f6ec7fd60 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Fri, 3 Jan 2025 11:51:43 -0500 Subject: [PATCH 15/23] Tests for parsing out package name --- crates/turborepo-lib/src/boundaries.rs | 41 ++++++++++++++++++++------ 1 file changed, 32 insertions(+), 9 deletions(-) diff --git a/crates/turborepo-lib/src/boundaries.rs b/crates/turborepo-lib/src/boundaries.rs index 92d7db9ace2c4..0d6e923ce5a29 100644 --- a/crates/turborepo-lib/src/boundaries.rs +++ b/crates/turborepo-lib/src/boundaries.rs @@ -352,6 +352,18 @@ impl Run { }) } + fn get_package_name(import: &str) -> String { + if import.starts_with("@") { + import.split('/').take(2).join("/") + } else { + import + .split_once("/") + .map(|(import, _)| import) + .unwrap_or(import) + .to_string() + } + } + fn check_package_import( &self, import: &str, @@ -364,15 +376,7 @@ impl Run { unresolved_external_dependencies: Option<&BTreeMap>, resolver: &Resolver, ) -> Option { - let package_name = if import.starts_with("@") { - import.split('/').take(2).join("/") - } else { - import - .split_once("/") - .map(|(import, _)| import) - .unwrap_or(import) - .to_string() - }; + let package_name = Self::get_package_name(import); if package_name.starts_with("@types/") && matches!(import_type, ImportType::Value) { return Some(BoundariesDiagnostic::NotTypeOnlyImport { @@ -438,3 +442,22 @@ impl Run { None } } + +#[cfg(test)] +mod test { + use test_case::test_case; + + use super::*; + + #[test_case("", ""; "empty")] + #[test_case("ship", "ship"; "basic")] + #[test_case("@types/ship", "@types/ship"; "types")] + #[test_case("@scope/ship", "@scope/ship"; "scoped")] + #[test_case("@scope/foo/bar", "@scope/foo"; "scoped with path")] + #[test_case("foo/bar", "foo"; "regular with path")] + #[test_case("foo/", "foo"; "trailing slash")] + #[test_case("foo/bar/baz", "foo"; "multiple slashes")] + fn test_get_package_name(import: &str, expected: &str) { + assert_eq!(Run::get_package_name(import), expected); + } +} From 3b92b67957d6d6eb4fe3696066cec89c8275649c Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Fri, 3 Jan 2025 16:17:23 -0500 Subject: [PATCH 16/23] More PR feedback --- crates/turborepo-lib/src/boundaries.rs | 5 ++--- crates/turborepo-lib/src/query/mod.rs | 5 +---- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/crates/turborepo-lib/src/boundaries.rs b/crates/turborepo-lib/src/boundaries.rs index 0d6e923ce5a29..c6766f04bc0b9 100644 --- a/crates/turborepo-lib/src/boundaries.rs +++ b/crates/turborepo-lib/src/boundaries.rs @@ -184,7 +184,6 @@ impl Run { // We assume the tsconfig.json is at the root of the package let tsconfig_path = package_root.join_component("tsconfig.json"); - // TODO: Load tsconfig.json let resolver = Tracer::create_resolver(tsconfig_path.exists().then(|| tsconfig_path.as_ref())); @@ -296,9 +295,9 @@ impl Run { } // We use `relation_to_path` and not `contains` because `contains` // panics on invalid paths with too many `..` components - if matches!( + if !matches!( package_path.relation_to_path(&resolved_import_path), - PathRelation::Divergent | PathRelation::Child + PathRelation::Parent ) { Ok(Some(BoundariesDiagnostic::ImportLeavesPackage { import: import.to_string(), diff --git a/crates/turborepo-lib/src/query/mod.rs b/crates/turborepo-lib/src/query/mod.rs index 905ba422ef316..b4283d2a9793b 100644 --- a/crates/turborepo-lib/src/query/mod.rs +++ b/crates/turborepo-lib/src/query/mod.rs @@ -1,6 +1,5 @@ - -mod external_package; mod boundaries; +mod external_package; mod file; mod package; mod server; @@ -15,7 +14,6 @@ use std::{ use async_graphql::{http::GraphiQLSource, *}; use axum::{response, response::IntoResponse}; use external_package::ExternalPackage; -use miette::Diagnostic; use package::Package; pub use server::run_server; use thiserror::Error; @@ -153,7 +151,6 @@ impl RepositoryQuery { #[graphql(concrete(name = "Packages", params(Package)))] #[graphql(concrete(name = "ChangedPackages", params(ChangedPackage)))] #[graphql(concrete(name = "Files", params(File)))] -#[graphql(concrete(name = "TraceErrors", params(file::TraceError)))] #[graphql(concrete(name = "ExternalPackages", params(ExternalPackage)))] #[graphql(concrete(name = "Diagnostics", params(Diagnostic)))] pub struct Array { From 8bd7e334a1634c3930fe5a27179faebb79c32759 Mon Sep 17 00:00:00 2001 From: Chris Olszewski Date: Tue, 14 Jan 2025 14:10:19 -0500 Subject: [PATCH 17/23] chore(boundaries): avoid npm package in test fixture --- .../integration/fixtures/boundaries/packages/utils/index.jsx | 1 + .../fixtures/boundaries/packages/utils/package.json | 3 +++ 2 files changed, 4 insertions(+) create mode 100644 turborepo-tests/integration/fixtures/boundaries/packages/utils/index.jsx create mode 100644 turborepo-tests/integration/fixtures/boundaries/packages/utils/package.json diff --git a/turborepo-tests/integration/fixtures/boundaries/packages/utils/index.jsx b/turborepo-tests/integration/fixtures/boundaries/packages/utils/index.jsx new file mode 100644 index 0000000000000..feaab5c7d7bef --- /dev/null +++ b/turborepo-tests/integration/fixtures/boundaries/packages/utils/index.jsx @@ -0,0 +1 @@ +export const ship = "Queen Anne"; diff --git a/turborepo-tests/integration/fixtures/boundaries/packages/utils/package.json b/turborepo-tests/integration/fixtures/boundaries/packages/utils/package.json new file mode 100644 index 0000000000000..6e3a19f2462e4 --- /dev/null +++ b/turborepo-tests/integration/fixtures/boundaries/packages/utils/package.json @@ -0,0 +1,3 @@ +{ + "name": "utils" +} From 074f41a147e987f3db188cf435cd8e63db807371 Mon Sep 17 00:00:00 2001 From: Chris Olszewski Date: Tue, 14 Jan 2025 14:19:32 -0500 Subject: [PATCH 18/23] chore(boundaries): fix lints --- crates/turbo-trace/src/import_finder.rs | 8 +++++--- crates/turborepo-lib/src/boundaries.rs | 11 ++++++----- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/crates/turbo-trace/src/import_finder.rs b/crates/turbo-trace/src/import_finder.rs index 6d15244042d29..52d1adb9e6b22 100644 --- a/crates/turbo-trace/src/import_finder.rs +++ b/crates/turbo-trace/src/import_finder.rs @@ -4,9 +4,11 @@ use swc_ecma_visit::{Visit, VisitWith}; use crate::tracer::ImportTraceType; -/// The type of import that we find, either an import with a `type` keyword -/// (indicating that it is importing only types) or an import without the `type` -/// keyword (indicating that it is importing values and possibly types). +/// The type of import that we find. +/// +/// Either an import with a `type` keyword (indicating that it is importing only +/// types) or an import without the `type` keyword (indicating that it is +/// importing values and possibly types). #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum ImportType { Type, diff --git a/crates/turborepo-lib/src/boundaries.rs b/crates/turborepo-lib/src/boundaries.rs index c6766f04bc0b9..c4b012962a732 100644 --- a/crates/turborepo-lib/src/boundaries.rs +++ b/crates/turborepo-lib/src/boundaries.rs @@ -113,7 +113,7 @@ impl BoundariesResult { impl Run { pub async fn check_boundaries(&self) -> Result { let packages = self.pkg_dep_graph().packages(); - let repo = Repository::discover(&self.repo_root()).ok().map(Mutex::new); + let repo = Repository::discover(self.repo_root()).ok().map(Mutex::new); let mut diagnostics = vec![]; let source_map = SourceMap::default(); for (package_name, package_info) in packages { @@ -248,10 +248,10 @@ impl Run { // We have a file import let check_result = if import.starts_with(".") { - self.check_file_import(&file_path, &package_root, &import, span, &file_content)? - } else if Self::is_potential_package_name(&import) { + self.check_file_import(&file_path, package_root, import, span, &file_content)? + } else if Self::is_potential_package_name(import) { self.check_package_import( - &import, + import, *import_type, span, &file_path, @@ -286,7 +286,7 @@ impl Run { let dir_path = file_path .parent() .ok_or_else(|| Error::NoParentDir(file_path.to_owned()))?; - let resolved_import_path = dir_path.join_unix_path(&import_path).clean()?; + let resolved_import_path = dir_path.join_unix_path(import_path).clean()?; // We have to check for this case because `relation_to_path` returns `Parent` if // the paths are equal and there's nothing wrong with importing the // package you're in. @@ -363,6 +363,7 @@ impl Run { } } + #[allow(clippy::too_many_arguments)] fn check_package_import( &self, import: &str, From 8a9f9cccfdb09e2fcd49f2d5a7f1be644af3ed85 Mon Sep 17 00:00:00 2001 From: Chris Olszewski Date: Tue, 14 Jan 2025 15:56:44 -0500 Subject: [PATCH 19/23] chore(boundaries): fix lint in test module --- crates/turborepo/tests/common/mod.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/crates/turborepo/tests/common/mod.rs b/crates/turborepo/tests/common/mod.rs index c8dc504d2ea39..dd81ac178f7dc 100644 --- a/crates/turborepo/tests/common/mod.rs +++ b/crates/turborepo/tests/common/mod.rs @@ -37,8 +37,9 @@ pub fn setup_fixture( Ok(()) } -/// Executes a command with different arguments in a specific fixture and -/// package manager and snapshots the output as JSON. +/// Executes a command and snapshots the output as JSON. +/// +/// Takes fixture, package manager, and command, and sets of arguments. /// Creates a snapshot file for each set of arguments. /// Note that the command must return valid JSON #[macro_export] @@ -46,7 +47,7 @@ macro_rules! check_json { ($fixture:expr, $package_manager:expr, $command:expr, $($name:expr => $query:expr,)*) => { { let tempdir = tempfile::tempdir()?; - crate::common::setup_fixture($fixture, $package_manager, tempdir.path())?; + $crate::common::setup_fixture($fixture, $package_manager, tempdir.path())?; $( println!("Running command: `turbo {} {}` in {}", $command, $query, $fixture); let output = assert_cmd::Command::cargo_bin("turbo")? From 444be331d2637f2d47ea601e6aaad88af436784a Mon Sep 17 00:00:00 2001 From: Chris Olszewski Date: Tue, 14 Jan 2025 16:08:01 -0500 Subject: [PATCH 20/23] fix(cli): bump stack size on windows --- crates/turborepo/build.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 crates/turborepo/build.rs diff --git a/crates/turborepo/build.rs b/crates/turborepo/build.rs new file mode 100644 index 0000000000000..13a55cac0b2c7 --- /dev/null +++ b/crates/turborepo/build.rs @@ -0,0 +1,13 @@ +const STACK_SIZE: usize = 8 * 1024 * 1024; + +fn main() { + // clap's proc macro uses stack allocated arrays for parsing cli args + // We have a large enough CLI that we attempt to put over 1MB onto the stack. + // This causes an issue on Windows where the default stack size is 1MB + // See + // https://learn.microsoft.com/en-us/windows/win32/procthread/thread-stack-size + // https://github.com/clap-rs/clap/issues/5134 + if std::env::var("CARGO_CFG_TARGET_ENV").ok().as_deref() == Some("msvc") { + println!("cargo:rustc-link-arg=/stack:{STACK_SIZE}"); + } +} From 58192759640a5cffd5824301608df176055466bb Mon Sep 17 00:00:00 2001 From: Chris Olszewski Date: Wed, 15 Jan 2025 08:35:10 -0500 Subject: [PATCH 21/23] temp fix --- crates/turborepo-lib/src/query/file.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/turborepo-lib/src/query/file.rs b/crates/turborepo-lib/src/query/file.rs index 713cd372d3970..b8f01d87f25b3 100644 --- a/crates/turborepo-lib/src/query/file.rs +++ b/crates/turborepo-lib/src/query/file.rs @@ -137,7 +137,8 @@ impl TraceResult { files: result .files .into_iter() - .sorted_by(|a, b| a.0.cmp(&b.0)) + .sorted_by_cached_key(|(path, _)| run.repo_root().anchor(path).unwrap()) + // .sorted_by(|a, b| a.0.cmp(&b.0)) .map(|(path, file)| Ok(File::new(run.clone(), path)?.with_ast(file.ast))) .collect::>()?, errors: result.errors.into_iter().map(|e| e.into()).collect(), From 91049afa473d259da8580af06e8a778120cb6a73 Mon Sep 17 00:00:00 2001 From: Chris Olszewski Date: Wed, 15 Jan 2025 10:11:08 -0500 Subject: [PATCH 22/23] fix(query): sort by real path --- crates/turborepo-lib/src/query/file.rs | 14 +++++++------- crates/turborepo-lib/src/query/mod.rs | 9 +++++++++ 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/crates/turborepo-lib/src/query/file.rs b/crates/turborepo-lib/src/query/file.rs index b8f01d87f25b3..0b4aac18b469b 100644 --- a/crates/turborepo-lib/src/query/file.rs +++ b/crates/turborepo-lib/src/query/file.rs @@ -133,14 +133,14 @@ struct TraceResult { impl TraceResult { fn new(result: turbo_trace::TraceResult, run: Arc) -> Result { + let mut files = result + .files + .into_iter() + .map(|(path, file)| Ok(File::new(run.clone(), path)?.with_ast(file.ast))) + .collect::, Error>>()?; + files.sort_by(|a, b| a.path.cmp(&b.path)); Ok(Self { - files: result - .files - .into_iter() - .sorted_by_cached_key(|(path, _)| run.repo_root().anchor(path).unwrap()) - // .sorted_by(|a, b| a.0.cmp(&b.0)) - .map(|(path, file)| Ok(File::new(run.clone(), path)?.with_ast(file.ast))) - .collect::>()?, + files: Array::from(files), errors: result.errors.into_iter().map(|e| e.into()).collect(), }) } diff --git a/crates/turborepo-lib/src/query/mod.rs b/crates/turborepo-lib/src/query/mod.rs index b4283d2a9793b..c8157249c88ff 100644 --- a/crates/turborepo-lib/src/query/mod.rs +++ b/crates/turborepo-lib/src/query/mod.rs @@ -158,6 +158,15 @@ pub struct Array { length: usize, } +impl From> for Array { + fn from(value: Vec) -> Self { + Self { + length: value.len(), + items: value, + } + } +} + impl Deref for Array { type Target = [T]; fn deref(&self) -> &Self::Target { From 0ac291dbab2c39246c5e88df0e6ffcac0a76d984 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Wed, 22 Jan 2025 11:45:37 +0530 Subject: [PATCH 23/23] revert oxc-resolver --- Cargo.lock | 8 ++++---- Cargo.toml | 2 +- crates/turbo-trace/src/tracer.rs | 11 ++++++++++- crates/turborepo-lib/src/query/file.rs | 1 - 4 files changed, 15 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4ce54e4c94904..8dd037cedb45f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3617,9 +3617,9 @@ dependencies = [ [[package]] name = "oxc_resolver" -version = "3.0.3" +version = "2.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bed381b6ab4bbfebfc7a011ad43b110ace8d201d02a39c0e09855f16b8f3f741" +checksum = "da33ec82d0f4770f4b5c120d6a1d8e45de2d99ae2672a7ee6bd29092ada945f2" dependencies = [ "cfg-if", "dashmap 6.1.0", @@ -6116,7 +6116,7 @@ dependencies = [ "futures", "globwalk", "miette", - "oxc_resolver 3.0.3", + "oxc_resolver 2.1.1", "swc_common", "swc_ecma_ast", "swc_ecma_parser", @@ -6427,7 +6427,7 @@ dependencies = [ "notify", "num_cpus", "owo-colors", - "oxc_resolver 3.0.3", + "oxc_resolver 2.1.1", "path-clean", "petgraph", "pidlock", diff --git a/Cargo.toml b/Cargo.toml index 93b1480790bd3..914fc54acf985 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -118,7 +118,7 @@ notify = "6.1.1" num_cpus = "1.15.0" once_cell = "1.17.1" owo-colors = "3.5.0" -oxc_resolver = { version = "3.0.3" } +oxc_resolver = { version = "2.1.0" } parking_lot = "0.12.1" path-clean = "1.0.1" pathdiff = "0.2.1" diff --git a/crates/turbo-trace/src/tracer.rs b/crates/turbo-trace/src/tracer.rs index 0ae9770e01869..da2a4b001c8ad 100644 --- a/crates/turbo-trace/src/tracer.rs +++ b/crates/turbo-trace/src/tracer.rs @@ -1,4 +1,4 @@ -use std::{collections::HashMap, sync::Arc}; +use std::{collections::HashMap, fmt, sync::Arc}; use camino::{Utf8Path, Utf8PathBuf}; use globwalk::WalkType; @@ -95,6 +95,15 @@ pub struct TraceResult { pub files: HashMap, } +impl fmt::Debug for TraceResult { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("TraceResult") + .field("files", &self.files) + .field("errors", &self.errors) + .finish() + } +} + /// The type of imports to trace. #[derive(Debug, Clone, Copy, PartialEq, Eq)] #[allow(dead_code)] diff --git a/crates/turborepo-lib/src/query/file.rs b/crates/turborepo-lib/src/query/file.rs index 0b4aac18b469b..49251173b6078 100644 --- a/crates/turborepo-lib/src/query/file.rs +++ b/crates/turborepo-lib/src/query/file.rs @@ -2,7 +2,6 @@ use std::sync::Arc; use async_graphql::{Enum, Object, SimpleObject}; use camino::Utf8PathBuf; -use itertools::Itertools; use miette::SourceCode; use swc_ecma_ast::EsVersion; use swc_ecma_parser::{EsSyntax, Syntax, TsSyntax};