Closes #54538: `unused_patterns` lint #54820

kleimkuhler · 2018-10-04T16:50:51Z

rust-highfive · 2018-10-04T16:50:54Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @zackmdavis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

src/librustc_lint/unused.rs

rust-highfive · 2018-10-04T17:09:50Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:16:29]    Compiling rustc_lint v0.0.0 (/checkout/src/librustc_lint)
[00:16:31] error[E0308]: match arms have incompatible types
[00:16:31]    --> librustc_lint/unused.rs:381:53
[00:16:31]     |
[00:16:31] 381 |           let (value, msg, struct_lit_needs_parens) = match p.node {
[00:16:31]     |  _____________________________________________________^
[00:16:31] 382 | |             Ident(.., Some(ref pat)) => (pat, "optional subpattern", false),
[00:16:31] 383 | |             Ref(ref pat, _) => (pat, "reference pattern", false),
[00:16:31] 384 | |             Slice(_, Some(ref pat), _) => (pat, "optional position pattern", false),
[00:16:31] 385 | |             Paren(_) => (p, "pattern", false),
[00:16:31] 386 | |             _ => return,
[00:16:31] 387 | |         };
[00:16:31] 387 | |         };
[00:16:31]     | |_________^ expected struct `syntax::ptr::P`, found struct `syntax::ast::Pat`
[00:16:31]     |
[00:16:31]     = note: expected type `(&syntax::ptr::P<syntax::ast::Pat>, &str, bool)`
[00:16:31]                found type `(&syntax::ast::Pat, &'static str, bool)`
[00:16:31] error: aborting due to previous error
[00:16:31] 
[00:16:31] For more information about this error, try `rustc --explain E0308`.
[00:16:31] error: Could not compile `rustc_lint`.
[00:16:31] error: Could not compile `rustc_lint`.
[00:16:31] warning: build failed, waiting for other jobs to finish...
[00:17:11] error: build failed
[00:17:11] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" " jemalloc" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "--message-format" "json"
[00:17:11] expected success, got: exit code: 101
[00:17:11] thread 'main' panicked at 'cargo must succeed', bootstrap/compile.rs:1112:9
[00:17:11] travis_fold:end:stage0-rustc

[00:17:11] travis_time:end:stage0-rustc:start=1538672263605734725,finish=1538672986283279588,duration=722677544863


[00:17:11] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:17:11] Build completed unsuccessfully in 0:12:57
[00:17:11] Makefile:28: recipe for target 'all' failed
[00:17:11] make: *** [all] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:1341f380
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
151412 ./src/tools/clang
149112 ./src/llvm-emscripten/test
144852 ./obj/build/x86_64-unknown-linux-gnu/stage0-rustc
135996 ./obj/build/bootstrap/debug/incremental/bootstrap-3ivyub3ic2113
135992 ./obj/build/bootstrap/debug/incremental/bootstrap-3ivyub3ic2113/s-f5ew7hrzv0-1v9mkjx-1akhp35ixzhyj
107652 ./obj/build/x86_64-unknown-linux-gnu/stage0/lib/rustlib/x86_64-unknown-linux-gnu/codegen-backends
104700 ./src/tools/lldb
103088 ./obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu
103084 ./obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

kleimkuhler · 2018-10-06T02:17:50Z

@nikomatsakis This is ready for review. The Paren check ended up being a lot simpler since the lint already checks sub patterns. I did not end up needing a match as it introduced double checking Pats.

@zackmdavis Thank you for input on this as well. For the issue #54538 I've been going through Niko on communication since I originally reached out to them about it. If you do have additional input on this I would appreciate it as well! (I missed the original r? @nikomatsakis when opening the [WIP])

