Repository: rust-analyzer/rust-analyzer
Pull request #3577 (2020-03-13 13:01:19)

Protect against infinite macro expansion in def collector
Something I noticed while trying to make macro expansion more resilient against errors.

There was a test for this, but it wasn't actually working because the first recursive expansion failed. (The comma...)

Even with this limit, that test (when fixed) still takes some time to pass because of the exponential growth of the expansions, so I disabled it and added a different one without growth.

CC @edwin0cheng 

edwin0cheng: Just a small nit, and otherwise LGTM.

Patch:
diff --git a/crates/ra_hir_def/src/nameres/collector.rs b/crates/ra_hir_def/src/nameres/collector.rs
index d0459d9b09..db9838cb5e 100644
--- a/crates/ra_hir_def/src/nameres/collector.rs
+++ b/crates/ra_hir_def/src/nameres/collector.rs
@@ -102,6 +102,7 @@ struct MacroDirective {
     module_id: LocalModuleId,
     ast_id: AstIdWithPath<ast::MacroCall>,
     legacy: Option<MacroCallId>,
+    depth: usize,
 }
 
 #[derive(Clone, Debug, Eq, PartialEq)]
@@ -134,6 +135,7 @@ where
         self.def_map.modules[module_id].origin = ModuleOrigin::CrateRoot { definition: file_id };
         ModCollector {
             def_collector: &mut *self,
+            macro_depth: 0,
             module_id,
             file_id: file_id.into(),
             raw_items: &raw_items,
@@ -516,7 +518,7 @@ where
         macros.retain(|directive| {
             if let Some(call_id) = directive.legacy {
                 res = ReachedFixedPoint::No;
-                resolved.push((directive.module_id, call_id));
+                resolved.push((directive.module_id, call_id, directive.depth));
                 return false;
             }
 
@@ -530,7 +532,7 @@ where
                 );
                 resolved_res.resolved_def.take_macros()
             }) {
-                resolved.push((directive.module_id, call_id));
+                resolved.push((directive.module_id, call_id, directive.depth));
                 res = ReachedFixedPoint::No;
                 return false;
             }
@@ -541,7 +543,7 @@ where
             if let Some(call_id) =
                 directive.ast_id.as_call_id(self.db, |path| self.resolve_attribute_macro(&path))
             {
-                resolved.push((directive.module_id, call_id));
+                resolved.push((directive.module_id, call_id, 0));
                 res = ReachedFixedPoint::No;
                 return false;
             }
@@ -552,8 +554,12 @@ where
         self.unexpanded_macros = macros;
         self.unexpanded_attribute_macros = attribute_macros;
 
-        for (module_id, macro_call_id) in resolved {
-            self.collect_macro_expansion(module_id, macro_call_id);
+        for (module_id, macro_call_id, depth) in resolved {
+            if depth > 1024 {
+                log::debug!("Max macro expansion depth reached");
+                continue;
+            }
+            self.collect_macro_expansion(module_id, macro_call_id, depth);
         }
 
         res
@@ -573,12 +579,18 @@ where
         None
     }
 
