Skip to content
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

parse errors result in massive, useless spew #31994

Closed
brson opened this issue Mar 1, 2016 · 14 comments
Closed

parse errors result in massive, useless spew #31994

brson opened this issue Mar 1, 2016 · 14 comments
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints

Comments

@brson
Copy link
Contributor

brson commented Mar 1, 2016

In a recent project I had a single typo - failing to close a paren. The error message is below. Remember, there is only one error here, a missing paren. Every single error here is useless. This happens a lot.

rust-install/src/mock/clitools.rs:336:17: 336:18 error: incorrect close delimiter: `]`
rust-install/src/mock/clitools.rs:336                 ]
                                                      ^
rust-install/src/mock/clitools.rs:333:13: 333:14 note: unclosed delimiter
rust-install/src/mock/clitools.rs:333             ("rustc".to_string(),
                                                  ^
rust-install/src/mock/clitools.rs:337:5: 337:6 error: incorrect close delimiter: `}`
rust-install/src/mock/clitools.rs:337     }
                                          ^
rust-install/src/mock/clitools.rs:332:25: 332:26 note: unclosed delimiter
rust-install/src/mock/clitools.rs:332         components: vec![
                                                              ^
rust-install/src/mock/clitools.rs:479:3: 479:3 error: this file contains an un-closed delimiter
rust-install/src/mock/clitools.rs:479 }
                                       ^
rust-install/src/mock/clitools.rs:329:90: 329:91 help: did you mean to close this delimiter?
rust-install/src/mock/clitools.rs:329 fn build_mock_rustc_installer(version: &str, version_hash: &str) -> MockInstallerBuilder {
                                                                                                                               ^
rust-install/src/mock/clitools.rs:340:1: 340:3 error: expected one of `.`, `;`, `}`, or an operator, found `fn`
rust-install/src/mock/clitools.rs:340 fn build_mock_cargo_installer(version: &str, version_hash: &str) -> MockInstallerBuilder {
                                      ^~
rust-install/src/mock/clitools.rs:73:5: 73:29 error: unresolved name `create_custom_toolchains` [E0425]
rust-install/src/mock/clitools.rs:73     create_custom_toolchains(config.customdir.path());
                                         ^~~~~~~~~~~~~~~~~~~~~~~~
rust-install/src/mock/clitools.rs:73:5: 73:29 help: run `rustc --explain E0425` to see a detailed explanation
rust-install/src/mock/clitools.rs:230:17: 230:43 error: unresolved name `build_mock_cargo_installer` [E0425]
rust-install/src/mock/clitools.rs:230     let cargo = build_mock_cargo_installer(version, version_hash);
                                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~
rust-install/src/mock/clitools.rs:230:17: 230:43 help: run `rustc --explain E0425` to see a detailed explanation
rust-install/src/mock/clitools.rs:231:21: 231:50 error: unresolved name `build_mock_rust_doc_installer` [E0425]
rust-install/src/mock/clitools.rs:231     let rust_docs = build_mock_rust_doc_installer(channel);
                                                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
rust-install/src/mock/clitools.rs:231:21: 231:50 help: run `rustc --explain E0425` to see a detailed explanation
rust-install/src/mock/clitools.rs:232:16: 232:40 error: unresolved name `build_combined_installer` [E0425]
rust-install/src/mock/clitools.rs:232     let rust = build_combined_installer(&[&std, &rustc, &cargo, &rust_docs]);
                                                     ^~~~~~~~~~~~~~~~~~~~~~~~
rust-install/src/mock/clitools.rs:232:16: 232:40 help: run `rustc --explain E0425` to see a detailed explanation
rust-install/src/mock/clitools.rs:335:27: 335:35 error: unresolved name `mock_bin` [E0425]
rust-install/src/mock/clitools.rs:335              vec![(rustc, mock_bin("rustc", version, version_hash))]
                                                                ^~~~~~~~
rust-install/src/mock/clitools.rs:335:14: 335:69 note: in this expansion of vec! (defined in <std macros>)
rust-install/src/mock/clitools.rs:332:21: 337:6 note: in this expansion of vec! (defined in <std macros>)
rust-install/src/mock/clitools.rs:335:27: 335:35 help: run `rustc --explain E0425` to see a detailed explanation
error: aborting due to 9 previous errors
Could not compile `rust-install`.

To learn more, run the command again with --verbose.
@brson brson added A-diagnostics Area: Messages for errors, warnings, and lints I-wrong labels Mar 1, 2016
@nagisa
Copy link
Member

nagisa commented Mar 1, 2016

I blame the fact that we’re trying to recover from errors too hard. At the very least we should not continue into resolve after parse failures, should we? That will never work out.

@petrochenkov
Copy link
Contributor

This is a result of #31555 and/or #31065, so the post-parsing error messages are kind of expected, but still mostly harmful.

@nikomatsakis
Copy link
Contributor

I think continuing after parse errors can work well, but perhaps we should disable this sort of recovery on stable channels until we are better able to handle it. Another common failure I see is lifetime errors due to random type-checking failures -- I'm not exactly sure what causes those, but I'm sure you all have seen them.

@nikomatsakis
Copy link
Contributor

We could recover "opt in" with -Z continue or something, but I'd like us to all experience the errors better so we can work better on heuristics and improvements...

@alexcrichton
Copy link
Member

Although not related to parse errors, I've found a very similar bug related to worse error messages in the case of a resolve failure: #31997

@nikomatsakis
Copy link
Contributor

We probably want some kind of meta issue here, ultimately -- but my feeling is that we should continue to work towards better support for generating more errors, but we should not jump the gun and inflict these things on the "waiting public" until they are ready.

cc @rust-lang/compiler @rust-lang/tools

@alexcrichton
Copy link
Member

I personally like the idea of perhaps a -Z flag to change the behavior here in terms of not letting this propagate to stable. I'm not a huge fan of using a flag to test our error messages as the likelihood of anyone using it seems very small, however.

@nikomatsakis
Copy link
Contributor

I personally like the idea of perhaps a -Z flag to change the behavior here in terms of not letting this propagate to stable.

The main reason I proposed having the more advanced errors enabled by default (not behind a flag) on nightly was to ensure that we actually exercise the code, find ICEs, and so forth...

@nikomatsakis
Copy link
Contributor

We discussed in the @rust-lang/core meeting today. General feeling was that we should make surgical changes to try and stop issuing so many follow-on messages, and then revisit later when we think we can do better about being more targeted in these error reports to things we think are likely real problems. We decided having nightly diverge from beta behavior was a bad idea.

@nikomatsakis
Copy link
Contributor

Does anyone have a kind of standalone example of this?

@brson
Copy link
Contributor Author

brson commented Mar 9, 2016

I spent a few minutes trying to make one and failed.

@nikomatsakis
Copy link
Contributor

Nonetheless it seems clear that the recovery in #31555 is at fault. So maybe we can modify the logic there -- but it'd be great to have some kind of test, so that we have some idea when we can turn that recovery back on. I'll try to experiment today. I plan to fix the problems in #31997 first, since I have a smaller testcase there.

@brson
Copy link
Contributor Author

brson commented Mar 12, 2016

@nikomatsakis
Copy link
Contributor

For the record, this is the output I see from @brson's test case: https://gist.github.com/nikomatsakis/b5d93abd92fb30487753

@nrc nrc self-assigned this Mar 16, 2016
pnkfelix added a commit to pnkfelix/rust that referenced this issue Mar 26, 2016
This works by adding a method, `fn abort_if_no_parse_recovery`, to the
diagnostic handler in `syntax::errors`, and calling it after each
error is emitted in the parser.

(We might consider adding a debugflag to do such aborts in other
places where we are currently attempting recovery, such as resolve,
but I think the parser is the really important case to handle in the
face of rust-lang#31994 and the parser bugs of varying degrees that were
injected by parse error recovery.)
@bors bors closed this as completed in 2731dc1 Mar 26, 2016
pnkfelix added a commit to pnkfelix/rust that referenced this issue Mar 30, 2016
This works by adding a boolean flag, `continue_after_error`, to
`syntax::errors::Handler` that can be imperatively set to `true` or
`false` via a new `fn set_continue_after_error`.

The flag starts off true (since we generally try to recover from
compiler errors, and `Handler` is shared across all phases).

Then, during the `phase_1_parse_input`, we consult the setting of the
`-Z continue-parse-after-error` debug flag to determine whether we
should leave the flag set to `true` or should change it to `false`.

----

(We might consider adding a debugflag to do such aborts in other
places where we are currently attempting recovery, such as resolve,
but I think the parser is the really important case to handle in the
face of rust-lang#31994 and the parser bugs of varying degrees that were
injected by parse error recovery.)
Manishearth added a commit to Manishearth/rust that referenced this issue Mar 30, 2016
…ebugflag, r=nrc

Gate parser recovery via debugflag

Gate parser recovery via debugflag

Put in `-Z continue_parse_after_error`

This works by adding a method, `fn abort_if_no_parse_recovery`, to the
diagnostic handler in `syntax::errors`, and calling it after each
error is emitted in the parser.

(We might consider adding a debugflag to do such aborts in other
places where we are currently attempting recovery, such as resolve,
but I think the parser is the really important case to handle in the
face of rust-lang#31994 and the parser bugs of varying degrees that were
injected by parse error recovery.)

r? @nikomatsakis
pnkfelix added a commit to pnkfelix/rust that referenced this issue Apr 7, 2016
This works by adding a boolean flag, `continue_after_error`, to
`syntax::errors::Handler` that can be imperatively set to `true` or
`false` via a new `fn set_continue_after_error`.

The flag starts off true (since we generally try to recover from
compiler errors, and `Handler` is shared across all phases).

Then, during the `phase_1_parse_input`, we consult the setting of the
`-Z continue-parse-after-error` debug flag to determine whether we
should leave the flag set to `true` or should change it to `false`.

----

(We might consider adding a debugflag to do such aborts in other
places where we are currently attempting recovery, such as resolve,
but I think the parser is the really important case to handle in the
face of rust-lang#31994 and the parser bugs of varying degrees that were
injected by parse error recovery.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints
Projects
None yet
Development

No branches or pull requests

6 participants