New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not emit type errors on recovered blocks #46732

Merged
merged 2 commits into from Dec 22, 2017

Conversation

Projects
None yet
6 participants
@estebank
Contributor

estebank commented Dec 14, 2017

When a parse error occurs on a block, the parser will recover and create
a block with the statements collected until that point. Now a flag
stating that a recovery has been performed in this block is propagated
so that the type checker knows that the type of the block (which will be
identified as ()) shouldn't be checked against the expectation to
reduce the amount of irrelevant diagnostic errors shown to the user.

Fix #44579.

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Dec 14, 2017

Collaborator

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

Collaborator

rust-highfive commented Dec 14, 2017

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Dec 14, 2017

Contributor

The case reported in #44579 is 100% reasonable recovery.
The "mismatched types" error is exactly what would be reported if the pub struct Pub items wasn't ill-formed, so I'd close this as not-a-defect.

(Also, the "unmatched visibility" wording was really weird and unhelpful (unmatched with what?), it's really nice that it's changed to "missing struct" on nightly.)

Contributor

petrochenkov commented Dec 14, 2017

The case reported in #44579 is 100% reasonable recovery.
The "mismatched types" error is exactly what would be reported if the pub struct Pub items wasn't ill-formed, so I'd close this as not-a-defect.

(Also, the "unmatched visibility" wording was really weird and unhelpful (unmatched with what?), it's really nice that it's changed to "missing struct" on nightly.)

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Dec 14, 2017

Contributor

Output on nightly, the first error is much more visible now:

error: missing `struct` for struct definition
  --> src/main.rs:11:8
   |
11 |     pub Foo { text }    // NB: Syntax error here
   |        ^
help: add `struct` here to parse `Foo` as a public struct
   |
11 |     pub struct Foo { text }    // NB: Syntax error here
   |         ^^^^^^

error[E0308]: mismatched types
  --> src/main.rs:7:23
   |
7  |   pub fn parse() -> Foo {
   |  _______________________^
8  | |     let args: Vec<String> = env::args().collect();
9  | |     let text = args[1].clone();
10 | |     
11 | |     pub Foo { text }    // NB: Syntax error here
12 | | }
   | |_^ expected struct `Foo`, found ()
   |
   = note: expected type `Foo`
              found type `()`
Contributor

petrochenkov commented Dec 14, 2017

Output on nightly, the first error is much more visible now:

error: missing `struct` for struct definition
  --> src/main.rs:11:8
   |
11 |     pub Foo { text }    // NB: Syntax error here
   |        ^
help: add `struct` here to parse `Foo` as a public struct
   |
11 |     pub struct Foo { text }    // NB: Syntax error here
   |         ^^^^^^

error[E0308]: mismatched types
  --> src/main.rs:7:23
   |
7  |   pub fn parse() -> Foo {
   |  _______________________^
8  | |     let args: Vec<String> = env::args().collect();
9  | |     let text = args[1].clone();
10 | |     
11 | |     pub Foo { text }    // NB: Syntax error here
12 | | }
   | |_^ expected struct `Foo`, found ()
   |
   = note: expected type `Foo`
              found type `()`
@estebank

This comment has been minimized.

Show comment
Hide comment
@estebank

estebank Dec 14, 2017

Contributor

@petrochenkov the first error is more prominent now in this case, but any syntax error will cause the type error to happen. Consider the following case instead:

pub struct Foo {
    text: String
}

pub fn parse() -> Foo {
    fn
    Foo { text: "".to_string() }
}
error: expected one of `(` or `<`, found `{`
 --> src/main.rs:7:9
  |
7 |     Foo { text: "".to_string() }
  |         ^ expected one of `(` or `<` here

error[E0308]: mismatched types
 --> src/main.rs:5:23
  |
5 |   pub fn parse() -> Foo {
  |  _______________________^
6 | |     fn
7 | |     Foo { text: "".to_string() }
8 | | }
  | |_^ expected struct `Foo`, found ()
  |
  = note: expected type `Foo`
             found type `()`

error: aborting due to 2 previous errors

The second error won't happen once the syntax error is fixed, but the compiler can't know that, and it is still the most vertically prominent error in the output. I feel that masking the type error when we know the block had a parse error (that we made an effort to continue after) is a better default.

