Auto merge of #48552 - kennytm:lower-unstable-priority, r=nikomatsakis

Lower the priority of unstable methods when picking a candidate.

Previously, when searching for the impl of a method, we do not consider the stability of the impl. This leads to lots of insta-inference-regressions due to method ambiguity when a popular name is chosen. This has happened multiple times in Rust's history e.g.

* `f64::from_bits` #40470
* `Ord::{min, max}` #42496
* `Ord::clamp` #44095 (eventually got reverted due to these breakages)
* `Iterator::flatten` #48115 (recently added)

This PR changes the probing order so that unstable items are considered last. If a stable item is found, the unstable items will not be considered (but a future-incompatible warning will still be emitted), thus allowing stable code continue to function without using qualified names.

Once the unstable feature is stabilized, the ambiguity error will still be emitted, but the user can also use newly stable std methods, while the current situation is that downstream user is forced to update the code without any immediate benefit.

(I hope that we could bring back `Ord::clamp` if this PR is merged.)

3 files changed, 101 insertions, 46 deletions

diff --git a/src/librustc/lint/builtin.rs b/src/librustc/lint/builtin.rs
index b4ed9c269bd..97cfcf0f607 100644
--- a/src/librustc/lint/builtin.rs
+++ b/src/librustc/lint/builtin.rs
@@ -260,6 +260,12 @@ declare_lint! {
     "floating-point literals cannot be used in patterns"
 }
 
+declare_lint! {
+    pub UNSTABLE_NAME_COLLISION,
+    Warn,
+    "detects name collision with an existing but unstable method"
+}
+
 /// Does nothing as a lint pass, but registers some `Lint`s
 /// which are used by other parts of the compiler.
 #[derive(Copy, Clone)]
@@ -307,7 +313,8 @@ impl LintPass for HardwiredLints {
             SINGLE_USE_LIFETIME,
             TYVAR_BEHIND_RAW_POINTER,
             ELIDED_LIFETIME_IN_PATH,
-            BARE_TRAIT_OBJECT
+            BARE_TRAIT_OBJECT,
+            UNSTABLE_NAME_COLLISION,
         )
     }
 }
diff --git a/src/librustc/lint/mod.rs b/src/librustc/lint/mod.rs
index cd038d067a1..1497be2d5ba 100644
--- a/src/librustc/lint/mod.rs
+++ b/src/librustc/lint/mod.rs
@@ -498,15 +498,21 @@ pub fn struct_lint_level<'a>(sess: &'a Session,
 
     // Check for future incompatibility lints and issue a stronger warning.
     let lints = sess.lint_store.borrow();
