fix suggestion error in [useless_vec]
#12116
Conversation
|
r? @Alexendoo (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
the
S-waiting-on-review
|
Oops, I... messed up the command, I'm sorry 😅 |
| { | ||
| // report the error around the `vec!` not inside `<std macros>:` | ||
| let span = expr.span.ctxt().outer_expn_data().call_site; |
There was a problem hiding this comment.
I guess the real question is: why does this return an empty span?
There was a problem hiding this comment.
in the example from the issue:
for elt in &(vec![...]) {
// ^^^^^^^^^^^^ exprexpr is the reference to the expansion of the vec macro because vec_args is constructed from higher::VecArgs::hir(cx, expr.peel_borrows()), I think a clearer solution would be get the parent callsite of expr.peel_borrows(), can stick it at the top once since outer_expn_data().call_site seems to repeated many times currently
clippy_lints/src/vec.rs
Outdated
| let maybe_len = len_span | ||
| .map(|sp| format!("; {}", snippet_with_applicability(cx, sp, "len", app))) | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
can this get an empty string, that would result in ; ?
I wonder if we should trim in this case to not suggest adding empty spaces in the suggestion, if that makes sense.
There was a problem hiding this comment.
Ahh, it might, if the span is empty in some rare occasions when macros involved (?).
Or, suggesting ; len by default... which is not a correct suggestion anyway...
I'll make sure no suggestion is given in these scenarios, but I can't guarantee to come up with some test cases to test it lol 😄
There was a problem hiding this comment.
Yeah, was just trying to think of "what could go wrong"
|
LGTM but I'm also kinda a noob at reviewing clippy prs, so I'd like to have a second eye on this by someone who is more knowledgeable (aka literally anyone else :D ) |
|
☔ The latest upstream changes (presumably #12331) made this pull request unmergeable. Please resolve the merge conflicts. |
J-ZhengLi
force-pushed
the
issue12101
branch
3 times, most recently
from
d19011e
to
929f746
Compare
@Alexendoo Thanks for the hint 😄 Also, since you already looked this one, and matthiaskrgr asked for a second eye, do you want to be assigned to this? |
|
Thanks, looks good! @bors r+ |
fix suggestion error in [`useless_vec`] fixes: #12101 --- changelog: fix suggestion error in [`useless_vec`] r+ `@matthiaskrgr` since they opened the issue?
|
💔 Test failed - checks-action_test |
|
hmmm @Alexendoo, this is weird, bot says I need to run |
|
@bors retry |
fix suggestion error in [`useless_vec`] fixes: #12101 --- changelog: fix suggestion error in [`useless_vec`] r+ `@matthiaskrgr` since they opened the issue?
|
💔 Test failed - checks-action_test |
|
@bors retry |
fix suggestion error in [`useless_vec`] fixes: #12101 --- changelog: fix suggestion error in [`useless_vec`] r+ `@matthiaskrgr` since they opened the issue?
|
💔 Test failed - checks-action_test |
|
Still a different error each time... @bors r+ |
|
💡 This pull request was already approved, no need to approve it again.
|
|
Oops, @bors retry |
fix suggestion error in [`useless_vec`] fixes: #12101 --- changelog: fix suggestion error in [`useless_vec`] r+ `@matthiaskrgr` since they opened the issue?
|
💔 Test failed - checks-action_test |
|
@bors retry |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
fixes: #12101
changelog: fix suggestion error in [
useless_vec]r+ @matthiaskrgr since they opened the issue?