From a5d2a1d3dcaaa78830d788fa3fdee73b3aa5e3fc Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Wed, 24 Apr 2024 23:19:37 -0400 Subject: [PATCH 1/6] refactor(toml): extract dependency-to-source-id to function --- src/cargo/util/toml/mod.rs | 160 ++++++++++++++++++++----------------- 1 file changed, 85 insertions(+), 75 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 60e7104d45b..77c3a08a47e 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -1810,6 +1810,90 @@ fn detailed_dep_to_dependency( } } + let new_source_id = to_dependency_source_id(orig, name_in_toml, manifest_ctx)?; + + let (pkg_name, explicit_name_in_toml) = match orig.package { + Some(ref s) => (&s[..], Some(name_in_toml)), + None => (name_in_toml, None), + }; + + let version = orig.version.as_deref(); + let mut dep = Dependency::parse(pkg_name, version, new_source_id)?; + deprecated_underscore( + &orig.default_features2, + &orig.default_features, + "default-features", + name_in_toml, + "dependency", + manifest_ctx.warnings, + ); + dep.set_features(orig.features.iter().flatten()) + .set_default_features(orig.default_features().unwrap_or(true)) + .set_optional(orig.optional.unwrap_or(false)) + .set_platform(manifest_ctx.platform.clone()); + if let Some(registry) = &orig.registry { + let registry_id = SourceId::alt_registry(manifest_ctx.gctx, registry)?; + dep.set_registry_id(registry_id); + } + if let Some(registry_index) = &orig.registry_index { + let url = registry_index.into_url()?; + let registry_id = SourceId::for_registry(&url)?; + dep.set_registry_id(registry_id); + } + + if let Some(kind) = kind { + dep.set_kind(kind); + } + if let Some(name_in_toml) = explicit_name_in_toml { + dep.set_explicit_name_in_toml(name_in_toml); + } + + if let Some(p) = orig.public { + dep.set_public(p); + } + + if let (Some(artifact), is_lib, target) = ( + orig.artifact.as_ref(), + orig.lib.unwrap_or(false), + orig.target.as_deref(), + ) { + if manifest_ctx.gctx.cli_unstable().bindeps { + let artifact = Artifact::parse(&artifact.0, is_lib, target)?; + if dep.kind() != DepKind::Build + && artifact.target() == Some(ArtifactTarget::BuildDependencyAssumeTarget) + { + bail!( + r#"`target = "target"` in normal- or dev-dependencies has no effect ({})"#, + name_in_toml + ); + } + dep.set_artifact(artifact) + } else { + bail!("`artifact = …` requires `-Z bindeps` ({})", name_in_toml); + } + } else if orig.lib.is_some() || orig.target.is_some() { + for (is_set, specifier) in [ + (orig.lib.is_some(), "lib"), + (orig.target.is_some(), "target"), + ] { + if !is_set { + continue; + } + bail!( + "'{}' specifier cannot be used without an 'artifact = …' value ({})", + specifier, + name_in_toml + ) + } + } + Ok(dep) +} + +fn to_dependency_source_id( + orig: &manifest::TomlDetailedDependency

, + name_in_toml: &str, + manifest_ctx: &mut ManifestContext<'_, '_>, +) -> CargoResult { let new_source_id = match ( orig.git.as_ref(), orig.path.as_ref(), @@ -1895,81 +1979,7 @@ fn detailed_dep_to_dependency( (None, None, None, None) => SourceId::crates_io(manifest_ctx.gctx)?, }; - let (pkg_name, explicit_name_in_toml) = match orig.package { - Some(ref s) => (&s[..], Some(name_in_toml)), - None => (name_in_toml, None), - }; - - let version = orig.version.as_deref(); - let mut dep = Dependency::parse(pkg_name, version, new_source_id)?; - deprecated_underscore( - &orig.default_features2, - &orig.default_features, - "default-features", - name_in_toml, - "dependency", - manifest_ctx.warnings, - ); - dep.set_features(orig.features.iter().flatten()) - .set_default_features(orig.default_features().unwrap_or(true)) - .set_optional(orig.optional.unwrap_or(false)) - .set_platform(manifest_ctx.platform.clone()); - if let Some(registry) = &orig.registry { - let registry_id = SourceId::alt_registry(manifest_ctx.gctx, registry)?; - dep.set_registry_id(registry_id); - } - if let Some(registry_index) = &orig.registry_index { - let url = registry_index.into_url()?; - let registry_id = SourceId::for_registry(&url)?; - dep.set_registry_id(registry_id); - } - - if let Some(kind) = kind { - dep.set_kind(kind); - } - if let Some(name_in_toml) = explicit_name_in_toml { - dep.set_explicit_name_in_toml(name_in_toml); - } - - if let Some(p) = orig.public { - dep.set_public(p); - } - - if let (Some(artifact), is_lib, target) = ( - orig.artifact.as_ref(), - orig.lib.unwrap_or(false), - orig.target.as_deref(), - ) { - if manifest_ctx.gctx.cli_unstable().bindeps { - let artifact = Artifact::parse(&artifact.0, is_lib, target)?; - if dep.kind() != DepKind::Build - && artifact.target() == Some(ArtifactTarget::BuildDependencyAssumeTarget) - { - bail!( - r#"`target = "target"` in normal- or dev-dependencies has no effect ({})"#, - name_in_toml - ); - } - dep.set_artifact(artifact) - } else { - bail!("`artifact = …` requires `-Z bindeps` ({})", name_in_toml); - } - } else if orig.lib.is_some() || orig.target.is_some() { - for (is_set, specifier) in [ - (orig.lib.is_some(), "lib"), - (orig.target.is_some(), "target"), - ] { - if !is_set { - continue; - } - bail!( - "'{}' specifier cannot be used without an 'artifact = …' value ({})", - specifier, - name_in_toml - ) - } - } - Ok(dep) + Ok(new_source_id) } pub trait ResolveToPath { From 3505b057924e3e75ed99392373eb20e23ef9bfde Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Wed, 24 Apr 2024 23:32:31 -0400 Subject: [PATCH 2/6] refactor(toml): show name with underscore for readability --- src/cargo/util/toml/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 77c3a08a47e..d15bc31f0c4 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -1897,15 +1897,15 @@ fn to_dependency_source_id( let new_source_id = match ( orig.git.as_ref(), orig.path.as_ref(), - orig.registry.as_ref(), + orig.registry.as_deref(), orig.registry_index.as_ref(), ) { - (Some(_), _, Some(_), _) | (Some(_), _, _, Some(_)) => bail!( + (Some(_git), _, Some(_registry), _) | (Some(_git), _, _, Some(_registry)) => bail!( "dependency ({}) specification is ambiguous. \ Only one of `git` or `registry` is allowed.", name_in_toml ), - (_, _, Some(_), Some(_)) => bail!( + (_, _, Some(_registry), Some(_registry_index)) => bail!( "dependency ({}) specification is ambiguous. \ Only one of `registry` or `registry-index` is allowed.", name_in_toml From cc7fcaf57cde0725f1e84c0065a08fd526d0d798 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Wed, 24 Apr 2024 23:43:00 -0400 Subject: [PATCH 3/6] refactor(toml): move git and patch conflict to separate match arm --- src/cargo/util/toml/mod.rs | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index d15bc31f0c4..21da0a0e3e7 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -1910,15 +1910,14 @@ fn to_dependency_source_id( Only one of `registry` or `registry-index` is allowed.", name_in_toml ), - (Some(git), maybe_path, _, _) => { - if maybe_path.is_some() { - bail!( - "dependency ({}) specification is ambiguous. \ - Only one of `git` or `path` is allowed.", - name_in_toml - ); - } - + (Some(_git), Some(_path), _, _) => { + bail!( + "dependency ({}) specification is ambiguous. \ + Only one of `git` or `path` is allowed.", + name_in_toml + ); + } + (Some(git), None, _, _) => { let n_details = [&orig.branch, &orig.tag, &orig.rev] .iter() .filter(|d| d.is_some()) From 4b0fac9c050c7a75f8072b18b3495b55dc7e9a09 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Wed, 24 Apr 2024 23:46:55 -0400 Subject: [PATCH 4/6] refactor(toml): make match patterns explicit --- src/cargo/util/toml/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 21da0a0e3e7..27621f02bd1 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -1910,14 +1910,14 @@ fn to_dependency_source_id( Only one of `registry` or `registry-index` is allowed.", name_in_toml ), - (Some(_git), Some(_path), _, _) => { + (Some(_git), Some(_path), None, None) => { bail!( "dependency ({}) specification is ambiguous. \ Only one of `git` or `path` is allowed.", name_in_toml ); } - (Some(git), None, _, _) => { + (Some(git), None, None, None) => { let n_details = [&orig.branch, &orig.tag, &orig.rev] .iter() .filter(|d| d.is_some()) From 7ed7612a7923406be12ae7a6bab3c77fa37497f6 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Wed, 24 Apr 2024 23:54:52 -0400 Subject: [PATCH 5/6] refactor(toml): format arg captures --- src/cargo/util/toml/mod.rs | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 27621f02bd1..3e5438cb68d 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -1901,20 +1901,17 @@ fn to_dependency_source_id( orig.registry_index.as_ref(), ) { (Some(_git), _, Some(_registry), _) | (Some(_git), _, _, Some(_registry)) => bail!( - "dependency ({}) specification is ambiguous. \ + "dependency ({name_in_toml}) specification is ambiguous. \ Only one of `git` or `registry` is allowed.", - name_in_toml ), (_, _, Some(_registry), Some(_registry_index)) => bail!( - "dependency ({}) specification is ambiguous. \ + "dependency ({name_in_toml}) specification is ambiguous. \ Only one of `registry` or `registry-index` is allowed.", - name_in_toml ), (Some(_git), Some(_path), None, None) => { bail!( - "dependency ({}) specification is ambiguous. \ + "dependency ({name_in_toml}) specification is ambiguous. \ Only one of `git` or `path` is allowed.", - name_in_toml ); } (Some(git), None, None, None) => { @@ -1925,9 +1922,8 @@ fn to_dependency_source_id( if n_details > 1 { bail!( - "dependency ({}) specification is ambiguous. \ + "dependency ({name_in_toml}) specification is ambiguous. \ Only one of `branch`, `tag` or `rev` is allowed.", - name_in_toml ); } @@ -1942,12 +1938,11 @@ fn to_dependency_source_id( if let Some(fragment) = loc.fragment() { let msg = format!( - "URL fragment `#{}` in git URL is ignored for dependency ({}). \ + "URL fragment `#{fragment}` in git URL is ignored for dependency ({name_in_toml}). \ If you were trying to specify a specific git revision, \ - use `rev = \"{}\"` in the dependency declaration.", - fragment, name_in_toml, fragment + use `rev = \"{fragment}\"` in the dependency declaration.", ); - manifest_ctx.warnings.push(msg) + manifest_ctx.warnings.push(msg); } SourceId::for_git(&loc, reference)? From d855cd634acde0d2063a6b1bb144ebd35a595f1f Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Wed, 24 Apr 2024 23:58:35 -0400 Subject: [PATCH 6/6] refactor(toml): remove unnecessary `?` operators --- src/cargo/util/toml/mod.rs | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 3e5438cb68d..e54db889229 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -1894,7 +1894,7 @@ fn to_dependency_source_id( name_in_toml: &str, manifest_ctx: &mut ManifestContext<'_, '_>, ) -> CargoResult { - let new_source_id = match ( + match ( orig.git.as_ref(), orig.path.as_ref(), orig.registry.as_deref(), @@ -1945,7 +1945,7 @@ fn to_dependency_source_id( manifest_ctx.warnings.push(msg); } - SourceId::for_git(&loc, reference)? + SourceId::for_git(&loc, reference) } (None, Some(path), _, _) => { let path = path.resolve(manifest_ctx.gctx); @@ -1960,20 +1960,18 @@ fn to_dependency_source_id( if manifest_ctx.source_id.is_path() { let path = manifest_ctx.root.join(path); let path = paths::normalize_path(&path); - SourceId::for_path(&path)? + SourceId::for_path(&path) } else { - manifest_ctx.source_id + Ok(manifest_ctx.source_id) } } - (None, None, Some(registry), None) => SourceId::alt_registry(manifest_ctx.gctx, registry)?, + (None, None, Some(registry), None) => SourceId::alt_registry(manifest_ctx.gctx, registry), (None, None, None, Some(registry_index)) => { let url = registry_index.into_url()?; - SourceId::for_registry(&url)? + SourceId::for_registry(&url) } - (None, None, None, None) => SourceId::crates_io(manifest_ctx.gctx)?, - }; - - Ok(new_source_id) + (None, None, None, None) => SourceId::crates_io(manifest_ctx.gctx), + } } pub trait ResolveToPath {