Skip to content

Commit

Permalink
Auto merge of rust-lang#125380 - compiler-errors:wc-obj-safety, r=oli…
Browse files Browse the repository at this point in the history
…-obk

Make `WHERE_CLAUSES_OBJECT_SAFETY` a regular object safety violation

#### The issue

In rust-lang#50781, we have known about unsound `where` clauses in function arguments:

```rust
trait Impossible {}

trait Foo {
    fn impossible(&self)
    where
        Self: Impossible;
}

impl Foo for &() {
    fn impossible(&self)
    where
        Self: Impossible,
    {}
}

// `where` clause satisfied for the object, meaning that the function now *looks* callable.
impl Impossible for dyn Foo {}

fn main() {
    let x: &dyn Foo = &&();
    x.impossible();
}
```

... which currently segfaults at runtime because we try to call a method in the vtable that doesn't exist. :(

#### What did u change

This PR removes the `WHERE_CLAUSES_OBJECT_SAFETY` lint and instead makes it a regular object safety violation. I choose to make this into a hard error immediately rather than a `deny` because of the time that has passed since this lint was authored, and the single (1) regression (see below).

That means that it's OK to mention `where Self: Trait` where clauses in your trait, but making such a trait into a `dyn Trait` object will report an object safety violation just like `where Self: Sized`, etc.

```rust
trait Impossible {}

trait Foo {
    fn impossible(&self)
    where
        Self: Impossible; // <~ This definition is valid, just not object-safe.
}

impl Foo for &() {
    fn impossible(&self)
    where
        Self: Impossible,
    {}
}

fn main() {
    let x: &dyn Foo = &&(); // <~ THIS is where we emit an error.
}
```

#### Regressions

From a recent crater run, there's only one crate that relies on this behavior: rust-lang#124305 (comment). The crate looks unmaintained and there seems to be no dependents.

#### Further

We may later choose to relax this (e.g. when the where clause is implied by the supertraits of the trait or something), but this is not something I propose to do in this FCP.

For example, given:

```
trait Tr {
  fn f(&self) where Self: Blanket;
}

impl<T: ?Sized> Blanket for T {}
```

Proving that some placeholder `S` implements `S: Blanket` would be sufficient to prove that the same (blanket) impl applies for both `Concerete: Blanket` and `dyn Trait: Blanket`.

Repeating here that I don't think we need to implement this behavior right now.

----

r? lcnr
  • Loading branch information
bors committed Jun 3, 2024
2 parents 7c52d2d + 511f1cf commit e6ebf60
Show file tree
Hide file tree
Showing 28 changed files with 92 additions and 209 deletions.
2 changes: 1 addition & 1 deletion compiler/rustc_hir_analysis/src/check/wfcheck.rs
Original file line number Diff line number Diff line change
Expand Up @@ -881,7 +881,7 @@ fn check_object_unsafe_self_trait_by_name(tcx: TyCtxt<'_>, item: &hir::TraitItem
_ => {}
}
if !trait_should_be_self.is_empty() {
if tcx.check_is_object_safe(trait_def_id) {
if tcx.is_object_safe(trait_def_id) {
return;
}
let sugg = trait_should_be_self.iter().map(|span| (*span, "Self".to_string())).collect();
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_hir_analysis/src/coherence/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ fn check_object_overlap<'tcx>(
});

