From 201a833eef3c3cf8beeab0461e3ffc772b07c83e Mon Sep 17 00:00:00 2001 From: Arlie Davis Date: Fri, 11 Dec 2020 11:32:48 -0800 Subject: [PATCH 1/2] Improve error handling in `symbols` proc-macro This improves how the `symbols` proc-macro handles errors. If it finds an error in its input, the macro does not panic. Instead, it still produces an output token stream. That token stream will contain `compile_error!(...)` macro invocations. This will still cause compilation to fail (which is what we want), but it will prevent meaningless errors caused by the output not containing symbols that the macro normally generates. This solves a small (but annoying) problem. When you're editing rustc_span/src/symbol.rs, and you get something wrong (dup symbol name, misordered symbol), you want to get only the errors that are relevant, not a burst of errors that are irrelevant. This change also uses the correct Span when reporting errors, so you get errors that point to the correct place in rustc_span/src/symbol.rs where something is wrong. This also adds several unit tests which test the `symbols` proc-macro. This commit also makes it easy to run the `symbols` proc-macro as an ordinary Cargo test. Just run `cargo test`. This makes it easier to do development on the macro itself, such as running it under a debugger. This commit also uses the `Punctuated` type in `syn` for parsing comma-separated lists, rather than doing it manually. The output of the macro is not changed at all by this commit, so rustc should be completely unchanged. This just improves quality of life during development. --- compiler/rustc_macros/src/lib.rs | 2 +- compiler/rustc_macros/src/symbols.rs | 154 ++++++++++++++------- compiler/rustc_macros/src/symbols/tests.rs | 111 +++++++++++++++ 3 files changed, 213 insertions(+), 54 deletions(-) create mode 100644 compiler/rustc_macros/src/symbols/tests.rs diff --git a/compiler/rustc_macros/src/lib.rs b/compiler/rustc_macros/src/lib.rs index 5c28839c9b7e4..152ae159aed44 100644 --- a/compiler/rustc_macros/src/lib.rs +++ b/compiler/rustc_macros/src/lib.rs @@ -21,7 +21,7 @@ pub fn rustc_queries(input: TokenStream) -> TokenStream { #[proc_macro] pub fn symbols(input: TokenStream) -> TokenStream { - symbols::symbols(input) + symbols::symbols(input.into()).into() } decl_derive!([HashStable, attributes(stable_hasher)] => hash_stable::hash_stable_derive); diff --git a/compiler/rustc_macros/src/symbols.rs b/compiler/rustc_macros/src/symbols.rs index 94d4ad78e8d90..f449900d5c245 100644 --- a/compiler/rustc_macros/src/symbols.rs +++ b/compiler/rustc_macros/src/symbols.rs @@ -1,8 +1,35 @@ -use proc_macro::TokenStream; +//! Proc macro which builds the Symbol table +//! +//! # Debugging +//! +//! Since this proc-macro does some non-trivial work, debugging it is important. +//! This proc-macro can be invoked as an ordinary unit test, like so: +//! +//! ```bash +//! cd compiler/rustc_macros +//! cargo test symbols::test_symbols -- --nocapture +//! ``` +//! +//! This unit test finds the `symbols!` invocation in `compiler/rustc_span/src/symbol.rs` +//! and runs it. It verifies that the output token stream can be parsed as valid module +//! items and that no errors were produced. +//! +//! You can also view the generated code by using `cargo expand`: +//! +//! ```bash +//! cargo install cargo-expand # this is necessary only once +//! cd compiler/rustc_span +//! cargo expand > /tmp/rustc_span.rs # it's a big file +//! ``` + +use proc_macro2::{Span, TokenStream}; use quote::quote; -use std::collections::HashSet; +use std::collections::HashMap; use syn::parse::{Parse, ParseStream, Result}; -use syn::{braced, parse_macro_input, Ident, LitStr, Token}; +use syn::{braced, punctuated::Punctuated, Ident, LitStr, Token}; + +#[cfg(test)] +mod tests; mod kw { syn::custom_keyword!(Keywords); @@ -19,7 +46,6 @@ impl Parse for Keyword { let name = input.parse()?; input.parse::()?; let value = input.parse()?; - input.parse::()?; Ok(Keyword { name, value }) } @@ -37,28 +63,14 @@ impl Parse for Symbol { Ok(_) => Some(input.parse()?), Err(_) => None, }; - input.parse::()?; Ok(Symbol { name, value }) } } -/// A type used to greedily parse another type until the input is empty. -struct List(Vec); - -impl Parse for List { - fn parse(input: ParseStream<'_>) -> Result { - let mut list = Vec::new(); - while !input.is_empty() { - list.push(input.parse()?); - } - Ok(List(list)) - } -} - struct Input { - keywords: List, - symbols: List, + keywords: Punctuated, + symbols: Punctuated, } impl Parse for Input { @@ -66,49 +78,86 @@ impl Parse for Input { input.parse::()?; let content; braced!(content in input); - let keywords = content.parse()?; + let keywords = Punctuated::parse_terminated(&content)?; input.parse::()?; let content; braced!(content in input); - let symbols = content.parse()?; + let symbols = Punctuated::parse_terminated(&content)?; Ok(Input { keywords, symbols }) } } +#[derive(Default)] +struct Errors { + list: Vec, +} + +impl Errors { + fn error(&mut self, span: Span, message: String) { + self.list.push(syn::Error::new(span, message)); + } +} + pub fn symbols(input: TokenStream) -> TokenStream { - let input = parse_macro_input!(input as Input); + let (mut output, errors) = symbols_with_errors(input); + + // If we generated any errors, then report them as compiler_error!() macro calls. + // This lets the errors point back to the most relevant span. It also allows us + // to report as many errors as we can during a single run. + output.extend(errors.into_iter().map(|e| e.to_compile_error())); + + output +} + +fn symbols_with_errors(input: TokenStream) -> (TokenStream, Vec) { + let mut errors = Errors::default(); + + let input: Input = match syn::parse2(input) { + Ok(input) => input, + Err(e) => { + // This allows us to display errors at the proper span, while minimizing + // unrelated errors caused by bailing out (and not generating code). + errors.list.push(e); + Input { keywords: Default::default(), symbols: Default::default() } + } + }; let mut keyword_stream = quote! {}; let mut symbols_stream = quote! {}; let mut digits_stream = quote! {}; let mut prefill_stream = quote! {}; let mut counter = 0u32; - let mut keys = HashSet::::new(); - let mut prev_key: Option = None; - let mut errors = Vec::::new(); - - let mut check_dup = |str: &str, errors: &mut Vec| { - if !keys.insert(str.to_string()) { - errors.push(format!("Symbol `{}` is duplicated", str)); + let mut keys = + HashMap::::with_capacity(input.keywords.len() + input.symbols.len() + 10); + let mut prev_key: Option<(Span, String)> = None; + + let mut check_dup = |span: Span, str: &str, errors: &mut Errors| { + if let Some(prev_span) = keys.get(str) { + errors.error(span, format!("Symbol `{}` is duplicated", str)); + errors.error(*prev_span, format!("location of previous definition")); + } else { + keys.insert(str.to_string(), span); } }; - let mut check_order = |str: &str, errors: &mut Vec| { - if let Some(ref prev_str) = prev_key { + let mut check_order = |span: Span, str: &str, errors: &mut Errors| { + if let Some((prev_span, ref prev_str)) = prev_key { if str < prev_str { - errors.push(format!("Symbol `{}` must precede `{}`", str, prev_str)); + errors.error(span, format!("Symbol `{}` must precede `{}`", str, prev_str)); + errors.error(prev_span, format!("location of previous symbol `{}`", prev_str)); } } - prev_key = Some(str.to_string()); + prev_key = Some((span, str.to_string())); }; // Generate the listed keywords. - for keyword in &input.keywords.0 { + for keyword in input.keywords.iter() { let name = &keyword.name; let value = &keyword.value; - check_dup(&value.value(), &mut errors); + let value_string = value.value(); + check_dup(keyword.name.span(), &value_string, &mut errors); prefill_stream.extend(quote! { #value, }); @@ -120,14 +169,15 @@ pub fn symbols(input: TokenStream) -> TokenStream { } // Generate the listed symbols. - for symbol in &input.symbols.0 { + for symbol in input.symbols.iter() { let name = &symbol.name; let value = match &symbol.value { Some(value) => value.value(), None => name.to_string(), }; - check_dup(&value, &mut errors); - check_order(&name.to_string(), &mut errors); + check_dup(symbol.name.span(), &value, &mut errors); + check_order(symbol.name.span(), &name.to_string(), &mut errors); + prefill_stream.extend(quote! { #value, }); @@ -142,7 +192,7 @@ pub fn symbols(input: TokenStream) -> TokenStream { // Generate symbols for the strings "0", "1", ..., "9". for n in 0..10 { let n = n.to_string(); - check_dup(&n, &mut errors); + check_dup(Span::call_site(), &n, &mut errors); prefill_stream.extend(quote! { #n, }); @@ -152,14 +202,7 @@ pub fn symbols(input: TokenStream) -> TokenStream { counter += 1; } - if !errors.is_empty() { - for error in errors.into_iter() { - eprintln!("error: {}", error) - } - panic!("errors in `Keywords` and/or `Symbols`"); - } - - let tt = TokenStream::from(quote! { + let output = quote! { macro_rules! keywords { () => { #keyword_stream @@ -184,11 +227,16 @@ pub fn symbols(input: TokenStream) -> TokenStream { ]) } } - }); + }; - // To see the generated code generated, uncomment this line, recompile, and - // run the resulting output through `rustfmt`. - //eprintln!("{}", tt); + (output, errors.list) - tt + // To see the generated code, use the "cargo expand" command. + // Do this once to install: + // cargo install cargo-expand + // + // Then, cd to rustc_span and run: + // cargo expand > /tmp/rustc_span_expanded.rs + // + // and read that file. } diff --git a/compiler/rustc_macros/src/symbols/tests.rs b/compiler/rustc_macros/src/symbols/tests.rs new file mode 100644 index 0000000000000..90a28b518020e --- /dev/null +++ b/compiler/rustc_macros/src/symbols/tests.rs @@ -0,0 +1,111 @@ +use super::*; + +// This test is mainly here for interactive development. Use this test while +// you're working on the proc-macro defined in this file. +#[test] +fn test_symbols() { + // We textually include the symbol.rs file, which contains the list of all + // symbols, keywords, and common words. Then we search for the + // `symbols! { ... }` call. + + static SYMBOL_RS_FILE: &str = include_str!("../../../rustc_span/src/symbol.rs"); + + let file = syn::parse_file(SYMBOL_RS_FILE).unwrap(); + let symbols_path: syn::Path = syn::parse_quote!(symbols); + + let m: &syn::ItemMacro = file + .items + .iter() + .filter_map(|i| { + if let syn::Item::Macro(m) = i { + if m.mac.path == symbols_path { Some(m) } else { None } + } else { + None + } + }) + .next() + .expect("did not find `symbols!` macro invocation."); + + let body_tokens = m.mac.tokens.clone(); + + test_symbols_macro(body_tokens, &[]); +} + +fn test_symbols_macro(input: TokenStream, expected_errors: &[&str]) { + let (output, found_errors) = symbols_with_errors(input); + + // It should always parse. + let _parsed_file = syn::parse2::(output).unwrap(); + + assert_eq!( + found_errors.len(), + expected_errors.len(), + "Macro generated a different number of errors than expected" + ); + + for (found_error, &expected_error) in found_errors.iter().zip(expected_errors.iter()) { + let found_error_str = format!("{}", found_error); + assert_eq!(found_error_str, expected_error); + } +} + +#[test] +fn check_dup_keywords() { + let input = quote! { + Keywords { + Crate: "crate", + Crate: "crate", + } + Symbols {} + }; + test_symbols_macro( + input, + &["Symbol `crate` is duplicated", "location of previous definition"], + ); +} + +#[test] +fn check_dup_symbol() { + let input = quote! { + Keywords {} + Symbols { + splat, + splat, + } + }; + test_symbols_macro( + input, + &["Symbol `splat` is duplicated", "location of previous definition"], + ); +} + +#[test] +fn check_dup_symbol_and_keyword() { + let input = quote! { + Keywords { + Splat: "splat", + } + Symbols { + splat, + } + }; + test_symbols_macro( + input, + &["Symbol `splat` is duplicated", "location of previous definition"], + ); +} + +#[test] +fn check_symbol_order() { + let input = quote! { + Keywords {} + Symbols { + zebra, + aardvark, + } + }; + test_symbols_macro( + input, + &["Symbol `aardvark` must precede `zebra`", "location of previous symbol `zebra`"], + ); +} From 1a5b9b037e0fb8cf204e44e604ff4ad09612b8b6 Mon Sep 17 00:00:00 2001 From: Arlie Davis Date: Sun, 13 Dec 2020 13:36:01 -0800 Subject: [PATCH 2/2] ./x.py fmt --- compiler/rustc_macros/src/symbols/tests.rs | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/compiler/rustc_macros/src/symbols/tests.rs b/compiler/rustc_macros/src/symbols/tests.rs index 90a28b518020e..82b4b876978f2 100644 --- a/compiler/rustc_macros/src/symbols/tests.rs +++ b/compiler/rustc_macros/src/symbols/tests.rs @@ -58,10 +58,7 @@ fn check_dup_keywords() { } Symbols {} }; - test_symbols_macro( - input, - &["Symbol `crate` is duplicated", "location of previous definition"], - ); + test_symbols_macro(input, &["Symbol `crate` is duplicated", "location of previous definition"]); } #[test] @@ -73,10 +70,7 @@ fn check_dup_symbol() { splat, } }; - test_symbols_macro( - input, - &["Symbol `splat` is duplicated", "location of previous definition"], - ); + test_symbols_macro(input, &["Symbol `splat` is duplicated", "location of previous definition"]); } #[test] @@ -89,10 +83,7 @@ fn check_dup_symbol_and_keyword() { splat, } }; - test_symbols_macro( - input, - &["Symbol `splat` is duplicated", "location of previous definition"], - ); + test_symbols_macro(input, &["Symbol `splat` is duplicated", "location of previous definition"]); } #[test]