Fix infinite loop in cast_sign_loss when peeling unwrap method calls
#12508
Conversation
rustbot
added
the
S-waiting-on-review
| @@ -134,11 +134,12 @@ fn expr_sign<'cx>(cx: &LateContext<'cx>, expr: &Expr<'_>, ty: impl Into<Option<T | |||
| // Peel unwrap(), expect(), etc. | |||
| while let Some(&found_name) = METHODS_UNWRAP.iter().find(|&name| &method_name == name) | |||
| && let Some(arglist) = method_chain_args(expr, &[found_name]) | |||
There was a problem hiding this comment.
actually now that I'm taking a closer look at the lint code, this method_chain_args call seems redundant? It doesn't really seem to do anything in this context, except for returning the expression back that was passed in, as far as I can tell.
The line right above it already checks that it's a call to one of the unwrap family of methods.
I removed it locally and the tests are still passing, would be good to have someone else look over this before I actually remove it tho, maybe it's relevant for something else
There was a problem hiding this comment.
The only additional check it does is whether the method path or any of the rags come from a macro/expansion. If so, the call will return None.
I don't think that should matter in the context of this lint.
There was a problem hiding this comment.
do you mean that we should just keep it then? I guess we can. this just seemed odd to me because the function is generally useful for finding method chains, but is being used for a very different purpose here and doesn't actually look for any chains the way it's used (perhaps confusingly, since the loop is what ends up looking for chains) and does a bunch of unnecessary work like building a vec
|
Thank you! @bors r+ |
Fix infinite loop in `cast_sign_loss` when peeling unwrap method calls Fixes #12506 The lint wants to peel method calls but didn't actually reassign the expression, leading to an infinite loop. changelog: Fix infinite loop in [`cast_sign_loss`] when having two chained `.unwrap()` calls
|
💔 Test failed - checks-action_test |
|
@llogiq Any idea of what went wrong? |
|
Probably just a spurious ci error. That happened on another PR and retrying worked. But I also wanted to talk about that one method call first before merging while we're at it, and it looks like its waiting for that atm. I'd also be fine with just leaving it if we want this fixed asap |
|
@bors retry |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
y21
added
the
beta-nominated
|
Nominated for beta backport since the bug made it to beta and looks like something that people might accidentally run into |
FWIW, here's the actual real-world code that caused me to open #12506: fn page_size() -> usize {
nix::unistd::sysconf(nix::unistd::SysconfVar::PAGE_SIZE)
.unwrap()
.unwrap() as usize
}The lack of error checking is because we just use this in a test case. |
flip1995
added
beta-accepted
…ulacrum [beta] Clippy backport r? `@Mark-Simulacrum` Backports: - rust-lang/rust-clippy#12486 - rust-lang/rust-clippy#12572 - rust-lang/rust-clippy#12508 - rust-lang/rust-clippy#12617 The first one is a bit bigger as usual for a backport. But it fixes a major issue with this lint that we overlooked. So I think this is worth it. After that was merged into nightly, there were no new issues opened about this lint, so IMO this is safe to backport to `beta` and put into stable.
xFrednet
removed
the
beta-accepted
Fixes #12506
The lint wants to peel method calls but didn't actually reassign the expression, leading to an infinite loop.
changelog: Fix infinite loop in [
cast_sign_loss] when having two chained.unwrap()calls