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

Modify message for keyword as identifier name #46497

Merged
merged 6 commits into from
Dec 7, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
29 changes: 23 additions & 6 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::symbol::keywords;
use syntax::visit::{self, Visitor};
use syntax_pos::Span;
use errors;
Expand All @@ -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() &&
Copy link
Contributor

@petrochenkov petrochenkov Dec 6, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@petrochenkov petrochenkov Dec 6, 2017

Choose a reason for hiding this comment

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

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 '_.

!lifetime.ident.name.is_static_keyword() {
self.err_handler().span_err(lifetime.span, "lifetimes cannot use keyword names");
}
}

fn check_label(&self, label: Ident, span: Span) {
if label.name == keywords::StaticLifetime.name() || label.name == "'_" {
if label.name.is_static_keyword() || !label.without_first_quote().is_valid()
|| label.name == "'_" {
self.err_handler().span_err(span, &format!("invalid label name `{}`", label.name));
}
}
Expand Down Expand Up @@ -202,19 +209,21 @@ impl<'a> Visitor<'a> for AstValidator<'a> {

fn visit_item(&mut self, item: &'a Item) {
match item.node {
ItemKind::Impl(.., Some(..), _, ref impl_items) => {
ItemKind::Impl(.., ref generics, Some(..), _, ref impl_items) => {
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))
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually a good first task because the visitor is a quite important piece of infrastructure and it makes you familiar with it.

}
ItemKind::Impl(.., None, _, _) => {
ItemKind::Impl(.., ref generics, None, _, _) => {
self.invalid_visibility(&item.vis,
item.span,
Some("place qualifiers on individual impl items instead"));
generics.lifetimes.iter().for_each(|l| self.check_lifetime(&l.lifetime))
}
ItemKind::AutoImpl(..) => {
self.invalid_visibility(&item.vis, item.span, None);
Expand All @@ -225,13 +234,14 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
Some("place qualifiers on individual foreign items \
instead"));
}
ItemKind::Enum(ref def, _) => {
ItemKind::Enum(ref def, ref generics) => {
for variant in &def.variants {
self.invalid_non_exhaustive_attribute(variant);
for field in variant.node.data.fields() {
self.invalid_visibility(&field.vis, field.span, None);
}
}
generics.lifetimes.iter().for_each(|l| self.check_lifetime(&l.lifetime))
}
ItemKind::Trait(is_auto, _, ref generics, ref bounds, ref trait_items) => {
if is_auto == IsAuto::Yes {
Expand Down Expand Up @@ -268,6 +278,7 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
}
}
}
generics.lifetimes.iter().for_each(|l| self.check_lifetime(&l.lifetime))
}
ItemKind::Mod(_) => {
// Ensure that `path` attributes on modules are recorded as used (c.f. #35584).
Expand All @@ -278,7 +289,7 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
self.session.buffer_lint(lint, item.id, item.span, msg);
}
}
ItemKind::Union(ref vdata, _) => {
ItemKind::Union(ref vdata, ref generics) => {
if !vdata.is_struct() {
self.err_handler().span_err(item.span,
"tuple and unit unions are not permitted");
Expand All @@ -287,6 +298,12 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
self.err_handler().span_err(item.span,
"unions cannot have zero fields");
}
generics.lifetimes.iter().for_each(|l| self.check_lifetime(&l.lifetime))
}
ItemKind::Fn(.., ref generics, _) |
ItemKind::Ty(_, ref generics) |
ItemKind::Struct(_, ref generics) => {
generics.lifetimes.iter().for_each(|l| self.check_lifetime(&l.lifetime))
}
_ => {}
}
Expand Down
14 changes: 1 addition & 13 deletions src/libsyntax/parse/lexer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use codemap::{CodeMap, FilePathMapping};
use errors::{FatalError, DiagnosticBuilder};
use parse::{token, ParseSess};
use str::char_at;
use symbol::{Symbol, keywords};
use symbol::{Symbol};
use std_unicode::property::Pattern_White_Space;

use std::borrow::Cow;
Expand Down Expand Up @@ -1296,18 +1296,6 @@ impl<'a> StringReader<'a> {
self.mk_ident(&format!("'{}", lifetime_name))
});

// Conjure up a "keyword checking ident" to make sure that
// the lifetime name is not a keyword.
let keyword_checking_ident = self.with_str_from(start, |lifetime_name| {
self.mk_ident(lifetime_name)
});
let keyword_checking_token = &token::Ident(keyword_checking_ident);
let last_bpos = self.pos;
if keyword_checking_token.is_reserved_ident() &&
!keyword_checking_token.is_keyword(keywords::Static) {
self.err_span_(start, last_bpos, "lifetimes cannot use keyword names");
}

