Don't make statement nonterminals match pattern nonterminals #120221

compiler-errors · 2024-01-22T02:40:42Z

Right now, the heuristic we use to check if a token may begin a pattern nonterminal falls back to may_be_ident:

rust/compiler/rustc_parse/src/parser/nonterminal.rs

Lines 21 to 37 in ef71f10

 fn may_be_ident(nt: &token::Nonterminal) -> bool { 

 match nt { 

 NtStmt(_) 

 | NtPat(_) 

 | NtExpr(_) 

 | NtTy(_) 

 | NtIdent(..) 

 | NtLiteral(_) // `true`, `false` 

 | NtMeta(_) 

 | NtPath(_) => true, 

 NtItem(_) 

 | NtBlock(_) 

 | NtVis(_) 

 | NtLifetime(_) => false, 

 } 

 }

This has the unfortunate side effect that a stmt nonterminal eagerly matches against a pat nonterminal, leading to a parse error:

macro_rules! m {
    ($pat:pat) => {};
    ($stmt:stmt) => {};
}

macro_rules! m2 {
    ($stmt:stmt) => {
        m! { $stmt }
    };
}

m2! { let x = 1 }

This PR fixes it by more accurately reflecting the set of nonterminals that may begin a pattern nonterminal.

As a side-effect, I modified Token::can_begin_pattern to work correctly and used that in Parser::nonterminal_may_begin_with.

rustbot · 2024-01-22T02:40:51Z

r? @nnethercote

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

nnethercote · 2024-01-22T04:03:28Z

compiler/rustc_ast/src/token.rs

- NtBlock(..) |
- NtPath(..)),
+ /// **NB**: Take care when modifying this function, since it will change
+ /// the stable set of tokens that are allowed to match an pat nonterminal.


Suggested change

/// the stable set of tokens that are allowed to match an pat nonterminal.

/// the stable set of tokens that are allowed to match a pat nonterminal.

nnethercote · 2024-01-22T04:09:08Z

compiler/rustc_ast/src/token.rs

+ AndAnd | // double reference
+ Literal(_) | // literal
+ DotDot | // range pattern (future compat)
+ DotDotDot | // range pattern (future compat)


AFAICT, the net effect here:

All identifiers now match, i.e. more than before

DotDotEq no longer matches

BinOp(Or) matches if allow_leading_or is true

NtExpr, NtMeta, NtTy now match

NtBlock no longer matches

Is that right?

Yes, though it should be noted that can_begin_pattern was previously only used for diagnostics and was previously just a rip off of can_begin_expr from above.

A better way to look at the changes here is to compare it to the previous match that was located in nonterminal_may_begin_with. This PR copies that logic over, adds an uninterpolate call for consistency with can_begin_expr (rather than manually handling the NtIdent arm), and adjusting the match arm for the Interpolated token.

To sum it up, I believe the only token kind that now no longer matches pat nonterminals is the statement nonterminal.

nnethercote · 2024-01-22T04:18:35Z

compiler/rustc_parse/src/parser/nonterminal.rs

- }
- }
+ NonterminalKind::PatParam { .. } => token.can_begin_pattern(false),
+ NonterminalKind::PatWithOr => token.can_begin_pattern(true),


And the net effect here is that NtStmt no longer matches. Which I think is the change that actually fixes the original problem, right?

nnethercote · 2024-01-22T04:19:52Z

How did you find this problem? Just curious.

This appears to change more than is necessary to fix the cited bug. Is that intended?

compiler-errors · 2024-01-22T04:35:56Z

How did you find this problem? Just curious.

I noticed it while authoring #120218, and thinking a bit about why there was a Token::can_begin_pattern (that, for the record, I believe I added for diagnostics purposes originally) that was not being used by Parser::nonterminal_may_begin_with.

This appears to change more than is necessary to fix the cited bug. Is that intended?

Yes. I don't see why we need to maintain an incorrect Token::can_begin_pattern just for diagnostics rather than just fixing it to actually reflect the set of tokens that can match pat nonterminals and using it everywhere.

nnethercote · 2024-01-22T06:17:47Z

Thanks for the explanations. r=me with the "a"/"an" nit fixed.

compiler-errors · 2024-02-05T21:01:46Z

Actually, given that this is a change to a contract (the macro matchers), I'll pass this by t-lang as well.