rust-highfive · 2018-10-06T03:08:47Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:49:34] ................................................................................................i... 2200/4553
[00:49:38] .................................................................................................... 2300/4553
[00:49:42] .................................................................................................... 2400/4553
[00:49:45] .................................................................................................... 2500/4553
[00:49:49] ........iiiiiiiii................................................................................... 2600/4553
[00:49:55] .................................................................................................... 2800/4553
[00:49:59] .................................................................................................... 2900/4553
[00:50:01] ............................i....................................................................... 3000/4553
[00:50:04] ........................................................................................i.i..ii..... 3100/4553
---
[00:50:50] 
[00:50:50] running 2872 tests
[00:51:00] .................................................................................................... 100/2872
[00:51:10] .............................................................................i...................... 200/2872
[00:51:17] ...........................................................................F....F................... 300/2872
[00:51:35] .................................................................................................... 500/2872
[00:51:45] .................................................................................................... 600/2872
[00:51:58] .................................................................................................... 700/2872
[00:52:08] .................................................................................................... 800/2872
---
[00:55:19] .................................................................................................... 2500/2872
[00:55:43] .................................................................................................... 2600/2872
[00:55:51] .................................................................................................... 2700/2872
[00:55:59] .................................................................................................... 2800/2872
[00:56:08] To only update this specific test, also pass `--test-args binding/pat-tuple-7.rs`
[00:56:08] error: 1 errors occurred comparing output.
[00:56:08] status: exit code: 0
[00:56:08] status: exit code: 0
[00:56:08] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/run-pass/binding/pat-tuple-7.rs" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/binding/pat-tuple-7/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/binding/pat-tuple-7/auxiliary"
[00:56:08] ------------------------------------------
[00:56:08] 
[00:56:08] ------------------------------------------
[00:56:08] stderr:
[00:56:08] stderr:
[00:56:08] ------------------------------------------
[00:56:08] {"message":"unnecessary parentheses around pattern","code":{"code":"unused_parens","explanation":null},"level":"warning","spans":[{"file_name":"/checkout/src/test/run-pass/binding/pat-tuple-7.rs","byte_start":514,"byte_end":519,"line_start":15,"line_end":15,"column_start":9,"column_end":14,"is_primary":true,"text":[{"text":"        (pat) => assert_eq!(pat, 0)","highlight_start":9,"highlight_end":14}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[{"message":"#[warn(unused_parens)] on by default","code":null,"level":"note","spans":[],"children":[],"rendered":null},{"message":"remove these parentheses","code":null,"level":"help/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/binding/range-inclusive-pattern-precedence/auxiliary"
[00:56:08] ------------------------------------------
[00:56:08] 
[00:56:08] ------------------------------------------
[00:56:08] stderr:
[00:56:08] stderr:
[00:56:08] ------------------------------------------
[00:56:08] {"message":"unnecessary parentheses around pattern","code":{"code":"unused_parens","explanation":null},"level":"warning","spans":[{"file_name":"/checkout/src/test/run-pass/binding/range-inclusive-pattern-precedence.rs","byte_start":573,"byte_end":582,"line_start":18,"line_end":18,"column_start":10,"column_end":19,"is_primary":true,"text":[{"text":"        &(18..=18) => {}","highlight_start":10,"highlight_end":19}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[{"message":"#[warn(unused_parens)] on by default","code":null,"level":"note","spans":[],"children":[],"rendered":null},{"message":"remove these parentheses","code":null,"level":"help","spans":[{"file_name":"/checkout/src/test/run-pass/binding/range-inclusive-pattern-precedence.rs","byte_start":573,"byte_end":582,"line_start":18,"line_end":18,"column_start":10,"column_end":19,"is_primary":true,"text":[{"text":"        &(18..=18) => {}","highlight_start":10,"highlight_end":19}],"label":null,"suggested_replacement":"18 ..=18","suggestion_applicability":"MachineApplicable","expansion":null}],"children":[],"rendered":null}],"rendered":"warning: unnecessary parentheses around pattern\n  --> /checkout/src/test/run-paull,"suggestion_applicability":null,"expansion":null}],"children":[{"message":"remove these parentheses","code":null,"level":"help","spans":[{"file_name":"/checkout/src/test/run-pass/binding/range-inclusive-pattern-precedence.rs","byte_start":843,"byte_end":858,"line_start":30,"line_end":30,"column_start":13,"column_end":28,"is_primary":true,"text":[{"text":"        box (VALUE..=VALUE) => {}","highlight_start":13,"highlight_end":28}],"label":null,"suggested_replacement":"VALUE ..=VALUE","suggestion_applicability":"MachineApplicable","expansion":null}],"children":[],"rendered":null}],"rendered":"warning: unnecessary parentheses around pattern\n  --> /checkout/src/test/run-pass/binding/range-inclusive-pattern-precedence.rs:30:13\n   |\nLL |         box (VALUE..=VALUE) => {}\n   |             ^^^^^^^^^^^^^^^ help: remove these parentheses\n\n"}
[00:56:08] ------------------------------------------
[00:56:08] 
[00:56:08] thread '[run-pass] run-pass/binding/range-inclusive-pattern-precedence.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:3267:9
[00:56:08] 
---
151464 ./obj/build/bootstrap/debug/incremental
151412 ./src/tools/clang
149116 ./src/llvm-emscripten/test
135996 ./obj/build/bootstrap/debug/incremental/bootstrap-3ivyub3ic2113
135992 ./obj/build/bootstrap/debug/incremental/bootstrap-3ivyub3ic2113/s-f5gfcgykwr-1xq5cpj-1akhp3

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

kleimkuhler · 2018-10-06T05:51:14Z

src/test/run-pass/binding/pat-tuple-7.rs

@@ -12,6 +12,6 @@

 fn main() {
    match 0 {
-        (pat) => assert_eq!(pat, 0)
+        pat => assert_eq!(pat, 0)


This test may not be serving a purpose anymore with the removal of parentheses. If it should stick around, I can keep or this change or silence the warning that now gets generated.

zackmdavis

We need to somehow not lint on cases where parens in patterns are necessary, and test such cases. I don't offhand know the spec for when parens-in-patterns are truly necessary, but I don't think this patch is there yet. Consider this program:

fn main() {
    match &2 {
        &1..=3 => {},
        _ => {}
    }
}

We get an error (added in 6399d16) imploring us to add parentheses:

error: the range pattern here has ambiguous interpretation
 --> paren.rs:3:10
  |
3 |         &1..=3 => {},
  |          ^^^^^ help: add parentheses to clarify the precedence: `(1 ..=3)`

error: aborting due to previous error

But if we do so, then a compiler built on this branch fires the lint and says that we shouldn't have:

zmd@ReflectiveCoherence:~/Code/Misc$ rustc +stage1 paren.rs 
warning: unnecessary parentheses around pattern
 --> paren.rs:3:10
  |
3 |         &(1..=3) => {},
  |          ^^^^^^^ help: remove these parentheses
  |
  = note: #[warn(unused_parens)] on by default

zackmdavis · 2018-10-06T06:20:10Z

src/test/ui/lint/issue-54538-unused-parens-lint.rs

+        (_) => {}     //~ WARNING: unnecessary parentheses around pattern
+        (y) => {}     //~ WARNING: unnecessary parentheses around pattern
+        (ref r) => {} //~ WARNING: unnecessary parentheses around pattern
+        e @ 1...2 | (e @ (3...4)) => {}


..= is the modern style.

zackmdavis · 2018-10-06T06:27:24Z

I missed the original r? @nikomatsakis when opening the [WIP])

It still works in the comments—

r? @nikomatsakis

zackmdavis · 2018-10-06T06:30:53Z

It still works in the comments—

Oh, looks like the bot already reassigned it hours ago because the r-question magic phrase appeared in your comment.

rust-highfive · 2018-10-06T06:47:39Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:49:59] ................................................................................................i... 2200/4557
[00:50:04] .................................................................................................... 2300/4557
[00:50:08] .................................................................................................... 2400/4557
[00:50:12] .................................................................................................... 2500/4557
[00:50:15] ........iiiiiiiii................................................................................... 2600/4557
[00:50:22] .................................................................................................... 2800/4557
[00:50:26] .................................................................................................... 2900/4557
[00:50:29] .............................i...................................................................... 3000/4557
[00:50:32] .........................................................................................i.i..ii.... 3100/4557
---
[00:51:19] 
[00:51:19] running 2872 tests
[00:51:29] .................................................................................................... 100/2872
[00:51:39] .............................................................................i...................... 200/2872
[00:51:47] ................................................................................F................... 300/2872
[00:52:04] .................................................................................................... 500/2872
[00:52:15] .................................................................................................... 600/2872
[00:52:29] .................................................................................................... 700/2872
[00:52:39] .................................................................................................... 800/2872
---
[00:56:49] normalized stderr:
[00:56:49] warning: unnecessary parentheses around pattern
[00:56:49]   --> $DIR/range-inclusive-pattern-precedence.rs:18:10
[00:56:49]    |
[00:56:49] LL |         &(18..=18) => {}
[00:56:49]    |
[00:56:49]    = note: #[warn(unused_parens)] on by default
[00:56:49] 
[00:56:49] warning: unnecessary parentheses around pattern
[00:56:49] warning: unnecessary parentheses around pattern
[00:56:49]   --> $DIR/range-inclusive-pattern-precedence.rs:22:10
[00:56:49]    |
[00:56:49] LL |         &(VALUE..=VALUE) => {}
[00:56:49] 
[00:56:49] warning: unnecessary parentheses around pattern
[00:56:49]   --> $DIR/range-inclusive-pattern-precedence.rs:26:13
[00:56:49]    |
[00:56:49]    |
[00:56:49] LL |         box (18..=18) => {}
[00:56:49] 
[00:56:49] warning: unnecessary parentheses around pattern
[00:56:49]   --> $DIR/range-inclusive-pattern-precedence.rs:30:13
[00:56:49]    |
[00:56:49]    |
[00:56:49] LL |         box (VALUE..=VALUE) => {}
[00:56:49] 
[00:56:49] 
[00:56:49] 
[00:56:49] 
[00:56:49] 
[00:56:49] The actual stderr differed from the expected stderr.
[00:56:49] Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/binding/range-inclusive-pattern-precedence/range-inclusive-pattern-precedence.stderr
[00:56:49] To update references, rerun the tests and pass the `--bless` flag
[00:56:49] To only update this specific test, also pass `--test-args binding/range-inclusive-pattern-precedence.rs`
[00:56:49] error: 1 errors occurred comparing output.
[00:56:49] status: exit code: 0
[00:56:49] status: exit code: 0
[00:56:49] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/run-pass/binding/range-inclusive-pattern-precedence.rs" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/binding/range-inclusive-pattern-precedence/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x{}\n   |          ^^^^^^^^^ help: remove these parentheses\n   |\n   = note: #[warn(unused_parens)] on by default\n\n"}
[00:56:49] {"message":"unnecessary parentheses around pattern","code":{"code":"unused_parens","explanation":null},"level":"warning","spans":[{"file_name":"/checkout/src/test/run-pass/binding/range-inclusive-pattern-precedence.rs","byte_start":653,"byte_end":668,"line_start":22,"line_end":22,"column_start":10,"column_end":25,"is_primary":true,"text":[{"text":"        &(VALUE..=VALUE) => {}","highlight_start":10,"highlight_end":25}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[{"message":"remove these parentheses","code":null,"level":"help","spans":[{"file_name":"/checkout/src/test/run-pass/binding/range-inclusive-pattern-precedence.rs","byte_start":653,"byte_end":668,"line_start":22,"line_end":22,"column_start":10,"column_end":25,"is_primary":true,"text":[{"text":"        &(VALUE..=VALUE) => {}","highlight_start":10,"highlight_end":25}],"label":null,"suggested_replacement":"VALUE ..=VALUE","suggestion_applicability":"MachineApplicable","expansion":null}],"children":[],"rendered":null}],"rendered":"warning: unnecessary parentheses around pattern\n  --> /checkout/src/test/run-pass/binding/range-inclusive-pattern-precedence.rs:22:10\n   |\nLL |         &(VALUE..=VALUE) => {}\n   |          ^^^^^^^^^^^^^^^ help: remove these parentheses\n\n"}
[00:56:49] {"message":"unnecessary parentheses around pattern","code":{"code":"unused_parens","explanation":null},"level":"warning","spans":[{"file_name":"/checkout/src/test/run-pass/binding/range-inclusive-pattern-precedence.rs","byte_start":751,"byte_end":760,"line_start":26,"line_end":26,"column_start":13,"column_end":22,"is_primary":true,"text":[{"text":"        box (18..=18) => {}","highlight_start":13,"highlight_end":22}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[{"message":"remove these parentheses","code":null,"level":"help","spans":[{"file_name":"/checkout/src/test/run-pass/binding/range-inclusive-pattern-precedence.rs","byte_start":751,"byte_end":760,"line_start":26,"line_end":26,"column_start":13,"column_end":22,"is_primary":true,"text":[{"text":"        box (18..=18) => {}","highlight_start":13,"highlight_end":22}],"label":null,"suggested_replacement":"18 ..=18","suggestion_applicability":"MachineApplicable","expansion":null}],"children":[],"rendered":null}],"rendered":"warning: unnecessary parentheses around pattern\n  --> /checkout/src/test/run-pass/binding/range-inclusive-pattern-precedence.rs:26:13\n   |\nLL |         box (18..=18) => {}\n   |             ^^^^^^^^^ help: remove these parentheses\n\n"}
[00:56:49] {"message":"unnecessary parentheses around pattern","code":{"code":"unused_parens","explanation":null},"level":"warning","spans":[{"file_name":"/checkout/src/test/run-pass/binding/range-inclusive-pattern-precedence.rs","byte_start":843,"byte_end":858,"line_start":30,"line_end":30,"column_start":13,"column_end":28,"is_primary":true,"text":[{"text":"        box (VALUE..=VALUE) => {}","highlight_start":13,"highlight_end":28}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[{"message":"remove these parentheses","code":null,"level":"help","spans":[{"file_name":"/checkout/src/test/run-pass/binding/range-inclusive-pattern-precedence.rs","byte_start":843,"byte_end":858,"line_start":30,"line_end":30,"column_start":13,"column_end":28,"is_primary":true,"text":[{"text":"        box (VALUE..=VALUE) => {}","highlight_start":13,"highlight_end":28}],"label":null,"suggested_replacement":"VALUE ..=VALUE","suggestion_applicability":"MachineApplicable","expansion":null}],"children":[],"rendered":null}],"rendered":"warning: unnecessary parentheses around pattern\n  --> /checkout/src/test/run-pass/binding/range-inclusive-pattern-precedence.rs:30:13\n   |\nLL |         box (VALUE..=VALUE) => {}\n   |             ^^^^^^^^^^^^^^^ help: remove these parentheses\n\n"}
[00:56:49] ------------------------------------------
[00:56:49] 
[00:56:49] thread '[run-pass] run-pass/binding/range-inclusive-pattern-precedence.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:3267:9
[00:56:49] note: Run with `RUST_BACKTRACE=1` for a backtrace.
---
[00:56:49] 
[00:56:49] thread 'main' panicked at 'Some tests failed', tools/compiletest/src/main.rs:499:22
[00:56:49] 
[00:56:49] 
[00:56:49] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/run-pass" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "run-pass" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-5.0/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Zunstable-options " "--target-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "5.0.0\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[00:56:49] 
[00:56:49] 
[00:56:49] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[00:56:49] Build completed unsuccessfully in 0:11:47
[00:56:49] Build completed unsuccessfully in 0:11:47
[00:56:49] Makefile:58: recipe for target 'check' failed
[00:56:49] make: *** [check] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0758f604
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

kleimkuhler · 2018-10-07T05:13:00Z

@zackmdavis There is no warning for Ranges now and tests are passing. I added some more discrete test cases which helped out.

nikomatsakis

This is looking good — but I think we could make it more precise with relative ease. I left a comment. What do you think?

src/librustc_lint/unused.rs

rust-highfive · 2018-10-09T04:23:14Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:04:13] tidy error: /checkout/src/librustc_lint/unused.rs:340: trailing whitespace
[00:04:14] some tidy checks failed
[00:04:14] 
[00:04:14] 
[00:04:14] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:14] 
[00:04:14] 
[00:04:14] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:14] Build completed unsuccessfully in 0:00:49
[00:04:14] Build completed unsuccessfully in 0:00:49
[00:04:14] Makefile:79: recipe for target 'tidy' failed
[00:04:14] make: *** [tidy] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:126b70d2
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:00b84c2e:start=1539058516165519762,finish=1539058516170399495,duration=4879733
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:16040e40
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:02942b2f
travis_time:start:02942b2f
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:16f9a529
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

nikomatsakis

Hmm, I left a review here, but as I mentioned on Zulip, I realized that my strategy for fixing the problem isn't quite going to work, because in the lint visitor, you are not in control of what gets visited.

I am debating the best fix. One option would be to keep a little mutable state in the UnusedParens lint itself; we could for example add the ids for 'necessary' parens into a set stored in the lint. Then when we reach the paren node, we can check if it is present in that set.

I am not thrilled about that solution (mutable state == more complexity) but I'm not sure I see a better choice just now, given the way lints are setup.

src/test/run-pass/binding/pat-tuple-7.rs

nikomatsakis · 2018-10-09T16:52:39Z

src/librustc_lint/unused.rs

+
+    fn check_pat(&mut self, cx: &EarlyContext, p: &ast::Pat) {
+        use syntax::ast::PatKind::{Paren, Range};
+        if let Some(subpattern) = UnusedParens::is_high_precedence_unary_pat(p) {


A comment here would be good. Something like:

The only "necessary" use for parentheses is to when you have a high-precedence pattern (like &P) that is applied to a low-precedence pattern (like a ..= b). This is because &a ..= b would be parsed incorrectly.

nikomatsakis · 2018-10-09T17:37:56Z

Discussing with @Manishearth, it seems like another option would be to modify the lint to override check_item and then we could use the visitor ourselves, so we have complete control over the walk.

Or we add a mutable set. I'm not sure if I have an opinion about which would be best.

This uses a copied version of `check_unused_parens_expr` that is specific to `ast::Pat`. `check_unused_parens_` could possibly be made more generic to work with any `ast::*` that has `node` and `span` fields. This also only checks for the case of parens around the wildcard pattern. It covers the case highlighted in the issue, but could check for a lot more.

kleimkuhler · 2018-10-10T04:59:12Z

After some thorough discussion with @nikomatsakis on the Zulip topic, we decided it would be best to not try and be more precise with the lint as it would introduce more complexity, and expressions are also not this precise.

The complexity introduced would be both in the logic of the lint, as well as warnings on patterns that would become less readable with unnecessary parentheses removed.

For example, the pattern &(a..=b) could already be a difficult pattern to read depending on the expressions that a and b are. If there are unnecessary parentheses in either of those subpatterns, they are most likely there for readability.

nikomatsakis · 2018-10-15T17:10:04Z

@bors r+

bors · 2018-10-15T17:10:05Z

📌 Commit 0e411c2 has been approved by nikomatsakis

@nikomatsakis

…erns-lint, r=nikomatsakis Closes rust-lang#54538: `unused_patterns` lint Closes rust-lang#54538 r? @nikomatsakis

@ghost

Rollup of 11 pull requests Successful merges: - #54820 (Closes #54538: `unused_patterns` lint) - #54963 (Cleanup rustc/session) - #54991 (add test for #23189) - #55025 (Add missing lifetime fragment specifier to error message.) - #55047 (doc: make core::fmt::Error example more simple) - #55048 (Don't collect to vectors where unnecessary) - #55060 (clarify pointer add/sub function safety concerns) - #55062 (Make EvalContext::step public again) - #55066 (Fix incorrect link in println! documentation) - #55081 (Deduplicate tests) - #55088 (Update rustc documentation link) Failed merges: r? @ghost

bors · 2018-10-15T22:20:30Z

⌛ Testing commit 0e411c2 with merge 46880f4...

rust-highfive assigned zackmdavis Oct 4, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 4, 2018

kleimkuhler commented Oct 4, 2018

View reviewed changes

src/librustc_lint/unused.rs Outdated Show resolved Hide resolved

zackmdavis added a commit to zackmdavis/rust that referenced this pull request Oct 5, 2018

WIP experimental commit while reviewing rust-lang#54820

b4c948b

zackmdavis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 5, 2018

kleimkuhler force-pushed the issue-54538-unused_patterns-lint branch from 24eddc4 to f976927 Compare October 6, 2018 02:10

kleimkuhler changed the title ~~[WIP] unused_patterns lint~~ Closes #54820: unused_patterns lint Oct 6, 2018

kleimkuhler changed the title ~~Closes #54820: unused_patterns lint~~ Closes #54538: unused_patterns lint Oct 6, 2018

rust-highfive assigned nikomatsakis and unassigned zackmdavis Oct 6, 2018

kleimkuhler force-pushed the issue-54538-unused_patterns-lint branch from f976927 to 93eb6f6 Compare October 6, 2018 05:41

kleimkuhler commented Oct 6, 2018

View reviewed changes

zackmdavis reviewed Oct 6, 2018

View reviewed changes

kleimkuhler force-pushed the issue-54538-unused_patterns-lint branch from 366ab55 to ae05749 Compare October 7, 2018 02:49

nikomatsakis requested changes Oct 8, 2018

View reviewed changes

src/librustc_lint/unused.rs Show resolved Hide resolved

kleimkuhler force-pushed the issue-54538-unused_patterns-lint branch from ae05749 to 2a2ed22 Compare October 9, 2018 04:09

kleimkuhler force-pushed the issue-54538-unused_patterns-lint branch from 2a2ed22 to 2cb37ea Compare October 9, 2018 04:41

nikomatsakis reviewed Oct 9, 2018

View reviewed changes

kleimkuhler added 4 commits October 9, 2018 21:10

Share outer paren trimming logic

5217527

Simply unused_parens check and add tests

46b07d6

Fix Range warning and improve tests

47014df

Add clarifying pattern lint comment and revert test

0e411c2

kleimkuhler force-pushed the issue-54538-unused_patterns-lint branch from 2cb37ea to 0e411c2 Compare October 10, 2018 04:54

kleimkuhler mentioned this pull request Oct 10, 2018

Don't lint 'unused_parens on if (break _) { .. }` #54885

Merged

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 15, 2018

Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 15, 2018

Rollup merge of rust-lang#54820 - kleimkuhler:issue-54538-unused_patt…

7ba24e8

…erns-lint, r=nikomatsakis Closes rust-lang#54538: `unused_patterns` lint Closes rust-lang#54538 r? @nikomatsakis

Manishearth mentioned this pull request Oct 15, 2018

Rollup of 11 pull requests #55095

Merged

bors merged commit 0e411c2 into rust-lang:master Oct 15, 2018

kleimkuhler deleted the issue-54538-unused_patterns-lint branch October 15, 2018 22:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Closes #54538: `unused_patterns` lint #54820

Closes #54538: `unused_patterns` lint #54820

kleimkuhler commented Oct 4, 2018 •

edited

Loading

rust-highfive commented Oct 4, 2018

rust-highfive commented Oct 4, 2018

kleimkuhler commented Oct 6, 2018

rust-highfive commented Oct 6, 2018

kleimkuhler Oct 6, 2018

zackmdavis left a comment

zackmdavis Oct 6, 2018

zackmdavis commented Oct 6, 2018

zackmdavis commented Oct 6, 2018

rust-highfive commented Oct 6, 2018

kleimkuhler commented Oct 7, 2018

nikomatsakis left a comment

rust-highfive commented Oct 9, 2018

nikomatsakis left a comment

nikomatsakis Oct 9, 2018

nikomatsakis commented Oct 9, 2018

kleimkuhler commented Oct 10, 2018

nikomatsakis commented Oct 15, 2018

bors commented Oct 15, 2018

bors commented Oct 15, 2018

Closes #54538: unused_patterns lint #54820

Closes #54538: unused_patterns lint #54820

Conversation

kleimkuhler commented Oct 4, 2018 • edited Loading

rust-highfive commented Oct 4, 2018

rust-highfive commented Oct 4, 2018

kleimkuhler commented Oct 6, 2018

rust-highfive commented Oct 6, 2018

kleimkuhler Oct 6, 2018

Choose a reason for hiding this comment

zackmdavis left a comment

Choose a reason for hiding this comment

zackmdavis Oct 6, 2018

Choose a reason for hiding this comment

zackmdavis commented Oct 6, 2018

zackmdavis commented Oct 6, 2018

rust-highfive commented Oct 6, 2018

kleimkuhler commented Oct 7, 2018

nikomatsakis left a comment

Choose a reason for hiding this comment

rust-highfive commented Oct 9, 2018

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Oct 9, 2018

Choose a reason for hiding this comment

nikomatsakis commented Oct 9, 2018

kleimkuhler commented Oct 10, 2018

nikomatsakis commented Oct 15, 2018

bors commented Oct 15, 2018

bors commented Oct 15, 2018

Closes #54538: `unused_patterns` lint #54820

Closes #54538: `unused_patterns` lint #54820

kleimkuhler commented Oct 4, 2018 •

edited

Loading