So it's been like a couple months since i wrote the last post about how i tackled an e-needs-test issue
This time, like i promised, i wanna go through something a bit more user-facing together with you - diagnostics
A diagnostic is that nice, colored explanation the compiler shows you when something goes wrong. Instead of just shouting "error" and walking away, the compiler calmly points at what's off and usually nudges you toward a fix
For example
error[E0596]: cannot borrow `*some_string` as mutable, as it is behind a `&` reference --> src/main.rs:8:5 | 8 | some_string.push_str(", world"); | ^^^^^^^^^^^ `some_string` is a `&` reference, so it cannot be borrowed as mutable | help: consider changing this to be a mutable reference | 7 | fn change(some_string: &mut String) { | +++ For more information about this error, try `rustc --explain E0596`. error: could not compile `f` (bin "f") due to 1 previous error
Beautiful, isn't it? Well, the thing is they're not as perfect as they look at first glance. If we open the issues we'll see a huge pile of issues on this topic, and even though the rust team puts a lot of effort into making them as user-friendly as possible (you know, those stories where you get 20kb of unreadable output from gcc), teachable and simple - i don't think it'll ever be possible to make them 100% perfect. But that doesn't mean we shouldn't improve them at all
Picking an Issue
So let's grab an issue and try to solve one
I'll say right away, the difficulty in this area ranges from "change one word" to "rewrite half of the borrow checker". But on average it's pretty gentle and somewhere in between, so solving them is interesting and fun (mostly)
Bear in mind that your experience may differ significantly from mine, because diagnostics permeate the entire compiler, from the lexer right through to MIR; there's so much going on that it's impossible to take everything into account: parser recovery, name resolution, type checking, trait resolution, borrow checking; and that's just a small part of it ><
But you gotta pick the issue wisely, for the same reason as both warnings above
This one caught my eye
Shortly, right now for this code
fn take(_: impl Iterator<0>) {}
The compiler gives this error
error[E0107]: trait takes 0 generic arguments but 1 generic argument was supplied --> src/lib.rs:1:17 | 1 | fn take(_: impl Iterator<0>) {} | ^^^^^^^^ expected 0 generic arguments | note: trait defined here, with 0 generic parameters --> /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/iter/traits/iterator.rs:40:11 | 40 | pub trait Iterator { | ^^^^^^^^ help: replace the generic bound with the associated type | 1 | fn take(_: impl Iterator<Item = 0>) {} | ++++++
Which is wrong and doesn't make any sense
And this code (it's a bit outdated)
MGCA (min_generic_const_args) is a nightly feature that lets you use const values directly as generic arguments - Trait<0> instead of Trait<K = 0>. That's what this whole second example lives in
#![feature(min_generic_const_args)]
trait Trait { #[type_const] const K: i32; }
fn take(_: impl Trait<0>) {}
Spits out this error
error[E0107]: trait takes 0 generic arguments but 1 generic argument was supplied --> src/lib.rs:3:17 | 3 | fn take(_: impl Trait<0>) {} | ^^^^^--- help: remove the unnecessary generics | | | expected 0 generic arguments | note: trait defined here, with 0 generic parameters --> src/lib.rs:2:7 | 2 | trait Trait { #[type_const] const K: i32; } | ^^^^^
Even though it should suggest something like
impl Trait<K = 0>
Starting to Dig
First thing i make a clean branch
$ git checkout -b fix-mgca-assoc-sugg
And right away i kick off a compiler build
$ ./x b
So i don't have to do it later, plus I'll need to run the code above with the -Ztrack-diagnostics flag to find at least the rough place where the diagnostic gets created
While the compiler is building, one interesting detail about this issue - @fmease originally wanted to work on this problem but dropped his assignment
Ideally it's worth messaging him to ask what went wrong and why he decided not to fix it further - as one of the people who knows this area well, he'd give the right hints and help out
Reaching out to whoever was originally on an issue (or anyone who knows the area) is one of the best moves you can make. In the overwhelming majority of cases people respond super positively - maybe not right away, but positively
And so, 5 minutes later
Build completed successfully in 0:04:57
My compiler built (yeah I'm doing everything in real time, on one screen i have a text editor and on the other a code editor)
Let's run it on the file right away and see what's going on
gh-Kivooeo@dev-desktop-eu-1 ~/r/rust (fix-mgca-assoc-sugg) > ./build/aarch64-unknown-linux-gnu/stage1/bin/rustc -Ztrack-diagnostics /home/gh-Kivooeo/test_/src/main.rs --edition 2024 error[E0107]: trait takes 0 generic arguments but 1 generic argument was supplied --> /home/gh-Kivooeo/test_/src/main.rs:1:17 | 1 | fn take(_: impl Iterator<0>) {} | ^^^^^^^^ expected 0 generic arguments | = note: -Ztrack-diagnostics: created at compiler/rustc_hir_analysis/src/hir_ty_lowering/generics.rs:575:18 note: trait defined here, with 0 generic parameters --> library/core/src/iter/traits/iterator.rs:42:17 | 42 | pub const trait Iterator { | ^^^^^^^^ help: replace the generic bound with the associated type | 1 | fn take(_: impl Iterator<Item = 0>) {} | ++++++ error: aborting due to 1 previous error
Let's look into compiler/rustc_hir_analysis/src/hir_ty_lowering/generics.rs:575:18, hmm, at first glance this looks like exactly the function we need
/// Checks that the correct number of generic arguments have been provided.
/// This is used both for datatypes and function calls.
#[instrument(skip(cx, gen_pos), level = "debug")]
pub(crate) fn check_generic_arg_count(
cx: &dyn HirTyLowerer<'_>,
def_id: DefId,
seg: &hir::PathSegment<'_>,
gen_params: &ty::Generics,
gen_pos: GenericArgPosition,
has_self: bool,
) -> GenericArgCountResult
But looking at the logic, i don't see anything that could produce a suggestion like "add Item = 0"
Finding Where the Suggestion Is Born
So let's grep for the error text "replace the generic bound with the associated type". This grep found nothing except output in tests, so let's shorten it down to "replace the generic" and we immediately found what we need!
fn suggest_removing_args_or_generics(&self, err: &mut Diag<'_, impl EmissionGuarantee>)
And the specific check inside this function
if !self.is_in_trait_impl() {
let unused_generics = &self.gen_args.args[self.num_expected_type_or_const_args()..];
let suggestions = iter::zip(unused_generics, &unbound_types)
.map(|(potential, name)| {
(potential.span().shrink_to_lo(), format!("{name} = "))
})
.collect::<Vec<_>>();
if !suggestions.is_empty() {
err.multipart_suggestion(
format!(
"replace the generic bound{s} with the associated type{s}",
s = pluralize!(unbound_types.len())
),
suggestions,
Applicability::MaybeIncorrect,
);
}
}
Here you can clearly see it by the suggestions variable. Now we gotta think where to move next
We have an interesting call chain through num_expected_type_or_const_args which is used here, but it can't really help us much
I didn't give you the full context above. Here's what the whole check looks like
// If there is a single unbound associated type and a single excess generic param
// suggest replacing the generic param with the associated type bound
if provided_args_matches_unbound_traits && !unbound_types.is_empty() {
// Don't suggest if we're in a trait impl as
// that would result in invalid syntax (fixes #116464)
if !self.is_in_trait_impl() {
Why did i do that? I didn't look at the outer checks right away myself, and the comments give pretty nice hints
Let's look at how unbound_types is defined
let unbound_types = self.get_unbound_associated_types();
Now let's look at this method
fn get_unbound_associated_types(&self) -> Vec<String> {
if self.tcx.is_trait(self.def_id) { // ok that's true
let items: &AssocItems = self.tcx.associated_items(self.def_id); // get assoc item, nice
items
.in_definition_order() // fetch it in def. order
.filter(|item| { // filter by:
item.is_type() // only assoc types
&& !item.is_impl_trait_in_trait() // not trait
&& !self
.gen_args
.constraints
.iter()
.any(|constraint| constraint.ident.name == item.name())
//^ exclude associated types that are already bound in the generic args
})
.map(|item| self.tcx.item_ident(item.def_id).to_string()) // map it to string
.collect() // and collect
} else {
Vec::default() // otherwise just empty vec
}
}
Anyway it's all clear. Surprisingly this code is pretty readable on its own, so let's make a simple change and see what happens?
- item.is_type()
+ (item.is_type() || item.is_type_const())
After that i run this interesting command
$ ./x b --keep-stage 1 && ./build/aarch64-unknown-linux-gnu/stage1/bin/rustc -Ztrack-diagnostics /home/gh-Kivooeo/test_/src/main.rs --edition 2024
Use --keep-stage 1 if you don't wanna recompile std and core, despite the flags name it works exactly like that
So... and for some reason it doesn't work
error: cannot find attribute `type_const` in this scope --> /home/gh-Kivooeo/test_/src/main.rs:2:17 | 2 | trait Trait { #[type_const] const K: i32; } | ^^^^^^^^^^ | = note: -Ztrack-diagnostics: created at compiler/rustc_resolve/src/macros.rs:1071:46
Ah right, i forgot to mention the code examples are a bit outdated. Now you have to write type const, so the example becomes that
And voila ✨
error[E0107]: trait takes 0 generic arguments but 1 generic argument was supplied --> /home/gh-Kivooeo/test_/src/main.rs:3:17 | 3 | fn take(_: impl Trait<0>) {} | ^^^^^ expected 0 generic arguments | = note: -Ztrack-diagnostics: created at compiler/rustc_hir_analysis/src/hir_ty_lowering/generics.rs:575:18 note: trait defined here, with 0 generic parameters --> /home/gh-Kivooeo/test_/src/main.rs:2:7 | 2 | trait Trait { type const K: i32; } | ^^^^^ help: replace the generic bound with the associated type | 3 | fn take(_: impl Trait<K = 0>) {} | +++
The Actual Fix
Now the only thing left is to deal with the first case, where we get a really not-great diagnostic
For that i think we can go back to the zip logic, because it's the thing that adds this suggestion. We need to filter it somehow
Good news, this function is used only in this one specific place, so we can ruthlessly change its signature. I suggest this one
fn get_unbound_associated_types(&self) -> Vec<&AssocItem>
Not a string but the actual AssocItem, so we don't have to do (String, bool)
And we add a filter after the zip call, that checks the following - if we wanna suggest a const (like 0 for example) then we should check it's a type const, and if we wanna suggest a type then we should check it's a type
Literally just like this
.filter(|(potential, item)| match potential {
hir::GenericArg::Const(_) => item.is_type_const(),
hir::GenericArg::Type(_) => item.is_type(),
_ => false,
})
Annd... yeah, it works!
error[E0107]: trait takes 0 generic arguments but 1 generic argument was supplied --> /home/gh-Kivooeo/test_/src/main.rs:1:17 | 1 | fn take(_: impl Iterator<0>) {} | ^^^^^^^^ expected 0 generic arguments | = note: -Ztrack-diagnostics: created at compiler/rustc_hir_analysis/src/hir_ty_lowering/generics.rs:575:18 note: trait defined here, with 0 generic parameters --> library/core/src/iter/traits/iterator.rs:42:17 | 42 | pub const trait Iterator { | ^^^^^^^^
No more nonsense suggestion, just the error, exactly what we wanted
Running the Test Suite
Great, now let's run ./x test ui - I'm expecting a big number of regressions in our test suite
To my surprise only two tests. Let's look at their contents
First test: tests/ui/const-generics/issues/issue-87493.rs
Looks like this
pub trait MyTrait {
type Assoc;
}
pub fn foo<S, T>(_s: S, _t: T)
where
S: MyTrait,
T: MyTrait<Assoc == S::Assoc>,
//~^ ERROR: expected one of `,` or `>`, found `==`
//~| ERROR: trait takes 0 generic arguments but 1 generic argument was supplied
{
}
fn main() {}
The original error was this
error[E0107]: trait takes 0 generic arguments but 1 generic argument was supplied --> src/main.rs:8:8 | 8 | T: MyTrait<Assoc == S::Assoc>, | ^^^^^^^ expected 0 generic arguments | note: trait defined here, with 0 generic parameters --> src/main.rs:1:11 | 1 | pub trait MyTrait { | ^^^^^^^ help: replace the generic bound with the associated type | 8 | T: MyTrait<Assoc = Assoc == S::Assoc>, | +++++++
You know, just for fun i decided to try writing what the compiler suggests here, and you know what i got? You'll never guess...
help: replace the generic bound with the associated type | 8 | T: MyTrait<Assoc = Assoc = Assoc == S::Assoc>, | +++++++
I tried it. The compiler responded with an even more broken version of the same suggestion - Assoc = Assoc = Assoc == S::Assoc. A beginner could sit there applying this forever... After our fix this problem got fixed and now it (kinda) correctly suggests the fix
error: expected one of `,` or `>`, found `==` --> /home/gh-Kivooeo/test_/src/main.rs:8:22 | 8 | T: MyTrait<Assoc == S::Assoc>, | ^^ expected one of `,` or `>` | = note: -Ztrack-diagnostics: created at compiler/rustc_parse/src/parser/diagnostics.rs:2528:34 help: if you meant to use an associated type binding, replace `==` with `=` | 8 - T: MyTrait<Assoc == S::Assoc>, 8 + T: MyTrait<Assoc = S::Assoc>, |
At least it compiles if you do what the compiler asks :)
Now let's look at the second test
//! This test used to ICE: #121134
//! The issue is that we're trying to prove a projection, but there's
//! no bound for the projection's trait, and the projection has the wrong
//! kind of generic parameter (lifetime vs type).
//! When actually calling the function with those broken bounds, trying to
//! instantiate the bounds with inference vars would ICE.
#![feature(unboxed_closures)]
trait Output<'a> {
type Type;
}
struct Wrapper;
impl Wrapper {
fn do_something_wrapper<O, F>(&mut self, _: F)
where
F: for<'a> FnOnce(<F as Output<i32>>::Type),
//~^ ERROR: trait takes 0 generic arguments but 1 generic argument was supplied
{
}
}
fn main() {
let mut wrapper = Wrapper;
wrapper.do_something_wrapper(|value| ());
}
Anyway, some complicated code, but what's interesting, right now the compiler suggests the following - add Type =
help: replace the generic bound with the associated type | 18 | F: for<'a> FnOnce(<F as Output<Type = i32>>::Type), | ++++++
And after we add Type = i32, it suggests this
error[E0229]: associated item constraints are not allowed here --> src/main.rs:18:40 | 18 | F: for<'a> FnOnce(<F as Output<Type = i32>>::Type), | ^^^^^^^^^^ associated item constraint not allowed here | help: consider removing this associated item binding | 18 - F: for<'a> FnOnce(<F as Output<Type = i32>>::Type), 18 + F: for<'a> FnOnce(<F as Output<>>::Type),
What...? Yeah, this was kinda to the point about diagnostics not being perfect in the compiler. Anyway i ran around in circles for a pretty long time trying to do what the compiler suggests, and in the end it just didn't compile
Anyway, i think this isn't our problem anymore :)
The very fact that the compiler no longer suggests adding Type = here is already a win, and the other diagnostics fall out of scope of this fix
So with a calm soul we update the tests so they run with the new expected output. For the first one we just run with --bless
$ ./x test tests/ui/const-generics/issues/issue-87493.rs tests/ui/traits/generic_param_mismatch_in_unsatisfied_projection.rs --bless
And now we look
running 2 tests
..
test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 21417 filtered out; finished in 51.58ms
Perfect!
Wrapping Up
Now, the last thing left to do is write a new test and add @fmease's suggestion to change the wording of this error
format!(
- "replace the generic bound{s} with the associated type{s}",
+ "turn the generic argument{s} into an associated item binding{s}",
s = pluralize!(unbound_types.len())
)
Now let's add a new test, if you don't know how to do that i suggest you read the previous post about adding tests!
Here's what our test will look like
//! Regression test for <https://github.com/rust-lang/rust/issues/151602>.
#![feature(min_generic_const_args)]
trait Trait {
type const K: i32;
}
fn take(_: impl Trait<0>) {}
//~^ ERROR: trait takes 0 generic arguments but 1 generic argument was supplied [E0107]
fn main() {}
Since we changed the error wording, we'll need to run all the tests again and update the .stderr files for all of them
$ ./x test ui --bless
I think after that the only thing left is to open the pull request and assign @fmease to it (using r?)
What Happens Next?
While the tests are running, let's talk about this for a sec. I actually admit there might be some edge cases in this fix or the test, I'm not super familiar with the MGCA feature, even though i wrote the Ctor support and a couple other things for it. That was more thanks to the great mentorship and review from @BoxyUwU
Also a lot of useful info on this matter was said here
Well, it seems tests are compiled fine, except one test which requiers me to update the error:
fn trait_bound_generic<I: T<u8, u16>>(_i: I) {
//~^ ERROR trait takes 0 generic arguments
- //~| HELP replace the generic bounds with the associated types
+ //~| HELP turn the generic arguments into an associated item bindings
}
So it felt to me that fix is very ready for being published, let's run ./x test tidy to make very sure before opening a PR. Good, tidy seems fine about it
Build completed successfully in 0:00:41
Next step is to open a PR (done here), assign @fmease with r? and... well, that's basically it - now we wait :)
The Review Comes Back
So the wait wasn't that long actually, and @fmease left a few comments. And honestly this is the part i enjoy the most - when someone who knows the area way better than you looks at your code and points at the spots you were too eager to skim past
The first and the most important one was about my filter. Remember this part?
.filter(|(potential, item)| match potential {
hir::GenericArg::Const(_) => item.is_type_const(),
hir::GenericArg::Type(_) => item.is_type(),
_ => false,
})
Looks fine right? Well, the thing is i was zipping unused_generics with unbound_types by position, and only then filtering by kind. But the assoc items come in definition order, and the generic args come in the order the user wrote them - there's no reason these two line up. @fmease gave a really clean example
trait Trait { type A; type const B: usize; }
fn f<T: Trait<(), 0>>(); // my PR emits a suggestion
trait Trait { type const A: usize; type B; }
fn f<T: Trait<(), 0>>(); // my PR does *not* emit one
In the second case the zip produces pairs like [(TyAssocConst, Ty), (AssocTy, Const)] and neither pair survives the filter, so we just silently drop a perfectly good suggestion. Oops
The fix is to stop pairing by position entirely. Instead i split the unbound items into two lazy iterators by kind, and let each generic arg pull the next item of its own kind
let mut unbound_assoc_consts = unbound_assoc_items.iter().filter(|item| item.is_type_const());
let mut unbound_assoc_types = unbound_assoc_items.iter().filter(|item| item.is_type());
let suggestions = unused_generics
.iter()
.filter_map(|potential| {
let item = match potential {
hir::GenericArg::Const(_) => unbound_assoc_consts.next(),
hir::GenericArg::Type(_) => unbound_assoc_types.next(),
_ => None,
}?;
Some((
potential.span().shrink_to_lo(),
// FIXME: This doesn't account for generic associated items
format!("{} = ", self.tcx.item_ident(item.def_id)),
))
})
.collect::<Vec<_>>();
Each .next() advances its own cursor, so a type arg always grabs the next assoc type and a const the next assoc const, no matter how they're interleaved - and ? just bails (emitting nothing) if a queue runs dry instead of mispairing
The second comment was a tiny but nice one. The wording
"turn the generic argument{s} into an associated item binding{s}"
reads totally fine in singular, but the moment you have more than one it becomes "turn the generic arguments into an associated item bindings" which is, well, not english. So the an should only show up when there's exactly one. Quick fix
let s = pluralize!(suggestions.len());
let article = if suggestions.len() == 1 { "an " } else { "" };
err.multipart_suggestion(
format!("turn the generic argument{s} into {article}associated item binding{s}"),
suggestions,
Applicability::MaybeIncorrect,
);
Also notice i switched the pluralization from unbound_types.len() to suggestions.len() - now that not every excess arg necessarily produces a binding, the count should reflect what we actually suggest
Then there was the comment above the whole block, which was honestly just a bit outdated. It still said
// If there is a single unbound associated type and a single excess generic param
// suggest replacing the generic param with the associated type bound
but the code handles multiple of them now, and assoc consts too, not only types. @fmease suggested generalizing it
// If there is an identical amount of unbound associated items and excess generic args
// suggest turning the generic args into associated item bindings.
with a funny little caveat that even "identical amount" isn't strictly true anymore after my kind-matching change (equal counts no longer guarantee a full set of bindings, since a type only pairs with a type and a const with a const). But it's still way better than the old wording, so in it goes
And the last one - @fmease asked me to drop a // FIXME: This doesn't account for generic associated items right above the format!("{} = ", …), since that's the bit that can't express a generic assoc item like type Assoc<T>; (we'd produce Assoc = with nowhere to put its generics). A separate (preexisting) bug, but a FIXME is the honest thing to leave behind
Don't be shy to leave FIXMEs. A FIXME that names a real limitation is much more useful than pretending the code is perfect - it's basically a little note to the next person (or future you)
Round Two of Nits
I pinged @rustbot ready and, like clockwork, a second smaller round came back. This one was the comfy kind - all naming and wording, nothing scary
The running theme was "make names tell the truth": a variable still called unbound_types was actually holding assoc consts too by now, so it got an honest rename, a one-off helper with a misleading name got inlined away, and a comment that said generic param where it meant generic arg got fixed (this corner of the compiler mixes those two up constantly)
A big chunk of review comments are exactly this - a name, a word, a stale comment. It's not "you did it wrong", it's just how code stays readable for whoever touches it next, and it's the easiest kind of feedback to address
He also asked for one more test: that we don't suggest nonsense for a plain Iterator<0>. So i extended some test under tests/ui/suggestions/
pub trait T<X, Y> {
type A;
type B;
type C;
}
pub struct Foo {
i: Box<dyn T<usize, usize, usize, usize, B = usize>>,
//~^ ERROR trait takes 2 generic arguments but 4 generic arguments were supplied
}
fn take(_: impl Iterator<0>) {} // <- here is what i've actually added !!!
//~^ ERROR trait takes 0 generic arguments but 1 generic argument was supplied
fn main() {}
The whole point is the .stderr has no "turn the generic argument" help - Iterator has an assoc type but no assoc const, so the const queue is empty and we suggest nothing, exactly as intended
After all that the suggestion finally does the right thing even in the weird ordering cases, the grammar is correct in both singular and plural, the names say what they mean, and the comment doesn't lie anymore
And now this is ready for merge 🎉 🎉 🎉
What I Hope You Take From This
If there's one thing i want you to leave with, it's that you really don't need to understand the whole compiler to make a diagnostic better. We took something that suggested complete nonsense, followed -Ztrack-diagnostics to the rough spot, found the exact code with a lazy grep, and made the suggestion only show up when it actually makes sense
A-diagnostics issues are great for this - a lot of them sit comfortably in the easy-to-medium range, they're user-facing so your work is immediately visible, and the people you'll meet along the way are genuinely kind
So if any of this looked fun - go pick one, grep around, ask questions, and don't be afraid to be wrong. That's literally how all of us started
And honestly, if you wanna try one but feel a little lost - you can just assign me as your reviewer with r? @Kivooeo (or ping me anywhere), i'm always happy to mentor newcomers on any kind of issues
On a side note, if you want to support future blog posts, my work on the compiler and mentoring newcomers, i'd be really grateful if you took a look at my sponsor page <3
