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

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

Merged
merged 15 commits into from Jan 15, 2019

Conversation

Projects
None yet
5 participants
@estebank
Copy link
Contributor

estebank commented Jan 12, 2019

Continue evaluating further errors after parser errors on:

  • trailing type argument attribute
  • lifetime in incorrect location
  • incorrect binary literal
  • missing for in impl Trait for Foo
  • type argument in where clause
  • incorrect float literal
  • incorrect .. in pattern
  • associated types
  • incorrect discriminator value variant error

and others. All of these were found by making continue-parse-after-error true by default to identify errors that would need few changes. There are now only a handful of errors that have any change with continue-parse-after-error enabled.

These changes make it so rust won't stop evaluation after finishing parsing, enabling type checking errors to be displayed on the existing code without having to fix the parse errors.

Each commit has an individual diagnostic change with their corresponding tests.

CC #48724.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 12, 2019

r? @pnkfelix

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

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 12, 2019

Is it possible to flip the default and introduce span_err_irrecoverable or something for errors that stop compilation?
This way it will be obvious what cases still need fixing.
(Also replacing span_err(span, msg) with struct_span_err(span, msg).emit() will be unnecessary.)

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 12, 2019

In the previous PRs (#57379, #57272 etc) I adjusted tests so they don't report errors that are not related to the point of the test and are not consequences of recovery (e.g. unresolved names, missing main).
I'll go through the tests and mark the errors that I think are "off-topic" and are better avoided.

Show resolved Hide resolved ...trs-with-no-formal-in-generics/attrs-with-no-formal-in-generics-3.stderr Outdated
Show resolved Hide resolved src/test/ui/parser/issue-14303-fncall.stderr Outdated
Show resolved Hide resolved src/test/ui/parser/issue-14303-path.stderr Outdated
Show resolved Hide resolved src/test/ui/parser/issue-1802-2.stderr Outdated
Show resolved Hide resolved src/test/ui/parser/pat-tuple-2.stderr Outdated
Show resolved Hide resolved src/test/ui/parser/pat-tuple-3.stderr Outdated
Show resolved Hide resolved src/test/ui/parser/tag-variant-disr-non-nullary.stderr Outdated
@estebank

This comment has been minimized.

Copy link
Contributor Author

estebank commented Jan 13, 2019

r? @petrochenkov

Is it possible to flip the default and introduce span_err_irrecoverable or something for errors that stop compilation?
This way it will be obvious what cases still need fixing.
(Also replacing span_err(span, msg) with struct_span_err(span, msg).emit() will be unnecessary.)

Makes sense.


EDIT: I just realized that we still need to use struct_span_err directly in most places, as we're also adding suggestions/labels. I won't pursue changing span_err as it will have little improvement. The only thing that would be useful is maybe renaming span_err to span_err_irrecoverable as proposed.


In the previous PRs (#57379#57272 etc) I adjusted tests so they don't report errors that are not related to the point of the test and are not consequences of recovery (e.g. unresolved names, missing main).

I'd like to have some regression tests to make sure that the parser doesn't suddenly start blocking further processing again by accident. Do you think it makes sense to have a few marked explicitly that way?

|
LL | where Q: for <#[rustc_1] 'a, 'b, #[oops]> Fn(RefIntPair<'a,'b>) -> &'b u32
| ^^^^^^^
LL | where Q: for <#[allow(unused)] 'a, 'b, #[oops]> Fn(RefIntPair<'a,'b>) -> &'b u32

This comment has been minimized.

@estebank

estebank Jan 13, 2019

Author Contributor
  1. I didn't know we could have attributes annotating lifetimes, 2) we have no attributes to annotate lifetimes or type arguments at this time :)
@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 13, 2019

The only thing that would be useful is maybe renaming span_err to span_err_irrecoverable as proposed.

Hmm, looks like that would require touching a lot of other code because behavior of span_err changes dynamically from irrecoverable to recoverable.
Ok, let is stay as is for now.

I'd like to have some regression tests to make sure that the parser doesn't suddenly start blocking further processing again by accident. Do you think it makes sense to have a few marked explicitly that way?

Yes, it would be good to have a single dedicated test rather than "accidental" errors here and there.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 13, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 13, 2019

📌 Commit 28ea03e has been approved by petrochenkov

Centril added a commit to Centril/rust that referenced this pull request Jan 14, 2019

Rollup merge of rust-lang#57540 - estebank:eval-more, r=petrochenkov
Modify some parser diagnostics to continue evaluating beyond the parser

Continue evaluating further errors after parser errors on:
 - trailing type argument attribute
 - lifetime in incorrect location
 - incorrect binary literal
 - missing `for` in `impl Trait for Foo`
 - type argument in `where` clause
 - incorrect float literal
 - incorrect `..` in pattern
 - associated types
 - incorrect discriminator value variant error

and others. All of these were found by making `continue-parse-after-error` `true` by default to identify errors that would need few changes. There are now only a handful of errors that have any change with `continue-parse-after-error` enabled.

These changes make it so `rust` _won't_ stop evaluation after finishing parsing, enabling type checking errors to be displayed on the existing code without having to fix the parse errors.

Each commit has an individual diagnostic change with their corresponding tests.

CC rust-lang#48724.

bors added a commit that referenced this pull request Jan 14, 2019

Auto merge of #57599 - Centril:rollup, r=Centril
Rollup of 5 pull requests

Successful merges:

 - #57442 (Simplify `ConstValue::ScalarPair`)
 - #57480 (Clean up and fix a bug in query plumbing)
 - #57481 (provide suggestion for invalid boolean cast)
 - #57540 (Modify some parser diagnostics to continue evaluating beyond the parser)
 - #57570 (Querify local `plugin_registrar_fn` and `proc_macro_decls_static`)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request Jan 14, 2019

Rollup merge of rust-lang#57540 - estebank:eval-more, r=petrochenkov
Modify some parser diagnostics to continue evaluating beyond the parser

Continue evaluating further errors after parser errors on:
 - trailing type argument attribute
 - lifetime in incorrect location
 - incorrect binary literal
 - missing `for` in `impl Trait for Foo`
 - type argument in `where` clause
 - incorrect float literal
 - incorrect `..` in pattern
 - associated types
 - incorrect discriminator value variant error

and others. All of these were found by making `continue-parse-after-error` `true` by default to identify errors that would need few changes. There are now only a handful of errors that have any change with `continue-parse-after-error` enabled.

These changes make it so `rust` _won't_ stop evaluation after finishing parsing, enabling type checking errors to be displayed on the existing code without having to fix the parse errors.

Each commit has an individual diagnostic change with their corresponding tests.

CC rust-lang#48724.

Centril added a commit to Centril/rust that referenced this pull request Jan 14, 2019

Rollup merge of rust-lang#57540 - estebank:eval-more, r=petrochenkov
Modify some parser diagnostics to continue evaluating beyond the parser

Continue evaluating further errors after parser errors on:
 - trailing type argument attribute
 - lifetime in incorrect location
 - incorrect binary literal
 - missing `for` in `impl Trait for Foo`
 - type argument in `where` clause
 - incorrect float literal
 - incorrect `..` in pattern
 - associated types
 - incorrect discriminator value variant error

and others. All of these were found by making `continue-parse-after-error` `true` by default to identify errors that would need few changes. There are now only a handful of errors that have any change with `continue-parse-after-error` enabled.

These changes make it so `rust` _won't_ stop evaluation after finishing parsing, enabling type checking errors to be displayed on the existing code without having to fix the parse errors.

Each commit has an individual diagnostic change with their corresponding tests.

CC rust-lang#48724.

bors added a commit that referenced this pull request Jan 14, 2019

Auto merge of #57607 - Centril:rollup, r=Centril
Rollup of 8 pull requests

Successful merges:

 - #57043 (Fix poor worst case performance of set intersection)
 - #57480 (Clean up and fix a bug in query plumbing)
 - #57481 (provide suggestion for invalid boolean cast)
 - #57540 (Modify some parser diagnostics to continue evaluating beyond the parser)
 - #57570 (Querify local `plugin_registrar_fn` and `proc_macro_decls_static`)
 - #57572 (Unaccept `extern_in_paths`)
 - #57585 (Recover from item trailing semicolon)
 - #57589 (Add a debug_assert to Vec::set_len)

Failed merges:

r? @ghost

@bors bors merged commit 28ea03e into rust-lang:master Jan 15, 2019

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