Merge #3580

3580: More error-resilient MBE expansion r=matklad a=flodiebold

This is the beginning of an attempt to make macro-by-example expansion more resilient, so that we still get an expansion even if no rule exactly matches, with the goal to make completion work better in macro calls.

 The general idea is to make everything return `(T, Option<ExpandError>)` instead of `Result<T, ExpandError>`; and then to try each macro arm in turn, and somehow choose the 'best' matching rule if none matches without errors. Finding that 'best' match isn't done yet; I'm currently counting how many tokens were consumed from the args before an error, but it also needs to take into account whether there were further patterns that had nothing to match.

I'll continue this later, but I'm interested whether you think this is the right path, @matklad & @edwin0cheng.

Co-authored-by: Florian Diebold <[email protected]>
Co-authored-by: Florian Diebold <[email protected]>

14 files changed, 471 insertions, 181 deletions

diff --git a/crates/ra_hir_expand/src/db.rs b/crates/ra_hir_expand/src/db.rs
index c3e1c68b7..d171d2dfd 100644
--- a/crates/ra_hir_expand/src/db.rs
+++ b/crates/ra_hir_expand/src/db.rs
@@ -2,7 +2,7 @@
 
 use std::sync::Arc;
 
-use mbe::MacroRules;
+use mbe::{ExpandResult, MacroRules};
 use ra_db::{salsa, SourceDatabase};
 use ra_parser::FragmentKind;
 use ra_prof::profile;
@@ -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),
-            TokenExpander::BuiltinDerive(it) => it.expand(db, id, tt),
+            // FIXME switch these to ExpandResult as well
+            TokenExpander::Builtin(it) => it.expand(db, id, tt).into(),
+            TokenExpander::BuiltinDerive(it) => it.expand(db, id, tt).into(),
         }
     }
 
@@ -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,38 @@ 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(
-                    "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, &macro_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 ExpandResult(tt, err) = macro_rules.0.expand(db, lazy_id, &macro_arg.0);
     // Set a hard limit for the expanded tt
     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 +233,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)
-                    })
-                    .map(|n| format!("{:#}", n.value))
-                    .collect::<Vec<_>>()
-                    .join("\n");
-
-                    log::warn!(
-                        "fail on macro_parse: (reason: {} macro_call: {:#}) parents: {}",
-                        err,
-                        node.value,
-                        parents
-                    );
-                }
-                _ => {
-                    log::warn!("fail on macro_parse: (reason: {})", err);
-                }
+    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::<Vec<_>>()
+                .join("\n");
+
+                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..82ec16913 100644
--- a/crates/ra_ide/src/completion/complete_dot.rs
+++ b/crates/ra_ide/src/completion/complete_dot.rs
@@ -720,7 +720,18 @@ mod tests {
                 }
                 ",
             ),
-            @r###"[]"###
+            @r###"
+        [
+            CompletionItem {
+                label: "the_field",
+                source_range: [156; 156),
+                delete: [156; 156),
+                insert: "the_field",
+                kind: Field,
+                detail: "u32",
+            },
+        ]
+        "###
         );
     }
 
