Introduce needless_option_take lint

[b-ncMN]

• Followed [lint naming conventions][lint_naming]
• Added passing UI tests (including committed .stderr file)
• cargo test passes locally
• Executed cargo dev update_lints
• Added lint documentation
• Run cargo dev fmt

Fixes #8618

changelog: Introduce [needless_option_take] lint

[rust-highfive]

r? @llogiq

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

[llogiq]

Hi there! Thank you for making clippy better. If you need anything, let me know.

[b-ncMN]

I think this is ready

[b-ncMN]

Is it normal for these to fail?
They don't seem to be related to my changes

[Alexendoo]

The test output can be pretty large but you want to look out for the diffs in them, e.g.

https://github.com/rust-lang/rust-clippy/runs/5955895411?check_suite_focus=true#step:7:939
https://github.com/rust-lang/rust-clippy/runs/5955895411?check_suite_focus=true#step:7:1416

[b-ncMN]

The test output can be pretty large but you want to look out for the diffs in them, e.g.

https://github.com/rust-lang/rust-clippy/runs/5955895411?check_suite_focus=true#step:7:939 https://github.com/rust-lang/rust-clippy/runs/5955895411?check_suite_focus=true#step:7:1416

I see,
but those files weren't added by me, this is weird

[Alexendoo]

Since the lint is enabled by default it's also run on all the existing test files. The diff there is between the expected stderr in the .stderr file and the result of running clippy on it

[b-ncMN]

Since the lint is enabled by default it's also run on all the existing test files. The diff there is between the expected stderr in the .stderr file and the result of running clippy on it

Right!
So those failing lints need to be modified as well, is that correct?
For a second I thought I merged other people's branches into my branch but no, thank god :^)

what I don't understand too are the redundant-closures lint errors, that's not a lint of mine,
was that not modified when that lint was added?
I figured I probably need to edit the errors coming from my lint first, and then see what happens, this may be a side effect 🤔

[Alexendoo]

The redundant closure errors are part of the existing tests, you can see them in https://github.com/rust-lang/rust-clippy/blob/c7e68638431b2a6639fcbeb44de790619de3b9b1/tests/ui/eta.stderr

The diff shown is between that file and the result of running clippy with your additions on https://github.com/rust-lang/rust-clippy/blob/c7e68638431b2a6639fcbeb44de790619de3b9b1/tests/ui/eta.rs. It shows a new error pointing at

rust-clippy/tests/ui/eta.rs

Line 118 in c7e6863

Thunk(Box::new(move || option.take().unwrap()()))

This is a false positive though, rather than updating the tests the lint should be changed so it doesn't fire, we don't want to be linting things of the form option.take()

[xFrednet]

From the failed CI, the lint message should start with a lowercase

- Called `Option::take()` on a temporary value
+ called `Option::take()` on a temporary value

[b-ncMN]

From the failed CI, the lint message should start with a lowercase

- Called `Option::take()` on a temporary value

+ called `Option::take()` on a temporary value

I know, just didn't have time to apply it yet.

[bors]

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

[b-ncMN]

Can I get some reviews again ? Meanwhile I rebase the branch

[b-ncMN]

Do we need an MSRV for rust features from rust 1.0.0 ? https://doc.rust-lang.org/src/core/option.rs.html#1550

[llogiq]

Do we need an MSRV for rust features from rust 1.0.0 ? https://doc.rust-lang.org/src/core/option.rs.html#1550

I don't think so, we don't support pre-1.0.0 Rust versions.

[b-ncMN]

So wait, my code now currently does not check for temporaries, just as_ref, that's not what I had in mind at first

[llogiq]

That's ok. Checking for temporaries is a bit harder, as you'll have to check if the bindings are used later on (which clippy_utils has methods for, if memory serves). But I'd be OK with merging this less capable lint and noting in the docs that using a temporary binding will currently lead to a false negative, then tackling that in a followup PR.

[b-ncMN]

That's ok. Checking for temporaries is a bit harder, as you'll have to check if the bindings are used later on (which clippy_utils has methods for, if memory serves). But I'd be OK with merging this less capable lint and noting in the docs that using a temporary binding will currently lead to a false negative, then tackling that in a followup PR.

Ok then let's do this, I will try to tackle the next step.
And I still wonder why using a temporary binding will produce a false positive 🤔

