tests: Do not use `-Z parse-only`, continue compilation to test recovery #57379

petrochenkov · 2019-01-06T17:57:37Z

Make tests closer to reality!

The next step will be enabling -Z continue-parse-after-error by default and looking at the regressions.

A few instances of -Z parse-only are kept when it's appropriate, see e.g ui/impl-trait/impl-trait-plus-priority.rs, which tests mostly semantically wrong code and would generate too much useless noise if allowed to continue.

petrochenkov · 2019-01-06T17:58:48Z

cc @nikomatsakis @nrc just in case

Mark-Simulacrum · 2019-01-06T20:24:34Z

src/libsyntax/feature_gate.rs

@@ -1543,7 +1543,6 @@ impl<'a> Visitor<'a> for PostExpansionVisitor<'a> {

    fn visit_item(&mut self, i: &'a ast::Item) {
        match i.node {
-            ast::ItemKind::Static(..) |


Was this an intentional change?

Yes, test ui/parser/underscore_static.rs uncovered an unnecessary feature-gate error after removing -Z parse-only, so this change fixed it.

estebank

Thank you so much for taking the time to do this work!

I left a bunch of comments that are not review comments but rather a TODO list for things that we should clean up.

estebank · 2019-01-06T21:35:09Z

src/test/ui/parser/bad-match.stderr

   |
-LL |   let isize x = 5;
+LL |   let isize x = 5; //~ ERROR expected one of `:`, `;`, `=`, or `@`, found `x`


For future reference, I prefer to have the error comments just outside of view of the stderr files to make it easier to evaluate them in isolation :)

Hm, indeed.
That probably means one line below most of the time, like

let isize x = 5; //~^ ERROR expected one of `:`, `;`, `=`, or `@`, found `x`

?

Yeah, that's usually what I do. I only deviate from that when the line below would somehow still be part of the diagnostic, where I add as many ^ as necessary, but there's a point of diminishing returns.

when the line below would somehow still be part of the diagnostic, where I add as many ^ as necessary

We need //~v ERROR text text text 😄

estebank · 2019-01-06T21:40:31Z

src/test/ui/parser/bind-struct-early-modifiers.stderr

+  --> $DIR/bind-struct-early-modifiers.rs:4:9
+   |
+LL |         Foo { ref x: ref x } => {}, //~ ERROR expected `,`
+   |         ^^^^^^^^^^^^^^^^^^^^ missing field `x`


estebank · 2019-01-06T21:42:26Z

src/test/ui/parser/default.stderr

+   |     -------------------------- `foo` from trait
+...
+LL | impl Foo for u32 { //~ ERROR not all trait items implemented, missing: `foo`
+   | ^^^^^^^^^^^^^^^^ missing `foo` in implementation


This looks suspect, I'm guessing we're currently not continuing with the function as is when we encounter pub...

estebank · 2019-01-06T21:42:50Z

src/test/ui/parser/doc-before-rbrace.stderr

@@ -1,5 +1,5 @@
 error[E0585]: found a documentation comment that doesn't document anything
-  --> $DIR/doc-before-rbrace.rs:4:21
+  --> $DIR/doc-before-rbrace.rs:2:21
   |
 LL |     println!("Hi"); /// hi
   |                     ^^^^^^


Missing primary label.

estebank · 2019-01-06T21:44:51Z

src/test/ui/parser/issue-19096.stderr

+   |     ^^^ expected (), found integer
+   |
+   = note: expected type `()`
+              found type `{integer}`


This entire error is unfortunate.

estebank · 2019-01-06T21:45:42Z

src/test/ui/parser/issue-32214.stderr

-LL | pub fn test<W, I: Iterator<Item=(), W> >() {}
-   |                                     ^
+LL | pub fn test<W, I: Trait<Item=(), W> >() {}
+   |                                  ^


This should suggest the appropriate place for the type parameter, like we do for lifetimes.

estebank · 2019-01-06T21:48:24Z

src/test/ui/parser/lex-bad-char-literals-3.stderr

+   |     ^^^^ expected char, found reference
+   |
+   = note: expected type `char`
+              found type `&'static str`


We should give the recovered value TyErr (or even keep the char type with a placeholder value) instead of TyStr to avoid these.

estebank · 2019-01-06T21:50:57Z

src/test/ui/parser/struct-field-numeric-shorthand.stderr

@@ -1,10 +1,17 @@
 error: expected identifier, found `0`
-  --> $DIR/struct-field-numeric-shorthand.rs:6:19
+  --> $DIR/struct-field-numeric-shorthand.rs:4:19
   |
 LL |     let _ = Rgb { 0, 1, 2 }; //~ ERROR expected identifier, found `0`
   |             ---   ^ expected identifier
   |             |
   |             while parsing this struct


Ideally this should provide appropriate syntax suggestion.

estebank · 2019-01-06T21:52:09Z

src/test/ui/parser/struct-literal-in-match-discriminant.rs

 struct Foo {
    x: isize,
 }

 fn main() {
-    match Foo {
+    match Foo { //~ ERROR expected value, found struct `Foo`
        x: 3    //~ ERROR expected one of `=>`, `@`, `if`, or `|`, found `:`
    } {


This ideally would be recovered to avoid the knock down errors, but that's unlikely to happen soon.

estebank · 2019-01-06T21:52:45Z

src/test/ui/parser/trait-object-lifetime-parens.stderr

-LL | fn f<T: Copy + ('a)>() {} //~ ERROR parenthesized lifetime bounds are not supported
-   |                   ^
+LL | fn f<'a, T: Trait + ('a)>() {} //~ ERROR parenthesized lifetime bounds are not supported
+   |                        ^


Should suggest the appropriate syntax.

estebank · 2019-01-06T21:54:38Z

@bors r+

bors · 2019-01-06T21:54:39Z

📌 Commit 1f64f60 has been approved by estebank

bors · 2019-01-07T01:05:51Z

⌛ Testing commit 1f64f60 with merge 789a15a...

tests: Do not use `-Z parse-only`, continue compilation to test recovery Make tests closer to reality! The next step will be enabling `-Z continue-parse-after-error` by default and looking at the regressions. A few instances of `-Z parse-only` are kept when it's appropriate, see e.g `ui/impl-trait/impl-trait-plus-priority.rs`, which tests mostly semantically wrong code and would generate too much useless noise if allowed to continue.

bors · 2019-01-07T03:44:36Z

☀️ Test successful - status-appveyor, status-travis
Approved by: estebank
Pushing 789a15a to master...

@estebank

Filter away test annotations from UI test output If you worked with UI tests for some time you could notice one issue affecting their readability and also readability of diffs when the tests change. Look at the output of this test. ```rust fn main() { let 1 = 2; //~ ERROR refutable pattern in local binding } ``` ``` error[E0005]: refutable pattern in local binding: `-2147483648i32..=0i32` not covered --> src/main.rs:2:9 | 2 | let 1 = 2; //~ ERROR refutable pattern in local binding | ^ pattern `-2147483648i32..=0i32` not covered error: aborting due to previous error For more information about this error, try `rustc --explain E0005`. ``` You can see that the "refutable pattern in local binding" is duplicated. One instance is the actual error, and the second instance is the expected error annotation. This annotation is useful in the test input, but in the output it clutters the text and makes it harder to see what text refers to actual errors and what is just comments, especially if there are many errors in a single test file. @estebank [reported](#57379 (comment)) using the next trick to avoid the clutter ```rust fn main() { let 1 = 2; //~^ ERROR refutable pattern in local binding } ``` ``` error[E0005]: refutable pattern in local binding: `-2147483648i32..=0i32` not covered --> src/main.rs:2:9 | 2 | let 1 = 2; | ^ pattern `-2147483648i32..=0i32` not covered error: aborting due to previous error For more information about this error, try `rustc --explain E0005`. ``` , i.e. using `//~^` and placing the annotation one line below will remove the annotation from the output. However, this doesn't always works (consider errors with multi-line spans), and shouldn't be necessary in general! `compiletest` could automatically filter away its own annotations from the output instead. This is exactly what this PR does. r? @davidtwco

rust-highfive assigned estebank Jan 6, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 6, 2019

This comment has been minimized.

Sign in to view

tests: Do not use -Z parse-only, continue compilation to test recovery

1f64f60

petrochenkov force-pushed the parsrecov branch from 9fa2f02 to 1f64f60 Compare January 6, 2019 19:21

Mark-Simulacrum reviewed Jan 6, 2019

View reviewed changes

estebank approved these changes Jan 6, 2019

View reviewed changes

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-review Status: Awaiting review from the assignee but also interested parties. labels Jan 6, 2019

bors merged commit 1f64f60 into rust-lang:master Jan 7, 2019

bors mentioned this pull request Jan 7, 2019

Stabilize unrestricted_attribute_tokens #57367

Merged

petrochenkov mentioned this pull request Jan 12, 2019

Modify some parser diagnostics to continue evaluating beyond the parser #57540

Merged

petrochenkov mentioned this pull request Mar 9, 2019

Filter away test annotations from UI test output #59044

Merged

petrochenkov deleted the parsrecov branch June 5, 2019 16:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: Do not use `-Z parse-only`, continue compilation to test recovery #57379

tests: Do not use `-Z parse-only`, continue compilation to test recovery #57379

petrochenkov commented Jan 6, 2019 •

edited

Loading

petrochenkov commented Jan 6, 2019

This comment has been minimized.

Mark-Simulacrum Jan 6, 2019

petrochenkov Jan 6, 2019

estebank left a comment

estebank Jan 6, 2019

petrochenkov Jan 6, 2019

estebank Jan 6, 2019

petrochenkov Jan 6, 2019

estebank Jan 6, 2019

estebank Jan 6, 2019

estebank Jan 6, 2019

estebank Jan 6, 2019

estebank Jan 6, 2019

estebank Jan 6, 2019

estebank Jan 6, 2019

estebank Jan 6, 2019

estebank Jan 6, 2019

estebank Jan 6, 2019

estebank commented Jan 6, 2019

bors commented Jan 6, 2019

bors commented Jan 7, 2019

bors commented Jan 7, 2019

tests: Do not use -Z parse-only, continue compilation to test recovery #57379

tests: Do not use -Z parse-only, continue compilation to test recovery #57379

Conversation

petrochenkov commented Jan 6, 2019 • edited Loading

petrochenkov commented Jan 6, 2019

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank 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

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

estebank commented Jan 6, 2019

bors commented Jan 6, 2019

bors commented Jan 7, 2019

bors commented Jan 7, 2019

tests: Do not use `-Z parse-only`, continue compilation to test recovery #57379

tests: Do not use `-Z parse-only`, continue compilation to test recovery #57379

petrochenkov commented Jan 6, 2019 •

edited

Loading