Improve expression & attribute parsing by Centril · Pull Request #69760 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation38 Commits19 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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 <math xmlns="http://www.w3.org/1998/Math/MathML"><semantics><mrow><mi>b</mi><mi mathvariant="normal">‘</mi><mi>w</mi><mi>h</mi><mi>e</mi><mi>r</mi><mi>e</mi><mi mathvariant="normal">‘</mi></mrow><annotation encoding="application/x-tex">b where </annotation></semantics></math>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

This comment has been minimized.

Comment on lines +43 to +58

| 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 | | |

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we only suggest this for loops or blocks?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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: `;`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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 `;`",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;.

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

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

Comment on lines +1 to +7

| 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 | |

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than this nitpick, r=me

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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... :)

Comment on lines +28 to +49

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 `
--> $DIR/block-no-opening-brace.rs:30:9
\|
LL \| async
\| - expected one of `move`, `
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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:-/

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`

This comment has been minimized.

📌 Commit 458383d has been approved by estebank

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

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

Mar 10, 2020

Centril added a commit to Centril/rust that referenced this pull request

Mar 10, 2020

Improve expression & attribute parsing

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 <math xmlns="http://www.w3.org/1998/Math/MathML"><semantics><mrow><mi>b</mi><mi mathvariant="normal">‘</mi><mi>w</mi><mi>h</mi><mi>e</mi><mi>r</mi><mi>e</mi><mi mathvariant="normal">‘</mi></mrow><annotation encoding="application/x-tex">b where </annotation></semantics></math>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

Centril added a commit to Centril/rust that referenced this pull request

Mar 10, 2020

Improve expression & attribute parsing

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 <math xmlns="http://www.w3.org/1998/Math/MathML"><semantics><mrow><mi>b</mi><mi mathvariant="normal">‘</mi><mi>w</mi><mi>h</mi><mi>e</mi><mi>r</mi><mi>e</mi><mi mathvariant="normal">‘</mi></mrow><annotation encoding="application/x-tex">b where </annotation></semantics></math>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

bors added a commit that referenced this pull request

Mar 10, 2020

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

bors added a commit that referenced this pull request

Mar 11, 2020

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

Centril deleted the parse-expr-improve branch

March 11, 2020 16:33

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jul 22, 2023

…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:

macro_rules! always {
  ($block:block) => {
    if true $block
  }
}

fn main() {
    always!({});
}

Counterpoint:

However, for context, this is not supported:

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

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Apr 16, 2025

…=davidtwco

Improve parse errors for stray lifetimes in type position

While technically & syntactically speaking lifetimes do begin[^1] types in type contexts (this essentially excludes generic argument lists) and require a following + to form a complete type ('a + denotes a bare trait object type), the likelihood that a user meant to write a lifetime-prefixed bare trait object type in modern editions (Rust ≥2021) when placing a lifetime into a type context is incredibly low (they would need to add at least three tokens to turn it into a semantically well-formed TOT: 'a → dyn 'a + Trait).

Therefore let's lie in modern editions (just like in PR rust-lang#131239, a precedent if you will) by stating "expected type, found lifetime" in such cases which is a lot more a approachable, digestible and friendly compared to "lifetime in trait object type must be followed by +" (as added in PR rust-lang#69760).

I've also added recovery for "ampersand-less" reference types (e.g., 'a (), 'a mut Ty) in modern editions because it was trivial to do and I think it's not unlikely to occur in practice.

Fixes rust-lang#133413.

[^1]: For example, in the context of decl macros, this implies that a lone 'a always matches syntax fragment ty ("even if" there's a later macro matcher expecting syntax fragment lifetime). Rephrased, lifetimes (in type contexts) commit to the type parser.

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Apr 17, 2025

Rollup merge of rust-lang#139854 - fmease:modern-diag-for-lt-in-ty, r=davidtwco

Improve parse errors for stray lifetimes in type position

I've also added recovery for "ampersand-less" reference types (e.g., 'a (), 'a mut Ty) in modern editions because it was trivial to do and I think it's not unlikely to occur in practice.

Fixes rust-lang#133413.

github-actions bot pushed a commit to rust-lang/rustc-dev-guide that referenced this pull request

Apr 19, 2025

Improve parse errors for stray lifetimes in type position

Therefore let's lie in modern editions (just like in PR rust-lang/rust#131239, a precedent if you will) by stating "expected type, found lifetime" in such cases which is a lot more a approachable, digestible and friendly compared to "lifetime in trait object type must be followed by +" (as added in PR rust-lang/rust#69760).

I've also added recovery for "ampersand-less" reference types (e.g., 'a (), 'a mut Ty) in modern editions because it was trivial to do and I think it's not unlikely to occur in practice.

Fixes #133413.

Labels

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.