Skip to content

Ignore as_deref_mut in needless_option_as_deref #8064

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

Closed
wants to merge 1 commit into from

Conversation

Alexendoo
Copy link
Member

Fixes #7846
Fixes #8047

It would be possible to only lint as_deref_mut where the Option is movable + unused later, but it wasn't clear to me how to test for that. So for now at least ignore it

changelog: [needless_option_as_deref]: No longer lints on as_deref_mut

@rust-highfive
Copy link

r? @Manishearth

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 2, 2021
@Manishearth
Copy link
Member

Currently still catching up on stuff from travel, might not be able to provide a timely review

r? @giraffate (feel free to reassign)

@surechen
Copy link
Contributor

surechen commented Dec 4, 2021

Hi, hope I won't disturb you. I 'm also interested in this issue and hope to discuss it together.

According to lint's comments, I think the author of lint wants to detect the situation "Option<&mut t> to Option<&mut t>", because this is not necessary, as_deref_mut is suitable for "&mut Option to Option<&mut t>", but because as_deref_mut has another meaning: "Leaves the original Option in-place, creating a new one containing a mutable reference to the inner type's Deref ::Target type.”, which caused a problem in the suggestion code in issue #8047.

So I think this lint is valuable for detecting unnecessary use of as_deref_mut.

For the example in issue #8047:

fn foo(_x: Option<&mut i32>) {}

fn main() {
     let mut y = 0;
     let mut x = Some(&mut y);
    
     foo(x.as_deref_mut()); // lint suggestion   `x`
     println!("{:?}", x); // use after move
}

Is it possible to scan to check the use after move situation in #8047 or just change the suggestion level to MaybeIncorrect?

@Alexendoo
Copy link
Member Author

Yeah this is more of a quick fix, it introduces a false negative in return for fixing false positives. MaybeIncorrect would mean the suggestion isn't automatically applied by --fix but the warning would still be there

Detecting it properly would be preferable, but I wasn't sure how to go about that easily. That could always come as a later PR, or somebody may point out something I missed

@Manishearth
Copy link
Member

Worth bringing up in the Clippy zulip channel and seeing what others think! https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy

@giraffate
Copy link
Contributor

@bors
Copy link
Contributor

bors commented Dec 30, 2021

☔ The latest upstream changes (presumably #8196) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Jan 27, 2022

☔ The latest upstream changes (presumably #8359) made this pull request unmergeable. Please resolve the merge conflicts.

@Alexendoo Alexendoo closed this Apr 6, 2022
@Alexendoo Alexendoo deleted the as_deref_mut branch April 6, 2022 14:51
bors added a commit that referenced this pull request Apr 7, 2022
Fix `as_deref_mut` false positives in `needless_option_as_deref`

Also moves it into `methods/`

Fixes #7846
Fixes #8047

changelog: [`needless_option_as_deref`]: No longer lints for `as_deref_mut` on Options that cannot be moved

supersedes #8064
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
6 participants