Skip to content

Object safe for dispatch #57545

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 23, 2019
Merged

Object safe for dispatch #57545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions src/librustc/infer/error_reporting/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1146,10 +1146,17 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {

let span = cause.span(self.tcx);

diag.span_label(span, terr.to_string());
if let Some((sp, msg)) = secondary_span {
diag.span_label(sp, msg);
}
// Ignore msg for object safe coercion
// since E0038 message will be printed
match terr {
TypeError::ObjectUnsafeCoercion(_) => {}
_ => {
diag.span_label(span, terr.to_string());
if let Some((sp, msg)) = secondary_span {
diag.span_label(sp, msg);
}
}
};

if let Some((expected, found)) = expected_found {
match (terr, is_simple_error, expected == found) {
Expand All @@ -1169,6 +1176,9 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
&sort_string(values.found),
);
}
(TypeError::ObjectUnsafeCoercion(_), ..) => {
diag.note_unsuccessfull_coercion(found, expected);
}
(_, false, _) => {
if let Some(exp_found) = exp_found {
self.suggest_as_ref_where_appropriate(span, &exp_found, diag);
Expand Down Expand Up @@ -1267,6 +1277,10 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
let span = trace.cause.span(self.tcx);
let failure_code = trace.cause.as_failure_code(terr);
let mut diag = match failure_code {
FailureCode::Error0038(did) => {
let violations = self.tcx.object_safety_violations(did);
self.tcx.report_object_safety_error(span, did, violations)
}
FailureCode::Error0317(failure_str) => {
struct_span_err!(self.tcx.sess, span, E0317, "{}", failure_str)
}
Expand Down Expand Up @@ -1628,6 +1642,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
}

enum FailureCode {
Error0038(DefId),
Error0317(&'static str),
Error0580(&'static str),
Error0308(&'static str),
Expand Down Expand Up @@ -1666,6 +1681,7 @@ impl<'tcx> ObligationCause<'tcx> {
TypeError::IntrinsicCast => {
Error0308("cannot coerce intrinsics to function pointers")
}
TypeError::ObjectUnsafeCoercion(did) => Error0038(did.clone()),
_ => Error0308("mismatched types"),
},
}
Expand Down
32 changes: 15 additions & 17 deletions src/librustc/traits/error_reporting.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -790,15 +790,11 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {

ty::Predicate::ObjectSafe(trait_def_id) => {
let violations = self.tcx.object_safety_violations(trait_def_id);
if let Some(err) = self.tcx.report_object_safety_error(
self.tcx.report_object_safety_error(
span,
trait_def_id,
violations,
) {
err
} else {
return;
}
)
}

ty::Predicate::ClosureKind(closure_def_id, closure_substs, kind) => {
Expand Down Expand Up @@ -934,11 +930,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {

TraitNotObjectSafe(did) => {
let violations = self.tcx.object_safety_violations(did);
if let Some(err) = self.tcx.report_object_safety_error(span, did, violations) {
err
} else {
return;
}
self.tcx.report_object_safety_error(span, did, violations)
}

// already reported in the query
Expand Down Expand Up @@ -1493,11 +1485,7 @@ impl<'tcx> TyCtxt<'tcx> {
span: Span,
trait_def_id: DefId,
violations: Vec<ObjectSafetyViolation>,
) -> Option<DiagnosticBuilder<'tcx>> {
if self.sess.trait_methods_not_found.borrow().contains(&span) {
// Avoid emitting error caused by non-existing method (#58734)
return None;
}
) -> DiagnosticBuilder<'tcx> {
let trait_str = self.def_path_str(trait_def_id);
let span = self.sess.source_map().def_span(span);
let mut err = struct_span_err!(
Expand All @@ -1515,7 +1503,13 @@ impl<'tcx> TyCtxt<'tcx> {
};
}
}
Some(err)

if self.sess.trait_methods_not_found.borrow().contains(&span) {
// Avoid emitting error caused by non-existing method (#58734)
err.cancel();
}

err
}
}

Expand Down Expand Up @@ -1926,6 +1920,10 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
err.note(&format!("required for the cast to the object type `{}`",
self.ty_to_string(object_ty)));
}
ObligationCauseCode::Coercion { source: _, target } => {
err.note(&format!("required by cast to type `{}`",
self.ty_to_string(target)));
}
ObligationCauseCode::RepeatVec => {
err.note("the `Copy` trait is required because the \
repeated element will be copied");
Expand Down
3 changes: 3 additions & 0 deletions src/librustc/traits/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,9 @@ pub enum ObligationCauseCode<'tcx> {
/// Obligation incurred due to an object cast.
ObjectCastObligation(/* Object type */ Ty<'tcx>),

/// Obligation incurred due to a coercion.
Coercion { source: Ty<'tcx>, target: Ty<'tcx> },

// Various cases where expressions must be sized/copy/etc:
/// L = X implies that L is Sized
AssignmentLhsSized,
Expand Down
8 changes: 7 additions & 1 deletion src/librustc/traits/select.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2246,7 +2246,13 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
}

if let Some(principal) = data.principal() {
principal.with_self_ty(self.tcx(), self_ty)
if !self.infcx.tcx.features().object_safe_for_dispatch {
principal.with_self_ty(self.tcx(), self_ty)
} else if self.tcx().is_object_safe(principal.def_id()) {
principal.with_self_ty(self.tcx(), self_ty)
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

else if?

return;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the check for the feature flag necessary here? It seems like you could check if the trait is object safe anyway, even if the feature is not enabled. Or would that result in duplicate errors?

Copy link
Contributor Author

@bovinebuddha bovinebuddha Mar 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is the code which adds the automatic 'impl Trait for dyn Trait' predicate to trait objects. Maybe the branches should be clarified and a comment added. Like:

// If we should add the automatic 'impl Trait for dyn Trait' or not
let object_impl_trait = !self.infcx.tcx.features().object_safe_for_dispatch ||
                        self.tcx().is_object_safe(principal.def_id());
 
if (object_impl_trait) {
  principal.with_self_ty(self.tcx(), self_ty)
} else {
 return;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At minimum, a comment would be good here -- I think I vaguely prefer the if object_impl_trait { .. } version as well.

But a comment like this would be great:

// Prior to RFC 2027, dyn Foo: Foo was always true. But RFC 2027 makes that only true if Foo is object safe.

} else {
// Only auto-trait bounds exist.
return;
Expand Down
4 changes: 4 additions & 0 deletions src/librustc/traits/structural_impls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,6 +481,10 @@ impl<'a, 'tcx> Lift<'tcx> for traits::ObligationCauseCode<'a> {
.and_then(|r| Some(super::ObjectTypeBound(ty, r)))
),
super::ObjectCastObligation(ty) => tcx.lift(&ty).map(super::ObjectCastObligation),
super::Coercion { source, target } => Some(super::Coercion {
source: tcx.lift(&source)?,
target: tcx.lift(&target)?,
}),
super::AssignmentLhsSized => Some(super::AssignmentLhsSized),
super::TupleInitializerSized => Some(super::TupleInitializerSized),
super::StructInitializerSized => Some(super::StructInitializerSized),
Expand Down
3 changes: 2 additions & 1 deletion src/librustc/ty/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ pub enum TypeError<'tcx> {
ProjectionMismatched(ExpectedFound<DefId>),
ProjectionBoundsLength(ExpectedFound<usize>),
ExistentialMismatch(ExpectedFound<&'tcx ty::List<ty::ExistentialPredicate<'tcx>>>),

ObjectUnsafeCoercion(DefId),
ConstMismatch(ExpectedFound<&'tcx ty::Const<'tcx>>),

IntrinsicCast,
Expand Down Expand Up @@ -179,6 +179,7 @@ impl<'tcx> fmt::Display for TypeError<'tcx> {
IntrinsicCast => {
write!(f, "cannot coerce intrinsics to function pointers")
}
ObjectUnsafeCoercion(_) => write!(f, "coercion to object-unsafe trait object"),
}
}
}
Expand Down
2 changes: 2 additions & 0 deletions src/librustc/ty/structural_impls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -749,6 +749,7 @@ impl<'a, 'tcx> Lift<'tcx> for ty::error::TypeError<'a> {
ExistentialMismatch(ref x) => return tcx.lift(x).map(ExistentialMismatch),
ConstMismatch(ref x) => return tcx.lift(x).map(ConstMismatch),
IntrinsicCast => IntrinsicCast,
ObjectUnsafeCoercion(ref x) => return tcx.lift(x).map(ObjectUnsafeCoercion),
})
}
}
Expand Down Expand Up @@ -1356,6 +1357,7 @@ EnumTypeFoldableImpl! {
(ty::error::TypeError::ExistentialMismatch)(x),
(ty::error::TypeError::ConstMismatch)(x),
(ty::error::TypeError::IntrinsicCast),
(ty::error::TypeError::ObjectUnsafeCoercion)(x),
}
}

Expand Down
25 changes: 15 additions & 10 deletions src/librustc/ty/wf.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,16 +380,21 @@ impl<'a, 'tcx> WfPredicates<'a, 'tcx> {
// obligations that don't refer to Self and
// checking those

let cause = self.cause(traits::MiscObligation);
let component_traits =
data.auto_traits().chain(data.principal_def_id());
self.out.extend(
component_traits.map(|did| traits::Obligation::new(
cause.clone(),
param_env,
ty::Predicate::ObjectSafe(did)
))
);
let defer_to_coercion =
self.infcx.tcx.features().object_safe_for_dispatch;

if !defer_to_coercion {
let cause = self.cause(traits::MiscObligation);
let component_traits =
data.auto_traits().chain(data.principal_def_id());
self.out.extend(
component_traits.map(|did| traits::Obligation::new(
cause.clone(),
param_env,
ty::Predicate::ObjectSafe(did)
))
);
}
}

// Inference variables are the complicated case, since we don't
Expand Down
26 changes: 26 additions & 0 deletions src/librustc_errors/diagnostic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,32 @@ impl Diagnostic {
self.note_expected_found_extra(label, expected, found, &"", &"")
}

pub fn note_unsuccessfull_coercion(&mut self,
expected: DiagnosticStyledString,
found: DiagnosticStyledString)
-> &mut Self
{
let mut msg: Vec<_> =
vec![(format!("required when trying to coerce from type `"),
Style::NoStyle)];
msg.extend(expected.0.iter()
.map(|x| match *x {
StringPart::Normal(ref s) => (s.to_owned(), Style::NoStyle),
StringPart::Highlighted(ref s) => (s.to_owned(), Style::Highlight),
}));
msg.push((format!("` to type '"), Style::NoStyle));
msg.extend(found.0.iter()
.map(|x| match *x {
StringPart::Normal(ref s) => (s.to_owned(), Style::NoStyle),
StringPart::Highlighted(ref s) => (s.to_owned(), Style::Highlight),
}));
msg.push((format!("`"), Style::NoStyle));

// For now, just attach these as notes
self.highlighted_note(msg);
self
}

pub fn note_expected_found_extra(&mut self,
label: &dyn fmt::Display,
expected: DiagnosticStyledString,
Expand Down
5 changes: 5 additions & 0 deletions src/librustc_errors/diagnostic_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,11 @@ impl<'a> DiagnosticBuilder<'a> {
found_extra: &dyn fmt::Display,
) -> &mut Self);

forward!(pub fn note_unsuccessfull_coercion(&mut self,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I think it is spelled unsuccessful (with one l), at least in American English.

expected: DiagnosticStyledString,
found: DiagnosticStyledString,
) -> &mut Self);

forward!(pub fn note(&mut self, msg: &str) -> &mut Self);
forward!(pub fn span_note<S: Into<MultiSpan>>(&mut self,
sp: S,
Expand Down
3 changes: 1 addition & 2 deletions src/librustc_typeck/astconv.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1275,8 +1275,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
span,
item.trait_ref().def_id(),
object_safety_violations
)
.map(|mut err| err.emit());
).emit();
return tcx.types.err;
}
}
Expand Down
43 changes: 32 additions & 11 deletions src/librustc_typeck/check/cast.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,21 +428,36 @@ impl<'a, 'tcx> CastCheck<'tcx> {
self.report_cast_to_unsized_type(fcx);
} else if self.expr_ty.references_error() || self.cast_ty.references_error() {
// No sense in giving duplicate error messages
} else if self.try_coercion_cast(fcx) {
self.trivial_cast_lint(fcx);
debug!(" -> CoercionCast");
fcx.tables.borrow_mut().set_coercion_cast(self.expr.hir_id.local_id);

} else {
match self.do_check(fcx) {
Ok(k) => {
debug!(" -> {:?}", k);
match self.try_coercion_cast(fcx) {
Ok(()) => {
self.trivial_cast_lint(fcx);
debug!(" -> CoercionCast");
fcx.tables.borrow_mut()
.set_coercion_cast(self.expr.hir_id.local_id);
}
Err(ty::error::TypeError::ObjectUnsafeCoercion(did)) => {
self.report_object_unsafe_cast(&fcx, did);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these changes would fall out from the approach I highlighted, and then changing the trait error handling code.

}
Err(_) => {
match self.do_check(fcx) {
Ok(k) => {
debug!(" -> {:?}", k);
}
Err(e) => self.report_cast_error(fcx, e),
};
}
Err(e) => self.report_cast_error(fcx, e),
};
}
}

fn report_object_unsafe_cast(&self, fcx: &FnCtxt<'a, 'tcx>, did: DefId) {
let violations = fcx.tcx.object_safety_violations(did);
let mut err = fcx.tcx.report_object_safety_error(self.cast_span, did, violations);
err.note(&format!("required by cast to type '{}'", fcx.ty_to_string(self.cast_ty)));
err.emit();
}

/// Checks a cast, and report an error if one exists. In some cases, this
/// can return Ok and create type errors in the fcx rather than returning
/// directly. coercion-cast is handled in check instead of here.
Expand Down Expand Up @@ -646,8 +661,14 @@ impl<'a, 'tcx> CastCheck<'tcx> {
}
}

fn try_coercion_cast(&self, fcx: &FnCtxt<'a, 'tcx>) -> bool {
fcx.try_coerce(self.expr, self.expr_ty, self.cast_ty, AllowTwoPhase::No).is_ok()
fn try_coercion_cast(
&self,
fcx: &FnCtxt<'a, 'tcx>,
) -> Result<(), ty::error::TypeError<'_>> {
match fcx.try_coerce(self.expr, self.expr_ty, self.cast_ty, AllowTwoPhase::No) {
Ok(_) => Ok(()),
Err(err) => Err(err),
}
}
}

Expand Down
Loading