Auto merge of #8814 - ehuss:warn-feature-syntax, r=alexcrichton

Add a future-compatibility warning on allowed feature name characters.

This adds a restriction on the valid syntax of a feature name. An warning is issued if a feature does not match the new validation, with the intent that it will be an error in the future.

The new restriction is:

* The first character must be a [Unicode XID start character](https://unicode.org/reports/tr31/) (most letters), a digit, or `_`.
* Subsequent characters must be a [Unicode XID continue character](https://unicode.org/reports/tr31/) (a digit, `_`, or most letters), `-`, or `+`.

The changes around passing in `config` to `Summary` can mostly be reverted when this is changed to an error.

I'm a little concerned that we don't have a mechanism to silence the warning. Should we add one?

Author: bors

crates/resolver-tests/src/lib.rs

@@ -24,15 +24,15 @@ use proptest::string::string_regex;
 use varisat::{self, ExtendFormula};
 
 pub fn resolve(deps: Vec<Dependency>, registry: &[Summary]) -> CargoResult<Vec<PackageId>> {
-    resolve_with_config(deps, registry, None)
+    resolve_with_config(deps, registry, &Config::default().unwrap())
 }
 
 pub fn resolve_and_validated(
     deps: Vec<Dependency>,
     registry: &[Summary],
     sat_resolve: Option<SatResolve>,
 ) -> CargoResult<Vec<PackageId>> {
-    let resolve = resolve_with_config_raw(deps.clone(), registry, None);
+    let resolve = resolve_with_config_raw(deps.clone(), registry, &Config::default().unwrap());
 
     match resolve {
         Err(e) => {
@@ -109,7 +109,7 @@ pub fn resolve_and_validated(
 pub fn resolve_with_config(
     deps: Vec<Dependency>,
     registry: &[Summary],
-    config: Option<&Config>,
+    config: &Config,
 ) -> CargoResult<Vec<PackageId>> {
     let resolve = resolve_with_config_raw(deps, registry, config)?;
     Ok(resolve.sort())
@@ -118,7 +118,7 @@ pub fn resolve_with_config(
 pub fn resolve_with_config_raw(
     deps: Vec<Dependency>,
     registry: &[Summary],
-    config: Option<&Config>,
+    config: &Config,
 ) -> CargoResult<Resolve> {
     struct MyRegistry<'a> {
         list: &'a [Summary],
@@ -170,15 +170,22 @@ pub fn resolve_with_config_raw(
         list: registry,
         used: HashSet::new(),
     };
-    let summary = Summary::new(pkg_id("root"), deps, &BTreeMap::new(), None::<&String>).unwrap();
+    let summary = Summary::new(
+        config,
+        pkg_id("root"),
+        deps,
+        &BTreeMap::new(),
+        None::<&String>,
+    )
+    .unwrap();
     let opts = ResolveOpts::everything();
     let start = Instant::now();
     let resolve = resolver::resolve(
         &[(summary, opts)],
         &[],
         &mut registry,
         &HashSet::new(),
-        config,
+        Some(config),
         true,
     );
 
@@ -564,7 +571,14 @@ pub fn pkg_dep<T: ToPkgId>(name: T, dep: Vec<Dependency>) -> Summary {
     } else {
         None
     };
-    Summary::new(name.to_pkgid(), dep, &BTreeMap::new(), link).unwrap()
+    Summary::new(
+        &Config::default().unwrap(),
+        name.to_pkgid(),
+        dep,
+        &BTreeMap::new(),
+        link,
+    )
+    .unwrap()
 }
 
 pub fn pkg_id(name: &str) -> PackageId {
@@ -585,14 +599,22 @@ pub fn pkg_loc(name: &str, loc: &str) -> Summary {
     } else {
         None
     };
-    Summary::new(pkg_id_loc(name, loc), Vec::new(), &BTreeMap::new(), link).unwrap()
+    Summary::new(
+        &Config::default().unwrap(),
+        pkg_id_loc(name, loc),
+        Vec::new(),
+        &BTreeMap::new(),
+        link,
+    )
+    .unwrap()
 }
 
 pub fn remove_dep(sum: &Summary, ind: usize) -> Summary {
     let mut deps = sum.dependencies().to_vec();
     deps.remove(ind);
     // note: more things will need to be copied over in the future, but it works for now.
     Summary::new(
+        &Config::default().unwrap(),
         sum.package_id(),
         deps,
         &BTreeMap::new(),

crates/resolver-tests/tests/resolve.rs

@@ -87,7 +87,7 @@ proptest! {
             let mres = resolve_with_config(
                 vec![dep_req(&this.name(), &format!("={}", this.version()))],
                 &reg,
-                Some(&config),
+                &config,
             );
 
             prop_assert_eq!(
@@ -584,7 +584,7 @@ fn test_resolving_minimum_version_with_transitive_deps() {
     let res = resolve_with_config(
         vec![dep_req("foo", "1.0.0"), dep_req("bar", "1.0.0")],
         &reg,
-        Some(&config),
+        &config,
     )
     .unwrap();
 

src/cargo/core/summary.rs

@@ -1,6 +1,6 @@
 use crate::core::{Dependency, PackageId, SourceId};
 use crate::util::interning::InternedString;
-use crate::util::CargoResult;
+use crate::util::{CargoResult, Config};
 use anyhow::bail;
 use semver::Version;
 use std::collections::{BTreeMap, HashMap, HashSet};
@@ -31,6 +31,7 @@ struct Inner {
 
 impl Summary {
     pub fn new(
+        config: &Config,
         pkg_id: PackageId,
         dependencies: Vec<Dependency>,
         features: &BTreeMap<InternedString, Vec<InternedString>>,
@@ -49,7 +50,8 @@ impl Summary {
                 )
             }
         }
-        let (feature_map, has_namespaced_features) = build_feature_map(features, &dependencies)?;
+        let (feature_map, has_namespaced_features) =
+            build_feature_map(config, pkg_id, features, &dependencies)?;
         Ok(Summary {
             inner: Rc::new(Inner {
                 package_id: pkg_id,
@@ -161,6 +163,8 @@ impl Hash for Summary {
 /// included a `dep:` prefixed namespaced feature (used for gating on
 /// nightly).
 fn build_feature_map(
+    config: &Config,
+    pkg_id: PackageId,
     features: &BTreeMap<InternedString, Vec<InternedString>>,
     dependencies: &[Dependency],
 ) -> CargoResult<(FeatureMap, bool)> {
@@ -223,6 +227,7 @@ fn build_feature_map(
                 feature
             );
         }
+        validate_feature_name(config, pkg_id, feature)?;
         for fv in fvs {
             // Find data for the referenced dependency...
             let dep_data = {
@@ -400,3 +405,34 @@ impl fmt::Display for FeatureValue {
 }
 
 pub type FeatureMap = BTreeMap<InternedString, Vec<FeatureValue>>;
+
+fn validate_feature_name(config: &Config, pkg_id: PackageId, name: &str) -> CargoResult<()> {
+    let mut chars = name.chars();
+    const FUTURE: &str = "This was previously accepted but is being phased out; \
+        it will become a hard error in a future release.\n\
+        For more information, see issue #8813 <https://github.com/rust-lang/cargo/issues/8813>, \
+        and please leave a comment if this will be a problem for your project.";
+    if let Some(ch) = chars.next() {
+        if !(unicode_xid::UnicodeXID::is_xid_start(ch) || ch == '_' || ch.is_digit(10)) {
+            config.shell().warn(&format!(
+                "invalid character `{}` in feature `{}` in package {}, \
+                the first character must be a Unicode XID start character or digit \
+                (most letters or `_` or `0` to `9`)\n\
+                {}",
+                ch, name, pkg_id, FUTURE
+            ))?;
+        }
+    }
+    for ch in chars {
+        if !(unicode_xid::UnicodeXID::is_xid_continue(ch) || ch == '-' || ch == '+') {
+            config.shell().warn(&format!(
+                "invalid character `{}` in feature `{}` in package {}, \
+                characters must be Unicode XID characters or `+` \
+                (numbers, `+`, `-`, `_`, or most letters)\n\
+                {}",
+                ch, name, pkg_id, FUTURE
+            ))?;
+        }
+    }
+    Ok(())
+}

src/cargo/sources/registry/index.rs

@@ -270,6 +270,7 @@ impl<'cfg> RegistryIndex<'cfg> {
         'a: 'b,
     {
         let source_id = self.source_id;
+        let config = self.config;
         let namespaced_features = self.config.cli_unstable().namespaced_features;
 
         // First up actually parse what summaries we have available. If Cargo
@@ -289,13 +290,15 @@ impl<'cfg> RegistryIndex<'cfg> {
             .versions
             .iter_mut()
             .filter_map(move |(k, v)| if req.matches(k) { Some(v) } else { None })
-            .filter_map(move |maybe| match maybe.parse(raw_data, source_id) {
-                Ok(summary) => Some(summary),
-                Err(e) => {
-                    info!("failed to parse `{}` registry package: {}", name, e);
-                    None
-                }
-            })
+            .filter_map(
+                move |maybe| match maybe.parse(config, raw_data, source_id) {
+                    Ok(summary) => Some(summary),
+                    Err(e) => {
+                        info!("failed to parse `{}` registry package: {}", name, e);
+                        None
+                    }
+                },
+            )
             .filter(move |is| is.summary.unstable_gate(namespaced_features).is_ok()))
     }
 
@@ -521,7 +524,7 @@ impl Summaries {
                 // allow future cargo implementations to break the
                 // interpretation of each line here and older cargo will simply
                 // ignore the new lines.
-                let summary = match IndexSummary::parse(line, source_id) {
+                let summary = match IndexSummary::parse(config, line, source_id) {
                     Ok(summary) => summary,
                     Err(e) => {
                         log::info!("failed to parse {:?} registry package: {}", relative, e);
@@ -684,12 +687,17 @@ impl MaybeIndexSummary {
     /// Does nothing if this is already `Parsed`, and otherwise the `raw_data`
     /// passed in is sliced with the bounds in `Unparsed` and then actually
     /// parsed.
-    fn parse(&mut self, raw_data: &[u8], source_id: SourceId) -> CargoResult<&IndexSummary> {
+    fn parse(
+        &mut self,
+        config: &Config,
+        raw_data: &[u8],
+        source_id: SourceId,
+    ) -> CargoResult<&IndexSummary> {
         let (start, end) = match self {
             MaybeIndexSummary::Unparsed { start, end } => (*start, *end),
             MaybeIndexSummary::Parsed(summary) => return Ok(summary),
         };
-        let summary = IndexSummary::parse(&raw_data[start..end], source_id)?;
+        let summary = IndexSummary::parse(config, &raw_data[start..end], source_id)?;
         *self = MaybeIndexSummary::Parsed(summary);
         match self {
             MaybeIndexSummary::Unparsed { .. } => unreachable!(),
@@ -709,7 +717,7 @@ impl IndexSummary {
     /// a package.
     ///
     /// The `line` provided is expected to be valid JSON.
-    fn parse(line: &[u8], source_id: SourceId) -> CargoResult<IndexSummary> {
+    fn parse(config: &Config, line: &[u8], source_id: SourceId) -> CargoResult<IndexSummary> {
         let RegistryPackage {
             name,
             vers,
@@ -725,7 +733,7 @@ impl IndexSummary {
             .into_iter()
             .map(|dep| dep.into_dep(source_id))
             .collect::<CargoResult<Vec<_>>>()?;
-        let mut summary = Summary::new(pkgid, deps, &features, links)?;
+        let mut summary = Summary::new(config, pkgid, deps, &features, links)?;
         summary.set_checksum(cksum);
         Ok(IndexSummary {
             summary,

src/cargo/util/toml/mod.rs

@@ -1191,6 +1191,7 @@ impl TomlManifest {
         let empty_features = BTreeMap::new();
 
         let summary = Summary::new(
+            config,
             pkgid,
             deps,
             me.features.as_ref().unwrap_or(&empty_features),