diff --git a/crates/ty_ide/src/docstring.rs b/crates/ty_ide/src/docstring.rs index c592ed84aa..b749b1feb3 100644 --- a/crates/ty_ide/src/docstring.rs +++ b/crates/ty_ide/src/docstring.rs @@ -7,11 +7,13 @@ //! logic needs to be tolerant of variations. use regex::Regex; -use ruff_python_trivia::leading_indentation; +use ruff_python_trivia::{PythonWhitespace, leading_indentation}; use ruff_source_file::UniversalNewlines; use std::collections::HashMap; use std::sync::LazyLock; +use crate::MarkupKind; + // Static regex instances to avoid recompilation static GOOGLE_SECTION_REGEX: LazyLock = LazyLock::new(|| { Regex::new(r"(?i)^\s*(Args|Arguments|Parameters)\s*:\s*$") @@ -35,21 +37,120 @@ static REST_PARAM_REGEX: LazyLock = LazyLock::new(|| { .expect("reST parameter regex should be valid") }); -/// Extract parameter documentation from popular docstring formats. -/// Returns a map of parameter names to their documentation. -pub fn get_parameter_documentation(docstring: &str) -> HashMap { - let mut param_docs = HashMap::new(); +/// A docstring which hasn't yet been interpreted or rendered +/// +/// Used to ensure handlers of docstrings select a rendering mode. +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] +pub struct Docstring(String); - // Google-style docstrings - param_docs.extend(extract_google_style_params(docstring)); +impl Docstring { + /// Create a new docstring from the raw string literal contents + pub fn new(raw: String) -> Self { + Docstring(raw) + } - // NumPy-style docstrings - param_docs.extend(extract_numpy_style_params(docstring)); + /// Render the docstring to the given markup format + pub fn render(&self, kind: MarkupKind) -> String { + match kind { + MarkupKind::PlainText => self.render_plaintext(), + MarkupKind::Markdown => self.render_markdown(), + } + } - // reST/Sphinx-style docstrings - param_docs.extend(extract_rest_style_params(docstring)); + /// Render the docstring for plaintext display + pub fn render_plaintext(&self) -> String { + documentation_trim(&self.0) + } - param_docs + /// Render the docstring for markdown display + pub fn render_markdown(&self) -> String { + let trimmed = documentation_trim(&self.0); + // TODO: now actually parse it and "render" it to markdown. + // + // For now we just wrap the content in a plaintext codeblock + // to avoid the contents erroneously being interpreted as markdown. + format!("```text\n{trimmed}\n```") + } + + /// Extract parameter documentation from popular docstring formats. + /// Returns a map of parameter names to their documentation. + pub fn parameter_documentation(&self) -> HashMap { + let mut param_docs = HashMap::new(); + + // Google-style docstrings + param_docs.extend(extract_google_style_params(&self.0)); + + // NumPy-style docstrings + param_docs.extend(extract_numpy_style_params(&self.0)); + + // reST/Sphinx-style docstrings + param_docs.extend(extract_rest_style_params(&self.0)); + + param_docs + } +} + +/// Normalizes tabs and trims a docstring as specified in PEP-0257 +/// +/// See: +fn documentation_trim(docs: &str) -> String { + // First apply tab expansion as we don't want tabs in our output + // (python says tabs are equal to 8 spaces). + // + // We also trim off all trailing whitespace here to eliminate trailing newlines so we + // don't need to handle trailing blank lines later. We can't trim away leading + // whitespace yet, because we need to identify the first line and handle it specially. + let expanded = docs.trim_end().replace('\t', " "); + + // Compute the minimum indention of all non-empty non-first lines + // and statistics about leading blank lines to help trim them later. + let mut min_indent = usize::MAX; + let mut leading_blank_lines = 0; + let mut is_first_line = true; + let mut found_non_blank_line = false; + for line_obj in expanded.universal_newlines() { + let line = line_obj.as_str(); + let indent = leading_indentation(line); + if indent == line { + // Blank line + if !found_non_blank_line { + leading_blank_lines += 1; + } + } else { + // Non-blank line + found_non_blank_line = true; + // First line doesn't affect min-indent + if !is_first_line { + min_indent = min_indent.min(indent.len()); + } + } + is_first_line = false; + } + + let mut output = String::new(); + let mut lines = expanded.universal_newlines(); + + // If the first line is non-blank then we need to include it *fully* trimmed + // As its indentation is ignored (effectively treated as having min_indent). + if leading_blank_lines == 0 { + if let Some(first_line) = lines.next() { + output.push_str(first_line.as_str().trim_whitespace()); + output.push('\n'); + } + } + + // For the rest of the lines remove the minimum indent (if possible) and trailing whitespace. + // + // We computed min_indent by only counting python whitespace, and all python whitespace + // is ascii, so we can just remove that many bytes from the front. + for line_obj in lines.skip(leading_blank_lines) { + let line = line_obj.as_str(); + let trimmed_line = line[min_indent.min(line.len())..].trim_whitespace_end(); + output.push_str(trimmed_line); + output.push('\n'); + } + + output } /// Extract parameter documentation from Google-style docstrings. @@ -135,9 +236,14 @@ fn extract_google_style_params(docstring: &str) -> HashMap { param_docs } -/// Calculate the indentation level of a line (number of leading whitespace characters) +/// Calculate the indentation level of a line. +/// +/// Based on python's expandtabs (where tabs are considered 8 spaces). fn get_indentation_level(line: &str) -> usize { - leading_indentation(line).len() + leading_indentation(line) + .chars() + .map(|s| if s == '\t' { 8 } else { 1 }) + .sum() } /// Extract parameter documentation from NumPy-style docstrings. @@ -380,6 +486,8 @@ fn extract_rest_style_params(docstring: &str) -> HashMap { #[cfg(test)] mod tests { + use insta::assert_snapshot; + use super::*; #[test] @@ -397,7 +505,8 @@ mod tests { str: The return value description "#; - let param_docs = get_parameter_documentation(docstring); + let docstring = Docstring::new(docstring.to_owned()); + let param_docs = docstring.parameter_documentation(); assert_eq!(param_docs.len(), 3); assert_eq!(¶m_docs["param1"], "The first parameter description"); @@ -406,6 +515,35 @@ mod tests { "The second parameter description\nThis is a continuation of param2 description." ); assert_eq!(¶m_docs["param3"], "A parameter without type annotation"); + + assert_snapshot!(docstring.render_plaintext(), @r" + This is a function description. + + Args: + param1 (str): The first parameter description + param2 (int): The second parameter description + This is a continuation of param2 description. + param3: A parameter without type annotation + + Returns: + str: The return value description + "); + + assert_snapshot!(docstring.render_markdown(), @r" + ```text + This is a function description. + + Args: + param1 (str): The first parameter description + param2 (int): The second parameter description + This is a continuation of param2 description. + param3: A parameter without type annotation + + Returns: + str: The return value description + + ``` + "); } #[test] @@ -429,7 +567,8 @@ mod tests { The return value description "#; - let param_docs = get_parameter_documentation(docstring); + let docstring = Docstring::new(docstring.to_owned()); + let param_docs = docstring.parameter_documentation(); assert_eq!(param_docs.len(), 3); assert_eq!( @@ -444,6 +583,47 @@ mod tests { param_docs.get("param3").expect("param3 should exist"), "A parameter without type annotation" ); + + assert_snapshot!(docstring.render_plaintext(), @r" + This is a function description. + + Parameters + ---------- + param1 : str + The first parameter description + param2 : int + The second parameter description + This is a continuation of param2 description. + param3 + A parameter without type annotation + + Returns + ------- + str + The return value description + "); + + assert_snapshot!(docstring.render_markdown(), @r" + ```text + This is a function description. + + Parameters + ---------- + param1 : str + The first parameter description + param2 : int + The second parameter description + This is a continuation of param2 description. + param3 + A parameter without type annotation + + Returns + ------- + str + The return value description + + ``` + "); } #[test] @@ -452,8 +632,18 @@ mod tests { This is a simple function description without parameter documentation. "#; - let param_docs = get_parameter_documentation(docstring); + let docstring = Docstring::new(docstring.to_owned()); + let param_docs = docstring.parameter_documentation(); assert!(param_docs.is_empty()); + + assert_snapshot!(docstring.render_plaintext(), @"This is a simple function description without parameter documentation."); + + assert_snapshot!(docstring.render_markdown(), @r" + ```text + This is a simple function description without parameter documentation. + + ``` + "); } #[test] @@ -471,7 +661,8 @@ mod tests { NumPy-style parameter "#; - let param_docs = get_parameter_documentation(docstring); + let docstring = Docstring::new(docstring.to_owned()); + let param_docs = docstring.parameter_documentation(); assert_eq!(param_docs.len(), 3); assert_eq!( @@ -486,6 +677,35 @@ mod tests { param_docs.get("param3").expect("param3 should exist"), "NumPy-style parameter" ); + + assert_snapshot!(docstring.render_plaintext(), @r" + This is a function description. + + Args: + param1 (str): Google-style parameter + param2 (int): Another Google-style parameter + + Parameters + ---------- + param3 : bool + NumPy-style parameter + "); + + assert_snapshot!(docstring.render_markdown(), @r" + ```text + This is a function description. + + Args: + param1 (str): Google-style parameter + param2 (int): Another Google-style parameter + + Parameters + ---------- + param3 : bool + NumPy-style parameter + + ``` + "); } #[test] @@ -501,7 +721,8 @@ mod tests { :rtype: str "#; - let param_docs = get_parameter_documentation(docstring); + let docstring = Docstring::new(docstring.to_owned()); + let param_docs = docstring.parameter_documentation(); assert_eq!(param_docs.len(), 3); assert_eq!( @@ -516,6 +737,31 @@ mod tests { param_docs.get("param3").expect("param3 should exist"), "A parameter without type annotation" ); + + assert_snapshot!(docstring.render_plaintext(), @r" + This is a function description. + + :param str param1: The first parameter description + :param int param2: The second parameter description + This is a continuation of param2 description. + :param param3: A parameter without type annotation + :returns: The return value description + :rtype: str + "); + + assert_snapshot!(docstring.render_markdown(), @r" + ```text + This is a function description. + + :param str param1: The first parameter description + :param int param2: The second parameter description + This is a continuation of param2 description. + :param param3: A parameter without type annotation + :returns: The return value description + :rtype: str + + ``` + "); } #[test] @@ -535,7 +781,8 @@ mod tests { NumPy-style parameter "#; - let param_docs = get_parameter_documentation(docstring); + let docstring = Docstring::new(docstring.to_owned()); + let param_docs = docstring.parameter_documentation(); assert_eq!(param_docs.len(), 4); assert_eq!( @@ -554,6 +801,39 @@ mod tests { param_docs.get("param4").expect("param4 should exist"), "NumPy-style parameter" ); + + assert_snapshot!(docstring.render_plaintext(), @r" + This is a function description. + + Args: + param1 (str): Google-style parameter + + :param int param2: reST-style parameter + :param param3: Another reST-style parameter + + Parameters + ---------- + param4 : bool + NumPy-style parameter + "); + + assert_snapshot!(docstring.render_markdown(), @r" + ```text + This is a function description. + + Args: + param1 (str): Google-style parameter + + :param int param2: reST-style parameter + :param param3: Another reST-style parameter + + Parameters + ---------- + param4 : bool + NumPy-style parameter + + ``` + "); } #[test] @@ -577,7 +857,8 @@ mod tests { The return value description "#; - let param_docs = get_parameter_documentation(docstring); + let docstring = Docstring::new(docstring.to_owned()); + let param_docs = docstring.parameter_documentation(); assert_eq!(param_docs.len(), 3); assert_eq!( @@ -592,6 +873,47 @@ mod tests { param_docs.get("param3").expect("param3 should exist"), "A parameter without type annotation" ); + + assert_snapshot!(docstring.render_plaintext(), @r" + This is a function description. + + Parameters + ---------- + param1 : str + The first parameter description + param2 : int + The second parameter description + This is a continuation of param2 description. + param3 + A parameter without type annotation + + Returns + ------- + str + The return value description + "); + + assert_snapshot!(docstring.render_markdown(), @r" + ```text + This is a function description. + + Parameters + ---------- + param1 : str + The first parameter description + param2 : int + The second parameter description + This is a continuation of param2 description. + param3 + A parameter without type annotation + + Returns + ------- + str + The return value description + + ``` + "); } #[test] @@ -611,7 +933,8 @@ mod tests { \t\tA parameter without type annotation "; - let param_docs = get_parameter_documentation(docstring); + let docstring = Docstring::new(docstring.to_owned()); + let param_docs = docstring.parameter_documentation(); assert_eq!(param_docs.len(), 3); assert_eq!( @@ -626,6 +949,37 @@ mod tests { param_docs.get("param3").expect("param3 should exist"), "A parameter without type annotation" ); + + assert_snapshot!(docstring.render_plaintext(), @r" + This is a function description. + + Parameters + ---------- + param1 : str + The first parameter description + param2 : int + The second parameter description + This is a continuation of param2 description. + param3 + A parameter without type annotation + "); + + assert_snapshot!(docstring.render_markdown(), @r" + ```text + This is a function description. + + Parameters + ---------- + param1 : str + The first parameter description + param2 : int + The second parameter description + This is a continuation of param2 description. + param3 + A parameter without type annotation + + ``` + "); } #[test] @@ -639,9 +993,13 @@ mod tests { // Test with Unix-style line endings (\n) - should work the same let docstring_unix = "This is a function description.\n\nArgs:\n param1 (str): The first parameter\n param2 (int): The second parameter\n"; - let param_docs_windows = get_parameter_documentation(docstring_windows); - let param_docs_mac = get_parameter_documentation(docstring_mac); - let param_docs_unix = get_parameter_documentation(docstring_unix); + let docstring_windows = Docstring::new(docstring_windows.to_owned()); + let docstring_mac = Docstring::new(docstring_mac.to_owned()); + let docstring_unix = Docstring::new(docstring_unix.to_owned()); + + let param_docs_windows = docstring_windows.parameter_documentation(); + let param_docs_mac = docstring_mac.parameter_documentation(); + let param_docs_unix = docstring_unix.parameter_documentation(); // All should produce the same results assert_eq!(param_docs_windows.len(), 2); @@ -660,5 +1018,62 @@ mod tests { param_docs_unix.get("param1"), Some(&"The first parameter".to_string()) ); + + assert_snapshot!(docstring_windows.render_plaintext(), @r" + This is a function description. + + Args: + param1 (str): The first parameter + param2 (int): The second parameter + "); + + assert_snapshot!(docstring_windows.render_markdown(), @r" + ```text + This is a function description. + + Args: + param1 (str): The first parameter + param2 (int): The second parameter + + ``` + "); + + assert_snapshot!(docstring_mac.render_plaintext(), @r" + This is a function description. + + Args: + param1 (str): The first parameter + param2 (int): The second parameter + "); + + assert_snapshot!(docstring_mac.render_markdown(), @r" + ```text + This is a function description. + + Args: + param1 (str): The first parameter + param2 (int): The second parameter + + ``` + "); + + assert_snapshot!(docstring_unix.render_plaintext(), @r" + This is a function description. + + Args: + param1 (str): The first parameter + param2 (int): The second parameter + "); + + assert_snapshot!(docstring_unix.render_markdown(), @r" + ```text + This is a function description. + + Args: + param1 (str): The first parameter + param2 (int): The second parameter + + ``` + "); } } diff --git a/crates/ty_ide/src/goto.rs b/crates/ty_ide/src/goto.rs index e6bc282706..799c7c264d 100644 --- a/crates/ty_ide/src/goto.rs +++ b/crates/ty_ide/src/goto.rs @@ -1,3 +1,4 @@ +use crate::docstring::Docstring; pub use crate::goto_declaration::goto_declaration; pub use crate::goto_definition::goto_definition; pub use crate::goto_type_definition::goto_type_definition; @@ -146,6 +147,94 @@ pub(crate) enum GotoTarget<'a> { }, } +/// The resolved definitions for a `GotoTarget` +#[derive(Debug, Clone)] +pub(crate) enum DefinitionsOrTargets<'db> { + /// We computed actual Definitions we can do followup queries on. + Definitions(Vec>), + /// We directly computed a navigation. + /// + /// We can't get docs or usefully compute goto-definition for this. + Targets(crate::NavigationTargets), +} + +impl<'db> DefinitionsOrTargets<'db> { + /// Get the "goto-declaration" interpretation of this definition + /// + /// In this case it basically returns exactly what was found. + pub(crate) fn declaration_targets( + self, + db: &'db dyn crate::Db, + ) -> Option { + match self { + DefinitionsOrTargets::Definitions(definitions) => { + definitions_to_navigation_targets(db, None, definitions) + } + DefinitionsOrTargets::Targets(targets) => Some(targets), + } + } + + /// Get the "goto-definition" interpretation of this definition + /// + /// In this case we apply stub-mapping to try to find the "real" implementation + /// if the definition we have is found in a stub file. + pub(crate) fn definition_targets( + self, + db: &'db dyn crate::Db, + ) -> Option { + match self { + DefinitionsOrTargets::Definitions(definitions) => { + definitions_to_navigation_targets(db, Some(&StubMapper::new(db)), definitions) + } + DefinitionsOrTargets::Targets(targets) => Some(targets), + } + } + + /// Get the docstring for this definition + /// + /// Typically documentation only appears on implementations and not stubs, + /// so this will check both the goto-declarations and goto-definitions (in that order) + /// and return the first one found. + pub(crate) fn docstring(self, db: &'db dyn crate::Db) -> Option { + let definitions = match self { + DefinitionsOrTargets::Definitions(definitions) => definitions, + // Can't find docs for these + // (make more cases DefinitionOrTargets::Definitions to get more docs!) + 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 { + return Some(Docstring::new(docstring)); + } + } + + // If the definition is located within a stub file and no docstring + // is present, try to map the symbol to an implementation file and extract + // the docstring from that location. + 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)); + } + } + } + + None + } +} + impl GotoTarget<'_> { pub(crate) fn inferred_type<'db>(&self, model: &SemanticModel<'db>) -> Option> { let ty = match self { @@ -173,63 +262,70 @@ impl GotoTarget<'_> { Some(ty) } - /// Gets the navigation ranges for this goto target. - /// If a stub mapper is provided, definitions from stub files will be mapped to - /// their corresponding source file implementations. The `alias_resolution` - /// parameter controls whether import aliases (i.e. "x" in "from a import b as x") are - /// resolved or returned as is. We want to resolve them in some cases (like - /// "goto declaration") but not in others (like find references or rename). - pub(crate) fn get_definition_targets( + /// Gets the definitions for this goto target. + /// + /// The `alias_resolution` parameter controls whether import aliases + /// (i.e. "x" in "from a import b as x") are resolved or returned as is. + /// We want to resolve them in some cases (like "goto declaration") but not in others + /// (like find references or rename). + /// + /// + /// Ideally this would always return `DefinitionsOrTargets::Definitions` + /// as this is more useful for doing stub mapping (goto-definition) and + /// retrieving docstrings. However for now some cases are stubbed out + /// as just returning a raw `NavigationTarget`. + pub(crate) fn get_definition_targets<'db>( &self, file: ruff_db::files::File, - db: &dyn crate::Db, - stub_mapper: Option<&StubMapper>, + db: &'db dyn crate::Db, alias_resolution: ImportAliasResolution, - ) -> Option { + ) -> Option> { use crate::NavigationTarget; use ruff_python_ast as ast; match self { GotoTarget::Expression(expression) => match expression { - ast::ExprRef::Name(name) => definitions_to_navigation_targets( - db, - stub_mapper, + ast::ExprRef::Name(name) => Some(DefinitionsOrTargets::Definitions( definitions_for_name(db, file, name), - ), - ast::ExprRef::Attribute(attribute) => definitions_to_navigation_targets( - db, - stub_mapper, + )), + ast::ExprRef::Attribute(attribute) => Some(DefinitionsOrTargets::Definitions( ty_python_semantic::definitions_for_attribute(db, file, attribute), - ), + )), _ => None, }, // For already-defined symbols, they are their own definitions GotoTarget::FunctionDef(function) => { let range = function.name.range; - Some(crate::NavigationTargets::single(NavigationTarget { - file, - focus_range: range, - full_range: function.range(), - })) + Some(DefinitionsOrTargets::Targets( + crate::NavigationTargets::single(NavigationTarget { + file, + focus_range: range, + full_range: function.range(), + }), + )) } GotoTarget::ClassDef(class) => { let range = class.name.range; - Some(crate::NavigationTargets::single(NavigationTarget { - file, - focus_range: range, - full_range: class.range(), - })) + Some(DefinitionsOrTargets::Targets( + crate::NavigationTargets::single(NavigationTarget { + file, + focus_range: range, + full_range: class.range(), + }), + )) } GotoTarget::Parameter(parameter) => { let range = parameter.name.range; - Some(crate::NavigationTargets::single(NavigationTarget { - file, - focus_range: range, - full_range: parameter.range(), - })) + Some(DefinitionsOrTargets::Targets( + crate::NavigationTargets::single(NavigationTarget { + file, + focus_range: range, + full_range: parameter.range(), + }), + )) } // For import aliases (offset within 'y' or 'z' in "from x import y as z") @@ -237,15 +333,15 @@ impl GotoTarget<'_> { alias, import_from, .. } => { let symbol_name = alias.name.as_str(); - let definitions = definitions_for_imported_symbol( - db, - file, - import_from, - symbol_name, - alias_resolution, - ); - - definitions_to_navigation_targets(db, stub_mapper, definitions) + Some(DefinitionsOrTargets::Definitions( + definitions_for_imported_symbol( + db, + file, + import_from, + symbol_name, + alias_resolution, + ), + )) } GotoTarget::ImportModuleComponent { @@ -260,7 +356,7 @@ impl GotoTarget<'_> { let target_module_name = components[..=*component_index].join("."); // Try to resolve the module - resolve_module_to_navigation_target(db, stub_mapper, &target_module_name) + definitions_for_module(db, &target_module_name) } // Handle import aliases (offset within 'z' in "import x.y as z") @@ -268,14 +364,16 @@ impl GotoTarget<'_> { if alias_resolution == ImportAliasResolution::ResolveAliases { let full_module_name = alias.name.as_str(); // Try to resolve the module - resolve_module_to_navigation_target(db, stub_mapper, full_module_name) + definitions_for_module(db, full_module_name) } else { let alias_range = alias.asname.as_ref().unwrap().range; - Some(crate::NavigationTargets::single(NavigationTarget { - file, - focus_range: alias_range, - full_range: alias.range(), - })) + Some(DefinitionsOrTargets::Targets( + crate::NavigationTargets::single(NavigationTarget { + file, + focus_range: alias_range, + full_range: alias.range(), + }), + )) } } @@ -283,19 +381,17 @@ impl GotoTarget<'_> { GotoTarget::KeywordArgument { keyword, call_expression, - } => { - let definitions = - definitions_for_keyword_argument(db, file, keyword, call_expression); - definitions_to_navigation_targets(db, stub_mapper, definitions) - } + } => Some(DefinitionsOrTargets::Definitions( + definitions_for_keyword_argument(db, file, keyword, call_expression), + )), // For exception variables, they are their own definitions (like parameters) GotoTarget::ExceptVariable(except_handler) => { if let Some(name) = &except_handler.name { let range = name.range; - Some(crate::NavigationTargets::single(NavigationTarget::new( - file, range, - ))) + Some(DefinitionsOrTargets::Targets( + crate::NavigationTargets::single(NavigationTarget::new(file, range)), + )) } else { None } @@ -305,9 +401,9 @@ impl GotoTarget<'_> { GotoTarget::PatternMatchRest(pattern_mapping) => { if let Some(rest_name) = &pattern_mapping.rest { let range = rest_name.range; - Some(crate::NavigationTargets::single(NavigationTarget::new( - file, range, - ))) + Some(DefinitionsOrTargets::Targets( + crate::NavigationTargets::single(NavigationTarget::new(file, range)), + )) } else { None } @@ -317,9 +413,9 @@ impl GotoTarget<'_> { GotoTarget::PatternMatchAsName(pattern_as) => { if let Some(name) = &pattern_as.name { let range = name.range; - Some(crate::NavigationTargets::single(NavigationTarget::new( - file, range, - ))) + Some(DefinitionsOrTargets::Targets( + crate::NavigationTargets::single(NavigationTarget::new(file, range)), + )) } else { None } @@ -656,11 +752,10 @@ pub(crate) fn find_goto_target( } /// Helper function to resolve a module name and create a navigation target. -fn resolve_module_to_navigation_target( - db: &dyn crate::Db, - stub_mapper: Option<&StubMapper>, +fn definitions_for_module<'db>( + db: &'db dyn crate::Db, module_name_str: &str, -) -> Option { +) -> Option> { use ty_python_semantic::{ModuleName, resolve_module}; if let Some(module_name) = ModuleName::new(module_name_str) { @@ -670,7 +765,7 @@ fn resolve_module_to_navigation_target( module_file, TextRange::default(), ))]; - return definitions_to_navigation_targets(db, stub_mapper, definitions); + return Some(DefinitionsOrTargets::Definitions(definitions)); } } } diff --git a/crates/ty_ide/src/goto_declaration.rs b/crates/ty_ide/src/goto_declaration.rs index df5fba0508..52025ac6e2 100644 --- a/crates/ty_ide/src/goto_declaration.rs +++ b/crates/ty_ide/src/goto_declaration.rs @@ -18,12 +18,9 @@ pub fn goto_declaration( let module = parsed_module(db, file).load(db); let goto_target = find_goto_target(&module, offset)?; - let declaration_targets = goto_target.get_definition_targets( - file, - db, - None, - ImportAliasResolution::ResolveAliases, - )?; + let declaration_targets = goto_target + .get_definition_targets(file, db, ImportAliasResolution::ResolveAliases)? + .declaration_targets(db)?; Some(RangedValue { range: FileRange::new(file, goto_target.range()), diff --git a/crates/ty_ide/src/goto_definition.rs b/crates/ty_ide/src/goto_definition.rs index 875a2b1d5a..a2aaee4b16 100644 --- a/crates/ty_ide/src/goto_definition.rs +++ b/crates/ty_ide/src/goto_definition.rs @@ -1,5 +1,4 @@ use crate::goto::find_goto_target; -use crate::stub_mapping::StubMapper; use crate::{Db, NavigationTargets, RangedValue}; use ruff_db::files::{File, FileRange}; use ruff_db::parsed::parsed_module; @@ -20,15 +19,9 @@ pub fn goto_definition( let module = parsed_module(db, file).load(db); let goto_target = find_goto_target(&module, offset)?; - // Create a StubMapper to map from stub files to source files - let stub_mapper = StubMapper::new(db); - - let definition_targets = goto_target.get_definition_targets( - file, - db, - Some(&stub_mapper), - ImportAliasResolution::ResolveAliases, - )?; + let definition_targets = goto_target + .get_definition_targets(file, db, ImportAliasResolution::ResolveAliases)? + .definition_targets(db)?; Some(RangedValue { range: FileRange::new(file, goto_target.range()), diff --git a/crates/ty_ide/src/hover.rs b/crates/ty_ide/src/hover.rs index fb4f7f0f3e..599d780dbf 100644 --- a/crates/ty_ide/src/hover.rs +++ b/crates/ty_ide/src/hover.rs @@ -1,3 +1,4 @@ +use crate::docstring::Docstring; use crate::goto::{GotoTarget, find_goto_target}; use crate::{Db, MarkupKind, RangedValue}; use ruff_db::files::{File, FileRange}; @@ -20,12 +21,19 @@ pub fn hover(db: &dyn Db, file: File, offset: TextSize) -> Option { #[derive(Debug, Clone, Eq, PartialEq)] pub enum HoverContent<'db> { Type(Type<'db>), + Docstring(Docstring), } impl<'db> HoverContent<'db> { @@ -120,6 +129,7 @@ impl fmt::Display for DisplayHoverContent<'_, '_> { .kind .fenced_code_block(ty.display(self.db), "python") .fmt(f), + HoverContent::Docstring(docstring) => docstring.render(self.kind).fmt(f), } } } @@ -165,6 +175,261 @@ mod tests { "); } + #[test] + fn hover_function() { + let test = cursor_test( + r#" + def my_func(a, b): + '''This is such a great func!! + + Args: + a: first for a reason + b: coming for `a`'s title + ''' + return 0 + + my_func(1, 2) + "#, + ); + + assert_snapshot!(test.hover(), @r" + def my_func(a, b) -> Unknown + --------------------------------------------- + This is such a great func!! + + Args: + a: first for a reason + b: coming for `a`'s title + + --------------------------------------------- + ```python + def my_func(a, b) -> Unknown + ``` + --- + ```text + This is such a great func!! + + Args: + a: first for a reason + b: coming for `a`'s title + + ``` + --------------------------------------------- + info[hover]: Hovered content is + --> main.py:11:9 + | + 9 | return 0 + 10 | + 11 | my_func(1, 2) + | ^^^^^-^ + | | | + | | Cursor offset + | source + | + "); + } + + #[test] + fn hover_class() { + let test = cursor_test( + r#" + class MyClass: + ''' + This is such a great class!! + + Don't you know? + + Everyone loves my class!! + + ''' + def __init__(self, val): + """initializes MyClass (perfectly)""" + self.val = val + + def my_method(self, a, b): + '''This is such a great func!! + + Args: + a: first for a reason + b: coming for `a`'s title + ''' + return 0 + + MyClass + "#, + ); + + assert_snapshot!(test.hover(), @r" + + --------------------------------------------- + This is such a great class!! + + Don't you know? + + Everyone loves my class!! + + --------------------------------------------- + ```python + + ``` + --- + ```text + This is such a great class!! + + Don't you know? + + Everyone loves my class!! + + ``` + --------------------------------------------- + info[hover]: Hovered content is + --> main.py:24:9 + | + 22 | return 0 + 23 | + 24 | MyClass + | ^^^^^-^ + | | | + | | Cursor offset + | source + | + "); + } + + #[test] + fn hover_class_init() { + let test = cursor_test( + r#" + class MyClass: + ''' + This is such a great class!! + + Don't you know? + + Everyone loves my class!! + + ''' + def __init__(self, val): + """initializes MyClass (perfectly)""" + self.val = val + + def my_method(self, a, b): + '''This is such a great func!! + + Args: + a: first for a reason + b: coming for `a`'s title + ''' + return 0 + + x = MyClass(0) + "#, + ); + + assert_snapshot!(test.hover(), @r" + + --------------------------------------------- + This is such a great class!! + + Don't you know? + + Everyone loves my class!! + + --------------------------------------------- + ```python + + ``` + --- + ```text + This is such a great class!! + + Don't you know? + + Everyone loves my class!! + + ``` + --------------------------------------------- + info[hover]: Hovered content is + --> main.py:24:13 + | + 22 | return 0 + 23 | + 24 | x = MyClass(0) + | ^^^^^-^ + | | | + | | Cursor offset + | source + | + "); + } + + #[test] + fn hover_class_method() { + let test = cursor_test( + r#" + class MyClass: + ''' + This is such a great class!! + + Don't you know? + + Everyone loves my class!! + + ''' + def __init__(self, val): + """initializes MyClass (perfectly)""" + self.val = val + + def my_method(self, a, b): + '''This is such a great func!! + + Args: + a: first for a reason + b: coming for `a`'s title + ''' + return 0 + + x = MyClass(0) + x.my_method(2, 3) + "#, + ); + + assert_snapshot!(test.hover(), @r" + bound method MyClass.my_method(a, b) -> Unknown + --------------------------------------------- + This is such a great func!! + + Args: + a: first for a reason + b: coming for `a`'s title + + --------------------------------------------- + ```python + bound method MyClass.my_method(a, b) -> Unknown + ``` + --- + ```text + This is such a great func!! + + Args: + a: first for a reason + b: coming for `a`'s title + + ``` + --------------------------------------------- + info[hover]: Hovered content is + --> main.py:25:11 + | + 24 | x = MyClass(0) + 25 | x.my_method(2, 3) + | ^^^^^-^^^ + | | | + | | Cursor offset + | source + | + "); + } + #[test] fn hover_member() { let test = cursor_test( @@ -264,9 +529,15 @@ mod tests { fn hover_keyword_parameter() { let test = cursor_test( r#" - def test(a: int): ... + def test(ab: int): + """my cool test - test(a= 123) + Args: + ab: a nice little integer + """ + return 0 + + test(ab= 123) "#, ); @@ -279,15 +550,16 @@ mod tests { ``` --------------------------------------------- info[hover]: Hovered content is - --> main.py:4:18 - | - 2 | def test(a: int): ... - 3 | - 4 | test(a= 123) - | ^- Cursor offset - | | - | source - | + --> main.py:10:18 + | + 8 | return 0 + 9 | + 10 | test(ab= 123) + | ^- + | || + | |Cursor offset + | source + | "); } @@ -296,9 +568,13 @@ mod tests { let test = cursor_test( r#" - def foo(a, b): ... + def foo(a, b): + """The foo function""" + return 0 - def bar(a, b): ... + def bar(a, b): + """The bar function""" + return 1 if random.choice([True, False]): a = foo @@ -317,11 +593,11 @@ mod tests { ``` --------------------------------------------- info[hover]: Hovered content is - --> main.py:12:13 + --> main.py:16:13 | - 10 | a = bar - 11 | - 12 | a + 14 | a = bar + 15 | + 16 | a | ^- Cursor offset | | | source @@ -339,7 +615,18 @@ mod tests { "#, ); - test.write_file("lib.py", "a = 10").unwrap(); + test.write_file( + "lib.py", + r" + ''' + The cool lib_py module! + + Wow this module rocks. + ''' + a = 10 + ", + ) + .unwrap(); assert_snapshot!(test.hover(), @r" @@ -362,6 +649,32 @@ mod tests { "); } + #[test] + fn hover_module_import() { + let mut test = cursor_test( + r#" + import lib + + lib + "#, + ); + + test.write_file( + "lib.py", + r" + ''' + The cool lib_py module! + + Wow this module rocks. + ''' + a = 10 + ", + ) + .unwrap(); + + assert_snapshot!(test.hover(), @"Hover provided no content"); + } + #[test] fn hover_type_of_expression_with_type_var_type() { let test = cursor_test( @@ -654,6 +967,12 @@ mod tests { let test = cursor_test( r#" def foo(a: str | None, b): + ''' + My cool func + + Args: + a: hopefully a string, right?! + ''' if a is not None: print(a) "#, @@ -667,15 +986,15 @@ mod tests { ``` --------------------------------------------- info[hover]: Hovered content is - --> main.py:4:27 - | - 2 | def foo(a: str | None, b): - 3 | if a is not None: - 4 | print(a) - | ^- Cursor offset - | | - | source - | + --> main.py:10:27 + | + 8 | ''' + 9 | if a is not None: + 10 | print(a) + | ^- Cursor offset + | | + | source + | "); } diff --git a/crates/ty_ide/src/lib.rs b/crates/ty_ide/src/lib.rs index 1d3a8f7b68..45d5c897f0 100644 --- a/crates/ty_ide/src/lib.rs +++ b/crates/ty_ide/src/lib.rs @@ -22,7 +22,6 @@ mod workspace_symbols; pub use completion::completion; pub use doc_highlights::document_highlights; -pub use docstring::get_parameter_documentation; pub use document_symbols::{document_symbols, document_symbols_with_options}; pub use goto::{goto_declaration, goto_definition, goto_type_definition}; pub use goto_references::goto_references; diff --git a/crates/ty_ide/src/references.rs b/crates/ty_ide/src/references.rs index 13b0150183..1dcd747418 100644 --- a/crates/ty_ide/src/references.rs +++ b/crates/ty_ide/src/references.rs @@ -47,12 +47,9 @@ pub(crate) fn references( // Get the definitions for the symbol at the cursor position // When finding references, do not resolve any local aliases. - let target_definitions_nav = goto_target.get_definition_targets( - file, - db, - None, - ImportAliasResolution::PreserveAliases, - )?; + let target_definitions_nav = goto_target + .get_definition_targets(file, db, ImportAliasResolution::PreserveAliases)? + .definition_targets(db)?; let target_definitions: Vec = target_definitions_nav.into_iter().collect(); // Extract the target text from the goto target for fast comparison @@ -287,12 +284,10 @@ impl LocalReferencesFinder<'_> { if let Some(goto_target) = GotoTarget::from_covering_node(covering_node, offset) { // Get the definitions for this goto target - if let Some(current_definitions_nav) = goto_target.get_definition_targets( - self.file, - self.db, - None, - ImportAliasResolution::PreserveAliases, - ) { + if let Some(current_definitions_nav) = goto_target + .get_definition_targets(self.file, self.db, ImportAliasResolution::PreserveAliases) + .and_then(|definitions| definitions.declaration_targets(self.db)) + { let current_definitions: Vec = current_definitions_nav.into_iter().collect(); // Check if any of the current definitions match our target definitions diff --git a/crates/ty_ide/src/rename.rs b/crates/ty_ide/src/rename.rs index 58b44578f8..12be3b9cad 100644 --- a/crates/ty_ide/src/rename.rs +++ b/crates/ty_ide/src/rename.rs @@ -23,8 +23,9 @@ pub fn can_rename(db: &dyn Db, file: File, offset: TextSize) -> Option, + pub documentation: Option, /// Information about each of the parameters in left-to-right order. pub parameters: Vec, /// Index of the parameter that corresponds to the argument where the @@ -174,67 +174,39 @@ fn create_signature_details_from_call_signature_details( }) }; + let parameters = create_parameters_from_offsets( + &details.parameter_label_offsets, + &signature_label, + documentation.as_ref(), + &details.parameter_names, + ); SignatureDetails { - label: signature_label.clone(), - documentation: Some(documentation), - parameters: create_parameters_from_offsets( - &details.parameter_label_offsets, - &signature_label, - db, - details.definition, - &details.parameter_names, - ), + label: signature_label, + documentation, + parameters, active_parameter, } } /// Determine appropriate documentation for a callable type based on its original type. -fn get_callable_documentation(db: &dyn crate::Db, definition: Option) -> String { - if let Some(definition) = definition { - // 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 { - return docstring; - } - - // If the definition is located within a stub file and no docstring - // is present, try to map the symbol to an implementation file and extract - // the docstring from that location. - let stub_mapper = StubMapper::new(db); - let resolved_definition = ResolvedDefinition::Definition(definition); - - // Try to find the corresponding implementation definition - for mapped_definition in stub_mapper.map_definition(resolved_definition) { - if let ResolvedDefinition::Definition(impl_definition) = mapped_definition { - if let Some(impl_docstring) = impl_definition.docstring(db) { - return impl_docstring; - } - } - } - - // Fall back to empty string if no docstring found anywhere - String::new() - } else { - String::new() - } +fn get_callable_documentation( + db: &dyn crate::Db, + definition: Option, +) -> Option { + DefinitionsOrTargets::Definitions(vec![ResolvedDefinition::Definition(definition?)]) + .docstring(db) } /// Create `ParameterDetails` objects from parameter label offsets. fn create_parameters_from_offsets( parameter_offsets: &[TextRange], signature_label: &str, - db: &dyn crate::Db, - definition: Option, + docstring: Option<&Docstring>, parameter_names: &[String], ) -> Vec { // Extract parameter documentation from the function's docstring if available. - let param_docs = if let Some(definition) = definition { - let docstring = definition.docstring(db); - docstring - .map(|doc| get_parameter_documentation(&doc)) - .unwrap_or_default() + let param_docs = if let Some(docstring) = docstring { + docstring.parameter_documentation() } else { std::collections::HashMap::new() }; @@ -265,6 +237,7 @@ fn create_parameters_from_offsets( #[cfg(test)] mod tests { + use crate::docstring::Docstring; use crate::signature_help::SignatureHelpInfo; use crate::tests::{CursorTest, cursor_test}; @@ -298,17 +271,19 @@ mod tests { // Verify that the docstring is extracted and included in the documentation let expected_docstring = concat!( "This is a docstring for the example function.\n", - " \n", - " Args:\n", - " param1: The first parameter as a string\n", - " param2: The second parameter as an integer\n", - " \n", - " Returns:\n", - " A formatted string combining both parameters\n", - " " + "\n", + "Args:\n", + " param1: The first parameter as a string\n", + " param2: The second parameter as an integer\n", + "\n", + "Returns:\n", + " A formatted string combining both parameters\n", ); assert_eq!( - signature.documentation, + signature + .documentation + .as_ref() + .map(Docstring::render_plaintext), Some(expected_docstring.to_string()) ); @@ -518,9 +493,12 @@ mod tests { assert_eq!(param_y.documentation, Some("The y-coordinate".to_string())); // Should have the __init__ method docstring as documentation (not the class docstring) - let expected_docstring = "Initialize a point with x and y coordinates.\n \n Args:\n x: The x-coordinate\n y: The y-coordinate\n "; + let expected_docstring = "Initialize a point with x and y coordinates.\n\nArgs:\n x: The x-coordinate\n y: The y-coordinate\n"; assert_eq!( - signature.documentation, + signature + .documentation + .as_ref() + .map(Docstring::render_plaintext), Some(expected_docstring.to_string()) ); } @@ -566,7 +544,13 @@ mod tests { let signature = &result.signatures[0]; // Should have empty documentation for now - assert_eq!(signature.documentation, Some(String::new())); + assert_eq!( + signature + .documentation + .as_ref() + .map(Docstring::render_plaintext), + None + ); } #[test] @@ -756,9 +740,12 @@ def func() -> str: let signature = &result.signatures[0]; assert_eq!(signature.label, "() -> str"); - let expected_docstring = "This function does something."; + let expected_docstring = "This function does something.\n"; assert_eq!( - signature.documentation, + signature + .documentation + .as_ref() + .map(Docstring::render_plaintext), Some(expected_docstring.to_string()) ); } diff --git a/crates/ty_server/src/server/api/requests/signature_help.rs b/crates/ty_server/src/server/api/requests/signature_help.rs index a90051a2bd..e9b9f160b6 100644 --- a/crates/ty_server/src/server/api/requests/signature_help.rs +++ b/crates/ty_server/src/server/api/requests/signature_help.rs @@ -126,7 +126,9 @@ impl BackgroundDocumentRequestHandler for SignatureHelpRequestHandler { SignatureInformation { label: sig.label, - documentation: sig.documentation.map(Documentation::String), + documentation: sig + .documentation + .map(|docstring| Documentation::String(docstring.render_plaintext())), parameters: Some(parameters), active_parameter, } diff --git a/crates/ty_wasm/src/lib.rs b/crates/ty_wasm/src/lib.rs index b4caa26ced..9fd4653f77 100644 --- a/crates/ty_wasm/src/lib.rs +++ b/crates/ty_wasm/src/lib.rs @@ -532,7 +532,9 @@ impl Workspace { SignatureInformation { label: sig.label, - documentation: sig.documentation, + documentation: sig + .documentation + .map(|docstring| docstring.render_plaintext()), parameters, active_parameter: sig.active_parameter.and_then(|p| u32::try_from(p).ok()), }