Initial implementation of possible shortcircuiting collect lint by marcusklaas · Pull Request #2077 · rust-lang/rust-clippy

marcusklaas · 2017-09-20T03:53:30Z

This lint detects collect calls that generate collections of Option or Result and suggests instead collect into a Option<C> or Result<C, _> respectively, which has personally almost always been what I intend to do. Since this shortcircuiting feature of collect is not very discoverable, I believe this lint has some value.

There may be cases where one really does want a collection of result/ option, in which case this lint will produce a false positive. How should we deal with those? Below is a non-exhaustive list of options:

make the lint allow by default
only trigger lint when the iterator item type and its enclosing function type match (either both options or of some type Result<_, E>)
do not trigger lint when it is explicitly stated that we want a collection of options/ results, for example in

let collection: Vec<Option<_>> = some_iter().collect();

Oh, and mega thanks to @eddyb for his gracious guidance through the labyrinths of the rust compiler on the rust IRC!

oli-obk

I'd say we go with an allow by default lint until we've run it on a few codebases

oli-obk · 2017-09-20T08:23:00Z

+26 |     let _result: Vec<_> = a.iter().map(Some).collect();
+   |                  ^^^^^^
+   |
+help: if you are only interested in the `Some` values or the first `None`, try


The message about the first None seems a little weird

Agreed. Fixed.

oli-obk · 2017-09-20T08:24:05Z