@@ -752,6 +763,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; 552),
+                delete: [552; 552),
+                insert: "the_field",
+                kind: Field,
+                detail: "u32",
+            },
+        ]
+        "###
+        );
+    }
+
+    #[test]
     fn test_method_completion_3547() {
         assert_debug_snapshot!(
             do_ref_completion(
diff --git a/crates/ra_ide/src/completion/complete_pattern.rs b/crates/ra_ide/src/completion/complete_pattern.rs
index 6a1a66ef1..cb84bb934 100644
--- a/crates/ra_ide/src/completion/complete_pattern.rs
+++ b/crates/ra_ide/src/completion/complete_pattern.rs
@@ -89,7 +89,6 @@ mod tests {
 
     #[test]
     fn completes_in_simple_macro_call() {
-        // FIXME: doesn't work yet because of missing error recovery in macro expansion
         let completions = complete(
             r"
             macro_rules! m { ($e:expr) => { $e } }
@@ -102,6 +101,16 @@ mod tests {
             }
             ",
         );
-        assert_debug_snapshot!(completions, @r###"[]"###);
+        assert_debug_snapshot!(completions, @r###"
+        [
+            CompletionItem {
+                label: "E",
+                source_range: [151; 151),
+                delete: [151; 151),
+                insert: "E",
+                kind: Enum,
+            },
+        ]
+        "###);
     }
 }
diff --git a/crates/ra_ide/src/completion/complete_scope.rs b/crates/ra_ide/src/completion/complete_scope.rs
index 5ffff5a1c..81d3cc1b6 100644
--- a/crates/ra_ide/src/completion/complete_scope.rs
+++ b/crates/ra_ide/src/completion/complete_scope.rs
@@ -811,7 +811,44 @@ mod tests {
                 }
                 "
             ),
-            @"[]"
+            @r###"
+        [
+            CompletionItem {
+                label: "m!",
+                source_range: [145; 145),
+                delete: [145; 145),
+                insert: "m!($0)",
+                kind: Macro,
+                detail: "macro_rules! m",
+            },
+            CompletionItem {
+                label: "quux(…)",
+                source_range: [145; 145),
+                delete: [145; 145),
+                insert: "quux(${1:x})$0",
+                kind: Function,
+                lookup: "quux",
+                detail: "fn quux(x: i32)",
+                trigger_call_info: true,
+            },
+            CompletionItem {
+                label: "x",
+                source_range: [145; 145),
+                delete: [145; 145),
+                insert: "x",
+                kind: Binding,
+                detail: "i32",
+            },
+            CompletionItem {
+                label: "y",
+                source_range: [145; 145),
+                delete: [145; 145),
+                insert: "y",
+                kind: Binding,
+                detail: "i32",
+            },
+        ]
+        "###
         );
     }
 
@@ -869,6 +906,59 @@ mod tests {
     }
 
     #[test]
+    fn completes_in_simple_macro_without_closing_parens() {
+        assert_debug_snapshot!(
+                    do_reference_completion(
+                        r"
+                macro_rules! m { ($e:expr) => { $e } }
+                fn quux(x: i32) {
+                    let y = 92;
+                    m!(x<|>
+                }
+                "
+                    ),
+                    @r###"
+        [
+            CompletionItem {
+                label: "m!",
+                source_range: [145; 146),
+                delete: [145; 146),
+                insert: "m!($0)",
+                kind: Macro,
+                detail: "macro_rules! m",
+            },
+            CompletionItem {
+                label: "quux(…)",
+                source_range: [145; 146),
+                delete: [145; 146),
+                insert: "quux(${1:x})$0",
+                kind: Function,
+                lookup: "quux",
+                detail: "fn quux(x: i32)",
+                trigger_call_info: true,
+            },
+            CompletionItem {
+                label: "x",
+                source_range: [145; 146),
+                delete: [145; 146),
+                insert: "x",
+                kind: Binding,
+                detail: "i32",
+            },
+            CompletionItem {
+                label: "y",
+                source_range: [145; 146),
+                delete: [145; 146),
+                insert: "y",
+                kind: Binding,
+                detail: "i32",
+            },
+        ]
+        "###
+        );
+    }
+
+    #[test]
     fn completes_unresolved_uses() {
         assert_debug_snapshot!(
             do_reference_completion(
diff --git a/crates/ra_ide/src/completion/completion_context.rs b/crates/ra_ide/src/completion/completion_context.rs
index 3646fb8dc..54589a2a8 100644
--- a/crates/ra_ide/src/completion/completion_context.rs
+++ b/crates/ra_ide/src/completion/completion_context.rs
@@ -135,7 +135,7 @@ impl<'a> CompletionContext<'a> {
                 ),
             ) {
                 let new_offset = hypothetical_expansion.1.text_range().start();
-                if new_offset >= actual_expansion.text_range().end() {
+                if new_offset > actual_expansion.text_range().end() {
                     break;
                 }
                 original_file = actual_expansion;
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..6a9037bfc 100644
--- a/crates/ra_mbe/src/lib.rs
+++ b/crates/ra_mbe/src/lib.rs
@@ -150,7 +150,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);
@@ -209,5 +209,35 @@ fn validate(pattern: &tt::Subtree) -> Result<(), ParseError> {
     Ok(())
 }
 
+pub struct ExpandResult<T>(pub T, pub Option<ExpandError>);
+
+impl<T> ExpandResult<T> {
+    pub fn ok(t: T) -> ExpandResult<T> {
+        ExpandResult(t, None)
+    }
+
+    pub fn only_err(err: ExpandError) -> ExpandResult<T>
+    where
+        T: Default,
+    {
+        ExpandResult(Default::default(), Some(err))
+    }
+
+    pub fn map<U>(self, f: impl FnOnce(T) -> U) -> ExpandResult<U> {
+        ExpandResult(f(self.0), self.1)
+    }
+
+    pub fn result(self) -> Result<T, ExpandError> {
+        self.1.map(Err).unwrap_or(Ok(self.0))
+    }
+}
+
+impl<T: Default> From<Result<T, ExpandError>> for ExpandResult<T> {
+    fn from(result: Result<T, ExpandError>) -> ExpandResult<T> {
+        result
+            .map_or_else(|e| ExpandResult(Default::default(), Some(e)), |it| ExpandResult(it, None))
+    }
+}
+
 #[cfg(test)]
 mod tests;
diff --git a/crates/ra_mbe/src/mbe_expander.rs b/crates/ra_mbe/src/mbe_expander.rs
index b455b7321..b1eacf124 100644
--- a/crates/ra_mbe/src/mbe_expander.rs
+++ b/crates/ra_mbe/src/mbe_expander.rs
@@ -8,19 +8,51 @@ mod transcriber;
 use ra_syntax::SmolStr;
 use rustc_hash::FxHashMap;
 
-use crate::ExpandError;
+use crate::{ExpandError, ExpandResult};
 
-pub(crate) fn expand(
-    rules: &crate::MacroRules,
-    input: &tt::Subtree,
-) -> Result<tt::Subtree, ExpandError> {
-    rules.rules.iter().find_map(|it| expand_rule(it, input).ok()).ok_or(ExpandError::NoMatchingRule)
+pub(crate) fn expand(rules: &crate::MacroRules, input: &tt::Subtree) -> ExpandResult<tt::Subtree> {
+    expand_rules(&rules.rules, input)
 }
 
-fn expand_rule(rule: &crate::Rule, input: &tt::Subtree) -> Result<tt::Subtree, ExpandError> {
-    let bindings = matcher::match_(&rule.lhs, input)?;
-    let res = transcriber::transcribe(&rule.rhs, &bindings)?;
-    Ok(res)
+fn expand_rules(rules: &[crate::Rule], input: &tt::Subtree) -> ExpandResult<tt::Subtree> {
+    let mut match_: Option<(matcher::Match, &crate::Rule)> = None;
+    for rule in rules {
+        let new_match = match matcher::match_(&rule.lhs, input) {
+            Ok(m) => m,
+            Err(_e) => {
+                // error in pattern parsing
+                continue;
+            }
+        };
+        if new_match.err.is_none() {
+            // If we find a rule that applies without errors, we're done.
+            // Unconditionally returning the transcription here makes the
+            // `test_repeat_bad_var` test fail.
+            let ExpandResult(res, transcribe_err) =
+                transcriber::transcribe(&rule.rhs, &new_match.bindings);
+            if transcribe_err.is_none() {
+                return ExpandResult::ok(res);
+            }
+        }
+        // Use the rule if we matched more tokens, or had fewer errors
+        if let Some((prev_match, _)) = &match_ {
+            if (new_match.unmatched_tts, new_match.err_count)
+                < (prev_match.unmatched_tts, prev_match.err_count)
+            {
+                match_ = Some((new_match, rule));
+            }
+        } else {
+            match_ = Some((new_match, rule));
+        }
+    }
+    if let Some((match_, rule)) = match_ {
+        // if we got here, there was no match without errors
+        let ExpandResult(result, transcribe_err) =
+            transcriber::transcribe(&rule.rhs, &match_.bindings);
+        ExpandResult(result, match_.err.or(transcribe_err))
+    } else {
+        ExpandResult(tt::Subtree::default(), Some(ExpandError::NoMatchingRule))
+    }
 }
 
 /// The actual algorithm for expansion is not too hard, but is pretty tricky.
@@ -111,7 +143,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 {
@@ -135,10 +167,7 @@ mod tests {
         crate::MacroRules::parse(&definition_tt).unwrap()
     }
 
-    fn expand_first(
-        rules: &crate::MacroRules,
-        invocation: &str,
-    ) -> Result<tt::Subtree, ExpandError> {
+    fn expand_first(rules: &crate::MacroRules, invocation: &str) -> 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();
@@ -146,6 +175,6 @@ mod tests {
         let (invocation_tt, _) =
             ast_to_token_tree(&macro_invocation.token_tree().unwrap()).unwrap();
 
-        expand_rule(&rules.rules[0], &invocation_tt)
+        expand_rules(&rules.rules, &invocation_tt)
     }
 }
diff --git a/crates/ra_mbe/src/mbe_expander/matcher.rs b/crates/ra_mbe/src/mbe_expander/matcher.rs
index 49c53183a..2579382da 100644
--- a/crates/ra_mbe/src/mbe_expander/matcher.rs
+++ b/crates/ra_mbe/src/mbe_expander/matcher.rs
@@ -8,6 +8,7 @@ use crate::{
     ExpandError,
 };
 
+use super::ExpandResult;
 use ra_parser::{FragmentKind::*, TreeSink};
 use ra_syntax::{SmolStr, SyntaxKind};
 use tt::buffer::{Cursor, TokenBuffer};
@@ -58,36 +59,61 @@ macro_rules! err {
     };
 }
 
-macro_rules! bail {
-    ($($tt:tt)*) => {
-        return Err(err!($($tt)*))
-    };
+#[derive(Debug, Default)]
+pub(super) struct Match {
+    pub bindings: Bindings,
+    /// We currently just keep the first error and count the rest to compare matches.
+    pub err: Option<ExpandError>,
+    pub err_count: usize,
+    /// How many top-level token trees were left to match.
+    pub unmatched_tts: usize,
 }
 
-pub(super) fn match_(pattern: &tt::Subtree, src: &tt::Subtree) -> Result<Bindings, ExpandError> {
+impl Match {
+    pub fn add_err(&mut self, err: ExpandError) {
+        let prev_err = self.err.take();
+        self.err = prev_err.or(Some(err));
+        self.err_count += 1;
+    }
+}
+
+// General note: These functions have two channels to return errors, a `Result`
+// return value and the `&mut Match`. The returned Result is for pattern parsing
+// errors; if a branch of the macro definition doesn't parse, it doesn't make
+// sense to try using it. Matching errors are added to the `Match`. It might
+// make sense to make pattern parsing a separate step?
+
+pub(super) fn match_(pattern: &tt::Subtree, src: &tt::Subtree) -> Result<Match, ExpandError> {
     assert!(pattern.delimiter == None);
 
-    let mut res = Bindings::default();
+    let mut res = Match::default();
     let mut src = TtIter::new(src);
 
     match_subtree(&mut res, pattern, &mut src)?;
 
     if src.len() > 0 {
-        bail!("leftover tokens");
+        res.unmatched_tts += src.len();
+        res.add_err(err!("leftover tokens"));
     }
 
     Ok(res)
 }
 
 fn match_subtree(
-    bindings: &mut Bindings,
+    res: &mut Match,
     pattern: &tt::Subtree,
     src: &mut TtIter,
 ) -> Result<(), ExpandError> {
     for op in parse_pattern(pattern) {
         match op? {
             Op::TokenTree(tt::TokenTree::Leaf(lhs)) => {
-                let rhs = src.expect_leaf().map_err(|()| err!("expected leaf: `{}`", lhs))?;
+                let rhs = match src.expect_leaf() {
+                    Ok(l) => l,
+                    Err(()) => {
+                        res.add_err(err!("expected leaf: `{}`", lhs));
+                        continue;
+                    }
+                };
                 match (lhs, rhs) {
                     (
                         tt::Leaf::Punct(tt::Punct { char: lhs, .. }),
@@ -101,31 +127,51 @@ fn match_subtree(
                         tt::Leaf::Literal(tt::Literal { text: lhs, .. }),
                         tt::Leaf::Literal(tt::Literal { text: rhs, .. }),
                     ) if lhs == rhs => (),
-                    _ => return Err(ExpandError::UnexpectedToken),
+                    _ => {
+                        res.add_err(ExpandError::UnexpectedToken);
+                    }
                 }
             }
             Op::TokenTree(tt::TokenTree::Subtree(lhs)) => {
-                let rhs = src.expect_subtree().map_err(|()| err!("expected subtree"))?;
+                let rhs = match src.expect_subtree() {
+                    Ok(s) => s,
+                    Err(()) => {
+                        res.add_err(err!("expected subtree"));
+                        continue;
+                    }
+                };
                 if lhs.delimiter_kind() != rhs.delimiter_kind() {
-                    bail!("mismatched delimiter")
+                    res.add_err(err!("mismatched delimiter"));
+                    continue;
                 }
                 let mut src = TtIter::new(rhs);
-                match_subtree(bindings, lhs, &mut src)?;
+                match_subtree(res, lhs, &mut src)?;
                 if src.len() > 0 {
-                    bail!("leftover tokens");
+                    res.add_err(err!("leftover tokens"));
                 }
             }
             Op::Var { name, kind } => {
-                let kind = kind.as_ref().ok_or(ExpandError::UnexpectedToken)?;
-                match match_meta_var(kind.as_str(), src)? {
+                let kind = match kind {
+                    Some(k) => k,
+                    None => {
+                        res.add_err(ExpandError::UnexpectedToken);
+                        continue;
+                    }
+                };
+                let ExpandResult(matched, match_err) = match_meta_var(kind.as_str(), src);
+                match matched {
                     Some(fragment) => {
-                        bindings.inner.insert(name.clone(), Binding::Fragment(fragment));
+                        res.bindings.inner.insert(name.clone(), Binding::Fragment(fragment));
                     }
-                    None => bindings.push_optional(name),
+                    None if match_err.is_none() => res.bindings.push_optional(name),
+                    _ => {}
+                }
+                if let Some(err) = match_err {
+                    res.add_err(err);
                 }
             }
             Op::Repeat { subtree, kind, separator } => {
-                match_repeat(bindings, subtree, kind, separator, src)?
+                match_repeat(res, subtree, kind, separator, src)?;
             }
         }
     }
@@ -221,7 +267,7 @@ impl<'a> TtIter<'a> {
     pub(crate) fn expect_fragment(
         &mut self,
         fragment_kind: ra_parser::FragmentKind,
-    ) -> Result<tt::TokenTree, ()> {
+    ) -> ExpandResult<Option<tt::TokenTree>> {
         pub(crate) struct OffsetTokenSink<'a> {
             pub(crate) cursor: Cursor<'a>,
             pub(crate) error: bool,
@@ -246,45 +292,51 @@ impl<'a> TtIter<'a> {
 
         ra_parser::parse_fragment(&mut src, &mut sink, fragment_kind);
 
+        let mut err = None;
         if !sink.cursor.is_root() || sink.error {
-            // FIXME better recovery in this case would help completion inside macros immensely
-            return Err(());
+            err = Some(err!("expected {:?}", fragment_kind));
         }
 
         let mut curr = buffer.begin();
         let mut res = vec![];
 
-        while curr != sink.cursor {
-            if let Some(token) = curr.token_tree() {
-                res.push(token);
+        if sink.cursor.is_root() {
+            while curr != sink.cursor {
+                if let Some(token) = curr.token_tree() {
+                    res.push(token);
+                }
+                curr = curr.bump();
             }
-            curr = curr.bump();
         }
         self.inner = self.inner.as_slice()[res.len()..].iter();
-        match res.len() {
-            0 => Err(()),
-            1 => Ok(res[0].clone()),
-            _ => Ok(tt::TokenTree::Subtree(tt::Subtree {
+        if res.len() == 0 && err.is_none() {
+            err = Some(err!("no tokens consumed"));
+        }
+        let res = match res.len() {
+            1 => Some(res[0].clone()),
+            0 => None,
+            _ => Some(tt::TokenTree::Subtree(tt::Subtree {
                 delimiter: None,
                 token_trees: res.into_iter().cloned().collect(),
             })),
-        }
+        };
+        ExpandResult(res, err)
     }
 
     pub(crate) fn eat_vis(&mut self) -> Option<tt::TokenTree> {
         let mut fork = self.clone();
         match fork.expect_fragment(Visibility) {
-            Ok(tt) => {
+            ExpandResult(tt, None) => {
                 *self = fork;
-                Some(tt)
+                tt
             }
-            Err(()) => None,
+            ExpandResult(_, Some(_)) => None,
         }
     }
 }
 
 pub(super) fn match_repeat(
-    bindings: &mut Bindings,
+    res: &mut Match,
     pattern: &tt::Subtree,
     kind: RepeatKind,
     separator: Option<Separator>,
@@ -304,36 +356,46 @@ pub(super) fn match_repeat(
             }
         }
 
-        let mut nested = Bindings::default();
-        match match_subtree(&mut nested, pattern, &mut fork) {
-            Ok(()) => {
-                limit -= 1;
-                if limit == 0 {
-                    log::warn!("match_lhs excced in repeat pattern exceed limit => {:#?}\n{:#?}\n{:#?}\n{:#?}", pattern, src, kind, separator);
-                    break;
-                }
-                *src = fork;
+        let mut nested = Match::default();
+        match_subtree(&mut nested, pattern, &mut fork)?;
+        if nested.err.is_none() {
+            limit -= 1;
+            if limit == 0 {
+                log::warn!(
+                    "match_lhs exceeded repeat pattern limit => {:#?}\n{:#?}\n{:#?}\n{:#?}",
+                    pattern,
+                    src,
+                    kind,
+                    separator
+                );
+                break;
+            }
+            *src = fork;
 
-                bindings.push_nested(counter, nested)?;
-                counter += 1;
-                if counter == 1 {
-                    if let RepeatKind::ZeroOrOne = kind {
-                        break;
-                    }
+            if let Err(err) = res.bindings.push_nested(counter, nested.bindings) {
+                res.add_err(err);
+            }
+            counter += 1;
+            if counter == 1 {
+                if let RepeatKind::ZeroOrOne = kind {
+                    break;
                 }
             }
-            Err(_) => break,
+        } else {
+            break;
         }
     }
 
     match (kind, counter) {
-        (RepeatKind::OneOrMore, 0) => return Err(ExpandError::UnexpectedToken),
+        (RepeatKind::OneOrMore, 0) => {
+            res.add_err(ExpandError::UnexpectedToken);
+        }
         (_, 0) => {
             // Collect all empty variables in subtrees
             let mut vars = Vec::new();
             collect_vars(&mut vars, pattern)?;
             for var in vars {
-                bindings.push_empty(&var)
+                res.bindings.push_empty(&var)
             }
         }
         _ => (),
@@ -341,7 +403,7 @@ pub(super) fn match_repeat(
     Ok(())
 }
 
-fn match_meta_var(kind: &str, input: &mut TtIter) -> Result<Option<Fragment>, ExpandError> {
+fn match_meta_var(kind: &str, input: &mut TtIter) -> ExpandResult<Option<Fragment>> {
     let fragment = match kind {
         "path" => Path,
         "expr" => Expr,
@@ -352,34 +414,32 @@ fn match_meta_var(kind: &str, input: &mut TtIter) -> Result<Option<Fragment>, Ex
         "meta" => MetaItem,
         "item" => Item,
         _ => {
-            let tt = match kind {
-                "ident" => {
-                    let ident = input.expect_ident().map_err(|()| err!("expected ident"))?.clone();
-                    tt::Leaf::from(ident).into()
-                }
-                "tt" => input.expect_tt().map_err(|()| err!())?.clone(),
-                "lifetime" => {
-                    let ident = input.expect_lifetime().map_err(|()| err!())?;
-                    tt::Leaf::Ident(ident.clone()).into()
-                }
-                "literal" => {
-                    let literal = input.expect_literal().map_err(|()| err!())?.clone();
-                    tt::Leaf::from(literal).into()
-                }
+            let tt_result = match kind {
+                "ident" => input
+                    .expect_ident()
+                    .map(|ident| Some(tt::Leaf::from(ident.clone()).into()))
+                    .map_err(|()| err!("expected ident")),
+                "tt" => input.expect_tt().map(Some).map_err(|()| err!()),
+                "lifetime" => input
+                    .expect_lifetime()
+                    .map(|ident| Some(tt::Leaf::Ident(ident.clone()).into()))
+                    .map_err(|()| err!("expected lifetime")),
+                "literal" => input
+                    .expect_literal()
+                    .map(|literal| Some(tt::Leaf::from(literal.clone()).into()))
+                    .map_err(|()| err!()),
                 // `vis` is optional
                 "vis" => match input.eat_vis() {
-                    Some(vis) => vis,
-                    None => return Ok(None),
+                    Some(vis) => Ok(Some(vis)),
+                    None => Ok(None),
                 },
-                _ => return Err(ExpandError::UnexpectedToken),
+                _ => Err(ExpandError::UnexpectedToken),
             };
-            return Ok(Some(Fragment::Tokens(tt)));
+            return tt_result.map(|it| it.map(Fragment::Tokens)).into();
         }
     };
-    let tt =
-        input.expect_fragment(fragment).map_err(|()| err!("fragment did not parse as {}", kind))?;
-    let fragment = if kind == "expr" { Fragment::Ast(tt) } else { Fragment::Tokens(tt) };
-    Ok(Some(fragment))
+    let result = input.expect_fragment(fragment);
+    result.map(|tt| if kind == "expr" { tt.map(Fragment::Ast) } else { tt.map(Fragment::Tokens) })
 }
 
 fn collect_vars(buf: &mut Vec<SmolStr>, pattern: &tt::Subtree) -> Result<(), ExpandError> {
diff --git a/crates/ra_mbe/src/mbe_expander/transcriber.rs b/crates/ra_mbe/src/mbe_expander/transcriber.rs
index 7662020f3..4b173edd3 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<tt::Subtree, ExpandError> {
+pub(super) fn transcribe(template: &tt::Subtree, bindings: &Bindings) -> ExpandResult<tt::Subtree> {
     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 ExpandResult(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 ExpandResult(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 ExpandResult(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 })
+    ExpandResult(tt::Subtree { delimiter: template.delimiter, token_trees: buf }, err)
 }
 
-fn expand_var(ctx: &mut ExpandCtx, v: &SmolStr) -> Result<Fragment, ExpandError> {
-    let res = if v == "crate" {
+fn expand_var(ctx: &mut ExpandCtx, v: &SmolStr) -> ExpandResult<Fragment> {
+    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)
+        ExpandResult::ok(Fragment::Tokens(tt))
     } 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)
+        ExpandResult::ok(Fragment::Tokens(tt))
     } else {
-        ctx.bindings.get(&v, &mut ctx.nesting)?.clone()
-    };
-    Ok(res)
+        ctx.bindings.get(&v, &mut ctx.nesting).map_or_else(
+            |e| ExpandResult(Fragment::Tokens(tt::TokenTree::empty()), Some(e)),
+            |b| ExpandResult::ok(b.clone()),
+        )
+    }
 }
 
 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 ExpandResult(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 ExpandResult(Fragment::Tokens(tt), Some(ExpandError::UnexpectedToken));
+    }
+    ExpandResult::ok(Fragment::Tokens(tt))
 }
 
 fn push_fragment(buf: &mut Vec<tt::TokenTree>, fragment: Fragment) {
diff --git a/crates/ra_mbe/src/tests.rs b/crates/ra_mbe/src/tests.rs
index 6d5d1e9e6..44f381938 100644
--- a/crates/ra_mbe/src/tests.rs
+++ b/crates/ra_mbe/src/tests.rs
@@ -1430,7 +1430,7 @@ impl MacroFixture {
         let (invocation_tt, _) =
             ast_to_token_tree(&macro_invocation.token_tree().unwrap()).unwrap();
 
-        self.rules.expand(&invocation_tt)
+        self.rules.expand(&invocation_tt).result()
     }
 
     fn assert_expand_err(&self, invocation: &str, err: &ExpandError) {
@@ -1662,5 +1662,5 @@ fn test_expand_bad_literal() {
         macro_rules! foo { ($i:literal) => {}; }
     "#,
     )
-    .assert_expand_err(r#"foo!(&k");"#, &ExpandError::NoMatchingRule);
+    .assert_expand_err(r#"foo!(&k");"#, &ExpandError::BindingError("".to_string()));
 }
diff --git a/crates/ra_parser/src/grammar/expressions/atom.rs b/crates/ra_parser/src/grammar/expressions/atom.rs
index b77b683b5..2335d99b3 100644
--- a/crates/ra_parser/src/grammar/expressions/atom.rs
+++ b/crates/ra_parser/src/grammar/expressions/atom.rs
@@ -61,7 +61,7 @@ pub(super) const ATOM_EXPR_FIRST: TokenSet =
         LIFETIME,
     ]);
 
-const EXPR_RECOVERY_SET: TokenSet = token_set![LET_KW];
+const EXPR_RECOVERY_SET: TokenSet = token_set![LET_KW, R_DOLLAR];
 
 pub(super) fn atom_expr(p: &mut Parser, r: Restrictions) -> Option<(CompletedMarker, BlockLike)> {
     if let Some(m) = literal(p) {
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),