Don't make statement nonterminals match pattern nonterminals by compiler-errors · Pull Request #120221 · rust-lang/rust (original) (raw)

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

compiler-errors

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

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

r? @nnethercote

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

@rustbot 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 compiler-errors changed the titleDon't make pattern nonterminals match statement nonterminals Don't make statement nonterminals match pattern nonterminals

Jan 22, 2024

nnethercote

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.

Choose a reason for hiding this comment

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

/// 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.
AndAnd | // double reference
Literal(_) | // literal
DotDot | // range pattern (future compat)
DotDotDot | // range pattern (future compat)

Choose a reason for hiding this comment

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

AFAICT, the net effect here:

Is that right?

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Yes

@nnethercote

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

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

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

@compiler-errors

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

scottmcm

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 {

Choose a reason for hiding this comment

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

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

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

This comment was marked as outdated.

@scottmcm

This comment was marked as outdated.

@rfcbot

This comment was marked as outdated.

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

@scottmcm

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

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.

@bors 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.

S-waiting-on-team

Status: Awaiting decision from the relevant subteam (see the T- label).

labels

Aug 29, 2024

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request

Aug 29, 2024

@workingjubilee

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

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.

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

Aug 29, 2024

@bors

…kingjubilee

Rollup of 9 pull requests

Successful merges:

r? @ghost @rustbot modify labels: rollup

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request

Aug 29, 2024

@workingjubilee

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

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.

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

Aug 29, 2024

@bors

…kingjubilee

Rollup of 8 pull requests

Successful merges:

r? @ghost @rustbot modify labels: rollup

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

Aug 29, 2024

@bors

…kingjubilee

Rollup of 8 pull requests

Successful merges:

r? @ghost @rustbot modify labels: rollup

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

Aug 29, 2024

@bors

…kingjubilee

Rollup of 8 pull requests

Successful merges:

r? @ghost @rustbot modify labels: rollup

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

Aug 29, 2024

@bors

…kingjubilee

Rollup of 8 pull requests

Successful merges:

r? @ghost @rustbot modify labels: rollup

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request

Aug 30, 2024

@workingjubilee

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

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.

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

Aug 30, 2024

@bors

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request

Aug 30, 2024

@workingjubilee

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

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.

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

Aug 30, 2024

@bors

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

Aug 30, 2024

@bors

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

Aug 30, 2024

@bors

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

Aug 31, 2024

@matthiaskrgr

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

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.

This was referenced

Aug 31, 2024

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

Aug 31, 2024

@bors

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

Aug 31, 2024

@rust-timer

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:

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.

tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request

Oct 18, 2024

@tmeijn

This MR contains the following updates:

Package Update Change
rust minor 1.81.0 -> 1.82.0

MR created with the help of el-capitano/tools/renovate-bot.

Proposed changes to behavior should be submitted there as MRs.

Release Notes

rust-lang/rust (rust)

v1.82.0

Compare Source

==========================

Language

Compiler

Libraries

Stabilized APIs

These APIs are now stable in const contexts:

Cargo

Compatibility Notes

Internal Changes

These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools.

Configuration

📅 Schedule: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 Automerge: Disabled by config. Please merge this manually once you are satisfied.

♻ Rebasing: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 Ignore: Close this MR and you won't be reminded about this update again.

This MR has been generated by Renovate Bot.

wip-sync pushed a commit to NetBSD/pkgsrc-wip that referenced this pull request

Oct 27, 2024

@he32

Pkgsrc changes:

Upstream changes:

Version 1.82.0 (2024-10-17)

Language

Compiler

Libraries

Stabilized APIs

These APIs are now stable in const contexts:

Cargo

Compatibility Notes

Internal Changes

These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools.

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request

Feb 2, 2025

@he32

Pkgsrc changes:

Upstream changes:

Version 1.82.0 (2024-10-17)

Language

Compiler

Libraries

Stabilized APIs

These APIs are now stable in const contexts:

Cargo

Compatibility Notes

Internal Changes

These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools.