Skip to content
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

fix: checks for cyclic dependencies #3699

Merged
merged 9 commits into from
Jan 3, 2024
Merged
7 changes: 7 additions & 0 deletions test_programs/compile_failure/cyclic_dep/Nargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "cyclic_dep"
type = "bin"
authors = [""]

[dependencies]
dep1 = { path= "./dep1"}
Empty file.
7 changes: 7 additions & 0 deletions test_programs/compile_failure/cyclic_dep/dep1/Nargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "dep1"
type = "lib"
authors = [""]

[dependencies]
dep1 = { path= "../dep2"}
3 changes: 3 additions & 0 deletions test_programs/compile_failure/cyclic_dep/dep1/src/lib.nr
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
fn bar() {

}
7 changes: 7 additions & 0 deletions test_programs/compile_failure/cyclic_dep/dep2/Nargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "dep2"
type = "lib"
authors = [""]

[dependencies]
dep1 = { path= "../dep1"}
3 changes: 3 additions & 0 deletions test_programs/compile_failure/cyclic_dep/dep2/src/lib.nr
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
fn foo() {

}
7 changes: 7 additions & 0 deletions test_programs/compile_failure/cyclic_dep/src/main.nr
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
use dep1::foo;
use dep2::bar;

fn main() {
dep1::foo();
dep2::bar();
}
3 changes: 3 additions & 0 deletions tooling/nargo_toml/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,9 @@ pub enum ManifestError {

#[error(transparent)]
SemverError(SemverError),

#[error("Cyclic package dependency found when processing {cycle}")]
CyclicDependency { cycle: String },
}

#[allow(clippy::enum_variant_names)]
Expand Down
57 changes: 46 additions & 11 deletions tooling/nargo_toml/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,11 @@ struct PackageConfig {
}

impl PackageConfig {
fn resolve_to_package(&self, root_dir: &Path) -> Result<Package, ManifestError> {
fn resolve_to_package(
&self,
root_dir: &Path,
processed: &mut Vec<String>,
) -> Result<Package, ManifestError> {
let name: CrateName = if let Some(name) = &self.package.name {
name.parse().map_err(|_| ManifestError::InvalidPackageName {
toml: root_dir.join("Nargo.toml"),
Expand All @@ -136,7 +140,7 @@ impl PackageConfig {
toml: root_dir.join("Nargo.toml"),
name: name.into(),
})?;
let resolved_dep = dep_config.resolve_to_dependency(root_dir)?;
let resolved_dep = dep_config.resolve_to_dependency(root_dir, processed)?;

dependencies.insert(name, resolved_dep);
}
Expand Down Expand Up @@ -283,7 +287,11 @@ enum DependencyConfig {
}

impl DependencyConfig {
fn resolve_to_dependency(&self, pkg_root: &Path) -> Result<Dependency, ManifestError> {
fn resolve_to_dependency(
&self,
pkg_root: &Path,
processed: &mut Vec<String>,
) -> Result<Dependency, ManifestError> {
let dep = match self {
Self::Github { git, tag, directory } => {
let dir_path = clone_git_repo(git, tag).map_err(ManifestError::GitError)?;
Expand All @@ -300,13 +308,13 @@ impl DependencyConfig {
dir_path
};
let toml_path = project_path.join("Nargo.toml");
let package = resolve_package_from_toml(&toml_path)?;
let package = resolve_package_from_toml(&toml_path, processed)?;
Dependency::Remote { package }
}
Self::Path { path } => {
let dir_path = pkg_root.join(path);
let toml_path = dir_path.join("Nargo.toml");
let package = resolve_package_from_toml(&toml_path)?;
let package = resolve_package_from_toml(&toml_path, processed)?;
Dependency::Local { package }
}
};
Expand All @@ -325,9 +333,10 @@ fn toml_to_workspace(
nargo_toml: NargoToml,
package_selection: PackageSelection,
) -> Result<Workspace, ManifestError> {
let mut resolved = Vec::new();
let workspace = match nargo_toml.config {
Config::Package { package_config } => {
let member = package_config.resolve_to_package(&nargo_toml.root_dir)?;
let member = package_config.resolve_to_package(&nargo_toml.root_dir, &mut resolved)?;
match &package_selection {
PackageSelection::Selected(selected_name) if selected_name != &member.name => {
return Err(ManifestError::MissingSelectedPackage(member.name))
Expand All @@ -345,7 +354,7 @@ fn toml_to_workspace(
for (index, member_path) in workspace_config.members.into_iter().enumerate() {
let package_root_dir = nargo_toml.root_dir.join(&member_path);
let package_toml_path = package_root_dir.join("Nargo.toml");
let member = resolve_package_from_toml(&package_toml_path)?;
let member = resolve_package_from_toml(&package_toml_path, &mut resolved)?;

match &package_selection {
PackageSelection::Selected(selected_name) => {
Expand Down Expand Up @@ -402,17 +411,43 @@ fn read_toml(toml_path: &Path) -> Result<NargoToml, ManifestError> {
}

/// Resolves a Nargo.toml file into a `Package` struct as defined by our `nargo` core.
fn resolve_package_from_toml(toml_path: &Path) -> Result<Package, ManifestError> {
fn resolve_package_from_toml(
toml_path: &Path,
processed: &mut Vec<String>,
) -> Result<Package, ManifestError> {
// Checks for cyclic dependencies
let str_path = toml_path.to_str().expect("ICE - path is empty");
if processed.contains(&str_path.to_string()) {
let mut cycle = false;
let mut message = String::new();
for toml in processed {
cycle = cycle || toml == str_path;
if cycle {
message += &format!("{} referencing ", toml);
}
}
message += str_path;
return Err(ManifestError::CyclicDependency { cycle: message });
}
// Adds the package to the set of resolved packages
if let Some(str) = toml_path.to_str() {
processed.push(str.to_string());
}

let nargo_toml = read_toml(toml_path)?;

match nargo_toml.config {
let result = match nargo_toml.config {
Config::Package { package_config } => {
package_config.resolve_to_package(&nargo_toml.root_dir)
package_config.resolve_to_package(&nargo_toml.root_dir, processed)
}
Config::Workspace { .. } => {
Err(ManifestError::UnexpectedWorkspace(toml_path.to_path_buf()))
}
}
};
let pos =
processed.iter().position(|toml| toml == str_path).expect("added package must be here");
processed.remove(pos);
result
}

#[derive(Debug, PartialEq, Eq)]
Expand Down
Loading