-    fn collect_macro_expansion(&mut self, module_id: LocalModuleId, macro_call_id: MacroCallId) {
+    fn collect_macro_expansion(
+        &mut self,
+        module_id: LocalModuleId,
+        macro_call_id: MacroCallId,
+        depth: usize,
+    ) {
         let file_id: HirFileId = macro_call_id.as_file();
         let raw_items = self.db.raw_items(file_id);
         let mod_dir = self.mod_dirs[&module_id].clone();
         ModCollector {
             def_collector: &mut *self,
+            macro_depth: depth,
             file_id,
             module_id,
             raw_items: &raw_items,
@@ -595,6 +607,7 @@ where
 /// Walks a single module, populating defs, imports and macros
 struct ModCollector<'a, D> {
     def_collector: D,
+    macro_depth: usize,
     module_id: LocalModuleId,
     file_id: HirFileId,
     raw_items: &'a raw::RawItems,
@@ -684,6 +697,7 @@ where
 
                 ModCollector {
                     def_collector: &mut *self.def_collector,
+                    macro_depth: self.macro_depth,
                     module_id,
                     file_id: self.file_id,
                     raw_items: self.raw_items,
@@ -713,6 +727,7 @@ where
                         let raw_items = self.def_collector.db.raw_items(file_id.into());
                         ModCollector {
                             def_collector: &mut *self.def_collector,
+                            macro_depth: self.macro_depth,
                             module_id,
                             file_id: file_id.into(),
                             raw_items: &raw_items,
@@ -887,6 +902,7 @@ where
                 module_id: self.module_id,
                 ast_id,
                 legacy: Some(macro_call_id),
+                depth: self.macro_depth + 1,
             });
 
             return;
@@ -902,6 +918,7 @@ where
             module_id: self.module_id,
             ast_id,
             legacy: None,
+            depth: self.macro_depth + 1,
         });
     }
 
@@ -971,13 +988,26 @@ mod tests {
     }
 
     #[test]
