-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
Change keyword order for impl restrictions
#155442
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
Changes from 5 commits
5621173
5d8a297
a0b8e89
614994f
0e6efe5
470e462
cbba81e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1052,80 +1052,58 @@ impl<'a> Parser<'a> { | |
| } | ||
| } | ||
|
|
||
| /// Is there an `[ impl(in? path) ]? trait` item `dist` tokens ahead? | ||
| fn is_trait_with_maybe_impl_restriction_in_front(&self, dist: usize) -> bool { | ||
| // `trait` | ||
| if self.is_keyword_ahead(dist, &[kw::Trait]) { | ||
| return true; | ||
| } | ||
| // `impl(` | ||
| if !self.is_keyword_ahead(dist, &[kw::Impl]) | ||
| || !self.look_ahead(dist + 1, |t| t == &token::OpenParen) | ||
| { | ||
| return false; | ||
| } | ||
| // `crate | super | self) trait` | ||
| if self.is_keyword_ahead(dist + 2, &[kw::Crate, kw::Super, kw::SelfLower]) | ||
| && self.look_ahead(dist + 3, |t| t == &token::CloseParen) | ||
| && self.is_keyword_ahead(dist + 4, &[kw::Trait]) | ||
| { | ||
| return true; | ||
| } | ||
| // `impl(in? something) trait` | ||
| // We catch cases where the `in` keyword is missing to provide a | ||
| // better error message. This is handled later in | ||
| // `self.recover_incorrect_impl_restriction`. | ||
| self.tree_look_ahead(dist + 2, |t| { | ||
| if let TokenTree::Token(token, _) = t { token.is_keyword(kw::Trait) } else { false } | ||
| }) | ||
| .unwrap_or(false) | ||
| } | ||
|
|
||
| /// 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 { | ||
| // `[ impl(in? path) ]? trait` | ||
| if self.is_trait_with_maybe_impl_restriction_in_front(0) { | ||
| return true; | ||
| } | ||
| // `auto [ impl(in? path) ]? trait` | ||
| if self.check_keyword(exp!(Auto)) && self.is_trait_with_maybe_impl_restriction_in_front(1) { | ||
| return true; | ||
| } | ||
| // `unsafe auto? [ impl(in? path) ]? trait` | ||
| if self.check_keyword(exp!(Unsafe)) | ||
| && (self.is_trait_with_maybe_impl_restriction_in_front(1) | ||
| || self.is_keyword_ahead(1, &[kw::Auto]) | ||
| && self.is_trait_with_maybe_impl_restriction_in_front(2)) | ||
| { | ||
| return true; | ||
| } | ||
| // `const` ... | ||
| if !self.check_keyword(exp!(Const)) { | ||
| return false; | ||
| } | ||
| // `const [ impl(in? path) ]? trait` | ||
| if self.is_trait_with_maybe_impl_restriction_in_front(1) { | ||
| return true; | ||
| } | ||
| // `const (unsafe | auto) [ impl(in? path) ]? trait` | ||
| if self.is_keyword_ahead(1, &[kw::Unsafe, kw::Auto]) | ||
| && self.is_trait_with_maybe_impl_restriction_in_front(2) | ||
| { | ||
| return true; | ||
| const SUFFIXES: &[&[Symbol]] = &[ | ||
| &[kw::Trait], | ||
| &[kw::Auto, kw::Trait], | ||
| &[kw::Unsafe, kw::Trait], | ||
| &[kw::Unsafe, kw::Auto, kw::Trait], | ||
| &[kw::Const, kw::Trait], | ||
| &[kw::Const, kw::Auto, kw::Trait], | ||
| &[kw::Const, kw::Unsafe, kw::Trait], | ||
| &[kw::Const, kw::Unsafe, kw::Auto, kw::Trait], | ||
| ]; | ||
| // `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, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand the need to perform all this suffix checking if we have If the prefix is Or is there a case where
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (For comparison, this is how the keyword order change looks in my own Rust parser: https://github.com/fmease/rasur/pull/34/changes)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, when there is an
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe adding early returns like the following would be good? // `impl(in` unambiguously introduces an `impl` restriction
if self.is_keyword_ahead(2, &[kw::In]) {
return true;
}
// `impl(crate | self | super)` + SUFFIX
if self.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]))
})
{
return true;
}
// 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| {
if let TokenTree::Token(token, _) = t {
token.is_keyword(*kw)
} else {
false
}
})
.unwrap_or(false)
})
})
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cbba81e |
||
| // we treat `(in? path)` as a `TokenTree::Delimited`, | ||
| // so we look ahead over token trees rather than tokens. | ||
| SUFFIXES.iter().any(|suffix| { | ||
| suffix.iter().enumerate().all(|(i, kw)| { | ||
| self.tree_look_ahead(i + 2, |t| { | ||
| if let TokenTree::Token(token, _) = t { | ||
| token.is_keyword(*kw) | ||
| } else { | ||
| false | ||
| } | ||
| }) | ||
| .unwrap_or(false) | ||
| }) | ||
| }) | ||
| } else { | ||
| SUFFIXES.iter().any(|suffix| { | ||
| suffix.iter().enumerate().all(|(i, kw)| { | ||
| // We use `check_keyword` for the first token to include it in the expected tokens. | ||
| if i == 0 { | ||
| match *kw { | ||
| kw::Const => self.check_keyword(exp!(Const)), | ||
| kw::Unsafe => self.check_keyword(exp!(Unsafe)), | ||
| kw::Auto => self.check_keyword(exp!(Auto)), | ||
| kw::Trait => self.check_keyword(exp!(Trait)), | ||
| _ => unreachable!(), | ||
| } | ||
| } else { | ||
| self.is_keyword_ahead(i, &[*kw]) | ||
| } | ||
| }) | ||
| }) | ||
| } | ||
|
Urgau marked this conversation as resolved.
|
||
| // `const unsafe auto [ impl(in? path) ]? trait` | ||
| self.is_keyword_ahead(1, &[kw::Unsafe]) | ||
| && self.is_keyword_ahead(2, &[kw::Auto]) | ||
| && self.is_trait_with_maybe_impl_restriction_in_front(3) | ||
| } | ||
|
|
||
| /// Parses `const? unsafe? auto? [impl(in? path)]? trait Foo { ... }` or `trait Foo = Bar;`. | ||
| /// | ||
| /// FIXME(restrictions): The current keyword order follows the grammar specified in RFC 3323. | ||
| /// However, whether the restriction should be grouped closer to the visibility modifier | ||
| /// (e.g., `pub impl(crate) const unsafe auto trait`) remains an unresolved design question. | ||
| /// This ordering must be kept in sync with the logic in `check_trait_front_matter`. | ||
| /// Parses `[impl(in? path)]? const? unsafe? auto? trait Foo { ... }` or `trait Foo = Bar;`. | ||
| fn parse_item_trait(&mut self, attrs: &mut AttrVec, lo: Span) -> PResult<'a, ItemKind> { | ||
| let impl_restriction = self.parse_impl_restriction()?; | ||
| let constness = self.parse_constness(Case::Sensitive); | ||
| if let Const::Yes(span) = constness { | ||
| self.psess.gated_spans.gate(sym::const_trait_impl, span); | ||
|
|
@@ -1139,8 +1117,6 @@ impl<'a> Parser<'a> { | |
| IsAuto::No | ||
| }; | ||
|
|
||
| let impl_restriction = self.parse_impl_restriction()?; | ||
|
|
||
| self.expect_keyword(exp!(Trait))?; | ||
| let ident = self.parse_ident()?; | ||
| let mut generics = self.parse_generics()?; | ||
|
|
@@ -1181,10 +1157,10 @@ impl<'a> Parser<'a> { | |
| generics.where_clause = self.parse_where_clause()?; | ||
| let items = self.parse_item_list(attrs, |p| p.parse_trait_item(ForceCollect::No))?; | ||
| Ok(ItemKind::Trait(Box::new(Trait { | ||
| impl_restriction, | ||
| constness, | ||
| is_auto, | ||
| safety, | ||
| impl_restriction, | ||
| ident, | ||
| generics, | ||
| bounds, | ||
|
|
@@ -2966,8 +2942,8 @@ impl<'a> Parser<'a> { | |
| && !self.is_unsafe_foreign_mod() | ||
| // Rule out `async gen {` and `async gen move {` | ||
| && !self.is_async_gen_block() | ||
| // Rule out `const unsafe auto` and `const unsafe trait` and `const unsafe impl`. | ||
| && !self.is_keyword_ahead(2, &[kw::Auto, kw::Trait, kw::Impl]) | ||
| // Rule out `const unsafe auto` and `const unsafe trait` | ||
| && !self.is_keyword_ahead(2, &[kw::Auto, kw::Trait]) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will regress #![feature(const_trait_impl)]
const unsafe impl Trait for () {}
const unsafe trait Trait {}Can you please revert that change and add a test case for this syntax somewhere in 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 470e462 |
||
| ) | ||
| }) | ||
| // `extern ABI fn` | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thatimpl(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.
View changes since the review
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{)). ↩