typeck: suggest use of match_default_bindings feature #45409

tamird · 2017-10-20T13:04:34Z

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

tamird · 2017-10-20T17:34:25Z

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

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

nikomatsakis · 2017-10-23T17:25:02Z

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 · 2017-10-23T17:26:50Z

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 · 2017-10-23T17:26:55Z

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 · 2017-10-23T17:26:58Z

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 · 2017-10-23T17:27:02Z

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 · 2017-10-26T17:30:59Z

@kennytm should be waiting-on-review again.

nikomatsakis · 2017-10-30T15:06:00Z

@bors r+

bors · 2017-10-30T15:06:02Z

📌 Commit bd5a587 has been approved by nikomatsakis

nikomatsakis · 2017-10-30T15:06:27Z

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

tamird · 2017-10-30T15:15:08Z

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 · 2017-10-30T15:24:18Z

@bors r-

(Waiting on author to cleanup.)

Fixes rust-lang#45383. Updates rust-lang#42640.

tamird · 2017-10-30T23:32:47Z

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

kennytm · 2017-10-31T09:34:16Z

@bors r=nikomatsakis

bors · 2017-10-31T09:34:16Z

📌 Commit 9844777 has been approved by nikomatsakis

bors · 2017-10-31T10:24:18Z

⌛ Testing commit 9844777 with merge 772b3b75fb91588606b396da6e00fc91e447fce9...

bors · 2017-10-31T12:02:08Z

💔 Test failed - status-travis

kennytm · 2017-10-31T12:12:13Z

@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 · 2017-10-31T13:38:45Z

⌛ Testing commit 9844777 with merge a274ae0746b48c8dc7926e35a75b690a2b02e059...

bors · 2017-10-31T14:56:02Z

💔 Test failed - status-appveyor

alexcrichton · 2017-10-31T15:03:57Z

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 · 2017-10-31T15:04:25Z

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

@bors retry

alexcrichton · 2017-10-31T15:07:54Z

@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 · 2017-10-31T15:28:15Z

@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 · 2017-11-01T13:45:15Z

@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 · 2017-11-02T14:13:46Z

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 · 2017-11-02T14:13:51Z

@bors r+

bors · 2017-11-02T14:13:52Z

📌 Commit 9844777 has been approved by nikomatsakis

bors · 2017-11-02T16:28:24Z

⌛ Testing commit 9844777 with merge e340996...

@nikomatsakis

…atsakis typeck: suggest use of match_default_bindings feature Fixes #45383. Updates #42640. r? @nikomatsakis cc @tschottdorf This needs a UI test, but thought I'd get some early feedback.

kennytm · 2017-11-02T19:49:33Z

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

bors · 2017-11-02T21:26:14Z

☀️ 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 Status: Awaiting review from the assignee but also interested parties. label Oct 20, 2017

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

tamird force-pushed the suggest-match-default-bindings branch 3 times, most recently from a3dbc28 to 570f476 Compare October 21, 2017 11:18

nikomatsakis requested changes Oct 23, 2017

View reviewed changes

kennytm 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 25, 2017

tamird force-pushed the suggest-match-default-bindings branch from 570f476 to bd5a587 Compare October 26, 2017 01:55

kennytm 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 (such as code changes or more information) from the author. labels Oct 26, 2017

nikomatsakis approved these changes Oct 30, 2017

View reviewed changes

typeck: suggest use of match_default_bindings feature

9844777

Fixes rust-lang#45383. Updates rust-lang#42640.

tamird force-pushed the suggest-match-default-bindings branch from bd5a587 to 9844777 Compare October 30, 2017 23:09

kennytm 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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 31, 2017

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 31, 2017

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

bors mentioned this pull request Nov 2, 2017

new rules for merging expected and supplied types in closure signatures #45072

Merged

tamird deleted the suggest-match-default-bindings branch November 2, 2017 22:00

kennytm mentioned this pull request Nov 3, 2017

add TerminatorKind::FalseEdges and use it in matches #45384

Merged

leonardo-m mentioned this pull request Nov 3, 2017

Suggest simpler code in patterns rust-lang/rust-clippy#2201

Closed

typeck: suggest use of match_default_bindings feature #45409

typeck: suggest use of match_default_bindings feature #45409

Conversation

tamird commented Oct 20, 2017 • edited Loading

tamird commented Oct 20, 2017

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamird commented Oct 26, 2017

nikomatsakis commented Oct 30, 2017

bors commented Oct 30, 2017

nikomatsakis commented Oct 30, 2017

tamird commented Oct 30, 2017 via email

kennytm commented Oct 30, 2017

tamird commented Oct 30, 2017

kennytm commented Oct 31, 2017

bors commented Oct 31, 2017

bors commented Oct 31, 2017

bors commented Oct 31, 2017

kennytm commented Oct 31, 2017

bors commented Oct 31, 2017

bors commented Oct 31, 2017

alexcrichton commented Oct 31, 2017

kennytm commented Oct 31, 2017

alexcrichton commented Oct 31, 2017

kennytm commented Oct 31, 2017

nikomatsakis commented Nov 1, 2017 • edited Loading

nikomatsakis commented Nov 2, 2017

nikomatsakis commented Nov 2, 2017

bors commented Nov 2, 2017

bors commented Nov 2, 2017

kennytm commented Nov 2, 2017

bors commented Nov 2, 2017

tamird commented Oct 20, 2017 •

edited

Loading

nikomatsakis commented Nov 1, 2017 •

edited

Loading