Skip to content

clippy::only_used_in_recursion considers self, but unused_variables does not #10370

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.

Sign up for GitHub

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

Jump to bottom
Open
PatchMixolydic opened this issue Feb 18, 2023 · 3 comments · May be fixed by #14787
Open

clippy::only_used_in_recursion considers self, but unused_variables does not #10370

PatchMixolydic opened this issue Feb 18, 2023 · 3 comments · May be fixed by #14787
Assignees
@pommicket
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have

Comments

@PatchMixolydic
Copy link
Contributor

PatchMixolydic commented Feb 18, 2023 •
edited by rustbot
Loading

Summary

clippy::only_used_in_recursion can trip on self in methods. While this is technically correct, it is inconsistent with rustc's unused_variables lint.

Lint Name

clippy::only_used_in_recursion

Reproducer

I tried this code (playground):

pub enum Expression {
    Path(usize),
    Loop(Option<Box<Expression>>),
    FnCall(Box<Expression>, Vec<Box<Expression>>),
}

impl Expression {
    pub fn is_intrinsic(&self) -> bool {
        todo!()
    }
}

pub struct LowerState;

impl LowerState {
    fn find_id(&self, expr: &Expression) -> Option<usize> {
        match expr {
            Expression::Path(id) => Some(*id),
            Expression::FnCall(_, _) => None,
            Expression::Loop(body) => body.as_ref().and_then(|expr| self.find_id(expr)),
        }
    }

    pub fn lower_expression(&self, expr: Expression) {
        match &expr {
            Expression::Loop(_) => todo!(),
            Expression::Path(_) => todo!(),

            Expression::FnCall(f, _args) => {
                if expr.is_intrinsic() {
                    let _id = self.find_id(f);
                    todo!()
                }

                todo!()
            }
        }
    }
}

I saw this happen:

warning: parameter is only used in recursion
  --> src/lib.rs:16:17
   |
16 |     fn find_id(&self, expr: &Expression) -> Option<usize> {
   |                 ^^^^
   |
note: parameter used here
  --> src/lib.rs:20:69
   |
20 |             Expression::Loop(body) => body.as_ref().and_then(|expr| self.find_id(expr)),
   |                                                                     ^^^^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#only_used_in_recursion
   = note: `#[warn(clippy::only_used_in_recursion)]` on by default

I expected to see this happen: No warnings

Note that if the body of LowerState::find_id is replaced with todo!(), self is not marked as unused (playground):

// ...

impl LowerState {
    fn find_id(&self, expr: &Expression) -> Option<usize> {
        todo!()
    }

    // ...
}
warning: unused variable: `expr`
  --> src/lib.rs:16:23
   |
16 |     fn find_id(&self, expr: &Expression) -> Option<usize> {
   |                       ^^^^ help: if this is intentional, prefix it with an underscore: `_expr`
   |
   = note: `#[warn(unused_variables)]` on by default

Version

rustc 1.69.0-nightly (0416b1a6f 2023-02-14)
binary: rustc
commit-hash: 0416b1a6f6d5c42696494e1a3a33580fd3f669d8
commit-date: 2023-02-14
host: x86_64-unknown-linux-gnu
release: 1.69.0-nightly
LLVM version: 15.0.7

Additional Labels

No response
@PatchMixolydic PatchMixolydic added C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have labels Feb 18, 2023
@J-ZhengLi
Copy link
Member

J-ZhengLi commented Feb 24, 2023 •
edited
Loading

I think this lint shouldn't consider self as it's part of the characteristic of the method itself, so it's not inconsistent, just incorrect 😆 , I'll claim it for now and see what I can do.
@rustbot claim

@Jarcho
Copy link
Contributor

Jarcho commented Feb 24, 2023

Whether or not to lint self is a tradeoff. It doesn't necessarily indicate a bug the same as other parameters, but it also can't be renamed to _self like others can. This makes ignoring the lint much wordier.

Not using it can be for two reasons:

  1. Using method call syntax, but not actually needing self.
  2. Holding a value of the given type has meaning.

The first is basically meaningless for functions which can use Self:: instead, but could potentially reduce noise from other call sites. This applies equally for this lint and unused_variables.

The second is mainly used for handle/token types where recursion is much less likely to occur. Especially when the Self type is the handle/token. For unused_variables it makes sense to err on the side of allowing it since it is a common pattern. This isn't really the case for only_used_in_recursion, hence the reason for the inconsistency between the two of them.

@J-ZhengLi J-ZhengLi removed their assignment Feb 24, 2023
@pommicket
Copy link

pommicket commented May 11, 2025 •
edited
Loading

@rustbot claim

Personally I think it's strange that by default

impl Foo {
     fn x(&self, n: i8) -> i32 {
         if n >= 0 { 1 } else { -1 }
     }
}

is allowed but

impl Foo {
     fn x(&self, n: i8) -> i32 {
         if n >= 0 { 1 } else { -self.x(-n) }
     }
}

gives a warning. I am thinking of trying to add a separate clippy::self_only_used_in_recursion lint, which can be a "pedantic" lint, like clippy::unused_self.
But if anyone thinks that's not a good idea, let me know.

@pommicket pommicket linked a pull request May 12, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@pommicket pommicket

Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add clippy::self_only_used_in_recursion lint pommicket/rust-clippy
4 participants
@Jarcho @PatchMixolydic @pommicket @J-ZhengLi