Make MBE expansion more resilient (WIP)

crates/ra_hir_expand/src/db.rs

@@ -27,11 +27,12 @@ impl TokenExpander {
         db: &dyn AstDatabase,
         id: LazyMacroId,
         tt: &tt::Subtree,
-    ) -> Result<tt::Subtree, mbe::ExpandError> {
+    ) -> mbe::ExpandResult<tt::Subtree> {
         match self {
             TokenExpander::MacroRules(it) => it.expand(tt),
-            TokenExpander::Builtin(it) => it.expand(db, id, tt),
+            // FIXME switch these to ExpandResult as well
-            TokenExpander::BuiltinDerive(it) => it.expand(db, id, tt),
+            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<Arc<(TokenExpander, mbe::TokenMap)>>;
     fn parse_macro(&self, macro_file: MacroFile)
         -> Option<(Parse<SyntaxNode>, Arc<mbe::TokenMap>)>;
-    fn macro_expand(&self, macro_call: MacroCallId) -> Result<Arc<tt::Subtree>, String>;
+    fn macro_expand(&self, macro_call: MacroCallId) -> (Option<Arc<tt::Subtree>>, Option<String>);
 
     #[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<Arc<tt::Subtree>, String> {
+) -> (Option<Arc<tt::Subtree>>, Option<String>) {
     macro_expand_with_arg(db, id, None)
 }
 
@@ -174,31 +175,39 @@ fn macro_expand_with_arg(
     db: &dyn AstDatabase,
     id: MacroCallId,
     arg: Option<Arc<(tt::Subtree, mbe::TokenMap)>>,
-) -> Result<Arc<tt::Subtree>, String> {
+) -> (Option<Arc<tt::Subtree>>, Option<String>) {
     let lazy_id = match id {
         MacroCallId::LazyMacro(id) => id,
         MacroCallId::EagerMacro(id) => {
             if arg.is_some() {
-                return Err(
+                return (
-                    "hypothetical macro expansion not implemented for eager macro".to_owned()
+                    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 macro_rules = match db.macro_def(loc.def) {
-    let tt = macro_rules.0.expand(db, lazy_id, &macro_arg.0).map_err(|err| format!("{:?}", err))?;
+        Some(it) => it,
+        None => return (None, Some("Fail to find macro definition".into())),
+    };
+    let (tt, err) = macro_rules.0.expand(db, lazy_id, &macro_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<SyntaxNode> {
@@ -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
+    if let Some(err) = err {
-        .map_err(|err| {
+        // Note:
-            // Note:
+        // The final goal we would like to make all parse_macro success,
-            // The final goal we would like to make all parse_macro success,
+        // such that the following log will not call anyway.
-            // such that the following log will not call anyway.
+        match macro_call_id {
-            match macro_call_id {
+            MacroCallId::LazyMacro(id) => {
-                MacroCallId::LazyMacro(id) => {
+                let loc: MacroCallLoc = db.lookup_intern_macro(id);
-                    let loc: MacroCallLoc = db.lookup_intern_macro(id);
+                let node = loc.kind.node(db);
-                    let node = loc.kind.node(db);
 
-                    // collect parent information for warning log
+                // collect parent information for warning log
-                    let parents = std::iter::successors(loc.kind.file_id().call_node(db), |it| {
+                let parents = std::iter::successors(loc.kind.file_id().call_node(db), |it| {
-                        it.file_id.call_node(db)
+                    it.file_id.call_node(db)
-                    })
+                })
                     .map(|n| format!("{:#}", n.value))
                     .collect::<Vec<_>>()
                     .join("\n");
 
-                    log::warn!(
+                log::warn!(
-                        "fail on macro_parse: (reason: {} macro_call: {:#}) parents: {}",
+                    "fail on macro_parse: (reason: {} macro_call: {:#}) parents: {}",
-                        err,
+                    err,
-                        node.value,
+                    node.value,
-                        parents
+                    parents
-                    );
+                );
-                }
-                _ => {
-                    log::warn!("fail on macro_parse: (reason: {})", err);
-                }
             }
-        })
+            _ => {
-        .ok()?;
+                log::warn!("fail on macro_parse: (reason: {})", err);
+            }
+        }
+    };
+    let tt = tt?;
 
     let fragment_kind = to_fragment_kind(db, macro_call_id);
 

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 {() => {}}
 

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!(

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]

crates/ra_mbe/src/lib.rs

@@ -30,6 +30,8 @@ pub enum ExpandError {
     InvalidRepeat,
 }
 
+pub type ExpandResult<T> = (T, Option<ExpandError>);
+
 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<tt::Subtree, ExpandError> {
+    pub fn expand(&self, tt: &tt::Subtree) -> ExpandResult<tt::Subtree> {
         // apply shift
         let mut tt = tt.clone();
         self.shift.shift_all(&mut tt);

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<tt::Subtree, ExpandError> {
+) -> ExpandResult<tt::Subtree> {
-    rules.rules.iter().find_map(|it| expand_rule(it, input).ok()).ok_or(ExpandError::NoMatchingRule)
+    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<tt::Subtree, ExpandError> {
+fn expand_rule(rule: &crate::Rule, input: &tt::Subtree) -> ExpandResult<tt::Subtree> {
-    let bindings = matcher::match_(&rule.lhs, input)?;
+    let (bindings, bindings_err) = dbg!(matcher::match_(&rule.lhs, input));
-    let res = transcriber::transcribe(&rule.rhs, &bindings)?;
+    let (res, transcribe_err) = dbg!(transcriber::transcribe(&rule.rhs, &bindings));
-    Ok(res)
+    (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<tt::Subtree, ExpandError> {
+    ) -> ExpandResult<tt::Subtree> {
         let source_file = ast::SourceFile::parse(invocation).ok().unwrap();
         let macro_invocation =
             source_file.syntax().descendants().find_map(ast::MacroCall::cast).unwrap();

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<Bindings, ExpandError> {
+pub(super) fn match_(pattern: &tt::Subtree, src: &tt::Subtree) -> ExpandResult<Bindings> {
     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 {
+    if src.len() > 0 && err.is_none() {
-        bail!("leftover tokens");
+        err = Some(err!("leftover tokens"));
     }
 
-    Ok(res)
+    (res, err)
 }
 
 fn match_subtree(

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(
+pub(super) fn transcribe(template: &tt::Subtree, bindings: &Bindings) -> ExpandResult<tt::Subtree> {
-    template: &tt::Subtree,
-    bindings: &Bindings,
-) -> Result<tt::Subtree, ExpandError> {
     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<NestingState>,
 }
 
-fn expand_subtree(ctx: &mut ExpandCtx, template: &tt::Subtree) -> Result<tt::Subtree, ExpandError> {
+fn expand_subtree(ctx: &mut ExpandCtx, template: &tt::Subtree) -> ExpandResult<tt::Subtree> {
     let mut buf: Vec<tt::TokenTree> = 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<Fragment, ExpandError> {
+fn expand_var(ctx: &mut ExpandCtx, v: &SmolStr) -> ExpandResult<Fragment> {
-    let res = if v == "crate" {
+    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<Fragment, ExpandError>
             ],
         }
         .into();
-        Fragment::Tokens(tt)
+        (Fragment::Tokens(tt), None)
     } else {
-        ctx.bindings.get(&v, &mut ctx.nesting)?.clone()
+        ctx.bindings.get(&v, &mut ctx.nesting).map_or_else(
-    };
+            |e| (Fragment::Tokens(tt::TokenTree::empty()), Some(e)),
-    Ok(res)
+            |b| (b.clone(), None),
+        )
+    }
 }
 
 fn expand_repeat(
@@ -144,17 +155,17 @@ fn expand_repeat(
     template: &tt::Subtree,
     kind: RepeatKind,
     separator: Option<Separator>,
-) -> Result<Fragment, ExpandError> {
+) -> ExpandResult<Fragment> {
     let mut buf: Vec<tt::TokenTree> = 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 {
+        if e.is_some() {
-            Ok(t) => t,
+            continue;
-            Err(_) => 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<tt::TokenTree>, fragment: Fragment) {

crates/ra_mbe/src/tests.rs

@@ -1430,7 +1430,8 @@ impl MacroFixture {
         let (invocation_tt, _) =
             ast_to_token_tree(&macro_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) {

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),