]> git.proxmox.com Git - cargo.git/commitdiff
Implement suggestions for unknown features in workspace
authorAlik Aslanyan <inline0@protonmail.com>
Tue, 27 Apr 2021 09:09:44 +0000 (13:09 +0400)
committerAlik Aslanyan <inline0@protonmail.com>
Tue, 27 Apr 2021 09:09:44 +0000 (13:09 +0400)
Cargo.toml
src/cargo/core/workspace.rs
tests/testsuite/package_features.rs

index eaf8e8307e85d85b8faa1593c4bbca12fe334163..c7f17c0303908f2494681f0f53b9c7b3c2046845 100644 (file)
@@ -67,6 +67,7 @@ clap = "2.31.2"
 unicode-width = "0.1.5"
 openssl = { version = '0.10.11', optional = true }
 im-rc = "15.0.0"
+itertools = "0.10.0"
 
 # A noop dependency that changes in the Rust repository, it's a bit of a hack.
 # See the `src/tools/rustc-workspace-hack/README.md` file in `rust-lang/rust`
index 9a2d5920a748ea3a4c5c9cf551b932fb5ffa581c..3227203520ad86f3cdfdb9bc53e4587f28205185 100644 (file)
@@ -7,6 +7,7 @@ use std::slice;
 
 use anyhow::{bail, Context as _};
 use glob::glob;
+use itertools::*;
 use log::debug;
 use url::Url;
 
@@ -20,6 +21,7 @@ use crate::ops;
 use crate::sources::{PathSource, CRATES_IO_INDEX, CRATES_IO_REGISTRY};
 use crate::util::errors::{CargoResult, ManifestError};
 use crate::util::interning::InternedString;
+use crate::util::lev_distance;
 use crate::util::toml::{read_manifest, TomlDependency, TomlProfiles};
 use crate::util::{config::ConfigRelativePath, Config, Filesystem, IntoUrl};
 use cargo_util::paths;
@@ -1075,89 +1077,298 @@ impl<'cfg> Workspace<'cfg> {
         }
     }
 
-    /// New command-line feature selection behavior with resolver = "2" or the
-    /// root of a virtual workspace. See `allows_new_cli_feature_behavior`.
-    fn members_with_features_new(
+    /// Returns the requested features for the given member.
+    /// This filters out any named features that the member does not have.
+    fn collect_matching_features(
+        member: &Package,
+        cli_features: &CliFeatures,
+        found_features: &mut BTreeSet<FeatureValue>,
+    ) -> CliFeatures {
+        if cli_features.features.is_empty() || cli_features.all_features {
+            return cli_features.clone();
+        }
+
+        // Only include features this member defines.
+        let summary = member.summary();
+
+        // Features defined in the manifest
+        let summary_features = summary.features();
+
+        // Dependency name -> dependency
+        let dependencies: BTreeMap<InternedString, &Dependency> = summary
+            .dependencies()
+            .iter()
+            .map(|dep| (dep.name_in_toml(), dep))
+            .collect();
+
+        // Features that enable optional dependencies
+        let optional_dependency_names: BTreeSet<_> = dependencies
+            .iter()
+            .filter(|(_, dep)| dep.is_optional())
+            .map(|(name, _)| name)
+            .copied()
+            .collect();
+
+        let mut features = BTreeSet::new();
+
+        // Checks if a member contains the given feature.
+        let summary_or_opt_dependency_feature = |feature: &InternedString| -> bool {
+            summary_features.contains_key(feature) || optional_dependency_names.contains(feature)
+        };
+
+        for feature in cli_features.features.iter() {
+            match feature {
+                FeatureValue::Feature(f) => {
+                    if summary_or_opt_dependency_feature(f) {
+                        // feature exists in this member.
+                        features.insert(feature.clone());
+                        found_features.insert(feature.clone());
+                    }
+                }
+                // This should be enforced by CliFeatures.
+                FeatureValue::Dep { .. }
+                | FeatureValue::DepFeature {
+                    dep_prefix: true, ..
+                } => panic!("unexpected dep: syntax {}", feature),
+                FeatureValue::DepFeature {
+                    dep_name,
+                    dep_feature,
+                    dep_prefix: _,
+                    weak: _,
+                } => {
+                    if dependencies.contains_key(dep_name) {
+                        // pkg/feat for a dependency.
+                        // Will rely on the dependency resolver to validate `dep_feature`.
+                        features.insert(feature.clone());
+                        found_features.insert(feature.clone());
+                    } else if *dep_name == member.name()
+                        && summary_or_opt_dependency_feature(dep_feature)
+                    {
+                        // member/feat where "feat" is a feature in member.
+                        //
+                        // `weak` can be ignored here, because the member
+                        // either is or isn't being built.
+                        features.insert(FeatureValue::Feature(*dep_feature));
+                        found_features.insert(feature.clone());
+                    }
+                }
+            }
+        }
+        CliFeatures {
+            features: Rc::new(features),
+            all_features: false,
+            uses_default_features: cli_features.uses_default_features,
+        }
+    }
+
+    fn report_unknown_features_error(
         &self,
         specs: &[PackageIdSpec],
         cli_features: &CliFeatures,
-    ) -> CargoResult<Vec<(&Package, CliFeatures)>> {
-        // Keep track of which features matched *any* member, to produce an error
-        // if any of them did not match anywhere.
-        let mut found: BTreeSet<FeatureValue> = BTreeSet::new();
+        found_features: &BTreeSet<FeatureValue>,
+    ) -> CargoResult<()> {
+        // Keeps track of which features were contained in summary of `member` to suggest similar features in errors
+        let mut summary_features: Vec<InternedString> = Default::default();
 
-        // Returns the requested features for the given member.
-        // This filters out any named features that the member does not have.
-        let mut matching_features = |member: &Package| -> CliFeatures {
-            if cli_features.features.is_empty() || cli_features.all_features {
-                return cli_features.clone();
-            }
+        // Keeps track of `member` dependencies (`dep/feature`) and their features names to suggest similar features in error
+        let mut dependencies_features: BTreeMap<InternedString, &[InternedString]> =
+            Default::default();
+
+        // Keeps track of `member` optional dependencies names (which can be enabled with feature) to suggest similar features in error
+        let mut optional_dependency_names: Vec<InternedString> = Default::default();
+
+        // Keeps track of which features were contained in summary of `member` to suggest similar features in errors
+        let mut summary_features_per_member: BTreeMap<&Package, BTreeSet<InternedString>> =
+            Default::default();
+
+        // Keeps track of `member` optional dependencies (which can be enabled with feature) to suggest similar features in error
+        let mut optional_dependency_names_per_member: BTreeMap<&Package, BTreeSet<InternedString>> =
+            Default::default();
+
+        for member in self
+            .members()
+            .filter(|m| specs.iter().any(|spec| spec.matches(m.package_id())))
+        {
             // Only include features this member defines.
             let summary = member.summary();
-            let member_features = summary.features();
-            let mut features = BTreeSet::new();
-
-            // Checks if a member contains the given feature.
-            let contains = |feature: InternedString| -> bool {
-                member_features.contains_key(&feature)
-                    || summary
-                        .dependencies()
+
+            // Features defined in the manifest
+            summary_features.extend(summary.features().keys());
+            summary_features_per_member
+                .insert(member, summary.features().keys().copied().collect());
+
+            // Dependency name -> dependency
+            let dependencies: BTreeMap<InternedString, &Dependency> = summary
+                .dependencies()
+                .iter()
+                .map(|dep| (dep.name_in_toml(), dep))
+                .collect();
+
+            dependencies_features.extend(
+                dependencies
+                    .iter()
+                    .map(|(name, dep)| (*name, dep.features())),
+            );
+
+            // Features that enable optional dependencies
+            let optional_dependency_names_raw: BTreeSet<_> = dependencies
+                .iter()
+                .filter(|(_, dep)| dep.is_optional())
+                .map(|(name, _)| name)
+                .copied()
+                .collect();
+
+            optional_dependency_names.extend(optional_dependency_names_raw.iter());
+            optional_dependency_names_per_member.insert(member, optional_dependency_names_raw);
+        }
+
+        let levenshtein_test =
+            |a: InternedString, b: InternedString| lev_distance(a.as_str(), b.as_str()) < 4;
+
+        let suggestions: Vec<_> = cli_features
+            .features
+            .difference(found_features)
+            .map(|feature| match feature {
+                // Simple feature, check if any of the optional dependency features or member features are close enough
+                FeatureValue::Feature(typo) => {
+                    // Finds member features which are similar to the requested feature.
+                    let summary_features = summary_features
                         .iter()
-                        .any(|dep| dep.is_optional() && dep.name_in_toml() == feature)
-            };
+                        .filter(move |feature| levenshtein_test(**feature, *typo));
 
-            for feature in cli_features.features.iter() {
-                match feature {
-                    FeatureValue::Feature(f) => {
-                        if contains(*f) {
-                            // feature exists in this member.
-                            features.insert(feature.clone());
-                            found.insert(feature.clone());
-                        }
-                    }
-                    // This should be enforced by CliFeatures.
-                    FeatureValue::Dep { .. }
-                    | FeatureValue::DepFeature {
-                        dep_prefix: true, ..
-                    } => panic!("unexpected dep: syntax {}", feature),
-                    FeatureValue::DepFeature {
-                        dep_name,
-                        dep_feature,
-                        dep_prefix: _,
-                        weak: _,
-                    } => {
-                        if summary
-                            .dependencies()
-                            .iter()
-                            .any(|dep| dep.name_in_toml() == *dep_name)
-                        {
-                            // pkg/feat for a dependency.
-                            // Will rely on the dependency resolver to validate `dep_feature`.
-                            features.insert(feature.clone());
-                            found.insert(feature.clone());
-                        } else if *dep_name == member.name() && contains(*dep_feature) {
-                            // member/feat where "feat" is a feature in member.
-                            //
-                            // `weak` can be ignored here, because the member
-                            // either is or isn't being built.
-                            features.insert(FeatureValue::Feature(*dep_feature));
-                            found.insert(feature.clone());
-                        }
-                    }
+                    // Finds optional dependencies which name is similar to the feature
+                    let optional_dependency_features = optional_dependency_names
+                        .iter()
+                        .filter(move |feature| levenshtein_test(**feature, *typo));
+
+                    summary_features
+                        .chain(optional_dependency_features)
+                        .map(|s| s.to_string())
+                        .collect::<Vec<_>>()
                 }
-            }
-            CliFeatures {
-                features: Rc::new(features),
-                all_features: false,
-                uses_default_features: cli_features.uses_default_features,
-            }
-        };
+                FeatureValue::Dep { .. }
+                | FeatureValue::DepFeature {
+                    dep_prefix: true, ..
+                } => panic!("unexpected dep: syntax {}", feature),
+                FeatureValue::DepFeature {
+                    dep_name,
+                    dep_feature,
+                    dep_prefix: _,
+                    weak: _,
+                } => {
+                    // Finds set of `pkg/feat` that are very similar to current `pkg/feat`.
+                    let pkg_feat_similar = dependencies_features
+                        .iter()
+                        .filter(|(name, _)| levenshtein_test(**name, *dep_name))
+                        .map(|(name, features)| {
+                            (
+                                name,
+                                features
+                                    .iter()
+                                    .filter(|feature| levenshtein_test(**feature, *dep_feature))
+                                    .collect::<Vec<_>>(),
+                            )
+                        })
+                        .map(|(name, features)| {
+                            features
+                                .into_iter()
+                                .map(move |feature| format!("{}/{}", name, feature))
+                        })
+                        .flatten();
+
+                    // Finds set of `member/optional_dep` features which name is similar to current `pkg/feat`.
+                    let optional_dependency_features = optional_dependency_names_per_member
+                        .iter()
+                        .filter(|(package, _)| levenshtein_test(package.name(), *dep_name))
+                        .map(|(package, optional_dependencies)| {
+                            optional_dependencies
+                                .into_iter()
+                                .filter(|optional_dependency| {
+                                    levenshtein_test(**optional_dependency, *dep_name)
+                                })
+                                .map(move |optional_dependency| {
+                                    format!("{}/{}", package.name(), optional_dependency)
+                                })
+                        })
+                        .flatten();
+
+                    // Finds set of `member/feat` features which name is similar to current `pkg/feat`.
+                    let summary_features = summary_features_per_member
+                        .iter()
+                        .filter(|(package, _)| levenshtein_test(package.name(), *dep_name))
+                        .map(|(package, summary_features)| {
+                            summary_features
+                                .into_iter()
+                                .filter(|summary_feature| {
+                                    levenshtein_test(**summary_feature, *dep_name)
+                                })
+                                .map(move |summary_feature| {
+                                    format!("{}/{}", package.name(), summary_feature)
+                                })
+                        })
+                        .flatten();
+
+                    pkg_feat_similar
+                        .chain(optional_dependency_features)
+                        .chain(summary_features)
+                        .collect::<Vec<_>>()
+                }
+            })
+            .map(|v| v.into_iter())
+            .flatten()
+            .unique()
+            .filter(|element| {
+                !cli_features
+                    .features
+                    .contains(&FeatureValue::new(InternedString::new(element)))
+            })
+            .sorted()
+            .take(5)
+            .collect();
+
+        let unknown: Vec<_> = cli_features
+            .features
+            .difference(found_features)
+            .map(|feature| feature.to_string())
+            .sorted()
+            .collect();
+
+        if suggestions.is_empty() {
+            bail!(
+                "none of the selected packages contains these features: {}",
+                unknown.join(", ")
+            );
+        } else {
+            bail!(
+                "none of the selected packages contains these features: {}, did you mean: {}?",
+                unknown.join(", "),
+                suggestions.join(", ")
+            );
+        }
+    }
+
+    /// New command-line feature selection behavior with resolver = "2" or the
+    /// root of a virtual workspace. See `allows_new_cli_feature_behavior`.
+    fn members_with_features_new(
+        &self,
+        specs: &[PackageIdSpec],
+        cli_features: &CliFeatures,
+    ) -> CargoResult<Vec<(&Package, CliFeatures)>> {
+        // Keeps track of which features matched `member` to produce an error
+        // if any of them did not match anywhere.
+        let mut found_features = Default::default();
 
         let members: Vec<(&Package, CliFeatures)> = self
             .members()
             .filter(|m| specs.iter().any(|spec| spec.matches(m.package_id())))
-            .map(|m| (m, matching_features(m)))
+            .map(|m| {
+                (
+                    m,
+                    Workspace::collect_matching_features(m, cli_features, &mut found_features),
+                )
+            })
             .collect();
+
         if members.is_empty() {
             // `cargo build -p foo`, where `foo` is not a member.
             // Do not allow any command-line flags (defaults only).
@@ -1174,18 +1385,8 @@ impl<'cfg> Workspace<'cfg> {
                 .map(|m| (m, CliFeatures::new_all(false)))
                 .collect());
         }
-        if *cli_features.features != found {
-            let mut missing: Vec<_> = cli_features
-                .features
-                .difference(&found)
-                .map(|fv| fv.to_string())
-                .collect();
-            missing.sort();
-            // TODO: typo suggestions would be good here.
-            bail!(
-                "none of the selected packages contains these features: {}",
-                missing.join(", ")
-            );
+        if *cli_features.features != found_features {
+            self.report_unknown_features_error(specs, cli_features, &found_features)?;
         }
         Ok(members)
     }
index 957ed921cf3e396bd39387f0bf06f142d425d2af..e7605581b3548c5fc91433775f3cdf9ef0a7735b 100644 (file)
@@ -67,13 +67,15 @@ fn virtual_no_default_features() {
     p.cargo("check --features foo")
         .masquerade_as_nightly_cargo()
         .with_status(101)
-        .with_stderr("[ERROR] none of the selected packages contains these features: foo")
+        .with_stderr(
+            "[ERROR] none of the selected packages contains these features: foo, did you mean: f1?",
+        )
         .run();
 
     p.cargo("check --features a/dep1,b/f1,b/f2,f2")
         .masquerade_as_nightly_cargo()
         .with_status(101)
-        .with_stderr("[ERROR] none of the selected packages contains these features: b/f2, f2")
+        .with_stderr("[ERROR] none of the selected packages contains these features: b/f2, f2, did you mean: b/f1, f1?")
         .run();
 }