[ty] resolve docstrings for modules (#19898)

This also reintroduces the `ResolvedDefinition::Module` variant because
reverse-engineering it in several places is a bit confusing. In an ideal
world we wouldn't have `ResolvedDefinition::FileWithRange` as it kinda
kills the ability to do richer analysis, so I want to chip away at its
scope wherever I can (currently it's used to point at asname parts of
import statements when doing `ImportAliasResolution::PreserveAliases`,
and also keyword arguments).

This also makes a kind of odd change to allow a hover to *only* produce
a docstring. This works around an oddity where hovering over a module
name in an import fails to resolve to a `ty` even though hovering over
uses of that imported name *does*.

The two fixed tests reflect the two interesting cases here.

Author: Gankra

crates/ty_ide/src/goto.rs

@@ -7,7 +7,6 @@ use std::borrow::Cow;
 
 use crate::find_node::covering_node;
 use crate::stub_mapping::StubMapper;
-use ruff_db::files::FileRange;
 use ruff_db::parsed::ParsedModuleRef;
 use ruff_python_ast::{self as ast, AnyNodeRef};
 use ruff_python_parser::TokenKind;
@@ -203,15 +202,8 @@ impl<'db> DefinitionsOrTargets<'db> {
             DefinitionsOrTargets::Targets(_) => return None,
         };
         for definition in &definitions {
-            // TODO: get docstrings for modules
-            let ResolvedDefinition::Definition(definition) = definition else {
-                continue;
-            };
-            // First try to get the docstring from the original definition
-            let original_docstring = definition.docstring(db);
-
             // If we got a docstring from the original definition, use it
-            if let Some(docstring) = original_docstring {
+            if let Some(docstring) = definition.docstring(db) {
                 return Some(Docstring::new(docstring));
             }
         }
@@ -222,12 +214,9 @@ impl<'db> DefinitionsOrTargets<'db> {
         let stub_mapper = StubMapper::new(db);
 
         // Try to find the corresponding implementation definition
-        for mapped_definition in stub_mapper.map_definitions(definitions) {
-            // TODO: get docstrings for modules
-            if let ResolvedDefinition::Definition(impl_definition) = mapped_definition {
-                if let Some(impl_docstring) = impl_definition.docstring(db) {
-                    return Some(Docstring::new(impl_docstring));
-                }
+        for definition in stub_mapper.map_definitions(definitions) {
+            if let Some(docstring) = definition.docstring(db) {
+                return Some(Docstring::new(docstring));
             }
         }
 
@@ -703,6 +692,10 @@ fn convert_resolved_definitions_to_targets(
                     full_range: full_range.range(),
                 }
             }
+            ty_python_semantic::ResolvedDefinition::Module(file) => {
+                // For modules, navigate to the start of the file
+                crate::NavigationTarget::new(file, TextRange::default())
+            }
             ty_python_semantic::ResolvedDefinition::FileWithRange(file_range) => {
                 // For file ranges, navigate to the specific range within the file
                 crate::NavigationTarget::new(file_range.file(), file_range.range())
@@ -761,11 +754,9 @@ fn definitions_for_module<'db>(
     if let Some(module_name) = ModuleName::new(module_name_str) {
         if let Some(resolved_module) = resolve_module(db, &module_name) {
             if let Some(module_file) = resolved_module.file(db) {
-                let definitions = vec![ResolvedDefinition::FileWithRange(FileRange::new(
-                    module_file,
-                    TextRange::default(),
-                ))];
-                return Some(DefinitionsOrTargets::Definitions(definitions));
+                return Some(DefinitionsOrTargets::Definitions(vec![
+                    ResolvedDefinition::Module(module_file),
+                ]));
             }
         }
     }

crates/ty_ide/src/hover.rs

@@ -20,7 +20,7 @@ pub fn hover(db: &dyn Db, file: File, offset: TextSize) -> Option<RangedValue<Ho
     }
 
     let model = SemanticModel::new(db, file);
-    let ty = goto_target.inferred_type(&model)?;
+    let ty = goto_target.inferred_type(&model).map(HoverContent::Type);
     let docs = goto_target
         .get_definition_targets(
             file,
@@ -29,12 +29,20 @@ pub fn hover(db: &dyn Db, file: File, offset: TextSize) -> Option<RangedValue<Ho
         )
         .and_then(|definitions| definitions.docstring(db))
         .map(HoverContent::Docstring);
-    tracing::debug!("Inferred type of covering node is {}", ty.display(db));
+
+    if let Some(HoverContent::Type(ty)) = ty {
+        tracing::debug!("Inferred type of covering node is {}", ty.display(db));
+    }
 
     // TODO: Render the symbol's signature instead of just its type.
-    let mut contents = vec![HoverContent::Type(ty)];
+    let mut contents = Vec::new();
+    contents.extend(ty);
     contents.extend(docs);
 
+    if contents.is_empty() {
+        return None;
+    }
+
     Some(RangedValue {
         range: FileRange::new(file, goto_target.range()),
         value: Hover { contents },
@@ -630,9 +638,21 @@ mod tests {
 
         assert_snapshot!(test.hover(), @r"
         <module 'lib'>
+        ---------------------------------------------
+        The cool lib_py module!
+
+        Wow this module rocks.
+
         ---------------------------------------------
         ```python
         <module 'lib'>
+        ```
+        ---
+        ```text
+        The cool lib_py module!
+
+        Wow this module rocks.
+
         ```
         ---------------------------------------------
         info[hover]: Hovered content is
@@ -672,7 +692,31 @@ mod tests {
         )
         .unwrap();
 
-        assert_snapshot!(test.hover(), @"Hover provided no content");
+        assert_snapshot!(test.hover(), @r"
+        The cool lib_py module!
+
+        Wow this module rocks.
+
+        ---------------------------------------------
+        ```text
+        The cool lib_py module!
+
+        Wow this module rocks.
+
+        ```
+        ---------------------------------------------
+        info[hover]: Hovered content is
+         --> main.py:2:20
+          |
+        2 |             import lib
+          |                    ^^-
+          |                    | |
+          |                    | Cursor offset
+          |                    source
+        3 |
+        4 |             lib
+          |
+        ");
     }
 
     #[test]

crates/ty_python_semantic/src/semantic_index/definition.rs

@@ -113,7 +113,14 @@ impl<'db> Definition<'db> {
     }
 }
 
-/// Extract a docstring from a function or class body.
+/// Get the module-level docstring for the given file
+pub(crate) fn module_docstring(db: &dyn Db, file: File) -> Option<String> {
+    let module = parsed_module(db, file).load(db);
+    docstring_from_body(module.suite())
+        .map(|docstring_expr| docstring_expr.value.to_str().to_owned())
+}
+
+/// Extract a docstring from a function, module, or class body.
 fn docstring_from_body(body: &[ast::Stmt]) -> Option<&ast::ExprStringLiteral> {
     let stmt = body.first()?;
     // Require the docstring to be a standalone expression.

crates/ty_python_semantic/src/types/ide_support.rs

@@ -973,12 +973,11 @@ mod resolve_definition {
     use ruff_db::files::{File, FileRange};
     use ruff_db::parsed::{ParsedModuleRef, parsed_module};
     use ruff_python_ast as ast;
-    use ruff_text_size::{Ranged, TextRange};
     use rustc_hash::FxHashSet;
     use tracing::trace;
 
     use crate::module_resolver::file_to_module;
-    use crate::semantic_index::definition::{Definition, DefinitionKind};
+    use crate::semantic_index::definition::{Definition, DefinitionKind, module_docstring};
     use crate::semantic_index::scope::{NodeWithScopeKind, ScopeId};
     use crate::semantic_index::{global_scope, place_table, semantic_index, use_def_map};
     use crate::{Db, ModuleName, resolve_module, resolve_real_module};
@@ -992,6 +991,8 @@ mod resolve_definition {
     pub enum ResolvedDefinition<'db> {
         /// The import resolved to a specific definition within a module
         Definition(Definition<'db>),
+        /// The import resolved to an entire module
+        Module(File),
         /// The import resolved to a file with a specific range
         FileWithRange(FileRange),
     }
@@ -1000,9 +1001,18 @@ mod resolve_definition {
         fn file(&self, db: &'db dyn Db) -> File {
             match self {
                 ResolvedDefinition::Definition(definition) => definition.file(db),
+                ResolvedDefinition::Module(file) => *file,
                 ResolvedDefinition::FileWithRange(file_range) => file_range.file(),
             }
         }
+
+        pub fn docstring(&self, db: &'db dyn Db) -> Option<String> {
+            match self {
+                ResolvedDefinition::Definition(definition) => definition.docstring(db),
+                ResolvedDefinition::Module(file) => module_docstring(db, *file),
+                ResolvedDefinition::FileWithRange(_) => None,
+            }
+        }
     }
 
     /// Resolve import definitions to their targets.
@@ -1070,10 +1080,7 @@ mod resolve_definition {
 
                 // For simple imports like "import os", we want to navigate to the module itself.
                 // Return the module file directly instead of trying to find definitions within it.
-                vec![ResolvedDefinition::FileWithRange(FileRange::new(
-                    module_file,
-                    TextRange::default(),
-                ))]
+                vec![ResolvedDefinition::Module(module_file)]
             }
 
             DefinitionKind::ImportFrom(import_from_def) => {
@@ -1283,24 +1290,19 @@ mod resolve_definition {
                 }
                 trace!("Built Definition Path: {path:?}");
             }
-            ResolvedDefinition::FileWithRange(file_range) => {
-                return if file_range.range() == TextRange::default() {
-                    trace!(
-                        "Found module mapping: {} => {}",
-                        stub_file.path(db),
-                        real_file.path(db)
-                    );
-                    // This is just a reference to a module, no need to do paths
-                    Some(vec![ResolvedDefinition::FileWithRange(FileRange::new(
-                        real_file,
-                        TextRange::default(),
-                    ))])
-                } else {
-                    // Not yet implemented -- in this case we want to recover something like a Definition
-                    // and build a Definition Path, but this input is a bit too abstract for now.
-                    trace!("Found arbitrary FileWithRange by stub mapping, giving up");
-                    None
-                };
+            ResolvedDefinition::Module(_) => {
+                trace!(
+                    "Found module mapping: {} => {}",
+                    stub_file.path(db),
+                    real_file.path(db)
+                );
+                return Some(vec![ResolvedDefinition::Module(real_file)]);
+            }
+            ResolvedDefinition::FileWithRange(_) => {
+                // Not yet implemented -- in this case we want to recover something like a Definition
+                // and build a Definition Path, but this input is a bit too abstract for now.
+                trace!("Found arbitrary FileWithRange while stub mapping, giving up");
+                return None;
             }
         }