From 284dedf23bb7df1cdceaba2ddadeb159a3c594d7 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Mon, 10 Feb 2025 18:55:24 -0500 Subject: [PATCH 1/3] Add ignore directive for boundaries --- crates/turborepo-lib/src/boundaries/mod.rs | 70 +++++++++++++++++-- .../fixtures/boundaries/apps/my-app/index.ts | 17 +++++ 2 files changed, 81 insertions(+), 6 deletions(-) diff --git a/crates/turborepo-lib/src/boundaries/mod.rs b/crates/turborepo-lib/src/boundaries/mod.rs index dca94afb477bc..070b06ede6337 100644 --- a/crates/turborepo-lib/src/boundaries/mod.rs +++ b/crates/turborepo-lib/src/boundaries/mod.rs @@ -13,7 +13,10 @@ use globwalk::Settings; use miette::{Diagnostic, NamedSource, Report, SourceSpan}; use regex::Regex; use swc_common::{ - comments::SingleThreadedComments, errors::Handler, input::StringInput, FileName, SourceMap, + comments::{Comments, SingleThreadedComments}, + errors::Handler, + input::StringInput, + FileName, SourceMap, Span, }; use swc_ecma_ast::EsVersion; use swc_ecma_parser::{lexer::Lexer, Capturing, EsSyntax, Parser, Syntax, TsSyntax}; @@ -133,6 +136,9 @@ static PACKAGE_NAME_REGEX: LazyLock = LazyLock::new(|| { Regex::new(r"^(@[a-z0-9-~][a-z0-9-._~]*\/)?[a-z0-9-~][a-z0-9-._~]*$").unwrap() }); +/// Maximum number of ignored import warnings to show +const MAX_IGNORE_WARNINGS: usize = 10; + pub struct BoundariesResult { files_checked: usize, packages_checked: usize, @@ -223,6 +229,19 @@ impl Run { }) } + /// Returns the underlying reason if an import has been marked as ignored + fn get_ignored_comment(comments: &SingleThreadedComments, span: Span) -> Option { + if let Some(import_comments) = comments.get_leading(span.lo()) { + for comment in import_comments { + if let Some(reason) = comment.text.trim().strip_prefix("@boundaries-ignore") { + return Some(reason.to_string()); + } + } + } + + None + } + /// Either returns a list of errors and number of files checked or a single, /// fatal error async fn check_package( @@ -295,8 +314,6 @@ impl Run { Settings::default().ignore_nested_packages(), )?; - let files_checked = files.len(); - 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"); @@ -305,7 +322,9 @@ impl Run { Tracer::create_resolver(tsconfig_path.exists().then(|| tsconfig_path.as_ref())); let mut not_supported_extensions = HashSet::new(); - for file_path in files { + let mut warning_count = 0; + + for file_path in &files { if let Some(ext @ ("svelte" | "vue")) = file_path.extension() { not_supported_extensions.insert(ext.to_string()); continue; @@ -364,6 +383,36 @@ impl Run { let mut finder = ImportFinder::default(); module.visit_with(&mut finder); for (import, span, import_type) in finder.imports() { + // If the import is prefixed with `@boundaries-ignore`, we ignore it, but print + // a warning + match Self::get_ignored_comment(&comments, *span) { + Some(reason) if reason.is_empty() => { + if warning_count <= MAX_IGNORE_WARNINGS { + warn!( + "@boundaries-ignore requires a reason, e.g. `// \ + @boundaries-ignore implicit dependency`" + ); + } + + warning_count += 1; + } + Some(_) => { + if warning_count <= MAX_IGNORE_WARNINGS { + // Try to get the line number for warning + let line = source_map.lookup_line(span.lo()).map(|l| l.line); + if let Ok(line) = line { + warn!("ignoring import on line {} in {}", line, file_path); + } else { + warn!("ignoring import in {}", file_path); + } + } + warning_count += 1; + + continue; + } + None => {} + } + let (start, end) = source_map.span_to_char_offset(&source_file, *span); let start = start as usize; let end = end as usize; @@ -394,16 +443,25 @@ impl Run { } } + if warning_count > MAX_IGNORE_WARNINGS { + warn!( + "and {} more ignored imports", + warning_count - MAX_IGNORE_WARNINGS + ); + } + for ext in ¬_supported_extensions { warn!( "{} files are currently not supported, boundaries checks will not apply to them", ext ); } - if !not_supported_extensions.is_empty() { + + let printed_warnings = warning_count > 0 || !not_supported_extensions.is_empty(); + if !printed_warnings { println!(); } - Ok((files_checked, diagnostics)) + Ok((files.len(), diagnostics)) } } diff --git a/turborepo-tests/integration/fixtures/boundaries/apps/my-app/index.ts b/turborepo-tests/integration/fixtures/boundaries/apps/my-app/index.ts index 2575ef917a4ef..41a3948a3f92f 100644 --- a/turborepo-tests/integration/fixtures/boundaries/apps/my-app/index.ts +++ b/turborepo-tests/integration/fixtures/boundaries/apps/my-app/index.ts @@ -5,3 +5,20 @@ import { Ship } from "ship"; import { Ship } from "@types/ship"; // Import package that is not specified import { walkThePlank } from "module-package"; + +// Import from a package that is not specified, but we have `@boundaries-ignore` on it +// @boundaries-ignore +import { walkThePlank } from "module-package"; + +// Import also works with other ignore directives +// @boundaries-ignore +// @ts-ignore +import { walkThePlank } from "module-package"; + +// import also works with whitespace +// @boundaries-ignore +import { walkThePlank } from "module-package"; + +// @boundaries-ignore + +import { walkThePlank } from "module-package"; From ae729c38d73ed154bb2cc5ae2abd69d181287372 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Tue, 11 Feb 2025 13:26:13 -0500 Subject: [PATCH 2/3] Responding to feedback by refactoring our warnings logic --- crates/turborepo-lib/src/boundaries/mod.rs | 129 ++++++++---------- .../fixtures/boundaries/apps/my-app/index.ts | 8 +- 2 files changed, 62 insertions(+), 75 deletions(-) diff --git a/crates/turborepo-lib/src/boundaries/mod.rs b/crates/turborepo-lib/src/boundaries/mod.rs index 070b06ede6337..a37783ad90f6f 100644 --- a/crates/turborepo-lib/src/boundaries/mod.rs +++ b/crates/turborepo-lib/src/boundaries/mod.rs @@ -136,12 +136,14 @@ static PACKAGE_NAME_REGEX: LazyLock = LazyLock::new(|| { Regex::new(r"^(@[a-z0-9-~][a-z0-9-._~]*\/)?[a-z0-9-~][a-z0-9-._~]*$").unwrap() }); -/// Maximum number of ignored import warnings to show -const MAX_IGNORE_WARNINGS: usize = 10; +/// Maximum number of warnings to show +const MAX_WARNINGS: usize = 16; +#[derive(Default)] pub struct BoundariesResult { files_checked: usize, packages_checked: usize, + warnings: Vec, pub source_map: Arc, pub diagnostics: Vec, } @@ -182,6 +184,13 @@ impl BoundariesResult { ) }; + for warning in self.warnings.iter().take(MAX_WARNINGS) { + warn!("{}", warning); + } + if !self.warnings.is_empty() { + eprintln!(); + } + println!( "Checked {} files in {} packages, {}", self.files_checked, self.packages_checked, result_message @@ -193,11 +202,10 @@ impl Run { pub async fn check_boundaries(&self) -> Result { let package_tags = self.get_package_tags(); let rules_map = self.get_processed_rules_map(); - let packages = self.pkg_dep_graph().packages(); + let packages: Vec<_> = self.pkg_dep_graph().packages().collect(); let repo = Repository::discover(self.repo_root()).ok().map(Mutex::new); - let mut diagnostics = vec![]; - let source_map = SourceMap::default(); - let mut total_files_checked = 0; + let mut result = BoundariesResult::default(); + for (package_name, package_info) in packages { if !self.filtered_pkgs().contains(package_name) || matches!(package_name, PackageName::Root) @@ -205,28 +213,18 @@ impl Run { continue; } - let (files_checked, package_diagnostics) = self - .check_package( - &repo, - package_name, - package_info, - &source_map, - &package_tags, - &rules_map, - ) - .await?; - - total_files_checked += files_checked; - diagnostics.extend(package_diagnostics); + self.check_package( + &repo, + package_name, + package_info, + &package_tags, + &rules_map, + &mut result, + ) + .await?; } - Ok(BoundariesResult { - files_checked: total_files_checked, - // Subtract 1 for the root package - packages_checked: self.pkg_dep_graph().len() - 1, - source_map: Arc::new(source_map), - diagnostics, - }) + Ok(result) } /// Returns the underlying reason if an import has been marked as ignored @@ -249,17 +247,16 @@ impl Run { repo: &Option>, package_name: &PackageName, package_info: &PackageInfo, - source_map: &SourceMap, all_package_tags: &HashMap>>>, tag_rules: &Option, - ) -> Result<(usize, Vec), Error> { - let (files_checked, mut diagnostics) = self - .check_package_files(repo, package_name, package_info, source_map) + result: &mut BoundariesResult, + ) -> Result<(), Error> { + self.check_package_files(repo, package_name, package_info, result) .await?; if let Some(current_package_tags) = all_package_tags.get(package_name) { if let Some(tag_rules) = tag_rules { - diagnostics.extend(self.check_package_tags( + result.diagnostics.extend(self.check_package_tags( PackageNode::Workspace(package_name.clone()), current_package_tags, all_package_tags, @@ -275,7 +272,9 @@ impl Run { } } - Ok((files_checked, diagnostics)) + result.packages_checked += 1; + + Ok(()) } fn is_potential_package_name(import: &str) -> bool { @@ -287,8 +286,8 @@ impl Run { repo: &Option>, package_name: &PackageName, package_info: &PackageInfo, - source_map: &SourceMap, - ) -> Result<(usize, Vec), Error> { + result: &mut BoundariesResult, + ) -> Result<(), Error> { let package_root = self.repo_root().resolve(package_info.package_path()); let internal_dependencies = self .pkg_dep_graph() @@ -314,7 +313,6 @@ impl Run { Settings::default().ignore_nested_packages(), )?; - 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"); @@ -322,7 +320,6 @@ impl Run { Tracer::create_resolver(tsconfig_path.exists().then(|| tsconfig_path.as_ref())); let mut not_supported_extensions = HashSet::new(); - let mut warning_count = 0; for file_path in &files { if let Some(ext @ ("svelte" | "vue")) = file_path.extension() { @@ -343,7 +340,7 @@ impl Run { let comments = SingleThreadedComments::default(); - let source_file = source_map.new_source_file( + let source_file = result.source_map.new_source_file( FileName::Custom(file_path.to_string()).into(), file_content.clone(), ); @@ -374,7 +371,9 @@ impl Run { let module = match parser.parse_module() { Ok(module) => module, Err(err) => { - diagnostics.push(BoundariesDiagnostic::ParseError(file_path.to_owned(), err)); + result + .diagnostics + .push(BoundariesDiagnostic::ParseError(file_path.to_owned(), err)); continue; } }; @@ -387,33 +386,31 @@ impl Run { // a warning match Self::get_ignored_comment(&comments, *span) { Some(reason) if reason.is_empty() => { - if warning_count <= MAX_IGNORE_WARNINGS { - warn!( - "@boundaries-ignore requires a reason, e.g. `// \ - @boundaries-ignore implicit dependency`" - ); - } - - warning_count += 1; + result.warnings.push( + "@boundaries-ignore requires a reason, e.g. `// @boundaries-ignore \ + implicit dependency`" + .to_string(), + ); } Some(_) => { - if warning_count <= MAX_IGNORE_WARNINGS { - // Try to get the line number for warning - let line = source_map.lookup_line(span.lo()).map(|l| l.line); - if let Ok(line) = line { - warn!("ignoring import on line {} in {}", line, file_path); - } else { - warn!("ignoring import in {}", file_path); - } + // Try to get the line number for warning + let line = result.source_map.lookup_line(span.lo()).map(|l| l.line); + if let Ok(line) = line { + result + .warnings + .push(format!("ignoring import on line {} in {}", line, file_path)); + } else { + result + .warnings + .push(format!("ignoring import in {}", file_path)); } - warning_count += 1; continue; } None => {} } - let (start, end) = source_map.span_to_char_offset(&source_file, *span); + let (start, end) = result.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); @@ -438,30 +435,20 @@ impl Run { }; if let Some(diagnostic) = check_result { - diagnostics.push(diagnostic); + result.diagnostics.push(diagnostic); } } } - if warning_count > MAX_IGNORE_WARNINGS { - warn!( - "and {} more ignored imports", - warning_count - MAX_IGNORE_WARNINGS - ); - } - for ext in ¬_supported_extensions { - warn!( + result.warnings.push(format!( "{} files are currently not supported, boundaries checks will not apply to them", ext - ); + )); } - let printed_warnings = warning_count > 0 || !not_supported_extensions.is_empty(); - if !printed_warnings { - println!(); - } + result.files_checked += files.len(); - Ok((files.len(), diagnostics)) + Ok(()) } } diff --git a/turborepo-tests/integration/fixtures/boundaries/apps/my-app/index.ts b/turborepo-tests/integration/fixtures/boundaries/apps/my-app/index.ts index 41a3948a3f92f..6baec29605d51 100644 --- a/turborepo-tests/integration/fixtures/boundaries/apps/my-app/index.ts +++ b/turborepo-tests/integration/fixtures/boundaries/apps/my-app/index.ts @@ -7,18 +7,18 @@ import { Ship } from "@types/ship"; import { walkThePlank } from "module-package"; // Import from a package that is not specified, but we have `@boundaries-ignore` on it -// @boundaries-ignore +// @boundaries-ignore this is a test import { walkThePlank } from "module-package"; // Import also works with other ignore directives -// @boundaries-ignore +// @boundaries-ignore this is a test // @ts-ignore import { walkThePlank } from "module-package"; // import also works with whitespace -// @boundaries-ignore +// @boundaries-ignore here's another reason import { walkThePlank } from "module-package"; -// @boundaries-ignore +// @boundaries-ignore one more reason import { walkThePlank } from "module-package"; From fa656e8cb3a6ee6bb2f6bff3f8ff2941655e2024 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Tue, 11 Feb 2025 13:54:21 -0500 Subject: [PATCH 3/3] Clippy --- crates/turborepo-lib/src/boundaries/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/turborepo-lib/src/boundaries/mod.rs b/crates/turborepo-lib/src/boundaries/mod.rs index a37783ad90f6f..8c4c47c25664e 100644 --- a/crates/turborepo-lib/src/boundaries/mod.rs +++ b/crates/turborepo-lib/src/boundaries/mod.rs @@ -417,13 +417,13 @@ 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, *import_type, span, - &file_path, + file_path, &file_content, &package_info.package_json, &internal_dependencies,