Add error for `...` in expressions #45773

Badel2 · 2017-11-05T00:04:23Z

Follow-up to #44709
Tracking issue: #28237

Using ... in expressions was a warning, now it's an error
The error message suggests using .. or ..= instead, and explains the difference
Updated remaining occurrences of ... to ..=

r? petrochenkov

rust-highfive · 2017-11-05T00:04:34Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (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.

petrochenkov · 2017-11-05T00:15:04Z

I'm sad every time I have to @bors r+ this

bors · 2017-11-05T00:15:05Z

📌 Commit 0419a3b has been approved by petrochenkov

Badel2 · 2017-11-05T00:16:15Z

src/libsyntax/util/parser.rs

@@ -106,7 +106,8 @@ impl AssocOp {
            Token::OrOr => Some(LOr),
            Token::DotDot => Some(DotDot),
            Token::DotDotEq => Some(DotDotEq),
-            Token::DotDotDot => Some(DotDotEq), // remove this after SNAP
+            // DotDotDot is no longer supported, but we need some way to display the error
+            Token::DotDotDot => Some(DotDotEq),


My main concern is that in order to show the error we need to parse the expression, so here I'm parsing Token::DotDotDot as AssocOp::DotDotEq, and in src/libsyntax/parse/parser.rs I check for Token::DotDotDot and AssocOp::DotDotEq to show the error. An alternative would be returning None here but then the expression would be incomplete (adding the generic "expected one of" error).

I'm parsing Token::DotDotDot as AssocOp::DotDotEq

That's exactly what is needed for good error recovery.

Nice, thanks!

bors · 2017-11-06T00:35:39Z

⌛ Testing commit 0419a3b697be90d36efed6d91bee5c371e29f0a6 with merge c48637001106cd9eb78d037fb77aa7602d28f83c...

bors · 2017-11-06T02:02:08Z

💔 Test failed - status-travis

kennytm · 2017-11-06T04:33:59Z

Several run-pass/pretty tests failed, and one of them caused ICE.

run-pass/range_inclusive.rs

[01:22:25] ---- [pretty] run-pass/range_inclusive.rs stdout ----
[01:22:25] 	
[01:22:25] error: pretty-printing failed in round 1 revision None
[01:22:25] status: exit code: 101
[01:22:25] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "-" "-Zunstable-options" "--unpretty" "normal" "--target" "x86_64-unknown-linux-gnu" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/range_inclusive.stage2-x86_64-unknown-linux-gnu.pretty.aux" "-Crpath" "-O" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers"
[01:22:25] stdout:
[01:22:25] ------------------------------------------
[01:22:25] 
[01:22:25] ------------------------------------------
[01:22:25] stderr:
[01:22:25] ------------------------------------------
[01:22:25] error: `...` syntax cannot be used in expressions
[01:22:25]   --> <anon>:20:56
[01:22:25]    |
[01:22:25] 20 | fn return_range_to() -> RangeToInclusive<i32> { return ...1; }
[01:22:25]    |                                                        ^^^
[01:22:25]    |
[01:22:25]    = help: Use `..` if you need an exclusive range (a < b)
[01:22:25]    = help: or `..=` if you need an inclusive range (a <= b)
[01:22:25] 
[01:22:25] error: internal compiler error: unexpected panic
[01:22:25] 
[01:22:25] note: the compiler unexpectedly panicked. this is a bug.
[01:22:25] 
[01:22:25] note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
[01:22:25] 
[01:22:25] note: rustc 1.23.0-dev running on x86_64-unknown-linux-gnu
[01:22:25] 
[01:22:25] thread 'rustc' panicked at 'parse_prefix_range_expr: token DotDotDot is not DotDot/DotDotEq', /checkout/src/libsyntax/parse/parser.rs:3020:8
[01:22:25] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:22:25] 
[01:22:25] 
[01:22:25] ------------------------------------------
[01:22:25] 
[01:22:25] thread '[pretty] run-pass/range_inclusive.rs' panicked at 'explicit panic', /checkout/src/tools/compiletest/src/runtest.rs:2497:8
[01:22:25] note: Run with `RUST_BACKTRACE=1` for a backtrace.

run-pass/range_inclusive_gate.rs

[01:22:25] ---- [pretty] run-pass/range_inclusive_gate.rs stdout ----
[01:22:25] 	
[01:22:25] error: pretty-printing failed in round 1 revision None
[01:22:25] status: exit code: 101
[01:22:25] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "-" "-Zunstable-options" "--unpretty" "normal" "--target" "x86_64-unknown-linux-gnu" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/range_inclusive_gate.stage2-x86_64-unknown-linux-gnu.pretty.aux" "-Crpath" "-O" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers"
[01:22:25] stdout:
[01:22:25] ------------------------------------------
[01:22:25] // Copyright 2016 The Rust Project Developers. See the COPYRIGHT
[01:22:25] // file at the top-level directory of this distribution and at
[01:22:25] // http://rust-lang.org/COPYRIGHT.
[01:22:25] //
[01:22:25] // Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
[01:22:25] // http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
[01:22:25] // <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
[01:22:25] // option. This file may not be copied, modified, or distributed
[01:22:25] // except according to those terms.
[01:22:25] 
[01:22:25] // Test that you only need the syntax gate if you don't mention the structs.
[01:22:25] 
[01:22:25] #![feature(inclusive_range_syntax)]
[01:22:25] 
[01:22:25] fn main() {
[01:22:25]     let mut count = 0;
[01:22:25]     for i in 0_usize...10 { assert!(i >= 0 && i <= 10); count += i; }
[01:22:25]     assert_eq!(count , 55);
[01:22:25] }
[01:22:25] 
[01:22:25] 
[01:22:25] ------------------------------------------
[01:22:25] stderr:
[01:22:25] ------------------------------------------
[01:22:25] error: `...` syntax cannot be used in expressions
[01:22:25]   --> <anon>:17:21
[01:22:25]    |
[01:22:25] 17 |     for i in 0_usize...10 { assert!(i >= 0 && i <= 10); count += i; }
[01:22:25]    |                     ^^^
[01:22:25]    |
[01:22:25]    = help: Use `..` if you need an exclusive range (a < b)
[01:22:25]    = help: or `..=` if you need an inclusive range (a <= b)
[01:22:25] 
[01:22:25] error: aborting due to previous error

Badel2 · 2017-11-06T10:30:15Z

I'm confused. The error says that line 20 of range_inclusive.rs is ... when it was changed to ..=:

rust/src/test/run-pass/range_inclusive.rs

Line 20 in 54bbd56

fn return_range_to() -> RangeToInclusive<i32> { return ..=1; }

petrochenkov · 2017-11-06T11:10:42Z

@Badel2
The error may refer to the pretty-printed version of range_inclusive.rs which still may have ... if some code in pretty-printing wasn't updated.

Could you also add a test checking that if ... is actually written (...b and a ... b), then recovery works and no ICE happens.

Badel2 · 2017-11-06T12:13:31Z

I have added a parse-fail test, but I can't find the pretty-printing code. The closest I got is the code for ranges in patterns, but I can't find the one for expressions.

rust/src/librustc/hir/print.rs

Line 1821 in 19402f1

RangeEnd::Included => self.s.word("...")?,

petrochenkov · 2017-11-06T12:53:20Z

@Badel2
That's HIR pretty-printing.
There's also AST pretty-printing in libsyntax/print/pprust.rs and it's AST pretty-printing which is tested by pretty tests.
This line in particular looks suspicious

rust/src/libsyntax/print/pprust.rs

Line 2207 in 19402f1

self.s.word("...")?;

Badel2 · 2017-11-06T15:26:07Z

@petrochenkov Thanks! I hope it's fixed now.

petrochenkov · 2017-11-06T15:28:01Z

@bors r+

bors · 2017-11-06T15:28:01Z

📌 Commit c0addf6 has been approved by petrochenkov

Badel2 · 2017-11-06T18:10:16Z

That's strange, in my machine I run ./x.py test src/test/parse-fail/ and it works... According to the logs it only detected the first error. I have no idea why :)

petrochenkov · 2017-11-06T18:22:28Z

I see why only the first error is detected, the test is missing

// compile-flags: -Z parse-only -Z continue-parse-after-error

, but it should fail locally as well.

(-Z continue-parse-after-error allows reporting multiple errors during parsing phase, -Z parse-only is the only thing that differs parse-fail tests from compile-fail tests (less work for the compiler)).

Badel2 · 2017-11-06T19:29:20Z

@petrochenkov Same result, it fails on travis but it works on my machine. I haven't run ./x.py clean since the first commit 3 days ago, so maybe I'm using an outdated tool or something.

petrochenkov · 2017-11-06T19:44:39Z

src/libsyntax/parse/parser.rs

+            self.err_dotdotdot_syntax(self.span);
+        }
+
+        debug_assert!([token::DotDot, token::DotDotEq].contains(&self.token),


It looks like this debug_assert may be firing on CI.
IIRC, these kinds of asserts are enabled on PR merge, but disabled by default for local builds.

Ok, for some reason I thought the code would stop executing after the err.emit(). Now it all makes sense. Thank you very much!

petrochenkov · 2017-11-07T08:02:45Z

@bors r+

bors · 2017-11-07T08:02:46Z

📌 Commit b81a7b3 has been approved by petrochenkov

bors · 2017-11-10T01:40:27Z

⌛ Testing commit b81a7b3 with merge d5ff0e6...

Add error for `...` in expressions Follow-up to #44709 Tracking issue: #28237 * Using `...` in expressions was a warning, now it's an error * The error message suggests using `..` or `..=` instead, and explains the difference * Updated remaining occurrences of `...` to `..=` r? petrochenkov

bors · 2017-11-10T03:59:34Z

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

rust-highfive assigned nikomatsakis Nov 5, 2017

Badel2 commented Nov 5, 2017

View reviewed changes

kennytm added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Nov 5, 2017

Badel2 mentioned this pull request Nov 5, 2017

fixed range warning #45769

Closed

petrochenkov assigned petrochenkov and unassigned nikomatsakis Nov 5, 2017

Inclusive range updated to ..= syntax

4bd6be9

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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 6, 2017

Badel2 force-pushed the dotdoteq branch from 81ea2db to c0addf6 Compare November 6, 2017 15:24

Badel2 force-pushed the dotdoteq branch from c0addf6 to d1d9298 Compare November 6, 2017 18:25

Badel2 added 2 commits November 6, 2017 20:33

Using ... in expressions is now an error

3c41c28

Test error for ... in expressions

b81a7b3

petrochenkov reviewed Nov 6, 2017

View reviewed changes

Badel2 force-pushed the dotdoteq branch from d1d9298 to b81a7b3 Compare November 6, 2017 20:41

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 7, 2017

bors merged commit b81a7b3 into rust-lang:master Nov 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add error for `...` in expressions #45773

Add error for `...` in expressions #45773

Badel2 commented Nov 5, 2017

rust-highfive commented Nov 5, 2017

petrochenkov commented Nov 5, 2017

bors commented Nov 5, 2017

Badel2 Nov 5, 2017

petrochenkov Nov 5, 2017

Badel2 Nov 5, 2017

bors commented Nov 6, 2017

bors commented Nov 6, 2017

kennytm commented Nov 6, 2017

Badel2 commented Nov 6, 2017

petrochenkov commented Nov 6, 2017

Badel2 commented Nov 6, 2017

petrochenkov commented Nov 6, 2017

Badel2 commented Nov 6, 2017

petrochenkov commented Nov 6, 2017

bors commented Nov 6, 2017

Badel2 commented Nov 6, 2017

petrochenkov commented Nov 6, 2017

Badel2 commented Nov 6, 2017

petrochenkov Nov 6, 2017

Badel2 Nov 6, 2017

petrochenkov commented Nov 7, 2017

bors commented Nov 7, 2017

bors commented Nov 10, 2017

bors commented Nov 10, 2017

Add error for ... in expressions #45773

Add error for ... in expressions #45773

Conversation

Badel2 commented Nov 5, 2017

rust-highfive commented Nov 5, 2017

petrochenkov commented Nov 5, 2017

bors commented Nov 5, 2017

Badel2 Nov 5, 2017

Choose a reason for hiding this comment

petrochenkov Nov 5, 2017

Choose a reason for hiding this comment

Badel2 Nov 5, 2017

Choose a reason for hiding this comment

bors commented Nov 6, 2017

bors commented Nov 6, 2017

kennytm commented Nov 6, 2017

Badel2 commented Nov 6, 2017

petrochenkov commented Nov 6, 2017

Badel2 commented Nov 6, 2017

petrochenkov commented Nov 6, 2017

Badel2 commented Nov 6, 2017

petrochenkov commented Nov 6, 2017

bors commented Nov 6, 2017

Badel2 commented Nov 6, 2017

petrochenkov commented Nov 6, 2017

Badel2 commented Nov 6, 2017

petrochenkov Nov 6, 2017

Choose a reason for hiding this comment

Badel2 Nov 6, 2017

Choose a reason for hiding this comment

petrochenkov commented Nov 7, 2017

bors commented Nov 7, 2017

bors commented Nov 10, 2017

bors commented Nov 10, 2017

Add error for `...` in expressions #45773

Add error for `...` in expressions #45773