-    fn test_macro_expand_will_stop() {
+    fn test_macro_expand_will_stop_1() {
+        do_resolve(
+            r#"
+        macro_rules! foo {
+            ($($ty:ty)*) => { foo!($($ty)*); }
+        }
+        foo!(KABOOM);
+        "#,
+        );
+    }
+
+    #[ignore] // this test does succeed, but takes quite a while :/
+    #[test]
+    fn test_macro_expand_will_stop_2() {
         do_resolve(
             r#"
         macro_rules! foo {
-            ($($ty:ty)*) => { foo!($($ty)*, $($ty)*); }
+            ($($ty:ty)*) => { foo!($($ty)* $($ty)*); }
         }
-foo!(KABOOM);
+        foo!(KABOOM);
         "#,
         );
     }
diff --git a/crates/ra_parser/src/grammar/params.rs b/crates/ra_parser/src/grammar/params.rs
index f0da173cc1..3ca32185c1 100644
--- a/crates/ra_parser/src/grammar/params.rs
+++ b/crates/ra_parser/src/grammar/params.rs
@@ -56,17 +56,21 @@ fn list_(p: &mut Parser, flavor: Flavor) {
         // fn f(#[attr1] pat: Type) {}
         attributes::outer_attributes(p);
 
+        // test param_list_vararg
+        // extern "C" { fn printf(format: *const i8, ...) -> i32; }
+        match flavor {
+            FnDef | FnPointer if p.eat(T![...]) => break,
+            _ => (),
+        }
+
         if !p.at_ts(VALUE_PARAMETER_FIRST) {
             p.error("expected value parameter");
             break;
         }
-        let param = value_parameter(p, flavor);
+        value_parameter(p, flavor);
         if !p.at(ket) {
             p.expect(T![,]);
         }
-        if let Variadic(true) = param {
-            break;
-        }
     }
 
     p.expect(ket);
@@ -75,25 +79,14 @@ fn list_(p: &mut Parser, flavor: Flavor) {
 
 const VALUE_PARAMETER_FIRST: TokenSet = patterns::PATTERN_FIRST.union(types::TYPE_FIRST);
 
-struct Variadic(bool);
-
-fn value_parameter(p: &mut Parser, flavor: Flavor) -> Variadic {
-    let mut res = Variadic(false);
+fn value_parameter(p: &mut Parser, flavor: Flavor) {
     let m = p.start();
     match flavor {
-        // test param_list_vararg
-        // extern "C" { fn printf(format: *const i8, ...) -> i32; }
-        Flavor::FnDef | Flavor::FnPointer if p.eat(T![...]) => res = Variadic(true),
-
         // test fn_def_param
         // fn foo((x, y): (i32, i32)) {}
         Flavor::FnDef => {
             patterns::pattern(p);
-            if variadic_param(p) {
-                res = Variadic(true)
-            } else {
-                types::ascription(p);
-            }
+            types::ascription(p);
         }
         // test value_parameters_no_patterns
         // type F = Box<Fn(i32, &i32, &i32, ())>;
@@ -109,11 +102,7 @@ fn value_parameter(p: &mut Parser, flavor: Flavor) -> Variadic {
         Flavor::FnPointer => {
             if (p.at(IDENT) || p.at(UNDERSCORE)) && p.nth(1) == T![:] && !p.nth_at(1, T![::]) {
                 patterns::pattern_single(p);
-                if variadic_param(p) {
-                    res = Variadic(true)
-                } else {
-                    types::ascription(p);
-                }
+                types::ascription(p);
             } else {
                 types::type_(p);
             }
@@ -130,17 +119,6 @@ fn value_parameter(p: &mut Parser, flavor: Flavor) -> Variadic {
         }
     }
     m.complete(p, PARAM);
-    res
-}
-
-fn variadic_param(p: &mut Parser) -> bool {
-    if p.at(T![:]) && p.nth_at(1, T![...]) {
-        p.bump(T![:]);
-        p.bump(T![...]);
-        true
-    } else {
-        false
-    }
 }
 
 // test self_param


Test patch:
diff --git a/crates/ra_syntax/src/tests.rs b/crates/ra_syntax/src/tests.rs
index 6a8cb6bb57..d331d541eb 100644
--- a/crates/ra_syntax/src/tests.rs
+++ b/crates/ra_syntax/src/tests.rs
@@ -34,7 +34,6 @@ fn main() {
     "##;
 
     let parse = SourceFile::parse(code);
-    // eprintln!("{:#?}", parse.syntax_node());
     assert!(parse.ok().is_ok());
 }
 
diff --git a/crates/ra_syntax/test_data/parser/inline/ok/0032_fn_pointer_type.txt b/crates/ra_syntax/test_data/parser/inline/ok/0032_fn_pointer_type.txt
index 4c17f0db81..a0a8aea764 100644
--- a/crates/ra_syntax/test_data/parser/inline/ok/0032_fn_pointer_type.txt
+++ b/crates/ra_syntax/test_data/parser/inline/ok/0032_fn_pointer_type.txt
@@ -81,8 +81,7 @@ SOURCE_FILE@[0; 113)
         WHITESPACE@[97; 98) " "
         COMMA@[98; 99) ","
         WHITESPACE@[99; 100) " "
-        PARAM@[100; 103)
-          DOTDOTDOT@[100; 103) "..."
+        DOTDOTDOT@[100; 103) "..."
         WHITESPACE@[103; 104) " "
         R_PAREN@[104; 105) ")"
       WHITESPACE@[105; 106) " "
diff --git a/crates/ra_syntax/test_data/parser/inline/ok/0123_param_list_vararg.txt b/crates/ra_syntax/test_data/parser/inline/ok/0123_param_list_vararg.txt
index 6c3b17868c..836e8e55b6 100644
--- a/crates/ra_syntax/test_data/parser/inline/ok/0123_param_list_vararg.txt
+++ b/crates/ra_syntax/test_data/parser/inline/ok/0123_param_list_vararg.txt
@@ -32,8 +32,7 @@ SOURCE_FILE@[0; 57)
                       IDENT@[38; 40) "i8"
           COMMA@[40; 41) ","
           WHITESPACE@[41; 42) " "
-          PARAM@[42; 45)
-            DOTDOTDOT@[42; 45) "..."
+          DOTDOTDOT@[42; 45) "..."
           R_PAREN@[45; 46) ")"
         WHITESPACE@[46; 47) " "
         RET_TYPE@[47; 53)
diff --git a/crates/ra_syntax/test_data/parser/ok/0051_parameter_attrs.txt b/crates/ra_syntax/test_data/parser/ok/0051_parameter_attrs.txt
index 254eafc364..719c99c17f 100644
--- a/crates/ra_syntax/test_data/parser/ok/0051_parameter_attrs.txt
+++ b/crates/ra_syntax/test_data/parser/ok/0051_parameter_attrs.txt
@@ -118,8 +118,7 @@ SOURCE_FILE@[0; 519)
                   IDENT@[108; 112) "attr"
             R_BRACK@[112; 113) "]"
           WHITESPACE@[113; 114) " "
-          PARAM@[114; 117)
-            DOTDOTDOT@[114; 117) "..."
+          DOTDOTDOT@[114; 117) "..."
           R_PAREN@[117; 118) ")"
         WHITESPACE@[118; 119) " "
         RET_TYPE@[119; 125)
diff --git a/crates/ra_syntax/test_data/parser/ok/0063_variadic_fun.rs b/crates/ra_syntax/test_data/parser/ok/0063_variadic_fun.rs
deleted file mode 100644
index a16afbaf38..0000000000
--- a/crates/ra_syntax/test_data/parser/ok/0063_variadic_fun.rs
+++ /dev/null
@@ -1,5 +0,0 @@
-extern "C" {
-    fn a(_: *mut u8, ...,);
-    fn b(_: *mut u8, _: ...);
-    fn c(_: *mut u8, #[cfg(never)] [w, t, f]: ...,);
-}
diff --git a/crates/ra_syntax/test_data/parser/ok/0063_variadic_fun.txt b/crates/ra_syntax/test_data/parser/ok/0063_variadic_fun.txt
deleted file mode 100644
index 186f036269..0000000000
--- a/crates/ra_syntax/test_data/parser/ok/0063_variadic_fun.txt
+++ /dev/null
@@ -1,133 +0,0 @@
-SOURCE_FILE@[0; 126)
-  EXTERN_BLOCK@[0; 125)
-    ABI@[0; 10)
-      EXTERN_KW@[0; 6) "extern"
-      WHITESPACE@[6; 7) " "
-      STRING@[7; 10) "\"C\""
-    WHITESPACE@[10; 11) " "
-    EXTERN_ITEM_LIST@[11; 125)
-      L_CURLY@[11; 12) "{"
-      WHITESPACE@[12; 17) "\n    "
-      FN_DEF@[17; 40)
-        FN_KW@[17; 19) "fn"
-        WHITESPACE@[19; 20) " "
-        NAME@[20; 21)
-          IDENT@[20; 21) "a"
-        PARAM_LIST@[21; 39)
-          L_PAREN@[21; 22) "("
-          PARAM@[22; 32)
-            PLACEHOLDER_PAT@[22; 23)
-              UNDERSCORE@[22; 23) "_"
-            COLON@[23; 24) ":"
-            WHITESPACE@[24; 25) " "
-            POINTER_TYPE@[25; 32)
-              STAR@[25; 26) "*"
-              MUT_KW@[26; 29) "mut"
-              WHITESPACE@[29; 30) " "
-              PATH_TYPE@[30; 32)
-                PATH@[30; 32)
-                  PATH_SEGMENT@[30; 32)
-                    NAME_REF@[30; 32)
-                      IDENT@[30; 32) "u8"
-          COMMA@[32; 33) ","
-          WHITESPACE@[33; 34) " "
-          PARAM@[34; 37)
-            DOTDOTDOT@[34; 37) "..."
-          COMMA@[37; 38) ","
-          R_PAREN@[38; 39) ")"
-        SEMI@[39; 40) ";"
-      WHITESPACE@[40; 45) "\n    "
-      FN_DEF@[45; 70)
-        FN_KW@[45; 47) "fn"
-        WHITESPACE@[47; 48) " "
-        NAME@[48; 49)
-          IDENT@[48; 49) "b"
-        PARAM_LIST@[49; 69)
-          L_PAREN@[49; 50) "("
-          PARAM@[50; 60)
-            PLACEHOLDER_PAT@[50; 51)
-              UNDERSCORE@[50; 51) "_"
-            COLON@[51; 52) ":"
-            WHITESPACE@[52; 53) " "
-            POINTER_TYPE@[53; 60)
-              STAR@[53; 54) "*"
-              MUT_KW@[54; 57) "mut"
-              WHITESPACE@[57; 58) " "
-              PATH_TYPE@[58; 60)
-                PATH@[58; 60)
-                  PATH_SEGMENT@[58; 60)
-                    NAME_REF@[58; 60)
-                      IDENT@[58; 60) "u8"
-          COMMA@[60; 61) ","
-          WHITESPACE@[61; 62) " "
-          PARAM@[62; 68)
-            PLACEHOLDER_PAT@[62; 63)
-              UNDERSCORE@[62; 63) "_"
-            COLON@[63; 64) ":"
-            WHITESPACE@[64; 65) " "
-            DOTDOTDOT@[65; 68) "..."
-          R_PAREN@[68; 69) ")"
-        SEMI@[69; 70) ";"
-      WHITESPACE@[70; 75) "\n    "
-      FN_DEF@[75; 123)
-        FN_KW@[75; 77) "fn"
-        WHITESPACE@[77; 78) " "
-        NAME@[78; 79)
-          IDENT@[78; 79) "c"
-        PARAM_LIST@[79; 122)
-          L_PAREN@[79; 80) "("
-          PARAM@[80; 90)
-            PLACEHOLDER_PAT@[80; 81)
-              UNDERSCORE@[80; 81) "_"
-            COLON@[81; 82) ":"
-            WHITESPACE@[82; 83) " "
-            POINTER_TYPE@[83; 90)
-              STAR@[83; 84) "*"
-              MUT_KW@[84; 87) "mut"
-              WHITESPACE@[87; 88) " "
-              PATH_TYPE@[88; 90)
-                PATH@[88; 90)
-                  PATH_SEGMENT@[88; 90)
-                    NAME_REF@[88; 90)
-                      IDENT@[88; 90) "u8"
-          COMMA@[90; 91) ","
-          WHITESPACE@[91; 92) " "
-          ATTR@[92; 105)
-            POUND@[92; 93) "#"
-            L_BRACK@[93; 94) "["
-            PATH@[94; 97)
-              PATH_SEGMENT@[94; 97)
-                NAME_REF@[94; 97)
-                  IDENT@[94; 97) "cfg"
-            TOKEN_TREE@[97; 104)
-              L_PAREN@[97; 98) "("
-              IDENT@[98; 103) "never"
-              R_PAREN@[103; 104) ")"
-            R_BRACK@[104; 105) "]"
-          WHITESPACE@[105; 106) " "
-          PARAM@[106; 120)
-            SLICE_PAT@[106; 115)
-              L_BRACK@[106; 107) "["
-              BIND_PAT@[107; 108)
-                NAME@[107; 108)
-                  IDENT@[107; 108) "w"
-              COMMA@[108; 109) ","
-              WHITESPACE@[109; 110) " "
-              BIND_PAT@[110; 111)
-                NAME@[110; 111)
-                  IDENT@[110; 111) "t"
-              COMMA@[111; 112) ","
-              WHITESPACE@[112; 113) " "
-              BIND_PAT@[113; 114)
-                NAME@[113; 114)
-                  IDENT@[113; 114) "f"
-              R_BRACK@[114; 115) "]"
-            COLON@[115; 116) ":"
-            WHITESPACE@[116; 117) " "
-            DOTDOTDOT@[117; 120) "..."
-          COMMA@[120; 121) ","
-          R_PAREN@[121; 122) ")"
-        SEMI@[122; 123) ";"
-      WHITESPACE@[123; 124) "\n"
-      R_CURLY@[124; 125) "}"
-  WHITESPACE@[125; 126) "\n"