-
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? 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
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.