[useless_conversion]: only lint on paths to fn items and fix FP in macro#11070
Conversation
rustbot
added
the
S-waiting-on-review
llogiq
left a comment
llogiq
left a comment
There was a problem hiding this comment.
Looks good in general. I only have one question regarding simplicity, otherwise I consider this merge-worthy.
| /// Checks if the given `expr` is an argument of a macro invocation. | ||
| /// This is a slow-ish operation, so consider calling this late | ||
| /// to avoid slowing down the lint in the happy path when not emitting a warning | ||
| fn is_macro_argument(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { |
There was a problem hiding this comment.
Could we avoid that by keeping an Option<Span> in our lint to be set when we encounter an expression from a macro? That way this check would become a simple .is_some().
There was a problem hiding this comment.
I'm not sure. How/When does it get reset to None? If we just set it to Some(span) once we see an expression from an expansion, it would forever think we are in a macro, no? Or do you mean we should also then check if the span in the lint struct fully encloses the span currently being looked at, or something like that?
There was a problem hiding this comment.
Yeah, we can have check_expr set the span to Some(span) and then remove it if it matches in check_expr_post.
There was a problem hiding this comment.
Hmm, but wouldn't this at least also need a stack of spans, to handle nested macro expansions? Alternatively, could we just use a u32 that we increment/decrement for entering or leaving a span from an expansion and check if == 0 when about to lint?
|
☔ The latest upstream changes (presumably #11239) made this pull request unmergeable. Please resolve the merge conflicts. |
flip1995
added
the
beta-nominated
|
(I didn't change anything with this rebase, just fixed the conflicts) |
|
Thanks! I'm going ahead and approve this, so I can include it in the backports. @bors r+ |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
flip1995
added
beta-accepted
|
rust-lang/rust#114937 for beta and rust-lang/rust#114938 for master |
…k-Simulacrum [beta] Clippy backports for ICE fixes This backports PRs to beta, that fix ICEs, some lint grouping and FP fixes. Namely: - rust-lang/rust-clippy#11191 - rust-lang/rust-clippy#11172 - rust-lang/rust-clippy#11130 - rust-lang/rust-clippy#11106 - rust-lang/rust-clippy#11104 - rust-lang/rust-clippy#11077 - rust-lang/rust-clippy#11070 (This PR is not synced to the Rust repo yet, but I will open a separate PR to get it into `master`, before beta is branched: rust-lang#114938) - rust-lang/rust-clippy#11069 Kind of a big backport, but most of it is tests. r? `@Mark-Simulacrum` cc `@Manishearth`
xFrednet
removed
the
beta-accepted
[`useless_conversion`]: only lint on paths to fn items and fix FP in macro Fixes rust-lang#11065 (which is actually two issues: an ICE and a false positive) It now makes sure that the function call path points to a function-like item (and not e.g. a `const` like in the linked issue), so that calling `TyCtxt::fn_sig` later in the lint does not ICE (fixes rust-lang#11065 (comment)). It *also* makes sure that the expression is not part of a macro call (fixes rust-lang#11065 (comment)). ~~I'm not sure if there's a better way to check this other than to walk the parent expr chain and see if any of them are expansions.~~ (edit: it doesn't do this anymore) changelog: [`useless_conversion`]: fix ICE when call receiver is a non-fn item changelog: [`useless_conversion`]: don't lint if argument is a macro argument (fixes a FP) r? `@llogiq` (reviewed rust-lang#10814, which introduced these issues)
6 participants
Fixes #11065 (which is actually two issues: an ICE and a false positive)
It now makes sure that the function call path points to a function-like item (and not e.g. a
constlike in the linked issue), so that callingTyCtxt::fn_siglater in the lint does not ICE (fixes #11065 (comment)).It also makes sure that the expression is not part of a macro call (fixes #11065 (comment)).
I'm not sure if there's a better way to check this other than to walk the parent expr chain and see if any of them are expansions.(edit: it doesn't do this anymore)changelog: [
useless_conversion]: fix ICE when call receiver is a non-fn itemchangelog: [
useless_conversion]: don't lint if argument is a macro argument (fixes a FP)r? @llogiq (reviewed #10814, which introduced these issues)