From c6255028b1b67d5b70352c93d0d8e9922958f206 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 1 Nov 2019 21:36:22 -0400 Subject: [PATCH] traits w/ dyn-overlapping impls + associated types are not dyn-safe --- src/librustc/traits/error_reporting/mod.rs | 21 +-- src/librustc/traits/object_safety.rs | 194 ++++++++++++++++----- src/librustc_data_structures/lib.rs | 1 + src/librustc_data_structures/sync.rs | 1 + src/librustc_feature/builtin_attrs.rs | 2 +- src/test/ui/issue-59387.rs | 19 ++ src/test/ui/issue-59387.stderr | 15 ++ src/test/ui/issues/issue-50781.stderr | 3 +- 8 files changed, 199 insertions(+), 57 deletions(-) create mode 100644 src/test/ui/issue-59387.rs create mode 100644 src/test/ui/issue-59387.stderr diff --git a/src/librustc/traits/error_reporting/mod.rs b/src/librustc/traits/error_reporting/mod.rs index d1c369d0abbf3..54b044bc3ccb0 100644 --- a/src/librustc/traits/error_reporting/mod.rs +++ b/src/librustc/traits/error_reporting/mod.rs @@ -704,11 +704,11 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> { if self.predicate_may_hold(&unit_obligation) { err.note( "the trait is implemented for `()`. \ - Possibly this error has been caused by changes to \ - Rust's type-inference algorithm \ - (see: https://github.com/rust-lang/rust/issues/48950 \ - for more info). Consider whether you meant to use the \ - type `()` here instead.", + Possibly this error has been caused by changes to \ + Rust's type-inference algorithm \ + (see: https://github.com/rust-lang/rust/issues/48950 \ + for more info). Consider whether you meant to use the \ + type `()` here instead.", ); } } @@ -792,7 +792,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> { *span, format!( "closure is `FnOnce` because it moves the \ - variable `{}` out of its environment", + variable `{}` out of its environment", name ), ); @@ -802,7 +802,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> { *span, format!( "closure is `FnMut` because it mutates the \ - variable `{}` here", + variable `{}` here", name ), ); @@ -1012,7 +1012,7 @@ pub fn recursive_type_with_infinite_size_error( err.span_label(span, "recursive type has infinite size"); err.help(&format!( "insert indirection (e.g., a `Box`, `Rc`, or `&`) \ - at some point to make `{}` representable", + at some point to make `{}` representable", tcx.def_path_str(type_def_id) )); err @@ -1038,10 +1038,7 @@ pub fn report_object_safety_error( let mut reported_violations = FxHashSet::default(); for violation in violations { if reported_violations.insert(violation.clone()) { - match violation.span() { - Some(span) => err.span_label(span, violation.error_msg()), - None => err.note(&violation.error_msg()), - }; + violation.annotate_diagnostic(tcx, &mut err); } } diff --git a/src/librustc/traits/object_safety.rs b/src/librustc/traits/object_safety.rs index 15f81bb3f47ed..96dd9edd46f70 100644 --- a/src/librustc/traits/object_safety.rs +++ b/src/librustc/traits/object_safety.rs @@ -12,7 +12,9 @@ use super::elaborate_predicates; use crate::traits::{self, Obligation, ObligationCause}; use crate::ty::subst::{InternalSubsts, Subst}; -use crate::ty::{self, Predicate, ToPredicate, Ty, TyCtxt, TypeFoldable, WithConstness}; +use crate::ty::WithConstness; +use crate::ty::{self, ParamTy, Predicate, ToPredicate, Ty, TyCtxt, TypeFoldable}; +use rustc_errors::DiagnosticBuilder; use rustc_hir as hir; use rustc_hir::def_id::DefId; use rustc_session::lint::builtin::WHERE_CLAUSES_OBJECT_SAFETY; @@ -20,7 +22,6 @@ use rustc_span::symbol::Symbol; use rustc_span::{Span, DUMMY_SP}; use syntax::ast; -use std::borrow::Cow; use std::iter::{self}; #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] @@ -37,55 +38,91 @@ pub enum ObjectSafetyViolation { /// Associated const. AssocConst(ast::Name, Span), + + /// Dyn-overlapping impl: a trait is not dyn-safe if it contains + /// associated types and an impl with an unsized `Self` type + /// (which may potentially overlap `dyn`). See #57893. + DynOverlappingImpl(DefId), } impl ObjectSafetyViolation { - pub fn error_msg(&self) -> Cow<'static, str> { - match *self { + pub fn annotate_diagnostic(&self, tcx: TyCtxt<'_>, diag: &mut DiagnosticBuilder<'_>) { + let span_label = |diag: &mut DiagnosticBuilder<'_>, span: Span, msg: &String| { + if span != DUMMY_SP { + diag.span_label(span, msg); + } else { + diag.note(msg); + } + }; + + match self { ObjectSafetyViolation::SizedSelf => { - "the trait cannot require that `Self : Sized`".into() + diag.note("the trait cannot require that `Self : Sized`"); } ObjectSafetyViolation::SupertraitSelf => { - "the trait cannot use `Self` as a type parameter \ - in the supertraits or where-clauses" - .into() + diag.note( + "the trait cannot use `Self` as a type parameter \ + in the supertraits or where-clauses", + ); } - ObjectSafetyViolation::Method(name, MethodViolationCode::StaticMethod, _) => { - format!("associated function `{}` has no `self` parameter", name).into() + ObjectSafetyViolation::Method(name, MethodViolationCode::StaticMethod, span) => { + span_label( + diag, + *span, + &format!("associated function `{}` has no `self` parameter", name), + ); + } + ObjectSafetyViolation::Method(name, MethodViolationCode::ReferencesSelf, span) => { + span_label( + diag, + *span, + &format!( + "method `{}` references the `Self` type in its parameters or return type", + name, + ), + ); } - ObjectSafetyViolation::Method(name, MethodViolationCode::ReferencesSelf, _) => format!( - "method `{}` references the `Self` type in its parameters or return type", - name, - ) - .into(), ObjectSafetyViolation::Method( name, MethodViolationCode::WhereClauseReferencesSelf, - _, - ) => format!("method `{}` references the `Self` type in where clauses", name).into(), - ObjectSafetyViolation::Method(name, MethodViolationCode::Generic, _) => { - format!("method `{}` has generic type parameters", name).into() + span, + ) => { + span_label( + diag, + *span, + &format!("method `{}` references the `Self` type in where clauses", name), + ); } - ObjectSafetyViolation::Method(name, MethodViolationCode::UndispatchableReceiver, _) => { - format!("method `{}`'s `self` parameter cannot be dispatched on", name).into() + ObjectSafetyViolation::Method(name, MethodViolationCode::Generic, span) => { + span_label(diag, *span, &format!("method `{}` has generic type parameters", name)); + } + ObjectSafetyViolation::Method( + name, + MethodViolationCode::UndispatchableReceiver, + span, + ) => { + span_label( + diag, + *span, + &format!("method `{}`'s `self` parameter cannot be dispatched on", name), + ); } - ObjectSafetyViolation::AssocConst(name, _) => { - format!("the trait cannot contain associated consts like `{}`", name).into() + ObjectSafetyViolation::AssocConst(name, span) => { + span_label( + diag, + *span, + &format!("the trait cannot contain associated consts like `{}`", name), + ); } - } - } - - pub fn span(&self) -> Option { - // When `span` comes from a separate crate, it'll be `DUMMY_SP`. Treat it as `None` so - // diagnostics use a `note` instead of a `span_label`. - match *self { - ObjectSafetyViolation::AssocConst(_, span) - | ObjectSafetyViolation::Method(_, _, span) - if span != DUMMY_SP => - { - Some(span) + ObjectSafetyViolation::DynOverlappingImpl(impl_def_id) => { + span_label( + diag, + tcx.def_span(*impl_def_id), + &format!( + "traits with non-method items cannot have an impl with an unsized `Self` type" + ), + ); } - _ => None, } } } @@ -179,7 +216,7 @@ fn object_safety_violations_for_trait( { // 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.struct_span_lint_hir( + let mut lint = tcx.struct_span_lint_hir( WHERE_CLAUSES_OBJECT_SAFETY, hir::CRATE_HIR_ID, *span, @@ -187,9 +224,9 @@ fn object_safety_violations_for_trait( "the trait `{}` cannot be made into an object", tcx.def_path_str(trait_def_id) ), - ) - .note(&violation.error_msg()) - .emit(); + ); + violation.annotate_diagnostic(tcx, &mut lint); + lint.emit(); false } else { true @@ -211,6 +248,26 @@ fn object_safety_violations_for_trait( .map(|item| ObjectSafetyViolation::AssocConst(item.ident.name, item.ident.span)), ); + // Issue #57893. A trait cannot be considered dyn-safe if: + // + // (a) it has associated items that are not functions and + // (b) it has a potentially dyn-overlapping impl. + // + // Why don't functions matter? Because we never resolve + // them to their normalizd type until code generation + // time, in short. + let has_associated_non_fn = + tcx.associated_items(trait_def_id).any(|assoc_item| match assoc_item.kind { + ty::AssocKind::Method => false, + ty::AssocKind::Type | ty::AssocKind::OpaqueTy | ty::AssocKind::Const => true, + }); + + if has_associated_non_fn { + if let Some(impl_def_id) = impl_potentially_overlapping_dyn_trait(tcx, trait_def_id) { + violations.push(ObjectSafetyViolation::DynOverlappingImpl(impl_def_id)); + } + } + debug!( "object_safety_violations_for_trait(trait_def_id={:?}) = {:?}", trait_def_id, violations @@ -269,6 +326,59 @@ fn predicates_reference_self(tcx: TyCtxt<'_>, trait_def_id: DefId, supertraits_o }) } +fn generics_require_sized_param(tcx: TyCtxt<'_>, def_id: DefId, param_ty: ParamTy) -> bool { + debug!("generics_require_sized_param(def_id={:?}, param_ty={:?})", def_id, param_ty); + + let sized_def_id = match tcx.lang_items().sized_trait() { + Some(def_id) => def_id, + None => { + return false; /* No Sized trait, can't require it! */ + } + }; + + // Search for a predicate like `Self : Sized` amongst the trait bounds. + let predicates = tcx.predicates_of(def_id); + let predicates = predicates.instantiate_identity(tcx).predicates; + predicates.iter().any(|predicate| match predicate { + ty::Predicate::Trait(ref trait_pred, _) => { + debug!("generics_require_sized_param: trait_pred = {:?}", trait_pred); + + trait_pred.def_id() == sized_def_id + && trait_pred.skip_binder().self_ty().is_param(param_ty.index) + } + ty::Predicate::Projection(..) + | ty::Predicate::Subtype(..) + | ty::Predicate::RegionOutlives(..) + | ty::Predicate::WellFormed(..) + | ty::Predicate::ObjectSafe(..) + | ty::Predicate::ClosureKind(..) + | ty::Predicate::TypeOutlives(..) + | ty::Predicate::ConstEvaluatable(..) => false, + }) +} + +/// Searches for an impl where the `Self` type potentially overlaps +/// `dyn Trait` (where `Trait` is the trait with def-id +/// `trait_def_id`). +fn impl_potentially_overlapping_dyn_trait(tcx: TyCtxt<'_>, trait_def_id: DefId) -> Option { + debug!("impl_potentially_overlapping_dyn_trait({:?})", trait_def_id); + let mut found_match = None; + tcx.for_each_impl(trait_def_id, |impl_def_id| { + let impl_self_ty = tcx.type_of(impl_def_id); + match impl_self_ty.kind { + ty::Param(param_ty) => { + if !generics_require_sized_param(tcx, impl_def_id, param_ty) { + found_match = Some(impl_def_id); + debug!("Match found = {:?}; for param_ty {}", found_match, param_ty.name); + } + } + _ => (), + } + }); + + found_match +} + fn trait_has_sized_self(tcx: TyCtxt<'_>, trait_def_id: DefId) -> bool { generics_require_sized_self(tcx, trait_def_id) } @@ -417,7 +527,7 @@ fn virtual_call_violation_for_method<'tcx>( tcx.def_span(method.def_id), &format!( "receiver when `Self = {}` should have a ScalarPair ABI; \ - found {:?}", + found {:?}", trait_object_ty, abi ), ); @@ -718,7 +828,7 @@ fn contains_illegal_self_type_reference<'tcx>( } } - _ => true, // walk contained types, if any + _ => true, } }); diff --git a/src/librustc_data_structures/lib.rs b/src/librustc_data_structures/lib.rs index 6db2910bca496..f4afd3bf9a2c8 100644 --- a/src/librustc_data_structures/lib.rs +++ b/src/librustc_data_structures/lib.rs @@ -23,6 +23,7 @@ #![feature(integer_atomics)] #![feature(test)] #![feature(associated_type_bounds)] +#![feature(rustc_attrs)] #![cfg_attr(unix, feature(libc))] #![allow(rustc::default_hash_types)] diff --git a/src/librustc_data_structures/sync.rs b/src/librustc_data_structures/sync.rs index fa98b4d72dda2..2e484e1fecaff 100644 --- a/src/librustc_data_structures/sync.rs +++ b/src/librustc_data_structures/sync.rs @@ -29,6 +29,7 @@ pub use std::sync::atomic::Ordering::SeqCst; cfg_if! { if #[cfg(not(parallel_compiler))] { pub auto trait Send {} + pub auto trait Sync {} impl Send for T {} diff --git a/src/librustc_feature/builtin_attrs.rs b/src/librustc_feature/builtin_attrs.rs index a38726e3de81f..71dddf8d5edf4 100644 --- a/src/librustc_feature/builtin_attrs.rs +++ b/src/librustc_feature/builtin_attrs.rs @@ -143,7 +143,7 @@ macro_rules! rustc_attr { "the `#[", stringify!($attr), "]` attribute is just used for rustc unit tests \ - and will never be stable", + and will never be stable", ), ) }; diff --git a/src/test/ui/issue-59387.rs b/src/test/ui/issue-59387.rs new file mode 100644 index 0000000000000..17bd5874e3dab --- /dev/null +++ b/src/test/ui/issue-59387.rs @@ -0,0 +1,19 @@ +trait Object { + type Output; +} + +impl Object for T { + // ^-- Here is the blanket impl; relies on `?Sized`. + type Output = U; +} + +fn foo(x: >::Output) -> U { + x +} + +fn transmute(x: T) -> U { + foo::, U>(x) + //~^ ERROR the trait `Object` cannot be made into an object +} + +fn main() {} diff --git a/src/test/ui/issue-59387.stderr b/src/test/ui/issue-59387.stderr new file mode 100644 index 0000000000000..4e2f1c30f1e33 --- /dev/null +++ b/src/test/ui/issue-59387.stderr @@ -0,0 +1,15 @@ +error[E0038]: the trait `Object` cannot be made into an object + --> $DIR/issue-59387.rs:15:11 + | +LL | / impl Object for T { +LL | | // ^-- Here is the blanket impl; relies on `?Sized`. +LL | | type Output = U; +LL | | } + | |_- traits with non-method items cannot have an impl with an unsized `Self` type +... +LL | foo::, U>(x) + | ^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Object` cannot be made into an object + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0038`. diff --git a/src/test/ui/issues/issue-50781.stderr b/src/test/ui/issues/issue-50781.stderr index 02475ea97e3d1..1f65bf3deb2fd 100644 --- a/src/test/ui/issues/issue-50781.stderr +++ b/src/test/ui/issues/issue-50781.stderr @@ -2,7 +2,7 @@ error: the trait `X` cannot be made into an object --> $DIR/issue-50781.rs:6:8 | LL | fn foo(&self) where Self: Trait; - | ^^^ + | ^^^ method `foo` references the `Self` type in where clauses | note: lint level defined here --> $DIR/issue-50781.rs:1:9 @@ -11,7 +11,6 @@ LL | #![deny(where_clauses_object_safety)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! = note: for more information, see issue #51443 - = note: method `foo` references the `Self` type in where clauses error: aborting due to previous error