Uh oh!
There was an error while loading. Please reload this page.
Change keyword order for impl restrictions - #155442
Conversation
rustbot
commented
Apr 17, 2026
The parser was modified, potentially altering the grammar of (stable) Rust cc @fmease |
8e2d081 to
5d955caComparerust-log-analyzer
commented
Apr 17, 2026
The job Click to see the possible cause of the failure (guessed by this bot) |
5d955ca to
4178059CompareCoCo-Japan-pan
commented
Apr 17, 2026
The auto keyword was accidentally removed from the set of possible tokens, which caused UI tests to fail. |
This comment has been minimized.
This comment has been minimized.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
rustbot
commented
Apr 18, 2026
Some changes occurred in src/tools/clippy cc @rust-lang/clippy Some changes occurred in src/tools/rustfmt cc @rust-lang/rustfmt |
646fa9a to
78cb850Comparerustbot
commented
Apr 18, 2026
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
Uh oh!
There was an error while loading. Please reload this page.
78cb850 to
0e6efe5Compare| // Rule out `const unsafe auto` and `const unsafe trait` | ||
| && !self.is_keyword_ahead(2, &[kw::Auto, kw::Trait]) |
There was a problem hiding this comment.
This will regress
#![feature(const_trait_impl)]constunsafeimplTraitfor(){}constunsafetraitTrait{}Can you please revert that change and add a test case for this syntax somewhere in tests/ui/traits/const-traits/?
I know that you added this check in PR #152943 for unrelated reasons but it did silently fix this const-trait-impl issue that I unbeknownst to you originally reported in #148434 (comment) / #146122 (comment). I was aware of that when I reviewed your other PR but didn't mention it which I probably should've.
There was a problem hiding this comment.
470e462
Reverted the change and added a test case.
| /// Is this an `(const unsafe? auto? [ impl(in? path) ]? | unsafe auto? [ impl(in? path) ]? | auto [ impl(in? path) ]? | [ impl(in? path) ]?) trait` item? | ||
| /// Is this an `[impl(in? path)]? const? unsafe? auto? trait` item? | ||
| fn check_trait_front_matter(&mut self) -> bool { |
There was a problem hiding this comment.
If I'm not mistaken the only conflict with impl blocks we have to avoid is impl (crate) { etc. or am I missing classes of cases? Therefore we only need to ensure that impl(CRATE_OR_SUPER_OR_SELF) is not followed by {.
Well, a negative check might not be liked, so I guess it's alright to keep the "expensive" elaborate suffix checks1.
Footnotes
We have a sort of similar situation with
impl !Trait for Type {}versusimpl ! {}where rustc does a positive check (whether the next token can begin) but where rust analyzer does a negative check (whether it's not{)). ↩
| ]; | ||
| // `impl(` | ||
| if self.check_keyword(exp!(Impl)) && self.look_ahead(1, |t| t == &token::OpenParen) { | ||
| // Since the `path` in `impl(in? path)` can be arbitrarily long, |
There was a problem hiding this comment.
I don't understand the need to perform all this suffix checking if we have in, the sequence impl(in should unambiguously introduce an impl restriction that's its whole purpose; it's a disambiguator. Therefore I'm not sure why we have to assume the worst and perform a tree lookahead.
If the prefix is impl(crate, impl(super or impl(self then and only then do we need to check that it's followed by ) (easy) followed by NOT({) // SUFFIXES.
Or is there a case where impl(in may refer to something other than an impl restriction that I don't know about?
There was a problem hiding this comment.
(For comparison, this is how the keyword order change looks in my own Rust parser: https://github.com/fmease/rasur/pull/34/changes)
There was a problem hiding this comment.
Yes, when there is an in, we can immediately return true here.
Still, to recover the cases where in is lost like impl(path::to::module) and suggest to insert the in, I think using tree_look_ahead and check it's suffix is needed for positive tests.
There was a problem hiding this comment.
Maybe adding early returns like the following would be good?
// `impl(in` unambiguously introduces an `impl` restrictionifself.is_keyword_ahead(2,&[kw::In]){returntrue;}// `impl(crate | self | super)` + SUFFIXifself.is_keyword_ahead(2,&[kw::Crate, kw::SelfLower, kw::Super])
&& self.look_ahead(3, |t| t == &token::CloseParen)
&& SUFFIXES.iter().any(|suffix| {
suffix.iter().enumerate().all(|(i, kw)| self.is_keyword_ahead(i + 4,&[*kw]))}){returntrue;}// Recover cases like `impl(path::to::module)` + SUFFIX to suggest inserting `in`.SUFFIXES.iter().any(|suffix| {
suffix.iter().enumerate().all(|(i, kw)| {self.tree_look_ahead(i + 2, |t| {ifletTokenTree::Token(token, _) = t {
token.is_keyword(*kw)}else{false}}).unwrap_or(false)})})correctly parse `const unsafe trait` implementations.
CoCo-Japan-pan
commented
Apr 22, 2026
Following the positive-check approach for now. |
Rollup of 7 pull requests Successful merges: - #155469 (Account for titlecase in casing lints) - #155644 (delegation: support self ty propagation for functions in free to trait reuse) - #154957 (Fix ICE when const closure appears inside a non-const trait method) - #155442 (Change keyword order for `impl` restrictions) - #155561 (Use singular wording for single _ placeholders in type suggestions) - #155637 (Fix E0191 suggestion for empty dyn trait args) - #155661 (Remove `AttributeLintKind` variants - part 6)
Uh oh!
There was an error while loading. Please reload this page.
Rollup merge of #155442 - CoCo-Japan-pan:impl-restriction-reorder, r=Urgau,fmease,jhpratt Change keyword order for `impl` restrictions Based on #155222, this PR reorders keywords in trait definitions to group restrictions with visibility. It changes the order from `pub(...) const unsafe auto impl(...) trait Foo {...}` to `pub(...) impl(...) const unsafe auto trait Foo {...}`. Tracking issue for restrictions: #105077 r? @Urgau cc @jhpratt
…order, r=Urgau,fmease,jhpratt Change keyword order for `impl` restrictions Based on rust-lang#155222, this PR reorders keywords in trait definitions to group restrictions with visibility. It changes the order from `pub(...) const unsafe auto impl(...) trait Foo {...}` to `pub(...) impl(...) const unsafe auto trait Foo {...}`. Tracking issue for restrictions: rust-lang#105077 r? @Urgau cc @jhpratt
Rollup of 7 pull requests Successful merges: - rust-lang/rust#155469 (Account for titlecase in casing lints) - rust-lang/rust#155644 (delegation: support self ty propagation for functions in free to trait reuse) - rust-lang/rust#154957 (Fix ICE when const closure appears inside a non-const trait method) - rust-lang/rust#155442 (Change keyword order for `impl` restrictions) - rust-lang/rust#155561 (Use singular wording for single _ placeholders in type suggestions) - rust-lang/rust#155637 (Fix E0191 suggestion for empty dyn trait args) - rust-lang/rust#155661 (Remove `AttributeLintKind` variants - part 6)
View all comments
Based on #155222, this PR reorders keywords in trait definitions to group restrictions with visibility. It changes the order from
pub(...) const unsafe auto impl(...) trait Foo {...}topub(...) impl(...) const unsafe auto trait Foo {...}.Tracking issue for restrictions: #105077
r? @Urgau
cc @jhpratt