diff options
author | bors[bot] <26634292+bors[bot]@users.noreply.github.com> | 2020-03-21 16:02:07 +0000 |
---|---|---|
committer | GitHub <[email protected]> | 2020-03-21 16:02:07 +0000 |
commit | 50c6a315ab2bef493b618742989744fb370a32ca (patch) | |
tree | fc4ee3a02310c55167d62d4bd95931d161178155 | |
parent | 5e827bd948ea206328b126345ab8e909978b3b9b (diff) | |
parent | 9ff50d7e839c635632021d47e2dd3982916e4738 (diff) |
Merge #3671
3671: Add identity expansion checking in ill-form expansion r=flodiebold a=edwin0cheng
This PR try to add more checking code in error case in macro expansion. The bug in #3642 is introduced by #3580 , which allow ill-form macro expansion in *all* kind of macro expansions.
In general we should separate hypothetical macro expansion and the actual macro expansion call. However, currently the `Semantic` workflow we are using only support single macro expansion type, we might want to review it and make it works in both ways. (Maybe add a field in `MacroCallLoc` for differentiation)
Fix #3642
Co-authored-by: Edwin Cheng <[email protected]>
-rw-r--r-- | crates/ra_hir_expand/src/db.rs | 24 | ||||
-rw-r--r-- | crates/ra_hir_ty/src/tests/regression.rs | 31 | ||||
-rw-r--r-- | crates/ra_syntax/src/algo.rs | 4 |
3 files changed, 56 insertions, 3 deletions
diff --git a/crates/ra_hir_expand/src/db.rs b/crates/ra_hir_expand/src/db.rs index d171d2dfd..5a696542f 100644 --- a/crates/ra_hir_expand/src/db.rs +++ b/crates/ra_hir_expand/src/db.rs | |||
@@ -6,7 +6,7 @@ use mbe::{ExpandResult, MacroRules}; | |||
6 | use ra_db::{salsa, SourceDatabase}; | 6 | use ra_db::{salsa, SourceDatabase}; |
7 | use ra_parser::FragmentKind; | 7 | use ra_parser::FragmentKind; |
8 | use ra_prof::profile; | 8 | use ra_prof::profile; |
9 | use ra_syntax::{AstNode, Parse, SyntaxKind::*, SyntaxNode}; | 9 | use ra_syntax::{algo::diff, AstNode, Parse, SyntaxKind::*, SyntaxNode}; |
10 | 10 | ||
11 | use crate::{ | 11 | use crate::{ |
12 | ast_id_map::AstIdMap, BuiltinDeriveExpander, BuiltinFnLikeExpander, EagerCallLoc, EagerMacroId, | 12 | ast_id_map::AstIdMap, BuiltinDeriveExpander, BuiltinFnLikeExpander, EagerCallLoc, EagerMacroId, |
@@ -238,7 +238,7 @@ pub fn parse_macro_with_arg( | |||
238 | } else { | 238 | } else { |
239 | db.macro_expand(macro_call_id) | 239 | db.macro_expand(macro_call_id) |
240 | }; | 240 | }; |
241 | if let Some(err) = err { | 241 | if let Some(err) = &err { |
242 | // Note: | 242 | // Note: |
243 | // The final goal we would like to make all parse_macro success, | 243 | // The final goal we would like to make all parse_macro success, |
244 | // such that the following log will not call anyway. | 244 | // such that the following log will not call anyway. |
@@ -272,7 +272,25 @@ pub fn parse_macro_with_arg( | |||
272 | let fragment_kind = to_fragment_kind(db, macro_call_id); | 272 | let fragment_kind = to_fragment_kind(db, macro_call_id); |
273 | 273 | ||
274 | let (parse, rev_token_map) = mbe::token_tree_to_syntax_node(&tt, fragment_kind).ok()?; | 274 | let (parse, rev_token_map) = mbe::token_tree_to_syntax_node(&tt, fragment_kind).ok()?; |
275 | Some((parse, Arc::new(rev_token_map))) | 275 | |
276 | if err.is_none() { | ||
277 | Some((parse, Arc::new(rev_token_map))) | ||
278 | } else { | ||
279 | // FIXME: | ||
280 | // In future, we should propagate the actual error with recovery information | ||
281 | // instead of ignore the error here. | ||
282 | |||
283 | // Safe check for recurisve identity macro | ||
284 | let node = parse.syntax_node(); | ||
285 | let file: HirFileId = macro_file.into(); | ||
286 | let call_node = file.call_node(db)?; | ||
287 | |||
288 | if !diff(&node, &call_node.value).is_empty() { | ||
289 | Some((parse, Arc::new(rev_token_map))) | ||
290 | } else { | ||
291 | None | ||
292 | } | ||
293 | } | ||
276 | } | 294 | } |
277 | 295 | ||
278 | /// Given a `MacroCallId`, return what `FragmentKind` it belongs to. | 296 | /// Given a `MacroCallId`, return what `FragmentKind` it belongs to. |
diff --git a/crates/ra_hir_ty/src/tests/regression.rs b/crates/ra_hir_ty/src/tests/regression.rs index 14c8ed3a9..a02e3ee05 100644 --- a/crates/ra_hir_ty/src/tests/regression.rs +++ b/crates/ra_hir_ty/src/tests/regression.rs | |||
@@ -453,3 +453,34 @@ pub mod str { | |||
453 | // should be Option<char>, but currently not because of Chalk ambiguity problem | 453 | // should be Option<char>, but currently not because of Chalk ambiguity problem |
454 | assert_eq!("(Option<{unknown}>, Option<{unknown}>)", super::type_at_pos(&db, pos)); | 454 | assert_eq!("(Option<{unknown}>, Option<{unknown}>)", super::type_at_pos(&db, pos)); |
455 | } | 455 | } |
456 | |||
457 | #[test] | ||
458 | fn issue_3642_bad_macro_stackover() { | ||
459 | let (db, pos) = TestDB::with_position( | ||
460 | r#" | ||
461 | //- /main.rs | ||
462 | #[macro_export] | ||
463 | macro_rules! match_ast { | ||
464 | (match $node:ident { $($tt:tt)* }) => { match_ast!(match ($node) { $($tt)* }) }; | ||
465 | |||
466 | (match ($node:expr) { | ||
467 | $( ast::$ast:ident($it:ident) => $res:expr, )* | ||
468 | _ => $catch_all:expr $(,)? | ||
469 | }) => {{ | ||
470 | $( if let Some($it) = ast::$ast::cast($node.clone()) { $res } else )* | ||
471 | { $catch_all } | ||
472 | }}; | ||
473 | } | ||
474 | |||
475 | fn main() { | ||
476 | let anchor<|> = match_ast! { | ||
477 | match parent { | ||
478 | as => {}, | ||
479 | _ => return None | ||
480 | } | ||
481 | }; | ||
482 | }"#, | ||
483 | ); | ||
484 | |||
485 | assert_eq!("()", super::type_at_pos(&db, pos)); | ||
486 | } | ||
diff --git a/crates/ra_syntax/src/algo.rs b/crates/ra_syntax/src/algo.rs index 344cf0fbe..ffdbdc767 100644 --- a/crates/ra_syntax/src/algo.rs +++ b/crates/ra_syntax/src/algo.rs | |||
@@ -95,6 +95,10 @@ impl TreeDiff { | |||
95 | builder.replace(from.text_range(), to.to_string()) | 95 | builder.replace(from.text_range(), to.to_string()) |
96 | } | 96 | } |
97 | } | 97 | } |
98 | |||
99 | pub fn is_empty(&self) -> bool { | ||
100 | self.replacements.is_empty() | ||
101 | } | ||
98 | } | 102 | } |
99 | 103 | ||
100 | /// Finds minimal the diff, which, applied to `from`, will result in `to`. | 104 | /// Finds minimal the diff, which, applied to `from`, will result in `to`. |