这是indexloc提供的服务,不要输入任何密码
Skip to content

feat(boundaries): package name as tag punning #10151

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Mar 13, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions crates/turborepo-errors/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,12 @@ impl<T: Deserializable> Deserializable for Spanned<T> {
}
}

impl Spanned<String> {
pub fn as_str(&self) -> &str {
self.value.as_str()
}
}

Comment on lines +116 to +121
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is needed since Spanned already implements deref?

Suggested change
impl Spanned<String> {
pub fn as_str(&self) -> &str {
self.value.as_str()
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does Rust chain derefs? When I did as_deref for Option<Spanned<String>>, I got Option<&String> and not Option<&str>?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to specify the deref type sometimes to help out. I know I've needed to do that when working with Cow sometimes

impl<T> Spanned<T> {
pub fn new(t: T) -> Self {
Self {
Expand Down
20 changes: 20 additions & 0 deletions crates/turborepo-lib/src/boundaries/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<SourceSpan>,
#[source_code]
package_text: NamedSource<String>,
},
#[error("consider adding one of the following tags listed here")]
Allowlist {
#[label]
Expand All @@ -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<SourceSpan>,
#[source_code]
tag_text: NamedSource<String>,
#[related]
secondary: [SecondaryDiagnostic; 1],
},
#[error("Path `{path}` is not valid UTF-8. Turborepo only supports UTF-8 paths.")]
InvalidPath { path: String },
#[error(
Expand Down Expand Up @@ -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,
)?);
Expand Down
100 changes: 68 additions & 32 deletions crates/turborepo-lib/src/boundaries/tags.rs
Original file line number Diff line number Diff line change
@@ -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<String, ProcessedRule>;

pub struct ProcessedRule {
span: Spanned<()>,
dependencies: Option<ProcessedPermissions>,
dependents: Option<ProcessedPermissions>,
}

impl From<Rule> for ProcessedRule {
fn from(rule: Rule) -> Self {
impl From<Spanned<Rule>> for ProcessedRule {
fn from(rule: Spanned<Rule>) -> Self {
let (rule, span) = rule.split();
Self {
span,
dependencies: rule
.dependencies
.map(|dependencies| dependencies.into_inner().into()),
Expand Down Expand Up @@ -56,30 +61,6 @@ impl Run {
.and_then(|turbo_json| turbo_json.tags.as_ref())
}

pub(crate) fn get_package_tags(&self) -> HashMap<PackageName, Spanned<Vec<Spanned<String>>>> {
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<ProcessedRulesMap> {
self.root_turbo_json()
.boundaries
Expand All @@ -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()
})
}
Expand All @@ -99,14 +80,43 @@ impl Run {
fn validate_relation(
&self,
package_name: &PackageName,
package_json: &PackageJson,
relation_package_name: &PackageName,
tags: Option<&Spanned<Vec<Spanned<String>>>>,
allow_list: Option<&Spanned<HashSet<String>>>,
deny_list: Option<&Spanned<HashSet<String>>>,
) -> Result<Option<BoundariesDiagnostic>, 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 {
Expand Down Expand Up @@ -170,10 +180,34 @@ impl Run {
pub(crate) fn check_package_tags(
&self,
pkg: PackageNode,
package_json: &PackageJson,
current_package_tags: &Spanned<Vec<Spanned<String>>>,
tags_rules: &ProcessedRulesMap,
) -> Result<Vec<BoundariesDiagnostic>, 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 {
Expand All @@ -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(),
Expand All @@ -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(),
Expand Down
6 changes: 5 additions & 1 deletion crates/turborepo-lib/src/commands/prune.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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!(
Expand Down
2 changes: 1 addition & 1 deletion crates/turborepo-lib/src/engine/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion crates/turborepo-lib/src/engine/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -638,7 +638,7 @@ mod test {
};

let package = PackageJson {
name: Some(name.to_string()),
name: Some(Spanned::new(name.to_string())),
scripts,
..Default::default()
};
Expand Down
8 changes: 8 additions & 0 deletions crates/turborepo-lib/src/query/boundaries.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,14 @@ impl From<BoundariesDiagnostic> 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),
},
}
}
}
3 changes: 2 additions & 1 deletion crates/turborepo-lib/src/run/scope/filter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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()))
Expand Down
6 changes: 3 additions & 3 deletions crates/turborepo-lsp/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()
});
Expand Down Expand Up @@ -708,7 +708,7 @@ impl Backend {
{
Some("//".to_string())
} else {
package_json.name
package_json.name.map(|name| name.into_inner())
};
Some(
package_json
Expand Down
11 changes: 7 additions & 4 deletions crates/turborepo-repository/src/package_graph/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -590,6 +591,8 @@ impl PackageInfo {
mod test {
use std::assert_matches::assert_matches;

use turborepo_errors::Spanned;

use super::*;

struct MockDiscovery;
Expand Down Expand Up @@ -617,7 +620,7 @@ mod test {
let builder = PackageGraphBuilder::new(
&root,
PackageJson {
name: Some("root".into()),
name: Some(Spanned::new("root".into())),
..Default::default()
},
)
Expand All @@ -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()
},
);
Expand Down
10 changes: 7 additions & 3 deletions crates/turborepo-repository/src/package_graph/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,10 @@ pub struct PackageInfo {

impl PackageInfo {
pub fn package_name(&self) -> Option<String> {
self.package_json.name.clone()
self.package_json
.name
.as_ref()
.map(|name| name.as_inner().clone())
}

pub fn package_json_path(&self) -> &AnchoredSystemPath {
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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()
},
)
Expand Down
Loading
Loading