wrong_self_convention: `to_` convention respects `Copy` types #6924

mgacek8 · 2021-03-17T16:53:24Z

fixes #6727
changelog: wrong_self_convention: to_ convention respects Copy types

rust-highfive · 2021-03-17T16:53:40Z

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

mgacek8 · 2021-03-17T16:55:19Z

clippy_lints/src/methods/mod.rs

-    /// |`to_`  | not `_mut` |`&self`               |
+    /// |Prefix |Postfix     |`self` taken           | `self` type  |
+    /// |-------|------------|-----------------------|--------------|
+    /// |`as_`  | none       |`&self` or `&mut self` | any          |


I'm wondering if it wouldn't be more clear to have n/a instead of none where the Postfix doesn't matter.

I personally don't think an abbreviation of "not applicable" is clearer than "none" when there is no suffix, but that may be my dislike of needless abbreviations.

More details here: https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv

llogiq

This looks pretty solid, I just have one nitpick in the tests.

llogiq · 2021-03-18T08:39:27Z

tests/ui/use_self.fixed

@@ -79,6 +79,7 @@ mod issue2894 {
    }

    // This should not be linted


If this comment is correct, then why the #[allow(..)] below?

llogiq · 2021-03-18T08:40:34Z

tests/ui/use_self.rs

@@ -79,6 +79,7 @@ mod issue2894 {
    }

    // This should not be linted


Same here. I get that it's probably a different lint that should not trigger, but it still feels wrong.

Yes, the comment says that use_self shouldn't trigger.
I added the #[allow(clippy::wrong_self_convention)] to suppress the warning that occurred there, after current changes. I wasn't sure whether I should adjust the self convention there.
Agree it's better to adjust that uitest to the proper self convention, which I did in the next commit.

llogiq

I had another look and found another nit to pick 😉

llogiq · 2021-03-18T15:57:47Z

tests/ui/wrong_self_convention.stderr

@@ -191,5 +175,21 @@ LL |         fn from_i32(self);
   |
   = help: consider choosing a less ambiguous name

+error: methods with the following characteristics: (`to_*` and `self type is  Copy`) usually take self by value


Looking once more at the resulting error message, this gave me pause: "`self type is Copy`" should probably read "`self` type is `Copy`" (note the double space and the backticks).

Good catch, thanks!

llogiq

This is almost it... Just remove the "s from the characteristics.

llogiq · 2021-03-19T09:07:37Z

tests/ui/wrong_self_convention.stderr

  --> $DIR/wrong_self_convention.rs:40:15
   |
 LL |     fn is_i32(self) {}
   |               ^^^^
   |
   = help: consider choosing a less ambiguous name

-error: methods called `to_*` usually take self by reference
+error: methods with the following characteristics: ("to_*" and "`self` type is not `Copy`") usually take self by reference


Oops, I think you copied the " signs, which I only used to split the characteristics from the sentence.

One thing I'm not sure about now, should to_* have backticks or not?
E.g. this way:
error: methods with the following characteristics: (to_* and self type is not Copy) usually take self by reference
Or this:
error: methods with the following characteristics: (to_* and self type is not Copy) usually take self by reference

I think it should be the latter, to drive home that this should match a method name. We could also backtick the self in "usually take self by reference".

llogiq · 2021-03-20T06:59:08Z

Thank you! @bors r+

bors · 2021-03-20T06:59:09Z

📌 Commit 1f2d016 has been approved by llogiq

bors · 2021-03-20T06:59:16Z

⌛ Testing commit 1f2d016 with merge 1d3c539...

bors · 2021-03-20T07:12:54Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 1d3c539 to master...

Fix FP in `wrong_self_convention` lint Previously, this lint didn't check into impl block when it was implementing a trait. Recent improvements (#6924) have moved this check and some impl blocks are now checked but they shouldn't, such as in #7032. Fixes #7032 changelog: Fix FP when not taking `self` in impl block for `wrong_self_convention` lint

rust-highfive assigned llogiq Mar 17, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 17, 2021

mgacek8 commented Mar 17, 2021

View reviewed changes

mgacek8 force-pushed the issue6727_copy_types branch from 0bc7ac1 to e1abe44 Compare March 17, 2021 17:12

wrong_self_convention: to_ respects Copy types

ea15fb2

More details here: https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv

mgacek8 force-pushed the issue6727_copy_types branch from e1abe44 to ea15fb2 Compare March 17, 2021 21:01

llogiq reviewed Mar 18, 2021

View reviewed changes

Adjust use_self uitest to proper self convention

032cdfe

llogiq reviewed Mar 18, 2021

View reviewed changes

mgacek8 force-pushed the issue6727_copy_types branch from 980dbd3 to 39cabcd Compare March 19, 2021 08:13

llogiq reviewed Mar 19, 2021

View reviewed changes

mgacek8 force-pushed the issue6727_copy_types branch from 39cabcd to b6175cd Compare March 19, 2021 17:39

wrong_self_convention: Enhance lint message

1f2d016

mgacek8 force-pushed the issue6727_copy_types branch from b6175cd to 1f2d016 Compare March 19, 2021 19:30

bors merged commit 1d3c539 into rust-lang:master Mar 20, 2021

mgacek8 deleted the issue6727_copy_types branch March 20, 2021 10:35

dtolnay mentioned this pull request Mar 27, 2021

False positive wrong_self_convention inside trait impls #6983

Closed

ThibsG mentioned this pull request Apr 11, 2021

Fix FP in wrong_self_convention lint #7064

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wrong_self_convention: `to_` convention respects `Copy` types #6924

wrong_self_convention: `to_` convention respects `Copy` types #6924

mgacek8 commented Mar 17, 2021

rust-highfive commented Mar 17, 2021

mgacek8 Mar 17, 2021

llogiq Mar 18, 2021

llogiq left a comment

llogiq Mar 18, 2021

llogiq Mar 18, 2021

mgacek8 Mar 18, 2021

llogiq left a comment

llogiq Mar 18, 2021

mgacek8 Mar 18, 2021

llogiq left a comment

llogiq Mar 19, 2021

mgacek8 Mar 19, 2021

llogiq Mar 19, 2021

llogiq commented Mar 20, 2021

bors commented Mar 20, 2021

bors commented Mar 20, 2021

bors commented Mar 20, 2021

		@@ -79,6 +79,7 @@ mod issue2894 {
		}

		// This should not be linted

wrong_self_convention: to_ convention respects Copy types #6924

wrong_self_convention: to_ convention respects Copy types #6924

Conversation

mgacek8 commented Mar 17, 2021

rust-highfive commented Mar 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llogiq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llogiq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llogiq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llogiq commented Mar 20, 2021

bors commented Mar 20, 2021

bors commented Mar 20, 2021

bors commented Mar 20, 2021

wrong_self_convention: `to_` convention respects `Copy` types #6924

wrong_self_convention: `to_` convention respects `Copy` types #6924