New lint: `unnested_or_patterns` by Centril · Pull Request #5378 · rust-lang/rust-clippy

Centril · 2020-03-28T11:54:45Z

changelog: Adds a lint unnested_or_patterns, suggesting Some(0 | 2) as opposed to Some(0) | Some(2). The lint only fires on compilers capable of using #![feature(or_patterns)].

The lint is primarily encoded as a pure algorithm which to unnest or-patterns in an ast::Pat (fn unnest_or_patterns) through a MutVisitor. After that is done, and assuming that any change was detected, then pprust::pat_to_string is used to simply convert the transformed pattern into a suggestion.
The PR introduces a module utils::ast_utils with a bunch of functions for spanless & nodeless equality comparisons of ASTs.

cc rust-lang/rust#54883

Centril · 2020-03-29T05:33:59Z

CI seems happy. 🎉

bors · 2020-03-30T08:21:14Z

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

Centril

(note to self: some mistakes I made to fix before merging)

bors · 2020-03-30T20:13:32Z

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

flip1995

Sorry for taking so long for the review. I don't have an excuse, just wasn't really motivated to review PRs recently.

This looks really good. It'll need a rebase + cargo dev update_lints and you may want to address your own comments 😉

flip1995 · 2020-06-07T19:39:59Z

r? @phansch

No need to review the lint itself, I already did that. A second pair of eyes on fixing the dogfood fallout would be great though!

(I finished this lint, since Centril currently takes a break from OSS and this is too good to let it go to waste)

phansch

Maybe I just have to get used to or_patterns more - in some cases I find them harder to parse in my head than the un-nested version. Did you have the same feeling @flip1995? (in any case, r=me for the dogfood changes)

phansch · 2020-06-08T06:34:37Z

+help: nest the patterns
+   |
+LL |     if let box (0 | 1 | 2 | 3 | 4) = Box::new(0) {}
+   |            ^^^^^^^^^^^^^^^^^^^^^^^


whoa these suggestions are nice 🤯

Yes, they are built by modifying the AST and then pretty printing it IIUC. Centril definitely knows what he's doing 😄

phansch · 2020-06-08T06:48:44Z

-            ["as_ptr", "unwrap"] | ["as_ptr", "expect"] => {
-                lint_cstring_as_ptr(cx, expr, &arg_lists[1][0], &arg_lists[0][0])
-            },
+            ["as_ptr", "unwrap" | "expect"] => lint_cstring_as_ptr(cx, expr, &arg_lists[1][0], &arg_lists[0][0]),


For example here, is that really less complex than before? In my mind I first have to think about the precedence to figure out what's going on. Maybe it just takes some time to get used to it.

Maybe it just takes some time to get used to it.

I think that's it. But especially this example is really nice IMO. I think it expresses more the thought of ""as_ptr" is the first part, followed by "unwrap" OR "expect"", instead of "it's either an "as_ptr", "unwrap" array OR an "as_ptr", "expect" array. I can see why one would prefer the later way of writing this though.

What I struggeled with on first reviewing this was a pattern like:

(A | B, C | D) => { ... }

Because this is combinatorially evaluated (A+C, A+D, B+C, B+D), which may not be directly obvious, if your not used to this kind of pattern.

flip1995 · 2020-06-08T13:32:20Z

@bors r=flip1995,phansch

bors · 2020-06-08T13:32:22Z

📌 Commit a9ca832 has been approved by flip1995,phansch

bors · 2020-06-08T13:49:34Z

⌛ Testing commit a9ca832 with merge 08b84b3...

bors · 2020-06-08T14:17:12Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995,phansch
Pushing 08b84b3 to master...

matthiaskrgr · 2020-06-08T21:59:02Z

When rustfixing this lint on nightly, we will always get errors if #![feature(or_patterns)] is not enabled.

The following errors were reported:
error[E0658]: or-patterns syntax is experimental
   --> alacritty_terminal/src/ansi.rs:982:14
    |
