Merge #6765

6765: Fix file range computation in macros r=jonas-schievink a=jonas-schievink

This also aligns the diagnostics behavior of `TestDB` with the one from the real IDE (by making the logic from `semantics.rs` a method on `InFile<&SyntaxNode>`), which makes bugs like this easier to find.

This should fix the misplaced diagnostics seen in #6747 and other issues.

bors r+

Co-authored-by: Jonas Schievink <[email protected]>

Author: bors[bot]

crates/hir/src/lib.rs

@@ -39,7 +39,7 @@ pub use crate::{
         Struct, Trait, Type, TypeAlias, TypeParam, Union, VariantDef,
     },
     has_source::HasSource,
-    semantics::{original_range, PathResolution, Semantics, SemanticsScope},
+    semantics::{PathResolution, Semantics, SemanticsScope},
 };
 
 pub use hir_def::{

crates/hir/src/semantics.rs

@@ -13,10 +13,7 @@ use hir_expand::{hygiene::Hygiene, name::AsName, ExpansionInfo};
 use hir_ty::associated_type_shorthand_candidates;
 use itertools::Itertools;
 use rustc_hash::{FxHashMap, FxHashSet};
-use syntax::{
-    algo::{find_node_at_offset, skip_trivia_token},
-    ast, AstNode, Direction, SyntaxNode, SyntaxToken, TextRange, TextSize,
-};
+use syntax::{algo::find_node_at_offset, ast, AstNode, SyntaxNode, SyntaxToken, TextSize};
 
 use crate::{
     code_model::Access,
@@ -25,7 +22,7 @@ use crate::{
     semantics::source_to_def::{ChildContainer, SourceToDefCache, SourceToDefCtx},
     source_analyzer::{resolve_hir_path, SourceAnalyzer},
     AssocItem, Callable, Crate, Field, Function, HirFileId, ImplDef, InFile, Local, MacroDef,
-    Module, ModuleDef, Name, Origin, Path, ScopeDef, Trait, Type, TypeAlias, TypeParam, VariantDef,
+    Module, ModuleDef, Name, Path, ScopeDef, Trait, Type, TypeAlias, TypeParam, VariantDef,
 };
 
 #[derive(Debug, Clone, PartialEq, Eq)]
@@ -372,15 +369,15 @@ impl<'db> SemanticsImpl<'db> {
 
     fn original_range(&self, node: &SyntaxNode) -> FileRange {
         let node = self.find_file(node.clone());
-        original_range(self.db, node.as_ref())
+        node.as_ref().original_file_range(self.db.upcast())
     }
 
     fn diagnostics_display_range(&self, diagnostics: &dyn Diagnostic) -> FileRange {
         let src = diagnostics.display_source();
         let root = self.db.parse_or_expand(src.file_id).unwrap();
         let node = src.value.to_node(&root);
         self.cache(root, src.file_id);
-        original_range(self.db, src.with_value(&node))
+        src.with_value(&node).original_file_range(self.db.upcast())
     }
 
     fn ancestors_with_macros(&self, node: SyntaxNode) -> impl Iterator<Item = SyntaxNode> + '_ {
@@ -771,68 +768,3 @@ impl<'a> SemanticsScope<'a> {
         resolve_hir_path(self.db, &self.resolver, &path)
     }
 }
-
-// FIXME: Change `HasSource` trait to work with `Semantics` and remove this?
-pub fn original_range(db: &dyn HirDatabase, node: InFile<&SyntaxNode>) -> FileRange {
-    if let Some(range) = original_range_opt(db, node) {
-        let original_file = range.file_id.original_file(db.upcast());
-        if range.file_id == original_file.into() {
-            return FileRange { file_id: original_file, range: range.value };
-        }
-
-        log::error!("Fail to mapping up more for {:?}", range);
-        return FileRange { file_id: range.file_id.original_file(db.upcast()), range: range.value };
-    }
-
-    // Fall back to whole macro call
-    if let Some(expansion) = node.file_id.expansion_info(db.upcast()) {
-        if let Some(call_node) = expansion.call_node() {
-            return FileRange {
-                file_id: call_node.file_id.original_file(db.upcast()),
-                range: call_node.value.text_range(),
-            };
-        }
-    }
-
-    FileRange { file_id: node.file_id.original_file(db.upcast()), range: node.value.text_range() }
-}
-
-fn original_range_opt(
-    db: &dyn HirDatabase,
-    node: InFile<&SyntaxNode>,
-) -> Option<InFile<TextRange>> {
-    let expansion = node.file_id.expansion_info(db.upcast())?;
-
-    // the input node has only one token ?
-    let single = skip_trivia_token(node.value.first_token()?, Direction::Next)?
-        == skip_trivia_token(node.value.last_token()?, Direction::Prev)?;
-
-    Some(node.value.descendants().find_map(|it| {
-        let first = skip_trivia_token(it.first_token()?, Direction::Next)?;
-        let first = ascend_call_token(db, &expansion, node.with_value(first))?;
-
-        let last = skip_trivia_token(it.last_token()?, Direction::Prev)?;
-        let last = ascend_call_token(db, &expansion, node.with_value(last))?;
-
-        if (!single && first == last) || (first.file_id != last.file_id) {
-            return None;
-        }
-
-        Some(first.with_value(first.value.text_range().cover(last.value.text_range())))
-    })?)
-}
-
-fn ascend_call_token(
-    db: &dyn HirDatabase,
-    expansion: &ExpansionInfo,
-    token: InFile<SyntaxToken>,
-) -> Option<InFile<SyntaxToken>> {
-    let (mapped, origin) = expansion.map_token_up(token.as_ref())?;
-    if origin != Origin::Call {
-        return None;
-    }
-    if let Some(info) = mapped.file_id.expansion_info(db.upcast()) {
-        return ascend_call_token(db, &info, mapped);
-    }
-    Some(mapped)
-}

crates/hir_def/src/test_db.rs

@@ -158,17 +158,11 @@ impl TestDB {
             let src = d.display_source();
             let root = db.parse_or_expand(src.file_id).unwrap();
 
-            // Place all diagnostics emitted in macro files on the original caller.
-            // Note that this does *not* match IDE behavior.
-            let mut src = src.map(|ptr| ptr.to_node(&root));
-            while let Some(exp) = src.file_id.call_node(db) {
-                src = exp;
-            }
+            let node = src.map(|ptr| ptr.to_node(&root));
+            let frange = node.as_ref().original_file_range(db);
 
-            let file_id = src.file_id.original_file(db);
-            let range = src.value.text_range();
             let message = d.message().to_owned();
-            actual.entry(file_id).or_default().push((range, message));
+            actual.entry(frange.file_id).or_default().push((frange.range, message));
         });
 
         for (file_id, diags) in actual.iter_mut() {

crates/hir_expand/src/lib.rs

@@ -20,11 +20,11 @@ pub use mbe::{ExpandError, ExpandResult};
 use std::hash::Hash;
 use std::sync::Arc;
 
-use base_db::{impl_intern_key, salsa, CrateId, FileId};
+use base_db::{impl_intern_key, salsa, CrateId, FileId, FileRange};
 use syntax::{
-    algo,
+    algo::{self, skip_trivia_token},
     ast::{self, AstNode},
-    SyntaxNode, SyntaxToken, TextSize,
+    Direction, SyntaxNode, SyntaxToken, TextRange, TextSize,
 };
 
 use crate::ast_id_map::FileAstId;
@@ -445,6 +445,70 @@ impl InFile<SyntaxNode> {
     }
 }
 
+impl<'a> InFile<&'a SyntaxNode> {
+    pub fn original_file_range(self, db: &dyn db::AstDatabase) -> FileRange {
+        if let Some(range) = original_range_opt(db, self) {
+            let original_file = range.file_id.original_file(db);
+            if range.file_id == original_file.into() {
+                return FileRange { file_id: original_file, range: range.value };
+            }
+
+            log::error!("Fail to mapping up more for {:?}", range);
+            return FileRange { file_id: range.file_id.original_file(db), range: range.value };
+        }
+
+        // Fall back to whole macro call.
+        let mut node = self.cloned();
+        while let Some(call_node) = node.file_id.call_node(db) {
+            node = call_node;
+        }
+
+        let orig_file = node.file_id.original_file(db);
+        assert_eq!(node.file_id, orig_file.into());
+        FileRange { file_id: orig_file, range: node.value.text_range() }
+    }
+}
+
+fn original_range_opt(
+    db: &dyn db::AstDatabase,
+    node: InFile<&SyntaxNode>,
+) -> Option<InFile<TextRange>> {
+    let expansion = node.file_id.expansion_info(db)?;
+
+    // the input node has only one token ?
+    let single = skip_trivia_token(node.value.first_token()?, Direction::Next)?
+        == skip_trivia_token(node.value.last_token()?, Direction::Prev)?;
+
+    Some(node.value.descendants().find_map(|it| {
+        let first = skip_trivia_token(it.first_token()?, Direction::Next)?;
+        let first = ascend_call_token(db, &expansion, node.with_value(first))?;
+
+        let last = skip_trivia_token(it.last_token()?, Direction::Prev)?;
+        let last = ascend_call_token(db, &expansion, node.with_value(last))?;
+
+        if (!single && first == last) || (first.file_id != last.file_id) {
+            return None;
+        }
+
+        Some(first.with_value(first.value.text_range().cover(last.value.text_range())))
+    })?)
+}
+
+fn ascend_call_token(
+    db: &dyn db::AstDatabase,
+    expansion: &ExpansionInfo,
+    token: InFile<SyntaxToken>,
+) -> Option<InFile<SyntaxToken>> {
+    let (mapped, origin) = expansion.map_token_up(token.as_ref())?;
+    if origin != Origin::Call {
+        return None;
+    }
+    if let Some(info) = mapped.file_id.expansion_info(db) {
+        return ascend_call_token(db, &info, mapped);
+    }
+    Some(mapped)
+}
+
 impl InFile<SyntaxToken> {
     pub fn ancestors_with_macros(
         self,

crates/ide/src/display/navigation_target.rs

@@ -1,7 +1,7 @@
 //! FIXME: write short doc here
 
 use either::Either;
-use hir::{original_range, AssocItem, FieldSource, HasSource, InFile, ModuleSource};
+use hir::{AssocItem, FieldSource, HasSource, InFile, ModuleSource};
 use ide_db::base_db::{FileId, SourceDatabase};
 use ide_db::{defs::Definition, RootDatabase};
 use syntax::{
@@ -62,7 +62,8 @@ impl NavigationTarget {
     pub(crate) fn from_module_to_decl(db: &RootDatabase, module: hir::Module) -> NavigationTarget {
         let name = module.name(db).map(|it| it.to_string().into()).unwrap_or_default();
         if let Some(src) = module.declaration_source(db) {
-            let frange = original_range(db, src.as_ref().map(|it| it.syntax()));
+            let node = src.as_ref().map(|it| it.syntax());
+            let frange = node.original_file_range(db);
             let mut res = NavigationTarget::from_syntax(
                 frange.file_id,
                 name,
@@ -104,8 +105,8 @@ impl NavigationTarget {
         let name =
             node.value.name().map(|it| it.text().clone()).unwrap_or_else(|| SmolStr::new("_"));
         let focus_range =
-            node.value.name().map(|it| original_range(db, node.with_value(it.syntax())).range);
-        let frange = original_range(db, node.map(|it| it.syntax()));
+            node.value.name().map(|it| node.with_value(it.syntax()).original_file_range(db).range);
+        let frange = node.map(|it| it.syntax()).original_file_range(db);
 
         NavigationTarget::from_syntax(
             frange.file_id,
@@ -124,7 +125,7 @@ impl NavigationTarget {
     ) -> NavigationTarget {
         let name =
             named.value.name().map(|it| it.text().clone()).unwrap_or_else(|| SmolStr::new("_"));
-        let frange = original_range(db, node.map(|it| it.syntax()));
+        let frange = node.map(|it| it.syntax()).original_file_range(db);
 
         NavigationTarget::from_syntax(
             frange.file_id,
@@ -236,7 +237,7 @@ impl ToNav for hir::Module {
                 (node.syntax(), node.name().map(|it| it.syntax().text_range()))
             }
         };
-        let frange = original_range(db, src.with_value(syntax));
+        let frange = src.with_value(syntax).original_file_range(db);
         NavigationTarget::from_syntax(frange.file_id, name, focus, frange.range, syntax.kind())
     }
 }
@@ -246,14 +247,14 @@ impl ToNav for hir::ImplDef {
         let src = self.source(db);
         let derive_attr = self.is_builtin_derive(db);
         let frange = if let Some(item) = &derive_attr {
-            original_range(db, item.syntax())
+            item.syntax().original_file_range(db)
         } else {
-            original_range(db, src.as_ref().map(|it| it.syntax()))
+            src.as_ref().map(|it| it.syntax()).original_file_range(db)
         };
         let focus_range = if derive_attr.is_some() {
             None
         } else {
-            src.value.self_ty().map(|ty| original_range(db, src.with_value(ty.syntax())).range)
+            src.value.self_ty().map(|ty| src.with_value(ty.syntax()).original_file_range(db).range)
         };
 
         NavigationTarget::from_syntax(
@@ -278,7 +279,7 @@ impl ToNav for hir::Field {
                 res
             }
             FieldSource::Pos(it) => {
-                let frange = original_range(db, src.with_value(it.syntax()));
+                let frange = src.with_value(it.syntax()).original_file_range(db);
                 NavigationTarget::from_syntax(
                     frange.file_id,
                     "".into(),
@@ -331,7 +332,7 @@ impl ToNav for hir::Local {
             }
             Either::Right(it) => it.syntax().clone(),
         };
-        let full_range = original_range(db, src.with_value(&node));
+        let full_range = src.with_value(&node).original_file_range(db);
         let name = match self.name(db) {
             Some(it) => it.to_string().into(),
             None => "".into(),

crates/rust-analyzer/src/cli/analysis_stats.rs

@@ -8,7 +8,7 @@ use std::{
 
 use hir::{
     db::{AstDatabase, DefDatabase, HirDatabase},
-    original_range, AssocItem, Crate, HasSource, HirDisplay, ModuleDef,
+    AssocItem, Crate, HasSource, HirDisplay, ModuleDef,
 };
 use hir_def::FunctionId;
 use hir_ty::{Ty, TypeWalk};
@@ -232,7 +232,7 @@ impl AnalysisStatsCmd {
                             // But also, we should just turn the type mismatches into diagnostics and provide these
                             let root = db.parse_or_expand(src.file_id).unwrap();
                             let node = src.map(|e| e.to_node(&root).syntax().clone());
-                            let original_range = original_range(db, node.as_ref());
+                            let original_range = node.as_ref().original_file_range(db);
                             let path = vfs.file_path(original_range.file_id);
                             let line_index =
                                 host.analysis().file_line_index(original_range.file_id).unwrap();