-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix suggestion error for [manual_is_ascii_check
] with missing type
#11988
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
r? @Alexendoo (rustbot has picked a reviewer for you, use r? to override) |
c7ef3fc
to
8aabd7a
Compare
manual_is_ascii_check
] skips generic fn param, and suggest inferred ty param in closure
8aabd7a
to
936d19c
Compare
manual_is_ascii_check
] skips generic fn param, and suggest inferred ty param in closuremanual_is_ascii_check
] with missing type
let ExprKind::Lit(lit) = range_expr.kind else { | ||
return None; | ||
}; | ||
let sugg_ty_span = path_to_local(arg).and_then(|id| map.get(&id)).copied()?; |
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.
If it works it would be good to use find_binding_init
here rather than storing a map, would let us avoid any issues with nested closures
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.
Unfortunately I couldn't get it to work...
but regarding nested closures, I now clear the map only when entering new modules rather than leaving another closure, which should be able to prevent it.
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 yeah that makes sense, there's no local/init to be found
What could be done is to get the parent of the local's id which should be a Param
, for inferred types the ty_span
is the same as the span
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.
nice, this is the way to go, thanks~
Hey @Alexendoo, this is a ping from triage. Can you give this PR a review? It's totally fine if you don't have the time right now, you can reassign the PR to a random team member using @rustbot ready |
diag.span_suggestion(span, "try", format!("{recv}.{sugg}()"), app); | ||
if let Some((ty_span, ty_str)) = ty_sugg { | ||
diag.span_suggestion( | ||
ty_span, | ||
"also make sure to label the correct type", | ||
format!("{recv}: {ty_str}"), | ||
app, | ||
); | ||
} | ||
}, |
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.
Might be nicer to use a multipart suggestion here
☔ The latest upstream changes (presumably #12624) made this pull request unmergeable. Please resolve the merge conflicts. |
suggest explicit type when its inferred in closure
Thank you! @bors r+ |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
fixes: #11324
fixes: #11776
changelog: improve [
manual_is_ascii_check
] to suggest labeling type in closure, fix FP with type generics, and improve linting on ref expressions.