Implement shadowing lint

Author: compiler-errors

compiler/rustc_hir_typeck/messages.ftl

@@ -192,6 +192,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_item_multiple_shadowee = items from several supertraits are shadowed: {$traits}
+
+hir_typeck_supertrait_item_shadowee = item from `{$supertrait}` is shadowed by a subtrait item
+
+hir_typeck_supertrait_item_shadower = item from `{$subtrait}` shadows a supertrait item
+
+hir_typeck_supertrait_item_shadowing = trait item `{$item}` from `{$subtrait}` shadows identically named item from supertrait
+
 hir_typeck_trivial_cast = trivial {$numeric ->
     [true] numeric cast
     *[false] cast

compiler/rustc_hir_typeck/src/errors.rs

@@ -868,3 +868,38 @@ pub(crate) struct ReplaceCommaWithSemicolon {
     pub comma_span: Span,
     pub descr: &'static str,
 }
+
+#[derive(LintDiagnostic)]
+#[diag(hir_typeck_supertrait_item_shadowing)]
+pub(crate) struct SupertraitItemShadowing {
+    pub item: Symbol,
+    pub subtrait: Symbol,
+    #[subdiagnostic]
+    pub shadower: SupertraitItemShadower,
+    #[subdiagnostic]
+    pub shadowee: SupertraitItemShadowee,
+}
+
+#[derive(Subdiagnostic)]
+#[note(hir_typeck_supertrait_item_shadower)]
+pub(crate) struct SupertraitItemShadower {
+    pub subtrait: Symbol,
+    #[primary_span]
+    pub span: Span,
+}
+
+#[derive(Subdiagnostic)]
+pub(crate) enum SupertraitItemShadowee {
+    #[note(hir_typeck_supertrait_item_shadowee)]
+    Labeled {
+        #[primary_span]
+        span: Span,
+        supertrait: Symbol,
+    },
+    #[note(hir_typeck_supertrait_item_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::{
     FeedConstTy, GenericArgsLowerer, HirTyLowerer, IsMethodCall, RegionInferReason,
 };
 use rustc_infer::infer::{self, DefineOpaqueTypes, InferOk};
+use rustc_lint::builtin::SUPERTRAIT_ITEM_SHADOWING_USAGE;
 use rustc_middle::traits::{ObligationCauseCode, UnifyReceiverContext};
 use rustc_middle::ty::adjustment::{
     Adjust, Adjustment, AllowTwoPhase, AutoBorrow, AutoBorrowMutability, PointerCoercion,
@@ -24,6 +25,7 @@ use rustc_trait_selection::traits;
 use tracing::debug;
 
 use super::{MethodCallee, probe};
+use crate::errors::{SupertraitItemShadowee, SupertraitItemShadower, SupertraitItemShadowing};
 use crate::{FnCtxt, callee};
 
 struct ConfirmContext<'a, 'tcx> {
@@ -143,6 +145,8 @@ impl<'a, 'tcx> ConfirmContext<'a, 'tcx> {
         // Make sure nobody calls `drop()` explicitly.
         self.enforce_illegal_method_limitations(pick);
 
+        self.enforce_shadowed_supertrait_items(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.
@@ -672,6 +676,45 @@ impl<'a, 'tcx> ConfirmContext<'a, 'tcx> {
         }
     }
 
+    fn enforce_shadowed_supertrait_items(
+        &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 = SupertraitItemShadower { 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());
+            SupertraitItemShadowee::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();
+            SupertraitItemShadowee::Several { traits: traits.into(), spans: spans.into() }
+        };
+
+        self.tcx.emit_node_span_lint(
+            SUPERTRAIT_ITEM_SHADOWING_USAGE,
+            segment.hir_id,
+            segment.ident.span,
+            SupertraitItemShadowing { shadower, shadowee, item: segment.ident.name, subtrait },
+        );
+    }
+
     fn upcast(
         &mut self,
         source_trait_ref: ty::PolyTraitRef<'tcx>,

compiler/rustc_hir_typeck/src/method/probe.rs

@@ -226,6 +226,9 @@ pub(crate) struct Pick<'tcx> {
     /// to identify this method. Used only for deshadowing errors.
     /// Only applies for inherent impls.
     pub receiver_steps: Option<usize>,
+
+    /// Candidates that were shadowed by supertraits.
+    pub shadowed_candidates: Vec<ty::AssocItem>,
 }
 
 #[derive(Clone, Debug, PartialEq, Eq)]
@@ -1616,18 +1619,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));
             }
         }
 
@@ -1644,6 +1639,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)));
         }
@@ -1682,6 +1688,7 @@ impl<'tcx> Pick<'tcx> {
             self_ty,
             unstable_candidates: _,
             receiver_steps: _,
+            shadowed_candidates: _,
         } = *self;
         self_ty != other.self_ty || def_id != other.item.def_id
     }
@@ -2091,6 +2098,7 @@ impl<'a, 'tcx> ProbeContext<'a, 'tcx> {
             self_ty,
             unstable_candidates: vec![],
             receiver_steps: None,
+            shadowed_candidates: vec![],
         })
     }
 
@@ -2136,6 +2144,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(),
             receiver_steps: None,
         })
     }
@@ -2434,6 +2447,7 @@ impl<'tcx> Candidate<'tcx> {
                 InherentImplCandidate { receiver_steps, .. } => Some(receiver_steps),
                 _ => None,
             },
+            shadowed_candidates: vec![],
         }
     }
 }

compiler/rustc_lint_defs/src/builtin.rs

@@ -101,6 +101,7 @@ declare_lint_pass! {
         SINGLE_USE_LIFETIMES,
         SOFT_UNSTABLE,
         STABLE_FEATURES,
+        SUPERTRAIT_ITEM_SHADOWING_USAGE,
         TAIL_EXPR_DROP_ORDER,
         TEST_UNSTABLE_LINT,
         TEXT_DIRECTION_CODEPOINT_IN_COMMENT,
@@ -4916,6 +4917,48 @@ declare_lint! {
     };
 }
 
+declare_lint! {
+    /// The `supertrait_item_shadowing_usage` lint detects when the
+    /// usage of an item that is provided by both a subtrait and supertrait
+    /// is shadowed, preferring the subtrait.
+    ///
+    /// ### Example
+    ///
+    /// ```rust
+    /// #![feature(supertrait_item_shadowing)]
+    /// #![deny(supertrait_item_shadowing_usage)]
+    ///
+    /// 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 item would be
+    /// shadowed by a subtrait item when ambiguity occurs during item
+    /// selection. In order to mitigate side-effects of this happening
+    /// silently, this lint detects these cases when users want to deny them
+    /// or fix the call sites.
+    pub SUPERTRAIT_ITEM_SHADOWING_USAGE,
+    // FIXME(supertrait_item_shadowing): It is not decided if this should
+    // warn by default at the call site.
+    Allow,
+    "detects when a supertrait item is shadowed by a subtrait item",
+    @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,25 @@
+//@ run-pass
+//@ check-run-results
+
+#![feature(supertrait_item_shadowing)]
+#![warn(supertrait_item_shadowing_usage)]
+#![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 item `hello` from `B` shadows identically named item 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,24 @@
+warning: trait item `hello` from `B` shadows identically named item from supertrait
+  --> $DIR/common-ancestor.rs:23:8
+   |
+LL |     ().hello();
+   |        ^^^^^
+   |
+note: item from `B` shadows a supertrait item
+  --> $DIR/common-ancestor.rs:16:5
+   |
+LL |     fn hello(&self) {
+   |     ^^^^^^^^^^^^^^^
+note: item from `A` is shadowed by a subtrait item
+  --> $DIR/common-ancestor.rs:9:5
+   |
+LL |     fn hello(&self) {
+   |     ^^^^^^^^^^^^^^^
+note: the lint level is defined here
+  --> $DIR/common-ancestor.rs:5:9
+   |
+LL | #![warn(supertrait_item_shadowing_usage)]
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+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,30 @@
+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 -     ().hello();
+LL +     A::hello(&());
+   |
+help: disambiguate the method for candidate #2
+   |
+LL -     ().hello();
+LL +     B::hello(&());
+   |
+
+error: aborting due to 1 previous error
+
+For more information about this error, try `rustc --explain E0034`.