should_impl_trait - ignore methods with lifetime params #5725

tnielens · 2020-06-16T23:20:25Z

Fixes: #5617

changelog: don't lint should_implement_trait when an Iterator::next case has explicit parameters

rust-highfive · 2020-06-16T23:20:28Z

r? @yaahc

(rust_highfive has picked a reviewer for you, use r? to override)

yaahc

Looks good, just needs a changelog: added to the body of the PR

cc @flip1995

clippy_lints/src/methods/mod.rs

ghost · 2020-06-17T05:41:14Z

Does this check handle elided lifetimes?

    pub fn next<'b>(&'b mut self) -> Option<&'b mut T> {
        unimplemented!();
    }

is equivalent to

    pub fn next(&mut self) -> Option<&mut T> {
        unimplemented!();
    }

but will the second form be linted?

Also, I think this check should only apply to the next method specifally.
A method like

    fn eq<'a, 'b>(&'a self, other: &'b Self) -> bool { unimplemented!() }

can be moved to a PartialEq implementation regardless of lifetimes.

tnielens · 2020-06-18T09:21:07Z

The short-circuiting upon lifetime params is a little naive indeed.

I'll start by adding a test case for each trait method in the lint method list and see from there.

tnielens · 2020-06-19T20:25:10Z

I've added a test case for each checked method. All cases can be written with lifetime elision. If the user persists in writing explicitly lifetime params, he'll first hit the needles_lifetime lint.

tnielens · 2020-06-19T20:43:06Z

Does this check handle elided lifetimes?

It's a check done at hir level. Elided liftetimes are absent from the function declaration struct it seems.

clippy_lints/src/methods/mod.rs

tnielens · 2020-06-23T19:51:41Z

Three approaches to wrap this pr up:

This is a rare case, the user can explicitly allow the lint. Revert the lifetime filtering and keep test cases.
All trait methods implementations can be written with lifetime elision. The user is unlikely to write lifetimes explicitly down. If it does so he might have a good reason (like for the Iterator::next example, rust hasn't shipped GATs, the Iterator trait cannot model streaming iterators). Clippy should not lint these more complex cases as there is a risk of false positive. The pr is good so.
Iterator::next is a special case. Filter on explicit lifetimes only for that one. Other cases remain unchanged.

Any you'd agree with?

flip1995

Iterator::next is a special case. Filter on explicit lifetimes only for that one. Other cases remain unchanged.

Until now, I only remember issues complaining about the next method. To future prove this fix, you could add another field to the TRAIT_METHODS list, that specifies if the lint should be skipped, if there are lifetimes defined.

Extra points if you write a doc-comment for this list of tuples, what each field stands for (please :) ).

Sorry for ignoring this PR for so long, I didn't realize, that I was CCed.

tests/ui/methods.rs

tnielens · 2020-07-29T23:50:08Z

I changed the TRAIT_METHODS items from a large tuple to a new struct. That clarifies a bit the role of each field.

tnielens · 2020-07-30T00:12:37Z

Do you mind if I introduce an exclusion rule for the stderr line count limit?

flip1995

Please don't use double underscores for test names. If you want to structure the tests better rather put them in their own directory.

tests/ui/methods.rs

tests/ui/should_impl_trait__corner_cases.stderr

tests/ui/should_impl_trait__method_list.rs

clippy_lints/src/methods/mod.rs

flip1995 · 2020-07-31T11:51:28Z

Can you put the tests for this lint in its own directory? There's still an issue with undefined structs in the test cases.

flip1995 · 2020-07-31T20:22:28Z

Please remove the tests/ui/should_impl_trait/corner_cases.stderr file and this should be good to go.

flip1995 · 2020-08-04T09:12:13Z

@bors r=flip1995,yaahc rollup=always

bors · 2020-08-04T09:12:14Z

📌 Commit 108032e has been approved by flip1995,yaahc

…95,yaahc should_impl_trait - ignore methods with lifetime params Fixes: rust-lang#5617 changelog: don't lint should_implement_trait when an `Iterator::next` case has explicit parameters

bors · 2020-08-04T09:14:39Z

⌛ Testing commit 108032e with merge 9df7e19...

should_impl_trait - ignore methods with lifetime params Fixes: #5617 changelog: don't lint should_implement_trait when an `Iterator::next` case has explicit parameters

bors · 2020-08-04T09:19:55Z

💔 Test failed - checks-action_test

@ghost

Rollup of 6 pull requests Successful merges: - #5725 (should_impl_trait - ignore methods with lifetime params) - #5837 (needless_collect: catch x: Vec<_> = iter.collect(); x.into_iter() ...) - #5846 (Handle mapping to Option in `map_flatten` lint) - #5848 (Add derive_ord_xor_partial_ord lint) - #5852 (Add lint for duplicate methods of trait bounds) - #5856 (Remove old Symbol reexport) Failed merges: r? @ghost changelog: rollup

@ghost

Rollup of 6 pull requests Successful merges: - #5725 (should_impl_trait - ignore methods with lifetime params) - #5837 (needless_collect: catch x: Vec<_> = iter.collect(); x.into_iter() ...) - #5846 (Handle mapping to Option in `map_flatten` lint) - #5848 (Add derive_ord_xor_partial_ord lint) - #5852 (Add lint for duplicate methods of trait bounds) - #5856 (Remove old Symbol reexport) Failed merges: r? @ghost changelog: rollup

flip1995 · 2020-08-04T10:05:00Z

Please fix the dogfood errors.

Outdated

flip1995

Thanks! Waiting for rustup

