Implement shadowing lint

Author: compiler-errors

compiler/rustc_hir_typeck/messages.ftl

@@ -180,6 +180,14 @@ hir_typeck_suggest_boxing_when_appropriate = store this in the heap by calling `
 
 hir_typeck_suggest_ptr_null_mut = consider using `core::ptr::null_mut` instead
 
+hir_typeck_supertrait_method_multiple_shadowee = methods from several supertraits are shadowed: {$traits}
+
+hir_typeck_supertrait_method_shadowee = method from `{$supertrait}` is shadowed by a subtrait method
+
+hir_typeck_supertrait_method_shadower = method from `{$subtrait}` shadows a supertrait method
+
+hir_typeck_supertrait_method_shadowing = trait method `{$method}` from `{$subtrait}` shadows identically named method from supertrait
+
 hir_typeck_trivial_cast = trivial {$numeric ->
     [true] numeric cast
     *[false] cast

compiler/rustc_hir_typeck/src/errors.rs

@@ -724,3 +724,38 @@ pub(crate) struct PassToVariadicFunction<'a, 'tcx> {
     #[note(hir_typeck_teach_help)]
     pub(crate) teach: bool,
 }
+
+#[derive(LintDiagnostic)]
+#[diag(hir_typeck_supertrait_method_shadowing)]
+pub(crate) struct SupertraitMethodShadowing {
+    pub method: Symbol,
+    pub subtrait: Symbol,
+    #[subdiagnostic]
+    pub shadower: SupertraitMethodShadower,
+    #[subdiagnostic]
+    pub shadowee: SupertraitMethodShadowee,
+}
+
+#[derive(Subdiagnostic)]
+#[note(hir_typeck_supertrait_method_shadower)]
+pub(crate) struct SupertraitMethodShadower {
+    pub subtrait: Symbol,
+    #[primary_span]
+    pub span: Span,
+}
+
+#[derive(Subdiagnostic)]
+pub(crate) enum SupertraitMethodShadowee {
+    #[note(hir_typeck_supertrait_method_shadowee)]
+    Labeled {
+        #[primary_span]
+        span: Span,
+        supertrait: Symbol,
+    },
+    #[note(hir_typeck_supertrait_method_multiple_shadowee)]
+    Several {
+        #[primary_span]
+        spans: MultiSpan,
+        traits: DiagSymbolList,
+    },
+}

compiler/rustc_hir_typeck/src/method/confirm.rs

@@ -10,6 +10,7 @@ use rustc_hir_analysis::hir_ty_lowering::{
     GenericArgsLowerer, HirTyLowerer, IsMethodCall, RegionInferReason,
 };
 use rustc_infer::infer::{self, DefineOpaqueTypes, InferOk};
+use rustc_lint::builtin::SUPERTRAIT_ITEM_SHADOWING;
 use rustc_middle::traits::{ObligationCauseCode, UnifyReceiverContext};
 use rustc_middle::ty::adjustment::{
     Adjust, Adjustment, AllowTwoPhase, AutoBorrow, AutoBorrowMutability, PointerCoercion,
@@ -25,6 +26,9 @@ use rustc_trait_selection::traits;
 use tracing::debug;
 
 use super::{MethodCallee, probe};
+use crate::errors::{
+    SupertraitMethodShadowee, SupertraitMethodShadower, SupertraitMethodShadowing,
+};
 use crate::{FnCtxt, callee};
 
 struct ConfirmContext<'a, 'tcx> {
@@ -144,6 +148,8 @@ impl<'a, 'tcx> ConfirmContext<'a, 'tcx> {
         // Make sure nobody calls `drop()` explicitly.
         self.enforce_illegal_method_limitations(pick);
 
+        self.enforce_shadowed_supertrait_methods(pick, segment);
+
         // Add any trait/regions obligations specified on the method's type parameters.
         // We won't add these if we encountered an illegal sized bound, so that we can use
         // a custom error in that case.
@@ -663,6 +669,45 @@ impl<'a, 'tcx> ConfirmContext<'a, 'tcx> {
         }
     }
 
+    fn enforce_shadowed_supertrait_methods(
+        &self,
+        pick: &probe::Pick<'_>,
+        segment: &hir::PathSegment<'tcx>,
+    ) {
+        if pick.shadowed_candidates.is_empty() {
+            return;
+        }
+
+        let shadower_span = self.tcx.def_span(pick.item.def_id);
+        let subtrait = self.tcx.item_name(pick.item.trait_container(self.tcx).unwrap());
+        let shadower = SupertraitMethodShadower { span: shadower_span, subtrait };
+
+        let shadowee = if let [shadowee] = &pick.shadowed_candidates[..] {
+            let shadowee_span = self.tcx.def_span(shadowee.def_id);
+            let supertrait = self.tcx.item_name(shadowee.trait_container(self.tcx).unwrap());
+            SupertraitMethodShadowee::Labeled { span: shadowee_span, supertrait }
+        } else {
+            let (traits, spans): (Vec<_>, Vec<_>) = pick
+                .shadowed_candidates
+                .iter()
+                .map(|item| {
+                    (
+                        self.tcx.item_name(item.trait_container(self.tcx).unwrap()),
+                        self.tcx.def_span(item.def_id),
+                    )
+                })
+                .unzip();
+            SupertraitMethodShadowee::Several { traits: traits.into(), spans: spans.into() }
+        };
+
+        self.tcx.emit_node_span_lint(
+            SUPERTRAIT_ITEM_SHADOWING,
+            segment.hir_id,
+            segment.ident.span,
+            SupertraitMethodShadowing { shadower, shadowee, method: segment.ident.name, subtrait },
+        );
+    }
+
     fn upcast(
         &mut self,
         source_trait_ref: ty::PolyTraitRef<'tcx>,

compiler/rustc_hir_typeck/src/method/probe.rs

@@ -183,6 +183,9 @@ pub(crate) struct Pick<'tcx> {
 
     /// Unstable candidates alongside the stable ones.
     unstable_candidates: Vec<(Candidate<'tcx>, Symbol)>,
+
+    /// Candidates that were shadowed by supertraits.
+    pub shadowed_candidates: Vec<ty::AssocItem>,
 }
 
 #[derive(Clone, Debug, PartialEq, Eq)]
@@ -1319,18 +1322,10 @@ impl<'a, 'tcx> ProbeContext<'a, 'tcx> {
         debug!("applicable_candidates: {:?}", applicable_candidates);
 
         if applicable_candidates.len() > 1 {
-            if self.tcx.features().supertrait_item_shadowing {
-                if let Some(pick) =
-                    self.collapse_candidates_to_subtrait_pick(self_ty, &applicable_candidates)
-                {
-                    return Some(Ok(pick));
-                }
-            } else {
-                if let Some(pick) =
-                    self.collapse_candidates_to_trait_pick(self_ty, &applicable_candidates)
-                {
-                    return Some(Ok(pick));
-                }
+            if let Some(pick) =
+                self.collapse_candidates_to_trait_pick(self_ty, &applicable_candidates)
+            {
+                return Some(Ok(pick));
             }
         }
 
@@ -1347,6 +1342,17 @@ impl<'a, 'tcx> ProbeContext<'a, 'tcx> {
         }
 
         if applicable_candidates.len() > 1 {
+            // We collapse to a subtrait pick *after* filtering unstable candidates
+            // to make sure we don't prefer a unstable subtrait method over a stable
+            // supertrait method.
+            if self.tcx.features().supertrait_item_shadowing {
+                if let Some(pick) =
+                    self.collapse_candidates_to_subtrait_pick(self_ty, &applicable_candidates)
+                {
+                    return Some(Ok(pick));
+                }
+            }
+
             let sources = candidates.iter().map(|p| self.candidate_source(p, self_ty)).collect();
             return Some(Err(MethodError::Ambiguity(sources)));
         }
@@ -1385,6 +1391,7 @@ impl<'tcx> Pick<'tcx> {
             autoref_or_ptr_adjustment: _,
             self_ty,
             unstable_candidates: _,
+            shadowed_candidates: _,
         } = *self;
         self_ty != other.self_ty || def_id != other.item.def_id
     }
@@ -1761,6 +1768,7 @@ impl<'a, 'tcx> ProbeContext<'a, 'tcx> {
             autoref_or_ptr_adjustment: None,
             self_ty,
             unstable_candidates: vec![],
+            shadowed_candidates: vec![],
         })
     }
 
@@ -1806,6 +1814,11 @@ impl<'a, 'tcx> ProbeContext<'a, 'tcx> {
             autoref_or_ptr_adjustment: None,
             self_ty,
             unstable_candidates: vec![],
+            shadowed_candidates: probes
+                .iter()
+                .map(|(c, _)| c.item)
+                .filter(|item| item.def_id != child_pick.item.def_id)
+                .collect(),
         })
     }
 
@@ -2099,6 +2112,7 @@ impl<'tcx> Candidate<'tcx> {
             autoref_or_ptr_adjustment: None,
             self_ty,
             unstable_candidates,
+            shadowed_candidates: vec![],
         }
     }
 }

compiler/rustc_lint_defs/src/builtin.rs

@@ -100,6 +100,7 @@ declare_lint_pass! {
         SINGLE_USE_LIFETIMES,
         SOFT_UNSTABLE,
         STABLE_FEATURES,
+        SUPERTRAIT_ITEM_SHADOWING,
         TEST_UNSTABLE_LINT,
         TEXT_DIRECTION_CODEPOINT_IN_COMMENT,
         TRIVIAL_CASTS,
@@ -4964,6 +4965,43 @@ declare_lint! {
     };
 }
 
+declare_lint! {
+    /// The `supertrait_item_shadowing` lint detects when.
+    ///
+    /// ### Example
+    ///
+    /// ```rust
+    /// #![feature(supertrait_item_shadowing)]
+    ///
+    /// trait Upstream {
+    ///     fn hello(&self) {}
+    /// }
+    /// impl<T> Upstream for T {}
+    ///
+    /// trait Downstream: Upstream {
+    ///     fn hello(&self) {}
+    /// }
+    /// impl<T> Downstream for T {}
+    ///
+    /// struct MyType;
+    /// MyType.hello();
+    /// ```
+    ///
+    /// {{produces}}
+    ///
+    /// ### Explanation
+    ///
+    /// RFC 3624 specified a heuristic in which a supertrait method would be
+    /// shadowed by a subtrait method when ambiguity occurs during method
+    /// selection. In order to mitigate side-effects of this happening
+    /// silently, it was also decided that this would, since the code should
+    /// eventually be fixed to no longer trigger this ambiguity.
+    pub SUPERTRAIT_ITEM_SHADOWING,
+    Warn,
+    "detects when a supertrait method is shadowed by a subtrait method",
+    @feature_gate = supertrait_item_shadowing;
+}
+
 declare_lint! {
     /// The `ptr_to_integer_transmute_in_consts` lint detects pointer to integer
     /// transmute in const functions and associated constants.

tests/ui/methods/supertrait-shadowing/common-ancestor.rs

@@ -0,0 +1,24 @@
+//@ run-pass
+//@ check-run-results
+
+#![feature(supertrait_item_shadowing)]
+#![allow(dead_code)]
+
+trait A {
+    fn hello(&self) {
+        println!("A");
+    }
+}
+impl<T> A for T {}
+
+trait B: A {
+    fn hello(&self) {
+        println!("B");
+    }
+}
+impl<T> B for T {}
+
+fn main() {
+    ().hello();
+    //~^ WARN trait method `hello` from `B` shadows identically named method from supertrait
+}

tests/ui/methods/supertrait-shadowing/common-ancestor.run.stdout

@@ -0,0 +1 @@
+B

tests/ui/methods/supertrait-shadowing/common-ancestor.stderr

@@ -0,0 +1,20 @@
+warning: trait method `hello` from `B` shadows identically named method from supertrait
+  --> $DIR/common-ancestor.rs:22:8
+   |
+LL |     ().hello();
+   |        ^^^^^
+   |
+note: method from `B` shadows a supertrait method
+  --> $DIR/common-ancestor.rs:15:5
+   |
+LL |     fn hello(&self) {
+   |     ^^^^^^^^^^^^^^^
+note: method from `A` is shadowed by a subtrait method
+  --> $DIR/common-ancestor.rs:8:5
+   |
+LL |     fn hello(&self) {
+   |     ^^^^^^^^^^^^^^^
+   = note: `#[warn(supertrait_item_shadowing)]` on by default
+
+warning: 1 warning emitted
+

tests/ui/methods/supertrait-shadowing/no-common-ancestor.rs

@@ -0,0 +1,20 @@
+#![feature(supertrait_item_shadowing)]
+
+trait A {
+    fn hello(&self) {
+        println!("A");
+    }
+}
+impl<T> A for T {}
+
+trait B {
+    fn hello(&self) {
+        println!("B");
+    }
+}
+impl<T> B for T {}
+
+fn main() {
+    ().hello();
+    //~^ ERROR multiple applicable items in scope
+}

tests/ui/methods/supertrait-shadowing/no-common-ancestor.stderr

@@ -0,0 +1,28 @@
+error[E0034]: multiple applicable items in scope
+  --> $DIR/no-common-ancestor.rs:18:8
+   |
+LL |     ().hello();
+   |        ^^^^^ multiple `hello` found
+   |
+note: candidate #1 is defined in an impl of the trait `A` for the type `T`
+  --> $DIR/no-common-ancestor.rs:4:5
+   |
+LL |     fn hello(&self) {
+   |     ^^^^^^^^^^^^^^^
+note: candidate #2 is defined in an impl of the trait `B` for the type `T`
+  --> $DIR/no-common-ancestor.rs:11:5
+   |
+LL |     fn hello(&self) {
+   |     ^^^^^^^^^^^^^^^
+help: disambiguate the method for candidate #1
+   |
+LL |     A::hello(&());
+   |     ~~~~~~~~~~~~~
+help: disambiguate the method for candidate #2
+   |
+LL |     B::hello(&());
+   |     ~~~~~~~~~~~~~
+
+error: aborting due to 1 previous error
+
+For more information about this error, try `rustc --explain E0034`.