Edition-gated keywords #49611
Edition-gated keywords #49611
Conversation
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Ah, made a mistake, these need to parse as keywords not contextual keywords. |
pietroalbini
added
the
S-waiting-on-author
Manishearth
force-pushed
the
edition-kw
branch
4 times, most recently
from
02403a6
to
62cdeb9
Compare
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Manishearth
force-pushed
the
edition-kw
branch
2 times, most recently
from
d81e221
to
b1d099f
Compare
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
No longer WIP r? @nikomatsakis or @estebank I'm going to handle the lint elsewhere; because it's got some impl challenges that we have to work out. |
kennytm
added
S-waiting-on-review
|
☔ The latest upstream changes (presumably #49154) made this pull request unmergeable. Please resolve the merge conflicts. |
| // After modifying this list adjust `is_special_ident`, `is_used_keyword`/`is_unused_keyword`, | ||
| // this should be rarely necessary though if the keywords are kept in alphabetic order. | ||
| // After modifying this list adjust `is_special`, `is_used_keyword`/`is_unused_keyword` | ||
| // velow |
There was a problem hiding this comment.
This looks basically good, except I think we need some more tests... am I wrong? It feels like we're not testing all the combinations. For example, there is no Epoch 2018 code testing macros created by an Epoch 2018 auxiliary crate, and vice versa. I'd like to have those tests.
| @@ -41,13 +40,13 @@ impl<'a> AstValidator<'a> { | |||
| keywords::StaticLifetime.name(), | |||
| keywords::Invalid.name()]; | |||
| if !valid_names.contains(&lifetime.ident.name) && | |||
| token::is_reserved_ident(lifetime.ident.without_first_quote()) { | |||
| lifetime.ident.without_first_quote().is_reserved() { | |||
There was a problem hiding this comment.
Nit: Pre-existing, but can you move the { to the next line (and/or move the && to next line)? I find the condition blends in with body in this scenario. (I think rustfmt puts the && on next line and the {...? but I forget)
src/libsyntax_pos/symbol.rs
Outdated
| self.name <= self::keywords::Underscore.name() | ||
| } | ||
|
|
||
| /// Returns `true` if the token is a keyword used in the language. |
There was a problem hiding this comment.
I am confused about this. At minimum, can we update the comments to describe how this interacts with editions...? It seems like this is "assuming 2015 Edition"?
| pub fn main() { | ||
| let mut r#async = 1; | ||
|
|
||
| r#async = consumes_async!(r#async); |
There was a problem hiding this comment.
can we have a compile-fail test with consumes_async!(async)? and so forth?
src/libsyntax_pos/symbol.rs
Outdated
| /// Returns `true` if the token is a keyword used in the language. | ||
| #[inline] | ||
| pub fn is_used_keyword(self) -> bool { | ||
| self.name >= self::keywords::As.name() && self.name <= self::keywords::While.name() |
There was a problem hiding this comment.
plus...oh dear this is brittle. But ok, pre-existing. =) Feels like the macro should be able to generate these constants though.
There was a problem hiding this comment.
I thought hard about figuring out a way to do that; it's possible with dummy symbols but otherwise not really. Easier with a proc macro.
There was a problem hiding this comment.
Dummy symbols doesn't necessarily seem so bad to me though -- but anyway separate.
|
Updated |
…h edition-gated keywords
…h edition-gated keywords
|
r=me but it occurs to me that @petrochenkov would probably like to take a look at this |
|
@petrochenkov (poke) |
| #[inline] | ||
| pub fn is_reserved(self) -> bool { | ||
| self.is_special() || self.is_used_keyword() || self.is_unused_keyword() | ||
| } |
There was a problem hiding this comment.
If these functions are moved to methods, then they should rather become methods of Symbol (aka Name) than Ident.
There was a problem hiding this comment.
Can we let that be a mentored followup?
|
|
||
| pub fn main() { | ||
| let _ = consumes_async!(async); | ||
| //~^ ERROR no rules expected the token `async` |
There was a problem hiding this comment.
I don't think this is correct behavior, macros' left hand sides don't care about keywordy-ness status of tokens they match. They can equally well match tokens from code snippets written in any Rust editions or even in C++, Java or mix of both. (See #49520)
So the "pretend these are raw even if they aren't" shortcut doesn't generally work.
The correct solution requires a bit more effort, but not significantly more, thankfully:
- Tweak all instances of
is_keywordinvolving edition-dependent keywords (no such instances right now! hurray) - Make all instances of
is_reserved_ident(maybe all 13 of them) edition-dependent if the result makes difference for edition-dependent keywords. - Probably tweak metadata serialization/deserialization code for macros' right sides, not sure about details of this.
There was a problem hiding this comment.
I disagree; we're going to ask macro consumers to rustfix usages of async in macro invocations as well, because crates may be using those tokens verbatim (slurping them via tt and then pasting them out) as well as using them to match keywords.
I find it would be inconsistent if some macros stopped working in the 2018 epoch when fed async but not others.
thoughts, @nikomatsakis ?
There was a problem hiding this comment.
Also, FWIW for the lint we're going to have to tristate the bool field anyway, as a "specified raw, keyword but not on current epoch, and keyword on current epoch", and we can have the macro matcher consider the last two to be equal. But I'm not certain.
There was a problem hiding this comment.
This is not only about macros, what bothers me is that the "pretend these are raw" implementation works on fundamentally wrong level. There are no keywords in the token world, that's why neither lexer nor macros weren't concerned about keywords previously. Keywords appear only when we introduce some grammar on those tokens, so the edition-keyword problem need to be solved on grammar/syntax level as well.
This means is_reserved_ident locally and I think deserialization (or/and maybe expansion?) of macros from other crates. Any cross-crate-cross-edition compatibility for keywords would involve some amount of guessing (including this PR's) but we should be able to guess better if we are doing it at the right level.
I need some time to flesh this out, maybe make a prototype (this shouldn't be a large change), I'm not satisfied with what this PR does at all.
There was a problem hiding this comment.
Okay, so --- in the changes I haven't made yet (but planned to for the linting step once we figure that out better), there will be three states:
- actually raw
- allowed to be an ident (specified as non-raw on an edition where it is not keyword)
- not allowed to be an ident (specified as non-raw on an edition where it is a keyword)
This will mean that we no longer pretend things are raw, and we can do equality differently in different contexts.
For epoch hygiene to work we basically have to do this at the token level, though. I don't like having keywords pollute the lexer, but I don't see a better way of doing this that handles macros well.
There was a problem hiding this comment.
Linting what exactly? Identifiers that are not keywords in this edition, but will be keywords in the next edition?
Lints are attached to NodeIds and we don't have those in the parser yet and we don't have lint attribute "scopes" in the parser either.
I think we can just visit all identifiers remaining in the AST again during the early lint pass and report those with zero hygiene context (current crate) and "keywordy" name. This may leave some identifiers unreported, but that's not a big deal probably.
There was a problem hiding this comment.
We'd also need to lint stuff in macro defs and invocations, which complicates things.
As an edition breakage it's important that we lint everything.
There was a problem hiding this comment.
Regarding reporting "really everything", I think this is more of a problem with lint infrastructure rather than with edition keywords specifically.
There was a problem hiding this comment.
Agreed, but it's not clear how to fix it.
The proposal we had was that the parser can keep track of lint levels for specific lints and we just lint in the parser.
We can also stash lints by span as suggested by @oli-obk
There was a problem hiding this comment.
(though that still requires some way of visiting them)
petrochenkov
added
S-waiting-on-author
Manishearth
added
S-waiting-on-team
|
☔ The latest upstream changes (presumably #50033) made this pull request unmergeable. Please resolve the merge conflicts. |
kennytm
added
the
T-compiler
|
How does this relate to #50307? |
|
moved to #50307 |
Implement edition hygiene for keywords Determine "keywordness" of an identifier in its hygienic context. cc #49611 I've resurrected `proc` as an Edition-2015-only keyword for testing purposes, but it should probably be buried again. EDIT: `proc` is removed again.
This implements edition-gated keywords as discussed in #49610 .
Currently only does async.