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.

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

Pretty print Fn<(..., ...)> trait refs with parentheses (almost) always #118268

Merged
merged 2 commits into from
Dec 6, 2023
Merged
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
5 changes: 1 addition & 4 deletions compiler/rustc_hir_analysis/src/astconv/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1182,10 +1182,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
if let Some(bound_span) = bound_span {
err.span_label(
bound_span,
format!(
"ambiguous `{assoc_name}` from `{}`",
bound.print_only_trait_path(),
),
format!("ambiguous `{assoc_name}` from `{}`", bound.print_trait_sugared(),),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
format!("ambiguous `{assoc_name}` from `{}`", bound.print_trait_sugared(),),
format!("ambiguous `{assoc_name}` from `{}`", bound.print_trait_sugared()),

Copy link
Member Author

Choose a reason for hiding this comment

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

We have a lot of these :(

I ctrl+f'd for ,); and ,), yesterday and found like >50 instances.

);
if let Some(constraint) = &is_equality {
where_bounds.push(format!(
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_hir_analysis/src/astconv/object_safety.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
trait here instead: `trait NewTrait: {} {{}}`",
regular_traits
.iter()
// FIXME: This should `print_sugared`, but also needs to integrate projection bounds...
.map(|t| t.trait_ref().print_only_trait_path().to_string())
.collect::<Vec<_>>()
.join(" + "),
Expand Down
8 changes: 4 additions & 4 deletions compiler/rustc_hir_analysis/src/coherence/unsafety.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ pub(super) fn check_item(tcx: TyCtxt<'_>, def_id: LocalDefId) {
tcx.def_span(def_id),
E0199,
"implementing the trait `{}` is not unsafe",
trait_ref.print_only_trait_path()
trait_ref.print_trait_sugared()
)
.span_suggestion_verbose(
item.span.with_hi(item.span.lo() + rustc_span::BytePos(7)),
Expand All @@ -40,13 +40,13 @@ pub(super) fn check_item(tcx: TyCtxt<'_>, def_id: LocalDefId) {
tcx.def_span(def_id),
E0200,
"the trait `{}` requires an `unsafe impl` declaration",
trait_ref.print_only_trait_path()
trait_ref.print_trait_sugared()
)
.note(format!(
"the trait `{}` enforces invariants that the compiler can't check. \
Review the trait documentation and make sure this implementation \
upholds those invariants before adding the `unsafe` keyword",
trait_ref.print_only_trait_path()
trait_ref.print_trait_sugared()
))
.span_suggestion_verbose(
item.span.shrink_to_lo(),
Expand All @@ -69,7 +69,7 @@ pub(super) fn check_item(tcx: TyCtxt<'_>, def_id: LocalDefId) {
"the trait `{}` enforces invariants that the compiler can't check. \
Review the trait documentation and make sure this implementation \
upholds those invariants before adding the `unsafe` keyword",
trait_ref.print_only_trait_path()
trait_ref.print_trait_sugared()
))
.span_suggestion_verbose(
item.span.shrink_to_lo(),
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_hir_typeck/src/method/suggest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2288,7 +2288,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
ty::Adt(def, _) if def.did().is_local() => {
spans.push_span_label(
self.tcx.def_span(def.did()),
format!("must implement `{}`", pred.trait_ref.print_only_trait_path()),
format!("must implement `{}`", pred.trait_ref.print_trait_sugared()),
);
}
_ => {}
Expand All @@ -2299,7 +2299,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
let msg = if preds.len() == 1 {
format!(
"an implementation of `{}` might be missing for `{}`",
preds[0].trait_ref.print_only_trait_path(),
preds[0].trait_ref.print_trait_sugared(),
preds[0].self_ty()
)
} else {
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_infer/src/infer/error_reporting/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2219,8 +2219,8 @@ impl<'tcx> TypeErrCtxt<'_, 'tcx> {
infer::ExistentialProjection(exp_found) => self.expected_found_str(exp_found),
infer::PolyTraitRefs(exp_found) => {
let pretty_exp_found = ty::error::ExpectedFound {
expected: exp_found.expected.print_only_trait_path(),
found: exp_found.found.print_only_trait_path(),
expected: exp_found.expected.print_trait_sugared(),
found: exp_found.found.print_trait_sugared(),
};
match self.expected_found_str(pretty_exp_found) {
Some((expected, found, _, _)) if expected == found => {
Expand Down
46 changes: 45 additions & 1 deletion compiler/rustc_middle/src/ty/print/pretty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2640,6 +2640,23 @@ impl<'tcx> fmt::Debug for TraitRefPrintOnlyTraitPath<'tcx> {
}
}

/// Wrapper type for `ty::TraitRef` which opts-in to pretty printing only
/// the trait path, and additionally tries to "sugar" `Fn(...)` trait bounds.
#[derive(Copy, Clone, TypeFoldable, TypeVisitable, Lift)]
pub struct TraitRefPrintSugared<'tcx>(ty::TraitRef<'tcx>);

impl<'tcx> rustc_errors::IntoDiagnosticArg for TraitRefPrintSugared<'tcx> {
fn into_diagnostic_arg(self) -> rustc_errors::DiagnosticArgValue<'static> {
self.to_string().into_diagnostic_arg()
}
}

impl<'tcx> fmt::Debug for TraitRefPrintSugared<'tcx> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
fmt::Display::fmt(self, f)
}
}

/// Wrapper type for `ty::TraitRef` which opts-in to pretty printing only
/// the trait name. That is, it will print `Trait` instead of
/// `<T as Trait<U>>`.
Expand All @@ -2657,6 +2674,10 @@ impl<'tcx> ty::TraitRef<'tcx> {
TraitRefPrintOnlyTraitPath(self)
}

pub fn print_trait_sugared(self) -> TraitRefPrintSugared<'tcx> {
TraitRefPrintSugared(self)
}

pub fn print_only_trait_name(self) -> TraitRefPrintOnlyTraitName<'tcx> {
TraitRefPrintOnlyTraitName(self)
}
Expand All @@ -2666,6 +2687,10 @@ impl<'tcx> ty::Binder<'tcx, ty::TraitRef<'tcx>> {
pub fn print_only_trait_path(self) -> ty::Binder<'tcx, TraitRefPrintOnlyTraitPath<'tcx>> {
self.map_bound(|tr| tr.print_only_trait_path())
}

pub fn print_trait_sugared(self) -> ty::Binder<'tcx, TraitRefPrintSugared<'tcx>> {
self.map_bound(|tr| tr.print_trait_sugared())
}
estebank marked this conversation as resolved.
Show resolved Hide resolved
}

#[derive(Copy, Clone, TypeFoldable, TypeVisitable, Lift)]
Expand Down Expand Up @@ -2745,6 +2770,7 @@ forward_display_to_print! {
ty::PolyExistentialTraitRef<'tcx>,
ty::Binder<'tcx, ty::TraitRef<'tcx>>,
ty::Binder<'tcx, TraitRefPrintOnlyTraitPath<'tcx>>,
ty::Binder<'tcx, TraitRefPrintSugared<'tcx>>,
ty::Binder<'tcx, ty::FnSig<'tcx>>,
ty::Binder<'tcx, ty::TraitPredicate<'tcx>>,
ty::Binder<'tcx, TraitPredPrintModifiersAndPath<'tcx>>,
Expand Down Expand Up @@ -2844,6 +2870,24 @@ define_print_and_forward_display! {
p!(print_def_path(self.0.def_id, self.0.args));
}

TraitRefPrintSugared<'tcx> {
if !with_no_queries()
&& let Some(kind) = cx.tcx().fn_trait_kind_from_def_id(self.0.def_id)
&& let ty::Tuple(args) = self.0.args.type_at(1).kind()
{
p!(write("{}", kind.as_str()), "(");
estebank marked this conversation as resolved.
Show resolved Hide resolved
for (i, arg) in args.iter().enumerate() {
if i > 0 {
p!(", ");
}
p!(print(arg));
}
p!(")");
} else {
p!(print_def_path(self.0.def_id, self.0.args));
}
}
Comment on lines +2873 to +2889
Copy link
Contributor

Choose a reason for hiding this comment

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

This will never print the Output, is that ok?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's no Output that we have access to, unfortunately :/

Copy link
Contributor

@estebank estebank Nov 28, 2023

Choose a reason for hiding this comment

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

Didn't we hack something together for Future::Output somewhere?

Edit: I think I was thinking of pretty_print_opaque_impl_type.

Copy link
Member Author

@compiler-errors compiler-errors Nov 28, 2023

Choose a reason for hiding this comment

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

I guess I could eagerly look into the self type and see if it's something with a signature, but it's not always going to be possible. Let me see.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you manage it, great. If not, r=me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, when we extract this information from the self type, it's often not right. For example, when we have:

fn test<F: Fn(u32) -> u32>() {}

test(|_: i32| -> i32 { 0 });

Extracting the return type from the self type will give us an error message mentioning something like {closure}: Fn(u32) -> i32, not Fn(u32) -> u32.


TraitRefPrintOnlyTraitName<'tcx> {
p!(print_def_path(self.0.def_id, &[]));
}
Expand Down Expand Up @@ -2892,7 +2936,7 @@ define_print_and_forward_display! {
if let ty::ImplPolarity::Negative = self.polarity {
p!("!");
}
p!(print(self.trait_ref.print_only_trait_path()))
p!(print(self.trait_ref.print_trait_sugared()))
}

ty::ProjectionPredicate<'tcx> {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_trait_selection/src/traits/coherence.rs
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ fn overlap<'tcx>(
let trait_ref = infcx.resolve_vars_if_possible(trait_ref);
format!(
"of `{}` for `{}`",
trait_ref.print_only_trait_path(),
trait_ref.print_trait_sugared(),
trait_ref.self_ty()
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -184,14 +184,7 @@ impl<'tcx> TypeErrCtxtExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
flags.push((sym::cause, Some("MainFunctionType".to_string())));
}

if let Some(kind) = self.tcx.fn_trait_kind_from_def_id(trait_ref.def_id)
&& let ty::Tuple(args) = trait_ref.args.type_at(1).kind()
{
let args = args.iter().map(|ty| ty.to_string()).collect::<Vec<_>>().join(", ");
flags.push((sym::Trait, Some(format!("{}({args})", kind.as_str()))));
} else {
flags.push((sym::Trait, Some(trait_ref.print_only_trait_path().to_string())));
}
flags.push((sym::Trait, Some(trait_ref.print_trait_sugared().to_string())));
estebank marked this conversation as resolved.
Show resolved Hide resolved

// Add all types without trimmed paths or visible paths, ensuring they end up with
// their "canonical" def path.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -622,7 +622,7 @@ impl<'tcx> TypeErrCtxtExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
span.shrink_to_hi(),
format!(
"the trait `{}` is implemented for fn pointer `{}`, try casting using `as`",
cand.print_only_trait_path(),
cand.print_trait_sugared(),
cand.self_ty(),
),
format!(" as {}", cand.self_ty()),
Expand Down Expand Up @@ -1785,7 +1785,7 @@ impl<'tcx> InferCtxtPrivExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
ct_op: |ct| ct.normalize(self.tcx, ty::ParamEnv::empty()),
});
err.highlighted_help(vec![
(format!("the trait `{}` ", cand.print_only_trait_path()), Style::NoStyle),
(format!("the trait `{}` ", cand.print_trait_sugared()), Style::NoStyle),
("is".to_string(), Style::Highlight),
(" implemented for `".to_string(), Style::NoStyle),
(cand.self_ty().to_string(), Style::Highlight),
Expand Down Expand Up @@ -1821,7 +1821,7 @@ impl<'tcx> InferCtxtPrivExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
_ => (" implemented for `", ""),
};
err.highlighted_help(vec![
(format!("the trait `{}` ", cand.print_only_trait_path()), Style::NoStyle),
(format!("the trait `{}` ", cand.print_trait_sugared()), Style::NoStyle),
("is".to_string(), Style::Highlight),
(desc.to_string(), Style::NoStyle),
(cand.self_ty().to_string(), Style::Highlight),
Expand Down Expand Up @@ -1854,7 +1854,7 @@ impl<'tcx> InferCtxtPrivExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
let end = if candidates.len() <= 9 { candidates.len() } else { 8 };
err.help(format!(
"the following {other}types implement trait `{}`:{}{}",
trait_ref.print_only_trait_path(),
trait_ref.print_trait_sugared(),
candidates[..end].join(""),
if candidates.len() > 9 {
format!("\nand {} others", candidates.len() - 8)
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_trait_selection/src/traits/select/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ impl<'tcx> IntercrateAmbiguityCause<'tcx> {
IntercrateAmbiguityCause::DownstreamCrate { trait_ref, self_ty } => {
format!(
"downstream crates may implement trait `{trait_desc}`{self_desc}",
trait_desc = trait_ref.print_only_trait_path(),
trait_desc = trait_ref.print_trait_sugared(),
self_desc = if let Some(self_ty) = self_ty {
format!(" for type `{self_ty}`")
} else {
Expand All @@ -90,7 +90,7 @@ impl<'tcx> IntercrateAmbiguityCause<'tcx> {
format!(
"upstream crates may add a new impl of trait `{trait_desc}`{self_desc} \
in future versions",
trait_desc = trait_ref.print_only_trait_path(),
trait_desc = trait_ref.print_trait_sugared(),
self_desc = if let Some(self_ty) = self_ty {
format!(" for type `{self_ty}`")
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -412,7 +412,7 @@ fn report_conflicting_impls<'tcx>(
let msg = DelayDm(|| {
format!(
"conflicting implementations of trait `{}`{}{}",
overlap.trait_ref.print_only_trait_path(),
overlap.trait_ref.print_trait_sugared(),
overlap.self_ty.map_or_else(String::new, |ty| format!(" for type `{ty}`")),
match used_to_be_allowed {
Some(FutureCompatOverlapErrorKind::Issue33140) => ": (E0119)",
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/error-codes/E0401.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ error[E0283]: type annotations needed
LL | bfnr(x);
| ^^^^ cannot infer type of the type parameter `W` declared on the function `bfnr`
|
= note: multiple `impl`s satisfying `_: Fn<()>` found in the following crates: `alloc`, `core`:
= note: multiple `impl`s satisfying `_: Fn()` found in the following crates: `alloc`, `core`:
- impl<A, F> Fn<A> for &F
where A: Tuple, F: Fn<A>, F: ?Sized;
- impl<Args, F, A> Fn<Args> for Box<F, A>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ error[E0308]: mismatched types
LL | foo(bar, "string", |s| s.len() == 5);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ one type is more general than the other
|
= note: expected trait `for<'a, 'b> FnOnce<(&'a &'b str,)>`
found trait `for<'a> FnOnce<(&'a &str,)>`
= note: expected trait `for<'a, 'b> FnOnce(&'a &'b str)`
found trait `for<'a> FnOnce(&'a &str)`
note: this closure does not fulfill the lifetime requirements
--> $DIR/issue-71955.rs:45:24
|
Expand All @@ -23,8 +23,8 @@ error[E0308]: mismatched types
LL | foo(bar, "string", |s| s.len() == 5);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ one type is more general than the other
|
= note: expected trait `for<'a, 'b> FnOnce<(&'a &'b str,)>`
found trait `for<'a> FnOnce<(&'a &str,)>`
= note: expected trait `for<'a, 'b> FnOnce(&'a &'b str)`
found trait `for<'a> FnOnce(&'a &str)`
note: this closure does not fulfill the lifetime requirements
--> $DIR/issue-71955.rs:45:24
|
Expand All @@ -42,8 +42,8 @@ error[E0308]: mismatched types
LL | foo(baz, "string", |s| s.0.len() == 5);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ one type is more general than the other
|
= note: expected trait `for<'a, 'b> FnOnce<(&'a Wrapper<'b>,)>`
found trait `for<'a> FnOnce<(&'a Wrapper<'_>,)>`
= note: expected trait `for<'a, 'b> FnOnce(&'a Wrapper<'b>)`
found trait `for<'a> FnOnce(&'a Wrapper<'_>)`
note: this closure does not fulfill the lifetime requirements
--> $DIR/issue-71955.rs:48:24
|
Expand All @@ -61,8 +61,8 @@ error[E0308]: mismatched types
LL | foo(baz, "string", |s| s.0.len() == 5);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ one type is more general than the other
|
= note: expected trait `for<'a, 'b> FnOnce<(&'a Wrapper<'b>,)>`
found trait `for<'a> FnOnce<(&'a Wrapper<'_>,)>`
= note: expected trait `for<'a, 'b> FnOnce(&'a Wrapper<'b>)`
found trait `for<'a> FnOnce(&'a Wrapper<'_>)`
note: this closure does not fulfill the lifetime requirements
--> $DIR/issue-71955.rs:48:24
|
Expand Down
16 changes: 8 additions & 8 deletions tests/ui/lifetimes/issue-105675.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ error[E0308]: mismatched types
LL | thing(f);
| ^^^^^^^^ one type is more general than the other
|
= note: expected trait `for<'a, 'b> FnOnce<(&'a u32, &'b u32, u32)>`
found trait `for<'a> FnOnce<(&u32, &'a u32, u32)>`
= note: expected trait `for<'a, 'b> FnOnce(&'a u32, &'b u32, u32)`
found trait `for<'a> FnOnce(&u32, &'a u32, u32)`
note: this closure does not fulfill the lifetime requirements
--> $DIR/issue-105675.rs:4:13
|
Expand All @@ -27,8 +27,8 @@ error[E0308]: mismatched types
LL | thing(f);
| ^^^^^^^^ one type is more general than the other
|
= note: expected trait `for<'a, 'b> FnOnce<(&'a u32, &'b u32, u32)>`
found trait `for<'a> FnOnce<(&u32, &'a u32, u32)>`
= note: expected trait `for<'a, 'b> FnOnce(&'a u32, &'b u32, u32)`
found trait `for<'a> FnOnce(&u32, &'a u32, u32)`
note: this closure does not fulfill the lifetime requirements
--> $DIR/issue-105675.rs:4:13
|
Expand All @@ -46,8 +46,8 @@ error[E0308]: mismatched types
LL | thing(f);
| ^^^^^^^^ one type is more general than the other
|
= note: expected trait `for<'a, 'b> FnOnce<(&'a u32, &'b u32, u32)>`
found trait `FnOnce<(&u32, &u32, u32)>`
= note: expected trait `for<'a, 'b> FnOnce(&'a u32, &'b u32, u32)`
found trait `FnOnce(&u32, &u32, u32)`
note: this closure does not fulfill the lifetime requirements
--> $DIR/issue-105675.rs:8:13
|
Expand All @@ -69,8 +69,8 @@ error[E0308]: mismatched types
LL | thing(f);
| ^^^^^^^^ one type is more general than the other
|
= note: expected trait `for<'a, 'b> FnOnce<(&'a u32, &'b u32, u32)>`
found trait `FnOnce<(&u32, &u32, u32)>`
= note: expected trait `for<'a, 'b> FnOnce(&'a u32, &'b u32, u32)`
found trait `FnOnce(&u32, &u32, u32)`
note: this closure does not fulfill the lifetime requirements
--> $DIR/issue-105675.rs:8:13
|
Expand Down
4 changes: 2 additions & 2 deletions tests/ui/lifetimes/issue-79187-2.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ error[E0308]: mismatched types
LL | take_foo(|a| a);
| ^^^^^^^^^^^^^^^ one type is more general than the other
|
= note: expected trait `for<'a> Fn<(&'a i32,)>`
found trait `Fn<(&i32,)>`
= note: expected trait `for<'a> Fn(&'a i32)`
found trait `Fn(&i32)`
note: this closure does not fulfill the lifetime requirements
--> $DIR/issue-79187-2.rs:8:14
|
Expand Down
4 changes: 2 additions & 2 deletions tests/ui/lifetimes/issue-79187.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ error[E0308]: mismatched types
LL | thing(f);
| ^^^^^^^^ one type is more general than the other
|
= note: expected trait `for<'a> FnOnce<(&'a u32,)>`
found trait `FnOnce<(&u32,)>`
= note: expected trait `for<'a> FnOnce(&'a u32)`
found trait `FnOnce(&u32)`
note: this closure does not fulfill the lifetime requirements
--> $DIR/issue-79187.rs:4:13
|
Expand Down
4 changes: 2 additions & 2 deletions tests/ui/lifetimes/lifetime-errors/issue_74400.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ error[E0308]: mismatched types
LL | f(data, identity)
| ^^^^^^^^^^^^^^^^^ one type is more general than the other
|
= note: expected trait `for<'a> Fn<(&'a T,)>`
found trait `Fn<(&T,)>`
= note: expected trait `for<'a> Fn(&'a T)`
found trait `Fn(&T)`
note: the lifetime requirement is introduced here
--> $DIR/issue_74400.rs:8:34
|
Expand Down
Loading
Loading