Avoid redundant note when a #[derive] is already suggested#157126
Conversation
The `#[rustc_on_unimplemented]` note on `Debug` ("add `#[derive(Debug)]`
to `X` or manually `impl Debug for X`") duplicates the `consider
annotating X with #[derive(..)]` suggestion emitted by `suggest_derive`.
Skip the note when that suggestion will be shown, and keep it otherwise
so types whose derive can't be suggested (e.g. a field isn't `Debug`)
still get actionable guidance.
|
r? @adwinwhite rustbot has assigned @adwinwhite. Use Why was this reviewer chosen?The reviewer was selected based on:
|
| if derive_suggestion_will_be_shown { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
It makes more sense to not enter into the loop if every element will be skipped if a condition is true. The way it is written we're just wasting time iterating over every element in order to do nothing.
There was a problem hiding this comment.
Just to confirm, what is the difference between the output in this PR and the output if we remove the following lines? Are there some cases where the note is useful? It'd be a good idea to include that context in the comment or at least in this PR.
rust/library/core/src/fmt/mod.rs
Lines 1039 to 1043 in 6368fd5
There was a problem hiding this comment.
Yeah, good call flagging this. Short version: deleting those lines would kill the note everywhere, but it's only redundant some of the time, so I skip it conditionally instead.
suggest_derive shows consider annotating X with #[derive(..)] only when can_suggest_derive holds: a crate-local ADT, not a union, and all fields already implement the trait. When that fires, the note and the suggestion are saying the same thing, so the note is just clutter (that's what the stderr diffs are trimming).
When can_suggest_derive is false there's no suggestion at all, and then the note is the only nudge the user gets. Easiest case to picture is a local struct with a field that isn't Debug:
struct Inner; // no Debug
struct Outer {
inner: Inner,
}
fn main() {
println!("{:?}", Outer { inner: Inner });
}error[E0277]: `Outer` doesn't implement `Debug`
= note: add `#[derive(Debug)]` to `Outer` or manually `impl Debug for Outer`
No consider annotating line here, since Outer's field isn't Debug, so dropping the lines outright would leave this one with nothing actionable. Same story when main_trait_predicate != leaf_trait_predicate: the note's built from the main predicate and the suggestion from the leaf, so they don't overlap.
Happy to pull this into the PR description too. Thanks for pushing on it! :)
The condition is loop-invariant, so iterating the notes only to skip every one wastes work. Test the derive suggestion once and skip the whole loop when it will be shown.
|
@bors r+ |
…-note-on-unimplemented, r=estebank Avoid redundant note when a #[derive] is already suggested Fixes rust-lang#157118 The `Debug` `#[rustc_on_unimplemented]` note ("add `#[derive(Debug)]` to `X` or manually `impl Debug for X`") just repeats the `consider annotating X with #[derive(..)]` suggestion that `suggest_derive` already emits, so on these bound errors it's pure noise. Now the note only gets dropped when that suggestion will actually show, which is whenever `can_suggest_derive` holds: a crate-local ADT, not a union, with every field already implementing the trait. Deleting the note from the attribute outright would be too blunt, since when `can_suggest_derive` is false there's no suggestion and the note is the only hint the user gets. A crate-local struct with a non-`Debug` field still ends up with just: ``` error[E0277]: `Outer` doesn't implement `Debug` = note: add `#[derive(Debug)]` to `Outer` or manually `impl Debug for Outer` ``` Same story when `main_trait_predicate != leaf_trait_predicate`, since the note comes from the main predicate and the suggestion from the leaf.
Rollup of 6 pull requests Successful merges: - #156832 (library: use strict provenance lints consistently) - #157006 (net tests: let the OS pick the port numbers) - #157126 (Avoid redundant note when a #[derive] is already suggested) - #151690 (std: Refactor `env::var` function) - #155826 (std::process: uefi: avoid panicking in Stdio From impls.) - #156109 (Migrate libraries from ptr::slice_from_raw_parts to .cast_slice)
…uwer Rollup of 8 pull requests Successful merges: - #157006 (net tests: let the OS pick the port numbers) - #157126 (Avoid redundant note when a #[derive] is already suggested) - #151690 (std: Refactor `env::var` function) - #155826 (std::process: uefi: avoid panicking in Stdio From impls.) - #156109 (Migrate libraries from ptr::slice_from_raw_parts to .cast_slice) - #156642 (`offload_kernel` macro expansion) - #156823 (Add regression test) - #157162 (elaborate_drop: Avoid as_mut.unwrap dance, empty vectors are cheap.) Failed merges: - #157173 (rustc-dev-guide subtree update)
Rollup merge of #157126 - Dnreikronos:avoid-redundant-derive-note-on-unimplemented, r=estebank Avoid redundant note when a #[derive] is already suggested Fixes #157118 The `Debug` `#[rustc_on_unimplemented]` note ("add `#[derive(Debug)]` to `X` or manually `impl Debug for X`") just repeats the `consider annotating X with #[derive(..)]` suggestion that `suggest_derive` already emits, so on these bound errors it's pure noise. Now the note only gets dropped when that suggestion will actually show, which is whenever `can_suggest_derive` holds: a crate-local ADT, not a union, with every field already implementing the trait. Deleting the note from the attribute outright would be too blunt, since when `can_suggest_derive` is false there's no suggestion and the note is the only hint the user gets. A crate-local struct with a non-`Debug` field still ends up with just: ``` error[E0277]: `Outer` doesn't implement `Debug` = note: add `#[derive(Debug)]` to `Outer` or manually `impl Debug for Outer` ``` Same story when `main_trait_predicate != leaf_trait_predicate`, since the note comes from the main predicate and the suggestion from the leaf.
Fixes #157118
The
Debug#[rustc_on_unimplemented]note ("add#[derive(Debug)]toXor manuallyimpl Debug for X") just repeats theconsider annotating X with #[derive(..)]suggestion thatsuggest_derivealready emits, so on these bound errors it's pure noise.Now the note only gets dropped when that suggestion will actually show, which is whenever
can_suggest_deriveholds: a crate-local ADT, not a union, with every field already implementing the trait. Deleting the note from the attribute outright would be too blunt, since whencan_suggest_deriveis false there's no suggestion and the note is the only hint the user gets. A crate-local struct with a non-Debugfield still ends up with just:Same story when
main_trait_predicate != leaf_trait_predicate, since the note comes from the main predicate and the suggestion from the leaf.