Suggest adding missing braces in `const` block pattern #78173

camelid · 2020-10-21T00:57:50Z

Fixes #78168.

Previously it would only suggest wrapping the code in braces in regular
expressions; now it does it in patterns too.

r? @spastorino

src/test/ui/parser/keyword-const-as-identifier.stderr

compiler/rustc_parse/src/parser/stmt.rs

spastorino · 2020-10-21T12:25:45Z

compiler/rustc_parse/src/parser/expr.rs

@@ -1060,8 +1060,8 @@ impl<'a> Parser<'a> {
            })
        } else if self.eat_keyword(kw::Unsafe) {
            self.parse_block_expr(None, lo, BlockCheckMode::Unsafe(ast::UserProvided), attrs)
-        } else if self.check_inline_const() {
-            self.parse_const_expr(lo.to(self.token.span))
+        } else if self.eat_keyword(kw::Const) {


I think @petrochenkov and others wanted to avoid eating the keyword before checking if it's really an inline const. My first reaction was like ... but we're eating Unsafe right above. I'm not sure if there's another way to solve this problem and we could still rely on checking or if we just need to eat the keyword.

Anyway, would leave this to @petrochenkov

The difference is that unsafe just modifies the block expression, whereas const needs to create the ExprKind::Const - but maybe it doesn't have to be too complicated shrug.

compiler/rustc_parse/src/parser/mod.rs

spastorino · 2020-10-21T12:29:31Z

Another thought, I've 2 PRs open for inline consts that changes in some way the code you're also touching. It may be good to land those first and base your work on those.

camelid · 2020-10-21T18:54:53Z

Yeah, I expected to have merge conflicts :)

Weirdly, GitHub is reporting merge conflicts but bors is not. I think this issue has happened before; I’ll let T-infra take a look before I fix the conflicts so they can perhaps figure out the cause.

camelid · 2020-10-21T22:57:01Z

Rebased.

spastorino · 2020-10-22T20:52:35Z

src/test/ui/parser/keyword-const-as-identifier.stderr

   |
 LL |     let const = "foo";
-   |         ^^^^^ expected identifier, found keyword


This has regressed a bit now.
cc @oli-obk

The thing is that this:

let const { "foo" } = "foo";

is a totally valid statement (although it ICEs; see #78174). Perhaps we could give the old warning if it's just const and then = immediately after?

I see, but, what do you expect that code to do?.
I'd expect an error anyway something like error[E0005]: refutable pattern in local binding: ``&_`` not covered.

It's valid syntactically, not semantically. (And either way it shouldn't ICE.)

And yes, the error you suggested is correct; it's the one that occurs for let "foo" = "foo";: error[E0005]: refutable pattern in local binding: `&_` not covered

exactly, 👍

spastorino · 2020-10-23T13:15:27Z

Now both of my follow up PRs are in and you would need to rebase again :). I think this is fine modulo the eat_keyword part, which to be honest I'm not sure. I'm not sure if there's a way to avoid it, my first solution used it too and then after some reviews I got rid of it.

Anyway, this is fine by me but would leave this up to r? @petrochenkov

petrochenkov · 2020-10-24T15:50:36Z

@camelid
Could you rebase this over #78116? There were some significant changes in the same area in that PR.

src/test/ui/feature-gate-inline_const.stderr

src/test/ui/parser/keyword-const-as-identifier.stderr

petrochenkov · 2020-10-24T18:53:50Z

@camelid
I have a suggestion for a different approach for const in expressions.
In expressions we have a well established precedent for general-purpose unary operators, including keyword-based ones like box.

So I suggest to parse const expr in expressions exactly like a unary operator box expr, but then report a non-fatal error if expr is not actually a block.
The only difference would be an extra check at statement start to avoid interpreting const IDENT: ... or const _: ... items as const expressions.

In patterns we have the unary box operator too, so const recovery can potentially work in the same way as in expressions (parse const PAT, error if PAT is not a block), but I'm not sure about interactions with range patterns, they may require special cases.

JohnCSimon · 2020-11-09T16:13:28Z

hello from triage:
@camelid - can you address the comment from petrochenkov

camelid · 2020-11-09T18:15:35Z

Yes, this is on my todo list to figure out.

crlf0710 · 2020-11-28T06:34:17Z

@camelid Ping from triage: Any updates on this?

camelid · 2020-12-18T03:32:06Z

I'm hoping to get to this at some point soon, I just need to understand this part of the codebase better.

petrochenkov · 2020-12-27T12:34:32Z

This can be split into two parts - one for expressions (which should be pretty much a copypaste of what box does), and another for patterns which may require some more thought.

Previously it would only suggest wrapping the code in braces in regular expressions; now it does it in patterns too.

Needed for range pattern parsing.

camelid · 2020-12-31T01:59:35Z

Starting with a rebase.

camelid · 2021-01-01T03:14:26Z

I have a suggestion for a different approach for const in expressions.
In expressions we have a well established precedent for general-purpose unary operators, including keyword-based ones like box.

So I suggest to parse const expr in expressions exactly like a unary operator box expr, but then report a non-fatal error if expr is not actually a block.

@petrochenkov One issue with this is that prefix operators have high precedence, so the suggestion for wrapping the code in a block doesn't capture what it should:

error: expected inline const block
  --> $DIR/inline-const-without-block.rs:50:19
   |
LL |     let y = const 1 + 2 * 3 / 4;
   |                   ^ help: try placing this code inside a block: `{ 1 }`

