Merge #3405

3405: More principled approach for gotodef for field shorhand r=matklad a=matklad

Callers can now decide for themselves if they should prefer field or
local definition. By default, it's the local.



bors r+
🤖

Co-authored-by: Aleksey Kladov <[email protected]>

7 files changed, 79 insertions, 36 deletions

diff --git a/crates/ra_hir/src/semantics.rs b/crates/ra_hir/src/semantics.rs
index a0853957c..afc7f7ee7 100644
--- a/crates/ra_hir/src/semantics.rs
+++ b/crates/ra_hir/src/semantics.rs
@@ -107,8 +107,11 @@ impl<'db, DB: HirDatabase> Semantics<'db, DB> {
         self.analyze(field.syntax()).resolve_field(field)
     }
 
-    pub fn resolve_record_field(&self, field: &ast::RecordField) -> Option<StructField> {
-        self.analyze(field.syntax()).resolve_record_field(field)
+    pub fn resolve_record_field(
+        &self,
+        field: &ast::RecordField,
+    ) -> Option<(StructField, Option<Local>)> {
+        self.analyze(field.syntax()).resolve_record_field(self.db, field)
     }
 
     pub fn resolve_record_literal(&self, record_lit: &ast::RecordLit) -> Option<VariantDef> {
diff --git a/crates/ra_hir/src/source_analyzer.rs b/crates/ra_hir/src/source_analyzer.rs
index 4c121eb73..015389fb0 100644
--- a/crates/ra_hir/src/source_analyzer.rs
+++ b/crates/ra_hir/src/source_analyzer.rs
@@ -5,7 +5,7 @@
 //!
 //! So, this modules should not be used during hir construction, it exists
 //! purely for "IDE needs".
-use std::sync::Arc;
+use std::{iter::once, sync::Arc};
 
 use either::Either;
 use hir_def::{
@@ -25,8 +25,8 @@ use ra_syntax::{
 };
 
 use crate::{
-    db::HirDatabase, Adt, Const, EnumVariant, Function, Local, MacroDef, ModuleDef, Path, Static,
-    Struct, Trait, Type, TypeAlias, TypeParam,
+    db::HirDatabase, Adt, Const, EnumVariant, Function, Local, MacroDef, ModPath, ModuleDef, Path,
+    PathKind, Static, Struct, Trait, Type, TypeAlias, TypeParam,
 };
 
 /// `SourceAnalyzer` is a convenience wrapper which exposes HIR API in terms of
@@ -162,16 +162,27 @@ impl SourceAnalyzer {
 
     pub(crate) fn resolve_record_field(
         &self,
+        db: &impl HirDatabase,
         field: &ast::RecordField,
-    ) -> Option<crate::StructField> {
-        let expr_id = match field.expr() {
-            Some(it) => self.expr_id(&it)?,
+    ) -> Option<(crate::StructField, Option<Local>)> {
+        let (expr_id, local) = match field.expr() {
+            Some(it) => (self.expr_id(&it)?, None),
             None => {
                 let src = InFile { file_id: self.file_id, value: field };
-                self.body_source_map.as_ref()?.field_init_shorthand_expr(src)?
+                let expr_id = self.body_source_map.as_ref()?.field_init_shorthand_expr(src)?;
+                let local_name = field.name_ref()?.as_name();
+                let path = ModPath::from_segments(PathKind::Plain, once(local_name));
+                let local = match self.resolver.resolve_path_in_value_ns_fully(db, &path) {
+                    Some(ValueNs::LocalBinding(pat_id)) => {
+                        Some(Local { pat_id, parent: self.resolver.body_owner()? })
+                    }
+                    _ => None,
+                };
+                (expr_id, local)
             }
         };
-        self.infer.as_ref()?.record_field_resolution(expr_id).map(|it| it.into())
+        let struct_field = self.infer.as_ref()?.record_field_resolution(expr_id)?;
+        Some((struct_field.into(), local))
     }
 
     pub(crate) fn resolve_record_literal(
diff --git a/crates/ra_ide/src/goto_definition.rs b/crates/ra_ide/src/goto_definition.rs
index e67585203..76ee232a3 100644
--- a/crates/ra_ide/src/goto_definition.rs
+++ b/crates/ra_ide/src/goto_definition.rs
@@ -76,6 +76,8 @@ pub(crate) fn reference_definition(
 
     let name_kind = classify_name_ref(sema, name_ref);
     if let Some(def) = name_kind {
+        let def = def.definition();
+
         return match def.try_to_nav(sema.db) {
             Some(nav) => ReferenceResult::Exact(nav),
             None => ReferenceResult::Approximate(Vec::new()),
@@ -795,8 +797,8 @@ mod tests {
                 Foo { x<|> };
             }
             ",
-            "x RECORD_FIELD_DEF FileId(1) [13; 19) [13; 14)",
-            "x: i32|x",
+            "x BIND_PAT FileId(1) [42; 43)",
+            "x",
         )
     }
 }
diff --git a/crates/ra_ide/src/hover.rs b/crates/ra_ide/src/hover.rs
index 5073bb1cf..b31956626 100644
--- a/crates/ra_ide/src/hover.rs
+++ b/crates/ra_ide/src/hover.rs
@@ -153,7 +153,7 @@ pub(crate) fn hover(db: &RootDatabase, position: FilePosition) -> Option<RangeIn
     if let Some((node, name_kind)) = match_ast! {
         match (token.parent()) {
             ast::NameRef(name_ref) => {
-                classify_name_ref(&sema, &name_ref).map(|d| (name_ref.syntax().clone(), d))
+                classify_name_ref(&sema, &name_ref).map(|d| (name_ref.syntax().clone(), d.definition()))
             },
             ast::Name(name) => {
                 classify_name(&sema, &name).map(|d| (name.syntax().clone(), d.definition()))
diff --git a/crates/ra_ide/src/references.rs b/crates/ra_ide/src/references.rs
index f763013ae..2eb047c96 100644
--- a/crates/ra_ide/src/references.rs
+++ b/crates/ra_ide/src/references.rs
@@ -27,7 +27,10 @@ use test_utils::tested_by;
 
 use crate::{display::TryToNav, FilePosition, FileRange, NavigationTarget, RangeInfo};
 
-pub(crate) use self::{classify::classify_name_ref, rename::rename};
+pub(crate) use self::{
+    classify::{classify_name_ref, NameRefClass},
+    rename::rename,
+};
 pub(crate) use ra_ide_db::defs::{classify_name, NameDefinition};
 
 pub use self::search_scope::SearchScope;
@@ -160,7 +163,7 @@ fn find_name(
         return Some(RangeInfo::new(range, (name.text().to_string(), def)));
     }
     let name_ref = find_node_at_offset::<ast::NameRef>(&syntax, position.offset)?;
-    let def = classify_name_ref(sema, &name_ref)?;
+    let def = classify_name_ref(sema, &name_ref)?.definition();
     let range = name_ref.syntax().text_range();
     Some(RangeInfo::new(range, (name_ref.text().to_string(), def)))
 }
@@ -212,6 +215,7 @@ fn process_definition(
             // See https://github.com/rust-lang/rust/pull/68198#issuecomment-574269098
 
             if let Some(d) = classify_name_ref(&sema, &name_ref) {
+                let d = d.definition();
                 if d == def {
                     let kind =
                         if is_record_lit_name_ref(&name_ref) || is_call_expr_name_ref(&name_ref) {
diff --git a/crates/ra_ide/src/references/classify.rs b/crates/ra_ide/src/references/classify.rs
index fdd07d8d1..e837ca8a3 100644
--- a/crates/ra_ide/src/references/classify.rs
+++ b/crates/ra_ide/src/references/classify.rs
@@ -1,6 +1,6 @@
 //! Functions that are used to classify an element from its definition or reference.
 
-use hir::{PathResolution, Semantics};
+use hir::{Local, PathResolution, Semantics};
 use ra_ide_db::defs::NameDefinition;
 use ra_ide_db::RootDatabase;
 use ra_prof::profile;
@@ -9,10 +9,24 @@ use test_utils::tested_by;
 
 pub use ra_ide_db::defs::{from_module_def, from_struct_field};
 
+pub enum NameRefClass {
+    NameDefinition(NameDefinition),
+    FieldShorthand { local: Local, field: NameDefinition },
+}
+
+impl NameRefClass {
+    pub fn definition(self) -> NameDefinition {
+        match self {
+            NameRefClass::NameDefinition(def) => def,
+            NameRefClass::FieldShorthand { local, field: _ } => NameDefinition::Local(local),
+        }
+    }
+}
+
 pub(crate) fn classify_name_ref(
     sema: &Semantics<RootDatabase>,
     name_ref: &ast::NameRef,
-) -> Option<NameDefinition> {
+) -> Option<NameRefClass> {
     let _p = profile("classify_name_ref");
 
     let parent = name_ref.syntax().parent()?;
@@ -20,29 +34,34 @@ pub(crate) fn classify_name_ref(
     if let Some(method_call) = ast::MethodCallExpr::cast(parent.clone()) {
         tested_by!(goto_def_for_methods);
         if let Some(func) = sema.resolve_method_call(&method_call) {
-            return Some(from_module_def(func.into()));
+            return Some(NameRefClass::NameDefinition(NameDefinition::ModuleDef(func.into())));
         }
     }
 
     if let Some(field_expr) = ast::FieldExpr::cast(parent.clone()) {
         tested_by!(goto_def_for_fields);
         if let Some(field) = sema.resolve_field(&field_expr) {
-            return Some(from_struct_field(field));
+            return Some(NameRefClass::NameDefinition(NameDefinition::StructField(field)));
         }
     }
 
     if let Some(record_field) = ast::RecordField::cast(parent.clone()) {
         tested_by!(goto_def_for_record_fields);
         tested_by!(goto_def_for_field_init_shorthand);
-        if let Some(field_def) = sema.resolve_record_field(&record_field) {
-            return Some(from_struct_field(field_def));
+        if let Some((field, local)) = sema.resolve_record_field(&record_field) {
+            let field = NameDefinition::StructField(field);
+            let res = match local {
+                None => NameRefClass::NameDefinition(field),
+                Some(local) => NameRefClass::FieldShorthand { field, local },
+            };
+            return Some(res);
         }
     }
 
     if let Some(macro_call) = parent.ancestors().find_map(ast::MacroCall::cast) {
         tested_by!(goto_def_for_macros);
         if let Some(macro_def) = sema.resolve_macro_call(&macro_call) {
-            return Some(NameDefinition::Macro(macro_def));
+            return Some(NameRefClass::NameDefinition(NameDefinition::Macro(macro_def)));
         }
     }
 
@@ -63,5 +82,5 @@ pub(crate) fn classify_name_ref(
         PathResolution::Macro(def) => NameDefinition::Macro(def),
         PathResolution::SelfType(impl_def) => NameDefinition::SelfType(impl_def),
     };
-    Some(res)
+    Some(NameRefClass::NameDefinition(res))
 }
diff --git a/crates/ra_ide/src/syntax_highlighting.rs b/crates/ra_ide/src/syntax_highlighting.rs
index 28117b4d8..b89501aff 100644
--- a/crates/ra_ide/src/syntax_highlighting.rs
+++ b/crates/ra_ide/src/syntax_highlighting.rs
@@ -19,7 +19,11 @@ use ra_syntax::{
 };
 use rustc_hash::FxHashMap;
 
-use crate::{call_info::call_info_for_token, references::classify_name_ref, Analysis, FileId};
+use crate::{
+    call_info::call_info_for_token,
+    references::{classify_name_ref, NameRefClass},
+    Analysis, FileId,
+};
 
 pub(crate) use html::highlight_as_html;
 pub use tags::{Highlight, HighlightModifier, HighlightModifiers, HighlightTag};
@@ -186,24 +190,24 @@ fn highlight_element(
         }
 
         // Highlight references like the definitions they resolve to
-
-        // Special-case field init shorthand
-        NAME_REF if element.parent().and_then(ast::RecordField::cast).is_some() => {
-            HighlightTag::Field.into()
-        }
         NAME_REF if element.ancestors().any(|it| it.kind() == ATTR) => return None,
         NAME_REF => {
             let name_ref = element.into_node().and_then(ast::NameRef::cast).unwrap();
             let name_kind = classify_name_ref(sema, &name_ref)?;
 
-            if let NameDefinition::Local(local) = &name_kind {
-                if let Some(name) = local.name(db) {
-                    let shadow_count = bindings_shadow_count.entry(name.clone()).or_default();
-                    binding_hash = Some(calc_binding_hash(&name, *shadow_count))
+            match name_kind {
+                NameRefClass::NameDefinition(def) => {
+                    if let NameDefinition::Local(local) = &def {
+                        if let Some(name) = local.name(db) {
+                            let shadow_count =
+                                bindings_shadow_count.entry(name.clone()).or_default();
+                            binding_hash = Some(calc_binding_hash(&name, *shadow_count))
+                        }
+                    };
+                    highlight_name(db, def)
                 }
-            };
-
-            highlight_name(db, name_kind)
+                NameRefClass::FieldShorthand { .. } => HighlightTag::Field.into(),
+            }
         }
 
         // Simple token-based highlighting