return Ok(token::Lifetime(ident));
}

Expand Down
10 changes: 2 additions & 8 deletions src/libsyntax/parse/token.rs
Original file line number Diff line number Diff line change
Expand Up @@ -364,18 +364,12 @@ impl Token {

/// Returns `true` if the token is a keyword used in the language.
pub fn is_used_keyword(&self) -> bool {
match self.ident() {
Some(id) => id.name >= keywords::As.name() && id.name <= keywords::While.name(),
_ => false,
}
self.ident().map(|id| id.name.is_used_keyword()).unwrap_or(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) => id.name >= keywords::Abstract.name() && id.name <= keywords::Yield.name(),
_ => false,
}
self.ident().map(|id| id.name.is_unused_keyword()).unwrap_or(false)
}

pub fn glue(self, joint: Token) -> Option<Token> {
Expand Down
52 changes: 52 additions & 0 deletions src/libsyntax_pos/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,17 @@ impl Ident {
Ident::with_empty_ctxt(Symbol::intern(string))
}

pub fn without_first_quote(&self) -> Ident {
Ident { name: self.name.without_first_quote(), ctxt: self.ctxt }
}

pub fn modern(self) -> Ident {
Ident { name: self.name, ctxt: self.ctxt.modern() }
}

pub fn is_valid(&self) -> bool {
!self.name.is_used_keyword() && !self.name.is_unused_keyword()
}
}

impl fmt::Debug for Ident {
Expand Down Expand Up @@ -113,6 +121,24 @@ impl Symbol {
pub fn as_u32(self) -> u32 {
self.0
}

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

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

pub fn is_static_keyword(&self) -> bool {
self == &keywords::StaticLifetime.name()
}

pub fn without_first_quote(&self) -> Symbol {
Symbol::from(self.as_str().trim_left_matches('\''))
}
}

impl<'a> From<&'a str> for Symbol {
Expand Down Expand Up @@ -428,4 +454,30 @@ mod tests {
// gensym of *existing* string gets new number:
assert_eq!(i.gensym("dog"), Symbol(4294967293));
}

#[test]
fn is_used_keyword_test() {
let s = Symbol(5);
assert_eq!(s.is_used_keyword(), true);
}

#[test]
fn is_unused_keyword_test() {
let s = Symbol(40);
assert_eq!(s.is_unused_keyword(), true);
}

#[test]
fn is_valid_test() {
let i = Ident { name: Symbol(40), ctxt: SyntaxContext(0) };
assert_eq!(i.is_valid(), false);
let i = Ident { name: Symbol(61), ctxt: SyntaxContext(0) };
assert_eq!(i.is_valid(), true);
}

#[test]
fn without_first_quote_test() {
let i = Ident::from_str("'break");
assert_eq!(i.without_first_quote().name, keywords::Break.name());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,18 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// compile-flags: -Z parse-only -Z continue-parse-after-error


trait Serializable<'self, T> { //~ ERROR lifetimes cannot use keyword names
fn serialize(val : &'self T) -> Vec<u8> ; //~ ERROR lifetimes cannot use keyword names
fn deserialize(repr : &[u8]) -> &'self T; //~ ERROR lifetimes cannot use keyword names
fn serialize(val : &'self T) -> Vec<u8> ;
fn deserialize(repr : &[u8]) -> &'self T;
}

impl<'self> Serializable<str> for &'self str { //~ ERROR lifetimes cannot use keyword names
impl<'self> Serializable<str> for &'self str {
//~^ ERROR lifetimes cannot use keyword names
fn serialize(val : &'self str) -> Vec<u8> { //~ ERROR lifetimes cannot use keyword names
//~| ERROR missing lifetime specifier
fn serialize(val : &'self str) -> Vec<u8> {
vec![1]
}
fn deserialize(repr: &[u8]) -> &'self str { //~ ERROR lifetimes cannot use keyword names
fn deserialize(repr: &[u8]) -> &'self str {
"hi"
}
}
Expand Down
14 changes: 14 additions & 0 deletions src/test/compile-fail/issue-46311.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Copyright 2012-2014 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.

fn main() {
'break: loop { //~ ERROR invalid label name `'break`
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,9 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// compile-flags: -Z parse-only -Z continue-parse-after-error

fn foo<'a>(a: &'a isize) { }
fn bar(a: &'static isize) { }
fn baz(a: &'let isize) { } //~ ERROR lifetimes cannot use keyword names
fn zab(a: &'self isize) { } //~ ERROR lifetimes cannot use keyword names
fn baz<'let>(a: &'let isize) { } //~ ERROR lifetimes cannot use keyword names
fn zab<'self>(a: &'self isize) { } //~ ERROR lifetimes cannot use keyword names

fn main() { }