From ef1251f696b8e8d306e6a1a902502e8d75420118 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Sat, 9 Oct 2021 15:23:55 +0300 Subject: [PATCH] feat: report errors in macro definition Reporting macro *definition* error at the macro *call site* is a rather questionable approach, but at least we don't erase the errors altogether! --- .../hir_def/src/macro_expansion_tests/mbe.rs | 1 + .../macro_expansion_tests/mbe/meta_syntax.rs | 77 +++++++++++++++++++ .../mbe/tt_conversion.rs | 2 +- crates/hir_expand/src/db.rs | 59 +++++++------- crates/hir_expand/src/hygiene.rs | 2 +- crates/hir_expand/src/lib.rs | 2 +- crates/mbe/src/lib.rs | 13 +++- crates/mbe/src/subtree_source.rs | 11 --- crates/mbe/src/tests.rs | 1 - crates/mbe/src/tests/rule.rs | 48 ------------ 10 files changed, 121 insertions(+), 95 deletions(-) create mode 100644 crates/hir_def/src/macro_expansion_tests/mbe/meta_syntax.rs delete mode 100644 crates/mbe/src/tests/rule.rs diff --git a/crates/hir_def/src/macro_expansion_tests/mbe.rs b/crates/hir_def/src/macro_expansion_tests/mbe.rs index 27c1bfebd1f..50499840a3a 100644 --- a/crates/hir_def/src/macro_expansion_tests/mbe.rs +++ b/crates/hir_def/src/macro_expansion_tests/mbe.rs @@ -1,5 +1,6 @@ mod tt_conversion; mod matching; +mod meta_syntax; use expect_test::expect; diff --git a/crates/hir_def/src/macro_expansion_tests/mbe/meta_syntax.rs b/crates/hir_def/src/macro_expansion_tests/mbe/meta_syntax.rs new file mode 100644 index 00000000000..1c4bb9d7b84 --- /dev/null +++ b/crates/hir_def/src/macro_expansion_tests/mbe/meta_syntax.rs @@ -0,0 +1,77 @@ +use expect_test::expect; + +use crate::macro_expansion_tests::check; + +#[test] +fn well_formed_macro_rules() { + check( + r#" +macro_rules! m { + ($i:ident) => (); + ($(x),*) => (); + ($(x)_*) => (); + ($(x)i*) => (); + ($($i:ident)*) => ($_); + ($($true:ident)*) => ($true); + ($($false:ident)*) => ($false); + ($) => ($); +} +m!($); +"#, + expect![[r#" +macro_rules! m { + ($i:ident) => (); + ($(x),*) => (); + ($(x)_*) => (); + ($(x)i*) => (); + ($($i:ident)*) => ($_); + ($($true:ident)*) => ($true); + ($($false:ident)*) => ($false); + ($) => ($); +} +$ +"#]], + ) +} + +#[test] +fn malformed_macro_rules() { + check( + r#" +macro_rules! i1 { invalid } +i1!(); + +macro_rules! e1 { $i:ident => () } +e1!(); +macro_rules! e2 { ($i:ident) () } +e2!(); +macro_rules! e3 { ($(i:ident)_) => () } +e3!(); + +macro_rules! f1 { ($i) => ($i) } +f1!(); +macro_rules! f2 { ($i:) => ($i) } +f2!(); +macro_rules! f3 { ($i:_) => () } +f3!(); +"#, + expect![[r#" +macro_rules! i1 { invalid } +/* error: invalid macro definition: expected subtree */ + +macro_rules! e1 { $i:ident => () } +/* error: invalid macro definition: expected subtree */ +macro_rules! e2 { ($i:ident) () } +/* error: invalid macro definition: expected `=` */ +macro_rules! e3 { ($(i:ident)_) => () } +/* error: invalid macro definition: invalid repeat */ + +macro_rules! f1 { ($i) => ($i) } +/* error: invalid macro definition: bad fragment specifier 1 */ +macro_rules! f2 { ($i:) => ($i) } +/* error: invalid macro definition: bad fragment specifier 1 */ +macro_rules! f3 { ($i:_) => () } +/* error: invalid macro definition: bad fragment specifier 1 */ +"#]], + ) +} diff --git a/crates/hir_def/src/macro_expansion_tests/mbe/tt_conversion.rs b/crates/hir_def/src/macro_expansion_tests/mbe/tt_conversion.rs index 6059fcdb904..d445d7a2c61 100644 --- a/crates/hir_def/src/macro_expansion_tests/mbe/tt_conversion.rs +++ b/crates/hir_def/src/macro_expansion_tests/mbe/tt_conversion.rs @@ -72,7 +72,7 @@ macro_rules! m2 { ($x:ident) => {} } macro_rules! m1 { ($x:ident) => { ($x } } macro_rules! m2 { ($x:ident) => {} } -/* error: Failed to find macro definition */ +/* error: invalid macro definition: expected subtree */ /* error: Failed to lower macro args to token tree */ "#]], ) diff --git a/crates/hir_expand/src/db.rs b/crates/hir_expand/src/db.rs index fd1d8d2e60b..08ad9ffd948 100644 --- a/crates/hir_expand/src/db.rs +++ b/crates/hir_expand/src/db.rs @@ -8,7 +8,7 @@ use rustc_hash::FxHashSet; use syntax::{ algo::diff, - ast::{self, HasAttrs, HasName}, + ast::{self, HasAttrs}, AstNode, GreenNode, Parse, SyntaxNode, SyntaxToken, T, }; @@ -119,7 +119,7 @@ fn parse_macro_expansion( fn macro_arg_text(&self, id: MacroCallId) -> Option; /// Gets the expander for this macro. This compiles declarative macros, and /// just fetches procedural ones. - fn macro_def(&self, id: MacroDefId) -> Option>; + fn macro_def(&self, id: MacroDefId) -> Result, mbe::ParseError>; /// Expand macro call to a token tree. This query is LRUed (we keep 128 or so results in memory) fn macro_expand(&self, macro_call: MacroCallId) -> ExpandResult>>; @@ -145,7 +145,7 @@ pub fn expand_speculative( token_to_map: SyntaxToken, ) -> Option<(SyntaxNode, SyntaxToken)> { let loc = db.lookup_intern_macro(actual_macro_call); - let macro_def = db.macro_def(loc.def)?; + let macro_def = db.macro_def(loc.def).ok()?; let token_range = token_to_map.text_range(); // Build the subtree and token mapping for the speculative args @@ -360,45 +360,39 @@ fn macro_arg_text(db: &dyn AstDatabase, id: MacroCallId) -> Option { Some(arg.green().into()) } -fn macro_def(db: &dyn AstDatabase, id: MacroDefId) -> Option> { +fn macro_def(db: &dyn AstDatabase, id: MacroDefId) -> Result, mbe::ParseError> { match id.kind { MacroDefKind::Declarative(ast_id) => match ast_id.to_node(db) { ast::Macro::MacroRules(macro_rules) => { - let arg = macro_rules.token_tree()?; + let arg = macro_rules + .token_tree() + .ok_or_else(|| mbe::ParseError::Expected("expected a token tree".into()))?; let (tt, def_site_token_map) = mbe::syntax_node_to_token_tree(arg.syntax()); - let mac = match mbe::MacroRules::parse(&tt) { - Ok(it) => it, - Err(err) => { - let name = macro_rules.name().map(|n| n.to_string()).unwrap_or_default(); - tracing::warn!("fail on macro_def parse ({}): {:?} {:#?}", name, err, tt); - return None; - } - }; - Some(Arc::new(TokenExpander::MacroRules { mac, def_site_token_map })) + let mac = mbe::MacroRules::parse(&tt)?; + Ok(Arc::new(TokenExpander::MacroRules { mac, def_site_token_map })) } ast::Macro::MacroDef(macro_def) => { - let arg = macro_def.body()?; + let arg = macro_def + .body() + .ok_or_else(|| mbe::ParseError::Expected("expected a token tree".into()))?; let (tt, def_site_token_map) = mbe::syntax_node_to_token_tree(arg.syntax()); - let mac = match mbe::MacroDef::parse(&tt) { - Ok(it) => it, - Err(err) => { - let name = macro_def.name().map(|n| n.to_string()).unwrap_or_default(); - tracing::warn!("fail on macro_def parse ({}): {:?} {:#?}", name, err, tt); - return None; - } - }; - Some(Arc::new(TokenExpander::MacroDef { mac, def_site_token_map })) + let mac = mbe::MacroDef::parse(&tt)?; + Ok(Arc::new(TokenExpander::MacroDef { mac, def_site_token_map })) } }, - MacroDefKind::BuiltIn(expander, _) => Some(Arc::new(TokenExpander::Builtin(expander))), + MacroDefKind::BuiltIn(expander, _) => Ok(Arc::new(TokenExpander::Builtin(expander))), MacroDefKind::BuiltInAttr(expander, _) => { - Some(Arc::new(TokenExpander::BuiltinAttr(expander))) + Ok(Arc::new(TokenExpander::BuiltinAttr(expander))) } MacroDefKind::BuiltInDerive(expander, _) => { - Some(Arc::new(TokenExpander::BuiltinDerive(expander))) + Ok(Arc::new(TokenExpander::BuiltinDerive(expander))) } - MacroDefKind::BuiltInEager(..) => None, - MacroDefKind::ProcMacro(expander, ..) => Some(Arc::new(TokenExpander::ProcMacro(expander))), + MacroDefKind::BuiltInEager(..) => { + // FIXME: Return a random error here just to make the types align. + // This obviously should do something real instead. + Err(mbe::ParseError::UnexpectedToken("unexpected eager macro".to_string())) + } + MacroDefKind::ProcMacro(expander, ..) => Ok(Arc::new(TokenExpander::ProcMacro(expander))), } } @@ -419,8 +413,11 @@ fn macro_expand(db: &dyn AstDatabase, id: MacroCallId) -> ExpandResult it, - None => return ExpandResult::str_err("Failed to find macro definition".into()), + Ok(it) => it, + // FIXME: This is weird -- we effectively report macro *definition* + // errors lazily, when we try to expand the macro. Instead, they should + // be reported at the definition site (when we construct a def map). + Err(err) => return ExpandResult::str_err(format!("invalid macro definition: {}", err)), }; let ExpandResult { value: tt, err } = expander.expand(db, id, ¯o_arg.0); // Set a hard limit for the expanded tt diff --git a/crates/hir_expand/src/hygiene.rs b/crates/hir_expand/src/hygiene.rs index 5b3ccdeb605..b2879e37c35 100644 --- a/crates/hir_expand/src/hygiene.rs +++ b/crates/hir_expand/src/hygiene.rs @@ -195,7 +195,7 @@ fn make_hygiene_info( _ => None, }); - let macro_def = db.macro_def(loc.def)?; + let macro_def = db.macro_def(loc.def).ok()?; let (_, exp_map) = db.parse_macro_expansion(macro_file).value?; let macro_arg = db.macro_arg(macro_file.macro_call_id)?; diff --git a/crates/hir_expand/src/lib.rs b/crates/hir_expand/src/lib.rs index ba4ef8b70fe..ad6b84dd17e 100644 --- a/crates/hir_expand/src/lib.rs +++ b/crates/hir_expand/src/lib.rs @@ -143,7 +143,7 @@ pub fn expansion_info(self, db: &dyn db::AstDatabase) -> Option { _ => None, }); - let macro_def = db.macro_def(loc.def)?; + let macro_def = db.macro_def(loc.def).ok()?; let (parse, exp_map) = db.parse_macro_expansion(macro_file).value?; let macro_arg = db.macro_arg(macro_file.macro_call_id)?; diff --git a/crates/mbe/src/lib.rs b/crates/mbe/src/lib.rs index bc407e4171b..61e032e0afa 100644 --- a/crates/mbe/src/lib.rs +++ b/crates/mbe/src/lib.rs @@ -30,7 +30,7 @@ pub use ::parser::ParserEntryPoint; pub use tt::{Delimiter, DelimiterKind, Punct}; -#[derive(Debug, PartialEq, Eq)] +#[derive(Debug, PartialEq, Eq, Clone)] pub enum ParseError { UnexpectedToken(String), Expected(String), @@ -38,6 +38,17 @@ pub enum ParseError { RepetitionEmptyTokenTree, } +impl fmt::Display for ParseError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + ParseError::UnexpectedToken(it) => f.write_str(it), + ParseError::Expected(it) => f.write_str(it), + ParseError::InvalidRepeat => f.write_str("invalid repeat"), + ParseError::RepetitionEmptyTokenTree => f.write_str("empty token tree in repetition"), + } + } +} + #[derive(Debug, PartialEq, Eq, Clone)] pub enum ExpandError { NoMatchingRule, diff --git a/crates/mbe/src/subtree_source.rs b/crates/mbe/src/subtree_source.rs index 0f852d33d5b..247616aa1b6 100644 --- a/crates/mbe/src/subtree_source.rs +++ b/crates/mbe/src/subtree_source.rs @@ -17,17 +17,6 @@ pub(crate) struct SubtreeTokenSource { curr: (Token, usize), } -impl<'a> SubtreeTokenSource { - // Helper function used in test - #[cfg(test)] - pub(crate) fn text(&self) -> SmolStr { - match self.cached.get(self.curr.1) { - Some(tt) => tt.text.clone(), - _ => SmolStr::new(""), - } - } -} - impl<'a> SubtreeTokenSource { pub(crate) fn new(buffer: &TokenBuffer) -> SubtreeTokenSource { let mut current = buffer.begin(); diff --git a/crates/mbe/src/tests.rs b/crates/mbe/src/tests.rs index 26924fa8f3e..ad5ae3a32c2 100644 --- a/crates/mbe/src/tests.rs +++ b/crates/mbe/src/tests.rs @@ -1,5 +1,4 @@ mod expand; -mod rule; use std::{fmt::Write, iter}; diff --git a/crates/mbe/src/tests/rule.rs b/crates/mbe/src/tests/rule.rs deleted file mode 100644 index 691e359e4d7..00000000000 --- a/crates/mbe/src/tests/rule.rs +++ /dev/null @@ -1,48 +0,0 @@ -use syntax::{ast, AstNode}; - -use super::*; - -#[test] -fn test_valid_arms() { - fn check(macro_body: &str) { - let m = parse_macro_arm(macro_body); - m.unwrap(); - } - - check("($i:ident) => ()"); - check("($(x),*) => ()"); - check("($(x)_*) => ()"); - check("($(x)i*) => ()"); - check("($($i:ident)*) => ($_)"); - check("($($true:ident)*) => ($true)"); - check("($($false:ident)*) => ($false)"); - check("($) => ($)"); -} - -#[test] -fn test_invalid_arms() { - fn check(macro_body: &str, err: ParseError) { - let m = parse_macro_arm(macro_body); - assert_eq!(m, Err(err)); - } - check("invalid", ParseError::Expected("expected subtree".into())); - - check("$i:ident => ()", ParseError::Expected("expected subtree".into())); - check("($i:ident) ()", ParseError::Expected("expected `=`".into())); - check("($($i:ident)_) => ()", ParseError::InvalidRepeat); - - check("($i) => ($i)", ParseError::UnexpectedToken("bad fragment specifier 1".into())); - check("($i:) => ($i)", ParseError::UnexpectedToken("bad fragment specifier 1".into())); - check("($i:_) => ()", ParseError::UnexpectedToken("bad fragment specifier 1".into())); -} - -fn parse_macro_arm(arm_definition: &str) -> Result { - let macro_definition = format!(" macro_rules! m {{ {} }} ", arm_definition); - let source_file = ast::SourceFile::parse(¯o_definition).ok().unwrap(); - let macro_definition = - source_file.syntax().descendants().find_map(ast::MacroRules::cast).unwrap(); - - let (definition_tt, _) = - syntax_node_to_token_tree(macro_definition.token_tree().unwrap().syntax()); - crate::MacroRules::parse(&definition_tt) -} -- 2.44.0