typeck: suggest use of match_default_bindings feature #45409

tamird · Oct 20, 2017

This needs a UI test, but thought I'd get some early feedback.

tamird · Oct 20, 2017

OK, this should be in a reasonable state now - I had to do some additional juggling to prevent halfway-massaging in the case of the feature not being enabled.

nikomatsakis · Oct 23, 2017

nikomatsakis requested changes Oct 23, 2017

View changes

Looks great. I left one suggestion -- let me know what you think.

nikomatsakis · Oct 23, 2017

src/librustc_typeck/check/_match.rs

+                        err.span_suggestion(pat.span, "consider using", format!("&{}", &snippet));
+                    }
+                    err.emit();
+                    None


Hmm, it seems like it may be better to just update expected and def_bm either way -- i.e., if the feature gate is not enabled, we type-check as if it were, but issue an error. It'd certainly be less complicated, but it would also suppress the "duplicate-ish" errors we see below (e.g., mismatched types).

That's what I did initially. Added a commit that reverts to that so you can see the resulting UI test behaviour.

nikomatsakis · Oct 23, 2017

src/test/compile-fail/keyword-false-as-identifier.rs

@@ -9,5 +9,7 @@
 // except according to those terms.

 fn main() {
-    let false = "foo"; //~ error: mismatched types
+    let false = "foo";


let's change this test from "foo" to 22 -- I feel like the message about non-reference patterns isn't central to the test.

nikomatsakis · Oct 23, 2017

src/test/compile-fail/keyword-self-as-identifier.rs

@@ -9,5 +9,7 @@
 // except according to those terms.

 fn main() {
-    let Self = "foo"; //~ ERROR cannot find unit struct/variant or constant `Self` in this scope
+    let Self = "foo";


let's change this test from "foo" to 22 -- I feel like the message about non-reference patterns isn't central to the test.

nikomatsakis · Oct 23, 2017

src/test/compile-fail/keyword-super-as-identifier.rs

@@ -9,5 +9,7 @@
 // except according to those terms.

 fn main() {
-    let super = "foo"; //~ ERROR failed to resolve. There are too many initial `super`s
+    let super = "foo";


let's change this test from "foo" to 22 -- I feel like the message about non-reference patterns isn't central to the test.

nikomatsakis · Oct 23, 2017

src/test/compile-fail/keyword-true-as-identifier.rs

@@ -9,5 +9,7 @@
 // except according to those terms.

 fn main() {
-    let true = "foo"; //~ error: mismatched types
+    let true = "foo";


let's change this test from "foo" to 22 -- I feel like the message about non-reference patterns isn't central to the test.

tamird · Oct 26, 2017

@kennytm should be waiting-on-review again.

nikomatsakis · Oct 30, 2017

nikomatsakis approved these changes Oct 30, 2017

View changes

nikomatsakis · Oct 30, 2017

@bors r+

bors · Oct 30, 2017

📌 Commit bd5a587 has been approved by nikomatsakis

nikomatsakis · Oct 30, 2017

@tamird sorry for the delay; I was at the Rust Belt Rust conference. Looks good, thanks!

tamird · Oct 30, 2017

No worries! Can you let me squash it before it merges? I'll be back at my computer this evening.

…

On Oct 30, 2017 11:07, "Niko Matsakis" ***@***.***> wrote: @tamird <https://github.com/tamird> sorry for the delay; I was at the Rust Belt Rust conference. Looks good, thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#45409 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdsPCtdJcVVB-k4E2b71g94HiP_-9DDks5sxeYegaJpZM4QAo98> .

kennytm · Oct 30, 2017

@bors r-

(Waiting on author to cleanup.)

tamird · Oct 30, 2017

OK, should be good to go, unless y'all have tweaks for the message.

kennytm · Oct 31, 2017

@bors r=nikomatsakis

bors · Oct 31, 2017

📌 Commit 9844777 has been approved by nikomatsakis

bors · Oct 31, 2017

⌛️ Testing commit 9844777 with merge 772b3b7...

bors · Oct 31, 2017

💔 Test failed - status-travis

kennytm · Oct 31, 2017

@bors retry #45230 LLDB segfault at check x86_64-apple-darwin

[01:34:24] ---- [debuginfo-lldb] debuginfo/associated-types.rs stdout ----
[01:34:24] 	NOTE: compiletest thinks it is using LLDB version 370
[01:34:24] 
[01:34:24] error: Error while running LLDB
[01:34:24] status: signal: 11
[01:34:24] command: "/usr/bin/python" "/Users/travis/build/rust-lang/rust/src/etc/lldb_batchmode.py" "/Users/travis/build/rust-lang/rust/build/x86_64-apple-darwin/test/debuginfo/associated-types.stage2-x86_64-apple-darwin" "/Users/travis/build/rust-lang/rust/build/x86_64-apple-darwin/test/debuginfo/associated-types.debugger.script"
[01:34:24] stdout:
[01:34:24] ------------------------------------------
[01:34:24] LLDB batch-mode script
[01:34:24] ----------------------
[01:34:24] Debugger commands script is '/Users/travis/build/rust-lang/rust/build/x86_64-apple-darwin/test/debuginfo/associated-types.debugger.script'.
[01:34:24] Target executable is '/Users/travis/build/rust-lang/rust/build/x86_64-apple-darwin/test/debuginfo/associated-types.stage2-x86_64-apple-darwin'.
[01:34:24] Current working directory is '/Users/travis/build/rust-lang/rust'
[01:34:24] Creating a target for '/Users/travis/build/rust-lang/rust/build/x86_64-apple-darwin/test/debuginfo/associated-types.stage2-x86_64-apple-darwin'
[01:34:24] settings set auto-confirm true
[01:34:24] 
[01:34:24] version
[01:34:24] lldb-370.0.42 Swift-3.1 
[01:34:24] command script import /Users/travis/build/rust-lang/rust/./src/etc/lldb_rust_formatters.py
[01:34:24] type summary add --no-value --python-function lldb_rust_formatters.print_val -x ".*" --category Rust
[01:34:24] type category enable Rust
[01:34:24] 
[01:34:24] breakpoint set --file 'associated-types.rs' --line 110
[01:34:24] Breakpoint 1: where = associated-types.stage2-x86_64-apple-darwin`associated_types::assoc_struct<i32> + 4 at associated-types.rs:110, address = 0x0000000100000764 
[01:34:24] breakpoint set --file 'associated-types.rs' --line 117
[01:34:24] Breakpoint 2: where = associated-types.stage2-x86_64-apple-darwin`associated_types::assoc_local<i32> + 74 at associated-types.rs:117, address = 0x00000001000007fa 
[01:34:24] breakpoint set --file 'associated-types.rs' --line 121
[01:34:24] Breakpoint 3: where = associated-types.stage2-x86_64-apple-darwin`associated_types::assoc_arg<i32> + 12 at associated-types.rs:121, address = 0x000000010000084c 
[01:34:24] breakpoint set --file 'associated-types.rs' --line 129
[01:34:24] Breakpoint 4: where = associated-types.stage2-x86_64-apple-darwin`associated_types::assoc_tuple<i32> + 4 at associated-types.rs:129, address = 0x00000001000008d4 
[01:34:24] breakpoint set --file 'associated-types.rs' --line 136
[01:34:24] Breakpoint 5: where = associated-types.stage2-x86_64-apple-darwin`associated_types::assoc_enum<i32> + 102 at associated-types.rs:136, address = 0x0000000100000976 
[01:34:24] breakpoint set --file 'associated-types.rs' --line 139
[01:34:24] Breakpoint 6: where = associated-types.stage2-x86_64-apple-darwin`associated_types::assoc_enum<i32> + 143 at associated-types.rs:139, address = 0x000000010000099f 
[01:34:24] quit
[01:34:24] 
[01:34:24] 
[01:34:24] ------------------------------------------
[01:34:24] stderr:
[01:34:24] ------------------------------------------
[01:34:24] 
[01:34:24] ------------------------------------------
[01:34:24] 
[01:34:24] thread '[debuginfo-lldb] debuginfo/associated-types.rs' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:2485:8
[01:34:24] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:34:24] 
[01:34:24] 
[01:34:24] failures:
[01:34:24]     [debuginfo-lldb] debuginfo/associated-types.rs
[01:34:24] 
[01:34:24] test result: �[31mFAILED�(B�[m. 97 passed; 1 failed; 10 ignored; 0 measured; 0 filtered out

bors · Oct 31, 2017

⌛️ Testing commit 9844777 with merge a274ae0...

bors · Oct 31, 2017

💔 Test failed - status-appveyor

alexcrichton · Oct 31, 2017

test [run-pass] run-pass\rustc-rust-log.rs has been running for over 60 seconds
fatal runtime error: allocator memory exhausted

Looks like this test generated so much log output that it caused compiletest, which was collecting output, to OOM.

Unsure if it's caused by this PR (in that maybe it increased the amount of logging?) but it also seems like we may just need to remove the test...

kennytm · Oct 31, 2017

check i686-pc-windows-msvc runs out of memory?

@bors retry

alexcrichton · Oct 31, 2017

@kennytm we may want to watch this test though, I suspect this won't be the first time we see this error. Also the test ran for at least a minute which is a bit much

kennytm · Oct 31, 2017

@alexcrichton Sorry did not see your reply when commenting. @bors r-

This test case has failed before, e.g. #44142 (comment) which was legit. Probably this diff caused a lot more logs to appear and failing the test case?

diff --git a/src/librustc_typeck/check/_match.rs b/src/librustc_typeck/check/_match.rs
index ab8994bcae25..e25f7d796689 100644
--- a/src/librustc_typeck/check/_match.rs
+++ b/src/librustc_typeck/check/_match.rs
@@ -68,7 +69,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
             PatKind::Binding(..) |
             PatKind::Ref(..) => false,
         };
-        if is_non_ref_pat && tcx.sess.features.borrow().match_default_bindings {
+        if is_non_ref_pat {
             debug!("pattern is non reference pattern");
             let mut exp_ty = self.resolve_type_vars_with_obligations(&expected);

nikomatsakis · Nov 1, 2017

@kennytm I cannot imagine how this change would affect that particular test -- it compiles an empty file, so I don't think that this code will trigger at all.

I do think the test is useful, but I wonder if we should consider e.g. directing the log output into /dev/null or something so as not to overwhelm compile-test. This could be a compiletest flag, or perhaps we can modify the test to invoke rustc as a subprocess or something.

Regardless, I'm inclined to r+ again because I don't think the problem is related to this patch per se. But I'll try to run a local test to verify.

nikomatsakis · Nov 2, 2017

Before:

lunch-box. RUST_LOG=debug /usr/bin/time -o killme.master rustc rustc-rust-log.rs 2>&1 | wc -c
606664960
lunch-box. cat killme.master
17.36user 8.18system 0:25.55elapsed 99%CPU (0avgtext+0avgdata 107360maxresident)k
32inputs+8488outputs (4major+20279minor)pagefaults 0swaps

With this branch:

lunch-box. RUST_LOG=debug /usr/bin/time -o killme rustc rustc-rust-log.rs 2>&1 | wc -c
606664960
lunch-box. cat killme
22.50user 8.48system 0:31.05elapsed 99%CPU (0avgtext+0avgdata 108668maxresident)k
0inputs+8488outputs (0major+20557minor)pagefaults 0swaps

No real difference. This branch for some reason took an extra few seconds, but it's probably because I was compiling other working directories at the same time or something.

nikomatsakis · Nov 2, 2017

@bors r+

bors · Nov 2, 2017

📌 Commit 9844777 has been approved by nikomatsakis

bors · Nov 2, 2017

⌛️ Testing commit 9844777 with merge e340996...

kennytm · Nov 2, 2017

AppVeyor passed 😕 I'm going to record that as a spurious error.

bors · Nov 2, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing e340996 to master...

rust-highfive assigned nikomatsakis Oct 20, 2017

kennytm added the S-waiting-on-review label Oct 20, 2017

tamird force-pushed the tamird:suggest-match-default-bindings branch 2 times, most recently from 264265f to bca2201 Oct 20, 2017

tamird force-pushed the tamird:suggest-match-default-bindings branch 3 times, most recently from a3dbc28 to 570f476 Oct 20, 2017

kennytm added S-waiting-on-author and removed S-waiting-on-review labels Oct 25, 2017

tamird force-pushed the tamird:suggest-match-default-bindings branch from 570f476 to bd5a587 Oct 26, 2017

kennytm added S-waiting-on-review and removed S-waiting-on-author labels Oct 26, 2017

typeck: suggest use of match_default_bindings feature
Fixes #45383. Updates #42640.

Verified

This commit was signed with a verified signature.

tamird Tamir Duberstein

GPG key ID: 1C1E98CC8E17BB89 Learn about signing commits

Loading status checks…

9844777

tamird force-pushed the tamird:suggest-match-default-bindings branch from bd5a587 to 9844777 Oct 30, 2017

kennytm added S-waiting-on-bors and removed S-waiting-on-review labels Oct 31, 2017

kennytm added S-waiting-on-review and removed S-waiting-on-bors labels Oct 31, 2017

bors merged commit 9844777 into rust-lang:master Nov 2, 2017
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

bors referenced this pull request Nov 2, 2017
Merged
new rules for merging expected and supplied types in closure signatures #45072

tamird deleted the tamird:suggest-match-default-bindings branch Nov 2, 2017

kennytm referenced this pull request Nov 3, 2017
Merged
add TerminatorKind::FalseEdges and use it in matches #45384

leonardo-m referenced this pull request Nov 3, 2017
Closed
Suggest simpler code in patterns #2201

rust-lang/rust

Join GitHub today

typeck: suggest use of match_default_bindings feature #45409

Conversation

tamird commented Oct 20, 2017 • edited

rust-highfive assigned nikomatsakis Oct 20, 2017

kennytm added the S-waiting-on-review label Oct 20, 2017

tamird force-pushed the tamird:suggest-match-default-bindings branch 2 times, most recently from 264265f to bca2201 Oct 20, 2017

This comment has been minimized.

tamird commented Oct 20, 2017

tamird force-pushed the tamird:suggest-match-default-bindings branch 3 times, most recently from a3dbc28 to 570f476 Oct 20, 2017

nikomatsakis requested changes Oct 23, 2017 View changes

nikomatsakis left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

kennytm added S-waiting-on-author and removed S-waiting-on-review labels Oct 25, 2017

tamird force-pushed the tamird:suggest-match-default-bindings branch from 570f476 to bd5a587 Oct 26, 2017

This comment has been minimized.

tamird commented Oct 26, 2017

kennytm added S-waiting-on-review and removed S-waiting-on-author labels Oct 26, 2017

nikomatsakis approved these changes Oct 30, 2017 View changes

This comment has been minimized.

nikomatsakis commented Oct 30, 2017

This comment has been minimized.

bors commented Oct 30, 2017

This comment has been minimized.

nikomatsakis commented Oct 30, 2017

This comment has been minimized.

tamird commented Oct 30, 2017

This comment has been minimized.

kennytm commented Oct 30, 2017

tamird force-pushed the tamird:suggest-match-default-bindings branch from bd5a587 to 9844777 Oct 30, 2017

This comment has been minimized.

tamird commented Oct 30, 2017

This comment has been minimized.

kennytm commented Oct 31, 2017

This comment has been minimized.

bors commented Oct 31, 2017

kennytm added S-waiting-on-bors and removed S-waiting-on-review labels Oct 31, 2017

This comment has been minimized.

bors commented Oct 31, 2017

bors added a commit that referenced this pull request Oct 31, 2017

This comment has been minimized.

bors commented Oct 31, 2017

This comment has been minimized.

kennytm commented Oct 31, 2017

This comment has been minimized.

bors commented Oct 31, 2017

bors added a commit that referenced this pull request Oct 31, 2017

This comment has been minimized.

bors commented Oct 31, 2017

This comment has been minimized.

alexcrichton commented Oct 31, 2017

This comment has been minimized.

kennytm commented Oct 31, 2017

This comment has been minimized.

alexcrichton commented Oct 31, 2017

This comment has been minimized.

kennytm commented Oct 31, 2017

kennytm added S-waiting-on-review and removed S-waiting-on-bors labels Oct 31, 2017

This comment has been minimized.

nikomatsakis commented Nov 1, 2017 • edited

This comment has been minimized.

nikomatsakis commented Nov 2, 2017

This comment has been minimized.

nikomatsakis commented Nov 2, 2017

This comment has been minimized.

bors commented Nov 2, 2017

This comment has been minimized.

bors commented Nov 2, 2017

bors added a commit that referenced this pull request Nov 2, 2017

This comment has been minimized.

tamird commented Oct 20, 2017 •

edited

tamird force-pushed the tamird:suggest-match-default-bindings branch 2 times, most recently from `264265f` to `bca2201` Oct 20, 2017

tamird force-pushed the tamird:suggest-match-default-bindings branch 3 times, most recently from `a3dbc28` to `570f476` Oct 20, 2017

nikomatsakis requested changes Oct 23, 2017

View changes

tamird force-pushed the tamird:suggest-match-default-bindings branch from `570f476` to `bd5a587` Oct 26, 2017

nikomatsakis approved these changes Oct 30, 2017

View changes

tamird force-pushed the tamird:suggest-match-default-bindings branch from `bd5a587` to `9844777` Oct 30, 2017

nikomatsakis commented Nov 1, 2017 •

edited

bors merged commit `9844777` into rust-lang:master Nov 2, 2017
2 checks passed