Skip to content

Commit

Permalink
Auto merge of rust-lang#120835 - oli-obk:no_hir_coherence, r=cjgillot
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@bors
bors committed Feb 12, 2024
2 parents de4d615 + 614ff0f commit aebf451
Show file tree
Hide file tree
Showing 4 changed files with 109 additions and 111 deletions.
42 changes: 21 additions & 21 deletions compiler/rustc_hir_analysis/src/coherence/builtin.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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(()) }
}
}

Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand Down
5 changes: 3 additions & 2 deletions compiler/rustc_hir_analysis/src/coherence/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand Down
149 changes: 73 additions & 76 deletions compiler/rustc_hir_analysis/src/coherence/unsafety.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(())
}
24 changes: 12 additions & 12 deletions tests/ui/coherence/coherence-impls-copy.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
|
Expand All @@ -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
|
Expand All @@ -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
|
Expand Down

0 comments on commit aebf451

Please sign in to comment.