982 |             ('B' | 'e', None) => {
    |              ^^^^^^^^^
    |
    = note: see issue #54883 <https://github.com/rust-lang/rust/issues/54883> for more information
    = help: add `#![feature(or_patterns)]` to the crate attributes to enable

should we move this to pedantic/nursery until or_patterns are stable?

flip1995 · 2020-06-08T22:56:54Z

It shouldn't lint, if the or_patterns feature isn't enabled? 😮

matthiaskrgr · 2020-06-09T10:50:03Z

Hm, I can't reproduce this with the clippy of latest nightly. clippy 0.0.212 (fd4b177 2020-06-08)

But when I use the master toolchain and use master-clippy (I build clippy using rustc master and put the bins in ~/.rustup/toolchains/master/bin and run cargo +master clippy, I get unnested_or_patterns warnings. Hmm.

flip1995 · 2020-06-09T13:16:55Z

+}
+
+fn lint_unnested_or_patterns(cx: &EarlyContext<'_>, pat: &Pat) {
+    if !cx.sess.opts.unstable_features.is_nightly_build() {


Oh yeah, we should check, if the feature is enabled here. In that case, this was an oversight on my side.

Centril force-pushed the unnested-or-pats branch 3 times, most recently from 46aa951 to edc2802 Compare March 29, 2020 04:08

Centril force-pushed the unnested-or-pats branch from edc2802 to 26a873b Compare March 30, 2020 12:06

flip1995 added A-lint Area: New lints S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Mar 30, 2020

Centril commented Mar 30, 2020

View reviewed changes

flip1995 self-requested a review March 30, 2020 19:10

flip1995 approved these changes Apr 15, 2020

View reviewed changes

flip1995 self-assigned this May 25, 2020

Centril and others added 4 commits June 7, 2020 21:09

add unnested_or_patterns lint

7b6dc7b

dogfood unnested_or_patterns

78f158e

split unnested_or_patterns test

d6136b9

Apply self-review by Centril

ecabed6

flip1995 force-pushed the unnested-or-pats branch from 26a873b to ecabed6 Compare June 7, 2020 19:18

Fix rebase fallout

a9ca832

flip1995 force-pushed the unnested-or-pats branch from 61fb7ad to a9ca832 Compare June 7, 2020 19:38

rust-highfive assigned phansch and unassigned flip1995 Jun 7, 2020

phansch reviewed Jun 8, 2020

View reviewed changes

bors merged commit 08b84b3 into rust-lang:master Jun 8, 2020

bors mentioned this pull request Jun 8, 2020

Macro use sugg #5279

Merged

This was referenced Jun 8, 2020

Added restriction lint: pattern-type-mismatch #4841

Merged

#5626: lint iterator.map(|x| x) #5694

Merged

flip1995 reviewed Jun 9, 2020

View reviewed changes

matthiaskrgr mentioned this pull request Jun 10, 2020

unnested_or_patterns trigger on master build even if feature is not enabled #5704

Closed

Centril deleted the unnested-or-pats branch June 25, 2020 10:35

Uh oh!

Uh oh!

Conversation

Centril commented Mar 28, 2020

Uh oh!

Centril commented Mar 29, 2020

Uh oh!

bors commented Mar 30, 2020

Uh oh!

Centril left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bors commented Mar 30, 2020

Uh oh!

flip1995 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flip1995 commented Jun 7, 2020

Uh oh!

phansch left a comment

Choose a reason for hiding this comment

Uh oh!

phansch Jun 8, 2020

Choose a reason for hiding this comment

Uh oh!

flip1995 Jun 8, 2020

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flip1995 commented Jun 8, 2020

Uh oh!

bors commented Jun 8, 2020

Uh oh!

bors commented Jun 8, 2020

Uh oh!

bors commented Jun 8, 2020

Uh oh!

matthiaskrgr commented Jun 8, 2020

Uh oh!

flip1995 commented Jun 8, 2020

Uh oh!

matthiaskrgr commented Jun 9, 2020

Uh oh!

flip1995 Jun 9, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

flip1995 left a comment •

edited

Loading

phansch Jun 8, 2020 •

edited

Loading

flip1995 Jun 8, 2020 •

edited

Loading