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

New lint: same_name_method #7653

Merged
merged 1 commit into from
Sep 17, 2021
Merged

Conversation

lengyijun
Copy link
Contributor

@lengyijun lengyijun commented Sep 10, 2021

changelog: [`same_name_method`]
fix: #7632

It only compares a method in impl with another in impl trait for
It doesn't lint two methods in two traits.

I'm not sure my approach is the best way. I meet difficulty in other approaches.

@rust-highfive
Copy link

r? @llogiq

(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 Sep 10, 2021
@lengyijun lengyijun force-pushed the same_name_method_crate branch 5 times, most recently from 11f6fb7 to c81cd9a Compare September 10, 2021 04:46
@lengyijun lengyijun force-pushed the same_name_method_crate branch 3 times, most recently from adceb5f to 9e3ff2a Compare September 11, 2021 14:08
@llogiq
Copy link
Contributor

llogiq commented Sep 11, 2021

Apart from making this a restriction lint, I think this looks merge-worthy. I don't care too much about struct names in tests.

@lengyijun
Copy link
Contributor Author

Thx! I have removed the todos.
Although not satisfied with restriction, but being accepted is also a good start for this lint.

@llogiq
Copy link
Contributor

llogiq commented Sep 12, 2021

What's your take on restriction vs. pedantic? Why would you not be satisfied with the former?

@lengyijun
Copy link
Contributor Author

I have no deep insight into the category of lint. Please help we with this hard decision !
I just want this lint can be used more and get more feedback.

@llogiq
Copy link
Contributor

llogiq commented Sep 12, 2021

The idea behind pedantic is to group lints that are deemed beneficial to code style, but have enough false positives not to warn by default. restriction lints are for things that are not universally seen as problematic, but may be in some contexts.

As I don't see how your lints could have false positives, yet depending on the context people may choose to ignore it, this fits the restriction category somewhat better than pedantic.

@lengyijun
Copy link
Contributor Author

I agree on restriction now.

@llogiq
Copy link
Contributor

llogiq commented Sep 12, 2021

r=me with the changed lint category 👍

@lengyijun lengyijun force-pushed the same_name_method_crate branch from 9e3ff2a to 72b133c Compare September 14, 2021 00:02
@lengyijun lengyijun force-pushed the same_name_method_crate branch from 72b133c to e2cdaec Compare September 14, 2021 01:20
@lengyijun
Copy link
Contributor Author

@llogiq

@llogiq
Copy link
Contributor

llogiq commented Sep 17, 2021

Thank you @lengyijun ! @bors r+

@bors
Copy link
Contributor

bors commented Sep 17, 2021

📌 Commit e2cdaec has been approved by llogiq

@bors
Copy link
Contributor

bors commented Sep 17, 2021

⌛ Testing commit e2cdaec with merge ed7a82e...

@bors
Copy link
Contributor

bors commented Sep 17, 2021

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

@bors bors merged commit ed7a82e into rust-lang:master Sep 17, 2021
@Zenithsiz
Copy link

I'm not sure this is the correct place to comment, as it's already closed, but I feel as if this lint shouldn't trigger if the impl Type function has less arguments than the impl Trait for Type one (given all other parameters are the same).
I use that pattern when an extra unneeded argument is supplied by the trait that I don't require for the implementation. This allows other types to call the inherent function without providing the unneeded argument that won't be used any way.

Example:

trait T {
  fn do(&self, a: u32, b: u32);
}

struct A;

impl A {
  fn do(&self, a: u32) { ... }
}

impl T for A {
  fn do(&self, a: u32, _b: u32) { Self::do(self, a) }
}

@lengyijun lengyijun deleted the same_name_method_crate branch October 27, 2023 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

method_duplicate_name
7 participants