for component_def_id in component_def_ids {
if !tcx.check_is_object_safe(component_def_id) {
if !tcx.is_object_safe(component_def_id) {
// Without the 'object_safe_for_dispatch' feature this is an error
// which will be reported by wfcheck. Ignore it here.
// This is tested by `coherence-impl-trait-for-trait-object-safe.rs`.
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_hir_analysis/src/hir_ty_lowering/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
// For recursive traits, don't downgrade the error. (#119652)
is_downgradable = false;
}
tcx.check_is_object_safe(id)
tcx.is_object_safe(id)
}
_ => false,
})
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_interface/src/passes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -832,7 +832,7 @@ fn analysis(tcx: TyCtxt<'_>, (): ()) -> Result<()> {
let traits = tcx.traits(LOCAL_CRATE);

for &tr in traits {
if !tcx.check_is_object_safe(tr) {
if !tcx.is_object_safe(tr) {
continue;
}

Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_lint/src/multiple_supertrait_upcastable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,10 @@ declare_lint_pass!(MultipleSupertraitUpcastable => [MULTIPLE_SUPERTRAIT_UPCASTAB
impl<'tcx> LateLintPass<'tcx> for MultipleSupertraitUpcastable {
fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'tcx>) {
let def_id = item.owner_id.to_def_id();
// NOTE(nbdd0121): use `object_safety_violations` instead of `check_is_object_safe` because
// NOTE(nbdd0121): use `object_safety_violations` instead of `is_object_safe` because
// the latter will report `where_clause_object_safety` lint.
if let hir::ItemKind::Trait(_, _, _, _, _) = item.kind
&& cx.tcx.object_safety_violations(def_id).is_empty()
&& cx.tcx.is_object_safe(def_id)
{
let direct_super_traits_iter = cx
.tcx
Expand Down
42 changes: 0 additions & 42 deletions compiler/rustc_lint_defs/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,6 @@ declare_lint_pass! {
USELESS_DEPRECATED,
WARNINGS,
WASM_C_ABI,
WHERE_CLAUSES_OBJECT_SAFETY,
WRITES_THROUGH_IMMUTABLE_POINTER,
// tidy-alphabetical-end
]
Expand Down Expand Up @@ -2093,47 +2092,6 @@ declare_lint! {
"detects labels that are never used"
}

declare_lint! {
/// The `where_clauses_object_safety` lint detects for [object safety] of
/// [where clauses].
///
/// [object safety]: https://doc.rust-lang.org/reference/items/traits.html#object-safety
/// [where clauses]: https://doc.rust-lang.org/reference/items/generics.html#where-clauses
///
/// ### Example
///
/// ```rust,no_run
/// trait Trait {}
///
/// trait X { fn foo(&self) where Self: Trait; }
///
/// impl X for () { fn foo(&self) {} }
///
/// impl Trait for dyn X {}
///
/// // Segfault at opt-level 0, SIGILL otherwise.
/// pub fn main() { <dyn X as X>::foo(&()); }
/// ```
///
/// {{produces}}
///
/// ### Explanation
///
/// The compiler previously allowed these object-unsafe bounds, which was
/// incorrect. This is a [future-incompatible] lint to transition this to
/// a hard error in the future. See [issue #51443] for more details.
///
/// [issue #51443]: https://github.com/rust-lang/rust/issues/51443
/// [future-incompatible]: ../index.md#future-incompatible-lints
pub WHERE_CLAUSES_OBJECT_SAFETY,
Warn,
"checks the object safety of where clauses",
@future_incompatible = FutureIncompatibleInfo {
reason: FutureIncompatibilityReason::FutureReleaseErrorDontReportInDeps,
reference: "issue #51443 <https://github.com/rust-lang/rust/issues/51443>",
};
}

declare_lint! {
/// The `proc_macro_derive_resolution_fallback` lint detects proc macro
/// derives using inaccessible names from parent modules.
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_middle/src/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1309,7 +1309,7 @@ rustc_queries! {
query object_safety_violations(trait_id: DefId) -> &'tcx [ObjectSafetyViolation] {
desc { |tcx| "determining object safety of trait `{}`", tcx.def_path_str(trait_id) }
}
query check_is_object_safe(trait_id: DefId) -> bool {
query is_object_safe(trait_id: DefId) -> bool {
desc { |tcx| "checking if trait `{}` is object safe", tcx.def_path_str(trait_id) }
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,7 @@ pub fn transform_instance<'tcx>(
let trait_method = tcx.associated_item(method_id);
let trait_id = trait_ref.skip_binder().def_id;
if traits::is_vtable_safe_method(tcx, trait_id, trait_method)
&& tcx.object_safety_violations(trait_id).is_empty()
&& tcx.is_object_safe(trait_id)
{
// Trait methods will have a Self polymorphic parameter, where the concreteized
// implementatation will not. We need to walk back to the more general trait method
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_trait_selection/src/solve/assembly/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -714,7 +714,7 @@ impl<'tcx> EvalCtxt<'_, InferCtxt<'tcx>> {
};

// Do not consider built-in object impls for non-object-safe types.
if bounds.principal_def_id().is_some_and(|def_id| !tcx.check_is_object_safe(def_id)) {
if bounds.principal_def_id().is_some_and(|def_id| !tcx.is_object_safe(def_id)) {
return;
}

Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_trait_selection/src/solve/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ impl<'a, 'tcx> EvalCtxt<'a, InferCtxt<'tcx>> {
}

fn compute_object_safe_goal(&mut self, trait_def_id: DefId) -> QueryResult<'tcx> {
if self.interner().check_is_object_safe(trait_def_id) {
if self.interner().is_object_safe(trait_def_id) {
self.evaluate_added_goals_and_make_canonical_response(Certainty::Yes)
} else {
Err(NoSolution)
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_trait_selection/src/solve/trait_goals.rs
Original file line number Diff line number Diff line change
Expand Up @@ -789,7 +789,7 @@ impl<'tcx> EvalCtxt<'_, InferCtxt<'tcx>> {
let Goal { predicate: (a_ty, _), .. } = goal;

// Can only unsize to an object-safe trait.
if b_data.principal_def_id().is_some_and(|def_id| !tcx.check_is_object_safe(def_id)) {
if b_data.principal_def_id().is_some_and(|def_id| !tcx.is_object_safe(def_id)) {
return Err(NoSolution);
}

Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_trait_selection/src/traits/fulfill.rs
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,7 @@ impl<'a, 'tcx> ObligationProcessor for FulfillProcessor<'a, 'tcx> {
}

ty::PredicateKind::ObjectSafe(trait_def_id) => {
if !self.selcx.tcx().check_is_object_safe(trait_def_id) {
if !self.selcx.tcx().is_object_safe(trait_def_id) {
ProcessResult::Error(FulfillmentErrorCode::Select(Unimplemented))
} else {
ProcessResult::Changed(vec![])
Expand Down
85 changes: 5 additions & 80 deletions compiler/rustc_trait_selection/src/traits/object_safety.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use super::elaborate;
use crate::infer::TyCtxtInferExt;
use crate::traits::query::evaluate_obligation::InferCtxtExt;
use crate::traits::{self, Obligation, ObligationCause};
use rustc_errors::{FatalError, MultiSpan};
use rustc_errors::FatalError;
use rustc_hir as hir;
use rustc_hir::def_id::DefId;
use rustc_middle::query::Providers;
Expand All @@ -23,7 +23,6 @@ use rustc_middle::ty::{
};
use rustc_middle::ty::{GenericArg, GenericArgs};
use rustc_middle::ty::{TypeVisitableExt, Upcast};
use rustc_session::lint::builtin::WHERE_CLAUSES_OBJECT_SAFETY;
use rustc_span::symbol::Symbol;
use rustc_span::Span;
use rustc_target::abi::Abi;
Expand Down Expand Up @@ -65,45 +64,14 @@ fn object_safety_violations(tcx: TyCtxt<'_>, trait_def_id: DefId) -> &'_ [Object
)
}

fn check_is_object_safe(tcx: TyCtxt<'_>, trait_def_id: DefId) -> bool {
let violations = tcx.object_safety_violations(trait_def_id);

if violations.is_empty() {
return true;
}

// If the trait contains any other violations, then let the error reporting path
// report it instead of emitting a warning here.
if violations.iter().all(|violation| {
matches!(
violation,
ObjectSafetyViolation::Method(_, MethodViolationCode::WhereClauseReferencesSelf, _)
)
}) {
for violation in violations {
if let ObjectSafetyViolation::Method(
_,
MethodViolationCode::WhereClauseReferencesSelf,
span,
) = violation
{
lint_object_unsafe_trait(tcx, *span, trait_def_id, violation);
}
}
return true;
}

false
fn is_object_safe(tcx: TyCtxt<'_>, trait_def_id: DefId) -> bool {
tcx.object_safety_violations(trait_def_id).is_empty()
}

/// We say a method is *vtable safe* if it can be invoked on a trait
/// object. Note that object-safe traits can have some
/// non-vtable-safe methods, so long as they require `Self: Sized` or
/// otherwise ensure that they cannot be used when `Self = Trait`.
///
/// [`MethodViolationCode::WhereClauseReferencesSelf`] is considered object safe due to backwards
/// compatibility, see <https://github.com/rust-lang/rust/issues/51443> and
/// [`WHERE_CLAUSES_OBJECT_SAFETY`].
pub fn is_vtable_safe_method(tcx: TyCtxt<'_>, trait_def_id: DefId, method: ty::AssocItem) -> bool {
debug_assert!(tcx.generics_of(trait_def_id).has_self);
debug!("is_vtable_safe_method({:?}, {:?})", trait_def_id, method);
Expand All @@ -112,9 +80,7 @@ pub fn is_vtable_safe_method(tcx: TyCtxt<'_>, trait_def_id: DefId, method: ty::A
return false;
}

virtual_call_violations_for_method(tcx, trait_def_id, method)
.iter()
.all(|v| matches!(v, MethodViolationCode::WhereClauseReferencesSelf))
virtual_call_violations_for_method(tcx, trait_def_id, method).is_empty()
}

fn object_safety_violations_for_trait(
Expand Down Expand Up @@ -163,47 +129,6 @@ fn object_safety_violations_for_trait(
violations
}

/// Lint object-unsafe trait.
fn lint_object_unsafe_trait(
tcx: TyCtxt<'_>,
span: Span,
trait_def_id: DefId,
violation: &ObjectSafetyViolation,
) {
// Using `CRATE_NODE_ID` is wrong, but it's hard to get a more precise id.
// It's also hard to get a use site span, so we use the method definition span.
tcx.node_span_lint(WHERE_CLAUSES_OBJECT_SAFETY, hir::CRATE_HIR_ID, span, |err| {
err.primary_message(format!(
"the trait `{}` cannot be made into an object",
tcx.def_path_str(trait_def_id)
));
let node = tcx.hir().get_if_local(trait_def_id);
let mut spans = MultiSpan::from_span(span);
if let Some(hir::Node::Item(item)) = node {
spans.push_span_label(item.ident.span, "this trait cannot be made into an object...");
spans.push_span_label(span, format!("...because {}", violation.error_msg()));
} else {
spans.push_span_label(
span,
format!(
"the trait cannot be made into an object because {}",
violation.error_msg()
),
);
};
err.span_note(
spans,
"for a trait to be \"object safe\" it needs to allow building a vtable to allow the \
call to be resolvable dynamically; for more information visit \
<https://doc.rust-lang.org/reference/items/traits.html#object-safety>",
);
if node.is_some() {
// Only provide the help if its a local trait, otherwise it's not
violation.solution().add_to(err);
}
});
}

fn sized_trait_bound_spans<'tcx>(
tcx: TyCtxt<'tcx>,
bounds: hir::GenericBounds<'tcx>,
Expand Down Expand Up @@ -929,7 +854,7 @@ pub fn contains_illegal_impl_trait_in_trait<'tcx>(
pub fn provide(providers: &mut Providers) {
*providers = Providers {
object_safety_violations,
check_is_object_safe,
is_object_safe,
generics_require_sized_self,
..*providers
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -870,7 +870,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
if let Some(principal) = data.principal() {
if !self.infcx.tcx.features().object_safe_for_dispatch {
principal.with_self_ty(self.tcx(), self_ty)
} else if self.tcx().check_is_object_safe(principal.def_id()) {
} else if self.tcx().is_object_safe(principal.def_id()) {
principal.with_self_ty(self.tcx(), self_ty)
} else {
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1222,7 +1222,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
// `T` -> `Trait`
(_, &ty::Dynamic(data, r, ty::Dyn)) => {
let mut object_dids = data.auto_traits().chain(data.principal_def_id());
if let Some(did) = object_dids.find(|did| !tcx.check_is_object_safe(*did)) {
if let Some(did) = object_dids.find(|did| !tcx.is_object_safe(*did)) {
return Err(TraitNotObjectSafe(did));
}

Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_trait_selection/src/traits/select/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -798,7 +798,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
}

ty::PredicateKind::ObjectSafe(trait_def_id) => {
if self.tcx().check_is_object_safe(trait_def_id) {
if self.tcx().is_object_safe(trait_def_id) {
Ok(EvaluatedToOk)
} else {
Ok(EvaluatedToErr)
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/clean/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1451,7 +1451,7 @@ impl Trait {
tcx.trait_def(self.def_id).safety
}
pub(crate) fn is_object_safe(&self, tcx: TyCtxt<'_>) -> bool {
tcx.check_is_object_safe(self.def_id)
tcx.is_object_safe(self.def_id)
}
}

Expand Down
19 changes: 0 additions & 19 deletions src/tools/miri/tests/fail/issue-miri-2432.rs

This file was deleted.

15 changes: 0 additions & 15 deletions src/tools/miri/tests/fail/issue-miri-2432.stderr

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,22 +1,21 @@
#![feature(generic_const_exprs)]
#![allow(incomplete_features)]
#![deny(where_clauses_object_safety)]


const fn bar<T: ?Sized>() -> usize { 7 }

trait Foo {
fn test(&self) where [u8; bar::<Self>()]: Sized;
//~^ ERROR the trait `Foo` cannot be made into an object
//~| WARN this was previously accepted by the compiler but is being phased out
}

impl Foo for () {
fn test(&self) where [u8; bar::<Self>()]: Sized {}
}

fn use_dyn(v: &dyn Foo) {
//~^ ERROR the trait `Foo` cannot be made into an object
v.test();
//~^ ERROR the trait `Foo` cannot be made into an object
}

fn main() {}
Loading

0 comments on commit e6ebf60

Please sign in to comment.