+                    error_variant: "None",
+                })
+            } else if match_type(cx, normal_ty, &paths::RESULT) {
+                Some(Suggestion {


Why are these lowercase?

oli-obk · 2017-10-26T07:05:40Z

ping @marcusklaas

marcusklaas · 2017-10-26T10:15:23Z

Hi! Sorry about leaving this for such a long time. Given this lint's inherent propensity of generating false positives, maybe it's better to just close this?

oli-obk · 2017-10-26T10:18:00Z

Given this lint's inherent propensity of generating false positives, maybe it's better to just close this?

I'd have added it as allow-by-default for now. I like the idea, and maybe we can run it and figure out in which cases we don't want to lint and add them as exceptions

marcusklaas · 2017-10-26T10:19:33Z

Okay. That sounds fair. I will touch up this PR then!

marcusklaas · 2017-10-26T14:03:27Z

Build failure seems unrelated to PR. Master branch also fails to build.

oli-obk · 2018-04-03T20:34:51Z

Could you rebase this and use the declare_clippy_lint macro and make this lint a nursery lint?

mati865 · 2018-06-07T10:21:49Z

@marcusklaas ping

marcusklaas · 2018-06-07T22:40:35Z

The code appears to be broken and I do not know nearly enough about the compiler internals to fix it. This PR is best closed I think.

…

On 7 Jun 2018, 12:21, at 12:21, "Mateusz Mikuła" ***@***.***> wrote: @marcusklaas ping -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #2077 (comment)

oli-obk · 2018-06-07T23:19:28Z

Does somebody else want to take over? idk what the brokenness is, but it would be a shame to let this PR go to waste! It was pretty close to merging already during it's history, we should be able to get it there again.

marcusklaas · 2018-06-07T23:25:16Z

Don't feel bad about it. I had fun building it. And it may only have limited usefulness.

…

On 8 Jun 2018, 01:19, at 01:19, Oliver Schneider ***@***.***> wrote: Does somebody else want to take over? idk what the brokenness is, but it would be a shame to let this PR go to waste! It was pretty close to merging already during it's history, we should be able to get it there again. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #2077 (comment)

flip1995 · 2018-06-08T11:30:36Z

Rebased it and fixed a few bugs, which caused ICEs in the ui/infinite_iter and the dogfood test.

flip1995 · 2018-06-08T11:43:27Z

This lint still produces an ICE in rustfmt (travis).

This error is produced by this line:

https://github.com/marcusklaas/rust-clippy/blob/36993f89598e45ff7007568d0d188e7311c785d8/clippy_lints/src/collect.rs#L121-L124

phansch · 2018-06-08T18:21:18Z

Judging by the ICE message and the documentation of TyCtxt.normalize_erasing_regions, I think it's safe to say that the "normalization failed" path was hit. Possibly somewhere in rustc-rayon?

phansch · 2018-12-02T10:55:38Z

Rebased it on master again, let's see if it still fails

eddyb · 2018-12-04T07:40:02Z

 fn format_suggestion_pattern<'a, 'tcx>(
    cx: &LateContext<'a, 'tcx>,
-    collection_ty: &TypeVariants,
+    collection_ty: &TyKind<'_>,


TyKind should never be used directly, you can write Ty instead.
@Manishearth @oli-obk I think clippy might have a general problem wrt this.

another box for rust-lang/rust#49509

eddyb · 2018-12-04T07:40:19Z

            buf
        },
-        TypeVariants::TyParam(p) => p.to_string(),
+        TyKind::Param(p) => p.to_string(),


You should write these as e.g. ty::Param.

phansch · 2018-12-05T07:09:46Z

Removed the usage of TyKind in 5b7c206.

The ICE in rustfmt/rls is still there and it's caused by an incorrect use of normalize_erasing_regions here:

rust-clippy/clippy_lints/src/collect.rs

Lines 124 to 130 in 4f69870

    
           let assoc_item_id = assoc_item.def_id; 
        
           let substitutions = cx.tcx.mk_substs_trait(arg_ty, &[]); 
        
           let projection = cx.tcx.mk_projection(assoc_item_id, substitutions); 
        
           let normal_ty = cx.tcx.normalize_erasing_regions( 
        
               cx.param_env, 
        
               projection, 
        
           );

I'm not sure what exactly needs to be fixed, but I will try to fix it later today.

bors · 2018-12-12T06:22:31Z

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

flip1995 · 2018-12-27T16:21:30Z

ping from triage @phansch: Any updates on this? It seems that this just needs a stderr update.

phansch · 2018-12-28T15:00:08Z

thanks @flip1995, that's news to me. I think I got stuck on fixing the ICE last time but it indeed appears to be gone. I will have another look today.

phansch · 2018-12-28T22:07:27Z

So, currently compiling rustc-rayon fails with the previous ICE. The last time I tried, I failed to extract a minimal repro from rustc-rayon but I'm going to try it again tomorrow.

bors · 2019-01-03T02:50:42Z

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

bors · 2019-01-07T18:54:33Z

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

bors · 2019-04-18T07:50:56Z

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

flip1995 · 2019-06-06T11:38:19Z

half yearly ping @phansch. Should we S-inactive-closed this PR? 😄

phansch · 2019-06-06T12:02:56Z

Yeah, makes sense! I currently don't have time to continue this. IIRC, if someone wants to continue this, they should try to find a minimal repro of the ICE that doesn't require rustc-rayon and go on fixing the remaining issues from there.

oli-obk suggested changes Sep 20, 2017

View reviewed changes

marcusklaas force-pushed the try-collect branch 3 times, most recently from d18648e to 35cbf2f Compare October 26, 2017 14:02

oli-obk approved these changes Oct 27, 2017

View reviewed changes

flip1995 force-pushed the try-collect branch from 35cbf2f to ab1d6b2 Compare June 8, 2018 11:08

phansch added the S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work label Aug 30, 2018

phansch force-pushed the try-collect branch from 36993f8 to 1ff2f3f Compare December 2, 2018 10:55

phansch removed the S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work label Dec 4, 2018

eddyb reviewed Dec 4, 2018

View reviewed changes

flip1995 mentioned this pull request Dec 4, 2018

Internal lint for usage of ty::TyKind #3493

Closed

phansch force-pushed the try-collect branch from 31a4a32 to 5b7c206 Compare December 5, 2018 06:30

phansch self-assigned this Dec 5, 2018

phansch added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Dec 6, 2018

phansch force-pushed the try-collect branch from 4f69870 to 353280f Compare December 12, 2018 07:22

marcusklaas and others added 9 commits December 28, 2018 21:21

Add missed shortcircuit lint for collect calls

9c82058

Fix bugs which produced ICEs

60f4767

Fix dogfood errors

38dab0c

Make it compile and make ui/collect.rs tests pass again

682dc65

Don't use TyKind directly, use ty instead

0cbecd8

Update UI test .stderr files

4266e9d

Add missing licenses

2bd90a4

Fix dogfood errors (FxHashSet and pass-by-value)

0b09d35

Update UI test output

0a999af

phansch force-pushed the try-collect branch from 353280f to 0a999af Compare December 28, 2018 20:33

phansch closed this Apr 17, 2019

phansch reopened this Apr 17, 2019

Merge branch 'master' into try-collect

2749fab

phansch removed their assignment Jun 6, 2019

phansch closed this Jun 6, 2019

phansch added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jun 6, 2019

Conversation

marcusklaas commented Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oli-obk left a comment

Choose a reason for hiding this comment

Uh oh!

oli-obk Sep 20, 2017

Choose a reason for hiding this comment

Uh oh!

marcusklaas Oct 26, 2017

Choose a reason for hiding this comment

Uh oh!

oli-obk Sep 20, 2017

Choose a reason for hiding this comment

Uh oh!

marcusklaas Oct 26, 2017

Choose a reason for hiding this comment

Uh oh!

oli-obk commented Oct 26, 2017

Uh oh!

marcusklaas commented Oct 26, 2017

Uh oh!

oli-obk commented Oct 26, 2017

Uh oh!

marcusklaas commented Oct 26, 2017

Uh oh!

marcusklaas commented Oct 26, 2017

Uh oh!

oli-obk commented Apr 3, 2018

Uh oh!

mati865 commented Jun 7, 2018

Uh oh!

marcusklaas commented Jun 7, 2018 via email

Uh oh!

oli-obk commented Jun 7, 2018

Uh oh!

marcusklaas commented Jun 7, 2018 via email

Uh oh!

flip1995 commented Jun 8, 2018

Uh oh!

flip1995 commented Jun 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phansch commented Jun 8, 2018

Uh oh!

phansch commented Dec 2, 2018

Uh oh!

eddyb Dec 4, 2018

Choose a reason for hiding this comment

Uh oh!

oli-obk Dec 4, 2018

Choose a reason for hiding this comment

Uh oh!

eddyb Dec 4, 2018

Choose a reason for hiding this comment

Uh oh!

phansch commented Dec 5, 2018

Uh oh!

bors commented Dec 12, 2018

Uh oh!

flip1995 commented Dec 27, 2018

Uh oh!

phansch commented Dec 28, 2018

Uh oh!

phansch commented Dec 28, 2018

Uh oh!

bors commented Jan 3, 2019

Uh oh!

bors commented Jan 7, 2019

Uh oh!

bors commented Apr 18, 2019

Uh oh!

flip1995 commented Jun 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phansch commented Jun 6, 2019

Uh oh!

Reviewers

Assignees

marcusklaas commented Sep 20, 2017 •

edited

Loading

flip1995 commented Jun 8, 2018 •

edited

Loading

flip1995 commented Jun 6, 2019 •

edited

Loading