From e30ddc4f94866e690462d89d5d7332e456a93765 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 19 Mar 2024 12:02:53 -0500 Subject: [PATCH 01/12] refactor(toml): Remove a variable that doesnt need tracking --- src/cargo/util/toml/mod.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 3a4fd6847ae..3dbff95a436 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -926,7 +926,6 @@ pub fn to_real_manifest( .map(|mw| field_inherit_with(mw, "include", || inherit()?.include())) .transpose()? .unwrap_or_default(); - let empty_features = BTreeMap::new(); let summary = Summary::new( pkgid, @@ -934,7 +933,7 @@ pub fn to_real_manifest( &original_toml .features .as_ref() - .unwrap_or(&empty_features) + .unwrap_or(&Default::default()) .iter() .map(|(k, v)| { ( From dd8f8dc645820dd06916800527fb6c47de69ea34 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 19 Mar 2024 12:04:44 -0500 Subject: [PATCH 02/12] refactor(toml): Group workspace inheritance logic --- src/cargo/util/toml/mod.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 3dbff95a436..6063e65fd3e 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -519,18 +519,17 @@ pub fn to_real_manifest( `[workspace]`, only one can be specified" ), }; - - let package_name = &package.name; - if package_name.contains(':') { - features.require(Feature::open_namespaces())?; - } - let inherit_cell: LazyCell = LazyCell::new(); let inherit = || { inherit_cell .try_borrow_with(|| load_inheritable_fields(gctx, manifest_file, &workspace_config)) }; + let package_name = &package.name; + if package_name.contains(':') { + features.require(Feature::open_namespaces())?; + } + let version = package .version .clone() From b63e385a8907ae5b4c243b2c09c792d6ad8de577 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 26 Mar 2024 20:34:04 -0500 Subject: [PATCH 03/12] refactor(toml): Group workspace inheritance functions --- src/cargo/util/toml/mod.rs | 54 +++++++++++++++++++------------------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 6063e65fd3e..e465171c26c 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -1228,33 +1228,6 @@ fn to_workspace_config( ws_root_config } -fn load_inheritable_fields( - gctx: &GlobalContext, - resolved_path: &Path, - workspace_config: &WorkspaceConfig, -) -> CargoResult { - match workspace_config { - WorkspaceConfig::Root(root) => Ok(root.inheritable().clone()), - WorkspaceConfig::Member { - root: Some(ref path_to_root), - } => { - let path = resolved_path - .parent() - .unwrap() - .join(path_to_root) - .join("Cargo.toml"); - let root_path = paths::normalize_path(&path); - inheritable_from_path(gctx, root_path) - } - WorkspaceConfig::Member { root: None } => { - match find_workspace_root(&resolved_path, gctx)? { - Some(path_to_root) => inheritable_from_path(gctx, path_to_root), - None => Err(anyhow!("failed to find a workspace root")), - } - } - } -} - fn to_virtual_manifest( contents: String, document: toml_edit::ImDocument, @@ -1561,6 +1534,33 @@ fn unused_dep_keys( } } +fn load_inheritable_fields( + gctx: &GlobalContext, + resolved_path: &Path, + workspace_config: &WorkspaceConfig, +) -> CargoResult { + match workspace_config { + WorkspaceConfig::Root(root) => Ok(root.inheritable().clone()), + WorkspaceConfig::Member { + root: Some(ref path_to_root), + } => { + let path = resolved_path + .parent() + .unwrap() + .join(path_to_root) + .join("Cargo.toml"); + let root_path = paths::normalize_path(&path); + inheritable_from_path(gctx, root_path) + } + WorkspaceConfig::Member { root: None } => { + match find_workspace_root(&resolved_path, gctx)? { + Some(path_to_root) => inheritable_from_path(gctx, path_to_root), + None => Err(anyhow!("failed to find a workspace root")), + } + } + } +} + fn inheritable_from_path( gctx: &GlobalContext, workspace_path: PathBuf, From 386c4b6a654e91910c63adf00a004ea6972db04a Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 19 Mar 2024 12:34:04 -0500 Subject: [PATCH 04/12] refactor(toml): Consistently don't include pkgid This is the only error we do this for and we have the context for what manifest we are parsing. By removing this, it makes it easier to adjust lifetimes in the short term. --- src/cargo/util/toml/mod.rs | 7 +------ tests/testsuite/build_script.rs | 3 +-- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index e465171c26c..04cf57d3ade 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -687,12 +687,7 @@ pub fn to_real_manifest( if let Some(links) = &package.links { if !targets.iter().any(|t| t.is_custom_build()) { - bail!( - "package `{}` specifies that it links to `{}` but does not \ - have a custom build script", - pkgid, - links - ) + bail!("package specifies that it links to `{links}` but does not have a custom build script") } } diff --git a/tests/testsuite/build_script.rs b/tests/testsuite/build_script.rs index 433252941e6..fc3f734eb8f 100644 --- a/tests/testsuite/build_script.rs +++ b/tests/testsuite/build_script.rs @@ -993,8 +993,7 @@ fn links_no_build_cmd() { [ERROR] failed to parse manifest at `[..]/foo/Cargo.toml` Caused by: - package `foo v0.5.0 ([CWD])` specifies that it links to `a` but does \ -not have a custom build script + package specifies that it links to `a` but does not have a custom build script ", ) .run(); From 3b7fddb737a0f48ff97abc38772a9099d966e2dd Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 19 Mar 2024 12:35:51 -0500 Subject: [PATCH 05/12] refactor(toml): Delay pkgid/summary creation --- src/cargo/util/toml/mod.rs | 52 ++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 27 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 04cf57d3ade..1b4c5464ccd 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -538,14 +538,6 @@ pub fn to_real_manifest( package.version = version.clone().map(manifest::InheritableField::Value); - let pkgid = PackageId::new( - package.name.as_str().into(), - version - .clone() - .unwrap_or_else(|| semver::Version::new(0, 0, 0)), - source_id, - ); - let rust_version = if let Some(rust_version) = &package.rust_version { let rust_version = field_inherit_with(rust_version.clone(), "rust_version", || { inherit()?.rust_version() @@ -921,25 +913,6 @@ pub fn to_real_manifest( .transpose()? .unwrap_or_default(); - let summary = Summary::new( - pkgid, - deps, - &original_toml - .features - .as_ref() - .unwrap_or(&Default::default()) - .iter() - .map(|(k, v)| { - ( - InternedString::new(k), - v.iter().map(InternedString::from).collect(), - ) - }) - .collect(), - package.links.as_deref(), - rust_version.clone(), - )?; - let metadata = ManifestMetadata { description: package .description @@ -1087,6 +1060,31 @@ pub fn to_real_manifest( bail!("`package.publish` requires `package.version` be specified"); } + let pkgid = PackageId::new( + package.name.as_str().into(), + version + .clone() + .unwrap_or_else(|| semver::Version::new(0, 0, 0)), + source_id, + ); + let summary = Summary::new( + pkgid, + deps, + &original_toml + .features + .as_ref() + .unwrap_or(&Default::default()) + .iter() + .map(|(k, v)| { + ( + InternedString::new(k), + v.iter().map(InternedString::from).collect(), + ) + }) + .collect(), + package.links.as_deref(), + rust_version.clone(), + )?; if summary.features().contains_key("default-features") { warnings.push( "`default-features = [\"..\"]` was found in [features]. \ From 1855a4ad24b018b4ba6c04504332f225a11d96b3 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 19 Mar 2024 15:31:39 -0500 Subject: [PATCH 06/12] refactor(toml): Pull out the dep resolve/validate code --- src/cargo/util/toml/mod.rs | 158 ++++++++++++++++++------------------- 1 file changed, 79 insertions(+), 79 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 1b4c5464ccd..0bbc791067d 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -695,81 +695,8 @@ pub fn to_real_manifest( root: package_root, }; - #[tracing::instrument(skip(manifest_ctx, new_deps, workspace_config, inherit_cell))] - fn process_dependencies( - manifest_ctx: &mut ManifestContext<'_, '_>, - new_deps: Option<&BTreeMap>, - kind: Option, - workspace_config: &WorkspaceConfig, - inherit_cell: &LazyCell, - ) -> CargoResult>> { - let Some(dependencies) = new_deps else { - return Ok(None); - }; - - let inheritable = || { - inherit_cell.try_borrow_with(|| { - load_inheritable_fields( - manifest_ctx.gctx, - &manifest_ctx.root.join("Cargo.toml"), - &workspace_config, - ) - }) - }; - - let mut deps: BTreeMap = - BTreeMap::new(); - for (n, v) in dependencies.iter() { - let resolved = dependency_inherit_with(v.clone(), n, inheritable, manifest_ctx)?; - let dep = dep_to_dependency(&resolved, n, manifest_ctx, kind)?; - let name_in_toml = dep.name_in_toml().as_str(); - let kind_name = match kind { - Some(k) => k.kind_table(), - None => "dependencies", - }; - let table_in_toml = if let Some(platform) = &manifest_ctx.platform { - format!("target.{}.{kind_name}", platform.to_string()) - } else { - kind_name.to_string() - }; - unused_dep_keys( - name_in_toml, - &table_in_toml, - v.unused_keys(), - manifest_ctx.warnings, - ); - let mut resolved = resolved; - if let manifest::TomlDependency::Detailed(ref mut d) = resolved { - if d.public.is_some() { - if matches!(dep.kind(), DepKind::Normal) { - if !manifest_ctx - .features - .require(Feature::public_dependency()) - .is_ok() - && !manifest_ctx.gctx.cli_unstable().public_dependency - { - d.public = None; - manifest_ctx.warnings.push(format!( - "ignoring `public` on dependency {name}, pass `-Zpublic-dependency` to enable support for it", name = &dep.name_in_toml() - )) - } - } else { - d.public = None; - } - } - } - - manifest_ctx.deps.push(dep); - deps.insert( - n.clone(), - manifest::InheritableDependency::Value(resolved.clone()), - ); - } - Ok(Some(deps)) - } - // Collect the dependencies. - let dependencies = process_dependencies( + let dependencies = resolve_and_validate_dependencies( &mut manifest_ctx, original_toml.dependencies.as_ref(), None, @@ -785,7 +712,7 @@ pub fn to_real_manifest( ); } let dev_deps = original_toml.dev_dependencies(); - let dev_deps = process_dependencies( + let dev_deps = resolve_and_validate_dependencies( &mut manifest_ctx, dev_deps, Some(DepKind::Development), @@ -801,7 +728,7 @@ pub fn to_real_manifest( ); } let build_deps = original_toml.build_dependencies(); - let build_deps = process_dependencies( + let build_deps = resolve_and_validate_dependencies( &mut manifest_ctx, build_deps, Some(DepKind::Build), @@ -825,7 +752,7 @@ pub fn to_real_manifest( platform.check_cfg_attributes(manifest_ctx.warnings); Some(platform) }; - let deps = process_dependencies( + let deps = resolve_and_validate_dependencies( &mut manifest_ctx, platform.dependencies.as_ref(), None, @@ -841,7 +768,7 @@ pub fn to_real_manifest( ); } let build_deps = platform.build_dependencies(); - let build_deps = process_dependencies( + let build_deps = resolve_and_validate_dependencies( &mut manifest_ctx, build_deps, Some(DepKind::Build), @@ -857,7 +784,7 @@ pub fn to_real_manifest( ); } let dev_deps = platform.dev_dependencies(); - let dev_deps = process_dependencies( + let dev_deps = resolve_and_validate_dependencies( &mut manifest_ctx, dev_deps, Some(DepKind::Development), @@ -1200,6 +1127,79 @@ pub fn to_real_manifest( Ok(manifest) } +#[tracing::instrument(skip(manifest_ctx, new_deps, workspace_config, inherit_cell))] +fn resolve_and_validate_dependencies( + manifest_ctx: &mut ManifestContext<'_, '_>, + new_deps: Option<&BTreeMap>, + kind: Option, + workspace_config: &WorkspaceConfig, + inherit_cell: &LazyCell, +) -> CargoResult>> { + let Some(dependencies) = new_deps else { + return Ok(None); + }; + + let inheritable = || { + inherit_cell.try_borrow_with(|| { + load_inheritable_fields( + manifest_ctx.gctx, + &manifest_ctx.root.join("Cargo.toml"), + &workspace_config, + ) + }) + }; + + let mut deps: BTreeMap = + BTreeMap::new(); + for (n, v) in dependencies.iter() { + let resolved = dependency_inherit_with(v.clone(), n, inheritable, manifest_ctx)?; + let dep = dep_to_dependency(&resolved, n, manifest_ctx, kind)?; + let name_in_toml = dep.name_in_toml().as_str(); + let kind_name = match kind { + Some(k) => k.kind_table(), + None => "dependencies", + }; + let table_in_toml = if let Some(platform) = &manifest_ctx.platform { + format!("target.{}.{kind_name}", platform.to_string()) + } else { + kind_name.to_string() + }; + unused_dep_keys( + name_in_toml, + &table_in_toml, + v.unused_keys(), + manifest_ctx.warnings, + ); + let mut resolved = resolved; + if let manifest::TomlDependency::Detailed(ref mut d) = resolved { + if d.public.is_some() { + if matches!(dep.kind(), DepKind::Normal) { + if !manifest_ctx + .features + .require(Feature::public_dependency()) + .is_ok() + && !manifest_ctx.gctx.cli_unstable().public_dependency + { + d.public = None; + manifest_ctx.warnings.push(format!( + "ignoring `public` on dependency {name}, pass `-Zpublic-dependency` to enable support for it", name = &dep.name_in_toml() + )) + } + } else { + d.public = None; + } + } + } + + manifest_ctx.deps.push(dep); + deps.insert( + n.clone(), + manifest::InheritableDependency::Value(resolved.clone()), + ); + } + Ok(Some(deps)) +} + fn to_workspace_config( resolved_toml: &manifest::TomlWorkspace, package_root: &Path, From e4f1eb01c9471e579dde6200978ec2c62954c003 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 20 Mar 2024 17:04:10 -0500 Subject: [PATCH 07/12] refactor(toml): Decouple Dep and TomlDep creation --- src/cargo/util/toml/mod.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 0bbc791067d..0810ed8d91b 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -1151,10 +1151,8 @@ fn resolve_and_validate_dependencies( let mut deps: BTreeMap = BTreeMap::new(); - for (n, v) in dependencies.iter() { - let resolved = dependency_inherit_with(v.clone(), n, inheritable, manifest_ctx)?; - let dep = dep_to_dependency(&resolved, n, manifest_ctx, kind)?; - let name_in_toml = dep.name_in_toml().as_str(); + for (name_in_toml, v) in dependencies.iter() { + let resolved = dependency_inherit_with(v.clone(), name_in_toml, inheritable, manifest_ctx)?; let kind_name = match kind { Some(k) => k.kind_table(), None => "dependencies", @@ -1173,7 +1171,7 @@ fn resolve_and_validate_dependencies( let mut resolved = resolved; if let manifest::TomlDependency::Detailed(ref mut d) = resolved { if d.public.is_some() { - if matches!(dep.kind(), DepKind::Normal) { + if matches!(kind, None) { if !manifest_ctx .features .require(Feature::public_dependency()) @@ -1182,7 +1180,7 @@ fn resolve_and_validate_dependencies( { d.public = None; manifest_ctx.warnings.push(format!( - "ignoring `public` on dependency {name}, pass `-Zpublic-dependency` to enable support for it", name = &dep.name_in_toml() + "ignoring `public` on dependency {name_in_toml}, pass `-Zpublic-dependency` to enable support for it" )) } } else { @@ -1191,9 +1189,10 @@ fn resolve_and_validate_dependencies( } } + let dep = dep_to_dependency(&resolved, name_in_toml, manifest_ctx, kind)?; manifest_ctx.deps.push(dep); deps.insert( - n.clone(), + name_in_toml.clone(), manifest::InheritableDependency::Value(resolved.clone()), ); } From 5d2fa4497f083504f285c34b4e646f1da33079f7 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 20 Mar 2024 20:03:09 -0500 Subject: [PATCH 08/12] refactor(toml): Consolidate public nightly check with clearing it --- src/cargo/util/toml/mod.rs | 47 ++++++++++++++++---------------------- 1 file changed, 20 insertions(+), 27 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 0810ed8d91b..87e7beaa711 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -1171,20 +1171,32 @@ fn resolve_and_validate_dependencies( let mut resolved = resolved; if let manifest::TomlDependency::Detailed(ref mut d) = resolved { if d.public.is_some() { + let public_feature = manifest_ctx.features.require(Feature::public_dependency()); + let with_public_feature = public_feature.is_ok(); + let with_z_public = manifest_ctx.gctx.cli_unstable().public_dependency; + if !with_public_feature + && (!with_z_public && !manifest_ctx.gctx.nightly_features_allowed) + { + public_feature?; + } if matches!(kind, None) { - if !manifest_ctx - .features - .require(Feature::public_dependency()) - .is_ok() - && !manifest_ctx.gctx.cli_unstable().public_dependency - { + if !with_public_feature && !with_z_public { d.public = None; manifest_ctx.warnings.push(format!( "ignoring `public` on dependency {name_in_toml}, pass `-Zpublic-dependency` to enable support for it" )) } } else { - d.public = None; + let hint = format!( + "'public' specifier can only be used on regular dependencies, not {kind_name}", + ); + if with_public_feature || with_z_public { + bail!(hint) + } else { + // If public feature isn't enabled in nightly, we instead warn that. + manifest_ctx.warnings.push(hint); + d.public = None; + } } } } @@ -2131,26 +2143,7 @@ fn detailed_dep_to_dependency( } if let Some(p) = orig.public { - let public_feature = manifest_ctx.features.require(Feature::public_dependency()); - let with_z_public = manifest_ctx.gctx.cli_unstable().public_dependency; - let with_public_feature = public_feature.is_ok(); - if !with_public_feature && (!with_z_public && !manifest_ctx.gctx.nightly_features_allowed) { - public_feature?; - } - - if dep.kind() != DepKind::Normal { - let hint = format!( - "'public' specifier can only be used on regular dependencies, not {}", - dep.kind().kind_table(), - ); - match (with_public_feature, with_z_public) { - (true, _) | (_, true) => bail!(hint), - // If public feature isn't enabled in nightly, we instead warn that. - (false, false) => manifest_ctx.warnings.push(hint), - } - } else { - dep.set_public(p); - } + dep.set_public(p); } if let (Some(artifact), is_lib, target) = ( From 573fe524cf1e724aacd33e92e60bd6f0664fe73d Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 26 Mar 2024 20:30:55 -0500 Subject: [PATCH 09/12] fix(toml): Warn on unused workspace.dependencies keys on virtual workspaces --- src/cargo/util/toml/mod.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 87e7beaa711..ca1ef1d2950 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -1298,6 +1298,16 @@ fn to_virtual_manifest( let workspace_config = match original_toml.workspace { Some(ref toml_config) => { verify_lints(toml_config.lints.as_ref(), gctx, &mut warnings)?; + if let Some(ws_deps) = &toml_config.dependencies { + for (name, dep) in ws_deps { + unused_dep_keys( + name, + "workspace.dependencies", + dep.unused_keys(), + &mut warnings, + ); + } + } let ws_root_config = to_workspace_config(toml_config, root); gctx.ws_roots .borrow_mut() From 931259032e7df76f1c1e1d634852c61cd6bdc749 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 25 Mar 2024 16:47:46 -0500 Subject: [PATCH 10/12] refator(tom): Give up and allow warnings anywhere --- src/cargo/core/manifest.rs | 6 ++ src/cargo/ops/cargo_package.rs | 4 ++ src/cargo/util/toml/mod.rs | 102 ++++++++++++++++++--------------- 3 files changed, 66 insertions(+), 46 deletions(-) diff --git a/src/cargo/core/manifest.rs b/src/cargo/core/manifest.rs index 9c0a53909b8..af64bc5d55f 100644 --- a/src/cargo/core/manifest.rs +++ b/src/cargo/core/manifest.rs @@ -28,6 +28,12 @@ pub enum EitherManifest { } impl EitherManifest { + pub fn warnings_mut(&mut self) -> &mut Warnings { + match self { + EitherManifest::Real(r) => r.warnings_mut(), + EitherManifest::Virtual(v) => v.warnings_mut(), + } + } pub(crate) fn workspace_config(&self) -> &WorkspaceConfig { match *self { EitherManifest::Real(ref r) => r.workspace_config(), diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index e2ff5a45f89..d3147ae9e0e 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -458,6 +458,8 @@ fn build_lock(ws: &Workspace<'_>, orig_pkg: &Package) -> CargoResult { let toml_manifest = prepare_for_publish(orig_pkg.manifest().resolved_toml(), ws, orig_pkg.root())?; let source_id = orig_pkg.package_id().source_id(); + let mut warnings = Default::default(); + let mut errors = Default::default(); let manifest = to_real_manifest( contents.to_owned(), document.clone(), @@ -465,6 +467,8 @@ fn build_lock(ws: &Workspace<'_>, orig_pkg: &Package) -> CargoResult { source_id, orig_pkg.manifest_path(), gctx, + &mut warnings, + &mut errors, )?; let new_pkg = Package::new(manifest, orig_pkg.manifest_path()); diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index ca1ef1d2950..0f07653b0dd 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -48,6 +48,9 @@ pub fn read_manifest( source_id: SourceId, gctx: &GlobalContext, ) -> CargoResult { + let mut warnings = vec![]; + let mut errors = vec![]; + let contents = read_toml_string(path, gctx).map_err(|err| ManifestError::new(err, path.into()))?; let document = @@ -55,13 +58,31 @@ pub fn read_manifest( let original_toml = deserialize_toml(&document) .map_err(|e| emit_diagnostic(e.into(), &contents, path, gctx))?; - (|| { + let mut manifest = (|| { if original_toml.package().is_some() { - to_real_manifest(contents, document, original_toml, source_id, path, gctx) - .map(EitherManifest::Real) + to_real_manifest( + contents, + document, + original_toml, + source_id, + path, + gctx, + &mut warnings, + &mut errors, + ) + .map(EitherManifest::Real) } else { - to_virtual_manifest(contents, document, original_toml, source_id, path, gctx) - .map(EitherManifest::Virtual) + to_virtual_manifest( + contents, + document, + original_toml, + source_id, + path, + gctx, + &mut warnings, + &mut errors, + ) + .map(EitherManifest::Virtual) } })() .map_err(|err| { @@ -69,8 +90,16 @@ pub fn read_manifest( err.context(format!("failed to parse manifest at `{}`", path.display())), path.into(), ) - .into() - }) + })?; + + for warning in warnings { + manifest.warnings_mut().add_warning(warning); + } + for error in errors { + manifest.warnings_mut().add_critical_warning(error); + } + + Ok(manifest) } #[tracing::instrument(skip_all)] @@ -438,6 +467,8 @@ pub fn to_real_manifest( source_id: SourceId, manifest_file: &Path, gctx: &GlobalContext, + warnings: &mut Vec, + errors: &mut Vec, ) -> CargoResult { let embedded = is_embedded(manifest_file); let package_root = manifest_file.parent().unwrap(); @@ -463,13 +494,10 @@ pub fn to_real_manifest( } } - let mut warnings = vec![]; - let mut errors = vec![]; - // Parse features first so they will be available when parsing other parts of the TOML. let empty = Vec::new(); let cargo_features = original_toml.cargo_features.as_ref().unwrap_or(&empty); - let features = Features::new(cargo_features, gctx, &mut warnings, source_id.is_path())?; + let features = Features::new(cargo_features, gctx, warnings, source_id.is_path())?; let mut package = match (&original_toml.package, &original_toml.project) { (Some(_), Some(project)) => { @@ -494,15 +522,10 @@ pub fn to_real_manifest( let workspace_config = match (original_toml.workspace.as_ref(), package.workspace.as_ref()) { (Some(toml_config), None) => { - verify_lints(toml_config.lints.as_ref(), gctx, &mut warnings)?; + verify_lints(toml_config.lints.as_ref(), gctx, warnings)?; if let Some(ws_deps) = &toml_config.dependencies { for (name, dep) in ws_deps { - unused_dep_keys( - name, - "workspace.dependencies", - dep.unused_keys(), - &mut warnings, - ); + unused_dep_keys(name, "workspace.dependencies", dep.unused_keys(), warnings); } } let ws_root_config = to_workspace_config(toml_config, package_root); @@ -649,8 +672,8 @@ pub fn to_real_manifest( edition, &package.build, &package.metabuild, - &mut warnings, - &mut errors, + warnings, + errors, )?; if targets.iter().all(|t| t.is_custom_build()) { @@ -689,7 +712,7 @@ pub fn to_real_manifest( deps: &mut deps, source_id, gctx, - warnings: &mut warnings, + warnings, features: &features, platform: None, root: package_root, @@ -964,7 +987,7 @@ pub fn to_real_manifest( if let Some(profiles) = &original_toml.profile { let cli_unstable = gctx.cli_unstable(); - validate_profiles(profiles, cli_unstable, &features, &mut warnings)?; + validate_profiles(profiles, cli_unstable, &features, warnings)?; } let publish = package @@ -1075,7 +1098,7 @@ pub fn to_real_manifest( }), _unused_keys: Default::default(), }; - let mut manifest = Manifest::new( + let manifest = Manifest::new( Rc::new(contents), Rc::new(document), Rc::new(original_toml), @@ -1114,13 +1137,7 @@ pub fn to_real_manifest( .to_owned(), ); } - warn_on_unused(&manifest.original_toml()._unused_keys, &mut warnings); - for warning in warnings { - manifest.warnings_mut().add_warning(warning); - } - for error in errors { - manifest.warnings_mut().add_critical_warning(error); - } + warn_on_unused(&manifest.original_toml()._unused_keys, warnings); manifest.feature_gate()?; @@ -1239,6 +1256,8 @@ fn to_virtual_manifest( source_id: SourceId, manifest_file: &Path, gctx: &GlobalContext, + warnings: &mut Vec, + _errors: &mut Vec, ) -> CargoResult { let root = manifest_file.parent().unwrap(); @@ -1263,11 +1282,10 @@ fn to_virtual_manifest( bail!("this virtual manifest specifies a `{field}` section, which is not allowed"); } - let mut warnings = Vec::new(); let mut deps = Vec::new(); let empty = Vec::new(); let cargo_features = original_toml.cargo_features.as_ref().unwrap_or(&empty); - let features = Features::new(cargo_features, gctx, &mut warnings, source_id.is_path())?; + let features = Features::new(cargo_features, gctx, warnings, source_id.is_path())?; resolved_toml._unused_keys = Default::default(); @@ -1276,7 +1294,7 @@ fn to_virtual_manifest( deps: &mut deps, source_id, gctx, - warnings: &mut warnings, + warnings, platform: None, features: &features, root, @@ -1287,7 +1305,7 @@ fn to_virtual_manifest( ) }; if let Some(profiles) = &original_toml.profile { - validate_profiles(profiles, gctx.cli_unstable(), &features, &mut warnings)?; + validate_profiles(profiles, gctx.cli_unstable(), &features, warnings)?; } let resolve_behavior = original_toml .workspace @@ -1297,15 +1315,10 @@ fn to_virtual_manifest( .transpose()?; let workspace_config = match original_toml.workspace { Some(ref toml_config) => { - verify_lints(toml_config.lints.as_ref(), gctx, &mut warnings)?; + verify_lints(toml_config.lints.as_ref(), gctx, warnings)?; if let Some(ws_deps) = &toml_config.dependencies { for (name, dep) in ws_deps { - unused_dep_keys( - name, - "workspace.dependencies", - dep.unused_keys(), - &mut warnings, - ); + unused_dep_keys(name, "workspace.dependencies", dep.unused_keys(), warnings); } } let ws_root_config = to_workspace_config(toml_config, root); @@ -1318,7 +1331,7 @@ fn to_virtual_manifest( bail!("virtual manifests must be configured with [workspace]"); } }; - let mut manifest = VirtualManifest::new( + let manifest = VirtualManifest::new( Rc::new(contents), Rc::new(document), Rc::new(original_toml), @@ -1330,10 +1343,7 @@ fn to_virtual_manifest( resolve_behavior, ); - warn_on_unused(&manifest.original_toml()._unused_keys, &mut warnings); - for warning in warnings { - manifest.warnings_mut().add_warning(warning); - } + warn_on_unused(&manifest.original_toml()._unused_keys, warnings); Ok(manifest) } From 453f39f40e86cb1816aaa3a53451dcaee2b44b55 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 27 Mar 2024 12:42:59 -0500 Subject: [PATCH 11/12] refactor(toml): Centralize workspace config processing --- src/cargo/util/toml/mod.rs | 85 ++++++++++++++++++++------------------ 1 file changed, 45 insertions(+), 40 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 0f07653b0dd..dec86a6293b 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -520,28 +520,12 @@ pub fn to_real_manifest( (None, None) => bail!("no `package` section found"), }; - let workspace_config = match (original_toml.workspace.as_ref(), package.workspace.as_ref()) { - (Some(toml_config), None) => { - verify_lints(toml_config.lints.as_ref(), gctx, warnings)?; - if let Some(ws_deps) = &toml_config.dependencies { - for (name, dep) in ws_deps { - unused_dep_keys(name, "workspace.dependencies", dep.unused_keys(), warnings); - } - } - let ws_root_config = to_workspace_config(toml_config, package_root); - gctx.ws_roots - .borrow_mut() - .insert(package_root.to_path_buf(), ws_root_config.clone()); - WorkspaceConfig::Root(ws_root_config) - } - (None, root) => WorkspaceConfig::Member { - root: root.cloned(), - }, - (Some(..), Some(..)) => bail!( - "cannot configure both `package.workspace` and \ - `[workspace]`, only one can be specified" - ), - }; + let workspace_config = to_workspace_config(&original_toml, package_root, gctx, warnings)?; + if let WorkspaceConfig::Root(ws_root_config) = &workspace_config { + gctx.ws_roots + .borrow_mut() + .insert(package_root.to_owned(), ws_root_config.clone()); + } let inherit_cell: LazyCell = LazyCell::new(); let inherit = || { inherit_cell @@ -1229,6 +1213,37 @@ fn resolve_and_validate_dependencies( } fn to_workspace_config( + original_toml: &manifest::TomlManifest, + package_root: &Path, + gctx: &GlobalContext, + warnings: &mut Vec, +) -> CargoResult { + let workspace_config = match ( + original_toml.workspace.as_ref(), + original_toml.package().and_then(|p| p.workspace.as_ref()), + ) { + (Some(toml_config), None) => { + verify_lints(toml_config.lints.as_ref(), gctx, warnings)?; + if let Some(ws_deps) = &toml_config.dependencies { + for (name, dep) in ws_deps { + unused_dep_keys(name, "workspace.dependencies", dep.unused_keys(), warnings); + } + } + let ws_root_config = to_workspace_root_config(toml_config, package_root); + WorkspaceConfig::Root(ws_root_config) + } + (None, root) => WorkspaceConfig::Member { + root: root.cloned(), + }, + (Some(..), Some(..)) => bail!( + "cannot configure both `package.workspace` and \ + `[workspace]`, only one can be specified" + ), + }; + Ok(workspace_config) +} + +fn to_workspace_root_config( resolved_toml: &manifest::TomlWorkspace, package_root: &Path, ) -> WorkspaceRootConfig { @@ -1313,24 +1328,14 @@ fn to_virtual_manifest( .and_then(|ws| ws.resolver.as_deref()) .map(|r| ResolveBehavior::from_manifest(r)) .transpose()?; - let workspace_config = match original_toml.workspace { - Some(ref toml_config) => { - verify_lints(toml_config.lints.as_ref(), gctx, warnings)?; - if let Some(ws_deps) = &toml_config.dependencies { - for (name, dep) in ws_deps { - unused_dep_keys(name, "workspace.dependencies", dep.unused_keys(), warnings); - } - } - let ws_root_config = to_workspace_config(toml_config, root); - gctx.ws_roots - .borrow_mut() - .insert(root.to_path_buf(), ws_root_config.clone()); - WorkspaceConfig::Root(ws_root_config) - } - None => { - bail!("virtual manifests must be configured with [workspace]"); - } - }; + let workspace_config = to_workspace_config(&original_toml, root, gctx, warnings)?; + if let WorkspaceConfig::Root(ws_root_config) = &workspace_config { + gctx.ws_roots + .borrow_mut() + .insert(root.to_owned(), ws_root_config.clone()); + } else { + bail!("virtual manifests must be configured with [workspace]"); + } let manifest = VirtualManifest::new( Rc::new(contents), Rc::new(document), From 8a82df2c213ceb1a7e55aea79914990113ccb5ff Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 27 Mar 2024 15:09:02 -0500 Subject: [PATCH 12/12] refactor(toml): Consolidate workspace dependency verification --- src/cargo/util/toml/mod.rs | 39 +++++++++----------------------------- 1 file changed, 9 insertions(+), 30 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index dec86a6293b..ac885bf122c 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -479,21 +479,6 @@ pub fn to_real_manifest( ); }; - if let Some(deps) = original_toml - .workspace - .as_ref() - .and_then(|ws| ws.dependencies.as_ref()) - { - for (name, dep) in deps { - if dep.is_optional() { - bail!("{name} is optional, but workspace dependencies cannot be optional",); - } - if dep.is_public() { - bail!("{name} is public, but workspace dependencies cannot be public",); - } - } - } - // Parse features first so they will be available when parsing other parts of the TOML. let empty = Vec::new(); let cargo_features = original_toml.cargo_features.as_ref().unwrap_or(&empty); @@ -1225,6 +1210,15 @@ fn to_workspace_config( (Some(toml_config), None) => { verify_lints(toml_config.lints.as_ref(), gctx, warnings)?; if let Some(ws_deps) = &toml_config.dependencies { + for (name, dep) in ws_deps { + if dep.is_optional() { + bail!("{name} is optional, but workspace dependencies cannot be optional",); + } + if dep.is_public() { + bail!("{name} is public, but workspace dependencies cannot be public",); + } + } + for (name, dep) in ws_deps { unused_dep_keys(name, "workspace.dependencies", dep.unused_keys(), warnings); } @@ -1278,21 +1272,6 @@ fn to_virtual_manifest( let mut resolved_toml = original_toml.clone(); - if let Some(deps) = original_toml - .workspace - .as_ref() - .and_then(|ws| ws.dependencies.as_ref()) - { - for (name, dep) in deps { - if dep.is_optional() { - bail!("{name} is optional, but workspace dependencies cannot be optional",); - } - if dep.is_public() { - bail!("{name} is public, but workspace dependencies cannot be public",); - } - } - } - for field in original_toml.requires_package() { bail!("this virtual manifest specifies a `{field}` section, which is not allowed"); }