Improve expression & attribute parsing #69760

Centril · 2020-03-06T07:07:39Z

This PR includes misc improvements to expression and attribute parsing.

Some code simplifications
Better recovery for various block forms, e.g. loop statements } (missing { after loop). (See e.g., block-no-opening-brace.rs among others for examples.)
Added recovery for e.g., unsafe $b where $b refers to a block macro fragment. (See bad-interpolated-block.rs for examples.)
^--- These are done so that code sharing in block parsing is increased.
Added recovery for e.g., 'label: loop { ... } (See labeled-no-colon-expr.rs.)
Added recovery for e.g., &'lifetime expr (See regions-out-of-scope-slice.rs.)
Added recovery for e.g., fn foo() = expr; (See fn-body-eq-expr-semi.rs.)
Simplified attribute parsing code & slightly improved diagnostics.
Added recovery for e.g., Box<('a) + Trait>.
Added recovery for e.g, if true #[attr] {} else #[attr] {} else #[attr] if true {}.

r? @estebank

src/librustc_parse/parser/item.rs

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

src/test/ui/parser/regions-out-of-scope-slice.stderr

estebank · 2020-03-06T18:13:42Z

src/test/ui/parser/labeled-no-colon-expr.stderr

+error: labeled expression must be followed by `:`
+ --> $DIR/labeled-no-colon-expr.rs:8:9
+ |
+LL | 'l4 0;
+ | ----^
+ | | |
+ | | help: add `:` after the label
+ | the label


Should we only suggest this for loops or blocks?

Imo that unnecessarily complicates the diagnostic logic. I mostly included this for completeness, but getting both the colon and the block wrong (e.g. 'lab if cond { ... }) seems rather unlikely.

estebank · 2020-03-06T18:15:46Z

src/test/ui/parser/fn-body-eq-expr-semi.stderr

+LL | extern {
+ | ------ `extern` blocks define existing foreign functions and functions inside of them cannot have a body
+LL | fn foo() = 42;
+ | ^^^ ----- help: remove the invalid body: `;`


Consider using span_suggestion_short so that this doesn't include the : ';' part, but I leave it to you.

This is pre-existing logic in AST validation. I would prefer not adjusting that in this PR.

src/test/ui/parser/doc-comment-in-if-statement.stderr

src/test/ui/parser/closure-return-syntax.stderr

src/test/ui/parser/block-no-opening-brace.rs

estebank · 2020-03-06T18:22:02Z

src/librustc_parse/parser/item.rs

+ let span = eq_sp.to(self.prev_token.span);
+ self.struct_span_err(span, "function body cannot be `= expression;`")
+ .multipart_suggestion(
+ "surround the expression with `{` and `}` instead of `=` and `;`",


What about "give the function's expression a body" or similar? The suggestion text itself will imply the mechanical change, while the message should introduce concepts/terminology we use elsewhere.

I've used the current wording because it highlights that the users was thinking correctly, but used the wrong syntax. I think if we say "give the function's expression a body", then a user can reasonably react: "but I have given the function a body?!?" and "body" doesn't necessarily refer to { ... }, as the "body" of a const uses = expr ;.

src/librustc_parse/parser/expr.rs

src/librustc_parse/parser/attr.rs

estebank · 2020-03-06T18:28:35Z

LGTM, but left some nitpicks. Feel free to resolve them as you prefer.

Centril · 2020-03-07T16:19:07Z

@estebank Pushed some changes to address the review comments; please take a look. :)

estebank · 2020-03-07T23:26:51Z

src/test/ui/parser/doc-comment-in-if-statement.stderr

+error: expected outer doc comment
 --> $DIR/doc-comment-in-if-statement.rs:2:13
 |
 LL | if true /*!*/ {}
- | -- ^^^^^ expected `{`
- | |
- | this `if` expression has a condition, but no block
+ |  ^^^^^
+ |
+ = note: inner doc comments like this (starting with `//!` or `/*!`) can only appear before items


I feel the message is a bit misleading, as it should be closer to "found invalid inner doc comment"/"inner doc comments like this (...) can only appear at the mod level before all items", or something like that.

Other than this nitpick, r=me

I believe the message is actually accurate, because trait Foo { /*! foo */ } is legal. In other words, this applies to item containers in general (but there are other contexts where inner attributes are allowed as well, e.g., Foo { /*! foo */ };).

Anyways, this is a pre-existing wording, and I would prefer not to inflate the PR more... :)

estebank · 2020-03-07T23:31:40Z

src/test/ui/parser/block-no-opening-brace.stderr

+error: expected expression, found reserved keyword `try`
+ --> $DIR/block-no-opening-brace.rs:24:5
+ |
+LL | try
+ | ^^^ expected expression
+
+error: expected one of `move`, `|`, or `||`, found keyword `let`
+ --> $DIR/block-no-opening-brace.rs:30:9
+ |
+LL | async
+ | - expected one of `move`, `|`, or `||`
+LL | let x = 0;
+ | ^^^ unexpected token
+
+error[E0658]: async closures are unstable
+ --> $DIR/block-no-opening-brace.rs:29:5
+ |
+LL | async
+ | ^^^^^
+ |
+ = note: see issue #62290 <https://github.com/rust-lang/rust/issues/62290> for more information
+ = help: add `#![feature(async_closure)]` to the crate attributes to enable


(This happens because async block/closure parsing routes to a block if and only if there's a { ahead, and otherwise parses a closure. I want to fix this, but some other time. try blocks have a similar issue.)

At the very least a small improvement would be to add a check call for { so that the error is

expected one of `move`, `|`, `||` or `{`, found keyword `let`

Centril · 2020-03-10T08:50:03Z

@bors r=estebank

bors · 2020-03-10T08:50:04Z

📌 Commit 458383d has been approved by estebank

bors · 2020-03-10T08:50:05Z

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

@estebank

Improve expression & attribute parsing This PR includes misc improvements to expression and attribute parsing. 1. Some code simplifications 2. Better recovery for various block forms, e.g. `loop statements }` (missing `{` after `loop`). (See e.g., `block-no-opening-brace.rs` among others for examples.) 3. Added recovery for e.g., `unsafe $b` where `$b` refers to a `block` macro fragment. (See `bad-interpolated-block.rs` for examples.) 4. ^--- These are done so that code sharing in block parsing is increased. 5. Added recovery for e.g., `'label: loop { ... }` (See `labeled-no-colon-expr.rs`.) 6. Added recovery for e.g., `&'lifetime expr` (See `regions-out-of-scope-slice.rs`.) 7. Added recovery for e.g., `fn foo() = expr;` (See `fn-body-eq-expr-semi.rs`.) 8. Simplified attribute parsing code & slightly improved diagnostics. 9. Added recovery for e.g., `Box<('a) + Trait>`. 10. Added recovery for e.g, `if true #[attr] {} else #[attr] {} else #[attr] if true {}`. r? @estebank

@estebank

Improve expression & attribute parsing This PR includes misc improvements to expression and attribute parsing. 1. Some code simplifications 2. Better recovery for various block forms, e.g. `loop statements }` (missing `{` after `loop`). (See e.g., `block-no-opening-brace.rs` among others for examples.) 3. Added recovery for e.g., `unsafe $b` where `$b` refers to a `block` macro fragment. (See `bad-interpolated-block.rs` for examples.) 4. ^--- These are done so that code sharing in block parsing is increased. 5. Added recovery for e.g., `'label: loop { ... }` (See `labeled-no-colon-expr.rs`.) 6. Added recovery for e.g., `&'lifetime expr` (See `regions-out-of-scope-slice.rs`.) 7. Added recovery for e.g., `fn foo() = expr;` (See `fn-body-eq-expr-semi.rs`.) 8. Simplified attribute parsing code & slightly improved diagnostics. 9. Added recovery for e.g., `Box<('a) + Trait>`. 10. Added recovery for e.g, `if true #[attr] {} else #[attr] {} else #[attr] if true {}`. r? @estebank

@ghost

Rollup of 6 pull requests Successful merges: - #69122 (Backtrace Debug tweaks) - #69591 (Use TypeRelating for instantiating query responses) - #69760 (Improve expression & attribute parsing) - #69837 (Use smaller discriminants for generators) - #69838 (Expansion-driven outline module parsing) - #69859 (fix #62456) Failed merges: r? @ghost

@ghost

Rollup of 8 pull requests Successful merges: - #66472 (--show-coverage json) - #69603 (tidy: replace `make check` with `./x.py test` in documentation) - #69760 (Improve expression & attribute parsing) - #69828 (fix memory leak when vec::IntoIter panics during drop) - #69850 (panic_bounds_check: use caller_location, like PanicFnLangItem) - #69876 (Add long error explanation for E0739) - #69888 ([Miri] Use a session variable instead of checking for an env var always) - #69893 (librustc_codegen_llvm: Use slices instead of 0-terminated strings) Failed merges: r? @ghost

…xprs, r=WaffleLapkin Support interpolated block for `try` and `async` I'm putting this up for T-lang discussion, to decide whether or not they feel like this should be supported. This was raised in rust-lang#112952, which surprised me. There doesn't seem to be a *technical* reason why we don't support this. ### Precedent: This is supported: ```rust macro_rules! always { ($block:block) => { if true $block } } fn main() { always!({}); } ``` ### Counterpoint: However, for context, this is *not* supported: ```rust macro_rules! unsafe_block { ($block:block) => { unsafe $block } } fn main() { unsafe_block!({}); } ``` If this support for `async` and `try` with interpolated blocks is *not* desirable, then I can convert them to instead the same diagnostic as `unsafe $block` and make this situation a lot less ambiguous. ---- I'll try to write up more before T-lang triage on Tuesday. I couldn't find anything other than rust-lang#69760 for why something like `unsafe $block` is not supported, and even that PR doesn't have much information. Fixes rust-lang#112952

rust-highfive assigned estebank Mar 6, 2020

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

petrochenkov self-assigned this Mar 6, 2020

petrochenkov approved these changes Mar 6, 2020

View reviewed changes

src/librustc_parse/parser/item.rs Show resolved Hide resolved

petrochenkov removed their assignment Mar 6, 2020

This comment has been minimized.

Sign in to view