Merge #1229

1229: Mark unused mbe variable as `Binding::Empty` r=matklad a=edwin0cheng

This PR fixes a regression bug in In #1228, which incorrect expand an empty binding :

* Introduce a new Binding type `Binding::Empty`.
* Mark all unused binding variables are empty and error out in expansion instead of just by passing.


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

2 files changed, 61 insertions, 13 deletions

diff --git a/crates/ra_mbe/src/mbe_expander.rs b/crates/ra_mbe/src/mbe_expander.rs
index 8f8a79855..4b007647c 100644
--- a/crates/ra_mbe/src/mbe_expander.rs
+++ b/crates/ra_mbe/src/mbe_expander.rs
@@ -81,9 +81,25 @@ struct Bindings {
 enum Binding {
     Simple(tt::TokenTree),
     Nested(Vec<Binding>),
+    Empty,
 }
 
 impl Bindings {
+    fn push_optional(&mut self, name: &SmolStr) {
+        // FIXME: Do we have a better way to represent an empty token ?
+        // Insert an empty subtree for empty token
+        self.inner.insert(
+            name.clone(),
+            Binding::Simple(
+                tt::Subtree { delimiter: tt::Delimiter::None, token_trees: vec![] }.into(),
+            ),
+        );
+    }
+
+    fn push_empty(&mut self, name: &SmolStr) {
+        self.inner.insert(name.clone(), Binding::Empty);
+    }
+
     fn contains(&self, name: &SmolStr) -> bool {
         self.inner.contains_key(name)
     }
@@ -100,6 +116,12 @@ impl Bindings {
                     "could not find nested binding `{}`",
                     name
                 )))?,
+                Binding::Empty => {
+                    return Err(ExpandError::BindingError(format!(
+                        "could not find empty binding `{}`",
+                        name
+                    )))
+                }
             };
         }
         match b {
@@ -108,6 +130,10 @@ impl Bindings {
                 "expected simple binding, found nested binding `{}`",
                 name
             ))),
+            Binding::Empty => Err(ExpandError::BindingError(format!(
+                "expected simple binding, found empty binding `{}`",
+                name
+            ))),
         }
     }
 
@@ -140,6 +166,24 @@ impl Bindings {
     }
 }
 
+fn collect_vars(subtree: &crate::Subtree) -> Vec<SmolStr> {
+    let mut res = vec![];
+
+    for tkn in subtree.token_trees.iter() {
+        match tkn {
+            crate::TokenTree::Leaf(crate::Leaf::Var(crate::Var { text, .. })) => {
+                res.push(text.clone());
+            }
+            crate::TokenTree::Subtree(subtree) => {
+                res.extend(collect_vars(subtree));
+            }
+            _ => {}
+        }
+    }
+
+    res
+}
+
 fn match_lhs(pattern: &crate::Subtree, input: &mut TtCursor) -> Result<Bindings, ExpandError> {
     let mut res = Bindings::default();
     for pat in pattern.token_trees.iter() {
@@ -217,18 +261,7 @@ fn match_lhs(pattern: &crate::Subtree, input: &mut TtCursor) -> Result<Bindings,
                                 let vis = vis.clone();
                                 res.inner.insert(text.clone(), Binding::Simple(vis.into()));
                             } else {
-                                // FIXME: Do we have a better way to represent an empty token ?
-                                // Insert an empty subtree for empty token
-                                res.inner.insert(
-                                    text.clone(),
-                                    Binding::Simple(
-                                        tt::Subtree {
-                                            delimiter: tt::Delimiter::None,
-                                            token_trees: vec![],
-                                        }
-                                        .into(),
-                                    ),
-                                );
+                                res.push_optional(&text);
                             }
                         }
 
@@ -295,6 +328,10 @@ fn match_lhs(pattern: &crate::Subtree, input: &mut TtCursor) -> Result<Bindings,
                     crate::RepeatKind::OneOrMore if counter == 0 => {
                         return Err(ExpandError::UnexpectedToken);
                     }
+                    _ if counter == 0 => {
+                        // Collect all empty variables in subtrees
+                        collect_vars(subtree).iter().for_each(|s| res.push_empty(s));
+                    }
                     _ => {}
                 }
             }
diff --git a/crates/ra_mbe/src/tests.rs b/crates/ra_mbe/src/tests.rs
index cdbd4dd1c..bd5a44240 100644
--- a/crates/ra_mbe/src/tests.rs
+++ b/crates/ra_mbe/src/tests.rs
@@ -1244,7 +1244,12 @@ fn test_cfg_if_main() {
                     $( ( ($($meta),*) ($($it)*) ), )*
                     ( () ($($it2)*) ),
                 }
-            }
+            };
+
+            // Internal macro to Apply a cfg attribute to a list of items
+            (@__apply $m:meta, $($it:item)*) => {
+                $(#[$m] $it)*
+            };
         }
 "#,
     );
@@ -1262,6 +1267,12 @@ cfg_if !   {
  }        
 "#,         
         "__cfg_if_items ! {() ; ((target_env = \"msvc\") ()) , ((all (target_arch = \"wasm32\" , not (target_os = \"emscripten\"))) ()) , (() (mod libunwind ; pub use libunwind :: * ;)) ,}");
+
+    assert_expansion(MacroKind::Items, &rules, r#"
+cfg_if ! { @ __apply cfg ( all ( not ( any ( not ( any ( target_os = "solaris" , target_os = "illumos" ) ) ) ) ) ) , }    
+"#,
+        ""    
+    );
 }
 
 #[test]