Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix const items not being allowed to be called r#move or r#static #137140

Merged
merged 1 commit into from
Feb 17, 2025

Conversation

Noratrieb
Copy link
Member

Because of an ambiguity with const closures, the parser needs to ensure that for a const item, the const keyword isn't followed by a move or static keyword, as that would indicate a const closure:

fn main() {
  const move // ...
}

This check did not take raw identifiers into account, therefore being unable to distinguish between const move and const r#move. The latter is obviously not a const closure, so it should be allowed as a const item.

This fixes the check in the parser to only treat const ... as a const closure if it's followed by the proper keyword, and not a raw identifier.

Additionally, this adds a large test that tests for all raw identifiers in all kinds of positions, including const, to prevent issues like this one from occurring again.

fixes #137128

@rustbot
Copy link
Collaborator

rustbot commented Feb 16, 2025

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Feb 16, 2025
@Noratrieb
Copy link
Member Author

I don't really know if this is something that should get a lang FCP or not... This is definitely a bug and an (older) regression, so I'm inclined to say no. I don't think a lang FCP would be useful, as it's obvious that this should be allowed. But I'll @rust-lang/lang for visibility.

Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

The parser change looks good to me, I have a nit about the test.

@jieyouxu

This comment was marked as resolved.

@jieyouxu jieyouxu added A-parser Area: The parsing of Rust source code to an AST T-lang Relevant to the language team, which will review and decide on the PR/issue. I-lang-nominated Nominated for discussion during a lang team meeting. I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy; this does not imply nomination 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 Feb 16, 2025
@Noratrieb
Copy link
Member Author

I'm going to un-nominate this, because I don't think this is worth T-lang's very limited bandwidth. If anyone has objections, you can send them here, but I cannot imagine what an objection would look like :D.

@Noratrieb Noratrieb added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed I-lang-nominated Nominated for discussion during a lang team meeting. I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy; this does not imply nomination S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Feb 16, 2025
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

Yeah, this definitely does not need lang discussion because it's just a parser bug.

@Noratrieb Noratrieb 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 Feb 16, 2025
@Noratrieb
Copy link
Member Author

@theemathas pointed out that i wrote this test in a quite silly way, so I fixed that. And I added revisions.

@rustbot rustbot 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 Feb 16, 2025
Because of an ambiguity with const closures, the parser needs to ensure
that for a const item, the `const` keyword isn't followed by a `move` or
`static` keyword, as that would indicate a const closure:

```rust
fn main() {
  const move // ...
}
```

This check did not take raw identifiers into account, therefore being
unable to distinguish between `const move` and `const r#move`. The
latter is obviously not a const closure, so it should be allowed as a
const item.

This fixes the check in the parser to only treat `const ...` as a const
closure if it's followed by the *proper keyword*, and not a raw
identifier.

Additionally, this adds a large test that tests for all raw identifiers in
all kinds of positions, including `const`, to prevent issues like this
one from occurring again.
@Noratrieb Noratrieb 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 Feb 16, 2025
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Thanks, also r=me once PR CI is green.

@Noratrieb
Copy link
Member Author

image
looks green to me

@bors r=jieyouxu,compiler-errors

@bors
Copy link
Contributor

bors commented Feb 16, 2025

📌 Commit 8a02724 has been approved by jieyouxu,compiler-errors

It is now in the queue for this repository.

@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. labels Feb 16, 2025
@jieyouxu jieyouxu removed the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Feb 16, 2025
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Feb 16, 2025
…ompiler-errors

Fix const items not being allowed to be called `r#move` or `r#static`

Because of an ambiguity with const closures, the parser needs to ensure that for a const item, the `const` keyword isn't followed by a `move` or `static` keyword, as that would indicate a const closure:

```rust
fn main() {
  const move // ...
}
```

This check did not take raw identifiers into account, therefore being unable to distinguish between `const move` and `const r#move`. The latter is obviously not a const closure, so it should be allowed as a const item.

This fixes the check in the parser to only treat `const ...` as a const closure if it's followed by the *proper keyword*, and not a raw identifier.

Additionally, this adds a large test that tests for all raw identifiers in all kinds of positions, including `const`, to prevent issues like this one from occurring again.

fixes rust-lang#137128
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 17, 2025
…kingjubilee

Rollup of 12 pull requests

Successful merges:

 - rust-lang#136671 (Overhaul `rustc_middle::limits`)
 - rust-lang#136817 (Pattern Migration 2024: clean up and comment)
 - rust-lang#136844 (Use `const_error!` when possible)
 - rust-lang#136953 (rustc_target: import TargetMetadata)
 - rust-lang#137095 (Replace some u64 hashes with Hash64)
 - rust-lang#137100 (HIR analysis: Remove unnecessary abstraction over list of clauses)
 - rust-lang#137105 (Restrict DerefPure for Cow<T> impl to T = impl Clone, [impl Clone], str.)
 - rust-lang#137120 (Enable `relative-path-include-bytes-132203` rustdoc-ui test on Windows)
 - rust-lang#137125 (Re-add missing empty lines in the releases notes)
 - rust-lang#137140 (Fix const items not being allowed to be called `r#move` or `r#static`)
 - rust-lang#137145 (use add-core-stubs / minicore for a few more tests)
 - rust-lang#137149 (Remove SSE ABI from i586-pc-windows-msvc)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 17, 2025
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#136466 (Start removing `rustc_middle::hir::map::Map`)
 - rust-lang#136671 (Overhaul `rustc_middle::limits`)
 - rust-lang#136817 (Pattern Migration 2024: clean up and comment)
 - rust-lang#136844 (Use `const_error!` when possible)
 - rust-lang#137080 (bootstrap: add more tracing to compiler/std/llvm flows)
 - rust-lang#137101 (`invalid_from_utf8[_unchecked]`: also lint inherent methods)
 - rust-lang#137140 (Fix const items not being allowed to be called `r#move` or `r#static`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f071099 into rust-lang:master Feb 17, 2025
6 checks passed
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 17, 2025
Rollup merge of rust-lang#137140 - Noratrieb:const-move, r=jieyouxu,compiler-errors

Fix const items not being allowed to be called `r#move` or `r#static`

Because of an ambiguity with const closures, the parser needs to ensure that for a const item, the `const` keyword isn't followed by a `move` or `static` keyword, as that would indicate a const closure:

```rust
fn main() {
  const move // ...
}
```

This check did not take raw identifiers into account, therefore being unable to distinguish between `const move` and `const r#move`. The latter is obviously not a const closure, so it should be allowed as a const item.

This fixes the check in the parser to only treat `const ...` as a const closure if it's followed by the *proper keyword*, and not a raw identifier.

Additionally, this adds a large test that tests for all raw identifiers in all kinds of positions, including `const`, to prevent issues like this one from occurring again.

fixes rust-lang#137128
@rustbot rustbot added this to the 1.87.0 milestone Feb 17, 2025
@Noratrieb Noratrieb deleted the const-move branch February 17, 2025 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-parser Area: The parsing of Rust source code to an AST S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Undocumented: Identifier r#move cannot be used as the name of a constant
6 participants