Auto merge of #5811 - alexcrichton:rename-crate-feature-names, r=ehuss

Use listed dependency name for feature names

This commit updates the implementation of renamed dependencies to use the listed
name of a dependency in Cargo.toml for the name of the associated feature,
rather than using the package name. This'll allow disambiguating between
different packages of the same name and was the intention all along!

Closes #5753

Author: bors

src/cargo/core/compiler/build_context/mod.rs

@@ -104,7 +104,7 @@ impl<'a, 'cfg> BuildContext<'a, 'cfg> {
 
         let crate_name = dep.target.crate_name();
         let mut names = deps.iter()
-            .map(|d| d.rename().unwrap_or(&crate_name));
+            .map(|d| d.rename().map(|s| s.as_str()).unwrap_or(&crate_name));
         let name = names.next().unwrap_or(&crate_name);
         for n in names {
             if n == name {

src/cargo/core/compiler/context/unit_dependencies.rs

@@ -118,7 +118,7 @@ fn compute_deps<'a, 'cfg>(
 
             // If the dependency is optional, then we're only activating it
             // if the corresponding feature was activated
-            if dep.is_optional() && !bcx.resolve.features(id).contains(&*dep.name()) {
+            if dep.is_optional() && !bcx.resolve.features(id).contains(&*dep.name_in_toml()) {
                 return false;
             }
 

src/cargo/core/dependency.rs

@@ -28,7 +28,7 @@ struct Inner {
     specified_req: bool,
     kind: Kind,
     only_match_name: bool,
-    rename: Option<String>,
+    rename: Option<InternedString>,
 
     optional: bool,
     default_features: bool,
@@ -65,15 +65,15 @@ impl ser::Serialize for Dependency {
         S: ser::Serializer,
     {
         SerializedDependency {
-            name: &*self.name(),
+            name: &*self.package_name(),
             source: self.source_id(),
             req: self.version_req().to_string(),
             kind: self.kind(),
             optional: self.is_optional(),
             uses_default_features: self.uses_default_features(),
             features: self.features(),
             target: self.platform(),
-            rename: self.rename(),
+            rename: self.rename().map(|s| s.as_str()),
         }.serialize(s)
     }
 }
@@ -208,7 +208,49 @@ impl Dependency {
         &self.inner.req
     }
 
-    pub fn name(&self) -> InternedString {
+    /// This is the name of this `Dependency` as listed in `Cargo.toml`.
+    ///
+    /// Or in other words, this is what shows up in the `[dependencies]` section
+    /// on the left hand side. This is **not** the name of the package that's
+    /// being depended on as the dependency can be renamed. For that use
+    /// `package_name` below.
+    ///
+    /// Both of the dependencies below return `foo` for `name_in_toml`:
+    ///
+    /// ```toml
+    /// [dependencies]
+    /// foo = "0.1"
+    /// ```
+    ///
+    /// and ...
+    ///
+    /// ```toml
+    /// [dependencies]
+    /// foo = { version = "0.1", package = 'bar' }
+    /// ```
+    pub fn name_in_toml(&self) -> InternedString {
+        self.rename().unwrap_or(self.inner.name)
+    }
+
+    /// The name of the package that this `Dependency` depends on.
+    ///
+    /// Usually this is what's written on the left hand side of a dependencies
+    /// section, but it can also be renamed via the `package` key.
+    ///
+    /// Both of the dependencies below return `foo` for `package_name`:
+    ///
+    /// ```toml
+    /// [dependencies]
+    /// foo = "0.1"
+    /// ```
+    ///
+    /// and ...
+    ///
+    /// ```toml
+    /// [dependencies]
+    /// bar = { version = "0.1", package = 'foo' }
+    /// ```
+    pub fn package_name(&self) -> InternedString {
         self.inner.name
     }
 
@@ -239,8 +281,12 @@ impl Dependency {
         self.inner.platform.as_ref()
     }
 
-    pub fn rename(&self) -> Option<&str> {
-        self.inner.rename.as_ref().map(|s| &**s)
+    /// The renamed name of this dependency, if any.
+    ///
+    /// If the `package` key is used in `Cargo.toml` then this returns the same
+    /// value as `name_in_toml`.
+    pub fn rename(&self) -> Option<InternedString> {
+        self.inner.rename
     }
 
     pub fn set_kind(&mut self, kind: Kind) -> &mut Dependency {
@@ -285,7 +331,7 @@ impl Dependency {
     }
 
     pub fn set_rename(&mut self, rename: &str) -> &mut Dependency {
-        Rc::make_mut(&mut self.inner).rename = Some(rename.to_string());
+        Rc::make_mut(&mut self.inner).rename = Some(InternedString::new(rename));
         self
     }
 
@@ -295,7 +341,7 @@ impl Dependency {
         assert!(self.inner.req.matches(id.version()));
         trace!(
             "locking dep from `{}` with `{}` at {} to {}",
-            self.name(),
+            self.package_name(),
             self.version_req(),
             self.source_id(),
             id
@@ -346,7 +392,7 @@ impl Dependency {
 
     /// Returns true if the package (`sum`) can fulfill this dependency request.
     pub fn matches_ignoring_source(&self, id: &PackageId) -> bool {
-        self.name() == id.name() && self.version_req().matches(id.version())
+        self.package_name() == id.name() && self.version_req().matches(id.version())
     }
 
     /// Returns true if the package (`id`) can fulfill this dependency request.

src/cargo/core/registry.rs

@@ -197,7 +197,7 @@ impl<'cfg> PackageRegistry<'cfg> {
         // of summaries which should be the same length as `deps` above.
         let unlocked_summaries = deps.iter()
             .map(|dep| {
-                debug!("registring a patch for `{}` with `{}`", url, dep.name());
+                debug!("registring a patch for `{}` with `{}`", url, dep.package_name());
 
                 // Go straight to the source for resolving `dep`. Load it as we
                 // normally would and then ask it directly for the list of summaries
@@ -207,7 +207,7 @@ impl<'cfg> PackageRegistry<'cfg> {
                         format_err!(
                             "failed to load source for a dependency \
                              on `{}`",
-                            dep.name()
+                            dep.package_name()
                         )
                     })?;
 
@@ -223,22 +223,22 @@ impl<'cfg> PackageRegistry<'cfg> {
                         "patch for `{}` in `{}` did not resolve to any crates. If this is \
                          unexpected, you may wish to consult: \
                          https://github.com/rust-lang/cargo/issues/4678",
-                        dep.name(),
+                        dep.package_name(),
                         url
                     ),
                 };
                 if summaries.next().is_some() {
                     bail!(
                         "patch for `{}` in `{}` resolved to more than one candidate",
-                        dep.name(),
+                        dep.package_name(),
                         url
                     )
                 }
                 if summary.package_id().source_id().url() == url {
                     bail!(
                         "patch for `{}` in `{}` points to the same source, but \
                          patches must point to different sources",
-                        dep.name(),
+                        dep.package_name(),
                         url
                     );
                 }
@@ -306,7 +306,7 @@ impl<'cfg> PackageRegistry<'cfg> {
     fn query_overrides(&mut self, dep: &Dependency) -> CargoResult<Option<Summary>> {
         for s in self.overrides.iter() {
             let src = self.sources.get_mut(s).unwrap();
-            let dep = Dependency::new_override(&*dep.name(), s);
+            let dep = Dependency::new_override(&*dep.package_name(), s);
             let mut results = src.query_vec(&dep)?;
             if !results.is_empty() {
                 return Ok(Some(results.remove(0)));
@@ -369,21 +369,21 @@ http://doc.crates.io/specifying-dependencies.html#overriding-dependencies
                  modified to not match the previously resolved version\n\n\
                  {}",
                 override_summary.package_id().name(),
-                dep.name(),
+                dep.package_name(),
                 boilerplate
             );
             self.source_config.config().shell().warn(&msg)?;
             return Ok(());
         }
 
-        if let Some(id) = real_deps.get(0) {
+        if let Some(dep) = real_deps.get(0) {
             let msg = format!(
                 "\
                 path override for crate `{}` has altered the original list of
                 dependencies; the dependency on `{}` was removed\n\n
                 {}",
                 override_summary.package_id().name(),
-                id.name(),
+                dep.package_name(),
                 boilerplate
             );
             self.source_config.config().shell().warn(&msg)?;
@@ -439,7 +439,7 @@ impl<'cfg> Registry for PackageRegistry<'cfg> {
                          with `{}`, \
                          looking at sources",
                         patches.len(),
-                        dep.name(),
+                        dep.package_name(),
                         dep.source_id(),
                         dep.version_req()
                     );
@@ -451,7 +451,7 @@ impl<'cfg> Registry for PackageRegistry<'cfg> {
                         format_err!(
                             "failed to load source for a dependency \
                              on `{}`",
-                            dep.name()
+                            dep.package_name()
                         )
                     })?;
 
@@ -542,7 +542,7 @@ fn lock(locked: &LockedMap, patches: &HashMap<Url, Vec<PackageId>>, summary: Sum
         None => summary,
     };
     summary.map_dependencies(|dep| {
-        trace!("\t{}/{}/{}", dep.name(), dep.version_req(), dep.source_id());
+        trace!("\t{}/{}/{}", dep.package_name(), dep.version_req(), dep.source_id());
 
         // If we've got a known set of overrides for this summary, then
         // one of a few cases can arise:
@@ -578,7 +578,7 @@ fn lock(locked: &LockedMap, patches: &HashMap<Url, Vec<PackageId>>, summary: Sum
         // If anything does then we lock it to that and move on.
         let v = locked
             .get(dep.source_id())
-            .and_then(|map| map.get(&*dep.name()))
+            .and_then(|map| map.get(&*dep.package_name()))
             .and_then(|vec| vec.iter().find(|&&(ref id, _)| dep.matches_id(id)));
         if let Some(&(ref id, _)) = v {
             trace!("\tsecond hit on {}", id);
@@ -592,7 +592,7 @@ fn lock(locked: &LockedMap, patches: &HashMap<Url, Vec<PackageId>>, summary: Sum
         let v = patches.get(dep.source_id().url()).map(|vec| {
             let dep2 = dep.clone();
             let mut iter = vec.iter().filter(move |p| {
-                dep2.name() == p.name() && dep2.version_req().matches(p.version())
+                dep2.matches_ignoring_source(p)
             });
             (iter.next(), iter)
         });

src/cargo/core/resolver/conflict_cache.rs

@@ -81,7 +81,7 @@ impl ConflictCache {
             .entry(dep.clone())
             .or_insert_with(Vec::new);
         if !past.contains(con) {
-            trace!("{} adding a skip {:?}", dep.name(), con);
+            trace!("{} adding a skip {:?}", dep.package_name(), con);
             past.push(con.clone());
             for c in con.keys() {
                 self.dep_from_pid

src/cargo/core/resolver/context.rs

@@ -128,7 +128,7 @@ impl Context {
 
     pub fn prev_active(&self, dep: &Dependency) -> &[Summary] {
         self.activations
-            .get(&(dep.name(), dep.source_id().clone()))
+            .get(&(dep.package_name(), dep.source_id().clone()))
             .map(|v| &v[..])
             .unwrap_or(&[])
     }
@@ -178,27 +178,27 @@ impl Context {
         for dep in deps {
             // Skip optional dependencies, but not those enabled through a
             // feature
-            if dep.is_optional() && !reqs.deps.contains_key(&dep.name()) {
+            if dep.is_optional() && !reqs.deps.contains_key(&dep.name_in_toml()) {
                 continue;
             }
             // So we want this dependency. Move the features we want from
             // `feature_deps` to `ret` and register ourselves as using this
             // name.
-            let base = reqs.deps.get(&dep.name()).unwrap_or(&default_dep);
-            used_features.insert(dep.name());
+            let base = reqs.deps.get(&dep.name_in_toml()).unwrap_or(&default_dep);
+            used_features.insert(dep.name_in_toml());
             let always_required = !dep.is_optional()
                 && !s
                     .dependencies()
                     .iter()
-                    .any(|d| d.is_optional() && d.name() == dep.name());
+                    .any(|d| d.is_optional() && d.name_in_toml() == dep.name_in_toml());
             if always_required && base.0 {
                 self.warnings.push(format!(
                     "Package `{}` does not have feature `{}`. It has a required dependency \
                      with that name, but only optional dependencies can be used as features. \
                      This is currently a warning to ease the transition, but it will become an \
                      error in the future.",
                     s.package_id(),
-                    dep.name()
+                    dep.name_in_toml()
                 ));
             }
             let mut base = base.1.clone();
@@ -220,8 +220,8 @@ impl Context {
         let remaining = reqs
             .deps
             .keys()
-            .filter(|&s| !used_features.contains(s))
-            .map(|s| s.as_str())
+            .cloned()
+            .filter(|s| !used_features.contains(s))
             .collect::<Vec<_>>();
         if !remaining.is_empty() {
             let features = remaining.join(", ");
@@ -298,10 +298,10 @@ fn build_requirements<'a, 'b: 'a>(
             all_features: true, ..
         } => {
             for key in s.features().keys() {
-                reqs.require_feature(InternedString::new(&key))?;
+                reqs.require_feature(*key)?;
             }
             for dep in s.dependencies().iter().filter(|d| d.is_optional()) {
-                reqs.require_dependency(dep.name());
+                reqs.require_dependency(dep.name_in_toml());
             }
         }
         Method::Required {
@@ -389,7 +389,7 @@ impl<'r> Requirements<'r> {
         for fv in self
             .summary
             .features()
-            .get(&feat)
+            .get(feat.as_str())
             .expect("must be a valid feature")
         {
             match *fv {