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

Edition-gated keywords #49611

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/librustc/session/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1096,7 +1096,8 @@ pub fn build_session_(
};
let target_cfg = config::build_target_config(&sopts, &span_diagnostic);

let p_s = parse::ParseSess::with_span_handler(span_diagnostic, codemap);
let p_s = parse::ParseSess::with_span_handler(span_diagnostic, codemap,
sopts.debugging_opts.edition);
let default_sysroot = match sopts.maybe_sysroot {
Some(_) => None,
None => Some(filesearch::get_or_default_sysroot()),
Expand Down
8 changes: 4 additions & 4 deletions src/librustc_passes/ast_validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ use rustc::session::Session;
use syntax::ast::*;
use syntax::attr;
use syntax::codemap::Spanned;
use syntax::parse::token;
use syntax::symbol::keywords;
use syntax::visit::{self, Visitor};
use syntax_pos::Span;
Expand All @@ -40,14 +39,15 @@ impl<'a> AstValidator<'a> {
let valid_names = [keywords::UnderscoreLifetime.name(),
keywords::StaticLifetime.name(),
keywords::Invalid.name()];
if !valid_names.contains(&ident.name) &&
token::is_reserved_ident(ident.without_first_quote()) {
if !valid_names.contains(&ident.name)
&& ident.without_first_quote().is_reserved()
{
self.err_handler().span_err(ident.span, "lifetimes cannot use keyword names");
}
}

fn check_label(&self, ident: Ident) {
if token::is_reserved_ident(ident.without_first_quote()) {
if ident.without_first_quote().is_reserved() {
self.err_handler()
.span_err(ident.span, &format!("invalid label name `{}`", ident.name));
}
Expand Down
8 changes: 8 additions & 0 deletions src/libsyntax/edition.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

use std::fmt;
use std::str::FromStr;
use symbol::Symbol;

/// The edition of the compiler (RFC 2052)
#[derive(Clone, Copy, Hash, PartialOrd, Ord, Eq, PartialEq, Debug)]
Expand Down Expand Up @@ -56,6 +57,13 @@ impl Edition {
}
}

pub fn is_future_edition_keyword(&self, sym: Symbol) -> bool {
match *self {
Edition::Edition2015 => sym.is_future_edition_keyword_2015(),
Edition::Edition2018 => sym.is_future_edition_keyword_2018(),
}
}

pub fn feature_name(&self) -> &'static str {
match *self {
Edition::Edition2015 => "rust_2015_preview",
Expand Down
8 changes: 7 additions & 1 deletion src/libsyntax/parse/lexer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1117,7 +1117,7 @@ impl<'a> StringReader<'a> {
let c = self.ch;

if ident_start(c) {
let (is_ident_start, is_raw_ident) =
let (is_ident_start, mut is_raw_ident) =
match (c.unwrap(), self.nextch(), self.nextnextch()) {
// r# followed by an identifier starter is a raw identifier.
// This is an exception to the r# case below.
Expand Down Expand Up @@ -1155,9 +1155,13 @@ impl<'a> StringReader<'a> {
&format!("`r#{}` is not currently supported.", ident.name)
).raise();
}

if is_raw_ident {
let span = self.mk_sp(raw_start, self.pos);
self.sess.raw_identifier_spans.borrow_mut().push(span);
} else if self.sess.edition.is_future_edition_keyword(ident.name) {
// we pretend these are raw even if they aren't
is_raw_ident = true;
}
token::Ident(ident, is_raw_ident)
}));
Expand Down Expand Up @@ -1778,6 +1782,7 @@ mod tests {
use symbol::Symbol;
use syntax_pos::{BytePos, Span, NO_EXPANSION};
use codemap::CodeMap;
use edition::Edition;
use errors;
use feature_gate::UnstableFeatures;
use parse::token;
Expand All @@ -1802,6 +1807,7 @@ mod tests {
raw_identifier_spans: Lock::new(Vec::new()),
registered_diagnostics: Lock::new(ErrorMap::new()),
non_modrs_mods: Lock::new(vec![]),
edition: Edition::Edition2015,
}
}

Expand Down
8 changes: 6 additions & 2 deletions src/libsyntax/parse/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use rustc_data_structures::sync::{Lrc, Lock};
use ast::{self, CrateConfig};
use codemap::{CodeMap, FilePathMapping};
use syntax_pos::{self, Span, FileMap, NO_EXPANSION, FileName};
use edition::Edition;
use errors::{Handler, ColorConfig, DiagnosticBuilder};
use feature_gate::UnstableFeatures;
use parse::parser::Parser;
Expand Down Expand Up @@ -54,6 +55,7 @@ pub struct ParseSess {
// Spans where a `mod foo;` statement was included in a non-mod.rs file.
// These are used to issue errors if the non_modrs_mods feature is not enabled.
pub non_modrs_mods: Lock<Vec<(ast::Ident, Span)>>,
pub edition: Edition,
/// Used to determine and report recursive mod inclusions
included_mod_stack: Lock<Vec<PathBuf>>,
code_map: Lrc<CodeMap>,
Expand All @@ -66,10 +68,11 @@ impl ParseSess {
true,
false,
Some(cm.clone()));
ParseSess::with_span_handler(handler, cm)
ParseSess::with_span_handler(handler, cm, Edition::Edition2015)
}

pub fn with_span_handler(handler: Handler, code_map: Lrc<CodeMap>) -> ParseSess {
pub fn with_span_handler(handler: Handler, code_map: Lrc<CodeMap>,
edition: Edition) -> ParseSess {
ParseSess {
span_diagnostic: handler,
unstable_features: UnstableFeatures::from_environment(),
Expand All @@ -80,6 +83,7 @@ impl ParseSess {
included_mod_stack: Lock::new(vec![]),
code_map,
non_modrs_mods: Lock::new(vec![]),
edition,
}
}

Expand Down
31 changes: 5 additions & 26 deletions src/libsyntax/parse/token.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,28 +145,7 @@ pub fn is_path_segment_keyword(id: ast::Ident) -> bool {
// How was it written originally? Did it use the raw form? Let's try to guess.
pub fn is_raw_guess(ident: ast::Ident) -> bool {
ident.name != keywords::Invalid.name() &&
is_reserved_ident(ident) && !is_path_segment_keyword(ident)
}

// Returns true for reserved identifiers used internally for elided lifetimes,
// unnamed method parameters, crate root module, error recovery etc.
pub fn is_special_ident(id: ast::Ident) -> bool {
id.name <= keywords::Underscore.name()
}

/// Returns `true` if the token is a keyword used in the language.
pub fn is_used_keyword(id: ast::Ident) -> bool {
id.name >= keywords::As.name() && id.name <= keywords::While.name()
}

/// Returns `true` if the token is a keyword reserved for possible future use.
pub fn is_unused_keyword(id: ast::Ident) -> bool {
id.name >= keywords::Abstract.name() && id.name <= keywords::Yield.name()
}

/// Returns `true` if the token is either a special identifier or a keyword.
pub fn is_reserved_ident(id: ast::Ident) -> bool {
is_special_ident(id) || is_used_keyword(id) || is_unused_keyword(id)
ident.is_reserved() && !is_path_segment_keyword(ident)
}

#[derive(Clone, RustcEncodable, RustcDecodable, PartialEq, Eq, Hash, Debug)]
Expand Down Expand Up @@ -413,31 +392,31 @@ impl Token {
// unnamed method parameters, crate root module, error recovery etc.
pub fn is_special_ident(&self) -> bool {
match self.ident() {
Some((id, false)) => is_special_ident(id),
Some((id, false)) => id.is_special(),
_ => false,
}
}

/// Returns `true` if the token is a keyword used in the language.
pub fn is_used_keyword(&self) -> bool {
match self.ident() {
Some((id, false)) => is_used_keyword(id),
Some((id, false)) => id.is_used_keyword(),
_ => false,
}
}

/// Returns `true` if the token is a keyword reserved for possible future use.
pub fn is_unused_keyword(&self) -> bool {
match self.ident() {
Some((id, false)) => is_unused_keyword(id),
Some((id, false)) => id.is_unused_keyword(),
_ => false,
}
}

/// Returns `true` if the token is either a special identifier or a keyword.
pub fn is_reserved_ident(&self) -> bool {
match self.ident() {
Some((id, false)) => is_reserved_ident(id),
Some((id, false)) => id.is_reserved(),
_ => false,
}
}
Expand Down
70 changes: 61 additions & 9 deletions src/libsyntax_pos/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -278,8 +278,8 @@ macro_rules! declare_keywords {(
// NB: leaving holes in the ident table is bad! a different ident will get
// interned with the id from the hole, but it will be between the min and max
// of the reserved words, and thus tagged as "reserved".
// 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
Copy link
Contributor

Choose a reason for hiding this comment

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

*below

declare_keywords! {
// Special reserved identifiers used internally for elided lifetimes,
// unnamed method parameters, crate root module, error recovery etc.
Expand Down Expand Up @@ -325,6 +325,9 @@ declare_keywords! {
(37, Use, "use")
(38, Where, "where")
(39, While, "while")
// edition-gated used keywords go here
// be sure to update is_used_keyword and
// is_future_edition_keyword_* below

// Keywords reserved for future use.
(40, Abstract, "abstract")
Expand All @@ -343,17 +346,66 @@ declare_keywords! {
(53, Unsized, "unsized")
(54, Virtual, "virtual")
(55, Yield, "yield")
// edition-gated reserved keywords
// be sure to update is_unused_keyword and
// is_future_edition_keyword_* below
(56, Async, "async") // Rust 2018+ only

// Special lifetime names
(56, UnderscoreLifetime, "'_")
(57, StaticLifetime, "'static")
(57, UnderscoreLifetime, "'_")
(58, StaticLifetime, "'static")

// Weak keywords, have special meaning only in specific contexts.
(58, Auto, "auto")
(59, Catch, "catch")
(60, Default, "default")
(61, Dyn, "dyn")
(62, Union, "union")
(59, Auto, "auto")
(60, Catch, "catch")
(61, Default, "default")
(62, Dyn, "dyn")
(63, Union, "union")
}

impl Ident {
// Returns true for reserved identifiers used internally for elided lifetimes,
// unnamed method parameters, crate root module, error recovery etc.
#[inline]
pub fn is_special(self) -> bool {
self.name <= self::keywords::Underscore.name()
}

/// Returns `true` if the token is a keyword used in the language, for
/// at least one edition.
///
/// Keywords from future editions will be lexed as if they were raw identifiers
/// so they will not reach this step.
#[inline]
pub fn is_used_keyword(self) -> bool {
self.name >= self::keywords::As.name() && self.name <= self::keywords::While.name()
}

/// Returns `true` if the token is a keyword reserved for possible future use, for
/// at least one edition.
#[inline]
pub fn is_unused_keyword(self) -> bool {
self.name >= self::keywords::Abstract.name() && self.name <= self::keywords::Async.name()
}


/// Returns `true` if the token is either a special identifier or a keyword.
#[inline]
pub fn is_reserved(self) -> bool {
self.is_special() || self.is_used_keyword() || self.is_unused_keyword()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If these functions are moved to methods, then they should rather become methods of Symbol (aka Name) than Ident.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we let that be a mentored followup?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok.

}

impl Symbol {
#[inline]
pub fn is_future_edition_keyword_2015(self) -> bool {
self == self::keywords::Async.name()
}

#[inline]
pub fn is_future_edition_keyword_2018(self) -> bool {
false
}
}

// If an interner exists, return it. Otherwise, prepare a fresh one.
Expand Down
17 changes: 17 additions & 0 deletions src/test/compile-fail/auxiliary/edition-kw-macro-2015.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// compile-flags: -Zedition=2015 -Zunstable-options

#[macro_export]
macro_rules! consumes_async {
(async) => (1)
}

22 changes: 22 additions & 0 deletions src/test/compile-fail/edition-keywords-2018.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// compile-flags: -Zedition=2018 -Zunstable-options
// aux-build:edition-kw-macro-2015.rs

#![feature(raw_identifiers)]

#[macro_use]
extern crate edition_kw_macro_2015;

pub fn main() {
let _ = consumes_async!(async);
//~^ ERROR no rules expected the token `async`
Copy link
Contributor

@petrochenkov petrochenkov Apr 12, 2018

Choose a reason for hiding this comment

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

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_keyword involving 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@petrochenkov petrochenkov Apr 12, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@petrochenkov petrochenkov Apr 26, 2018

Choose a reason for hiding this comment

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

Regarding reporting "really everything", I think this is more of a problem with lint infrastructure rather than with edition keywords specifically.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

(though that still requires some way of visiting them)

}
18 changes: 18 additions & 0 deletions src/test/compile-fail/edition-kw.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// compile-flags: -Zedition=2018 -Zunstable-options

#![allow(unused)]

pub fn main() {
let async = 1;
//~^ ERROR expected pattern, found reserved keyword `async`
}
37 changes: 37 additions & 0 deletions src/test/run-pass/auxiliary/edition-kw-macro-2015.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// compile-flags: -Zedition=2015 -Zunstable-options
#![feature(raw_identifiers)]

#[macro_export]
macro_rules! produces_async {
() => (pub fn async() {})
}

#[macro_export]
macro_rules! produces_async_raw {
() => (pub fn r#async() {})
}

#[macro_export]
macro_rules! consumes_async {
(async) => (1)
}

#[macro_export]
macro_rules! consumes_async_raw {
(r#async) => (1)
}

#[macro_export]
macro_rules! consumes_ident {
($i:ident) => ($i)
}
Loading