-
Notifications
You must be signed in to change notification settings - Fork 1.8k
[ty] Resolve overloads for hovers #21417
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,6 @@ 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 docs = goto_target | ||
| .get_definition_targets( | ||
| file, | ||
|
|
@@ -30,9 +29,10 @@ pub fn hover(db: &dyn Db, file: File, offset: TextSize) -> Option<RangedValue<Ho | |
| .and_then(|definitions| definitions.docstring(db)) | ||
| .map(HoverContent::Docstring); | ||
|
|
||
| // TODO: Render the symbol's signature instead of just its type. | ||
| let mut contents = Vec::new(); | ||
| if let Some(ty) = ty { | ||
| if let Some(signature) = goto_target.call_type_simplified_by_overloads(&model) { | ||
| contents.push(HoverContent::Signature(signature)); | ||
| } else if let Some(ty) = goto_target.inferred_type(&model) { | ||
| tracing::debug!("Inferred type of covering node is {}", ty.display(db)); | ||
| contents.push(match ty { | ||
| Type::KnownInstance(KnownInstanceType::TypeVar(typevar)) => typevar | ||
|
|
@@ -62,7 +62,7 @@ pub struct Hover<'db> { | |
|
|
||
| impl<'db> Hover<'db> { | ||
| /// Renders the hover to a string using the specified markup kind. | ||
| pub const fn display<'a>(&'a self, db: &'a dyn Db, kind: MarkupKind) -> DisplayHover<'a> { | ||
| pub const fn display<'a>(&'a self, db: &'db dyn Db, kind: MarkupKind) -> DisplayHover<'db, 'a> { | ||
| DisplayHover { | ||
| db, | ||
| hover: self, | ||
|
|
@@ -93,13 +93,13 @@ impl<'a, 'db> IntoIterator for &'a Hover<'db> { | |
| } | ||
| } | ||
|
|
||
| pub struct DisplayHover<'a> { | ||
| db: &'a dyn Db, | ||
| hover: &'a Hover<'a>, | ||
| pub struct DisplayHover<'db, 'a> { | ||
| db: &'db dyn Db, | ||
| hover: &'a Hover<'db>, | ||
| kind: MarkupKind, | ||
| } | ||
|
Comment on lines
-96
to
100
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is debris from versions where this was being asked to store |
||
|
|
||
| impl fmt::Display for DisplayHover<'_> { | ||
| impl fmt::Display for DisplayHover<'_, '_> { | ||
| fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { | ||
| let mut first = true; | ||
| for content in &self.hover.contents { | ||
|
|
@@ -115,8 +115,9 @@ impl fmt::Display for DisplayHover<'_> { | |
| } | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, Eq, PartialEq)] | ||
| #[derive(Debug, Clone)] | ||
| pub enum HoverContent<'db> { | ||
| Signature(String), | ||
| Type(Type<'db>, Option<TypeVarVariance>), | ||
| Docstring(Docstring), | ||
| } | ||
|
|
@@ -140,6 +141,9 @@ pub(crate) struct DisplayHoverContent<'a, 'db> { | |
| impl fmt::Display for DisplayHoverContent<'_, '_> { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| match self.content { | ||
| HoverContent::Signature(signature) => { | ||
| self.kind.fenced_code_block(&signature, "python").fmt(f) | ||
| } | ||
| HoverContent::Type(ty, variance) => { | ||
| let variance = match variance { | ||
| Some(TypeVarVariance::Covariant) => " (covariant)", | ||
|
|
@@ -961,14 +965,12 @@ def ab(a: str): ... | |
|
|
||
| assert_snapshot!(test.hover(), @r" | ||
| (a: int) -> Unknown | ||
| (a: str) -> Unknown | ||
| --------------------------------------------- | ||
| the int overload | ||
|
|
||
| --------------------------------------------- | ||
| ```python | ||
| (a: int) -> Unknown | ||
| (a: str) -> Unknown | ||
| ``` | ||
| --- | ||
| ```text | ||
|
|
@@ -1025,14 +1027,12 @@ def ab(a: str): | |
| .build(); | ||
|
|
||
| assert_snapshot!(test.hover(), @r#" | ||
| (a: int) -> Unknown | ||
| (a: str) -> Unknown | ||
| --------------------------------------------- | ||
| the int overload | ||
|
|
||
| --------------------------------------------- | ||
| ```python | ||
| (a: int) -> Unknown | ||
| (a: str) -> Unknown | ||
| ``` | ||
| --- | ||
|
|
@@ -1094,7 +1094,6 @@ def ab(a: int): | |
| a: int, | ||
| b: int | ||
| ) -> Unknown | ||
| (a: int) -> Unknown | ||
| --------------------------------------------- | ||
| the two arg overload | ||
|
|
||
|
|
@@ -1104,7 +1103,6 @@ def ab(a: int): | |
| a: int, | ||
| b: int | ||
| ) -> Unknown | ||
| (a: int) -> Unknown | ||
| ``` | ||
| --- | ||
| ```text | ||
|
|
@@ -1161,20 +1159,12 @@ def ab(a: int): | |
| .build(); | ||
|
|
||
| assert_snapshot!(test.hover(), @r" | ||
| ( | ||
| a: int, | ||
| b: int | ||
| ) -> Unknown | ||
| (a: int) -> Unknown | ||
| --------------------------------------------- | ||
| the two arg overload | ||
|
|
||
| --------------------------------------------- | ||
| ```python | ||
| ( | ||
| a: int, | ||
| b: int | ||
| ) -> Unknown | ||
| (a: int) -> Unknown | ||
| ``` | ||
| --- | ||
|
|
@@ -1236,33 +1226,21 @@ def ab(a: int, *, c: int): | |
| .build(); | ||
|
|
||
| assert_snapshot!(test.hover(), @r" | ||
| (a: int) -> Unknown | ||
| ( | ||
| a: int, | ||
| *, | ||
| b: int | ||
| ) -> Unknown | ||
| ( | ||
| a: int, | ||
| *, | ||
| c: int | ||
| ) -> Unknown | ||
| --------------------------------------------- | ||
| keywordless overload | ||
|
|
||
| --------------------------------------------- | ||
| ```python | ||
| (a: int) -> Unknown | ||
| ( | ||
| a: int, | ||
| *, | ||
| b: int | ||
| ) -> Unknown | ||
| ( | ||
| a: int, | ||
| *, | ||
| c: int | ||
| ) -> Unknown | ||
| ``` | ||
| --- | ||
| ```text | ||
|
|
@@ -1323,12 +1301,6 @@ def ab(a: int, *, c: int): | |
| .build(); | ||
|
|
||
| assert_snapshot!(test.hover(), @r" | ||
| (a: int) -> Unknown | ||
| ( | ||
| a: int, | ||
| *, | ||
| b: int | ||
| ) -> Unknown | ||
| ( | ||
| a: int, | ||
| *, | ||
|
|
@@ -1339,12 +1311,6 @@ def ab(a: int, *, c: int): | |
|
|
||
| --------------------------------------------- | ||
| ```python | ||
| (a: int) -> Unknown | ||
| ( | ||
| a: int, | ||
| *, | ||
| b: int | ||
| ) -> Unknown | ||
| ( | ||
| a: int, | ||
| *, | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -17,7 +17,7 @@ use crate::types::{ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ClassBase, ClassLiteral, DynamicType, KnownClass, KnownInstanceType, Type, TypeContext, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| TypeVarBoundOrConstraints, class::CodeGeneratorKind, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use crate::{Db, HasType, NameKind, SemanticModel}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use crate::{Db, DisplaySettings, HasType, NameKind, SemanticModel}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use ruff_db::files::{File, FileRange}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use ruff_db::parsed::parsed_module; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use ruff_python_ast::name::Name; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -973,6 +973,65 @@ pub fn call_signature_details<'db>( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Given a call expression that has overloads, and whose overload is resolved to a | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// single option by its arguments, return the type of the Signature. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// This is only used for simplifying complex call types, so if we ever detect that | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// the given callable type *is* simple, or that our answer *won't* be simple, we | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// bail at out and return None, so that the original type can be used. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// We do this because `Type::Signature` intentionally loses a lot of context, and | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// so it has a "worse" display than say `Type::FunctionLiteral` or `Type::BoundMethod`, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// which this analysis would naturally wipe away. The contexts this function | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// succeeds in are those where we would print a complicated/ugly type anyway. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub fn call_type_simplified_by_overloads<'db>( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| db: &'db dyn Db, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| model: &SemanticModel<'db>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| call_expr: &ast::ExprCall, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> Option<String> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let func_type = call_expr.func.inferred_type(model); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Use into_callable to handle all the complex type conversions | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let callable_type = func_type.try_upcast_to_callable(db)?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is my understanding correct that this adds another "todo" call-site to astral-sh/ty#1086?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if we'll be able to do astral-sh/ty#1086; commented on that issue. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let bindings = callable_type.bindings(db); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // If the callable is trivial this analysis is useless, bail out | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if let Some(binding) = bindings.single_element() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| && binding.overloads().len() < 2 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return None; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Hand the overload resolution system as much type info as we have | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let args = CallArguments::from_arguments_typed(&call_expr.arguments, |_, splatted_value| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| splatted_value.inferred_type(model) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Try to resolve overloads with the arguments/types we have | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let mut resolved = bindings | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .match_parameters(db, &args) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .check_types(db, &args, TypeContext::default(), &[]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Only use the Ok | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .iter() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .flatten() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .flat_map(|binding| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| binding.matching_overloads().map(|(_, overload)| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| overload | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .signature | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .display_with(db, DisplaySettings::default().multiline()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .to_string() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1018
to
+1023
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I spent an unconscionable amount of time trying to have this return a Type or something more useful than a String but the types on Stringifying the raw Signature is the only thing that seems to be safe.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (An early version returned a full CallSignatureDetails but this usecase has no use of that so I dropped it as a waste of effort. Also this version that just returns a String is easier to iterate on (I kept trying to Not have a String and had to refactor like 4 calls and 2 types over and over as I experimented between |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1017
to
+1024
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I remember now facing a similar problem regarding using the I faced this issue while implementing #18452 and the way I resolved it is by capturing the required information ( I think we could possibly use a similar approach to get the actual ruff/crates/ty_python_semantic/src/types/call/bind.rs Lines 2125 to 2162 in dcc451d
Another approach could be to expand the |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .collect::<Vec<_>>(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // If at the end of this we still got multiple signatures (or no signatures), give up | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if resolved.len() != 1 { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return None; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| resolved.pop() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Returns the definitions of the binary operation along with its callable type. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub fn definitions_for_bin_op<'db>( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| db: &'db dyn Db, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we only apply overload simplification for hover mode? For example, TypeScript jumps to the matching overload if you use "go to definition," which matches my expectation. https://www.typescriptlang.org/play/?#code/GYVwdgxgLglg9mABFApgZygCgIYC5FggC2ARigE4CU+hpFA3AFCiSwLLpZ4HFnkA0iEvgzkYYAObVEo8RKYto8JKgw4avCoJIB+EVDGTp2MAE9EAb0aMAvtcaqsARgAMggESP3le0A
I'm surprised that Pylance doesn't seem to do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm strictly speaking I don't think goto-definition should jump to an overload unless we can't find the actual implementation (in which case it's actually goto-declaration). goto-declaration should ideally jump to the overload though.