-    if let Some(future_incompatible) = lints.future_incompatible(LintId::of(lint)) {
-        let future = if let Some(edition) = future_incompatible.edition {
-            format!("the {} edition", edition)
+    let lint_id = LintId::of(lint);
+    if let Some(future_incompatible) = lints.future_incompatible(lint_id) {
+        const STANDARD_MESSAGE: &str =
+            "this was previously accepted by the compiler but is being phased out; \
+             it will become a hard error";
+
+        let explanation = if lint_id == LintId::of(::lint::builtin::UNSTABLE_NAME_COLLISION) {
+            "once this method is added to the standard library, \
+             there will be ambiguity here, which will cause a hard error!"
+                .to_owned()
+        } else if let Some(edition) = future_incompatible.edition {
+            format!("{} in the {} edition!", STANDARD_MESSAGE, edition)
         } else {
-            "a future release".to_owned()
+            format!("{} in a future release!", STANDARD_MESSAGE)
         };
-        let explanation = format!("this was previously accepted by the compiler \
-                                   but is being phased out; \
-                                   it will become a hard error in {}!", future);
         let citation = format!("for more information, see {}",
                                future_incompatible.reference);
         err.warn(&explanation);
diff --git a/src/librustc/middle/stability.rs b/src/librustc/middle/stability.rs
index 16c33d6bd83..29c8ac046b8 100644
--- a/src/librustc/middle/stability.rs
+++ b/src/librustc/middle/stability.rs
@@ -474,6 +474,22 @@ struct Checker<'a, 'tcx: 'a> {
     tcx: TyCtxt<'a, 'tcx, 'tcx>,
 }
 
+/// Result of `TyCtxt::eval_stability`.
+pub enum EvalResult {
+    /// We can use the item because it is stable or we provided the
+    /// corresponding feature gate.
+    Allow,
+    /// We cannot use the item because it is unstable and we did not provide the
+    /// corresponding feature gate.
+    Deny {
+        feature: Symbol,
+        reason: Option<Symbol>,
+        issue: u32,
+    },
+    /// The item does not have the `#[stable]` or `#[unstable]` marker assigned.
+    Unmarked,
+}
+
 impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
     // (See issue #38412)
     fn skip_stability_check_due_to_privacy(self, mut def_id: DefId) -> bool {
@@ -509,14 +525,23 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
         }
     }
 
-    pub fn check_stability(self, def_id: DefId, id: NodeId, span: Span) {
+    /// Evaluates the stability of an item.
+    ///
+    /// Returns `EvalResult::Allow` if the item is stable, or unstable but the corresponding
+    /// `#![feature]` has been provided. Returns `EvalResult::Deny` which describes the offending
+    /// unstable feature otherwise.
+    ///
+    /// If `id` is `Some(_)`, this function will also check if the item at `def_id` has been
+    /// deprecated. If the item is indeed deprecated, we will emit a deprecation lint attached to
+    /// `id`.
+    pub fn eval_stability(self, def_id: DefId, id: Option<NodeId>, span: Span) -> EvalResult {
         if span.allows_unstable() {
             debug!("stability: \
                     skipping span={:?} since it is internal", span);
-            return;
+            return EvalResult::Allow;
         }
 
-        let lint_deprecated = |def_id: DefId, note: Option<Symbol>| {
+        let lint_deprecated = |def_id: DefId, id: NodeId, note: Option<Symbol>| {
             let path = self.item_path_str(def_id);
 
             let msg = if let Some(note) = note {
@@ -526,22 +551,21 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
             };
 
             self.lint_node(lint::builtin::DEPRECATED, id, span, &msg);
+            if id == ast::DUMMY_NODE_ID {
+                span_bug!(span, "emitted a deprecated lint with dummy node id: {:?}", def_id);
+            }
         };
 
         // Deprecated attributes apply in-crate and cross-crate.
-        if let Some(depr_entry) = self.lookup_deprecation_entry(def_id) {
-            let skip = if id == ast::DUMMY_NODE_ID {
-                true
-            } else {
+        if let Some(id) = id {
+            if let Some(depr_entry) = self.lookup_deprecation_entry(def_id) {
                 let parent_def_id = self.hir.local_def_id(self.hir.get_parent(id));
-                self.lookup_deprecation_entry(parent_def_id).map_or(false, |parent_depr| {
-                    parent_depr.same_origin(&depr_entry)
-                })
+                let skip = self.lookup_deprecation_entry(parent_def_id)
+                    .map_or(false, |parent_depr| parent_depr.same_origin(&depr_entry));
+                if !skip {
+                    lint_deprecated(def_id, id, depr_entry.attr.note);
+                }
             };
-
-            if !skip {
-                lint_deprecated(def_id, depr_entry.attr.note);
-            }
         }
 
         let is_staged_api = self.lookup_stability(DefId {
@@ -549,7 +573,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
             ..def_id
         }).is_some();
         if !is_staged_api {
-            return;
+            return EvalResult::Allow;
         }
 
         let stability = self.lookup_stability(def_id);
@@ -558,26 +582,26 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
 
         if let Some(&Stability{rustc_depr: Some(attr::RustcDeprecation { reason, .. }), ..})
                 = stability {
-            if id != ast::DUMMY_NODE_ID {
-                lint_deprecated(def_id, Some(reason));
+            if let Some(id) = id {
+                lint_deprecated(def_id, id, Some(reason));
             }
         }
 
         // Only the cross-crate scenario matters when checking unstable APIs
         let cross_crate = !def_id.is_local();
         if !cross_crate {
-            return
+            return EvalResult::Allow;
         }
 
         // Issue 38412: private items lack stability markers.
         if self.skip_stability_check_due_to_privacy(def_id) {
-            return
+            return EvalResult::Allow;
         }
 
         match stability {
-            Some(&Stability { level: attr::Unstable {ref reason, issue}, ref feature, .. }) => {
-                if self.stability().active_features.contains(feature) {
-                    return
+            Some(&Stability { level: attr::Unstable { reason, issue }, feature, .. }) => {
+                if self.stability().active_features.contains(&feature) {
+                    return EvalResult::Allow;
                 }
 
                 // When we're compiling the compiler itself we may pull in
@@ -589,19 +613,41 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
                 // the `-Z force-unstable-if-unmarked` flag present (we're
                 // compiling a compiler crate), then let this missing feature
                 // annotation slide.
-                if *feature == "rustc_private" && issue == 27812 {
+                if feature == "rustc_private" && issue == 27812 {
                     if self.sess.opts.debugging_opts.force_unstable_if_unmarked {
-                        return
+                        return EvalResult::Allow;
                     }
                 }
 
-                let msg = match *reason {
-                    Some(ref r) => format!("use of unstable library feature '{}': {}",
-                                           feature.as_str(), &r),
+                EvalResult::Deny { feature, reason, issue }
+            }
+            Some(_) => {
+                // Stable APIs are always ok to call and deprecated APIs are
+                // handled by the lint emitting logic above.
+                EvalResult::Allow
+            }
+            None => {
+                EvalResult::Unmarked
+            }
+        }
+    }
+
+    /// Checks if an item is stable or error out.
+    ///
+    /// If the item defined by `def_id` is unstable and the corresponding `#![feature]` does not
+    /// exist, emits an error.
+    ///
+    /// Additionally, this function will also check if the item is deprecated. If so, and `id` is
+    /// not `None`, a deprecated lint attached to `id` will be emitted.
+    pub fn check_stability(self, def_id: DefId, id: Option<NodeId>, span: Span) {
+        match self.eval_stability(def_id, id, span) {
+            EvalResult::Allow => {}
+            EvalResult::Deny { feature, reason, issue } => {
+                let msg = match reason {
+                    Some(r) => format!("use of unstable library feature '{}': {}", feature, r),
                     None => format!("use of unstable library feature '{}'", &feature)
                 };
 
-
                 let msp: MultiSpan = span.into();
                 let cm = &self.sess.parse_sess.codemap();
                 let span_key = msp.primary_span().and_then(|sp: Span|
@@ -624,12 +670,8 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
                                      GateIssue::Library(Some(issue)), &msg);
                 }
             }
-            Some(_) => {
-                // Stable APIs are always ok to call and deprecated APIs are
-                // handled by the lint emitting logic above.
-            }
-            None => {
-                span_bug!(span, "encountered unmarked API");
+            EvalResult::Unmarked => {
+                span_bug!(span, "encountered unmarked API: {:?}", def_id);
             }
         }
     }
@@ -655,7 +697,7 @@ impl<'a, 'tcx> Visitor<'tcx> for Checker<'a, 'tcx> {
                     None => return,
                 };
                 let def_id = DefId { krate: cnum, index: CRATE_DEF_INDEX };
-                self.tcx.check_stability(def_id, item.id, item.span);
+                self.tcx.check_stability(def_id, Some(item.id), item.span);
             }
 
             // For implementations of traits, check the stability of each item
@@ -668,8 +710,8 @@ impl<'a, 'tcx> Visitor<'tcx> for Checker<'a, 'tcx> {
                         let trait_item_def_id = self.tcx.associated_items(trait_did)
                             .find(|item| item.name == impl_item.name).map(|item| item.def_id);
                         if let Some(def_id) = trait_item_def_id {
-                            // Pass `DUMMY_NODE_ID` to skip deprecation warnings.
-                            self.tcx.check_stability(def_id, ast::DUMMY_NODE_ID, impl_item.span);
+                            // Pass `None` to skip deprecation warnings.
+                            self.tcx.check_stability(def_id, None, impl_item.span);
                         }
                     }
                 }
@@ -705,7 +747,7 @@ impl<'a, 'tcx> Visitor<'tcx> for Checker<'a, 'tcx> {
         match path.def {
             Def::Local(..) | Def::Upvar(..) |
             Def::PrimTy(..) | Def::SelfTy(..) | Def::Err => {}
-            _ => self.tcx.check_stability(path.def.def_id(), id, path.span)
+            _ => self.tcx.check_stability(path.def.def_id(), Some(id), path.span)
         }
         intravisit::walk_path(self, path)
     }