-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Improve type parameter suggestion heuristic for missing types #140073
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
base: master
Are you sure you want to change the base?
Conversation
95b39e7
to
037c08e
Compare
This comment has been minimized.
This comment has been minimized.
660a5f4
to
9a92054
Compare
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.
Also test that a common name like Item
still suggest making it generic
&& sugg.chars().skip(1).any(|c| c.is_lowercase()); | ||
|
||
// If it's not a known generic name and looks like a concrete type, skip the suggestion. | ||
if !is_common_generic && looks_like_camel_case && sugg.len() > 3 { |
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.
the len 3 check is always triggered on const parameter suggestions. check the length of ident
instead. Also add a test for const generics, too
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.
can you please show me example of test for const, because i dont see how can i do it
pub fn do_stuff<const T: usize>() -> Option<T> {
None
}
this surely wrong because trigger another errror
and
pub fn do_stuff<const T: usize>() -> Option<ForgetType> {
None
}
this works fine
and i cant do something like this because it's basically wrong
pub fn do_stuff() -> Option<const T: usize> {
None
}
so i need an example of how test it correctly
or do you meant something like this?
pub fn do_stuff() -> Option<MAX> {
None
}
MAX here looks like constant so should be a generic right?
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.
fn f() -> [(); N] {}
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.
this one works just fine i guess
error[E0425]: cannot find value `N` in this scope
--> C:\Users\Love\projects\asfgas\src/main.rs:1:16
|
1 | fn f() -> [(); N] {}
| ^ not found in this scope
|
help: you might be missing a const parameter
|
1 | fn f<const N: /* Type */>() -> [(); N] {}
| +++++++++++++++++++++
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.
Oli is saying that for const parameters you currently perform checks like common_generic_names.contains("const N: usize")
which will never fail! You should compare against the original ident (in this case N
)
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.
Note however that for const parameters, the list common_generic_names
doesn't make sense, it's specifically for type parameters, so you should be comparing against a different list or none at all
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.
yes i got it, but i was wondering how do i write tests for this const case more correctly, and i also want to ask if name is full uppercase, should we consider it as generic?
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.
We should either use a different list for const params (of common const parameter names of length>1 (idk which ones could be considered common (LEN
? LENGTH
?, SIZE
? perhaps I dunno)) or don't do such a check for them at all
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.
and i also want to ask if name is full uppercase, should we consider it as generic?
I guess not necessarily, similar to (generic) type parameters. Since ELEMS
can just as well refer to a const item. E.g.:
const ELEMS: u32 = 3;
fn f() -> [i32; ELEMZ] { todo!() }
.map(|c| c.is_uppercase()) | ||
.unwrap_or(false) |
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.
.map(|c| c.is_uppercase()) | |
.unwrap_or(false) | |
.is_some_and(|c| c.is_uppercase()) |
To close the issue when this PR is merged, change your main comment to contain |
// | ||
// This approach may produce false positives or negatives, but works well in most cases. | ||
|
||
let common_generic_names: &[&str] = &[ |
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.
let common_generic_names: &[&str] = &[ | |
let common_type_param_names: &[&str] = &[ |
since it doesn't make sense for const params
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.
Also, I'd like to draw attention to the
&& segment.ident.name != kw::SelfUpper
&& segment.ident.name != kw::Dyn =>
at the very start of the function. We're already filtering out certain identifiers (in this case Self
and dyn
). I'd like us to unify this check and the common_generic_names
one is some way or another
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.
this is good point, will it be better to move my logic right after this let (ident, span) = ...
or integrate them inside the match
?
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.
We already have a bunch of test files for this diagnostic:
- tests/ui/missing/missing-items/missing-const-parameter.rs
- tests/ui/missing/missing-items/missing-type-parameter2.rs
- tests/ui/missing/missing-items/missing-type-parameter.rs
Could you merge your new test cases into the appropriate preexisting test files? That would great, thanks!
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.
We already have a bunch of test files for this diagnostic:
- tests/ui/missing/missing-items/missing-const-parameter.rs
- tests/ui/missing/missing-items/missing-type-parameter2.rs
- tests/ui/missing/missing-items/missing-type-parameter.rs
Could you merge your new test cases into the appropriate preexisting test files? That would great, thanks!
@oli-obk @fmease check this changes, replaced and i also didnt liked idea about renaming array of common names, because it's just common generic names that looks like UpperCamelCase and it's len > 3, so this array and check is needed to remove false positive results |
This comment has been minimized.
This comment has been minimized.
25300e3
to
e94d192
Compare
I consider this solution a starting point and not final. I'm open to your ideas and suggestions for further improvements to the heuristic or implementation.
Summary
This PR improves the heuristic used by
rustc
to suggest type parameters for missing types, addressing issue #139999. The issue occurs whenrustc
mistakenly suggests adding a type parameter for a missing type (e.g.,ForgotToImport
) that is likely a concrete type rather than a generic. This can lead to confusing error messages, as shown in the example:Current output suggests adding a type parameter:
This PR refines the heuristic to avoid such suggestions for names that are likely concrete types.
Changes
The fix modifies the logic in the type suggestion code to skip type parameter suggestions when the missing type name:
Item
,Output
,Error
).T
,U
).Example
With this change, the example code above will no longer suggest a type parameter for
ForgotToImport
, resulting in a cleaner error message:Rationale
This approach balances precision and simplicity:
Related Issue
fixes #139999
cc @Wilfred (issue reporter)