[llogiq]

Thank you!

@bors r+

[bors]

📌 Commit 2903b56 has been approved by llogiq

[bors]

⌛ Testing commit 2903b56 with merge e5ebece...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing e5ebece to master...

[b-ncMN]

@llogiq do you mind pointing me to where I need to look at to get started on the followup ? I briefly tried searching for functions related to temporaries in the docs but didn't find anything

[llogiq]

Sorry for letting you wait so long, but I'm on vacation and have only spotty network. I'll try to get back to you within two days.

[llogiq]

Ok, so currently we only catch _.as_ref().take(). We can extend that by creating a function that recurses over all method calls, that returns an enum OptionCallChain { FoundAsRef, FoundLocal(HirId), FoundNone } with FoundAsRef returned if we encounter a method call to Option::as_ref, and FoundLocal(_) if we have a ExprPath(_) referencing a local variable.

In the FoundLocal case, we must visit the parent body with the take call's HirId, ignoring everything before that call and checking if we encounter that local variable again. We also need to check if the local was given to the function by ref. Then we only lint if the local is not encountered in the body or arguments. You can find Visitor implementations in clippy_utils for reference.

[b-ncMN]

In the FoundLocal case, we must visit the parent body with the take call's HirId, ignoring everything before that call and checking if we encounter that local variable again. We also need to check if the local was given to the function by ref. Then we only lint if the local is not encountered in the body or arguments. You can find Visitor implementations in clippy_utils for reference.

I'm confused about how to achieve the second part, the one just up there :
I've seen https://doc.rust-lang.org/nightly/nightly-rustc/rustc_hir/intravisit/trait.Visitor.html#method.visit_id, but I don't seem to have any walk_id function.
my currently looks like

struct TakeVisitor<'a, 'tcx> {
    cx: &'a LateContext<'tcx>,
    identifiers: HashSet<HirId>,
}

impl<'a, 'tcx> Visitor<'tcx> for TakeVisitor<'a, 'tcx> {
    fn visit_id(&mut self, hir_id: HirId) {
        self.identifiers.insert(hir_id);
        // walk_id()
    }
}

fn recurse_over_methods(cx: &LateContext<'_>, expr: &Expr<'_>) -> OptionCallChain {
    //dbg!(expr);
    match expr.kind {
        rustc_hir::ExprKind::MethodCall(method_path, [value], _) => {
            match method_path.ident.as_str() {
                // Checks if last function in line == "take"
                "take" => match value.kind {
                    rustc_hir::ExprKind::MethodCall(value_method_path, [_], _) => {
                        dbg!(method_path, value);
                        if value_method_path.ident.as_str() == "as_ref" {
                            return OptionCallChain::FoundAsRef;
                        }
                    },
                    _ => {},
                },
                _ => {},
            }
        },
        _ => {
            println!("not a method call");
        },
    };
    OptionCallChain::FoundAsRef
}

I hope I've understanded the part correctly, in which, I need to use walk_id to walk over the IDs, and insert each one of those into a HashSet if it does not match any previous entry

[llogiq]

There is nothing in an id we would have to walk.

We need to visit the parent item, not the method call. However, the visitor should only count uses of the local after encountering the take expr (which you can check by storing and comparing the expr.id).

So you need to a) walk the method chain until finding an ExprPath (which would be the foo in foo.take()), get its def I'd and the expr.id of the take() method call), b) get the parent item and if it is a fn check if foo comes from a function argument and if not c) run the visitor to check for later uses of our foo (because those would observe the change of our take).

[b-ncMN]

There is nothing in an id we would have to walk.

We need to visit the parent item, not the method call. However, the visitor should only count uses of the local after encountering the take expr (which you can check by storing and comparing the expr.id).

So you need to a) walk the method chain until finding an ExprPath (which would be the foo in foo.take()), get its def I'd and the expr.id of the take() method call), b) get the parent item and if it is a fn check if foo comes from a function argument and if not c) run the visitor to check for later uses of our foo (because those would observe the change of our take).

Oh, right,
I was a little confused by "In the FoundLocal case, we must visit the parent body with the take call's HirId"
So I need to walk the expr until I find ExprPath and find out if it contains take ?
What is the parent item you refer to in B ?
Could we take contact in Zulip ? I think it would be easier to communicate