From 4661c83530237349621793346ca5d4d8654d0cd1 Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Fri, 9 Feb 2024 09:38:22 +0000 Subject: [PATCH 1/5] Avoid accessing the HIR in the happy path of `coherent_trait` --- .../src/coherence/builtin.rs | 29 ++++++++++--------- .../rustc_hir_analysis/src/coherence/mod.rs | 3 +- .../ui/coherence/coherence-impls-copy.stderr | 24 +++++++-------- 3 files changed, 30 insertions(+), 26 deletions(-) diff --git a/compiler/rustc_hir_analysis/src/coherence/builtin.rs b/compiler/rustc_hir_analysis/src/coherence/builtin.rs index 5a3878445937b..59177f14f99e1 100644 --- 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,6 +49,7 @@ 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> { @@ -54,9 +59,7 @@ impl<'tcx> Checker<'tcx> { { 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 = res.and(f(self.tcx, self.impl_def_id)); } res } @@ -92,10 +95,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 +124,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 index 8cf1f2c9407f6..a71ea00a0448e 100644 --- a/compiler/rustc_hir_analysis/src/coherence/mod.rs +++ b/compiler/rustc_hir_analysis/src/coherence/mod.rs @@ -133,9 +133,10 @@ fn coherent_trait(tcx: TyCtxt<'_>, def_id: DefId) -> Result<(), ErrorGuaranteed> res = res.and(unsafety::check_item(tcx, impl_def_id)); 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/tests/ui/coherence/coherence-impls-copy.stderr b/tests/ui/coherence/coherence-impls-copy.stderr index 21dbc606321ff..2d2c5064043a9 100644 --- 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 | From 23277a502ef69b82d8d541e5dc5fb129135aef9e Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Fri, 9 Feb 2024 20:23:18 +0000 Subject: [PATCH 2/5] Simplify conditional erroring --- compiler/rustc_hir_analysis/src/coherence/builtin.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/compiler/rustc_hir_analysis/src/coherence/builtin.rs b/compiler/rustc_hir_analysis/src/coherence/builtin.rs index 59177f14f99e1..30f6bd56375dc 100644 --- a/compiler/rustc_hir_analysis/src/coherence/builtin.rs +++ b/compiler/rustc_hir_analysis/src/coherence/builtin.rs @@ -57,11 +57,7 @@ impl<'tcx> Checker<'tcx> { where F: FnMut(TyCtxt<'tcx>, LocalDefId) -> Result<(), ErrorGuaranteed>, { - let mut res = Ok(()); - if Some(self.trait_def_id) == trait_def_id { - res = res.and(f(self.tcx, self.impl_def_id)); - } - res + if Some(self.trait_def_id) == trait_def_id { f(self.tcx, self.impl_def_id) } else { Ok(()) } } } From 7f871ab9aa0d7cfe850ebe41ea5122fa9d1f218e Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Fri, 9 Feb 2024 20:23:40 +0000 Subject: [PATCH 3/5] No need for FnMut when FnOnce works now --- compiler/rustc_hir_analysis/src/coherence/builtin.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_hir_analysis/src/coherence/builtin.rs b/compiler/rustc_hir_analysis/src/coherence/builtin.rs index 30f6bd56375dc..fe1a9f2eebd66 100644 --- a/compiler/rustc_hir_analysis/src/coherence/builtin.rs +++ b/compiler/rustc_hir_analysis/src/coherence/builtin.rs @@ -53,9 +53,9 @@ struct Checker<'tcx> { } impl<'tcx> Checker<'tcx> { - fn check(&self, trait_def_id: Option, mut f: F) -> Result<(), ErrorGuaranteed> + fn check(&self, trait_def_id: Option, f: F) -> Result<(), ErrorGuaranteed> where - F: FnMut(TyCtxt<'tcx>, LocalDefId) -> Result<(), ErrorGuaranteed>, + F: FnOnce(TyCtxt<'tcx>, LocalDefId) -> Result<(), ErrorGuaranteed>, { if Some(self.trait_def_id) == trait_def_id { f(self.tcx, self.impl_def_id) } else { Ok(()) } } From 97b4b7f72b72965f4cde19761064821e2b0dc435 Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Fri, 9 Feb 2024 20:25:18 +0000 Subject: [PATCH 4/5] use `impl Trait` argument instead of generic param for simplicity --- compiler/rustc_hir_analysis/src/coherence/builtin.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_hir_analysis/src/coherence/builtin.rs b/compiler/rustc_hir_analysis/src/coherence/builtin.rs index fe1a9f2eebd66..fc911ecdad2f9 100644 --- a/compiler/rustc_hir_analysis/src/coherence/builtin.rs +++ b/compiler/rustc_hir_analysis/src/coherence/builtin.rs @@ -53,10 +53,11 @@ struct Checker<'tcx> { } impl<'tcx> Checker<'tcx> { - fn check(&self, trait_def_id: Option, f: F) -> Result<(), ErrorGuaranteed> - where - F: FnOnce(TyCtxt<'tcx>, LocalDefId) -> Result<(), ErrorGuaranteed>, - { + fn check( + &self, + trait_def_id: Option, + 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(()) } } } From 614ff0fae999f3aeff6024413993c58453b11d96 Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Fri, 9 Feb 2024 20:33:23 +0000 Subject: [PATCH 5/5] Don't reinvoke `impl_trait_ref` query after it was already invoked --- .../rustc_hir_analysis/src/coherence/mod.rs | 2 +- .../src/coherence/unsafety.rs | 149 +++++++++--------- 2 files changed, 74 insertions(+), 77 deletions(-) diff --git a/compiler/rustc_hir_analysis/src/coherence/mod.rs b/compiler/rustc_hir_analysis/src/coherence/mod.rs index a71ea00a0448e..60e1e6d5f877d 100644 --- a/compiler/rustc_hir_analysis/src/coherence/mod.rs +++ b/compiler/rustc_hir_analysis/src/coherence/mod.rs @@ -131,7 +131,7 @@ 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)); } diff --git a/compiler/rustc_hir_analysis/src/coherence/unsafety.rs b/compiler/rustc_hir_analysis/src/coherence/unsafety.rs index e3b5c724cdee2..d217d53587d56 100644 --- 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(()) }