Skip to content

Improve error message for lifetime error with dyn Trait #67378

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.

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

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
170 changes: 164 additions & 6 deletions src/librustc/infer/error_reporting/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ use crate::middle::region;
use crate::traits::{
IfExpressionCause, MatchExpressionArmCause, ObligationCause, ObligationCauseCode,
};
use crate::ty::error::TypeError;
use crate::ty::error::{TypeError, ExpectedFound};
use crate::ty::{self, subst::{Subst, SubstsRef}, Region, Ty, TyCtxt, TypeFoldable};

use errors::{Applicability, DiagnosticBuilder, DiagnosticStyledString};
Expand Down Expand Up @@ -1810,10 +1810,15 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
sub_region,
"...",
);
err.span_note(sup_trace.cause.span, &format!(
"...so that the {}",
sup_trace.cause.as_requirement_str()
));

if self.try_note_static_dyn_trait_impl(
&mut err, &sub_region, sub_trace,
).is_none() {
err.span_note(sup_trace.cause.span, &format!(
"...so that the {}",
sup_trace.cause.as_requirement_str()
));
}

err.note_expected_found(
&"",
Expand All @@ -1839,9 +1844,162 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
"...",
);

if let infer::Subtype(ref sub_trace) = sub_origin {
if self.try_note_static_dyn_trait_impl(&mut err, &sub_region, sub_trace).is_some() {
err.emit();
return;
}
}

self.note_region_origin(&mut err, &sub_origin);

err.emit();
}

fn try_note_static_dyn_trait_impl(
&self,
err: &mut DiagnosticBuilder<'_>,
region: Region<'tcx>,
trace: &TypeTrace<'tcx>,
) -> Option<()> {
debug!(
"try_note_static_dyn_trait_impl: err={:?} trace={:?} region={:?}", err, trace, region,
);

if let TypeTrace {
cause,
values: ValuePairs::Types(ExpectedFound { expected, found }),
} = trace {
let expected = self.resolve_vars_if_possible(expected);

debug!(
"try_note_static_dyn_trait_impl: expected={:?} expected.kind={:?} found={:?}",
expected, expected.kind, found,
);

let dyn_static_ty = expected.walk().find(|ty|
if let ty::Dynamic(_, _) = ty.kind { true } else { false });

debug!(
"try_note_static_dyn_trait_impl: dyn_static_ty={:?} dyn_static_ty.kind={:?}",
dyn_static_ty, dyn_static_ty.map(|dyn_static_ty| &dyn_static_ty.kind),
);

let dyn_trait_name = if let Some(dyn_static_ty) = dyn_static_ty {
if let ty::Dynamic(binder, _) = dyn_static_ty.kind {
binder.skip_binder().to_string()
} else {
return None;
}
} else {
return None;
};

if let ObligationCauseCode::ExprAssignable { expr_hir_id } = cause.code {
debug!(
"try_note_static_dyn_trait_impl: expr_hir_id={:?}", expr_hir_id
);

let expr_node = self.tcx.hir().get(expr_hir_id);
let parent_node = self.tcx.hir().get(
self.tcx.hir().get_parent_node(expr_hir_id));

debug!(
"try_note_static_dyn_trait_impl: expr_node={:?} parent_node={:?}",
expr_node, parent_node,
);

let call_expr = match (expr_node, parent_node) {
// The original expression is a param to a method call. Return the call expr.
(_, hir::Node::Expr(parent_expr)) => parent_expr,
// A return expr, which should be assignable to the return type.
// In that case, return the original expr.
(hir::Node::Expr(expr), hir::Node::Block(_)) => expr,
_ => {
return None;
}
};

debug!(
"try_note_static_dyn_trait_impl: call_expr={:?} call_expr.kind={:?}",
call_expr, call_expr.kind,
);

let tables = self.in_progress_tables.unwrap().borrow();

if let hir::ExprKind::MethodCall(_, _, args) = &call_expr.kind {
let method_def_id = tables.type_dependent_def_id(call_expr.hir_id).unwrap();
let trait_def_id = self.tcx.trait_of_item(method_def_id);

debug!("try_note_static_dyn_trait_impl: trait_def_id={:?}", trait_def_id);

// As this is a method call expression, we have at least one argument.
let receiver_arg = &args[0];

debug!(
"try_note_static_dyn_trait_impl: receiver_arg.kind={:?}",
tables.expr_ty(receiver_arg).kind
);

if let ty::Ref(_, arg_ty, _) = tables.expr_ty(receiver_arg).kind {
let mut trait_impl = None;

self.tcx.for_each_relevant_impl(
trait_def_id.unwrap(),
arg_ty,
|impl_def_id| {
trait_impl = Some(impl_def_id);
});

if let Some(impl_def_id) = trait_impl {
self.note_dyn_impl_and_suggest_anon_lifetime(
err,
impl_def_id,
&dyn_trait_name
);
return Some(());
}
}
}
}
}

None
}

pub fn note_dyn_impl_and_suggest_anon_lifetime(
&self,
err: &mut DiagnosticBuilder<'_>,
impl_def_id: DefId,
dyn_trait_name: &str,
) {
let impl_span = self.tcx.sess.source_map()
.def_span(self.tcx.def_span(impl_def_id));
debug!(
"try_report_static_dyn_trait: impl_span={:?}", impl_span,
);

err.span_note(
impl_span,
&format!("...because this implementation requires it"),
);

if let Ok(snippet) = self.tcx.sess.source_map().span_to_snippet(impl_span) {
err.span_suggestion(
impl_span,
&format!(
"you can add an explicit constraint to the implementation so that it applies \
to types with less than `'static` lifetime",
),
snippet.replace(dyn_trait_name, &format!("{} + '_", &dyn_trait_name)),
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have time to do an in-depth review right now, but we should try to come up with a different way of doing this. Fiddling with the textual representation of the code is a recipe for pain later on. It produces output that isn't always correct (like the problem with the missing parens and some incorrect suggestions I noticed below), and is brittle and affected by whitespace. I'll take another look later to see if I can have more actionable feedback on what to do instead.

Applicability::Unspecified,
);
} else {
debug!(
"try_report_static_dyn_trait: oh noes impl_span={:?}", impl_span,
);
}
}
}

impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
Expand Down Expand Up @@ -1948,7 +2106,7 @@ impl<'tcx> ObligationCause<'tcx> {
use crate::traits::ObligationCauseCode::*;
match self.code {
CompareImplMethodObligation { .. } => "method type is compatible with trait",
ExprAssignable => "expression is assignable",
ExprAssignable { .. } => "expression is assignable",
MatchExpressionArm(box MatchExpressionArmCause { source, .. }) => match source {
hir::MatchSource::IfLetDesugar { .. } => "`if let` arms have compatible types",
_ => "match arms have compatible types",
Expand Down
2 changes: 2 additions & 0 deletions src/librustc/infer/error_reporting/nice_region_error/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ mod named_anon_conflict;
mod placeholder_error;
mod outlives_closure;
mod static_impl_trait;
mod static_dyn_trait;
mod trait_impl_difference;
mod util;

Expand Down Expand Up @@ -73,6 +74,7 @@ impl<'cx, 'tcx> NiceRegionError<'cx, 'tcx> {
.map(|mut diag| { diag.emit(); ErrorReported })
.or_else(|| self.try_report_anon_anon_conflict())
.or_else(|| self.try_report_outlives_closure())
.or_else(|| self.try_report_static_dyn_trait())
.or_else(|| self.try_report_static_impl_trait())
.or_else(|| self.try_report_impl_not_conforming_to_trait())
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,18 @@ impl<'a, 'tcx> NiceRegionError<'a, 'tcx> {
return None;
}

let dyn_static_ty = new_ty.walk().find(|ty|
if let ty::Dynamic(_, ty::RegionKind::ReStatic) = ty.kind { true } else { false });

debug!(
"try_report_named_anon_conflict: dyn_static_ty={:?} dyn_static_ty.kind={:?}",
dyn_static_ty, dyn_static_ty.map(|dyn_static_ty| &dyn_static_ty.kind),
);

if dyn_static_ty.is_some() {
return None;
}

if let Some((_, fndecl)) = self.find_anon_type(anon, &br) {
if self.is_return_type_anon(scope_def_id, br, fndecl).is_some()
|| self.is_self_anon(is_first, scope_def_id)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
#![allow(unused)]
//! Error Reporting for dyn Traits.
use crate::infer::error_reporting::nice_region_error::NiceRegionError;
use crate::infer::lexical_region_resolve::RegionResolutionError;
use crate::ty::{self, BoundRegion, FreeRegion, RegionKind, DefIdTree, ParamEnv};
use crate::util::common::ErrorReported;
use errors::Applicability;
use crate::infer::{SubregionOrigin, ValuePairs, TypeTrace};
use crate::ty::error::ExpectedFound;
use crate::hir;
use crate::hir::def_id::DefId;
use crate::traits::ObligationCauseCode::ExprAssignable;
use crate::traits::ObligationCause;
use crate::ty::{TyCtxt, TypeFoldable};
use crate::ty::subst::{Subst, InternalSubsts, SubstsRef};
use syntax_pos::Span;

impl<'a, 'tcx> NiceRegionError<'a, 'tcx> {
/// Print the error message for lifetime errors when a static ref to a trait object is required
/// because of a `dyn Trait` impl.
pub(super) fn try_report_static_dyn_trait(&self) -> Option<ErrorReported> {
let (span, sub, sup) = self.regions();

debug!(
"try_report_static_dyn_trait: sub={:?}, sup={:?}, error={:?}",
sub,
sup,
self.error,
);

if let Some(ref error) = self.error {
let (found, origin_span) = match error {
RegionResolutionError::ConcreteFailure(SubregionOrigin::Subtype(box TypeTrace {
cause,
values: ValuePairs::TraitRefs(ExpectedFound { expected, found }),
}), _, _) => {
(found, cause.span)
},
// FIXME there is also the other region origin!
RegionResolutionError::SubSupConflict(_, _, SubregionOrigin::Subtype(box TypeTrace {
cause,
values: ValuePairs::TraitRefs(ExpectedFound { expected, found }),
}), _, _, _) => {
(found, cause.span)
}
_ => {
return None;
}
};

debug!(
"try_report_static_dyn_trait: found={:?} origin_span={:?}",
found, origin_span,
);

let found = self.infcx.resolve_vars_if_possible(found);
let dyn_static_ty = found.self_ty().walk().find(|ty|
if let ty::Dynamic(_, _) = ty.kind { true } else { false });

debug!(
"try_report_static_dyn_trait: dyn_static_ty={:?} dyn_static_ty.kind={:?}",
dyn_static_ty, dyn_static_ty.map(|dyn_static_ty| &dyn_static_ty.kind),
);

let dyn_trait_name = if let Some(dyn_static_ty) = dyn_static_ty {
if let ty::Dynamic(binder, _) = dyn_static_ty.kind {
binder.skip_binder().to_string()
} else {
return None;
}
} else {
return None;
};

let mut trait_impl = None;

self.tcx().for_each_relevant_impl(
found.def_id,
found.self_ty(),
|impl_def_id| {
debug!(
"try_report_static_dyn_trait: for_each_relevant_impl impl_def_id={:?}",
impl_def_id,
);

trait_impl = Some(impl_def_id);
});

debug!(
"try_report_static_dyn_trait: trait_impl={:?}", trait_impl,
);

if let Some(impl_def_id) = trait_impl {
self.emit_dyn_trait_err(origin_span, &dyn_trait_name, impl_def_id);
return Some(ErrorReported);
}
}
None
}

fn emit_dyn_trait_err(&self,
expr_span: Span,
dyn_trait_name: &String,
impl_def_id: DefId,
) {
let (_, sub, sup) = self.regions();

debug!(
"emit_dyn_trait_err: sup={:?} sub={:?} expr_span={:?} dyn_trait_name={:?}",
sup, sub, expr_span, dyn_trait_name,
);

let item_span = self.tcx().sess.source_map()
.def_span(self.tcx().def_span(impl_def_id));

let (lifetime_description, lt_sp_opt) = self.tcx().msg_span_from_free_region(sup);

let impl_span = item_span;

let mut err = self.tcx().sess.struct_span_err(
expr_span,
"cannot infer an appropriate lifetime",
);

if let Some(lt_sp_opt) = lt_sp_opt {
err.span_note(
lt_sp_opt,
&format!("first, the lifetime cannot outlive {}...", lifetime_description),
);
}

err.span_note(expr_span,
&format!("but, the lifetime must be valid for the {} lifetime...", sub));


self.infcx.note_dyn_impl_and_suggest_anon_lifetime(&mut err, impl_def_id, dyn_trait_name);

err.emit();
}
}
3 changes: 2 additions & 1 deletion src/librustc/infer/error_reporting/note.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,10 +150,11 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
match origin {
infer::Subtype(box trace) => {
let terr = TypeError::RegionsDoesNotOutlive(sup, sub);
let mut err = self.report_and_explain_type_error(trace, &terr);
let mut err = self.report_and_explain_type_error(trace.clone(), &terr);
self.tcx.note_and_explain_region(region_scope_tree, &mut err, "", sup, "...");
self.tcx.note_and_explain_region(region_scope_tree, &mut err,
"...does not necessarily outlive ", sub, "");
self.try_note_static_dyn_trait_impl(&mut err, sub, &trace);
err
}
infer::Reborrow(span) => {
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/traits/error_reporting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2462,7 +2462,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
{
let tcx = self.tcx;
match *cause_code {
ObligationCauseCode::ExprAssignable |
ObligationCauseCode::ExprAssignable { .. } |
ObligationCauseCode::MatchExpressionArm { .. } |
ObligationCauseCode::MatchExpressionArmPattern { .. } |
ObligationCauseCode::IfExpression { .. } |
Expand Down
Loading