@rustbot label: +I-lang-nominated

bors · 2024-03-21T20:16:13Z

☔ The latest upstream changes (presumably #122830) made this pull request unmergeable. Please resolve the merge conflicts.

scottmcm · 2024-03-27T15:12:26Z

compiler/rustc_ast/src/token.rs

- NtPath(..)),
+ /// **NB**: Take care when modifying this function, since it will change
+ /// the stable set of tokens that are allowed to match a pat nonterminal.
+ pub fn can_begin_pattern(&self, allow_leading_or: bool) -> bool {


nit: the bool here is very non-obvious in callers. Dunno if it'd be worth having an enum or something to make it clearer, or to have the bool-taking thing be non-pub with different names for the two cases, or something.

scottmcm · 2024-03-27T15:35:14Z

From CE: Right now the tokens that a macro matcher may begin with is a stable guarantee. We are relaxing the assumption that pattern matchers may begin with statement metavariables ($var whose type is stmt), because when we actually try to parse such a pattern, we are always guaranteed to fail. This only allows more code to compile, and would only break future code if we specifically wanted to begin patterns with statement metavariable.

I agree that it's weird to allow a :stmt in a pattern, so am happy to say we won't. Let's see what others think, since this conversation was in a sparsely-attended triage meeting:

@rfcbot fcp merge

The other thing we explored was what it would take to make this actually work, since you can actually put an :expr into a pattern. But CE argued that we don't actually like that that works, it's just something we're stuck with because people used it before :literal was available, which seems fair.

scottmcm · 2024-03-27T15:35:56Z

From CE: Right now the tokens that a macro matcher may begin with is a stable guarantee. We are relaxing the assumption that pattern matchers may begin with statement metavariables ($var whose type is stmt), because when we actually try to parse such a pattern, we are always guaranteed to fail. This only allows more code to compile, and would only break future code if we specifically wanted to begin patterns with statement metavariable.

I agree that it's weird to allow a :stmt in a pattern, so am happy to say we won't. Let's see what others think, since this conversation was in a sparsely-attended triage meeting:

@rfcbot fcp merge

The other thing we explored was what it would take to make this actually work, since you can actually put an :expr into a pattern. But CE argued that we don't actually like that that works, it's just something we're stuck with because people used it before :literal was available, which seems fair.

rfcbot · 2024-03-27T15:35:58Z

Team member @scottmcm has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

nnethercote · 2024-08-29T05:58:39Z

@bors r+

bors · 2024-08-29T05:58:42Z

📌 Commit c61f85b has been approved by nnethercote

It is now in the queue for this repository.

…-patterns, r=nnethercote Don't make statement nonterminals match pattern nonterminals Right now, the heuristic we use to check if a token may begin a pattern nonterminal falls back to `may_be_ident`: https://github.com/rust-lang/rust/blob/ef71f1047e04438181d7cb925a833e2ada6ab390/compiler/rustc_parse/src/parser/nonterminal.rs#L21-L37 This has the unfortunate side effect that a `stmt` nonterminal eagerly matches against a `pat` nonterminal, leading to a parse error: ```rust macro_rules! m { ($pat:pat) => {}; ($stmt:stmt) => {}; } macro_rules! m2 { ($stmt:stmt) => { m! { $stmt } }; } m2! { let x = 1 } ``` This PR fixes it by more accurately reflecting the set of nonterminals that may begin a pattern nonterminal. As a side-effect, I modified `Token::can_begin_pattern` to work correctly and used that in `Parser::nonterminal_may_begin_with`.

…kingjubilee Rollup of 9 pull requests Successful merges: - rust-lang#120221 (Don't make statement nonterminals match pattern nonterminals) - rust-lang#127912 (std: make `thread::current` available in all `thread_local!` destructors) - rust-lang#128166 (Improved `checked_isqrt` and `isqrt` methods) - rust-lang#129123 (rustdoc-json: Add test for `Self` type) - rust-lang#129366 (linker: Synchronize native library search in rustc and linker) - rust-lang#129527 (Don't use `TyKind` in a lint) - rust-lang#129534 (Deny `wasm_c_abi` lint to nudge the last 25%) - rust-lang#129640 (Re-enable android tests/benches in alloc/core) - rust-lang#129675 (allow BufReader::peek to be called on unsized types) r? `@ghost` `@rustbot` modify labels: rollup

…-patterns, r=nnethercote Don't make statement nonterminals match pattern nonterminals Right now, the heuristic we use to check if a token may begin a pattern nonterminal falls back to `may_be_ident`: https://github.com/rust-lang/rust/blob/ef71f1047e04438181d7cb925a833e2ada6ab390/compiler/rustc_parse/src/parser/nonterminal.rs#L21-L37 This has the unfortunate side effect that a `stmt` nonterminal eagerly matches against a `pat` nonterminal, leading to a parse error: ```rust macro_rules! m { ($pat:pat) => {}; ($stmt:stmt) => {}; } macro_rules! m2 { ($stmt:stmt) => { m! { $stmt } }; } m2! { let x = 1 } ``` This PR fixes it by more accurately reflecting the set of nonterminals that may begin a pattern nonterminal. As a side-effect, I modified `Token::can_begin_pattern` to work correctly and used that in `Parser::nonterminal_may_begin_with`.

…kingjubilee Rollup of 8 pull requests Successful merges: - rust-lang#120221 (Don't make statement nonterminals match pattern nonterminals) - rust-lang#129123 (rustdoc-json: Add test for `Self` type) - rust-lang#129366 (linker: Synchronize native library search in rustc and linker) - rust-lang#129527 (Don't use `TyKind` in a lint) - rust-lang#129534 (Deny `wasm_c_abi` lint to nudge the last 25%) - rust-lang#129640 (Re-enable android tests/benches in alloc/core) - rust-lang#129675 (allow BufReader::peek to be called on unsized types) - rust-lang#129731 (Allow running `./x.py test compiler`) r? `@ghost` `@rustbot` modify labels: rollup

…-patterns, r=nnethercote Don't make statement nonterminals match pattern nonterminals Right now, the heuristic we use to check if a token may begin a pattern nonterminal falls back to `may_be_ident`: https://github.com/rust-lang/rust/blob/ef71f1047e04438181d7cb925a833e2ada6ab390/compiler/rustc_parse/src/parser/nonterminal.rs#L21-L37 This has the unfortunate side effect that a `stmt` nonterminal eagerly matches against a `pat` nonterminal, leading to a parse error: ```rust macro_rules! m { ($pat:pat) => {}; ($stmt:stmt) => {}; } macro_rules! m2 { ($stmt:stmt) => { m! { $stmt } }; } m2! { let x = 1 } ``` This PR fixes it by more accurately reflecting the set of nonterminals that may begin a pattern nonterminal. As a side-effect, I modified `Token::can_begin_pattern` to work correctly and used that in `Parser::nonterminal_may_begin_with`.

…kingjubilee Rollup of 10 pull requests Successful merges: - rust-lang#120221 (Don't make statement nonterminals match pattern nonterminals) - rust-lang#127897 (add `aarch64_unknown_nto_qnx700` target - QNX 7.0 support for aarch64le) - rust-lang#129123 (rustdoc-json: Add test for `Self` type) - rust-lang#129642 (Bump backtrace to 0.3.74~ish) - rust-lang#129675 (allow BufReader::peek to be called on unsized types) - rust-lang#129723 (Simplify some extern providers) - rust-lang#129724 (Remove `Option<!>` return types.) - rust-lang#129725 (Stop using `ty::GenericPredicates` for non-predicates_of queries) - rust-lang#129733 (Subtree update of `rust-analyzer`) - rust-lang#129754 (wasi: Fix sleeping for `Duration::MAX`) r? `@ghost` `@rustbot` modify labels: rollup

…-patterns, r=nnethercote Don't make statement nonterminals match pattern nonterminals Right now, the heuristic we use to check if a token may begin a pattern nonterminal falls back to `may_be_ident`: https://github.com/rust-lang/rust/blob/ef71f1047e04438181d7cb925a833e2ada6ab390/compiler/rustc_parse/src/parser/nonterminal.rs#L21-L37 This has the unfortunate side effect that a `stmt` nonterminal eagerly matches against a `pat` nonterminal, leading to a parse error: ```rust macro_rules! m { ($pat:pat) => {}; ($stmt:stmt) => {}; } macro_rules! m2 { ($stmt:stmt) => { m! { $stmt } }; } m2! { let x = 1 } ``` This PR fixes it by more accurately reflecting the set of nonterminals that may begin a pattern nonterminal. As a side-effect, I modified `Token::can_begin_pattern` to work correctly and used that in `Parser::nonterminal_may_begin_with`.

…kingjubilee Rollup of 9 pull requests Successful merges: - rust-lang#120221 (Don't make statement nonterminals match pattern nonterminals) - rust-lang#129123 (rustdoc-json: Add test for `Self` type) - rust-lang#129642 (Bump backtrace to 0.3.74~ish) - rust-lang#129675 (allow BufReader::peek to be called on unsized types) - rust-lang#129723 (Simplify some extern providers) - rust-lang#129724 (Remove `Option<!>` return types.) - rust-lang#129725 (Stop using `ty::GenericPredicates` for non-predicates_of queries) - rust-lang#129733 (Subtree update of `rust-analyzer`) - rust-lang#129751 (interpret/visitor: make memory order iteration slightly more efficient) r? `@ghost` `@rustbot` modify labels: rollup

…-patterns, r=nnethercote Don't make statement nonterminals match pattern nonterminals Right now, the heuristic we use to check if a token may begin a pattern nonterminal falls back to `may_be_ident`: https://github.com/rust-lang/rust/blob/ef71f1047e04438181d7cb925a833e2ada6ab390/compiler/rustc_parse/src/parser/nonterminal.rs#L21-L37 This has the unfortunate side effect that a `stmt` nonterminal eagerly matches against a `pat` nonterminal, leading to a parse error: ```rust macro_rules! m { ($pat:pat) => {}; ($stmt:stmt) => {}; } macro_rules! m2 { ($stmt:stmt) => { m! { $stmt } }; } m2! { let x = 1 } ``` This PR fixes it by more accurately reflecting the set of nonterminals that may begin a pattern nonterminal. As a side-effect, I modified `Token::can_begin_pattern` to work correctly and used that in `Parser::nonterminal_may_begin_with`.

…iaskrgr Rollup of 15 pull requests Successful merges: - rust-lang#120221 (Don't make statement nonterminals match pattern nonterminals) - rust-lang#126183 (Separate core search logic with search ui) - rust-lang#129123 (rustdoc-json: Add test for `Self` type) - rust-lang#129366 (linker: Synchronize native library search in rustc and linker) - rust-lang#129527 (Don't use `TyKind` in a lint) - rust-lang#129534 (Deny `wasm_c_abi` lint to nudge the last 25%) - rust-lang#129640 (Re-enable android tests/benches in alloc/core) - rust-lang#129642 (Bump backtrace to 0.3.74~ish) - rust-lang#129675 (allow BufReader::peek to be called on unsized types) - rust-lang#129723 (Simplify some extern providers) - rust-lang#129724 (Remove `Option<!>` return types.) - rust-lang#129725 (Stop using `ty::GenericPredicates` for non-predicates_of queries) - rust-lang#129731 (Allow running `./x.py test compiler`) - rust-lang#129751 (interpret/visitor: make memory order iteration slightly more efficient) - rust-lang#129754 (wasi: Fix sleeping for `Duration::MAX`) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#120221 - compiler-errors:statements-are-not-patterns, r=nnethercote Don't make statement nonterminals match pattern nonterminals Right now, the heuristic we use to check if a token may begin a pattern nonterminal falls back to `may_be_ident`: https://github.com/rust-lang/rust/blob/ef71f1047e04438181d7cb925a833e2ada6ab390/compiler/rustc_parse/src/parser/nonterminal.rs#L21-L37 This has the unfortunate side effect that a `stmt` nonterminal eagerly matches against a `pat` nonterminal, leading to a parse error: ```rust macro_rules! m { ($pat:pat) => {}; ($stmt:stmt) => {}; } macro_rules! m2 { ($stmt:stmt) => { m! { $stmt } }; } m2! { let x = 1 } ``` This PR fixes it by more accurately reflecting the set of nonterminals that may begin a pattern nonterminal. As a side-effect, I modified `Token::can_begin_pattern` to work correctly and used that in `Parser::nonterminal_may_begin_with`.

rustbot assigned nnethercote Jan 22, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 22, 2024

compiler-errors changed the title ~~Don't make pattern nonterminals match statement nonterminals~~ Don't make statement nonterminals match pattern nonterminals Jan 22, 2024

nnethercote reviewed Jan 22, 2024

View reviewed changes

rust-cloud-vms bot force-pushed the statements-are-not-patterns branch from 90ac96c to 6917fc5 Compare January 26, 2024 18:33

rustbot added the I-lang-nominated Nominated for discussion during a lang team meeting. label Feb 5, 2024

traviscross added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Feb 7, 2024

compiler-errors added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 7, 2024

scottmcm reviewed Mar 27, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Mar 27, 2024

This comment was marked as outdated.

Sign in to view

rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Mar 27, 2024

scottmcm added disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 27, 2024

rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Mar 27, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 26, 2024

workingjubilee mentioned this pull request Aug 29, 2024

Rollup of 9 pull requests #129729

Closed

workingjubilee mentioned this pull request Aug 29, 2024

Rollup of 8 pull requests #129735

Closed

workingjubilee mentioned this pull request Aug 30, 2024

Rollup of 10 pull requests #129770

Closed

workingjubilee mentioned this pull request Aug 30, 2024

Rollup of 9 pull requests #129772

Closed

This was referenced Aug 31, 2024

Rollup of 17 pull requests #129807

Closed

Rollup of 15 pull requests #129809

Merged

bors merged commit 1fd0c71 into rust-lang:master Aug 31, 2024
6 checks passed

rustbot added this to the 1.82.0 milestone Aug 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't make statement nonterminals match pattern nonterminals #120221

Don't make statement nonterminals match pattern nonterminals #120221

compiler-errors commented Jan 22, 2024 •

edited

Loading

rustbot commented Jan 22, 2024

nnethercote Jan 22, 2024

nnethercote Jan 22, 2024 •

edited

Loading

compiler-errors Jan 22, 2024 •

edited

Loading

nnethercote Jan 22, 2024

compiler-errors Jan 22, 2024

nnethercote commented Jan 22, 2024

compiler-errors commented Jan 22, 2024

nnethercote commented Jan 22, 2024

compiler-errors commented Feb 5, 2024

bors commented Mar 21, 2024

scottmcm Mar 27, 2024

scottmcm commented Mar 27, 2024

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

scottmcm commented Mar 27, 2024

rfcbot commented Mar 27, 2024 •

edited

Loading

nnethercote commented Aug 29, 2024

bors commented Aug 29, 2024

	fn may_be_ident(nt: &token::Nonterminal) -> bool {
	match nt {
	NtStmt(_)
	\| NtPat(_)
	\| NtExpr(_)
	\| NtTy(_)
	\| NtIdent(..)
	\| NtLiteral(_) // `true`, `false`
	\| NtMeta(_)
	\| NtPath(_) => true,

	NtItem(_)
	\| NtBlock(_)
	\| NtVis(_)
	\| NtLifetime(_) => false,
	}
	}

	/// the stable set of tokens that are allowed to match an pat nonterminal.
	/// the stable set of tokens that are allowed to match a pat nonterminal.

Don't make statement nonterminals match pattern nonterminals #120221

Don't make statement nonterminals match pattern nonterminals #120221

Conversation

compiler-errors commented Jan 22, 2024 • edited Loading

rustbot commented Jan 22, 2024

nnethercote Jan 22, 2024

Choose a reason for hiding this comment

nnethercote Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

compiler-errors Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

nnethercote Jan 22, 2024

Choose a reason for hiding this comment

compiler-errors Jan 22, 2024

Choose a reason for hiding this comment

nnethercote commented Jan 22, 2024

compiler-errors commented Jan 22, 2024

nnethercote commented Jan 22, 2024

compiler-errors commented Feb 5, 2024

bors commented Mar 21, 2024

scottmcm Mar 27, 2024

Choose a reason for hiding this comment

scottmcm commented Mar 27, 2024

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

scottmcm commented Mar 27, 2024

rfcbot commented Mar 27, 2024 • edited Loading

nnethercote commented Aug 29, 2024

bors commented Aug 29, 2024

compiler-errors commented Jan 22, 2024 •

edited

Loading

nnethercote Jan 22, 2024 •

edited

Loading

compiler-errors Jan 22, 2024 •

edited

Loading

rfcbot commented Mar 27, 2024 •

edited

Loading