Rework only_used_in_recursion

[Jarcho]

fixes #8782
fixes #8629
fixes #8560
fixes #8556

This is a complete rewrite of the lint. This loses some capabilities of the old implementation. Namely the ability to track through tuple and slice patterns, as well as the ability to trace through assignments.

The two reported bugs are fixed with this. One was caused by using the name of the method rather than resolving to the DefId of the called method. The second was cause by using the existence of a cycle in the dependency graph to determine whether the parameter was used in recursion even though there were other ways to create a cycle in the graph.

Implementation wise this switches from using a visitor to walking up the tree from every use of each parameter until it has been determined the parameter is used for something other than recursion. This is likely to perform better as it avoids walking the entire function a second time, and it is unlikely to walk up the HIR tree very much. Some cases would perform worse though.

cc @buttercrab

changelog: Scale back only_used_in_recursion to fix false positives
changelog: Move only_used_in_recursion back to complexity

[rust-highfive]

r? @Manishearth

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

[xFrednet]

r? @Alexendoo

Would you maybe like to review this PR? 🙃

[Alexendoo]

Sure thing, I'll take a look at it

[bors]

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

[xFrednet]

Hey @Alexendoo, was this PR done after your comment was fixed? 🙃

[Alexendoo]

I wonder whether we want to mirror the unused behaviour of ignoring self entirely, I could see a preference for having method syntax even where it's not technically needed. It may also be surprising that adding a call would cause an unused style warning to appear

Other than that I have a few small bits of feedback, nothing major is jumping out at me but this is still a fairly complex lint

[ghost]

I tested this branch on bunch of crates and found a false positive.

---> lalrpop-0.19.8/src/normalize/lower/mod.rs:459:20                 
warning: parameter is only used in recursion                          
   --> src/normalize/lower/mod.rs:459:20
    |
459 |     fn symbol(&mut self, symbol: &pt::Symbol) -> r::Symbol {
    |                    ^^^^
    |
    = note: requested on the command line with `-W clippy::only-used-in-recursion`
note: parameter used here
   --> src/normalize/lower/mod.rs:463:79
    |
463 |             pt::SymbolKind::Choose(ref s) | pt::SymbolKind::Name(_, ref s) => self.symbol(s),
    |                                                                               ^^^^
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#only_used_in_recursion

This is what the function looks like. The lint misses the usage self.uses_error_recovery = true;.

    fn symbol(&mut self, symbol: &pt::Symbol) -> r::Symbol {
        match symbol.kind {
            pt::SymbolKind::Terminal(ref id) => r::Symbol::Terminal(id.clone()),
            pt::SymbolKind::Nonterminal(ref id) => r::Symbol::Nonterminal(id.clone()),
            pt::SymbolKind::Choose(ref s) | pt::SymbolKind::Name(_, ref s) => self.symbol(s),
            pt::SymbolKind::Error => {
                self.uses_error_recovery = true;
                r::Symbol::Terminal(TerminalString::Error)
            }

            pt::SymbolKind::Macro(..)
            | pt::SymbolKind::Repeat(..)
            | pt::SymbolKind::Expr(..)
            | pt::SymbolKind::AmbiguousId(_)
            | pt::SymbolKind::Lookahead
            | pt::SymbolKind::Lookbehind => unreachable!(
                "symbol `{}` should have been normalized away by now",
                symbol
            ),
        }
    }

[Jarcho]

Auto-deref makes linting not fun. Fixed.

[Jarcho]

I assume the reason for not linting self as unused is because it can't be renamed, so _self isn't an option.

The reasons for having an unused parameter in the first place are less applicable for this lint. Either the existence of the type has meaning, or API compatibility. The former is a pattern definitely used for self, but not really on recursive functions. The later is possible, but seems unlikely to occur outside of trait functions (for which self is already ignored).

[xFrednet]

I'll leave this PR to you @Alexendoo. Please don't hesitate to reach out if you need any assistance.

r? @Alexendoo

[Jarcho]

Should be fine now.

[Alexendoo]

Thanks! Just one last thing, a couple references to block_lint remain

[Jarcho]

Done.

[bors]

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

[Alexendoo]

Thanks!

@bors r+

[bors]

📌 Commit 39f4bee has been approved by Alexendoo

It is now in the queue for this repository.

[bors]

⌛ Testing commit 39f4bee with merge 3a54117...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Alexendoo
Pushing 3a54117 to master...

[danieleades]

this PR doesn't resolve the false positive here - marshallpierce/rust-base64#188