Repository navigation
Conversation
rustbot
added
the
S-waiting-on-community-reviews
|
Thanks for the pull request, and welcome! You should hear from one of our reviewers after this PR gets at least 2 reviews from the community. Please see the contribution instructions for more information. |
rustbot
added
needs-fcp
|
Lintcheck changes for b06b4db
This comment will be updated if you push new changes |
nathanjf
changed the title
undeclared_type] lint to the [restriction] group
There was a problem hiding this comment.
community review: looking at the lintcheck results, things like
let frame = Frame {
dw_die_offset: Some(frames.function.dw_die_offset),
function: frames.function.name.clone().map(|name| FunctionName {
name,
language: frames.unit.lang,
}),
location: loc,
};from https://docs.rs/addr2line/0.24.0/src/addr2line/frame.rs.html#104 are being warned about, but there isn't any type inference needed here, because the value is coming from an explicit object creation, rather than from a method call
Can these cases be excluded, or at least have a configuration option to ignore them?
|
Sure. I'll look into how to differentiate between those two cases and try to add in that configuration, but in my opinion it's still a variable being assigned a value, even though I do understand it's much more obvious what the intent is in that case. // Obvious what's happening, but no explicit type
let frame = Frame {
...
}
// Obvious what's happening, with an explicit type
let frame: Frame = Frame {
...
}What would an acceptable name for that configuration be? |
Sounds reasonable to me for structs, but what about enums and unions? Would they have different configuration options? |
|
The purpose of the rule would be to enforce declaring the types of variables. I personally would rather not add any of the configuration options because while clear in intent what those lines do, they are exceptions to the intent of the lint. The simplest solution I can think of would be to add a configuration to check for only variables which are declared and assigned the return value of a function. Though with that said, I am unsure what a clear and concise name would be for that. Something like This is a default off lint in the #[allow(undeclared_type)]
let frame = Frame {
...
} |
| if local.ty.is_some() { | ||
| return; | ||
| } | ||
| if local.span.from_expansion() { | ||
| return; | ||
| } | ||
| if local.span.in_derive_expansion() { | ||
| return; | ||
| } | ||
| if local.span.in_external_macro(cx.tcx.deref().sess.source_map()) { | ||
| return; | ||
| } | ||
| span_lint_and_help( | ||
| cx, | ||
| UNDECLARED_TYPE, | ||
| local.span, | ||
| "declared variable has an undeclared type", | ||
| Some(local.span), | ||
| "declare a type for this variable", | ||
| ); |
There was a problem hiding this comment.
please use if-let since it is much more readable
| if local.ty.is_some() { | |
| return; | |
| } | |
| if local.span.from_expansion() { | |
| return; | |
| } | |
| if local.span.in_derive_expansion() { | |
| return; | |
| } | |
| if local.span.in_external_macro(cx.tcx.deref().sess.source_map()) { | |
| return; | |
| } | |
| span_lint_and_help( | |
| cx, | |
| UNDECLARED_TYPE, | |
| local.span, | |
| "declared variable has an undeclared type", | |
| Some(local.span), | |
| "declare a type for this variable", | |
| ); | |
| if local.ty.is_none() | |
| && !local.span.from_expansion() | |
| && !local.span.in_derive_expansion() | |
| && !local.span.in_external_macro(cx.tcx.deref().sess.source_map()) { | |
| span_lint_and_help( | |
| cx, | |
| UNDECLARED_TYPE, | |
| local.span, | |
| "declared variable has an undeclared type", | |
| Some(local.span), | |
| "declare a type for this variable", | |
| ); | |
| } |
There was a problem hiding this comment.
I much prefer not nesting code and doing early returns instead. I collapsed all the macro checks into one if statement. I am thinking about other ways to write it that would satisfy both you and I.
| vec![] | ||
| } | ||
|
|
||
| fn main() { |
There was a problem hiding this comment.
please add an test case for the macro checks that you added so we don't accidentally regress because an bodged rebase or something
There was a problem hiding this comment.
Will get this done as soon as I have time
There was a problem hiding this comment.
If anyone is interested, I ran this lint on a slightly outdated fork of cargo-geiger I had laying around. I committed all the auto-fixes that worked, and for those that initially produced code that wouldn't compile, I reverted the fix and added an expect. Maybe the suggestions could be improved to work better in these cases? I'm not sure. The branch is here: https://github.com/hashcatHitman/cargo-geiger/tree/undeclared_type_lint_testing
|
☔ The latest upstream changes (possibly #13598) made this pull request unmergeable. Please resolve the merge conflicts. |
This branch has not been deployed
5 participants
changelog: [
undeclared_type]: Added [undeclared_type] lint.This is a new lint in the restriction group that warns the user whenever they declare a new variable without explicitly declaring it's type.