Merge #7115

7115: Migrate HasSource::source to return Option r=matklad a=nick96

I've made a start on fixing #6913 based on the provided work plan, migrating `HasSource::source` to return an `Option`. The simple cases are migrated but there are a few that I'm unsure exactly how they should be handled:

- Logging the processing of functions in `AnalysisStatsCmd::run`: In verbose mode it includes the path to the module containing the function and the syntax range. I've handled this with an if-let but would it be better to blow up here with `expect`? I'm not 100% on the code paths but if we're processing a function definition then the source should exist.

I've handled `source()` in all code paths as `None` being a valid return value but are there some cases where we should just blow up? Also, all I've done is bubble up the returned `None`s, there may be some places where we can recover and still provide something.

Co-authored-by: Nick Spain <[email protected]>
Co-authored-by: Nick Spain <[email protected]>

1 files changed, 61 insertions, 69 deletions

diff --git a/crates/ide/src/display/navigation_target.rs b/crates/ide/src/display/navigation_target.rs
index bcde2b6f1..e24c78301 100644
--- a/crates/ide/src/display/navigation_target.rs
+++ b/crates/ide/src/display/navigation_target.rs
@@ -210,41 +210,32 @@ impl ToNav for FileSymbol {
 impl TryToNav for Definition {
     fn try_to_nav(&self, db: &RootDatabase) -> Option<NavigationTarget> {
         match self {
-            Definition::Macro(it) => {
-                // FIXME: Currently proc-macro do not have ast-node,
-                // such that it does not have source
-                // more discussion: https://github.com/rust-analyzer/rust-analyzer/issues/6913
-                if it.is_proc_macro() {
-                    return None;
-                }
-                Some(it.to_nav(db))
-            }
-            Definition::Field(it) => Some(it.to_nav(db)),
+            Definition::Macro(it) => it.try_to_nav(db),
+            Definition::Field(it) => it.try_to_nav(db),
             Definition::ModuleDef(it) => it.try_to_nav(db),
-            Definition::SelfType(it) => Some(it.to_nav(db)),
+            Definition::SelfType(it) => it.try_to_nav(db),
             Definition::Local(it) => Some(it.to_nav(db)),
-            Definition::TypeParam(it) => Some(it.to_nav(db)),
-            Definition::LifetimeParam(it) => Some(it.to_nav(db)),
+            Definition::TypeParam(it) => it.try_to_nav(db),
+            Definition::LifetimeParam(it) => it.try_to_nav(db),
             Definition::Label(it) => Some(it.to_nav(db)),
-            Definition::ConstParam(it) => Some(it.to_nav(db)),
+            Definition::ConstParam(it) => it.try_to_nav(db),
         }
     }
 }
 
 impl TryToNav for hir::ModuleDef {
     fn try_to_nav(&self, db: &RootDatabase) -> Option<NavigationTarget> {
-        let res = match self {
-            hir::ModuleDef::Module(it) => it.to_nav(db),
-            hir::ModuleDef::Function(it) => it.to_nav(db),
-            hir::ModuleDef::Adt(it) => it.to_nav(db),
-            hir::ModuleDef::Variant(it) => it.to_nav(db),
-            hir::ModuleDef::Const(it) => it.to_nav(db),
-            hir::ModuleDef::Static(it) => it.to_nav(db),
-            hir::ModuleDef::Trait(it) => it.to_nav(db),
-            hir::ModuleDef::TypeAlias(it) => it.to_nav(db),
-            hir::ModuleDef::BuiltinType(_) => return None,
-        };
-        Some(res)
+        match self {
+            hir::ModuleDef::Module(it) => Some(it.to_nav(db)),
+            hir::ModuleDef::Function(it) => it.try_to_nav(db),
+            hir::ModuleDef::Adt(it) => it.try_to_nav(db),
+            hir::ModuleDef::Variant(it) => it.try_to_nav(db),
+            hir::ModuleDef::Const(it) => it.try_to_nav(db),
+            hir::ModuleDef::Static(it) => it.try_to_nav(db),
+            hir::ModuleDef::Trait(it) => it.try_to_nav(db),
+            hir::ModuleDef::TypeAlias(it) => it.try_to_nav(db),
+            hir::ModuleDef::BuiltinType(_) => None,
+        }
     }
 }
 
@@ -279,13 +270,13 @@ impl ToNavFromAst for hir::Trait {
     const KIND: SymbolKind = SymbolKind::Trait;
 }
 
-impl<D> ToNav for D
+impl<D> TryToNav for D
 where
     D: HasSource + ToNavFromAst + Copy + HasAttrs,
     D::Ast: ast::NameOwner + ShortLabel,
 {
-    fn to_nav(&self, db: &RootDatabase) -> NavigationTarget {
-        let src = self.source(db);
+    fn try_to_nav(&self, db: &RootDatabase) -> Option<NavigationTarget> {
+        let src = self.source(db)?;
         let mut res = NavigationTarget::from_named(
             db,
             src.as_ref().map(|it| it as &dyn ast::NameOwner),
@@ -293,7 +284,7 @@ where
         );
         res.docs = self.docs(db);
         res.description = src.value.short_label();
-        res
+        Some(res)
     }
 }
 
@@ -312,9 +303,9 @@ impl ToNav for hir::Module {
     }
 }
 
-impl ToNav for hir::Impl {
-    fn to_nav(&self, db: &RootDatabase) -> NavigationTarget {
-        let src = self.source(db);
+impl TryToNav for hir::Impl {
+    fn try_to_nav(&self, db: &RootDatabase) -> Option<NavigationTarget> {
+        let src = self.source(db)?;
         let derive_attr = self.is_builtin_derive(db);
         let frange = if let Some(item) = &derive_attr {
             item.syntax().original_file_range(db)
@@ -327,21 +318,21 @@ impl ToNav for hir::Impl {
             src.value.self_ty().map(|ty| src.with_value(ty.syntax()).original_file_range(db).range)
         };
 
-        NavigationTarget::from_syntax(
+        Some(NavigationTarget::from_syntax(
             frange.file_id,
             "impl".into(),
             focus_range,
             frange.range,
             SymbolKind::Impl,
-        )
+        ))
     }
 }
 
-impl ToNav for hir::Field {
-    fn to_nav(&self, db: &RootDatabase) -> NavigationTarget {
-        let src = self.source(db);
+impl TryToNav for hir::Field {
+    fn try_to_nav(&self, db: &RootDatabase) -> Option<NavigationTarget> {
+        let src = self.source(db)?;
 
-        match &src.value {
+        let field_source = match &src.value {
             FieldSource::Named(it) => {
                 let mut res =
                     NavigationTarget::from_named(db, src.with_value(it), SymbolKind::Field);
@@ -359,13 +350,14 @@ impl ToNav for hir::Field {
                     SymbolKind::Field,
                 )
             }
-        }
+        };
+        Some(field_source)
     }
 }
 
-impl ToNav for hir::MacroDef {
-    fn to_nav(&self, db: &RootDatabase) -> NavigationTarget {
-        let src = self.source(db);
+impl TryToNav for hir::MacroDef {
+    fn try_to_nav(&self, db: &RootDatabase) -> Option<NavigationTarget> {
+        let src = self.source(db)?;
         log::debug!("nav target {:#?}", src.value.syntax());
         let mut res = NavigationTarget::from_named(
             db,
@@ -373,26 +365,26 @@ impl ToNav for hir::MacroDef {
             SymbolKind::Macro,
         );
         res.docs = self.docs(db);
-        res
+        Some(res)
     }
 }
 
-impl ToNav for hir::Adt {
-    fn to_nav(&self, db: &RootDatabase) -> NavigationTarget {
+impl TryToNav for hir::Adt {
+    fn try_to_nav(&self, db: &RootDatabase) -> Option<NavigationTarget> {
         match self {
-            hir::Adt::Struct(it) => it.to_nav(db),
-            hir::Adt::Union(it) => it.to_nav(db),
-            hir::Adt::Enum(it) => it.to_nav(db),
+            hir::Adt::Struct(it) => it.try_to_nav(db),
+            hir::Adt::Union(it) => it.try_to_nav(db),
+            hir::Adt::Enum(it) => it.try_to_nav(db),
         }
     }
 }
 
-impl ToNav for hir::AssocItem {
-    fn to_nav(&self, db: &RootDatabase) -> NavigationTarget {
+impl TryToNav for hir::AssocItem {
+    fn try_to_nav(&self, db: &RootDatabase) -> Option<NavigationTarget> {
         match self {
-            AssocItem::Function(it) => it.to_nav(db),
-            AssocItem::Const(it) => it.to_nav(db),
-            AssocItem::TypeAlias(it) => it.to_nav(db),
+            AssocItem::Function(it) => it.try_to_nav(db),
+            AssocItem::Const(it) => it.try_to_nav(db),
+            AssocItem::TypeAlias(it) => it.try_to_nav(db),
         }
     }
 }
@@ -446,9 +438,9 @@ impl ToNav for hir::Label {
     }
 }
 
-impl ToNav for hir::TypeParam {
-    fn to_nav(&self, db: &RootDatabase) -> NavigationTarget {
-        let src = self.source(db);
+impl TryToNav for hir::TypeParam {
+    fn try_to_nav(&self, db: &RootDatabase) -> Option<NavigationTarget> {
+        let src = self.source(db)?;
         let full_range = match &src.value {
             Either::Left(it) => it.syntax().text_range(),
             Either::Right(it) => it.syntax().text_range(),
@@ -457,7 +449,7 @@ impl ToNav for hir::TypeParam {
             Either::Left(_) => None,
             Either::Right(it) => it.name().map(|it| it.syntax().text_range()),
         };
-        NavigationTarget {
+        Some(NavigationTarget {
             file_id: src.file_id.original_file(db),
             name: self.name(db).to_string().into(),
             kind: Some(SymbolKind::TypeParam),
@@ -466,15 +458,15 @@ impl ToNav for hir::TypeParam {
             container_name: None,
             description: None,
             docs: None,
-        }
+        })
     }
 }
 
-impl ToNav for hir::LifetimeParam {
-    fn to_nav(&self, db: &RootDatabase) -> NavigationTarget {
-        let src = self.source(db);
+impl TryToNav for hir::LifetimeParam {
+    fn try_to_nav(&self, db: &RootDatabase) -> Option<NavigationTarget> {
+        let src = self.source(db)?;
         let full_range = src.value.syntax().text_range();
-        NavigationTarget {
+        Some(NavigationTarget {
             file_id: src.file_id.original_file(db),
             name: self.name(db).to_string().into(),
             kind: Some(SymbolKind::LifetimeParam),
@@ -483,15 +475,15 @@ impl ToNav for hir::LifetimeParam {
             container_name: None,
             description: None,
             docs: None,
-        }
+        })
     }
 }
 
-impl ToNav for hir::ConstParam {
-    fn to_nav(&self, db: &RootDatabase) -> NavigationTarget {
-        let src = self.source(db);
+impl TryToNav for hir::ConstParam {
+    fn try_to_nav(&self, db: &RootDatabase) -> Option<NavigationTarget> {
+        let src = self.source(db)?;
         let full_range = src.value.syntax().text_range();
-        NavigationTarget {
+        Some(NavigationTarget {
             file_id: src.file_id.original_file(db),
             name: self.name(db).to_string().into(),
             kind: Some(SymbolKind::ConstParam),
@@ -500,7 +492,7 @@ impl ToNav for hir::ConstParam {
             container_name: None,
             description: None,
             docs: None,
-        }
+        })
     }
 }