Merge #1189

1189: Fix #1178 r=matklad a=edwin0cheng

This PR improves / fixes mbe :

1. Fixed a offest bug in `SourceTreeWalker`
2. Handle `*+` matcher properly
3. Add missing separator in rhs macro expansion.
4. Fixed bug in single token with empty delimiter subtree case. It is because the current `mbe_expander` will create an delimiter subtree for each expansion. But in `tt` case, all puncts expansion will be incorrect because of it. 
5. Fixed lifetime bug
6. Add more information on parse_macro fail
7. Add tests for above.



Co-authored-by: Edwin Cheng <[email protected]>

5 files changed, 194 insertions, 34 deletions

diff --git a/crates/ra_hir/src/ids.rs b/crates/ra_hir/src/ids.rs
index 2a1ed9b81..e771a311c 100644
--- a/crates/ra_hir/src/ids.rs
+++ b/crates/ra_hir/src/ids.rs
@@ -63,11 +63,15 @@ impl HirFileId {
         match file_id.0 {
             HirFileIdRepr::File(file_id) => db.parse(file_id),
             HirFileIdRepr::Macro(macro_call_id) => {
-                parse_macro(db, macro_call_id).unwrap_or_else(|| {
+                parse_macro(db, macro_call_id).unwrap_or_else(|err| {
                     // Note:
                     // The final goal we would like to make all parse_macro success,
                     // such that the following log will not call anyway.
-                    log::warn!("fail on macro_parse: {}", macro_call_id.debug_dump(db));
+                    log::warn!(
+                        "fail on macro_parse: (reason: {}) {}",
+                        err,
+                        macro_call_id.debug_dump(db)
+                    );
 
                     // returning an empty string looks fishy...
                     SourceFile::parse("")
@@ -77,14 +81,20 @@ impl HirFileId {
     }
 }
 
-fn parse_macro(db: &impl DefDatabase, macro_call_id: MacroCallId) -> Option<TreeArc<SourceFile>> {
+fn parse_macro(
+    db: &impl DefDatabase,
+    macro_call_id: MacroCallId,
+) -> Result<TreeArc<SourceFile>, String> {
     let loc = macro_call_id.loc(db);
     let macro_call = loc.ast_id.to_node(db);
-    let (macro_arg, _) = macro_call.token_tree().and_then(mbe::ast_to_token_tree)?;
-
-    let macro_rules = db.macro_def(loc.def)?;
-    let tt = macro_rules.expand(&macro_arg).ok()?;
-    Some(mbe::token_tree_to_ast_item_list(&tt))
+    let (macro_arg, _) = macro_call
+        .token_tree()
+        .and_then(mbe::ast_to_token_tree)
+        .ok_or("Fail to args in to tt::TokenTree")?;
+
+    let macro_rules = db.macro_def(loc.def).ok_or("Fail to find macro definition")?;
+    let tt = macro_rules.expand(&macro_arg).map_err(|err| format!("{:?}", err))?;
+    Ok(mbe::token_tree_to_ast_item_list(&tt))
 }
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
@@ -311,11 +321,18 @@ impl MacroCallId {
     pub fn debug_dump(&self, db: &impl DefDatabase) -> String {
         let loc = self.clone().loc(db);
         let node = loc.ast_id.to_node(db);
-        let syntax_str = node.syntax().to_string();
+        let syntax_str = node.syntax().text().chunks().collect::<Vec<_>>().join(" ");
 
         // dump the file name
         let file_id: HirFileId = self.clone().into();
         let original = file_id.original_file(db);
-        format!("macro call [file: {:#?}] : {}", db.file_relative_path(original), syntax_str)
+        let macro_rules = db.macro_def(loc.def);
+
+        format!(
+            "macro call [file: {:#?}] : {}\nhas rules: {}",
+            db.file_relative_path(original),
+            syntax_str,
+            macro_rules.is_some()
+        )
     }
 }
diff --git a/crates/ra_mbe/src/lib.rs b/crates/ra_mbe/src/lib.rs
index 9aad08db9..e78bc734b 100644
--- a/crates/ra_mbe/src/lib.rs
+++ b/crates/ra_mbe/src/lib.rs
@@ -220,9 +220,10 @@ impl_froms!(TokenTree: Leaf, Subtree);
         let expansion = syntax_node_to_token_tree(expansion.syntax()).unwrap().0;
         let file = token_tree_to_macro_items(&expansion);
         let file = file.unwrap().syntax().debug_dump().trim().to_string();
-        let file = file.replace("C_C__C", "$crate");
+        let tree = tree.unwrap().syntax().debug_dump().trim().to_string();
 
-        assert_eq!(tree.unwrap().syntax().debug_dump().trim(), file,);
+        let file = file.replace("C_C__C", "$crate");
+        assert_eq!(tree, file,);
     }
 
     #[test]
@@ -349,6 +350,21 @@ impl_froms!(TokenTree: Leaf, Subtree);
     }
 
     #[test]
+    fn test_match_group_pattern_with_multiple_statement_without_semi() {
+        let rules = create_rules(
+            r#"
+        macro_rules! foo {
+            ($ ($ i:ident),*) => ( fn baz { $ (
+                $i()
+            );*} );            
+        }
+"#,
+        );
+
+        assert_expansion(&rules, "foo! { foo, bar }", "fn baz {foo () ; bar () ;}");
+    }
+
+    #[test]
     fn test_match_group_empty_fixed_token() {
         let rules = create_rules(
             r#"
@@ -692,6 +708,33 @@ MACRO_ITEMS@[0; 40)
     }
 
     #[test]
+    fn test_ty_with_complex_type() {
+        let rules = create_rules(
+            r#"
+        macro_rules! foo {
+            ($ i:ty) => (
+                fn bar() -> $ i { unimplemented!() }
+            )
+        }
+"#,
+        );
+
+        // Reference lifetime struct with generic type
+        assert_expansion(
+            &rules,
+            "foo! { &'a Baz<u8> }",
+            "fn bar () -> & 'a Baz < u8 > {unimplemented ! ()}",
+        );
+
+        // extern "Rust" func type
+        assert_expansion(
+            &rules,
+            r#"foo! { extern "Rust" fn() -> Ret }"#,
+            r#"fn bar () -> extern "Rust" fn () -> Ret {unimplemented ! ()}"#,
+        );
+    }
+
+    #[test]
     fn test_pat_() {
         let rules = create_rules(
             r#"
@@ -854,6 +897,26 @@ MACRO_ITEMS@[0; 40)
 
     // The following tests are based on real world situations
     #[test]
+    fn test_vec() {
+        let rules = create_rules(
+            r#"
+         macro_rules! vec {
+            ($($item:expr),*) => {
+                {
+                    let mut v = Vec::new();
+                    $(
+                        v.push($item);
+                    )*
+                    v
+                }
+            };
+}
+"#,
+        );
+        assert_expansion(&rules, r#"vec!();"#, r#"{let mut v = Vec :: new () ;  v}"#);
+    }
+
+    #[test]
     fn test_winapi_struct() {
         // from https://github.com/retep998/winapi-rs/blob/a7ef2bca086aae76cf6c4ce4c2552988ed9798ad/src/macros.rs#L366
 
@@ -886,4 +949,26 @@ macro_rules! STRUCT {
         assert_expansion(&rules, r#"STRUCT!{#[cfg_attr(target_arch = "x86", repr(packed))] struct D3DCONTENTPROTECTIONCAPS {Caps : u8 ,}}"#, 
         "# [repr (C)] # [derive (Copy)] # [cfg_attr (target_arch = \"x86\" , repr (packed))] pub struct D3DCONTENTPROTECTIONCAPS {pub Caps : u8 ,} impl Clone for D3DCONTENTPROTECTIONCAPS {# [inline] fn clone (& self) -> D3DCONTENTPROTECTIONCAPS {* self}} # [cfg (feature = \"impl-default\")] impl Default for D3DCONTENTPROTECTIONCAPS {# [inline] fn default () -> D3DCONTENTPROTECTIONCAPS {unsafe {$crate :: _core :: mem :: zeroed ()}}}");
     }
+
+    #[test]
+    fn test_int_base() {
+        let rules = create_rules(
+            r#"
+macro_rules! int_base {
+    ($Trait:ident for $T:ident as $U:ident -> $Radix:ident) => {
+        #[stable(feature = "rust1", since = "1.0.0")]
+        impl fmt::$Trait for $T {
+            fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+                $Radix.fmt_int(*self as $U, f)
+            }
+        }
+    }
+}            
+"#,
+        );
+
+        assert_expansion(&rules, r#" int_base!{Binary for isize as usize -> Binary}"#, 
+        "# [stable (feature = \"rust1\" , since = \"1.0.0\")] impl fmt :: Binary for isize {fn fmt (& self , f : & mut fmt :: Formatter < \'_ >) -> fmt :: Result {Binary . fmt_int (* self as usize , f)}}"
+        );
+    }
 }
diff --git a/crates/ra_mbe/src/mbe_expander.rs b/crates/ra_mbe/src/mbe_expander.rs
index 66ea76698..00fb09a3b 100644
--- a/crates/ra_mbe/src/mbe_expander.rs
+++ b/crates/ra_mbe/src/mbe_expander.rs
@@ -221,11 +221,13 @@ fn match_lhs(pattern: &crate::Subtree, input: &mut TtCursor) -> Result<Bindings,
                 }
                 _ => return Err(ExpandError::UnexpectedToken),
             },
-            crate::TokenTree::Repeat(crate::Repeat { subtree, kind: _, separator }) => {
+            crate::TokenTree::Repeat(crate::Repeat { subtree, kind, separator }) => {
                 // Dirty hack to make macro-expansion terminate.
                 // This should be replaced by a propper macro-by-example implementation
                 let mut limit = 128;
+                let mut counter = 0;
                 while let Ok(nested) = match_lhs(subtree, input) {
+                    counter += 1;
                     limit -= 1;
                     if limit == 0 {
                         break;
@@ -239,6 +241,17 @@ fn match_lhs(pattern: &crate::Subtree, input: &mut TtCursor) -> Result<Bindings,
                         }
                     }
                 }
+
+                match kind {
+                    crate::RepeatKind::OneOrMore if counter == 0 => {
+                        return Err(ExpandError::UnexpectedToken);
+                    }
+                    crate::RepeatKind::ZeroOrOne if counter > 1 => {
+                        return Err(ExpandError::UnexpectedToken);
+                    }
+
+                    _ => {}
+                }
             }
             crate::TokenTree::Subtree(subtree) => {
                 let input_subtree =
@@ -274,6 +287,20 @@ fn expand_subtree(
     Ok(tt::Subtree { token_trees, delimiter: template.delimiter })
 }
 
+/// Reduce single token subtree to single token
+/// In `tt` matcher case, all tt tokens will be braced by a Delimiter::None
+/// which makes all sort of problems.
+fn reduce_single_token(mut subtree: tt::Subtree) -> tt::TokenTree {
+    if subtree.delimiter != tt::Delimiter::None || subtree.token_trees.len() != 1 {
+        return subtree.into();
+    }
+
+    match subtree.token_trees.pop().unwrap() {
+        tt::TokenTree::Subtree(subtree) => reduce_single_token(subtree),
+        tt::TokenTree::Leaf(token) => token.into(),
+    }
+}
+
 fn expand_tt(
     template: &crate::TokenTree,
     bindings: &Bindings,
@@ -282,11 +309,13 @@ fn expand_tt(
     let res: tt::TokenTree = match template {
         crate::TokenTree::Subtree(subtree) => expand_subtree(subtree, bindings, nesting)?.into(),
         crate::TokenTree::Repeat(repeat) => {
-            let mut token_trees = Vec::new();
+            let mut token_trees: Vec<tt::TokenTree> = Vec::new();
             nesting.push(0);
             // Dirty hack to make macro-expansion terminate.
             // This should be replaced by a propper macro-by-example implementation
             let mut limit = 128;
+            let mut has_sep = false;
+
             while let Ok(t) = expand_subtree(&repeat.subtree, bindings, nesting) {
                 limit -= 1;
                 if limit == 0 {
@@ -294,10 +323,26 @@ fn expand_tt(
                 }
                 let idx = nesting.pop().unwrap();
                 nesting.push(idx + 1);
-                token_trees.push(t.into())
+                token_trees.push(reduce_single_token(t).into());
+
+                if let Some(sep) = repeat.separator {
+                    let punct =
+                        tt::Leaf::from(tt::Punct { char: sep, spacing: tt::Spacing::Alone });
+                    token_trees.push(punct.into());
+                    has_sep = true;
+                }
             }
             nesting.pop().unwrap();
-            tt::Subtree { token_trees, delimiter: tt::Delimiter::None }.into()
+
+            // Dirty hack for remove the last sep
+            // if it is a "," undo the push
+            if has_sep && repeat.separator.unwrap() == ',' {
+                token_trees.pop();
+            }
+
+            // Check if it is a singel token subtree without any delimiter
+            // e.g {Delimiter:None> ['>'] /Delimiter:None>}
+            reduce_single_token(tt::Subtree { token_trees, delimiter: tt::Delimiter::None })
         }
         crate::TokenTree::Leaf(leaf) => match leaf {
             crate::Leaf::Ident(ident) => {
@@ -311,7 +356,13 @@ fn expand_tt(
                     tt::Leaf::from(tt::Ident { text: "$crate".into(), id: TokenId::unspecified() })
                         .into()
                 } else {
-                    bindings.get(&v.text, nesting)?.clone()
+                    let tkn = bindings.get(&v.text, nesting)?.clone();
+
+                    if let tt::TokenTree::Subtree(subtree) = tkn {
+                        reduce_single_token(subtree)
+                    } else {
+                        tkn
+                    }
                 }
             }
             crate::Leaf::Literal(l) => tt::Leaf::from(tt::Literal { text: l.text.clone() }).into(),
diff --git a/crates/ra_mbe/src/mbe_parser.rs b/crates/ra_mbe/src/mbe_parser.rs
index f37c422d3..0710062d9 100644
--- a/crates/ra_mbe/src/mbe_parser.rs
+++ b/crates/ra_mbe/src/mbe_parser.rs
@@ -20,15 +20,15 @@ pub(crate) fn parse(tt: &tt::Subtree) -> Result<crate::MacroRules, ParseError> {
 }
 
 fn parse_rule(p: &mut TtCursor) -> Result<crate::Rule, ParseError> {
-    let lhs = parse_subtree(p.eat_subtree()?)?;
+    let lhs = parse_subtree(p.eat_subtree()?, false)?;
     p.expect_char('=')?;
     p.expect_char('>')?;
-    let mut rhs = parse_subtree(p.eat_subtree()?)?;
+    let mut rhs = parse_subtree(p.eat_subtree()?, true)?;
     rhs.delimiter = crate::Delimiter::None;
     Ok(crate::Rule { lhs, rhs })
 }
 
-fn parse_subtree(tt: &tt::Subtree) -> Result<crate::Subtree, ParseError> {
+fn parse_subtree(tt: &tt::Subtree, transcriber: bool) -> Result<crate::Subtree, ParseError> {
     let mut token_trees = Vec::new();
     let mut p = TtCursor::new(tt);
     while let Some(tt) = p.eat() {
@@ -36,9 +36,9 @@ fn parse_subtree(tt: &tt::Subtree) -> Result<crate::Subtree, ParseError> {
             tt::TokenTree::Leaf(leaf) => match leaf {
                 tt::Leaf::Punct(tt::Punct { char: '$', .. }) => {
                     if p.at_ident().is_some() {
-                        crate::Leaf::from(parse_var(&mut p)?).into()
+                        crate::Leaf::from(parse_var(&mut p, transcriber)?).into()
                     } else {
-                        parse_repeat(&mut p)?.into()
+                        parse_repeat(&mut p, transcriber)?.into()
                     }
                 }
                 tt::Leaf::Punct(punct) => crate::Leaf::from(*punct).into(),
@@ -49,17 +49,17 @@ fn parse_subtree(tt: &tt::Subtree) -> Result<crate::Subtree, ParseError> {
                     crate::Leaf::from(crate::Literal { text: text.clone() }).into()
                 }
             },
-            tt::TokenTree::Subtree(subtree) => parse_subtree(&subtree)?.into(),
+            tt::TokenTree::Subtree(subtree) => parse_subtree(&subtree, transcriber)?.into(),
         };
         token_trees.push(child);
     }
     Ok(crate::Subtree { token_trees, delimiter: tt.delimiter })
 }
 
-fn parse_var(p: &mut TtCursor) -> Result<crate::Var, ParseError> {
+fn parse_var(p: &mut TtCursor, transcriber: bool) -> Result<crate::Var, ParseError> {
     let ident = p.eat_ident().unwrap();
     let text = ident.text.clone();
-    let kind = if p.at_char(':') {
+    let kind = if !transcriber && p.at_char(':') {
         p.bump();
         if let Some(ident) = p.eat_ident() {
             Some(ident.text.clone())
@@ -70,12 +70,13 @@ fn parse_var(p: &mut TtCursor) -> Result<crate::Var, ParseError> {
     } else {
         None
     };
+
     Ok(crate::Var { text, kind })
 }
 
-fn parse_repeat(p: &mut TtCursor) -> Result<crate::Repeat, ParseError> {
+fn parse_repeat(p: &mut TtCursor, transcriber: bool) -> Result<crate::Repeat, ParseError> {
     let subtree = p.eat_subtree().unwrap();
-    let mut subtree = parse_subtree(subtree)?;
+    let mut subtree = parse_subtree(subtree, transcriber)?;
     subtree.delimiter = crate::Delimiter::None;
     let sep = p.eat_punct().ok_or(ParseError::Expected(String::from("separator")))?;
     let (separator, rep) = match sep.char {
diff --git a/crates/ra_mbe/src/subtree_source.rs b/crates/ra_mbe/src/subtree_source.rs
index 16a053b49..20da1e9f7 100644
--- a/crates/ra_mbe/src/subtree_source.rs
+++ b/crates/ra_mbe/src/subtree_source.rs
@@ -21,6 +21,7 @@ impl<'a> From<&'a [tt::TokenTree]> for TokenSeq<'a> {
     }
 }
 
+#[derive(Debug)]
 enum DelimToken<'a> {
     Delim(&'a tt::Delimiter, bool),
     Token(&'a tt::TokenTree),
@@ -52,10 +53,10 @@ impl<'a> TokenSeq<'a> {
         }
     }
 
-    fn child_slice(&self) -> &[tt::TokenTree] {
+    fn child_slice(&self, pos: usize) -> &[tt::TokenTree] {
         match self {
-            TokenSeq::Subtree(subtree) => &subtree.token_trees,
-            TokenSeq::Seq(tokens) => &tokens,
+            TokenSeq::Subtree(subtree) => &subtree.token_trees[pos - 1..],
+            TokenSeq::Seq(tokens) => &tokens[pos..],
         }
     }
 }
@@ -114,7 +115,7 @@ impl<'a> SubTreeWalker<'a> {
                     WalkCursor::Token(0, convert_delim(subtree.delimiter, false))
                 }
                 tt::TokenTree::Leaf(leaf) => {
-                    let next_tokens = self.ts.child_slice();
+                    let next_tokens = self.ts.child_slice(0);
                     WalkCursor::Token(0, convert_leaf(&next_tokens, leaf))
                 }
             },
@@ -190,8 +191,8 @@ impl<'a> SubTreeWalker<'a> {
                     WalkCursor::Token(new_idx, convert_delim(subtree.delimiter, backward))
                 }
                 tt::TokenTree::Leaf(leaf) => {
-                    let next_tokens = top.child_slice();
-                    WalkCursor::Token(pos, convert_leaf(&next_tokens[pos..], leaf))
+                    let next_tokens = top.child_slice(pos);
+                    WalkCursor::Token(pos, convert_leaf(&next_tokens, leaf))
                 }
             },
             DelimToken::Delim(delim, is_end) => {
@@ -429,7 +430,12 @@ fn convert_literal(l: &tt::Literal) -> TtToken {
 }
 
 fn convert_ident(ident: &tt::Ident) -> TtToken {
-    let kind = SyntaxKind::from_keyword(ident.text.as_str()).unwrap_or(IDENT);
+    let kind = if let Some('\'') = ident.text.chars().next() {
+        LIFETIME
+    } else {
+        SyntaxKind::from_keyword(ident.text.as_str()).unwrap_or(IDENT)
+    };
+
     TtToken { kind, is_joint_to_next: false, text: ident.text.clone(), n_tokens: 1 }
 }