Skip to content

Commit

Permalink
Auto merge of #14106 - Eh2406:build_feature_map, r=ehuss
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@bors
bors committed Jun 20, 2024
2 parents 96fc12c + 85f6d2d commit fb05646
Showing 1 changed file with 54 additions and 95 deletions.
149 changes: 54 additions & 95 deletions src/cargo/core/summary.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -180,117 +179,78 @@ 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."
);
}
}
DepFeature {
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
Expand All @@ -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"
);
}
}
}
Expand All @@ -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."
);
}

Expand All @@ -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 => {
Expand All @@ -403,25 +351,36 @@ 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,
}
}
}

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}")
}
}
}
Expand Down

0 comments on commit fb05646

Please sign in to comment.