flip1995 · 2020-08-16T16:29:25Z

@bors r+

Thanks!

bors · 2020-08-16T16:29:26Z

📌 Commit f9ba829 has been approved by flip1995

bors · 2020-08-16T17:51:22Z

⌛ Testing commit f9ba829 with merge 3bd9889...

bors · 2020-08-16T18:11:02Z

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

rust-highfive assigned yaahc Jun 16, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 16, 2020

yaahc previously requested changes Jun 16, 2020

View reviewed changes

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved

yaahc 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 Jun 17, 2020

ebroto reviewed Jun 19, 2020

View reviewed changes

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved

flip1995 reviewed Jul 26, 2020

View reviewed changes

tests/ui/methods.rs Outdated Show resolved Hide resolved

tnielens force-pushed the should-impl-trait branch 3 times, most recently from 88e5dda to f300fcd Compare July 29, 2020 23:46

tnielens force-pushed the should-impl-trait branch 3 times, most recently from 3129220 to d8f898b Compare July 30, 2020 00:08

flip1995 reviewed Jul 30, 2020

View reviewed changes

tests/ui/methods.rs Show resolved Hide resolved

tests/ui/should_impl_trait__corner_cases.stderr Outdated Show resolved Hide resolved

tests/ui/should_impl_trait__method_list.rs Outdated Show resolved Hide resolved

flip1995 reviewed Jul 30, 2020

View reviewed changes

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved

tnielens force-pushed the should-impl-trait branch from e3ef2e6 to f83fad8 Compare July 31, 2020 20:17

tnielens force-pushed the should-impl-trait branch from f83fad8 to 108032e Compare July 31, 2020 20:34

flip1995 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 Jul 31, 2020

flip1995 unassigned yaahc Jul 31, 2020

flip1995 mentioned this pull request Aug 4, 2020

Rollup of 6 pull requests #5866

Closed

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-bors Status: The marked PR was approved and is only waiting bors labels Aug 4, 2020

tnielens added 5 commits August 9, 2020 15:10

should_impl_trait - ignore methods with lifetime params

a77e881

should_implement_trait - add test cases for every checked trait method

2bc0ecd

should_implement_trait - pr remarks

e6b2254

should_implement_trait - filter on explicit lifetime param only

7cc1a2e

should_impl_trait - pr comments

166c520

tnielens force-pushed the should-impl-trait branch from 108032e to 166c520 Compare August 9, 2020 13:10

should_impl_trait - self linting

f9ba829

tnielens force-pushed the should-impl-trait branch from 2191ec9 to f9ba829 Compare August 9, 2020 14:09

yaahc requested a review from flip1995 August 10, 2020 19:13

flip1995 approved these changes Aug 12, 2020

View reviewed changes

flip1995 added S-waiting-on-bors Status: The marked PR was approved and is only waiting bors and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Aug 12, 2020

bors merged commit 3bd9889 into rust-lang:master Aug 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

should_impl_trait - ignore methods with lifetime params #5725

should_impl_trait - ignore methods with lifetime params #5725

tnielens commented Jun 16, 2020 •

edited

Loading

rust-highfive commented Jun 16, 2020

yaahc left a comment •

edited

Loading

ghost commented Jun 17, 2020 •

edited by ghost

Loading

tnielens commented Jun 18, 2020

tnielens commented Jun 19, 2020

tnielens commented Jun 19, 2020 •

edited

Loading

tnielens commented Jun 23, 2020

flip1995 left a comment

tnielens commented Jul 29, 2020

tnielens commented Jul 30, 2020 •

edited

Loading

flip1995 left a comment

flip1995 commented Jul 31, 2020

flip1995 commented Jul 31, 2020

flip1995 commented Aug 4, 2020

bors commented Aug 4, 2020

bors commented Aug 4, 2020

bors commented Aug 4, 2020

flip1995 commented Aug 4, 2020

flip1995 left a comment

flip1995 commented Aug 16, 2020

bors commented Aug 16, 2020

bors commented Aug 16, 2020

bors commented Aug 16, 2020

should_impl_trait - ignore methods with lifetime params #5725

should_impl_trait - ignore methods with lifetime params #5725

Conversation

tnielens commented Jun 16, 2020 • edited Loading

rust-highfive commented Jun 16, 2020

yaahc left a comment • edited Loading

Choose a reason for hiding this comment

ghost commented Jun 17, 2020 • edited by ghost Loading

tnielens commented Jun 18, 2020

tnielens commented Jun 19, 2020

tnielens commented Jun 19, 2020 • edited Loading

tnielens commented Jun 23, 2020

flip1995 left a comment

Choose a reason for hiding this comment

tnielens commented Jul 29, 2020

tnielens commented Jul 30, 2020 • edited Loading

flip1995 left a comment

Choose a reason for hiding this comment

flip1995 commented Jul 31, 2020

flip1995 commented Jul 31, 2020

flip1995 commented Aug 4, 2020

bors commented Aug 4, 2020

bors commented Aug 4, 2020

bors commented Aug 4, 2020

flip1995 commented Aug 4, 2020

flip1995 left a comment

Choose a reason for hiding this comment

flip1995 commented Aug 16, 2020

bors commented Aug 16, 2020

bors commented Aug 16, 2020

bors commented Aug 16, 2020

tnielens commented Jun 16, 2020 •

edited

Loading

yaahc left a comment •

edited

Loading

ghost commented Jun 17, 2020 •

edited by ghost

Loading

tnielens commented Jun 19, 2020 •

edited

Loading

tnielens commented Jul 30, 2020 •

edited

Loading