Auto merge of rust-lang#120835 - oli-obk:no_hir_coherence, r=cjgillot

bors · bors · commit aebf4511e919 · 2024-02-12T07:28:13.000Z
Avoid accessing the HIR in the happy path of `coherent_trait`

Unfortunately the hir is still used in unsafety checks, and we do not have a way to avoid that. An impl's unsafety is not part of any query other than hir.

So this PR does not affect perf, but could still be considered a cleanup
diff --git a/compiler/rustc_hir_analysis/src/coherence/builtin.rs b/compiler/rustc_hir_analysis/src/coherence/builtin.rs
@@ -25,9 +25,13 @@ use rustc_trait_selection::traits::ObligationCtxt;
 use rustc_trait_selection::traits::{self, ObligationCause};
 use std::collections::BTreeMap;
 
-pub fn check_trait(tcx: TyCtxt<'_>, trait_def_id: DefId) -> Result<(), ErrorGuaranteed> {
+pub fn check_trait(
+    tcx: TyCtxt<'_>,
+    trait_def_id: DefId,
+    impl_def_id: LocalDefId,
+) -> Result<(), ErrorGuaranteed> {
     let lang_items = tcx.lang_items();
-    let checker = Checker { tcx, trait_def_id };
+    let checker = Checker { tcx, trait_def_id, impl_def_id };
     let mut res = checker.check(lang_items.drop_trait(), visit_implementation_of_drop);
     res = res.and(checker.check(lang_items.copy_trait(), visit_implementation_of_copy));
     res = res.and(
@@ -45,20 +49,16 @@ pub fn check_trait(tcx: TyCtxt<'_>, trait_def_id: DefId) -> Result<(), ErrorGuar
 struct Checker<'tcx> {
     tcx: TyCtxt<'tcx>,
     trait_def_id: DefId,
+    impl_def_id: LocalDefId,
 }
 
 impl<'tcx> Checker<'tcx> {
-    fn check<F>(&self, trait_def_id: Option<DefId>, mut f: F) -> Result<(), ErrorGuaranteed>
-    where
-        F: FnMut(TyCtxt<'tcx>, LocalDefId) -> Result<(), ErrorGuaranteed>,
-    {
-        let mut res = Ok(());
-        if Some(self.trait_def_id) == trait_def_id {
-            for &impl_def_id in self.tcx.hir().trait_impls(self.trait_def_id) {
-                res = res.and(f(self.tcx, impl_def_id));
-            }
-        }
-        res
+    fn check(
+        &self,
+        trait_def_id: Option<DefId>,
+        f: impl FnOnce(TyCtxt<'tcx>, LocalDefId) -> Result<(), ErrorGuaranteed>,
+    ) -> Result<(), ErrorGuaranteed> {
+        if Some(self.trait_def_id) == trait_def_id { f(self.tcx, self.impl_def_id) } else { Ok(()) }
     }
 }
 
@@ -92,10 +92,10 @@ fn visit_implementation_of_copy(
 
     debug!("visit_implementation_of_copy: self_type={:?} (free)", self_type);
 
-    let span = match tcx.hir().expect_item(impl_did).expect_impl() {
-        hir::Impl { polarity: hir::ImplPolarity::Negative(_), .. } => return Ok(()),
-        hir::Impl { self_ty, .. } => self_ty.span,
-    };
+    if let ty::ImplPolarity::Negative = tcx.impl_polarity(impl_did) {
+        return Ok(());
+    }
+    let span = tcx.hir().expect_item(impl_did).expect_impl().self_ty.span;
 
     let cause = traits::ObligationCause::misc(span, impl_did);
     match type_allowed_to_implement_copy(tcx, param_env, self_type, cause) {
@@ -121,10 +121,10 @@ fn visit_implementation_of_const_param_ty(
 
     let param_env = tcx.param_env(impl_did);
 
-    let span = match tcx.hir().expect_item(impl_did).expect_impl() {
-        hir::Impl { polarity: hir::ImplPolarity::Negative(_), .. } => return Ok(()),
-        impl_ => impl_.self_ty.span,
-    };
+    if let ty::ImplPolarity::Negative = tcx.impl_polarity(impl_did) {
+        return Ok(());
+    }
+    let span = tcx.hir().expect_item(impl_did).expect_impl().self_ty.span;
 
     let cause = traits::ObligationCause::misc(span, impl_did);
     match type_allowed_to_implement_const_param_ty(tcx, param_env, self_type, cause) {
diff --git a/compiler/rustc_hir_analysis/src/coherence/mod.rs b/compiler/rustc_hir_analysis/src/coherence/mod.rs
@@ -137,11 +137,12 @@ fn coherent_trait(tcx: TyCtxt<'_>, def_id: DefId) -> Result<(), ErrorGuaranteed>
         res = res.and(check_impl(tcx, impl_def_id, trait_ref));
         res = res.and(check_object_overlap(tcx, impl_def_id, trait_ref));
 
-        res = res.and(unsafety::check_item(tcx, impl_def_id));
+        res = res.and(unsafety::check_item(tcx, impl_def_id, trait_ref));
         res = res.and(tcx.ensure().orphan_check_impl(impl_def_id));
+        res = res.and(builtin::check_trait(tcx, def_id, impl_def_id));
     }
 
-    res.and(builtin::check_trait(tcx, def_id))
+    res
 }
 
 /// Checks whether an impl overlaps with the automatic `impl Trait for dyn Trait`.
diff --git a/compiler/rustc_hir_analysis/src/coherence/unsafety.rs b/compiler/rustc_hir_analysis/src/coherence/unsafety.rs
@@ -4,94 +4,91 @@
 use rustc_errors::{codes::*, struct_span_code_err};
 use rustc_hir as hir;
 use rustc_hir::Unsafety;
-use rustc_middle::ty::TyCtxt;
+use rustc_middle::ty::{TraitRef, TyCtxt};
 use rustc_span::def_id::LocalDefId;
 use rustc_span::ErrorGuaranteed;
 
-pub(super) fn check_item(tcx: TyCtxt<'_>, def_id: LocalDefId) -> Result<(), ErrorGuaranteed> {
+pub(super) fn check_item(
+    tcx: TyCtxt<'_>,
+    def_id: LocalDefId,
+    trait_ref: TraitRef<'_>,
+) -> Result<(), ErrorGuaranteed> {
     let item = tcx.hir().expect_item(def_id);
     let impl_ = item.expect_impl();
+    let trait_def = tcx.trait_def(trait_ref.def_id);
+    let unsafe_attr = impl_.generics.params.iter().find(|p| p.pure_wrt_drop).map(|_| "may_dangle");
+    match (trait_def.unsafety, unsafe_attr, impl_.unsafety, impl_.polarity) {
+        (Unsafety::Normal, None, Unsafety::Unsafe, hir::ImplPolarity::Positive) => {
+            return Err(struct_span_code_err!(
+                tcx.dcx(),
+                tcx.def_span(def_id),
+                E0199,
+                "implementing the trait `{}` is not unsafe",
+                trait_ref.print_trait_sugared()
+            )
+            .with_span_suggestion_verbose(
+                item.span.with_hi(item.span.lo() + rustc_span::BytePos(7)),
+                "remove `unsafe` from this trait implementation",
+                "",
+                rustc_errors::Applicability::MachineApplicable,
+            )
+            .emit());
+        }
 
-    if let Some(trait_ref) = tcx.impl_trait_ref(item.owner_id) {
-        let trait_ref = trait_ref.instantiate_identity();
-        let trait_def = tcx.trait_def(trait_ref.def_id);
-        let unsafe_attr =
-            impl_.generics.params.iter().find(|p| p.pure_wrt_drop).map(|_| "may_dangle");
-        match (trait_def.unsafety, unsafe_attr, impl_.unsafety, impl_.polarity) {
-            (Unsafety::Normal, None, Unsafety::Unsafe, hir::ImplPolarity::Positive) => {
-                return Err(struct_span_code_err!(
-                    tcx.dcx(),
-                    tcx.def_span(def_id),
-                    E0199,
-                    "implementing the trait `{}` is not unsafe",
-                    trait_ref.print_trait_sugared()
-                )
-                .with_span_suggestion_verbose(
-                    item.span.with_hi(item.span.lo() + rustc_span::BytePos(7)),
-                    "remove `unsafe` from this trait implementation",
-                    "",
-                    rustc_errors::Applicability::MachineApplicable,
-                )
-                .emit());
-            }
-
-            (Unsafety::Unsafe, _, Unsafety::Normal, hir::ImplPolarity::Positive) => {
-                return Err(struct_span_code_err!(
-                    tcx.dcx(),
-                    tcx.def_span(def_id),
-                    E0200,
-                    "the trait `{}` requires an `unsafe impl` declaration",
-                    trait_ref.print_trait_sugared()
-                )
-                .with_note(format!(
-                    "the trait `{}` enforces invariants that the compiler can't check. \
+        (Unsafety::Unsafe, _, Unsafety::Normal, hir::ImplPolarity::Positive) => {
+            return Err(struct_span_code_err!(
+                tcx.dcx(),
+                tcx.def_span(def_id),
+                E0200,
+                "the trait `{}` requires an `unsafe impl` declaration",
+                trait_ref.print_trait_sugared()
+            )
+            .with_note(format!(
+                "the trait `{}` enforces invariants that the compiler can't check. \
                     Review the trait documentation and make sure this implementation \
                     upholds those invariants before adding the `unsafe` keyword",
-                    trait_ref.print_trait_sugared()
-                ))
-                .with_span_suggestion_verbose(
-                    item.span.shrink_to_lo(),
-                    "add `unsafe` to this trait implementation",
-                    "unsafe ",
-                    rustc_errors::Applicability::MaybeIncorrect,
-                )
-                .emit());
-            }
+                trait_ref.print_trait_sugared()
+            ))
+            .with_span_suggestion_verbose(
+                item.span.shrink_to_lo(),
+                "add `unsafe` to this trait implementation",
+                "unsafe ",
+                rustc_errors::Applicability::MaybeIncorrect,
+            )
+            .emit());
+        }
 
-            (Unsafety::Normal, Some(attr_name), Unsafety::Normal, hir::ImplPolarity::Positive) => {
-                return Err(struct_span_code_err!(
-                    tcx.dcx(),
-                    tcx.def_span(def_id),
-                    E0569,
-                    "requires an `unsafe impl` declaration due to `#[{}]` attribute",
-                    attr_name
-                )
-                .with_note(format!(
-                    "the trait `{}` enforces invariants that the compiler can't check. \
+        (Unsafety::Normal, Some(attr_name), Unsafety::Normal, hir::ImplPolarity::Positive) => {
+            return Err(struct_span_code_err!(
+                tcx.dcx(),
+                tcx.def_span(def_id),
+                E0569,
+                "requires an `unsafe impl` declaration due to `#[{}]` attribute",
+                attr_name
+            )
+            .with_note(format!(
+                "the trait `{}` enforces invariants that the compiler can't check. \
                     Review the trait documentation and make sure this implementation \
                     upholds those invariants before adding the `unsafe` keyword",
-                    trait_ref.print_trait_sugared()
-                ))
-                .with_span_suggestion_verbose(
-                    item.span.shrink_to_lo(),
-                    "add `unsafe` to this trait implementation",
-                    "unsafe ",
-                    rustc_errors::Applicability::MaybeIncorrect,
-                )
-                .emit());
-            }
+                trait_ref.print_trait_sugared()
+            ))
+            .with_span_suggestion_verbose(
+                item.span.shrink_to_lo(),
+                "add `unsafe` to this trait implementation",
+                "unsafe ",
+                rustc_errors::Applicability::MaybeIncorrect,
+            )
+            .emit());
+        }
 
-            (_, _, Unsafety::Unsafe, hir::ImplPolarity::Negative(_)) => {
-                // Reported in AST validation
-                tcx.dcx().span_delayed_bug(item.span, "unsafe negative impl");
-            }
-            (_, _, Unsafety::Normal, hir::ImplPolarity::Negative(_))
-            | (Unsafety::Unsafe, _, Unsafety::Unsafe, hir::ImplPolarity::Positive)
-            | (Unsafety::Normal, Some(_), Unsafety::Unsafe, hir::ImplPolarity::Positive)
-            | (Unsafety::Normal, None, Unsafety::Normal, _) => {
-                // OK
-            }
+        (_, _, Unsafety::Unsafe, hir::ImplPolarity::Negative(_)) => {
+            // Reported in AST validation
+            tcx.dcx().span_delayed_bug(item.span, "unsafe negative impl");
+            Ok(())
         }
+        (_, _, Unsafety::Normal, hir::ImplPolarity::Negative(_))
+        | (Unsafety::Unsafe, _, Unsafety::Unsafe, hir::ImplPolarity::Positive)
+        | (Unsafety::Normal, Some(_), Unsafety::Unsafe, hir::ImplPolarity::Positive)
+        | (Unsafety::Normal, None, Unsafety::Normal, _) => Ok(()),
     }
-    Ok(())
 }
diff --git a/tests/ui/coherence/coherence-impls-copy.stderr b/tests/ui/coherence/coherence-impls-copy.stderr
@@ -30,6 +30,12 @@ LL | impl Copy for &'static [NotSync] {}
    |
    = note: define and implement a trait or new type instead
 
+error[E0206]: the trait `Copy` cannot be implemented for this type
+  --> $DIR/coherence-impls-copy.rs:21:15
+   |
+LL | impl Copy for &'static mut MyType {}
+   |               ^^^^^^^^^^^^^^^^^^^ type is not a structure or enumeration
+
 error[E0117]: only traits defined in the current crate can be implemented for arbitrary types
   --> $DIR/coherence-impls-copy.rs:25:1
    |
@@ -41,6 +47,12 @@ LL | impl Copy for (MyType, MyType) {}
    |
    = note: define and implement a trait or new type instead
 
+error[E0206]: the trait `Copy` cannot be implemented for this type
+  --> $DIR/coherence-impls-copy.rs:25:15
+   |
+LL | impl Copy for (MyType, MyType) {}
+   |               ^^^^^^^^^^^^^^^^ type is not a structure or enumeration
+
 error[E0117]: only traits defined in the current crate can be implemented for arbitrary types
   --> $DIR/coherence-impls-copy.rs:30:1
    |
@@ -52,18 +64,6 @@ LL | impl Copy for [MyType] {}
    |
    = note: define and implement a trait or new type instead
 
-error[E0206]: the trait `Copy` cannot be implemented for this type
-  --> $DIR/coherence-impls-copy.rs:21:15
-   |
-LL | impl Copy for &'static mut MyType {}
-   |               ^^^^^^^^^^^^^^^^^^^ type is not a structure or enumeration
-
-error[E0206]: the trait `Copy` cannot be implemented for this type
-  --> $DIR/coherence-impls-copy.rs:25:15
-   |
-LL | impl Copy for (MyType, MyType) {}
-   |               ^^^^^^^^^^^^^^^^ type is not a structure or enumeration
-
 error[E0206]: the trait `Copy` cannot be implemented for this type
   --> $DIR/coherence-impls-copy.rs:30:15
    |

Original file line number	Diff line number	Diff line change
`@@ -137,11 +137,12 @@ fn coherent_trait(tcx: TyCtxt<'_>, def_id: DefId) -> Result<(), ErrorGuaranteed>`
`137`	`137`	`res = res.and(check_impl(tcx, impl_def_id, trait_ref));`
`138`	`138`	`res = res.and(check_object_overlap(tcx, impl_def_id, trait_ref));`
`139`	`139`
`140`		`- res = res.and(unsafety::check_item(tcx, impl_def_id));`
	`140`	`+ res = res.and(unsafety::check_item(tcx, impl_def_id, trait_ref));`
`141`	`141`	`res = res.and(tcx.ensure().orphan_check_impl(impl_def_id));`
	`142`	`+ res = res.and(builtin::check_trait(tcx, def_id, impl_def_id));`
`142`	`143`	`}`
`143`	`144`
`144`		`- res.and(builtin::check_trait(tcx, def_id))`
	`145`	`+ res`
`145`	`146`	`}`
`146`	`147`
`147`	`148`	/// Checks whether an impl overlaps with the automatic `impl Trait for dyn Trait`.