From 7820a0b446e3892f4f41c6a561dd4439c0290901 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 1 May 2024 10:41:00 -0500 Subject: [PATCH 1/4] refactor(toml): Clarify which dep we are talking about --- src/cargo/util/toml/mod.rs | 41 +++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 10ae0b45d54..d31a734dafb 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -964,7 +964,7 @@ fn dependency_inherit_with<'a>( } fn inner_dependency_inherit_with<'a>( - dependency: manifest::TomlInheritedDependency, + pkg_dep: manifest::TomlInheritedDependency, name: &str, inherit: &dyn Fn() -> CargoResult<&'a InheritableFields>, package_root: &Path, @@ -982,35 +982,35 @@ fn inner_dependency_inherit_with<'a>( this could become a hard error in the future" )) } - inherit()?.get_dependency(name, package_root).map(|d| { - match d { - manifest::TomlDependency::Simple(s) => { - if let Some(false) = dependency.default_features() { + inherit()?.get_dependency(name, package_root).map(|ws_dep| { + match ws_dep { + manifest::TomlDependency::Simple(ws_version) => { + if let Some(false) = pkg_dep.default_features() { default_features_msg(name, None, warnings); } - if dependency.optional.is_some() - || dependency.features.is_some() - || dependency.public.is_some() + if pkg_dep.optional.is_some() + || pkg_dep.features.is_some() + || pkg_dep.public.is_some() { manifest::TomlDependency::Detailed(manifest::TomlDetailedDependency { - version: Some(s), - optional: dependency.optional, - features: dependency.features.clone(), - public: dependency.public, + version: Some(ws_version), + optional: pkg_dep.optional, + features: pkg_dep.features.clone(), + public: pkg_dep.public, ..Default::default() }) } else { - manifest::TomlDependency::Simple(s) + manifest::TomlDependency::Simple(ws_version) } } - manifest::TomlDependency::Detailed(d) => { - let mut d = d.clone(); - match (dependency.default_features(), d.default_features()) { + manifest::TomlDependency::Detailed(ws_dep) => { + let mut merged_dep = ws_dep.clone(); + match (pkg_dep.default_features(), merged_dep.default_features()) { // member: default-features = true and // workspace: default-features = false should turn on // default-features (Some(true), Some(false)) => { - d.default_features = Some(true); + merged_dep.default_features = Some(true); } // member: default-features = false and // workspace: default-features = true should ignore member @@ -1025,7 +1025,8 @@ fn inner_dependency_inherit_with<'a>( } _ => {} } - d.features = match (d.features.clone(), dependency.features.clone()) { + merged_dep.features = match (merged_dep.features.clone(), pkg_dep.features.clone()) + { (Some(dep_feat), Some(inherit_feat)) => Some( dep_feat .into_iter() @@ -1036,8 +1037,8 @@ fn inner_dependency_inherit_with<'a>( (None, Some(inherit_feat)) => Some(inherit_feat), (None, None) => None, }; - d.optional = dependency.optional; - manifest::TomlDependency::Detailed(d) + merged_dep.optional = pkg_dep.optional; + manifest::TomlDependency::Detailed(merged_dep) } } }) From bfe62a670d96d6e87b982a73e7acbf334a91250f Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 1 May 2024 10:51:36 -0500 Subject: [PATCH 2/4] refactor(toml): Always create a detailed dep This minimizes risk as we add more dependency features --- src/cargo/util/toml/mod.rs | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index d31a734dafb..9535e02f054 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -988,20 +988,13 @@ fn inner_dependency_inherit_with<'a>( if let Some(false) = pkg_dep.default_features() { default_features_msg(name, None, warnings); } - if pkg_dep.optional.is_some() - || pkg_dep.features.is_some() - || pkg_dep.public.is_some() - { - manifest::TomlDependency::Detailed(manifest::TomlDetailedDependency { - version: Some(ws_version), - optional: pkg_dep.optional, - features: pkg_dep.features.clone(), - public: pkg_dep.public, - ..Default::default() - }) - } else { - manifest::TomlDependency::Simple(ws_version) - } + manifest::TomlDependency::Detailed(manifest::TomlDetailedDependency { + version: Some(ws_version), + optional: pkg_dep.optional, + features: pkg_dep.features.clone(), + public: pkg_dep.public, + ..Default::default() + }) } manifest::TomlDependency::Detailed(ws_dep) => { let mut merged_dep = ws_dep.clone(); From e8ba122c8b4114898de0e953ffd86244d33c1ca6 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 1 May 2024 10:57:42 -0500 Subject: [PATCH 3/4] fix(toml): Don't lose 'public' when inheriting a dep When inheriting a simple dep, we preserved the package dep's `public` field but lost it when inheriting a detailed dep. This was done by consolidating the code paths. This also reduces the risk of us doing this again in the future. --- src/cargo/util/toml/mod.rs | 86 +++++++++++++++++--------------------- 1 file changed, 38 insertions(+), 48 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 9535e02f054..fcac6438eba 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -983,57 +983,47 @@ fn inner_dependency_inherit_with<'a>( )) } inherit()?.get_dependency(name, package_root).map(|ws_dep| { - match ws_dep { - manifest::TomlDependency::Simple(ws_version) => { - if let Some(false) = pkg_dep.default_features() { - default_features_msg(name, None, warnings); - } - manifest::TomlDependency::Detailed(manifest::TomlDetailedDependency { - version: Some(ws_version), - optional: pkg_dep.optional, - features: pkg_dep.features.clone(), - public: pkg_dep.public, - ..Default::default() - }) + let mut merged_dep = match ws_dep { + manifest::TomlDependency::Simple(ws_version) => manifest::TomlDetailedDependency { + version: Some(ws_version), + ..Default::default() + }, + manifest::TomlDependency::Detailed(ws_dep) => ws_dep.clone(), + }; + match (pkg_dep.default_features(), merged_dep.default_features()) { + // member: default-features = true and + // workspace: default-features = false should turn on + // default-features + (Some(true), Some(false)) => { + merged_dep.default_features = Some(true); } - manifest::TomlDependency::Detailed(ws_dep) => { - let mut merged_dep = ws_dep.clone(); - match (pkg_dep.default_features(), merged_dep.default_features()) { - // member: default-features = true and - // workspace: default-features = false should turn on - // default-features - (Some(true), Some(false)) => { - merged_dep.default_features = Some(true); - } - // member: default-features = false and - // workspace: default-features = true should ignore member - // default-features - (Some(false), Some(true)) => { - default_features_msg(name, Some(true), warnings); - } - // member: default-features = false and - // workspace: dep = "1.0" should ignore member default-features - (Some(false), None) => { - default_features_msg(name, None, warnings); - } - _ => {} - } - merged_dep.features = match (merged_dep.features.clone(), pkg_dep.features.clone()) - { - (Some(dep_feat), Some(inherit_feat)) => Some( - dep_feat - .into_iter() - .chain(inherit_feat) - .collect::>(), - ), - (Some(dep_fet), None) => Some(dep_fet), - (None, Some(inherit_feat)) => Some(inherit_feat), - (None, None) => None, - }; - merged_dep.optional = pkg_dep.optional; - manifest::TomlDependency::Detailed(merged_dep) + // member: default-features = false and + // workspace: default-features = true should ignore member + // default-features + (Some(false), Some(true)) => { + default_features_msg(name, Some(true), warnings); } + // member: default-features = false and + // workspace: dep = "1.0" should ignore member default-features + (Some(false), None) => { + default_features_msg(name, None, warnings); + } + _ => {} } + merged_dep.features = match (merged_dep.features.clone(), pkg_dep.features.clone()) { + (Some(dep_feat), Some(inherit_feat)) => Some( + dep_feat + .into_iter() + .chain(inherit_feat) + .collect::>(), + ), + (Some(dep_fet), None) => Some(dep_fet), + (None, Some(inherit_feat)) => Some(inherit_feat), + (None, None) => None, + }; + merged_dep.optional = pkg_dep.optional; + merged_dep.public = pkg_dep.public; + manifest::TomlDependency::Detailed(merged_dep) }) } From 1fc366826c0b0a375423435a694aa3ad53c63d30 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 1 May 2024 11:04:47 -0500 Subject: [PATCH 4/4] refactor(toml): Use exhaustive destructure to ensure we inherit dep fields --- src/cargo/util/toml/mod.rs | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index fcac6438eba..fc45afe4111 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -990,7 +990,20 @@ fn inner_dependency_inherit_with<'a>( }, manifest::TomlDependency::Detailed(ws_dep) => ws_dep.clone(), }; - match (pkg_dep.default_features(), merged_dep.default_features()) { + let manifest::TomlInheritedDependency { + workspace: _, + + features, + optional, + default_features, + default_features2, + public, + + _unused_keys: _, + } = &pkg_dep; + let default_features = default_features.or(*default_features2); + + match (default_features, merged_dep.default_features()) { // member: default-features = true and // workspace: default-features = false should turn on // default-features @@ -1010,7 +1023,7 @@ fn inner_dependency_inherit_with<'a>( } _ => {} } - merged_dep.features = match (merged_dep.features.clone(), pkg_dep.features.clone()) { + merged_dep.features = match (merged_dep.features.clone(), features.clone()) { (Some(dep_feat), Some(inherit_feat)) => Some( dep_feat .into_iter() @@ -1021,8 +1034,8 @@ fn inner_dependency_inherit_with<'a>( (None, Some(inherit_feat)) => Some(inherit_feat), (None, None) => None, }; - merged_dep.optional = pkg_dep.optional; - merged_dep.public = pkg_dep.public; + merged_dep.optional = *optional; + merged_dep.public = *public; manifest::TomlDependency::Detailed(merged_dep) }) }