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.

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

wrong_self_convention: fix lint in case of to_*_mut method #6828

Merged
merged 1 commit into from
Mar 15, 2021
Merged

wrong_self_convention: fix lint in case of to_*_mut method #6828

merged 1 commit into from
Mar 15, 2021

Conversation

mgacek8
Copy link
Contributor

@mgacek8 mgacek8 commented Mar 2, 2021

fixes #6758
changelog: wrong_self_convention: fix lint in case of to_*_mut method. When a method starts with to_ and ends with _mut, clippy expects a &mut self parameter, otherwise &self.

Any feedback is welcome. I was also thinking if shouldn't we treat to_ the same way as as_. Namely to accept self taken: &self or &mut self.

@rust-highfive
Copy link

r? @flip1995

(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 Mar 2, 2021
flip1995
flip1995 reviewed Mar 10, 2021
View reviewed changes
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

LGTM, except a NIT in the message I don't quite understand the reason.

tests/ui/wrong_self_convention.stderr Outdated Show resolved Hide resolved
@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 Mar 10, 2021
flip1995
flip1995 approved these changes Mar 11, 2021
View reviewed changes
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

LGTM. Just one low hanging fruit, it would be great if you could address this in this PR.

clippy_lints/src/methods/mod.rs Outdated
Comment on lines 1957 to 1962
span_lint(
cx,
lint,
first_arg_span,
&format!(
"methods called `{}` usually take {}; consider choosing a less ambiguous name",
conv,
"{} usually take {}; consider choosing a less ambiguous name",
Copy link
Member

Choose a reason for hiding this comment

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

While your at it: Could you change this to span_lint_and_help please? The consider choosing a less ambiguous name part should not be in the error message, but in a help message.

@mgacek8 mgacek8 force-pushed the issue_6758_enhance_wrong_self_convention branch 2 times, most recently from c403d14 to e077208 Compare March 11, 2021 13:49
@mgacek8
Copy link
Contributor Author

mgacek8 commented Mar 11, 2021

ui/def_id_nocore.rs failed for me on the CI after switching to span_lint_and_help. I was running tests locally on Windows, but this one has ignore-windows directive. Now it is fixed.

Another error now:

Error: stderr files exceeding limit of 200 lines:
/home/runner/work/rust-clippy/rust-clippy/tests/ui/wrong_self_convention.stderr: 211

@flip1995 should I somehow make that warning go away, or e.g. split the file?

@flip1995
Copy link
Member

flip1995 commented Mar 11, 2021 •
edited
Loading

Just split out the new test cases into a file wrong_self_conventions_mut.rs.

Also dogfood fails, which isn't run on windows either (for $reasons).

ui/def_id_nocore.rs failed for me on the CI after switching to span_lint_and_help. I was running tests locally on Windows, but this one has ignore-windows directive. Now it is fixed.

That should be fine.

@mgacek8 mgacek8 force-pushed the issue_6758_enhance_wrong_self_convention branch 2 times, most recently from 5ce0bff to c6d6431 Compare March 11, 2021 19:47
@mgacek8
Copy link
Contributor Author

mgacek8 commented Mar 11, 2021

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Mar 11, 2021
@bors
Copy link
Collaborator

bors commented Mar 11, 2021

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

@flip1995
Copy link
Member

This LGTM now. Unfortunately #6826 was just merged, which moved lints to their own file inside the methods module. I think the easiest way to resolve the merge conflicts would be to start with a new branch based on the current master, apply the changes you did to mod.rs to the new file wrong_self_convention.rs, copy the tests over and then force push to the branch of this PR (you can do this with git push origin HEAD:issue_6758_enhance_wrong_self_convention assuming you checked out the new branch).

Or you could pray that git is smart enough to figure this conflict out in a sane way. But I doubt that.

If you need help with that or don't want to deal with this conflict, let me know and I can do it for you.

@mgacek8
wrong_self_convention: fix lint in case of to_*_mut method
2547edb 
When a method starts with `to_` and ends with `_mut`, it should expect a `&mut self` parameter,
otherwise `&self`.
@mgacek8 mgacek8 force-pushed the issue_6758_enhance_wrong_self_convention branch from c6d6431 to 2547edb Compare March 12, 2021 08:05
@mgacek8
Copy link
Contributor Author

mgacek8 commented Mar 12, 2021

Resolved conflicts. Happy to see that refactoring in place!

@flip1995
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Mar 15, 2021

📌 Commit 2547edb has been approved by flip1995

@bors
Copy link
Collaborator

bors commented Mar 15, 2021

⌛ Testing commit 2547edb with merge 0929a24...

@bors
Copy link
Collaborator

bors commented Mar 15, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 0929a24 to master...

@bors bors merged commit 0929a24 into rust-lang:master Mar 15, 2021
@mgacek8 mgacek8 deleted the issue_6758_enhance_wrong_self_convention branch March 20, 2021 10:37
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

Assignees

@flip1995 flip1995

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Wrong lint in case of to_*_mut method
5 participants
@mgacek8 @rust-highfive @flip1995 @bors @rustbot