Skip to content
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

Enforce short type string in more errors #136898

Closed
wants to merge 1 commit into from
Closed

Enforce short type string in more errors #136898

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
31 changes: 17 additions & 14 deletions compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,8 +312,9 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
..
} = use_spans
{
let ty = self.infcx.tcx.short_string(deref_target_ty, err.long_ty_path());
err.note(format!(
"{} occurs due to deref coercion to `{deref_target_ty}`",
"{} occurs due to deref coercion to `{ty}`",
desired_action.as_noun(),
));

Expand Down Expand Up @@ -1222,13 +1223,14 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
),
};
let prefix = if !self.implements_clone(ty) {
let msg = format!("`{ty}` doesn't implement `Copy` or `Clone`");
let t = self.infcx.tcx.short_string(ty, err.long_ty_path());
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems a bit awful to have to do this everywhere. Can't we do something similar to the other global settings that change formatting for a scope?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I agree. My initial hope was to make it so only main messages needed to use this, but an even better approach would be to continue moving towards structured diagnostics and making the machinery handle the printing of Tys to always use short strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oli-obk would you prefer we came up with a "hands off" solution that doesn't require as much redundant logic first, or land this while I work on that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, I think the added verbosity to diagnostics impls that will get reverted later is not worth it now and would def prefer going for another solution first

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm encountering a small issue: TyCtxt contains the DiagCtxt, which gets constructed early. In order to properly shorten types, you need a FmtPrinter, which has a TyCtxt within it. I don't think that we can make it so DiagCtxt contains an optional TyCtxt (I'm assuming if I go down that route I'll end up with a cycle), which instead would require me to pass a tcx to every create_err and emit_err. There are >1500 of these. I created a new trait with two impls, one that shortens and one on () that does nothing, so when you don't have a TyCtxt it doesn't attempt to shorten, but I would still have to touch a significant part of the codebase to introduce this. And it would still only work for structured diagnostics. I think that for raw access I could introduce a macro so that instead of doing err.span_label(span, format!("{ty}")) we would do span_label!(tcx, err, span, "{ty}"), which should potentially allow me to customize the printing behavior when it comes to things that can be shortened, but whatever pattern I can think of, it will be intrusive one way or another :-/

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh. The other schemes just use TLS I think?

let msg = format!("`{t}` doesn't implement `Copy` or `Clone`");
if let ty::Adt(def, _) = ty.kind() {
err.span_note(self.infcx.tcx.def_span(def.did()), msg);
} else {
err.note(msg);
}
format!("if `{ty}` implemented `Clone`, you could ")
format!("if `{t}` implemented `Clone`, you could ")
} else {
String::new()
};
Expand Down Expand Up @@ -1270,10 +1272,8 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
let mut span: MultiSpan = ty_span.into();
span.push_span_label(ty_span, "consider implementing `Clone` for this type");
span.push_span_label(expr.span, "you could clone this value");
err.span_note(
span,
format!("if `{ty}` implemented `Clone`, you could clone the value"),
);
let t = self.infcx.tcx.short_string(ty, err.long_ty_path());
err.span_note(span, format!("if `{t}` implemented `Clone`, you could clone the value"));
} else if let ty::Param(param) = ty.kind()
&& let Some(_clone_trait_def) = self.infcx.tcx.lang_items().clone_trait()
&& let generics = self.infcx.tcx.generics_of(self.mir_def_id())
Expand Down Expand Up @@ -1302,10 +1302,8 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
"consider constraining this type parameter with `Clone`",
);
span.push_span_label(expr.span, "you could clone this value");
err.span_help(
span,
format!("if `{ty}` implemented `Clone`, you could clone the value"),
);
let t = self.infcx.tcx.short_string(ty, err.long_ty_path());
err.span_help(span, format!("if `{t}` implemented `Clone`, you could clone the value"));
}
}

Expand Down Expand Up @@ -1951,11 +1949,13 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
&& let None =
self.infcx.type_implements_trait_shallow(clone, inner, self.infcx.param_env)
{
let rcvr_ty = self.infcx.tcx.short_string(rcvr_ty, err.long_ty_path());
let inner_str = self.infcx.tcx.short_string(inner, err.long_ty_path());
err.span_label(
span,
format!(
"this call doesn't do anything, the result is still `{rcvr_ty}` \
because `{inner}` doesn't implement `Clone`",
"this call doesn't do anything, the result is still `{rcvr_ty}` because \
`{inner_str}` doesn't implement `Clone`",
),
);
types_to_constrain.insert(inner);
Expand Down Expand Up @@ -3067,6 +3067,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
let borrow_span = borrow_spans.var_or_use();

let mut err = self.cannot_borrow_across_destructor(borrow_span);
let dropped_ty = self.infcx.tcx.short_string(dropped_ty, err.long_ty_path());

let what_was_dropped = match self.describe_place(place.as_ref()) {
Some(name) => format!("`{name}`"),
Expand Down Expand Up @@ -3245,6 +3246,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
&& path.ident.name == sym::iter
&& let Some(ty) = expr_ty
{
let ty = self.infcx.tcx.short_string(ty, err.long_ty_path());
err.span_suggestion_verbose(
path.ident.span,
format!(
Expand Down Expand Up @@ -3799,7 +3801,8 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
Some(self.infcx.tcx.fn_arg_names(method_did)[0]),
)
{
err.note(format!("borrow occurs due to deref coercion to `{deref_target_ty}`"));
let ty = self.infcx.tcx.short_string(deref_target_ty, err.long_ty_path());
err.note(format!("borrow occurs due to deref coercion to `{ty}`"));
if let Some(deref_target_span) = deref_target_span {
err.span_note(deref_target_span, "deref defined here");
}
Expand Down
7 changes: 5 additions & 2 deletions compiler/rustc_hir_typeck/src/callee.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -675,13 +675,15 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
}

let callee_ty = self.resolve_vars_if_possible(callee_ty);
let mut path = None;
let mut err = self.dcx().create_err(errors::InvalidCallee {
span: callee_expr.span,
ty: match &unit_variant {
Some((_, kind, path)) => format!("{kind} `{path}`"),
None => format!("`{callee_ty}`"),
None => format!("`{}`", self.tcx.short_string(callee_ty, &mut path)),
},
});
*err.long_ty_path() = path;
if callee_ty.references_error() {
err.downgrade_to_delayed_bug();
}
Expand Down Expand Up @@ -764,6 +766,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
DefIdOrName::DefId(def_id) => self.tcx.def_descr(def_id),
DefIdOrName::Name(name) => name,
};
let output_ty = self.tcx.short_string(output_ty, err.long_ty_path());
err.span_label(
callee_expr.span,
format!("this {descr} returns an unsized value `{output_ty}`, so it cannot be called")
Expand All @@ -779,7 +782,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
}

if let Some(span) = self.tcx.hir().res_span(def) {
let callee_ty = callee_ty.to_string();
let callee_ty = self.tcx.short_string(callee_ty, err.long_ty_path());
let label = match (unit_variant, inner_callee_path) {
(Some((_, kind, path)), _) => Some(format!("{kind} `{path}` defined here")),
(_, Some(hir::QPath::Resolved(_, path))) => self
Expand Down
82 changes: 47 additions & 35 deletions compiler/rustc_hir_typeck/src/cast.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,15 +196,18 @@ fn make_invalid_casting_error<'a, 'tcx>(
cast_ty: Ty<'tcx>,
fcx: &FnCtxt<'a, 'tcx>,
) -> Diag<'a> {
type_error_struct!(
let mut path = None;
let expr = fcx.tcx.short_string(expr_ty, &mut path);
let cast = fcx.tcx.short_string(cast_ty, &mut path);
let mut err = type_error_struct!(
fcx.dcx(),
span,
expr_ty,
E0606,
"casting `{}` as `{}` is invalid",
fcx.ty_to_string(expr_ty),
fcx.ty_to_string(cast_ty)
)
"casting `{expr}` as `{cast}` is invalid",
);
*err.long_ty_path() = path;
err
}

/// If a cast from `from_ty` to `to_ty` is valid, returns a `Some` containing the kind
Expand Down Expand Up @@ -259,6 +262,7 @@ impl<'a, 'tcx> CastCheck<'tcx> {
}

fn report_cast_error(&self, fcx: &FnCtxt<'a, 'tcx>, e: CastError<'tcx>) {
let mut path = None;
match e {
CastError::ErrorGuaranteed(_) => {
// an error has already been reported
Expand Down Expand Up @@ -369,27 +373,26 @@ impl<'a, 'tcx> CastCheck<'tcx> {
fcx.dcx().emit_err(errors::CannotCastToBool { span: self.span, expr_ty, help });
}
CastError::CastToChar => {
let expr = fcx.tcx.short_string(self.expr_ty, &mut path);
let mut err = type_error_struct!(
fcx.dcx(),
self.span,
self.expr_ty,
E0604,
"only `u8` can be cast as `char`, not `{}`",
self.expr_ty
"only `u8` can be cast as `char`, not `{expr}`",
);
*err.long_ty_path() = path;
err.span_label(self.span, "invalid cast");
if self.expr_ty.is_numeric() {
if self.expr_ty == fcx.tcx.types.u32 {
match fcx.tcx.sess.source_map().span_to_snippet(self.expr.span) {
Ok(snippet) => err.span_suggestion(
self.span,
"try `char::from_u32` instead",
format!("char::from_u32({snippet})"),
Applicability::MachineApplicable,
),

Err(_) => err.span_help(self.span, "try `char::from_u32` instead"),
};
err.multipart_suggestion_verbose(
"try `char::from_u32` instead",
vec![
(self.span.shrink_to_lo(), "char::from_u32(".to_string()),
(self.span.with_lo(self.expr.span.hi()), ")".to_string()),
],
Applicability::MachineApplicable,
);
} else if self.expr_ty == fcx.tcx.types.i8 {
err.span_help(self.span, "try casting from `u8` instead");
} else {
Expand All @@ -399,15 +402,16 @@ impl<'a, 'tcx> CastCheck<'tcx> {
err.emit();
}
CastError::NonScalar => {
let expr = fcx.tcx.short_string(self.expr_ty, &mut path);
let cast = fcx.tcx.short_string(self.cast_ty, &mut path);
let mut err = type_error_struct!(
fcx.dcx(),
self.span,
self.expr_ty,
E0605,
"non-primitive cast: `{}` as `{}`",
self.expr_ty,
fcx.ty_to_string(self.cast_ty)
"non-primitive cast: `{expr}` as `{cast}`",
);
*err.long_ty_path() = path;
let mut sugg = None;
let mut sugg_mutref = false;
if let ty::Ref(reg, cast_ty, mutbl) = *self.cast_ty.kind() {
Expand Down Expand Up @@ -548,28 +552,35 @@ impl<'a, 'tcx> CastCheck<'tcx> {
err.emit();
}
CastError::SizedUnsizedCast => {
fcx.dcx().emit_err(errors::CastThinPointerToWidePointer {
let expr_ty = fcx.tcx.short_string(self.expr_ty, &mut path);
let cast_ty = fcx.tcx.short_string(self.cast_ty, &mut path);
let mut err = fcx.dcx().create_err(errors::CastThinPointerToWidePointer {
span: self.span,
expr_ty: self.expr_ty,
cast_ty: fcx.ty_to_string(self.cast_ty),
expr_ty,
cast_ty,
teach: fcx.tcx.sess.teach(E0607),
});
*err.long_ty_path() = path;
err.emit();
}
CastError::IntToWideCast(known_metadata) => {
let expr_if_nightly = fcx.tcx.sess.is_nightly_build().then_some(self.expr_span);
let cast_ty = fcx.resolve_vars_if_possible(self.cast_ty);
let expr_ty = fcx.ty_to_string(self.expr_ty);
let expr_ty = fcx.tcx.short_string(self.expr_ty, &mut path);
let cast_ty =
fcx.tcx.short_string(fcx.resolve_vars_if_possible(self.cast_ty), &mut path);
let metadata = known_metadata.unwrap_or("type-specific metadata");
let known_wide = known_metadata.is_some();
let span = self.cast_span;
fcx.dcx().emit_err(errors::IntToWide {
let mut err = fcx.dcx().create_err(errors::IntToWide {
span,
metadata,
expr_ty,
cast_ty,
expr_if_nightly,
known_wide,
});
*err.long_ty_path() = path;
err.emit();
}
CastError::UnknownCastPtrKind | CastError::UnknownExprPtrKind => {
let unknown_cast_to = match e {
Expand Down Expand Up @@ -605,16 +616,18 @@ impl<'a, 'tcx> CastCheck<'tcx> {
return err;
}

let tstr = fcx.ty_to_string(self.cast_ty);
let mut path = None;
let tstr = fcx.tcx.short_string(self.cast_ty, &mut path);
let expr_ty = fcx.tcx.short_string(fcx.resolve_vars_if_possible(self.expr_ty), &mut path);

let mut err = type_error_struct!(
fcx.dcx(),
self.span,
self.expr_ty,
E0620,
"cast to unsized type: `{}` as `{}`",
fcx.resolve_vars_if_possible(self.expr_ty),
tstr
"cast to unsized type: `{expr_ty}` as `{tstr}`",
);
*err.long_ty_path() = path;
match self.expr_ty.kind() {
ty::Ref(_, _, mt) => {
let mtstr = mt.prefix_str();
Expand Down Expand Up @@ -1164,18 +1177,17 @@ impl<'a, 'tcx> CastCheck<'tcx> {
if let Some((deref_ty, _)) = derefed {
// Give a note about what the expr derefs to.
if deref_ty != self.expr_ty.peel_refs() {
err.subdiagnostic(errors::DerefImplsIsEmpty {
span: self.expr_span,
deref_ty: fcx.ty_to_string(deref_ty),
});
let deref_ty = fcx.tcx.short_string(deref_ty, err.long_ty_path());
err.subdiagnostic(errors::DerefImplsIsEmpty { span: self.expr_span, deref_ty });
}

let expr_ty = fcx.tcx.short_string(self.expr_ty, err.long_ty_path());
// Create a multipart suggestion: add `!` and `.is_empty()` in
// place of the cast.
err.subdiagnostic(errors::UseIsEmpty {
lo: self.expr_span.shrink_to_lo(),
hi: self.span.with_lo(self.expr_span.hi()),
expr_ty: fcx.ty_to_string(self.expr_ty),
expr_ty,
});
}
}
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_hir_typeck/src/errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,7 @@ pub(crate) struct IntToWide<'tcx> {
pub span: Span,
pub metadata: &'tcx str,
pub expr_ty: String,
pub cast_ty: Ty<'tcx>,
pub cast_ty: String,
#[label(hir_typeck_int_to_fat_label_nightly)]
pub expr_if_nightly: Option<Span>,
pub known_wide: bool,
Expand Down Expand Up @@ -822,10 +822,10 @@ pub(crate) struct ReplaceWithName {

#[derive(Diagnostic)]
#[diag(hir_typeck_cast_thin_pointer_to_wide_pointer, code = E0607)]
pub(crate) struct CastThinPointerToWidePointer<'tcx> {
pub(crate) struct CastThinPointerToWidePointer {
#[primary_span]
pub span: Span,
pub expr_ty: Ty<'tcx>,
pub expr_ty: String,
pub cast_ty: String,
#[note(hir_typeck_teach_help)]
pub(crate) teach: bool,
Expand Down
10 changes: 8 additions & 2 deletions compiler/rustc_hir_typeck/src/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -606,13 +606,16 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
if let Some(ty) = self.lookup_derefing(expr, oprnd, oprnd_t) {
oprnd_t = ty;
} else {
let mut path = None;
let ty = self.tcx.short_string(oprnd_t, &mut path);
let mut err = type_error_struct!(
self.dcx(),
expr.span,
oprnd_t,
E0614,
"type `{oprnd_t}` cannot be dereferenced",
"type `{ty}` cannot be dereferenced",
);
*err.long_ty_path() = path;
let sp = tcx.sess.source_map().start_point(expr.span).with_parent(None);
if let Some(sp) =
tcx.sess.psess.ambiguous_block_expr_parse.borrow().get(&sp)
Expand Down Expand Up @@ -3286,13 +3289,16 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
let span = field.span;
debug!("no_such_field_err(span: {:?}, field: {:?}, expr_t: {:?})", span, field, expr_t);

let mut path = None;
let ty = self.tcx.short_string(expr_t, &mut path);
let mut err = type_error_struct!(
self.dcx(),
span,
expr_t,
E0609,
"no field `{field}` on type `{expr_t}`",
"no field `{field}` on type `{ty}`",
);
*err.long_ty_path() = path;

// try to add a suggestion in case the field is a nested field of a field of the Adt
let mod_id = self.tcx.parent_module(id).to_def_id();
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_hir_typeck/src/gather_locals.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ impl<'a, 'tcx> GatherLocalsVisitor<'a, 'tcx> {
debug!(
"local variable {:?} is assigned type {}",
decl.pat,
self.fcx.ty_to_string(*self.fcx.locals.borrow().get(&decl.hir_id).unwrap())
*self.fcx.locals.borrow().get(&decl.hir_id).unwrap()
);
}
}
Expand Down Expand Up @@ -174,7 +174,7 @@ impl<'a, 'tcx> Visitor<'tcx> for GatherLocalsVisitor<'a, 'tcx> {
debug!(
"pattern binding {} is assigned to {} with type {:?}",
ident,
self.fcx.ty_to_string(*self.fcx.locals.borrow().get(&p.hir_id).unwrap()),
*self.fcx.locals.borrow().get(&p.hir_id).unwrap(),
var_ty
);
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_hir_typeck/src/method/probe.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1555,7 +1555,7 @@ impl<'a, 'tcx> ProbeContext<'a, 'tcx> {
pick_diag_hints: &mut PickDiagHints<'_, 'tcx>,
pick_constraints: Option<&PickConstraintsForShadowed>,
) -> Option<PickResult<'tcx>> {
debug!("pick_method(self_ty={})", self.ty_to_string(self_ty));
debug!("pick_method(self_ty={})", self_ty);

for (kind, candidates) in
[("inherent", &self.inherent_candidates), ("extension", &self.extension_candidates)]
Expand Down
Loading
Loading