Merge #2562

2562: Fix NavigationTarget ranges r=matklad a=edwin0cheng

Fix the issue described in https://github.com/rust-analyzer/rust-analyzer/pull/2544#issuecomment-565572553

This PR change the order for finding `full_range` of `focus_range` in following orders:
1. map both ranges to macro_call
2. map focus range to a token inside macro call, and full range to the whole of macro call
3. map both ranges to the whole of macro call

And fix the corresponding tests and make these tests easily to follow.

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

2 files changed, 73 insertions, 45 deletions

diff --git a/crates/ra_ide/src/expand.rs b/crates/ra_ide/src/expand.rs
index 661628ae4..7a22bb0a4 100644
--- a/crates/ra_ide/src/expand.rs
+++ b/crates/ra_ide/src/expand.rs
@@ -1,56 +1,66 @@
 //! Utilities to work with files, produced by macros.
 use std::iter::successors;
 
-use hir::InFile;
+use hir::{InFile, Origin};
 use ra_db::FileId;
 use ra_syntax::{ast, AstNode, SyntaxNode, SyntaxToken, TextRange};
 
 use crate::{db::RootDatabase, FileRange};
 
 pub(crate) fn original_range(db: &RootDatabase, node: InFile<&SyntaxNode>) -> FileRange {
-    let expansion = match node.file_id.expansion_info(db) {
-        None => {
+    if let Some((range, Origin::Call)) = original_range_and_origin(db, node) {
+        return range;
+    }
+
+    if let Some(expansion) = node.file_id.expansion_info(db) {
+        if let Some(call_node) = expansion.call_node() {
             return FileRange {
-                file_id: node.file_id.original_file(db),
-                range: node.value.text_range(),
-            }
+                file_id: call_node.file_id.original_file(db),
+                range: call_node.value.text_range(),
+            };
         }
-        Some(it) => it,
-    };
+    }
+
+    FileRange { file_id: node.file_id.original_file(db), range: node.value.text_range() }
+}
+
+fn original_range_and_origin(
+    db: &RootDatabase,
+    node: InFile<&SyntaxNode>,
+) -> Option<(FileRange, Origin)> {
+    let expansion = node.file_id.expansion_info(db)?;
+
+    // the input node has only one token ?
+    let single = node.value.first_token()? == node.value.last_token()?;
+
     // FIXME: We should handle recurside macro expansions
+    let (range, origin) = node.value.descendants().find_map(|it| {
+        let first = it.first_token()?;
+        let last = it.last_token()?;
 
-    let range = node.value.descendants_with_tokens().find_map(|it| {
-        match it.as_token() {
-            // FIXME: Remove this branch after all `tt::TokenTree`s have a proper `TokenId`,
-            // and return the range of the overall macro expansions if mapping first and last tokens fails.
-            Some(token) => {
-                let token = expansion.map_token_up(node.with_value(&token))?;
-                Some(token.with_value(token.value.text_range()))
-            }
-            None => {
-                // Try to map first and last tokens of node, and, if success, return the union range of mapped tokens
-                let n = it.into_node()?;
-                let first = expansion.map_token_up(node.with_value(&n.first_token()?))?;
-                let last = expansion.map_token_up(node.with_value(&n.last_token()?))?;
-
-                // FIXME: Is is possible ?
-                if first.file_id != last.file_id {
-                    return None;
-                }
-
-                // FIXME: Add union method in TextRange
-                let range = union_range(first.value.text_range(), last.value.text_range());
-                Some(first.with_value(range))
-            }
+        if !single && first == last {
+            return None;
         }
-    });
 
-    return match range {
-        Some(it) => FileRange { file_id: it.file_id.original_file(db), range: it.value },
-        None => {
-            FileRange { file_id: node.file_id.original_file(db), range: node.value.text_range() }
+        // Try to map first and last tokens of node, and, if success, return the union range of mapped tokens
+        let (first, first_origin) = expansion.map_token_up(node.with_value(&first))?;
+        let (last, last_origin) = expansion.map_token_up(node.with_value(&last))?;
+
+        if first.file_id != last.file_id || first_origin != last_origin {
+            return None;
         }
-    };
+
+        // FIXME: Add union method in TextRange
+        Some((
+            first.with_value(union_range(first.value.text_range(), last.value.text_range())),
+            first_origin,
+        ))
+    })?;
+
+    return Some((
+        FileRange { file_id: range.file_id.original_file(db), range: range.value },
+        origin,
+    ));
 
     fn union_range(a: TextRange, b: TextRange) -> TextRange {
         let start = a.start().min(b.start());
diff --git a/crates/ra_ide/src/goto_definition.rs b/crates/ra_ide/src/goto_definition.rs
index 27052d72b..bee8e9df2 100644
--- a/crates/ra_ide/src/goto_definition.rs
+++ b/crates/ra_ide/src/goto_definition.rs
@@ -221,7 +221,7 @@ fn named_target(db: &RootDatabase, node: InFile<&SyntaxNode>) -> Option<Navigati
 
 #[cfg(test)]
 mod tests {
-    use test_utils::covers;
+    use test_utils::{assert_eq_text, covers};
 
     use crate::mock_analysis::analysis_and_position;
 
@@ -234,6 +234,24 @@ mod tests {
         nav.assert_match(expected);
     }
 
+    fn check_goto_with_range_content(fixture: &str, expected: &str, expected_range: &str) {
+        let (analysis, pos) = analysis_and_position(fixture);
+
+        let mut navs = analysis.goto_definition(pos).unwrap().unwrap().info;
+        assert_eq!(navs.len(), 1);
+        let nav = navs.pop().unwrap();
+        let file_text = analysis.file_text(pos.file_id).unwrap();
+
+        let actual_full_range = &file_text[nav.full_range()];
+        let actual_range = &file_text[nav.range()];
+
+        test_utils::assert_eq_text!(
+            &format!("{}|{}", actual_full_range, actual_range),
+            expected_range
+        );
+        nav.assert_match(expected);
+    }
+
     #[test]
     fn goto_definition_works_in_items() {
         check_goto(
@@ -363,28 +381,27 @@ mod tests {
 
     #[test]
     fn goto_definition_works_for_macro_defined_fn_with_arg() {
-        check_goto(
+        check_goto_with_range_content(
             "
             //- /lib.rs
             macro_rules! define_fn {
                 ($name:ident) => (fn $name() {})
             }
 
-            define_fn!(
-                foo
-            )
+            define_fn!(foo);
 
             fn bar() {
                <|>foo();
             }
             ",
-            "foo FN_DEF FileId(1) [80; 83) [80; 83)",
+            "foo FN_DEF FileId(1) [64; 80) [75; 78)",
+            "define_fn!(foo);|foo",
         );
     }
 
     #[test]
     fn goto_definition_works_for_macro_defined_fn_no_arg() {
-        check_goto(
+        check_goto_with_range_content(
             "
             //- /lib.rs
             macro_rules! define_fn {
@@ -397,7 +414,8 @@ mod tests {
                <|>foo();
             }
             ",
-            "foo FN_DEF FileId(1) [39; 42) [39; 42)",
+            "foo FN_DEF FileId(1) [51; 64) [51; 64)",
+            "define_fn!();|define_fn!();",
         );
     }