Skip to content
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

Add extra check to format_impl lint for self.fmt() #8678

Closed
wants to merge 1 commit into from

Conversation

jamesmcm
Copy link
Contributor

As mentioned in this Reddit comment:
https://www.reddit.com/r/rust/comments/tq602q/my_first_clippy_lint_detecting_use_of_self_as/i2fhdy1/

Previously use of self.fmt within the Format trait wasn't detected

Note we don't check fully qualified paths atm, but that shouldn't happen by accident anyway.

changelog: format_impl lint now checks for recursive use of self.fmt() too.

@rust-highfive
Copy link

r? @giraffate

(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 Apr 10, 2022
Copy link
Contributor

@giraffate giraffate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this occur regardless of Display or Debug trait? It appears to be simply a recursive call.

If so, it would be better to implement this as a new lint. It may be similar to #2624.

@jamesmcm
Copy link
Contributor Author

That's a good point, and yeah that would catch it too and is a more general solution.

The only difficult part there is writing the lint so it works for general functions and trait methods.

As mentioned in this Reddit comment:
https://www.reddit.com/r/rust/comments/tq602q/my_first_clippy_lint_detecting_use_of_self_as/i2fhdy1/

Previously use of self.fmt within the Format trait wasn't detected

Note we don't check fully qualified paths atm, but that shouldn't happen
by accident anyway.
@jamesmcm
Copy link
Contributor Author

It'd be similar to #8422 but that doesn't catch the case of ever having identical recursion e.g. recursing with exactly the same arguments we take in, at least when they are edited in another conditional branch atm.

i.e. it didn't trigger on:

enum Foo {
    Foo(usize),
    Bar(usize),
}

impl std::fmt::Display for Foo {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        match self {
            Self::Foo(_) => self.fmt(f),
            Self::Bar(x) => x.fmt(f),
        }
    }
}

Copy link
Contributor

@giraffate giraffate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

It might be better to enhance the only_used_in_recursion lint to catch this case.

@bors
Copy link
Collaborator

bors commented May 7, 2022

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

@blyxyas
Copy link
Member

blyxyas commented Oct 6, 2023

@jamesmcm I'm going to close this for now (we're doing cleaning of old PRs ❤️) are you interested in continuing development as a fix to only_used_in_recursion?

If you're still interested in this PR, don't hesitate in re-open it and continue your work.

@blyxyas blyxyas closed this Oct 6, 2023
@flip1995 flip1995 added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive-closed Status: Closed due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants