From 9aa6be71a53fd55d8e0a15254b329aab6a5bcb9a Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Mon, 9 Aug 2021 15:41:19 +0300 Subject: [PATCH 1/2] internal: remove useless helpers We generally avoid "syntax only" helper wrappers, which don't do much: they make code easier to write, but harder to read. They also make investigations harder, as "find_usages" needs to be invoked both for the wrapped and unwrapped APIs --- crates/cfg/src/tests.rs | 10 +++++----- crates/hir_def/src/attr.rs | 4 ++-- crates/hir_expand/src/builtin_macro.rs | 2 +- crates/hir_expand/src/db.rs | 4 ++-- crates/hir_expand/src/eager.rs | 2 +- crates/mbe/src/benchmark.rs | 5 ++--- crates/mbe/src/expander.rs | 8 +++++--- crates/mbe/src/lib.rs | 2 +- crates/mbe/src/syntax_bridge.rs | 12 +++--------- crates/mbe/src/tests.rs | 10 ++++++---- crates/mbe/src/tests/rule.rs | 5 ++--- crates/rust-analyzer/src/cargo_target_spec.rs | 4 ++-- 12 files changed, 32 insertions(+), 36 deletions(-) diff --git a/crates/cfg/src/tests.rs b/crates/cfg/src/tests.rs index d8736c893d7b..ea04114d3a3b 100644 --- a/crates/cfg/src/tests.rs +++ b/crates/cfg/src/tests.rs @@ -1,5 +1,5 @@ use expect_test::{expect, Expect}; -use mbe::ast_to_token_tree; +use mbe::syntax_node_to_token_tree; use syntax::{ast, AstNode}; use crate::{CfgAtom, CfgExpr, CfgOptions, DnfExpr}; @@ -8,7 +8,7 @@ fn assert_parse_result(input: &str, expected: CfgExpr) { let (tt, _) = { let source_file = ast::SourceFile::parse(input).ok().unwrap(); let tt = source_file.syntax().descendants().find_map(ast::TokenTree::cast).unwrap(); - ast_to_token_tree(&tt) + syntax_node_to_token_tree(tt.syntax()) }; let cfg = CfgExpr::parse(&tt); assert_eq!(cfg, expected); @@ -18,7 +18,7 @@ fn check_dnf(input: &str, expect: Expect) { let (tt, _) = { let source_file = ast::SourceFile::parse(input).ok().unwrap(); let tt = source_file.syntax().descendants().find_map(ast::TokenTree::cast).unwrap(); - ast_to_token_tree(&tt) + syntax_node_to_token_tree(tt.syntax()) }; let cfg = CfgExpr::parse(&tt); let actual = format!("#![cfg({})]", DnfExpr::new(cfg)); @@ -29,7 +29,7 @@ fn check_why_inactive(input: &str, opts: &CfgOptions, expect: Expect) { let (tt, _) = { let source_file = ast::SourceFile::parse(input).ok().unwrap(); let tt = source_file.syntax().descendants().find_map(ast::TokenTree::cast).unwrap(); - ast_to_token_tree(&tt) + syntax_node_to_token_tree(tt.syntax()) }; let cfg = CfgExpr::parse(&tt); let dnf = DnfExpr::new(cfg); @@ -42,7 +42,7 @@ fn check_enable_hints(input: &str, opts: &CfgOptions, expected_hints: &[&str]) { let (tt, _) = { let source_file = ast::SourceFile::parse(input).ok().unwrap(); let tt = source_file.syntax().descendants().find_map(ast::TokenTree::cast).unwrap(); - ast_to_token_tree(&tt) + syntax_node_to_token_tree(tt.syntax()) }; let cfg = CfgExpr::parse(&tt); let dnf = DnfExpr::new(cfg); diff --git a/crates/hir_def/src/attr.rs b/crates/hir_def/src/attr.rs index 0ef046e46909..b2f8e4d20d90 100644 --- a/crates/hir_def/src/attr.rs +++ b/crates/hir_def/src/attr.rs @@ -12,7 +12,7 @@ use either::Either; use hir_expand::{hygiene::Hygiene, name::AsName, AstId, InFile}; use itertools::Itertools; use la_arena::ArenaMap; -use mbe::{ast_to_token_tree, DelimiterKind}; +use mbe::{syntax_node_to_token_tree, DelimiterKind}; use smallvec::{smallvec, SmallVec}; use syntax::{ ast::{self, AstNode, AttrsOwner}, @@ -679,7 +679,7 @@ impl Attr { }; Some(Interned::new(AttrInput::Literal(value))) } else if let Some(tt) = ast.token_tree() { - Some(Interned::new(AttrInput::TokenTree(ast_to_token_tree(&tt).0))) + Some(Interned::new(AttrInput::TokenTree(syntax_node_to_token_tree(tt.syntax()).0))) } else { None }; diff --git a/crates/hir_expand/src/builtin_macro.rs b/crates/hir_expand/src/builtin_macro.rs index f556bc751dbd..c9149f27a997 100644 --- a/crates/hir_expand/src/builtin_macro.rs +++ b/crates/hir_expand/src/builtin_macro.rs @@ -610,7 +610,7 @@ mod tests { let fragment = crate::to_fragment_kind(¯o_call); let args = macro_call.token_tree().unwrap(); - let parsed_args = mbe::ast_to_token_tree(&args).0; + let parsed_args = mbe::syntax_node_to_token_tree(args.syntax()).0; let call_id = AstId::new(file_id.into(), ast_id_map.ast_id(¯o_call)); let arg_id = db.intern_macro(MacroCallLoc { diff --git a/crates/hir_expand/src/db.rs b/crates/hir_expand/src/db.rs index ee64564430e5..fa693e74ee1b 100644 --- a/crates/hir_expand/src/db.rs +++ b/crates/hir_expand/src/db.rs @@ -281,7 +281,7 @@ fn macro_def(db: &dyn AstDatabase, id: MacroDefId) -> Option> MacroDefKind::Declarative(ast_id) => match ast_id.to_node(db) { ast::Macro::MacroRules(macro_rules) => { let arg = macro_rules.token_tree()?; - let (tt, def_site_token_map) = mbe::ast_to_token_tree(&arg); + let (tt, def_site_token_map) = mbe::syntax_node_to_token_tree(arg.syntax()); let mac = match mbe::MacroRules::parse(&tt) { Ok(it) => it, Err(err) => { @@ -294,7 +294,7 @@ fn macro_def(db: &dyn AstDatabase, id: MacroDefId) -> Option> } ast::Macro::MacroDef(macro_def) => { let arg = macro_def.body()?; - let (tt, def_site_token_map) = mbe::ast_to_token_tree(&arg); + let (tt, def_site_token_map) = mbe::syntax_node_to_token_tree(arg.syntax()); let mac = match mbe::MacroDef::parse(&tt) { Ok(it) => it, Err(err) => { diff --git a/crates/hir_expand/src/eager.rs b/crates/hir_expand/src/eager.rs index 66601f4df37d..052f1bf20e5d 100644 --- a/crates/hir_expand/src/eager.rs +++ b/crates/hir_expand/src/eager.rs @@ -107,7 +107,7 @@ pub fn expand_eager_macro( mut diagnostic_sink: &mut dyn FnMut(mbe::ExpandError), ) -> Result { let parsed_args = diagnostic_sink.option_with( - || Some(mbe::ast_to_token_tree(¯o_call.value.token_tree()?).0), + || Some(mbe::syntax_node_to_token_tree(¯o_call.value.token_tree()?.syntax()).0), || err("malformed macro invocation"), )?; diff --git a/crates/mbe/src/benchmark.rs b/crates/mbe/src/benchmark.rs index 18eb97f0d58a..ffa316dd24c1 100644 --- a/crates/mbe/src/benchmark.rs +++ b/crates/mbe/src/benchmark.rs @@ -8,9 +8,8 @@ use syntax::{ use test_utils::{bench, bench_fixture, skip_slow_tests}; use crate::{ - ast_to_token_tree, parser::{Op, RepeatKind, Separator}, - MacroRules, + syntax_node_to_token_tree, MacroRules, }; #[test] @@ -65,7 +64,7 @@ fn macro_rules_fixtures_tt() -> FxHashMap { .filter_map(ast::MacroRules::cast) .map(|rule| { let id = rule.name().unwrap().to_string(); - let (def_tt, _) = ast_to_token_tree(&rule.token_tree().unwrap()); + let (def_tt, _) = syntax_node_to_token_tree(rule.token_tree().unwrap().syntax()); (id, def_tt) }) .collect() diff --git a/crates/mbe/src/expander.rs b/crates/mbe/src/expander.rs index bfef7f73d504..e9ef8ccecde2 100644 --- a/crates/mbe/src/expander.rs +++ b/crates/mbe/src/expander.rs @@ -120,7 +120,7 @@ mod tests { use syntax::{ast, AstNode}; use super::*; - use crate::ast_to_token_tree; + use crate::syntax_node_to_token_tree; #[test] fn test_expand_rule() { @@ -159,7 +159,8 @@ mod tests { let macro_definition = source_file.syntax().descendants().find_map(ast::MacroRules::cast).unwrap(); - let (definition_tt, _) = ast_to_token_tree(¯o_definition.token_tree().unwrap()); + let (definition_tt, _) = + syntax_node_to_token_tree(macro_definition.token_tree().unwrap().syntax()); crate::MacroRules::parse(&definition_tt).unwrap() } @@ -168,7 +169,8 @@ mod tests { let macro_invocation = source_file.syntax().descendants().find_map(ast::MacroCall::cast).unwrap(); - let (invocation_tt, _) = ast_to_token_tree(¯o_invocation.token_tree().unwrap()); + let (invocation_tt, _) = + syntax_node_to_token_tree(macro_invocation.token_tree().unwrap().syntax()); expand_rules(&rules.rules, &invocation_tt) } diff --git a/crates/mbe/src/lib.rs b/crates/mbe/src/lib.rs index fcc596756ca7..e0bbb825b93b 100644 --- a/crates/mbe/src/lib.rs +++ b/crates/mbe/src/lib.rs @@ -66,7 +66,7 @@ impl fmt::Display for ExpandError { pub use crate::{ syntax_bridge::{ - ast_to_token_tree, parse_exprs_with_sep, parse_to_token_tree, syntax_node_to_token_tree, + parse_exprs_with_sep, parse_to_token_tree, syntax_node_to_token_tree, token_tree_to_syntax_node, }, token_map::TokenMap, diff --git a/crates/mbe/src/syntax_bridge.rs b/crates/mbe/src/syntax_bridge.rs index ae6058cbc889..2aaa440a7124 100644 --- a/crates/mbe/src/syntax_bridge.rs +++ b/crates/mbe/src/syntax_bridge.rs @@ -13,12 +13,6 @@ use tt::buffer::{Cursor, TokenBuffer}; use crate::{subtree_source::SubtreeTokenSource, tt_iter::TtIter}; use crate::{ExpandError, TokenMap}; -/// Convert the syntax tree (what user has written) to a `TokenTree` (what macro -/// will consume). -pub fn ast_to_token_tree(ast: &impl ast::AstNode) -> (tt::Subtree, TokenMap) { - syntax_node_to_token_tree(ast.syntax()) -} - /// Convert the syntax node to a `TokenTree` (what macro /// will consume). pub fn syntax_node_to_token_tree(node: &SyntaxNode) -> (tt::Subtree, TokenMap) { @@ -812,7 +806,7 @@ mod tests { // - T!['}'] // - WHITE_SPACE let token_tree = ast::TokenTree::cast(token_tree).unwrap(); - let tt = ast_to_token_tree(&token_tree).0; + let tt = syntax_node_to_token_tree(token_tree.syntax()).0; assert_eq!(tt.delimiter_kind(), Some(tt::DelimiterKind::Brace)); } @@ -821,7 +815,7 @@ mod tests { fn test_token_tree_multi_char_punct() { let source_file = ast::SourceFile::parse("struct Foo { a: x::Y }").ok().unwrap(); let struct_def = source_file.syntax().descendants().find_map(ast::Struct::cast).unwrap(); - let tt = ast_to_token_tree(&struct_def).0; + let tt = syntax_node_to_token_tree(struct_def.syntax()).0; token_tree_to_syntax_node(&tt, FragmentKind::Item).unwrap(); } @@ -829,7 +823,7 @@ mod tests { fn test_missing_closing_delim() { let source_file = ast::SourceFile::parse("m!(x").tree(); let node = source_file.syntax().descendants().find_map(ast::TokenTree::cast).unwrap(); - let tt = ast_to_token_tree(&node).0.to_string(); + let tt = syntax_node_to_token_tree(node.syntax()).0.to_string(); assert_eq_text!(&*tt, "( x"); } } diff --git a/crates/mbe/src/tests.rs b/crates/mbe/src/tests.rs index 3698ff3f0f43..705cf5a2b16a 100644 --- a/crates/mbe/src/tests.rs +++ b/crates/mbe/src/tests.rs @@ -29,7 +29,8 @@ macro_rules! impl_fixture { let macro_invocation = source_file.syntax().descendants().find_map(ast::MacroCall::cast).unwrap(); - let (invocation_tt, _) = ast_to_token_tree(¯o_invocation.token_tree().unwrap()); + let (invocation_tt, _) = + syntax_node_to_token_tree(macro_invocation.token_tree().unwrap().syntax()); self.rules.expand(&invocation_tt).result() } @@ -100,7 +101,7 @@ macro_rules! impl_fixture { .descendants() .find_map(ast::TokenTree::cast) .unwrap(); - let mut wrapped = ast_to_token_tree(&wrapped).0; + let mut wrapped = syntax_node_to_token_tree(wrapped.syntax()).0; wrapped.delimiter = None; wrapped }; @@ -163,7 +164,8 @@ fn parse_macro_rules_to_tt(ra_fixture: &str) -> tt::Subtree { let macro_definition = source_file.syntax().descendants().find_map(ast::MacroRules::cast).unwrap(); - let (definition_tt, _) = ast_to_token_tree(¯o_definition.token_tree().unwrap()); + let (definition_tt, _) = + syntax_node_to_token_tree(macro_definition.token_tree().unwrap().syntax()); let parsed = parse_to_token_tree( &ra_fixture[macro_definition.token_tree().unwrap().syntax().text_range()], @@ -180,7 +182,7 @@ fn parse_macro_def_to_tt(ra_fixture: &str) -> tt::Subtree { let macro_definition = source_file.syntax().descendants().find_map(ast::MacroDef::cast).unwrap(); - let (definition_tt, _) = ast_to_token_tree(¯o_definition.body().unwrap()); + let (definition_tt, _) = syntax_node_to_token_tree(macro_definition.body().unwrap().syntax()); let parsed = parse_to_token_tree(&ra_fixture[macro_definition.body().unwrap().syntax().text_range()]) diff --git a/crates/mbe/src/tests/rule.rs b/crates/mbe/src/tests/rule.rs index 5c61a98fdf96..691e359e4d7a 100644 --- a/crates/mbe/src/tests/rule.rs +++ b/crates/mbe/src/tests/rule.rs @@ -1,7 +1,5 @@ use syntax::{ast, AstNode}; -use crate::ast_to_token_tree; - use super::*; #[test] @@ -44,6 +42,7 @@ fn parse_macro_arm(arm_definition: &str) -> Result Date: Mon, 9 Aug 2021 16:06:49 +0300 Subject: [PATCH 2/2] fix: avoid pathological macro expansions Today, rust-analyzer (and rustc, and bat, and IntelliJ) fail badly on some kinds of maliciously constructed code, like a deep sequence of nested parenthesis. "Who writes 100k nested parenthesis" you'd ask? Well, in a language with macros, a run-away macro expansion might do that (see the added tests)! Such expansion can be broad, rather than deep, so it bypasses recursion check at the macro-expansion layer, but triggers deep recursion in parser. In the ideal world, the parser would just handle deeply nested structs gracefully. We'll get there some day, but at the moment, let's try to be simple, and just avoid expanding macros with unbalanced parenthesis in the first place. closes #9358 --- Cargo.lock | 1 + crates/hir_def/src/nameres/tests/macros.rs | 18 +++++++++++++++ crates/hir_expand/Cargo.toml | 1 + crates/hir_expand/src/db.rs | 19 +++++++++++++++- crates/ide_completion/src/tests/expression.rs | 22 +------------------ crates/ide_ssr/src/tests.rs | 4 ++-- 6 files changed, 41 insertions(+), 24 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4d42c1c7be88..61180fcb9a89 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -508,6 +508,7 @@ version = "0.0.0" dependencies = [ "base_db", "cfg", + "cov-mark", "either", "expect-test", "la-arena", diff --git a/crates/hir_def/src/nameres/tests/macros.rs b/crates/hir_def/src/nameres/tests/macros.rs index 45448ba81b34..763dff5a2142 100644 --- a/crates/hir_def/src/nameres/tests/macros.rs +++ b/crates/hir_def/src/nameres/tests/macros.rs @@ -1,4 +1,5 @@ use super::*; + use crate::nameres::proc_macro::{ProcMacroDef, ProcMacroKind}; #[test] @@ -1021,3 +1022,20 @@ pub mod prelude { "#]], ) } + +#[test] +fn issue9358_bad_macro_stack_overflow() { + cov_mark::check!(issue9358_bad_macro_stack_overflow); + check( + r#" +macro_rules! m { + ($cond:expr) => { m!($cond, stringify!($cond)) }; + ($cond:expr, $($arg:tt)*) => { $cond }; +} +m!( +"#, + expect![[r#" + crate + "#]], + ) +} diff --git a/crates/hir_expand/Cargo.toml b/crates/hir_expand/Cargo.toml index 92d6c3e96a10..4645970266cc 100644 --- a/crates/hir_expand/Cargo.toml +++ b/crates/hir_expand/Cargo.toml @@ -9,6 +9,7 @@ edition = "2018" doctest = false [dependencies] +cov-mark = "2.0.0-pre.1" log = "0.4.8" either = "1.5.3" rustc-hash = "1.0.0" diff --git a/crates/hir_expand/src/db.rs b/crates/hir_expand/src/db.rs index fa693e74ee1b..a9bf6db476f3 100644 --- a/crates/hir_expand/src/db.rs +++ b/crates/hir_expand/src/db.rs @@ -5,7 +5,7 @@ use std::sync::Arc; use base_db::{salsa, SourceDatabase}; use limit::Limit; use mbe::{ExpandError, ExpandResult}; -use parser::FragmentKind; +use parser::{FragmentKind, T}; use syntax::{ algo::diff, ast::{self, NameOwner}, @@ -273,6 +273,23 @@ fn macro_arg_text(db: &dyn AstDatabase, id: MacroCallId) -> Option { let loc = db.lookup_intern_macro(id); let arg = loc.kind.arg(db)?; let arg = process_macro_input(db, arg, id); + if matches!(loc.kind, MacroCallKind::FnLike { .. }) { + let first = arg.first_child_or_token().map_or(T![.], |it| it.kind()); + let last = arg.last_child_or_token().map_or(T![.], |it| it.kind()); + let well_formed_tt = + matches!((first, last), (T!['('], T![')']) | (T!['['], T![']']) | (T!['{'], T!['}'])); + if !well_formed_tt { + // Don't expand malformed (unbalanced) macro invocations. This is + // less than ideal, but trying to expand unbalanced macro calls + // sometimes produces pathological, deeply nested code which breaks + // all kinds of things. + // + // Some day, we'll have explicit recursion counters for all + // recursive things, at which point this code might be removed. + cov_mark::hit!(issue9358_bad_macro_stack_overflow); + return None; + } + } Some(arg.green().into()) } diff --git a/crates/ide_completion/src/tests/expression.rs b/crates/ide_completion/src/tests/expression.rs index 95aaff01ac3e..9a42dd7b2751 100644 --- a/crates/ide_completion/src/tests/expression.rs +++ b/crates/ide_completion/src/tests/expression.rs @@ -308,27 +308,7 @@ fn quux(x: i32) { m!(x$0 } "#, - expect![[r#" - kw unsafe - kw match - kw while - kw while let - kw loop - kw if - kw if let - kw for - kw true - kw false - kw return - kw self - kw super - kw crate - lc y i32 - bt u32 - lc x i32 - fn quux(…) fn(i32) - ma m!(…) macro_rules! m - "#]], + expect![[r#""#]], ); } diff --git a/crates/ide_ssr/src/tests.rs b/crates/ide_ssr/src/tests.rs index 444c6b0afd0e..0b0c1111c466 100644 --- a/crates/ide_ssr/src/tests.rs +++ b/crates/ide_ssr/src/tests.rs @@ -921,13 +921,13 @@ fn preserves_whitespace_within_macro_expansion() { macro_rules! macro1 { ($a:expr) => {$a} } - fn f() {macro1!(1 * 2 + 3 + 4} + fn f() {macro1!(1 * 2 + 3 + 4)} "#, expect![[r#" macro_rules! macro1 { ($a:expr) => {$a} } - fn f() {macro1!(4 - (3 - 1 * 2)} + fn f() {macro1!(4 - (3 - 1 * 2))} "#]], ) }