Modify message for keyword as identifier name #46497
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
This error doesn't necessarily have to be reported in lexer. |
|
That's a good idea. Will do that tomorrow. Thanks for the suggestion. |
kennytm
added
the
S-waiting-on-author
AgustinCB
force-pushed
the
issue-46311
branch
from
08ee00b
to
65ccf24
Compare
|
Done! I'm not sure if it was the best task to make my first contribution, as I wasn't completely sure I understood all the details and some of the decisions that I made. Let me know if I'm missing something. Thanks. |
| self.invalid_visibility(&item.vis, item.span, None); | ||
| for impl_item in impl_items { | ||
| self.invalid_visibility(&impl_item.vis, impl_item.span, None); | ||
| if let ImplItemKind::Method(ref sig, _) = impl_item.node { | ||
| self.check_trait_fn_not_const(sig.constness); | ||
| } | ||
| } | ||
| generics.lifetimes.iter().for_each(|l| self.check_lifetime(&l.lifetime)) |
There was a problem hiding this comment.
It's not necessary to check lifetimes in all possible contexts manually, you can add a visit_lifetime method to impl<'a> Visitor<'a> for AstValidator<'a> and call check_lifetime from it.
Visitor is kind of an object-oriented infrastructure for walking through AST. The default visitor in libsyntax/visit.rs is a base visitor that does nothing and only walks the tree and other concrete visitors (like AstValidator) can override its methods and add something useful to them.
There was a problem hiding this comment.
This is actually a good first task because the visitor is a quite important piece of infrastructure and it makes you familiar with it.
| @@ -35,8 +34,16 @@ impl<'a> AstValidator<'a> { | |||
| &self.session.parse_sess.span_diagnostic | |||
| } | |||
|
|
|||
| fn check_lifetime(&self, lifetime: &Lifetime) { | |||
| if !lifetime.ident.without_first_quote().is_valid() && | |||
There was a problem hiding this comment.
lifetime.ident.without_first_quote().is_valid() -> token::Ident(lifetime.ident).is_reserved_ident() and all the new functions (is_valid, is_used_keyword, is_static_keyword, etc) can be removed.
Also without_first_quote is used only once so it's probably better inlined right here.
There was a problem hiding this comment.
Invalid names for lifetimes are all is_reserved_ident names except for keywords::Static and keywords::Invalid.
Invalid names for labels are simply all is_reserved_ident plus '_.
|
@bors r+ |
|
📌 Commit 29e2680 has been approved by |
Modify message for keyword as identifier name This is a temporary solution to #46311. The message is generic enough to cover both cases and is probably a fine enough solution to the specific problem described in the task. However, the underlying reason for this to be wrong is that `next_token_inner` returns `Lifetime` even if the token is a label. That's not simple, as the syntax for both can be quite similar and it may need to take a look to the next token to make a decision. I'm not sure I have enough knowledge about the project to be able to solve that (yet!), so I thought I'll fix the immediate problem first.
|
☀️ Test successful - status-appveyor, status-travis |
This is a temporary solution to #46311.
The message is generic enough to cover both cases and is probably a fine enough solution to the specific problem described in the task. However, the underlying reason for this to be wrong is that
next_token_innerreturnsLifetimeeven if the token is a label. That's not simple, as the syntax for both can be quite similar and it may need to take a look to the next token to make a decision. I'm not sure I have enough knowledge about the project to be able to solve that (yet!), so I thought I'll fix the immediate problem first.