I'm not sure if there's a good way to resolve this because of the precedence of prefix operators. I haven't made many changes to the parser before, so I would appreciate some extra guidance! Thanks :)

petrochenkov · 2021-01-01T15:59:15Z

const { 1 } is the behavior that I expected, if const starts working without braces like box, then it's exactly the priority that it will have.
We also don't know what it "should" capture in general case - code like const { f() } + 1 is as reasonable as const { f() + 1 }.

petrochenkov · 2021-02-23T10:18:25Z

Closing due to inactivity.

camelid added A-diagnostics Area: Messages for errors, warnings, and lints A-parser Area: The parsing of Rust source code to an AST. F-inline_const Inline constants (aka: const blocks, const expressions, anonymous constants) labels Oct 21, 2020

rust-highfive assigned spastorino Oct 21, 2020

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

camelid added the A-patterns Relating to patterns and pattern matching label Oct 21, 2020

camelid commented Oct 21, 2020

View reviewed changes

src/test/ui/parser/keyword-const-as-identifier.stderr Outdated Show resolved Hide resolved

compiler/rustc_parse/src/parser/stmt.rs Show resolved Hide resolved

compiler/rustc_parse/src/parser/stmt.rs Show resolved Hide resolved

camelid force-pushed the const-pat-missing-braces branch from 3e567f0 to 0bf61fd Compare October 21, 2020 01:04

camelid marked this pull request as ready for review October 21, 2020 03:27

spastorino reviewed Oct 21, 2020

View reviewed changes

camelid force-pushed the const-pat-missing-braces branch from b9e3545 to 31e18af Compare October 21, 2020 22:55

camelid added the A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. label Oct 22, 2020

spastorino reviewed Oct 22, 2020

View reviewed changes

rust-highfive assigned petrochenkov and unassigned spastorino Oct 23, 2020

spastorino approved these changes Oct 23, 2020

View reviewed changes

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 24, 2020

camelid force-pushed the const-pat-missing-braces branch from a75494e to 19ec18b Compare October 24, 2020 17:27

camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 24, 2020

camelid commented Oct 24, 2020

View reviewed changes

src/test/ui/feature-gate-inline_const.stderr Outdated Show resolved Hide resolved

src/test/ui/parser/keyword-const-as-identifier.stderr Outdated Show resolved Hide resolved

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 24, 2020

crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 28, 2020

camelid added 9 commits December 30, 2020 17:59

Suggest adding missing braces in const block pattern

5294992

Previously it would only suggest wrapping the code in braces in regular expressions; now it does it in patterns too.

Add run-pass test and fix failing test

f19fc49

Re-add mistakenly removed code

4dbe2ac

Update tests

24fa10f

Add back check_inline_const

4315b85

Needed for range pattern parsing.

Update tests

854f207

Update another test

d6d30cc

Fix bug

a72f39f

Fix span bug

d9dfa89

camelid force-pushed the const-pat-missing-braces branch from 502fcec to d9dfa89 Compare December 31, 2020 01:59

camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 1, 2021

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 1, 2021

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 19, 2021

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 14, 2021

petrochenkov closed this Feb 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suggest adding missing braces in `const` block pattern #78173

Suggest adding missing braces in `const` block pattern #78173

camelid commented Oct 21, 2020

spastorino Oct 21, 2020

eddyb Oct 23, 2020

spastorino commented Oct 21, 2020

camelid commented Oct 21, 2020

camelid commented Oct 21, 2020

spastorino Oct 22, 2020

camelid Oct 22, 2020

spastorino Oct 22, 2020

camelid Oct 22, 2020 •

edited

Loading

spastorino Oct 22, 2020

spastorino commented Oct 23, 2020

petrochenkov commented Oct 24, 2020

petrochenkov commented Oct 24, 2020

JohnCSimon commented Nov 9, 2020

camelid commented Nov 9, 2020

crlf0710 commented Nov 28, 2020

camelid commented Dec 18, 2020

petrochenkov commented Dec 27, 2020

camelid commented Dec 31, 2020

camelid commented Jan 1, 2021

petrochenkov commented Jan 1, 2021

petrochenkov commented Feb 23, 2021

Suggest adding missing braces in const block pattern #78173

Suggest adding missing braces in const block pattern #78173

Conversation

camelid commented Oct 21, 2020

spastorino Oct 21, 2020

Choose a reason for hiding this comment

eddyb Oct 23, 2020

Choose a reason for hiding this comment

spastorino commented Oct 21, 2020

camelid commented Oct 21, 2020

camelid commented Oct 21, 2020

spastorino Oct 22, 2020

Choose a reason for hiding this comment

camelid Oct 22, 2020

Choose a reason for hiding this comment

spastorino Oct 22, 2020

Choose a reason for hiding this comment

camelid Oct 22, 2020 • edited Loading

Choose a reason for hiding this comment

spastorino Oct 22, 2020

Choose a reason for hiding this comment

spastorino commented Oct 23, 2020

petrochenkov commented Oct 24, 2020

petrochenkov commented Oct 24, 2020

JohnCSimon commented Nov 9, 2020

camelid commented Nov 9, 2020

crlf0710 commented Nov 28, 2020

camelid commented Dec 18, 2020

petrochenkov commented Dec 27, 2020

camelid commented Dec 31, 2020

camelid commented Jan 1, 2021

petrochenkov commented Jan 1, 2021

petrochenkov commented Feb 23, 2021

Suggest adding missing braces in `const` block pattern #78173

Suggest adding missing braces in `const` block pattern #78173

camelid Oct 22, 2020 •

edited

Loading