-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Provide placeholder generics for traits in "no method found for type parameter" suggestions #132487
Provide placeholder generics for traits in "no method found for type parameter" suggestions #132487
Conversation
r? @nnethercote rustbot has assigned @nnethercote. Use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you surround the placeholder arguments with /*
and */
to highlight the fact that they are placeholders? E.g., Trait2</* '_, A, B */>
. Otherwise users may take them literally and wonder whether the suggestion is wrong if it leads to more errors. It's not a hypothetical btw, people have reported such suggestions as bugs.
This would also be consistent with the diagnostic suggestion for E0220 (associated type not found). See 02a2f02 were I impl'ed that.
let candidate_strs: Vec<String> = candidates | ||
.iter() | ||
.map(|cand| { | ||
use ty::GenericParamDefKind::{Const, Lifetime, Type}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to simplify this by using identity_for_item
over generics_of
. Compare:
rust/compiler/rustc_hir_analysis/src/hir_ty_lowering/errors.rs
Lines 253 to 263 in ef972a3
let trait_args = &ty::GenericArgs::identity_for_item(tcx, best_trait)[1..]; | |
let mut trait_ref = trait_name.clone(); | |
let applicability = if let [arg, args @ ..] = trait_args { | |
use std::fmt::Write; | |
write!(trait_ref, "</* {arg}").unwrap(); | |
args.iter().try_for_each(|arg| write!(trait_ref, ", {arg}")).unwrap(); | |
trait_ref += " */>"; | |
Applicability::HasPlaceholders | |
} else { | |
Applicability::MaybeIncorrect | |
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, disregard, that wouldn't consider generic parameter defaults.
2afb931
to
aa1dfdd
Compare
The placeholders should now be surrounded by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Sorry for the slight delay. I was just dumbfounded that we don't have an API for that already somewhere. I guess one could use GenericArgs::identity_for_item
plus Generics::own_args_no_defaults
but er, that would perform much worse in this case.
I have small stylistic suggestions but nothing major. Lastly, could you squash your commits into one?
…for type parameter" suggestions
8a3c058
to
02add7d
Compare
Done. Thanks! |
@bors r+ rollup (diagnostics) |
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#132487 (Provide placeholder generics for traits in "no method found for type parameter" suggestions) - rust-lang#132627 (cleanup: Remove outdated comment of `thir_body`) - rust-lang#132653 (Don't use `maybe_unwrap_block` when checking for macro calls in a block expr) - rust-lang#132793 (Update mdbook to 0.4.42) - rust-lang#132847 (elem_offset / subslice_range: use addr() instead of 'as usize') - rust-lang#132869 (split up the first paragraph of doc comments for better summaries) - rust-lang#132929 (Check for null in the `alloc_zeroed` example) - rust-lang#132933 (Make sure that we suggest turbofishing the right type arg for never suggestion) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#132487 - dianne:include-trait-args-in-suggestion, r=fmease Provide placeholder generics for traits in "no method found for type parameter" suggestions In the diagnostics for the error ``no method named `method` found for type parameter `T` in the current scope [E0599]``, the compiler will suggest adding bounds on `T` for traits that define a method named `method`. However, these suggestions didn't include any generic arguments, so applying them would result in a `missing generics for trait` or `missing lifetime specifier` error. This PR adds placeholder arguments to the suggestion in such cases. Specifically, I tried to base the placeholders off of what's done in suggestions for when generics are missing or too few are provided: - The placeholder for a parameter without a default is the name of the parameter. - Placeholders are not provided for parameters with defaults. Placeholder arguments are enclosed in `/*` and `*/`, and the applicability of the suggestion is downgraded to `Applicability::HasPlaceholders` if any placeholders are provided. Fixes rust-lang#132407
In the diagnostics for the error
no method named `method` found for type parameter `T` in the current scope [E0599]
, the compiler will suggest adding bounds onT
for traits that define a method namedmethod
. However, these suggestions didn't include any generic arguments, so applying them would result in amissing generics for trait
ormissing lifetime specifier
error. This PR adds placeholder arguments to the suggestion in such cases. Specifically, I tried to base the placeholders off of what's done in suggestions for when generics are missing or too few are provided:Placeholder arguments are enclosed in
/*
and*/
, and the applicability of the suggestion is downgraded toApplicability::HasPlaceholders
if any placeholders are provided.Fixes #132407