Contributor

estebank commented Dec 14, 2017

@petrochenkov the first error is more prominent now in this case, but any syntax error will cause the type error to happen. Consider the following case instead:

pub struct Foo {
    text: String
}

pub fn parse() -> Foo {
    fn
    Foo { text: "".to_string() }
}
error: expected one of `(` or `<`, found `{`
 --> src/main.rs:7:9
  |
7 |     Foo { text: "".to_string() }
  |         ^ expected one of `(` or `<` here

error[E0308]: mismatched types
 --> src/main.rs:5:23
  |
5 |   pub fn parse() -> Foo {
  |  _______________________^
6 | |     fn
7 | |     Foo { text: "".to_string() }
8 | | }
  | |_^ expected struct `Foo`, found ()
  |
  = note: expected type `Foo`
             found type `()`

error: aborting due to 2 previous errors

The second error won't happen once the syntax error is fixed, but the compiler can't know that, and it is still the most vertically prominent error in the output. I feel that masking the type error when we know the block had a parse error (that we made an effort to continue after) is a better default.

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 15, 2017

Contributor

☔️ The latest upstream changes (presumably #46641) made this pull request unmergeable. Please resolve the merge conflicts.

Contributor

bors commented Dec 15, 2017

☔️ The latest upstream changes (presumably #46641) made this pull request unmergeable. Please resolve the merge conflicts.

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Dec 15, 2017

Contributor

If some piece of code look so similar to a legal statement that we recover it as a legal statement (e.g. an item statement in case of pub struct Foo), then it should not poison the outer block in which it's defined, only possibly its inner block.

pub fn parse() -> Foo { <not_poisoned>
    let args: Vec<String> = env::args().collect();
    let text = args[1].clone();
    
    pub Foo { <poisoned> text </poisoned> }
</not_poisoned> }

In the example with fn it looks like we don't recover fn Foo { text: "".to_string() } as a function item statement, it's interpreted as just some random token soup. In this case it should indeed poison the block in which it's defined.

pub fn parse() -> Foo { <poisoned>
    fn
    Foo { text: "".to_string() }
</poisoned> }

I need to make some experiments.

Contributor

petrochenkov commented Dec 15, 2017

If some piece of code look so similar to a legal statement that we recover it as a legal statement (e.g. an item statement in case of pub struct Foo), then it should not poison the outer block in which it's defined, only possibly its inner block.

pub fn parse() -> Foo { <not_poisoned>
    let args: Vec<String> = env::args().collect();
    let text = args[1].clone();
    
    pub Foo { <poisoned> text </poisoned> }
</not_poisoned> }

In the example with fn it looks like we don't recover fn Foo { text: "".to_string() } as a function item statement, it's interpreted as just some random token soup. In this case it should indeed poison the block in which it's defined.

pub fn parse() -> Foo { <poisoned>
    fn
    Foo { text: "".to_string() }
</poisoned> }

I need to make some experiments.

}))
}
/// Parse a statement, including the trailing semicolon.
pub fn parse_full_stmt(&mut self, macro_legacy_warnings: bool) -> PResult<'a, Option<Stmt>> {
let mut stmt = match self.parse_stmt_(macro_legacy_warnings) {
let mut stmt = match self.parse_stmt_without_recovery(macro_legacy_warnings)? {

This comment has been minimized.

@petrochenkov

petrochenkov Dec 16, 2017

Contributor

This seems to be a change independent from adding the recovered flag to blocks?
From what I see in the test diffs I'd keep this recovery in place.

@petrochenkov

petrochenkov Dec 16, 2017

Contributor

This seems to be a change independent from adding the recovered flag to blocks?
From what I see in the test diffs I'd keep this recovery in place.

This comment has been minimized.

@estebank

estebank Dec 17, 2017

Contributor

The recovery happens in the callee already, and using parse_stmt_ swallows errors that we do want to see in parse_block_tail.

@estebank

estebank Dec 17, 2017

Contributor

The recovery happens in the callee already, and using parse_stmt_ swallows errors that we do want to see in parse_block_tail.

pub Foo { text }
}
//~^^ ERROR missing `struct` for struct definition

This comment has been minimized.

@petrochenkov

petrochenkov Dec 16, 2017

Contributor

Could you add the example with fn (#46732 (comment)) as a test too?

@petrochenkov

petrochenkov Dec 16, 2017

Contributor

Could you add the example with fn (#46732 (comment)) as a test too?

This comment has been minimized.

@estebank

estebank Dec 17, 2017

Contributor

added

@estebank

estebank Dec 17, 2017

Contributor

added

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Dec 16, 2017

Contributor

r=me with comments addressed

I still want to look at the statement recovery more carefully, but not right now (probably replace the recovered flag with ExprKind::Err and statements based on it). Into backlog it goes...

Contributor

petrochenkov commented Dec 16, 2017

r=me with comments addressed

I still want to look at the statement recovery more carefully, but not right now (probably replace the recovered flag with ExprKind::Err and statements based on it). Into backlog it goes...

@estebank

This comment has been minimized.

Show comment
Hide comment
@estebank

estebank Dec 18, 2017

Contributor

@bors r=petrochencov

Contributor

estebank commented Dec 18, 2017

@bors r=petrochencov

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 18, 2017

Contributor

📌 Commit 351f06a has been approved by petrochencov

Contributor

bors commented Dec 18, 2017

📌 Commit 351f06a has been approved by petrochencov

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 18, 2017

Contributor

⌛️ Testing commit 351f06a with merge 184693d...

Contributor

bors commented Dec 18, 2017

⌛️ Testing commit 351f06a with merge 184693d...

bors added a commit that referenced this pull request Dec 18, 2017

Auto merge of #46732 - estebank:silence-recovered-blocks, r=petrochencov
Do not emit type errors on recovered blocks

When a parse error occurs on a block, the parser will recover and create
a block with the statements collected until that point. Now a flag
stating that a recovery has been performed in this block is propagated
so that the type checker knows that the type of the block (which will be
identified as `()`) shouldn't be checked against the expectation to
reduce the amount of irrelevant diagnostic errors shown to the user.

Fix #44579.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 18, 2017

Contributor

💔 Test failed - status-appveyor

Contributor

bors commented Dec 18, 2017

💔 Test failed - status-appveyor

@estebank

This comment has been minimized.

Show comment
Hide comment
@estebank

estebank Dec 18, 2017

Contributor

@bors r-

rustfmt needs to be updated with this change for this PR to be able to land:

error[E0063]: missing field `recovered` in initializer of `syntax::ast::Block`
   --> src\tools\rustfmt\src\closures.rs:127:17
    |
127 |     let block = ast::Block {
    |                 ^^^^^^^^^^ missing `recovered`
error: aborting due to previous error

How do we deal with cross-repo commits?

Contributor

estebank commented Dec 18, 2017

@bors r-

rustfmt needs to be updated with this change for this PR to be able to land:

error[E0063]: missing field `recovered` in initializer of `syntax::ast::Block`
   --> src\tools\rustfmt\src\closures.rs:127:17
    |
127 |     let block = ast::Block {
    |                 ^^^^^^^^^^ missing `recovered`
error: aborting due to previous error

How do we deal with cross-repo commits?

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Dec 18, 2017

Contributor

How do we deal with cross-repo commits?

rustfmt and rls are marked as "Broken" in https://github.com/rust-lang/rust/blob/master/src/tools/toolstate.toml, a pull request to rustfmt can be sent asyncroniously.

Contributor

petrochenkov commented Dec 18, 2017

How do we deal with cross-repo commits?

rustfmt and rls are marked as "Broken" in https://github.com/rust-lang/rust/blob/master/src/tools/toolstate.toml, a pull request to rustfmt can be sent asyncroniously.

@estebank

This comment has been minimized.

Show comment
Hide comment
@estebank

estebank Dec 18, 2017

Contributor

Already marked as broken, looks like the failure was due to sscache.

@bors r=petrochenkov

Contributor

estebank commented Dec 18, 2017

Already marked as broken, looks like the failure was due to sscache.

@bors r=petrochenkov

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 18, 2017

Contributor

📌 Commit 351f06a has been approved by petrochenkov

Contributor

bors commented Dec 18, 2017

📌 Commit 351f06a has been approved by petrochenkov

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 19, 2017

Contributor

⌛️ Testing commit 351f06a with merge a6c1141...

Contributor

bors commented Dec 19, 2017

⌛️ Testing commit 351f06a with merge a6c1141...

bors added a commit that referenced this pull request Dec 19, 2017

Auto merge of #46732 - estebank:silence-recovered-blocks, r=petrochenkov
Do not emit type errors on recovered blocks

When a parse error occurs on a block, the parser will recover and create
a block with the statements collected until that point. Now a flag
stating that a recovery has been performed in this block is propagated
so that the type checker knows that the type of the block (which will be
identified as `()`) shouldn't be checked against the expectation to
reduce the amount of irrelevant diagnostic errors shown to the user.

Fix #44579.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 19, 2017

Contributor

💔 Test failed - status-travis

Contributor

bors commented Dec 19, 2017

💔 Test failed - status-travis

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Dec 19, 2017

Member

RLS and rustfmt are still broken.

Member

kennytm commented Dec 19, 2017

RLS and rustfmt are still broken.

estebank added some commits Dec 14, 2017

Do not emit type errors on recovered blocks
When a parse error occurs on a block, the parser will recover and create
a block with the statements collected until that point. Now a flag
stating that a recovery has been performed in this block is propagated
so that the type checker knows that the type of the block (which will be
identified as `()`) shouldn't be checked against the expectation to
reduce the amount of irrelevant diagnostic errors shown to the user.
@estebank

This comment has been minimized.

Show comment
Hide comment
@estebank

estebank Dec 21, 2017

Contributor

@bors try

Contributor

estebank commented Dec 21, 2017

@bors try

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 21, 2017

Contributor

⌛️ Trying commit d90d5d1 with merge 91fe28d...

Contributor

bors commented Dec 21, 2017

⌛️ Trying commit d90d5d1 with merge 91fe28d...

bors added a commit that referenced this pull request Dec 21, 2017

Auto merge of #46732 - estebank:silence-recovered-blocks, r=<try>
Do not emit type errors on recovered blocks

When a parse error occurs on a block, the parser will recover and create
a block with the statements collected until that point. Now a flag
stating that a recovery has been performed in this block is propagated
so that the type checker knows that the type of the block (which will be
identified as `()`) shouldn't be checked against the expectation to
reduce the amount of irrelevant diagnostic errors shown to the user.

Fix #44579.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 22, 2017

Contributor

☀️ Test successful - status-travis
State: approved= try=True

Contributor

bors commented Dec 22, 2017

☀️ Test successful - status-travis
State: approved= try=True

@estebank

This comment has been minimized.

Show comment
Hide comment
@estebank

estebank Dec 22, 2017

Contributor

@bors r=petrochenkov

Contributor

estebank commented Dec 22, 2017

@bors r=petrochenkov

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 22, 2017

Contributor

📌 Commit d90d5d1 has been approved by petrochenkov

Contributor

bors commented Dec 22, 2017

📌 Commit d90d5d1 has been approved by petrochenkov

bors added a commit that referenced this pull request Dec 22, 2017

Auto merge of #46732 - estebank:silence-recovered-blocks, r=petrochenkov
Do not emit type errors on recovered blocks

When a parse error occurs on a block, the parser will recover and create
a block with the statements collected until that point. Now a flag
stating that a recovery has been performed in this block is propagated
so that the type checker knows that the type of the block (which will be
identified as `()`) shouldn't be checked against the expectation to
reduce the amount of irrelevant diagnostic errors shown to the user.

Fix #44579.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 22, 2017

Contributor

⌛️ Testing commit d90d5d1 with merge c2ecab1...

Contributor

bors commented Dec 22, 2017

⌛️ Testing commit d90d5d1 with merge c2ecab1...

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 22, 2017

Contributor

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

Contributor

bors commented Dec 22, 2017

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

@bors bors merged commit d90d5d1 into rust-lang:master Dec 22, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@bluss

This comment has been minimized.

Show comment
Hide comment
@bluss

bluss Jan 1, 2018

Contributor

Awesome! Thanks a lot for this

Contributor

bluss commented Jan 1, 2018

Awesome! Thanks a lot for this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment