Skip to content

Add a lint for unused self - #4619

Merged
bors merged 5 commits into
rust-lang:masterfrom
james9909:unused-self
Oct 15, 2019
Merged

Add a lint for unused self#4619
bors merged 5 commits into
rust-lang:masterfrom
james9909:unused-self

Conversation

@james9909

@james9909 james9909 commented Oct 3, 2019

Copy link
Copy Markdown
Contributor

changelog: Adds a new lint: unused_self

Closes #4550.

@iddm

iddm commented Oct 3, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

I'd also add a negative test, at least for traits - that we don't lint unused_self within traits.

@james9909

james9909 commented Oct 3, 2019

Copy link
Copy Markdown
Contributor Author

There's already a test for the Debug trait, should I add more?

@phansch phansch added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 4, 2019
@bors

bors commented Oct 4, 2019

Copy link
Copy Markdown
Contributor

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

@iddm

iddm commented Oct 4, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

There's already a test for the Debug trait, should I add more?

Sorry, missed it. If you mean this:

mod not_applicable {
    use std::fmt;

    struct A {}

    impl fmt::Debug for A {
        fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
            write!(f, "A")
        }
    }

    impl A {
        fn method(x: u8, y: u8) {}
    }
}

Then I have a question - does your implementation checks only impl or trait as well?
What about this code, how will it work?

trait A {
    fn method(&self) -> u64 { }
}

Will unused self be linted? Just to be sure. Thanks!

flip1995
flip1995 requested changes Oct 4, 2019
View reviewed changes

@flip1995 flip1995 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Impl LGTM. Can you remove the file tests/ui/unused_self.stdout?

Also once this lint is pedantic, it should be possible to remove the allows from the unrelated tests.

Comment thread clippy_lints/src/unused_self.rs Outdated
Comment thread clippy_lints/src/unused_self.rs Outdated
Comment thread clippy_lints/src/unused_self.rs Outdated
@flip1995 flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Oct 4, 2019
@james9909

james9909 commented Oct 4, 2019

Copy link
Copy Markdown
Contributor Author

Will unused self be linted? Just to be sure. Thanks!

No, the lint here won't be triggered but I'll add this to the tests.

flip1995
flip1995 approved these changes Oct 7, 2019
View reviewed changes

@flip1995 flip1995 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

@bors

bors commented Oct 8, 2019

Copy link
Copy Markdown
Contributor

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

Centril
Centril reviewed Oct 14, 2019
View reviewed changes
Comment thread clippy_lints/src/unused_self.rs Outdated
@bors

bors commented Oct 14, 2019

Copy link
Copy Markdown
Contributor

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

@flip1995

flip1995 commented Oct 15, 2019

Copy link
Copy Markdown
Member

Rebased to get rid of the merge commits. Thanks for implementing this lint!

@bors r+

@bors

bors commented Oct 15, 2019

Copy link
Copy Markdown
Contributor

📌 Commit adf5886 has been approved by flip1995

@bors

bors commented Oct 15, 2019

Copy link
Copy Markdown
Contributor

⌛ Testing commit adf5886 with merge 55e7818...

bors added a commit that referenced this pull request Oct 15, 2019
@bors
Auto merge of #4619 - james9909:unused-self, r=flip1995
55e7818 
Add a lint for unused self

changelog: Adds a new lint: `unused_self`

Closes #4550.
@bors

bors commented Oct 15, 2019

Copy link
Copy Markdown
Contributor

☀️ Test successful - checks-travis, status-appveyor
Approved by: flip1995
Pushing 55e7818 to master...

@bors
bors merged commit adf5886 into rust-lang:master Oct 15, 2019
@bors bors mentioned this pull request Oct 15, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@flip1995 flip1995 flip1995 approved these changes
+2 more reviewers
@Centril Centril Centril left review comments
@iddm iddm iddm left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Lint methods which have "self" argument but don't use it.

6 participants

@james9909 @iddm @bors @flip1995 @Centril @phansch