development/cargo
development/cargo
1
0
Fork 0
mirror of https://github.com/rust-lang/cargo synced 2024-07-02 16:09:07 +00:00
Code Issues Projects Releases Wiki Activity

Auto merge of #14106 - Eh2406:build_feature_map, r=ehuss

Browse Source 
Simplify checking feature syntax

### What does this PR try to resolve?

Similar to #14089, in my PubGrub work I had to try to understand the `build_feature_map` code that determines if the name refers to a feature or an optional dependency. There were a couple of little things I wanted to clean up while I was staring at the code. Specifically:
- there was a lot of vertical space taken up with arguments to `bail` that could be in line at the format string.
- some match statements were used where a named helper method would be clearer.
- `split_once` could replace a `find ... split_at` dance.
- in `dep_map` we were constructing a full `Vec<Dependency>`, when we only cared about whether there was any dependency and whether any of the dependencies were optional.

### How should we test and review this PR?

It's an internal re-factor, and the tests still pass.

### Additional information

If you're having questions about this code while you're reviewing, this would be a perfect opportunity for better naming and comments. Please ask questions.

`@epage:` After this PR I am likely to copy this code into my pubgrub tester. Are there other users who might be interested in looking at a cargo.toml or an index entry and determining what feature names are available and what dependencies they enable? If so maybe this function should be moved to one of the new stable-ish reusable libraries.
This commit is contained in:
bors 2024-06-20 14:08:43 +00:00
commit fb056466a2
1 changed files with 54 additions and 95 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

149
src/cargo/core/summary.rs
View File

@ -156,13 +156,12 @@ fn build_feature_map(
dependencies: &[Dependency],
) -> CargoResult<FeatureMap> {
use self::FeatureValue::*;
let mut dep_map = HashMap::new();
// A map of dependency names to whether there are any that are optional.
let mut dep_map: HashMap<InternedString, bool> = HashMap::new();
for dep in dependencies.iter() {
dep_map
.entry(dep.name_in_toml())
.or_insert_with(Vec::new)
.push(dep);
*dep_map.entry(dep.name_in_toml()).or_insert(false) |= dep.is_optional();
}
let dep_map = dep_map; // We are done mutating this variable
let mut map: FeatureMap = features
.iter()
@ -180,91 +179,61 @@ fn build_feature_map(
let explicitly_listed: HashSet<_> = map
.values()
.flatten()
.filter_map(|fv| match fv {
Dep { dep_name } => Some(*dep_name),
_ => None,
})
.filter_map(|fv| fv.explicit_dep_name())
.collect();
for dep in dependencies {
if !dep.is_optional() {
continue;
}
let dep_name_in_toml = dep.name_in_toml();
if features.contains_key(&dep_name_in_toml) || explicitly_listed.contains(&dep_name_in_toml)
{
let dep_name = dep.name_in_toml();
if features.contains_key(&dep_name) || explicitly_listed.contains(&dep_name) {
continue;
}
let fv = Dep {
dep_name: dep_name_in_toml,
};
map.insert(dep_name_in_toml, vec![fv]);
map.insert(dep_name, vec![Dep { dep_name }]);
}
let map = map; // We are done mutating this variable
// Validate features are listed properly.
for (feature, fvs) in &map {
FeatureName::new(feature)?;
for fv in fvs {
// Find data for the referenced dependency...
let dep_data = {
match fv {
Feature(dep_name) | Dep { dep_name, .. } | DepFeature { dep_name, .. } => {
dep_map.get(dep_name)
}
}
};
let is_optional_dep = dep_data
.iter()
.flat_map(|d| d.iter())
.any(|d| d.is_optional());
let dep_data = dep_map.get(&fv.feature_or_dep_name());
let is_any_dep = dep_data.is_some();
let is_optional_dep = dep_data.is_some_and(|&o| o);
match fv {
Feature(f) => {
if !features.contains_key(f) {
if !is_any_dep {
bail!(
"feature `{}` includes `{}` which is neither a dependency \
nor another feature",
feature,
fv
);
"feature `{feature}` includes `{fv}` which is neither a dependency \
nor another feature"
);
}
if is_optional_dep {
if !map.contains_key(f) {
bail!(
"feature `{}` includes `{}`, but `{}` is an \
"feature `{feature}` includes `{fv}`, but `{f}` is an \
optional dependency without an implicit feature\n\
Use `dep:{}` to enable the dependency.",
feature,
fv,
f,
f
Use `dep:{f}` to enable the dependency."
);
}
} else {
bail!("feature `{}` includes `{}`, but `{}` is not an optional dependency\n\
bail!("feature `{feature}` includes `{fv}`, but `{f}` is not an optional dependency\n\
A non-optional dependency of the same name is defined; \
consider adding `optional = true` to its definition.",
feature, fv, f);
consider adding `optional = true` to its definition.");
}
}
}
Dep { dep_name } => {
if !is_any_dep {
bail!(
"feature `{}` includes `{}`, but `{}` is not listed as a dependency",
feature,
fv,
dep_name
);
bail!("feature `{feature}` includes `{fv}`, but `{dep_name}` is not listed as a dependency");
}
if !is_optional_dep {
bail!(
"feature `{}` includes `{}`, but `{}` is not an optional dependency\n\
"feature `{feature}` includes `{fv}`, but `{dep_name}` is not an optional dependency\n\
A non-optional dependency of the same name is defined; \
consider adding `optional = true` to its definition.",
feature,
fv,
dep_name
consider adding `optional = true` to its definition."
);
}
}
@ -272,25 +241,16 @@ fn build_feature_map(
dep_name,
dep_feature,
weak,
..
} => {
// Early check for some unlikely syntax.
if dep_feature.contains('/') {
bail!(
"multiple slashes in feature `{}` (included by feature `{}`) are not allowed",
fv,
feature
);
bail!("multiple slashes in feature `{fv}` (included by feature `{feature}`) are not allowed");
}
// dep: cannot be combined with /
if let Some(stripped_dep) = dep_name.strip_prefix("dep:") {
let has_other_dep = explicitly_listed.contains(stripped_dep);
let is_optional = dep_map
.get(stripped_dep)
.iter()
.flat_map(|d| d.iter())
.any(|d| d.is_optional());
let is_optional = dep_map.get(stripped_dep).is_some_and(|&o| o);
let extra_help = if *weak || has_other_dep || !is_optional {
// In this case, the user should just remove dep:.
// Note that "hiding" an optional dependency
@ -314,18 +274,14 @@ fn build_feature_map(
// Validation of the feature name will be performed in the resolver.
if !is_any_dep {
bail!(
"feature `{}` includes `{}`, but `{}` is not a dependency",
feature,
fv,
dep_name
);
bail!("feature `{feature}` includes `{fv}`, but `{dep_name}` is not a dependency");
}
if *weak && !is_optional_dep {
bail!("feature `{}` includes `{}` with a `?`, but `{}` is not an optional dependency\n\
bail!(
"feature `{feature}` includes `{fv}` with a `?`, but `{dep_name}` is not an optional dependency\n\
A non-optional dependency of the same name is defined; \
consider removing the `?` or changing the dependency to be optional",
feature, fv, dep_name);
consider removing the `?` or changing the dependency to be optional"
);
}
}
}
@ -341,15 +297,13 @@ fn build_feature_map(
_ => None,
})
.collect();
if let Some(dep) = dependencies
if let Some((dep, _)) = dep_map
.iter()
.find(|dep| dep.is_optional() && !used.contains(&dep.name_in_toml()))
.find(|&(dep, &is_optional)| is_optional && !used.contains(dep))
{
bail!(
"optional dependency `{}` is not included in any feature\n\
Make sure that `dep:{}` is included in one of features in the [features] table.",
dep.name_in_toml(),
dep.name_in_toml(),
"optional dependency `{dep}` is not included in any feature\n\
Make sure that `dep:{dep}` is included in one of features in the [features] table."
);
}
@ -376,19 +330,13 @@ pub enum FeatureValue {
impl FeatureValue {
pub fn new(feature: InternedString) -> FeatureValue {
match feature.find('/') {
Some(pos) => {
let (dep, dep_feat) = feature.split_at(pos);
let dep_feat = &dep_feat[1..];
let (dep, weak) = if let Some(dep) = dep.strip_suffix('?') {
(dep, true)
} else {
(dep, false)
};
match feature.split_once('/') {
Some((dep, dep_feat)) => {
let dep_name = dep.strip_suffix('?');
FeatureValue::DepFeature {
dep_name: InternedString::new(dep),
dep_name: InternedString::new(dep_name.unwrap_or(dep)),
dep_feature: InternedString::new(dep_feat),
weak,
weak: dep_name.is_some(),
}
}
None => {
@ -403,9 +351,20 @@ impl FeatureValue {
}
}
/// Returns `true` if this feature explicitly used `dep:` syntax.
pub fn has_dep_prefix(&self) -> bool {
matches!(self, FeatureValue::Dep { .. })
/// Returns the name of the dependency if and only if it was explicitly named with the `dep:` syntax.
fn explicit_dep_name(&self) -> Option<InternedString> {
match self {
FeatureValue::Dep { dep_name, .. } => Some(*dep_name),
_ => None,
}
}
fn feature_or_dep_name(&self) -> InternedString {
match self {
FeatureValue::Feature(dep_name)
| FeatureValue::Dep { dep_name, .. }
| FeatureValue::DepFeature { dep_name, .. } => *dep_name,
}
}
}
@ -413,15 +372,15 @@ impl fmt::Display for FeatureValue {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
use self::FeatureValue::*;
match self {
Feature(feat) => write!(f, "{}", feat),
Dep { dep_name } => write!(f, "dep:{}", dep_name),
Feature(feat) => write!(f, "{feat}"),
Dep { dep_name } => write!(f, "dep:{dep_name}"),
DepFeature {
dep_name,
dep_feature,
weak,
} => {
let weak = if *weak { "?" } else { "" };
write!(f, "{}{}/{}", dep_name, weak, dep_feature)
write!(f, "{dep_name}{weak}/{dep_feature}")
}
}
}