diff --git a/crates/turborepo-errors/src/lib.rs b/crates/turborepo-errors/src/lib.rs index c0c4806765392..400685d7c8363 100644 --- a/crates/turborepo-errors/src/lib.rs +++ b/crates/turborepo-errors/src/lib.rs @@ -113,6 +113,12 @@ impl Deserializable for Spanned { } } +impl Spanned { + pub fn as_str(&self) -> &str { + self.value.as_str() + } +} + impl Spanned { pub fn new(t: T) -> Self { Self { diff --git a/crates/turborepo-lib/src/boundaries/mod.rs b/crates/turborepo-lib/src/boundaries/mod.rs index 25ff4f7278e68..df332762bdcfa 100644 --- a/crates/turborepo-lib/src/boundaries/mod.rs +++ b/crates/turborepo-lib/src/boundaries/mod.rs @@ -41,6 +41,14 @@ use crate::{ #[derive(Clone, Debug, Error, Diagnostic)] pub enum SecondaryDiagnostic { + #[error("package `{package} is defined here")] + PackageDefinedHere { + package: String, + #[label] + package_span: Option, + #[source_code] + package_text: NamedSource, + }, #[error("consider adding one of the following tags listed here")] Allowlist { #[label] @@ -59,6 +67,17 @@ pub enum SecondaryDiagnostic { #[derive(Clone, Debug, Error, Diagnostic)] pub enum BoundariesDiagnostic { + #[error("Tag `{tag}` cannot share the same name as package `{package}`")] + TagSharesPackageName { + tag: String, + package: String, + #[label("tag defined here")] + tag_span: Option, + #[source_code] + tag_text: NamedSource, + #[related] + secondary: [SecondaryDiagnostic; 1], + }, #[error("Path `{path}` is not valid UTF-8. Turborepo only supports UTF-8 paths.")] InvalidPath { path: String }, #[error( @@ -320,6 +339,7 @@ impl Run { if let Some(tag_rules) = tag_rules { result.diagnostics.extend(self.check_package_tags( PackageNode::Workspace(package_name.clone()), + &package_info.package_json, tags, tag_rules, )?); diff --git a/crates/turborepo-lib/src/boundaries/tags.rs b/crates/turborepo-lib/src/boundaries/tags.rs index b86a58aea9a4f..98b19e7cfd7dd 100644 --- a/crates/turborepo-lib/src/boundaries/tags.rs +++ b/crates/turborepo-lib/src/boundaries/tags.rs @@ -1,25 +1,30 @@ use std::collections::{HashMap, HashSet}; -use tracing::warn; +use miette::NamedSource; use turborepo_errors::Spanned; -use turborepo_repository::package_graph::{PackageName, PackageNode}; +use turborepo_repository::{ + package_graph::{PackageName, PackageNode}, + package_json::PackageJson, +}; use crate::{ boundaries::{config::Rule, BoundariesDiagnostic, Error, Permissions, SecondaryDiagnostic}, run::Run, - turbo_json::TurboJson, }; pub type ProcessedRulesMap = HashMap; pub struct ProcessedRule { + span: Spanned<()>, dependencies: Option, dependents: Option, } -impl From for ProcessedRule { - fn from(rule: Rule) -> Self { +impl From> for ProcessedRule { + fn from(rule: Spanned) -> Self { + let (rule, span) = rule.split(); Self { + span, dependencies: rule .dependencies .map(|dependencies| dependencies.into_inner().into()), @@ -56,30 +61,6 @@ impl Run { .and_then(|turbo_json| turbo_json.tags.as_ref()) } - pub(crate) fn get_package_tags(&self) -> HashMap>>> { - let mut package_tags = HashMap::new(); - for (package, _) in self.pkg_dep_graph().packages() { - if let Ok(TurboJson { - tags: Some(tags), - boundaries, - .. - }) = self.turbo_json_loader().load(package) - { - if boundaries.as_ref().is_some_and(|b| b.tags.is_some()) - && !matches!(package, PackageName::Root) - { - warn!( - "Boundaries rules can only be defined in the root turbo.json. Any rules \ - defined in a package's turbo.json will be ignored." - ) - } - package_tags.insert(package.clone(), tags.clone()); - } - } - - package_tags - } - pub(crate) fn get_processed_rules_map(&self) -> Option { self.root_turbo_json() .boundaries @@ -88,7 +69,7 @@ impl Run { .map(|tags| { tags.as_inner() .iter() - .map(|(k, v)| (k.clone(), v.as_inner().clone().into())) + .map(|(k, v)| (k.clone(), v.clone().into())) .collect() }) } @@ -99,14 +80,43 @@ impl Run { fn validate_relation( &self, package_name: &PackageName, + package_json: &PackageJson, relation_package_name: &PackageName, tags: Option<&Spanned>>>, allow_list: Option<&Spanned>>, deny_list: Option<&Spanned>>, ) -> Result, Error> { - // If there is no allow list, then we vacuously have a tag in the allow list - let mut has_tag_in_allowlist = allow_list.is_none(); + // We allow "punning" the package name as a tag, so if the allow list contains + // the package name, then we have a tag in the allow list + // Likewise, if the allow list is empty, then we vacuously have a tag in the + // allow list + let mut has_tag_in_allowlist = + allow_list.is_none_or(|allow_list| allow_list.contains(relation_package_name.as_str())); let tags_span = tags.map(|tags| tags.to(())).unwrap_or_default(); + if let Some(deny_list) = deny_list { + if deny_list.contains(relation_package_name.as_str()) { + let (span, text) = package_json + .name + .as_ref() + .map(|name| name.span_and_text("turbo.json")) + .unwrap_or_else(|| (None, NamedSource::new("package.json", String::new()))); + let deny_list_spanned = deny_list.to(()); + let (deny_list_span, deny_list_text) = + deny_list_spanned.span_and_text("turbo.json"); + + return Ok(Some(BoundariesDiagnostic::DeniedTag { + source_package_name: package_name.clone(), + package_name: relation_package_name.clone(), + tag: relation_package_name.to_string(), + span, + text, + secondary: [SecondaryDiagnostic::Denylist { + span: deny_list_span, + text: deny_list_text, + }], + })); + } + } for tag in tags.into_iter().flatten().flatten() { if let Some(allow_list) = allow_list { @@ -170,10 +180,34 @@ impl Run { pub(crate) fn check_package_tags( &self, pkg: PackageNode, + package_json: &PackageJson, current_package_tags: &Spanned>>, tags_rules: &ProcessedRulesMap, ) -> Result, Error> { let mut diagnostics = Vec::new(); + + // We don't allow tags to share the same name as the package because + // we allow package names to be used as a tag + if let Some(rule) = tags_rules.get(pkg.as_package_name().as_str()) { + let (tag_span, tag_text) = rule.span.span_and_text("turbo.json"); + let (package_span, package_text) = package_json + .name + .as_ref() + .map(|name| name.span_and_text("package.json")) + .unwrap_or_else(|| (None, NamedSource::new("package.json", "".into()))); + diagnostics.push(BoundariesDiagnostic::TagSharesPackageName { + tag: pkg.as_package_name().to_string(), + package: pkg.as_package_name().to_string(), + tag_span, + tag_text, + secondary: [SecondaryDiagnostic::PackageDefinedHere { + package: pkg.as_package_name().to_string(), + package_span, + package_text, + }], + }); + } + for tag in current_package_tags.iter() { if let Some(rule) = tags_rules.get(tag.as_inner()) { if let Some(dependency_permissions) = &rule.dependencies { @@ -186,6 +220,7 @@ impl Run { diagnostics.extend(self.validate_relation( pkg.as_package_name(), + package_json, dependency.as_package_name(), dependency_tags, dependency_permissions.allow.as_ref(), @@ -202,6 +237,7 @@ impl Run { let dependent_tags = self.load_package_tags(dependent.as_package_name()); diagnostics.extend(self.validate_relation( pkg.as_package_name(), + package_json, dependent.as_package_name(), dependent_tags, dependent_permissions.allow.as_ref(), diff --git a/crates/turborepo-lib/src/commands/prune.rs b/crates/turborepo-lib/src/commands/prune.rs index e172381956fc5..ad05a5d9c0d77 100644 --- a/crates/turborepo-lib/src/commands/prune.rs +++ b/crates/turborepo-lib/src/commands/prune.rs @@ -307,7 +307,11 @@ impl<'a> Prune<'a> { }; trace!( "target: {}", - info.package_json.name.as_deref().unwrap_or_default() + info.package_json + .name + .as_ref() + .map(|name| name.as_str()) + .unwrap_or_default() ); trace!("workspace package.json: {}", &info.package_json_path); trace!( diff --git a/crates/turborepo-lib/src/engine/builder.rs b/crates/turborepo-lib/src/engine/builder.rs index 1157c7e4714e3..bf2976d2923e9 100644 --- a/crates/turborepo-lib/src/engine/builder.rs +++ b/crates/turborepo-lib/src/engine/builder.rs @@ -760,7 +760,7 @@ mod test { $( let path = $root.join_components(&["packages", $name, "package.json"]); let dependencies = Some($deps.iter().map(|dep: &&str| (dep.to_string(), "workspace:*".to_string())).collect()); - let package_json = PackageJson { name: Some($name.to_string()), dependencies, ..Default::default() }; + let package_json = PackageJson { name: Some(Spanned::new($name.to_string())), dependencies, ..Default::default() }; _map.insert(path, package_json); )+ _map diff --git a/crates/turborepo-lib/src/engine/mod.rs b/crates/turborepo-lib/src/engine/mod.rs index 075fc7d31c2ed..645c9a9b2b1d6 100644 --- a/crates/turborepo-lib/src/engine/mod.rs +++ b/crates/turborepo-lib/src/engine/mod.rs @@ -638,7 +638,7 @@ mod test { }; let package = PackageJson { - name: Some(name.to_string()), + name: Some(Spanned::new(name.to_string())), scripts, ..Default::default() }; diff --git a/crates/turborepo-lib/src/query/boundaries.rs b/crates/turborepo-lib/src/query/boundaries.rs index df5100f82f523..0642ade33a727 100644 --- a/crates/turborepo-lib/src/query/boundaries.rs +++ b/crates/turborepo-lib/src/query/boundaries.rs @@ -92,6 +92,14 @@ impl From for Diagnostic { import: None, reason: None, }, + BoundariesDiagnostic::TagSharesPackageName { tag, tag_span, .. } => Diagnostic { + message, + path: None, + start: tag_span.map(|span| span.offset()), + end: tag_span.map(|span| span.offset() + span.len()), + import: None, + reason: Some(tag), + }, } } } diff --git a/crates/turborepo-lib/src/run/scope/filter.rs b/crates/turborepo-lib/src/run/scope/filter.rs index d465b112807a4..8877679ad6dbc 100644 --- a/crates/turborepo-lib/src/run/scope/filter.rs +++ b/crates/turborepo-lib/src/run/scope/filter.rs @@ -713,6 +713,7 @@ mod test { use tempfile::TempDir; use test_case::test_case; use turbopath::{AbsoluteSystemPathBuf, AnchoredSystemPathBuf, RelativeUnixPathBuf}; + use turborepo_errors::Spanned; use turborepo_repository::{ change_mapper::PackageInclusionReason, discovery::PackageDiscovery, @@ -806,7 +807,7 @@ mod test { RelativeUnixPathBuf::new(format!("{package_path}/package.json")).unwrap(), ), PackageJson { - name: Some(name.to_string()), + name: Some(Spanned::new(name.to_string())), dependencies: dependencies.get(name).map(|v| { v.iter() .map(|name| (name.to_string(), "*".to_string())) diff --git a/crates/turborepo-lsp/src/lib.rs b/crates/turborepo-lsp/src/lib.rs index e24ca17907b24..729fcec1058a3 100644 --- a/crates/turborepo-lsp/src/lib.rs +++ b/crates/turborepo-lsp/src/lib.rs @@ -325,7 +325,7 @@ impl LanguageServer for Backend { let package_json_name = if repo_root.contains(&wd.package_json) { Some("//") } else { - package_json.name.as_deref() + package_json.name.as_ref().map(|name| name.as_str()) }; // todo: use jsonc_ast instead of text search @@ -613,7 +613,7 @@ impl LanguageServer for Backend { .iter() .flat_map(|p| p.scripts.keys().map(move |k| (p.name.clone(), k))) .map(|(package, s)| CompletionItem { - label: format!("{}#{}", package.unwrap_or_default(), s), + label: format!("{}#{}", package.unwrap_or_default().into_inner(), s), kind: Some(CompletionItemKind::FIELD), ..Default::default() }); @@ -708,7 +708,7 @@ impl Backend { { Some("//".to_string()) } else { - package_json.name + package_json.name.map(|name| name.into_inner()) }; Some( package_json diff --git a/crates/turborepo-repository/src/package_graph/builder.rs b/crates/turborepo-repository/src/package_graph/builder.rs index 5be2c2945a8b9..406d1f1e7180b 100644 --- a/crates/turborepo-repository/src/package_graph/builder.rs +++ b/crates/turborepo-repository/src/package_graph/builder.rs @@ -237,7 +237,8 @@ impl<'a, T: PackageDiscovery> BuildState<'a, ResolvedPackageManager, T> { let name = PackageName::Other( json.name .clone() - .ok_or(Error::PackageJsonMissingName(package_json_path))?, + .ok_or(Error::PackageJsonMissingName(package_json_path))? + .into_inner(), ); let entry = PackageInfo { package_json: json, @@ -590,6 +591,8 @@ impl PackageInfo { mod test { use std::assert_matches::assert_matches; + use turborepo_errors::Spanned; + use super::*; struct MockDiscovery; @@ -617,7 +620,7 @@ mod test { let builder = PackageGraphBuilder::new( &root, PackageJson { - name: Some("root".into()), + name: Some(Spanned::new("root".into())), ..Default::default() }, ) @@ -627,14 +630,14 @@ mod test { map.insert( root.join_component("a"), PackageJson { - name: Some("foo".into()), + name: Some(Spanned::new("foo".into())), ..Default::default() }, ); map.insert( root.join_component("b"), PackageJson { - name: Some("foo".into()), + name: Some(Spanned::new("foo".into())), ..Default::default() }, ); diff --git a/crates/turborepo-repository/src/package_graph/mod.rs b/crates/turborepo-repository/src/package_graph/mod.rs index 446332aef78dc..44e5d6508df33 100644 --- a/crates/turborepo-repository/src/package_graph/mod.rs +++ b/crates/turborepo-repository/src/package_graph/mod.rs @@ -70,7 +70,10 @@ pub struct PackageInfo { impl PackageInfo { pub fn package_name(&self) -> Option { - self.package_json.name.clone() + self.package_json + .name + .as_ref() + .map(|name| name.as_inner().clone()) } pub fn package_json_path(&self) -> &AnchoredSystemPath { @@ -156,7 +159,7 @@ impl PackageGraph { if matches!(package_name, PackageName::Root) { continue; } - let name = info.package_json.name.as_deref(); + let name = info.package_json.name.as_ref().map(|name| name.as_str()); match name { Some("") | None => { let package_json_path = self.repo_root.resolve(info.package_json_path()); @@ -620,6 +623,7 @@ mod test { use std::assert_matches::assert_matches; use serde_json::json; + use turborepo_errors::Spanned; use super::*; use crate::discovery::PackageDiscovery; @@ -649,7 +653,7 @@ mod test { let pkg_graph = PackageGraph::builder( &root, PackageJson { - name: Some("my-package".to_owned()), + name: Some(Spanned::new("my-package".to_owned())), ..Default::default() }, ) diff --git a/crates/turborepo-repository/src/package_json.rs b/crates/turborepo-repository/src/package_json.rs index 1f0b3b2335a60..5b143872fdc68 100644 --- a/crates/turborepo-repository/src/package_json.rs +++ b/crates/turborepo-repository/src/package_json.rs @@ -18,7 +18,7 @@ use turborepo_unescape::UnescapedString; #[serde(rename_all = "camelCase")] pub struct PackageJson { #[serde(skip_serializing_if = "Option::is_none")] - pub name: Option, + pub name: Option>, #[serde(skip_serializing_if = "Option::is_none")] pub version: Option, #[serde(skip_serializing_if = "Option::is_none")] @@ -54,7 +54,7 @@ pub struct PnpmConfig { #[derive(Debug, Clone, Default, PartialEq, Eq, Deserializable)] pub struct RawPackageJson { - pub name: Option, + pub name: Option>, pub version: Option, pub package_manager: Option>, pub dependencies: Option>, @@ -111,7 +111,7 @@ impl WithMetadata for RawPackageJson { impl From for PackageJson { fn from(raw: RawPackageJson) -> Self { Self { - name: raw.name.map(|s| s.into()), + name: raw.name.map(|s| s.map(|s| s.into())), version: raw.version.map(|s| s.into()), package_manager: raw.package_manager.map(|s| s.map(|s| s.into())), dependencies: raw diff --git a/crates/turborepo/tests/snapshots/boundaries__boundaries_tags_get_boundaries_lints_(npm@10.5.0).snap b/crates/turborepo/tests/snapshots/boundaries__boundaries_tags_get_boundaries_lints_(npm@10.5.0).snap index 1819338c8d9dc..b9302e766ca9f 100644 --- a/crates/turborepo/tests/snapshots/boundaries__boundaries_tags_get_boundaries_lints_(npm@10.5.0).snap +++ b/crates/turborepo/tests/snapshots/boundaries__boundaries_tags_get_boundaries_lints_(npm@10.5.0).snap @@ -21,6 +21,10 @@ expression: query_output { "message": "Package `@vercel/not-allowed-dependent` found without any tag listed in allowlist for `@vercel/my-app`", "import": "@vercel/not-allowed-dependent" + }, + { + "message": "Package `@vercel/package-as-tag` found with tag listed in denylist for `@vercel/allowed-tag`: `@vercel/package-as-tag`", + "import": "@vercel/package-as-tag" } ] } diff --git a/docs/site/content/repo-docs/reference/boundaries.mdx b/docs/site/content/repo-docs/reference/boundaries.mdx index 4ebf993de3c05..b8d401191572e 100644 --- a/docs/site/content/repo-docs/reference/boundaries.mdx +++ b/docs/site/content/repo-docs/reference/boundaries.mdx @@ -83,6 +83,22 @@ Likewise, you can add restrictions for a tag's dependents, i.e. packages that im } ``` +Package names can also be used in place of a tag in allow and deny lists. + +```json title="turbo.json" +{ + "boundaries": { + "tags": { + "private": { + "dependents": { + "deny": ["@repo/my-pkg"] + } + } + } + } +} +``` + Tags allow you to ensure that the wrong package isn't getting imported somewhere in your graph. These rules are applied even for dependencies of dependencies, so if you import a package that in turn imports another package with a denied tag, you will still get a rule violation. diff --git a/packages/turbo-repository/rust/src/internal.rs b/packages/turbo-repository/rust/src/internal.rs index 2bd5c72d58a3a..770fea1b05817 100644 --- a/packages/turbo-repository/rust/src/internal.rs +++ b/packages/turbo-repository/rust/src/internal.rs @@ -132,7 +132,11 @@ impl Workspace { // package_json_path) path.parent() .map(|package_path| { - Ok(Package::new(name, &self.workspace_state.root, package_path)) + Ok(Package::new( + name.into_inner(), + &self.workspace_state.root, + package_path, + )) }) .or_else(|| Some(Err(Error::MissingParent(path.to_owned())))) }) diff --git a/turborepo-tests/integration/fixtures/boundaries_tags/apps/my-app/package.json b/turborepo-tests/integration/fixtures/boundaries_tags/apps/my-app/package.json index c0989cf724c93..c6408a57e8442 100644 --- a/turborepo-tests/integration/fixtures/boundaries_tags/apps/my-app/package.json +++ b/turborepo-tests/integration/fixtures/boundaries_tags/apps/my-app/package.json @@ -6,6 +6,7 @@ }, "dependencies": { "@vercel/allowed-tag": "*", - "@vercel/allowed-and-denied-tag": "*" + "@vercel/allowed-and-denied-tag": "*", + "@vercel/package-as-tag": "*" } } diff --git a/turborepo-tests/integration/fixtures/boundaries_tags/packages/allowed-tag/package.json b/turborepo-tests/integration/fixtures/boundaries_tags/packages/allowed-tag/package.json index dc8c1450a92d4..8840761734b20 100644 --- a/turborepo-tests/integration/fixtures/boundaries_tags/packages/allowed-tag/package.json +++ b/turborepo-tests/integration/fixtures/boundaries_tags/packages/allowed-tag/package.json @@ -2,6 +2,7 @@ "name": "@vercel/allowed-tag", "module": "my-module.mjs", "dependencies": { - "@vercel/no-allowlist": "*" + "@vercel/no-allowlist": "*", + "@vercel/package-as-tag": "*" } } \ No newline at end of file diff --git a/turborepo-tests/integration/fixtures/boundaries_tags/packages/package-as-tag/package.json b/turborepo-tests/integration/fixtures/boundaries_tags/packages/package-as-tag/package.json new file mode 100644 index 0000000000000..6e0958d350fa4 --- /dev/null +++ b/turborepo-tests/integration/fixtures/boundaries_tags/packages/package-as-tag/package.json @@ -0,0 +1,3 @@ +{ + "name": "@vercel/package-as-tag" +} \ No newline at end of file diff --git a/turborepo-tests/integration/fixtures/boundaries_tags/turbo.json b/turborepo-tests/integration/fixtures/boundaries_tags/turbo.json index becf2c42b2707..195cdbe07a89f 100644 --- a/turborepo-tests/integration/fixtures/boundaries_tags/turbo.json +++ b/turborepo-tests/integration/fixtures/boundaries_tags/turbo.json @@ -4,7 +4,8 @@ "web": { "dependencies": { "allow": [ - "types" + "types", + "@vercel/package-as-tag" ], "deny": [ "unsafe" @@ -15,6 +16,11 @@ } }, "types": { + "dependencies": { + "deny": [ + "@vercel/package-as-tag" + ] + }, "dependents": { "allow": [ "web"