From b973158aeb337041d4e1434cf5d8c609a0b02bef Mon Sep 17 00:00:00 2001 From: Florian Diebold Date: Fri, 13 Mar 2020 13:03:31 +0100 Subject: Make MBE expansion more resilient (WIP) --- crates/ra_hir_expand/src/db.rs | 90 +++++++++++++++------------ crates/ra_hir_ty/src/tests/macros.rs | 8 +-- crates/ra_ide/src/completion/complete_dot.rs | 37 +++++++++++ crates/ra_ide/src/expand_macro.rs | 2 +- crates/ra_mbe/src/lib.rs | 4 +- crates/ra_mbe/src/mbe_expander.rs | 29 ++++++--- crates/ra_mbe/src/mbe_expander/matcher.rs | 11 ++-- crates/ra_mbe/src/mbe_expander/transcriber.rs | 69 +++++++++++--------- crates/ra_mbe/src/tests.rs | 3 +- crates/ra_tt/src/lib.rs | 6 ++ 10 files changed, 168 insertions(+), 91 deletions(-) (limited to 'crates') diff --git a/crates/ra_hir_expand/src/db.rs b/crates/ra_hir_expand/src/db.rs index c3e1c68b7..e7b81a1e6 100644 --- a/crates/ra_hir_expand/src/db.rs +++ b/crates/ra_hir_expand/src/db.rs @@ -27,11 +27,12 @@ impl TokenExpander { db: &dyn AstDatabase, id: LazyMacroId, tt: &tt::Subtree, - ) -> Result { + ) -> mbe::ExpandResult { match self { TokenExpander::MacroRules(it) => it.expand(tt), - TokenExpander::Builtin(it) => it.expand(db, id, tt), - TokenExpander::BuiltinDerive(it) => it.expand(db, id, tt), + // FIXME switch these to ExpandResult as well + TokenExpander::Builtin(it) => it.expand(db, id, tt).map_or_else(|e| (tt::Subtree::default(), Some(e)), |r| (r, None)), + TokenExpander::BuiltinDerive(it) => it.expand(db, id, tt).map_or_else(|e| (tt::Subtree::default(), Some(e)), |r| (r, None)), } } @@ -66,7 +67,7 @@ pub trait AstDatabase: SourceDatabase { fn macro_def(&self, id: MacroDefId) -> Option>; fn parse_macro(&self, macro_file: MacroFile) -> Option<(Parse, Arc)>; - fn macro_expand(&self, macro_call: MacroCallId) -> Result, String>; + fn macro_expand(&self, macro_call: MacroCallId) -> (Option>, Option); #[salsa::interned] fn intern_eager_expansion(&self, eager: EagerCallLoc) -> EagerMacroId; @@ -153,7 +154,7 @@ pub(crate) fn macro_arg( pub(crate) fn macro_expand( db: &dyn AstDatabase, id: MacroCallId, -) -> Result, String> { +) -> (Option>, Option) { macro_expand_with_arg(db, id, None) } @@ -174,31 +175,39 @@ fn macro_expand_with_arg( db: &dyn AstDatabase, id: MacroCallId, arg: Option>, -) -> Result, String> { +) -> (Option>, Option) { let lazy_id = match id { MacroCallId::LazyMacro(id) => id, MacroCallId::EagerMacro(id) => { if arg.is_some() { - return Err( - "hypothetical macro expansion not implemented for eager macro".to_owned() + return ( + None, + Some("hypothetical macro expansion not implemented for eager macro".to_owned()) ); } else { - return Ok(db.lookup_intern_eager_expansion(id).subtree); + return (Some(db.lookup_intern_eager_expansion(id).subtree), None); } } }; let loc = db.lookup_intern_macro(lazy_id); - let macro_arg = arg.or_else(|| db.macro_arg(id)).ok_or("Fail to args in to tt::TokenTree")?; + let macro_arg = match arg.or_else(|| db.macro_arg(id)) { + Some(it) => it, + None => return (None, Some("Fail to args in to tt::TokenTree".into())), + }; - let macro_rules = db.macro_def(loc.def).ok_or("Fail to find macro definition")?; - let tt = macro_rules.0.expand(db, lazy_id, ¯o_arg.0).map_err(|err| format!("{:?}", err))?; + let macro_rules = match db.macro_def(loc.def) { + Some(it) => it, + None => return (None, Some("Fail to find macro definition".into())), + }; + let (tt, err) = macro_rules.0.expand(db, lazy_id, ¯o_arg.0); // Set a hard limit for the expanded tt + eprintln!("expansion size: {}", tt.count()); let count = tt.count(); if count > 65536 { - return Err(format!("Total tokens count exceed limit : count = {}", count)); + return (None, Some(format!("Total tokens count exceed limit : count = {}", count))); } - Ok(Arc::new(tt)) + (Some(Arc::new(tt)), err.map(|e| format!("{:?}", e))) } pub(crate) fn parse_or_expand(db: &dyn AstDatabase, file_id: HirFileId) -> Option { @@ -225,42 +234,41 @@ pub fn parse_macro_with_arg( let _p = profile("parse_macro_query"); let macro_call_id = macro_file.macro_call_id; - let expansion = if let Some(arg) = arg { + let (tt, err) = if let Some(arg) = arg { macro_expand_with_arg(db, macro_call_id, Some(arg)) } else { db.macro_expand(macro_call_id) }; - let tt = expansion - .map_err(|err| { - // Note: - // The final goal we would like to make all parse_macro success, - // such that the following log will not call anyway. - match macro_call_id { - MacroCallId::LazyMacro(id) => { - let loc: MacroCallLoc = db.lookup_intern_macro(id); - let node = loc.kind.node(db); - - // collect parent information for warning log - let parents = std::iter::successors(loc.kind.file_id().call_node(db), |it| { - it.file_id.call_node(db) - }) + if let Some(err) = err { + // Note: + // The final goal we would like to make all parse_macro success, + // such that the following log will not call anyway. + match macro_call_id { + MacroCallId::LazyMacro(id) => { + let loc: MacroCallLoc = db.lookup_intern_macro(id); + let node = loc.kind.node(db); + + // collect parent information for warning log + let parents = std::iter::successors(loc.kind.file_id().call_node(db), |it| { + it.file_id.call_node(db) + }) .map(|n| format!("{:#}", n.value)) .collect::>() .join("\n"); - log::warn!( - "fail on macro_parse: (reason: {} macro_call: {:#}) parents: {}", - err, - node.value, - parents - ); - } - _ => { - log::warn!("fail on macro_parse: (reason: {})", err); - } + log::warn!( + "fail on macro_parse: (reason: {} macro_call: {:#}) parents: {}", + err, + node.value, + parents + ); + } + _ => { + log::warn!("fail on macro_parse: (reason: {})", err); } - }) - .ok()?; + } + }; + let tt = tt?; let fragment_kind = to_fragment_kind(db, macro_call_id); diff --git a/crates/ra_hir_ty/src/tests/macros.rs b/crates/ra_hir_ty/src/tests/macros.rs index 3b7022ad5..2e309a379 100644 --- a/crates/ra_hir_ty/src/tests/macros.rs +++ b/crates/ra_hir_ty/src/tests/macros.rs @@ -462,7 +462,7 @@ fn main() { fn infer_builtin_macros_include() { let (db, pos) = TestDB::with_position( r#" -//- /main.rs +//- /main.rs #[rustc_builtin_macro] macro_rules! include {() => {}} @@ -483,7 +483,7 @@ fn bar() -> u32 {0} fn infer_builtin_macros_include_concat() { let (db, pos) = TestDB::with_position( r#" -//- /main.rs +//- /main.rs #[rustc_builtin_macro] macro_rules! include {() => {}} @@ -507,7 +507,7 @@ fn bar() -> u32 {0} fn infer_builtin_macros_include_concat_with_bad_env_should_failed() { let (db, pos) = TestDB::with_position( r#" -//- /main.rs +//- /main.rs #[rustc_builtin_macro] macro_rules! include {() => {}} @@ -534,7 +534,7 @@ fn bar() -> u32 {0} fn infer_builtin_macros_include_itself_should_failed() { let (db, pos) = TestDB::with_position( r#" -//- /main.rs +//- /main.rs #[rustc_builtin_macro] macro_rules! include {() => {}} diff --git a/crates/ra_ide/src/completion/complete_dot.rs b/crates/ra_ide/src/completion/complete_dot.rs index f07611d88..a30d1c2de 100644 --- a/crates/ra_ide/src/completion/complete_dot.rs +++ b/crates/ra_ide/src/completion/complete_dot.rs @@ -751,6 +751,43 @@ mod tests { ); } + #[test] + fn macro_expansion_resilient() { + assert_debug_snapshot!( + do_ref_completion( + r" + macro_rules! dbg { + () => {}; + ($val:expr) => { + match $val { tmp => { tmp } } + }; + // Trailing comma with single argument is ignored + ($val:expr,) => { $crate::dbg!($val) }; + ($($val:expr),+ $(,)?) => { + ($($crate::dbg!($val)),+,) + }; + } + struct A { the_field: u32 } + fn foo(a: A) { + dbg!(a.<|>) + } + ", + ), + @r###" + [ + CompletionItem { + label: "the_field", + source_range: [552; 553), + delete: [552; 553), + insert: "the_field", + kind: Field, + detail: "u32", + }, + ] + "### + ); + } + #[test] fn test_method_completion_3547() { assert_debug_snapshot!( diff --git a/crates/ra_ide/src/expand_macro.rs b/crates/ra_ide/src/expand_macro.rs index f6667cb33..e58526f31 100644 --- a/crates/ra_ide/src/expand_macro.rs +++ b/crates/ra_ide/src/expand_macro.rs @@ -259,7 +259,7 @@ fn some_thing() -> u32 { ); assert_eq!(res.name, "foo"); - assert_snapshot!(res.expansion, @r###"bar!()"###); + assert_snapshot!(res.expansion, @r###""###); } #[test] diff --git a/crates/ra_mbe/src/lib.rs b/crates/ra_mbe/src/lib.rs index 43afe24cc..3adec4978 100644 --- a/crates/ra_mbe/src/lib.rs +++ b/crates/ra_mbe/src/lib.rs @@ -30,6 +30,8 @@ pub enum ExpandError { InvalidRepeat, } +pub type ExpandResult = (T, Option); + pub use crate::syntax_bridge::{ ast_to_token_tree, parse_to_token_tree, syntax_node_to_token_tree, token_tree_to_syntax_node, TokenMap, @@ -150,7 +152,7 @@ impl MacroRules { Ok(MacroRules { rules, shift: Shift::new(tt) }) } - pub fn expand(&self, tt: &tt::Subtree) -> Result { + pub fn expand(&self, tt: &tt::Subtree) -> ExpandResult { // apply shift let mut tt = tt.clone(); self.shift.shift_all(&mut tt); diff --git a/crates/ra_mbe/src/mbe_expander.rs b/crates/ra_mbe/src/mbe_expander.rs index b455b7321..da3952428 100644 --- a/crates/ra_mbe/src/mbe_expander.rs +++ b/crates/ra_mbe/src/mbe_expander.rs @@ -8,19 +8,30 @@ mod transcriber; use ra_syntax::SmolStr; use rustc_hash::FxHashMap; -use crate::ExpandError; +use crate::{ExpandResult, ExpandError}; pub(crate) fn expand( rules: &crate::MacroRules, input: &tt::Subtree, -) -> Result { - rules.rules.iter().find_map(|it| expand_rule(it, input).ok()).ok_or(ExpandError::NoMatchingRule) +) -> ExpandResult { + let (mut result, mut err) = (tt::Subtree::default(), Some(ExpandError::NoMatchingRule)); + for rule in &rules.rules { + let (res, e) = expand_rule(rule, input); + if e.is_none() { + // if we find a rule that applies without errors, we're done + return (res, None); + } + // TODO decide which result is better + result = res; + err = e; + } + (result, err) } -fn expand_rule(rule: &crate::Rule, input: &tt::Subtree) -> Result { - let bindings = matcher::match_(&rule.lhs, input)?; - let res = transcriber::transcribe(&rule.rhs, &bindings)?; - Ok(res) +fn expand_rule(rule: &crate::Rule, input: &tt::Subtree) -> ExpandResult { + let (bindings, bindings_err) = dbg!(matcher::match_(&rule.lhs, input)); + let (res, transcribe_err) = dbg!(transcriber::transcribe(&rule.rhs, &bindings)); + (res, bindings_err.or(transcribe_err)) } /// The actual algorithm for expansion is not too hard, but is pretty tricky. @@ -111,7 +122,7 @@ mod tests { } fn assert_err(macro_body: &str, invocation: &str, err: ExpandError) { - assert_eq!(expand_first(&create_rules(&format_macro(macro_body)), invocation), Err(err)); + assert_eq!(expand_first(&create_rules(&format_macro(macro_body)), invocation).1, Some(err)); } fn format_macro(macro_body: &str) -> String { @@ -138,7 +149,7 @@ mod tests { fn expand_first( rules: &crate::MacroRules, invocation: &str, - ) -> Result { + ) -> ExpandResult { let source_file = ast::SourceFile::parse(invocation).ok().unwrap(); let macro_invocation = source_file.syntax().descendants().find_map(ast::MacroCall::cast).unwrap(); diff --git a/crates/ra_mbe/src/mbe_expander/matcher.rs b/crates/ra_mbe/src/mbe_expander/matcher.rs index 49c53183a..f9d4952c6 100644 --- a/crates/ra_mbe/src/mbe_expander/matcher.rs +++ b/crates/ra_mbe/src/mbe_expander/matcher.rs @@ -11,6 +11,7 @@ use crate::{ use ra_parser::{FragmentKind::*, TreeSink}; use ra_syntax::{SmolStr, SyntaxKind}; use tt::buffer::{Cursor, TokenBuffer}; +use super::ExpandResult; impl Bindings { fn push_optional(&mut self, name: &SmolStr) { @@ -64,19 +65,19 @@ macro_rules! bail { }; } -pub(super) fn match_(pattern: &tt::Subtree, src: &tt::Subtree) -> Result { +pub(super) fn match_(pattern: &tt::Subtree, src: &tt::Subtree) -> ExpandResult { assert!(pattern.delimiter == None); let mut res = Bindings::default(); let mut src = TtIter::new(src); - match_subtree(&mut res, pattern, &mut src)?; + let mut err = match_subtree(&mut res, pattern, &mut src).err(); - if src.len() > 0 { - bail!("leftover tokens"); + if src.len() > 0 && err.is_none() { + err = Some(err!("leftover tokens")); } - Ok(res) + (res, err) } fn match_subtree( diff --git a/crates/ra_mbe/src/mbe_expander/transcriber.rs b/crates/ra_mbe/src/mbe_expander/transcriber.rs index 7662020f3..c53c2d35e 100644 --- a/crates/ra_mbe/src/mbe_expander/transcriber.rs +++ b/crates/ra_mbe/src/mbe_expander/transcriber.rs @@ -3,6 +3,7 @@ use ra_syntax::SmolStr; +use super::ExpandResult; use crate::{ mbe_expander::{Binding, Bindings, Fragment}, parser::{parse_template, Op, RepeatKind, Separator}, @@ -49,10 +50,7 @@ impl Bindings { } } -pub(super) fn transcribe( - template: &tt::Subtree, - bindings: &Bindings, -) -> Result { +pub(super) fn transcribe(template: &tt::Subtree, bindings: &Bindings) -> ExpandResult { assert!(template.delimiter == None); let mut ctx = ExpandCtx { bindings: &bindings, nesting: Vec::new() }; expand_subtree(&mut ctx, template) @@ -75,35 +73,46 @@ struct ExpandCtx<'a> { nesting: Vec, } -fn expand_subtree(ctx: &mut ExpandCtx, template: &tt::Subtree) -> Result { +fn expand_subtree(ctx: &mut ExpandCtx, template: &tt::Subtree) -> ExpandResult { let mut buf: Vec = Vec::new(); + let mut err = None; for op in parse_template(template) { - match op? { + let op = match op { + Ok(op) => op, + Err(e) => { + err = Some(e); + break; + } + }; + match op { Op::TokenTree(tt @ tt::TokenTree::Leaf(..)) => buf.push(tt.clone()), Op::TokenTree(tt::TokenTree::Subtree(tt)) => { - let tt = expand_subtree(ctx, tt)?; + let (tt, e) = expand_subtree(ctx, tt); + err = err.or(e); buf.push(tt.into()); } Op::Var { name, kind: _ } => { - let fragment = expand_var(ctx, name)?; + let (fragment, e) = expand_var(ctx, name); + err = err.or(e); push_fragment(&mut buf, fragment); } Op::Repeat { subtree, kind, separator } => { - let fragment = expand_repeat(ctx, subtree, kind, separator)?; + let (fragment, e) = expand_repeat(ctx, subtree, kind, separator); + err = err.or(e); push_fragment(&mut buf, fragment) } } } - Ok(tt::Subtree { delimiter: template.delimiter, token_trees: buf }) + (tt::Subtree { delimiter: template.delimiter, token_trees: buf }, err) } -fn expand_var(ctx: &mut ExpandCtx, v: &SmolStr) -> Result { - let res = if v == "crate" { +fn expand_var(ctx: &mut ExpandCtx, v: &SmolStr) -> ExpandResult { + if v == "crate" { // We simply produce identifier `$crate` here. And it will be resolved when lowering ast to Path. let tt = tt::Leaf::from(tt::Ident { text: "$crate".into(), id: tt::TokenId::unspecified() }) .into(); - Fragment::Tokens(tt) + (Fragment::Tokens(tt), None) } else if !ctx.bindings.contains(v) { // Note that it is possible to have a `$var` inside a macro which is not bound. // For example: @@ -132,11 +141,13 @@ fn expand_var(ctx: &mut ExpandCtx, v: &SmolStr) -> Result ], } .into(); - Fragment::Tokens(tt) + (Fragment::Tokens(tt), None) } else { - ctx.bindings.get(&v, &mut ctx.nesting)?.clone() - }; - Ok(res) + ctx.bindings.get(&v, &mut ctx.nesting).map_or_else( + |e| (Fragment::Tokens(tt::TokenTree::empty()), Some(e)), + |b| (b.clone(), None), + ) + } } fn expand_repeat( @@ -144,17 +155,17 @@ fn expand_repeat( template: &tt::Subtree, kind: RepeatKind, separator: Option, -) -> Result { +) -> ExpandResult { let mut buf: Vec = Vec::new(); ctx.nesting.push(NestingState { idx: 0, at_end: false, hit: false }); // Dirty hack to make macro-expansion terminate. - // This should be replaced by a propper macro-by-example implementation + // This should be replaced by a proper macro-by-example implementation let limit = 65536; let mut has_seps = 0; let mut counter = 0; loop { - let res = expand_subtree(ctx, template); + let (mut t, e) = expand_subtree(ctx, template); let nesting_state = ctx.nesting.last_mut().unwrap(); if nesting_state.at_end || !nesting_state.hit { break; @@ -172,10 +183,10 @@ fn expand_repeat( break; } - let mut t = match res { - Ok(t) => t, - Err(_) => continue, - }; + if e.is_some() { + continue; + } + t.delimiter = None; push_subtree(&mut buf, t); @@ -209,14 +220,14 @@ fn expand_repeat( buf.pop(); } - if RepeatKind::OneOrMore == kind && counter == 0 { - return Err(ExpandError::UnexpectedToken); - } - // Check if it is a single token subtree without any delimiter // e.g {Delimiter:None> ['>'] /Delimiter:None>} let tt = tt::Subtree { delimiter: None, token_trees: buf }.into(); - Ok(Fragment::Tokens(tt)) + + if RepeatKind::OneOrMore == kind && counter == 0 { + return (Fragment::Tokens(tt), Some(ExpandError::UnexpectedToken)); + } + (Fragment::Tokens(tt), None) } fn push_fragment(buf: &mut Vec, fragment: Fragment) { diff --git a/crates/ra_mbe/src/tests.rs b/crates/ra_mbe/src/tests.rs index 6d5d1e9e6..4d3140fa9 100644 --- a/crates/ra_mbe/src/tests.rs +++ b/crates/ra_mbe/src/tests.rs @@ -1430,7 +1430,8 @@ impl MacroFixture { let (invocation_tt, _) = ast_to_token_tree(¯o_invocation.token_tree().unwrap()).unwrap(); - self.rules.expand(&invocation_tt) + let (tt, err) = self.rules.expand(&invocation_tt); + err.map(Err).unwrap_or(Ok(tt)) } fn assert_expand_err(&self, invocation: &str, err: &ExpandError) { diff --git a/crates/ra_tt/src/lib.rs b/crates/ra_tt/src/lib.rs index 10f424aae..1e2fb8b91 100644 --- a/crates/ra_tt/src/lib.rs +++ b/crates/ra_tt/src/lib.rs @@ -40,6 +40,12 @@ pub enum TokenTree { } impl_froms!(TokenTree: Leaf, Subtree); +impl TokenTree { + pub fn empty() -> Self { + TokenTree::Subtree(Subtree::default()) + } +} + #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub enum Leaf { Literal(Literal), -- cgit v1.2.3