From 136f9b1a54cdaaf7e1035e83dee2773c565a69a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Krasnoborski?= Date: Fri, 2 Feb 2018 16:07:29 +0100 Subject: [PATCH 1/8] Warn when `$name:matcher` syntax is used on expansion side --- src/libsyntax/ext/tt/quoted.rs | 88 ++++++++++++++++++++++++---------- 1 file changed, 63 insertions(+), 25 deletions(-) diff --git a/src/libsyntax/ext/tt/quoted.rs b/src/libsyntax/ext/tt/quoted.rs index c55dfaba8f6b2..d955fad6ac40e 100644 --- a/src/libsyntax/ext/tt/quoted.rs +++ b/src/libsyntax/ext/tt/quoted.rs @@ -190,31 +190,33 @@ pub fn parse( // Given the parsed tree, if there is a metavar and we are expecting matchers, actually // parse out the matcher (i.e. in `$id:ident` this would parse the `:` and `ident`). match tree { - TokenTree::MetaVar(start_sp, ident) if expect_matchers => { - let span = match trees.next() { - Some(tokenstream::TokenTree::Token(span, token::Colon)) => match trees.next() { - Some(tokenstream::TokenTree::Token(end_sp, ref tok)) => match tok.ident() { - Some(kind) => { - let span = end_sp.with_lo(start_sp.lo()); - result.push(TokenTree::MetaVarDecl(span, ident, kind)); - continue; - } - _ => end_sp, - }, - tree => tree.as_ref() - .map(tokenstream::TokenTree::span) - .unwrap_or(span), - }, - tree => tree.as_ref() - .map(tokenstream::TokenTree::span) - .unwrap_or(start_sp), - }; - sess.missing_fragment_specifiers.borrow_mut().insert(span); - result.push(TokenTree::MetaVarDecl( - span, - ident, - keywords::Invalid.ident(), - )); + TokenTree::MetaVar(start_sp, ident) => { + if expect_matchers { + match parse_matcher(start_sp, ident, &mut trees) { + Ok((meta_var_decl, _, _)) => result.push(meta_var_decl), + Err(span) => { + sess.missing_fragment_specifiers.borrow_mut().insert(span); + result.push(TokenTree::MetaVarDecl( + span, + ident, + keywords::Invalid.ident(), + )); + } + } + } else { + if let Ok((_, colon_sp, end_sp)) = parse_matcher(start_sp, ident, &mut trees.clone()) { + if start_sp.hi() == colon_sp.lo() && colon_sp.hi() == end_sp.lo() { + sess.span_diagnostic + .struct_span_warn( + colon_sp.with_hi(end_sp.hi()), + "matchers are treated literally on the expansion side" + ) + .help("if this is what you want, insert spaces to silence this warning") + .emit(); + } + } + result.push(tree) + } } // Not a metavar or no matchers allowed, so just return the tree @@ -224,6 +226,42 @@ pub fn parse( result } +/// Parses a `:`. Doesn't report error on its own. +/// +/// # Parameters +/// +/// - `start_sp` - span of metavariable name preceding the `:` +/// - `ident` - the name of metavariable +/// - `trees` - iterator over trees +/// +/// # Returns +/// +/// On success, returns a parsed `MetaVarDecl`, span of `:` and span of matcher's type +/// On error, returns a span for error. +fn parse_matcher(start_sp: Span, ident: ast::Ident, trees: &mut I) -> Result<(TokenTree, Span, Span), Span> +where + I: Iterator +{ + match trees.next() { + Some(tokenstream::TokenTree::Token(colon_sp, token::Colon)) => match trees.next() { + Some(tokenstream::TokenTree::Token(end_sp, ref tok)) => match tok.ident() { + Some(kind) => { + let span = end_sp.with_lo(start_sp.lo()); + Ok((TokenTree::MetaVarDecl(span, ident, kind), colon_sp, end_sp)) + } + _ => Err(end_sp), + }, + tree => { + let span = tree.as_ref() + .map(tokenstream::TokenTree::span) + .unwrap_or(colon_sp); + Err(span) + } + }, + tree => Err(tree.as_ref().map(tokenstream::TokenTree::span).unwrap_or(start_sp)) + } +} + /// Takes a `tokenstream::TokenTree` and returns a `self::TokenTree`. Specifically, this takes a /// generic `TokenTree`, such as is used in the rest of the compiler, and returns a `TokenTree` /// for use in parsing a macro. From 48df6bfc3b00556b15e9faea8f80c59e526d97e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Krasnoborski?= Date: Mon, 5 Feb 2018 22:06:49 +0100 Subject: [PATCH 2/8] comply to tidy --- src/libsyntax/ext/tt/quoted.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/libsyntax/ext/tt/quoted.rs b/src/libsyntax/ext/tt/quoted.rs index d955fad6ac40e..32a0410d24e62 100644 --- a/src/libsyntax/ext/tt/quoted.rs +++ b/src/libsyntax/ext/tt/quoted.rs @@ -204,16 +204,19 @@ pub fn parse( } } } else { - if let Ok((_, colon_sp, end_sp)) = parse_matcher(start_sp, ident, &mut trees.clone()) { - if start_sp.hi() == colon_sp.lo() && colon_sp.hi() == end_sp.lo() { + match parse_matcher(start_sp, ident, &mut trees.clone()) { + Ok((_, colon_sp, end_sp)) + if start_sp.hi() == colon_sp.lo() && colon_sp.hi() == end_sp.lo() => { sess.span_diagnostic .struct_span_warn( colon_sp.with_hi(end_sp.hi()), "matchers are treated literally on the expansion side" ) - .help("if this is what you want, insert spaces to silence this warning") + .help("if this is what you want, \ + insert spaces to silence this warning") .emit(); } + _ => {} } result.push(tree) } @@ -238,7 +241,11 @@ pub fn parse( /// /// On success, returns a parsed `MetaVarDecl`, span of `:` and span of matcher's type /// On error, returns a span for error. -fn parse_matcher(start_sp: Span, ident: ast::Ident, trees: &mut I) -> Result<(TokenTree, Span, Span), Span> +fn parse_matcher( + start_sp: Span, + ident: ast::Ident, + trees: &mut I +) -> Result<(TokenTree, Span, Span), Span> where I: Iterator { From 13f2a9d7f5ebb0d8311ba01f2cb47e8aa188a9b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Krasnoborski?= Date: Mon, 5 Feb 2018 22:48:25 +0100 Subject: [PATCH 3/8] silence the lint in the codebase --- src/libcore/fmt/mod.rs | 2 +- src/libcore/tuple.rs | 10 +++++----- src/libserialize/serialize.rs | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/libcore/fmt/mod.rs b/src/libcore/fmt/mod.rs index 65aacb23bd768..08c49002b5579 100644 --- a/src/libcore/fmt/mod.rs +++ b/src/libcore/fmt/mod.rs @@ -1716,7 +1716,7 @@ macro_rules! tuple { () => (); ( $($name:ident,)+ ) => ( #[stable(feature = "rust1", since = "1.0.0")] - impl<$($name:Debug),*> Debug for ($($name,)*) where last_type!($($name,)+): ?Sized { + impl<$($name: Debug),*> Debug for ($($name,)*) where last_type!($($name,)+): ?Sized { #[allow(non_snake_case, unused_assignments, deprecated)] fn fmt(&self, f: &mut Formatter) -> Result { let mut builder = f.debug_tuple(""); diff --git a/src/libcore/tuple.rs b/src/libcore/tuple.rs index 4c5370194fecb..f151cdd77a0d6 100644 --- a/src/libcore/tuple.rs +++ b/src/libcore/tuple.rs @@ -22,7 +22,7 @@ macro_rules! tuple_impls { )+) => { $( #[stable(feature = "rust1", since = "1.0.0")] - impl<$($T:PartialEq),+> PartialEq for ($($T,)+) where last_type!($($T,)+): ?Sized { + impl<$($T: PartialEq),+> PartialEq for ($($T,)+) where last_type!($($T,)+): ?Sized { #[inline] fn eq(&self, other: &($($T,)+)) -> bool { $(self.$idx == other.$idx)&&+ @@ -34,10 +34,10 @@ macro_rules! tuple_impls { } #[stable(feature = "rust1", since = "1.0.0")] - impl<$($T:Eq),+> Eq for ($($T,)+) where last_type!($($T,)+): ?Sized {} + impl<$($T: Eq),+> Eq for ($($T,)+) where last_type!($($T,)+): ?Sized {} #[stable(feature = "rust1", since = "1.0.0")] - impl<$($T:PartialOrd + PartialEq),+> PartialOrd for ($($T,)+) + impl<$($T: PartialOrd + PartialEq),+> PartialOrd for ($($T,)+) where last_type!($($T,)+): ?Sized { #[inline] fn partial_cmp(&self, other: &($($T,)+)) -> Option { @@ -62,7 +62,7 @@ macro_rules! tuple_impls { } #[stable(feature = "rust1", since = "1.0.0")] - impl<$($T:Ord),+> Ord for ($($T,)+) where last_type!($($T,)+): ?Sized { + impl<$($T: Ord),+> Ord for ($($T,)+) where last_type!($($T,)+): ?Sized { #[inline] fn cmp(&self, other: &($($T,)+)) -> Ordering { lexical_cmp!($(self.$idx, other.$idx),+) @@ -70,7 +70,7 @@ macro_rules! tuple_impls { } #[stable(feature = "rust1", since = "1.0.0")] - impl<$($T:Default),+> Default for ($($T,)+) { + impl<$($T: Default),+> Default for ($($T,)+) { #[inline] fn default() -> ($($T,)+) { ($({ let x: $T = Default::default(); x},)+) diff --git a/src/libserialize/serialize.rs b/src/libserialize/serialize.rs index 854ca4eb3b03b..fa60b11b1cb6b 100644 --- a/src/libserialize/serialize.rs +++ b/src/libserialize/serialize.rs @@ -679,7 +679,7 @@ macro_rules! count_idents { macro_rules! tuple { () => (); ( $($name:ident,)+ ) => ( - impl<$($name:Decodable),*> Decodable for ($($name,)*) { + impl<$($name: Decodable),*> Decodable for ($($name,)*) { #[allow(non_snake_case)] fn decode(d: &mut D) -> Result<($($name,)*), D::Error> { let len: usize = count_idents!($($name,)*); @@ -693,7 +693,7 @@ macro_rules! tuple { }) } } - impl<$($name:Encodable),*> Encodable for ($($name,)*) { + impl<$($name: Encodable),*> Encodable for ($($name,)*) { #[allow(non_snake_case)] fn encode(&self, s: &mut S) -> Result<(), S::Error> { let ($(ref $name,)*) = *self; From 41d9228c46a0ccecefe9d5208cc1c9a1bc5f23f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Krasnoborski?= Date: Sun, 25 Feb 2018 02:23:42 +0100 Subject: [PATCH 4/8] warn only on valid specifiers --- src/libsyntax/ext/tt/quoted.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/libsyntax/ext/tt/quoted.rs b/src/libsyntax/ext/tt/quoted.rs index 32a0410d24e62..7e9ecac3d5a4f 100644 --- a/src/libsyntax/ext/tt/quoted.rs +++ b/src/libsyntax/ext/tt/quoted.rs @@ -204,9 +204,15 @@ pub fn parse( } } } else { + const VALID_SPECIFIERS: &[&str] = &[ + "ident", "block", "stmt", "expr", "pat", "ty", + "path", "meta", "tt", "item", "vis" + ]; match parse_matcher(start_sp, ident, &mut trees.clone()) { - Ok((_, colon_sp, end_sp)) - if start_sp.hi() == colon_sp.lo() && colon_sp.hi() == end_sp.lo() => { + Ok((TokenTree::MetaVarDecl(_, _, kind), colon_sp, end_sp)) + if start_sp.hi() == colon_sp.lo() + && colon_sp.hi() == end_sp.lo() + && VALID_SPECIFIERS.contains(&&*kind.name.as_str()) => { sess.span_diagnostic .struct_span_warn( colon_sp.with_hi(end_sp.hi()), From 838fc5fc072ff60ff623908c329bfcee87cf1eb8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Krasnoborski?= Date: Sun, 25 Feb 2018 16:05:40 +0100 Subject: [PATCH 5/8] modify warning message and use span_suggestion --- src/libsyntax/ext/tt/quoted.rs | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/libsyntax/ext/tt/quoted.rs b/src/libsyntax/ext/tt/quoted.rs index 7e9ecac3d5a4f..c03cefd8c7c5c 100644 --- a/src/libsyntax/ext/tt/quoted.rs +++ b/src/libsyntax/ext/tt/quoted.rs @@ -209,17 +209,26 @@ pub fn parse( "path", "meta", "tt", "item", "vis" ]; match parse_matcher(start_sp, ident, &mut trees.clone()) { - Ok((TokenTree::MetaVarDecl(_, _, kind), colon_sp, end_sp)) + Ok((TokenTree::MetaVarDecl(full_sp, _, kind), colon_sp, end_sp)) if start_sp.hi() == colon_sp.lo() && colon_sp.hi() == end_sp.lo() && VALID_SPECIFIERS.contains(&&*kind.name.as_str()) => { + let specifier_sp = colon_sp.with_hi(end_sp.hi()); sess.span_diagnostic .struct_span_warn( - colon_sp.with_hi(end_sp.hi()), - "matchers are treated literally on the expansion side" + specifier_sp, + "macro expansion includes fragment specifier" + ) + .span_suggestion( + full_sp, + "to just use the macro argument, remove the fragment specifier", + format!("${}", ident) + ) + .span_suggestion( + specifier_sp, + "to suppress this warning, add a space after the colon", + format!(": {}", kind) ) - .help("if this is what you want, \ - insert spaces to silence this warning") .emit(); } _ => {} From ea01e9eb7049b47876dbfb9d4b75c506df1bfc64 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Krasnoborski?= Date: Sun, 25 Feb 2018 15:51:51 +0100 Subject: [PATCH 6/8] Add a test --- .../ui/fragment_specifier_in_expansion.rs | 30 +++++++++++++++++++ .../ui/fragment_specifier_in_expansion.stderr | 14 +++++++++ 2 files changed, 44 insertions(+) create mode 100644 src/test/ui/fragment_specifier_in_expansion.rs create mode 100644 src/test/ui/fragment_specifier_in_expansion.stderr diff --git a/src/test/ui/fragment_specifier_in_expansion.rs b/src/test/ui/fragment_specifier_in_expansion.rs new file mode 100644 index 0000000000000..2fa111a6874a3 --- /dev/null +++ b/src/test/ui/fragment_specifier_in_expansion.rs @@ -0,0 +1,30 @@ +// 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 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Tests the "fragment specifier in expansion" warning + +// must-compile-successfully + +#![allow(unused)] + +macro_rules! warn_me { + ( $thing:tt ) => { $thing:tt } + //~^ WARN fragment specifier +} + +macro_rules! with_space { + ( $thing:tt ) => { $thing: tt } +} + +macro_rules! unknown_specifier { + ( $thing:tt ) => { $thing:foobar } +} + +fn main() {} diff --git a/src/test/ui/fragment_specifier_in_expansion.stderr b/src/test/ui/fragment_specifier_in_expansion.stderr new file mode 100644 index 0000000000000..9eda3d5e5d9ae --- /dev/null +++ b/src/test/ui/fragment_specifier_in_expansion.stderr @@ -0,0 +1,14 @@ +warning: macro expansion includes fragment specifier + --> $DIR/fragment_specifier_in_expansion.rs:18:30 + | +18 | ( $thing:tt ) => { $thing:tt } + | ^^^ +help: to just use the macro argument, remove the fragment specifier + | +18 | ( $thing:tt ) => { $thing } + | ^^^^^^ +help: to suppress this warning, add a space after the colon + | +18 | ( $thing:tt ) => { $thing: tt } + | ^^^^ + From bab97b2cf7471bf7431a41f2bb82046397e9913b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Krasnoborski?= Date: Sun, 25 Feb 2018 19:22:41 +0100 Subject: [PATCH 7/8] silence the macro in tests using nested macros --- src/test/compile-fail/hygiene/globs.rs | 2 +- src/test/compile-fail/hygiene/nested_macro_privacy.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/compile-fail/hygiene/globs.rs b/src/test/compile-fail/hygiene/globs.rs index 7ba217061c66e..a67f0d72e0366 100644 --- a/src/test/compile-fail/hygiene/globs.rs +++ b/src/test/compile-fail/hygiene/globs.rs @@ -47,7 +47,7 @@ macro n($i:ident) { } } - macro n($j:ident) { + macro n($j: ident) { mod test { use super::*; fn g() { diff --git a/src/test/compile-fail/hygiene/nested_macro_privacy.rs b/src/test/compile-fail/hygiene/nested_macro_privacy.rs index 6612359649c19..2ded2e81def70 100644 --- a/src/test/compile-fail/hygiene/nested_macro_privacy.rs +++ b/src/test/compile-fail/hygiene/nested_macro_privacy.rs @@ -14,7 +14,7 @@ macro n($foo:ident, $S:ident, $i:ident, $m:ident) { mod $foo { #[derive(Default)] pub struct $S { $i: u32 } - pub macro $m($e:expr) { $e.$i } + pub macro $m($e: expr) { $e.$i } } } From 36453f6aec8f6e76180fee70c7d4d22707159a5d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Krasnoborski?= Date: Sun, 25 Feb 2018 19:22:53 +0100 Subject: [PATCH 8/8] rustfmt --- src/libsyntax/ext/tt/quoted.rs | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/src/libsyntax/ext/tt/quoted.rs b/src/libsyntax/ext/tt/quoted.rs index c03cefd8c7c5c..e44f6f0733a43 100644 --- a/src/libsyntax/ext/tt/quoted.rs +++ b/src/libsyntax/ext/tt/quoted.rs @@ -205,29 +205,29 @@ pub fn parse( } } else { const VALID_SPECIFIERS: &[&str] = &[ - "ident", "block", "stmt", "expr", "pat", "ty", - "path", "meta", "tt", "item", "vis" + "ident", "block", "stmt", "expr", "pat", "ty", "path", "meta", "tt", + "item", "vis", ]; match parse_matcher(start_sp, ident, &mut trees.clone()) { Ok((TokenTree::MetaVarDecl(full_sp, _, kind), colon_sp, end_sp)) - if start_sp.hi() == colon_sp.lo() - && colon_sp.hi() == end_sp.lo() - && VALID_SPECIFIERS.contains(&&*kind.name.as_str()) => { + if start_sp.hi() == colon_sp.lo() && colon_sp.hi() == end_sp.lo() + && VALID_SPECIFIERS.contains(&&*kind.name.as_str()) => + { let specifier_sp = colon_sp.with_hi(end_sp.hi()); sess.span_diagnostic .struct_span_warn( specifier_sp, - "macro expansion includes fragment specifier" + "macro expansion includes fragment specifier", ) .span_suggestion( full_sp, "to just use the macro argument, remove the fragment specifier", - format!("${}", ident) + format!("${}", ident), ) .span_suggestion( specifier_sp, "to suppress this warning, add a space after the colon", - format!(": {}", kind) + format!(": {}", kind), ) .emit(); } @@ -259,10 +259,10 @@ pub fn parse( fn parse_matcher( start_sp: Span, ident: ast::Ident, - trees: &mut I + trees: &mut I, ) -> Result<(TokenTree, Span, Span), Span> where - I: Iterator + I: Iterator, { match trees.next() { Some(tokenstream::TokenTree::Token(colon_sp, token::Colon)) => match trees.next() { @@ -280,7 +280,9 @@ where Err(span) } }, - tree => Err(tree.as_ref().map(tokenstream::TokenTree::span).unwrap_or(start_sp)) + tree => Err(tree.as_ref() + .map(tokenstream::TokenTree::span) + .unwrap_or(start_sp)), } }