[ty] improve diagnostics for failure to call overloaded function #18073
Conversation
BurntSushi
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
BurntSushi
force-pushed
the
ag/diag-overload
branch
from
773fbf3 to
6fc7040
Compare
MichaReiser
changed the title
MichaReiser
added
ty
BurntSushi
requested review from
dhruvmanila
and removed request for
dcreager and
sharkdp
|
Here are screenshots showing before/after for this Python file: from typing import overload
@overload
def f(x: int) -> int: ...
@overload
def f(x: str) -> str: ...
def f(x: int | str) -> int | str:
return x
f(b"foo")Before:
After:
|
BurntSushi
force-pushed
the
ag/diag-overload
branch
from
6fc7040 to
1e0580b
Compare
|
|
I'm not sure having a separate subdiagnostic for each overload definition scales well for functions with many overloads. Here's a single diagnostic I get for a bad Does |
|
By comparison, here's mypy's diagnostic for the same bad |
|
What does rustc do if there's multiple possible methods to call (I think I saw this in the past). Does it use notes or sub diagnostics? |
|
Notes are sub-diagnostics. @AlexWaygood Those don't include the surrounding context or line numbers of the function definitions though? I think that might be swinging too far in the concise direction? In any case, we don't have a way, today, of rendering concise single-line code snippets. One thing we could do, I think relatively easily, is add a way of specifying the context window size for code snippets. It's hard-coded to 2 lines (above and below), but we could shrink that down to 0. It won't be as concise as mypy, but it will be more concise than what we have today. I think this PR is an improvement over the status quo. I definitely do not claim it is the best we can do though. |
Why are those useful in this situation? I find them distracting, personally; I think all I care about in this situation is the types, if I'm a user. I think you can get a pretty-printed version of the signature of each overload by calling |
Two counter-arguments (even though I also partially agree with you @AlexWaygood):
|
|
I agree that having one link back to the original source code is useful, for sure. But a separate snippet for each overload feels quite excessive to me! |
Yes, maybe. But that |
But this would also be solved by my proposal of printing the signature of each overload as a note rather than attempting to go back to the original source code of each overload as an annotation, I think? |
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
I think this is a huge improvement to what we have today. The only thing I would change (in this iteration) is to highlight the function up to all arguments so that the entire signature is visible
I do agree with Alex that it would be nice to have a more compact representation. But I don't think this should be blocking for this improvement (and getting there is probably also easier because of what's implemented in this PR)
dhruvmanila
left a comment
dhruvmanila
left a comment
There was a problem hiding this comment.
Thank you!
Currently, there are only a few reasons for why an overload might not match - arity mismatch for the arguments passed in to the required parameters or a non-assignable type. This excludes the reasons due to Todo type because ty doesn't support certain features like unpacking arguments.
I'm thinking to either tackle that as part of astral-sh/ty#104 or follow-up to that i.e., store the reason for why a certain overload is not matched. But, if you're planning to do this as a follow-up to this PR, I'm happy to just extend that when the full algorithm is implemented.
|
Here's a diff relative to this PR branch that would implement my suggestion: diff --git a/crates/ty_python_semantic/src/types/call/bind.rs b/crates/ty_python_semantic/src/types/call/bind.rs
index 33fab5cc6a..7930b3c896 100644
--- a/crates/ty_python_semantic/src/types/call/bind.rs
+++ b/crates/ty_python_semantic/src/types/call/bind.rs
@@ -1113,34 +1113,37 @@ impl<'db> CallableBinding<'db> {
String::new()
}
));
- if let Some(overloaded_types) = self
- .signature_type
- .into_function_literal()
- .and_then(|funty| funty.to_overloaded(context.db()))
- {
- for overload_type in &overloaded_types.overloads {
- if let Some((name_span, _)) =
- overload_type.parameter_span(context.db(), None)
+ if let Some(function) = self.signature_type.into_function_literal() {
+ if let Some(overloaded_function) = function.to_overloaded(context.db()) {
+ if let Some((first_overload_span, _)) =
+ overloaded_function.overloads[0].parameter_span(context.db(), None)
+ {
+ let mut sub =
+ SubDiagnostic::new(Severity::Info, "First overload defined here");
+ sub.annotate(Annotation::primary(first_overload_span));
+ diag.sub(sub);
+ }
+
+ diag.info(format_args!(
+ "Possible overloads for function `{}`:",
+ function.name(context.db())
+ ));
+ for overload in &function.signature(context.db()).overloads.overloads {
+ diag.info(format_args!(" {}", overload.display(context.db())));
+ }
+
+ if let Some((name_span, _)) = overloaded_function
+ .implementation
+ .and_then(|funty| funty.parameter_span(context.db(), None))
{
let mut sub = SubDiagnostic::new(
Severity::Info,
- "Unmatched overload defined here",
+ "Overload implementation defined here",
);
sub.annotate(Annotation::primary(name_span));
diag.sub(sub);
}
}
- if let Some((name_span, _)) = overloaded_types
- .implementation
- .and_then(|funty| funty.parameter_span(context.db(), None))
- {
- let mut sub = SubDiagnostic::new(
- Severity::Info,
- "Overload implementation defined here",
- );
- sub.annotate(Annotation::primary(name_span));
- diag.sub(sub);
- }
}
if let Some(union_diag) = union_diag {
union_diag.add_union_context(context.db(), &mut diag);
diff --git a/crates/ty_python_semantic/src/types/signatures.rs b/crates/ty_python_semantic/src/types/signatures.rs
index 25169d3f57..182cce3eb4 100644
--- a/crates/ty_python_semantic/src/types/signatures.rs
+++ b/crates/ty_python_semantic/src/types/signatures.rs
@@ -123,7 +123,7 @@ pub(crate) struct CallableSignature<'db> {
///
/// By using `SmallVec`, we avoid an extra heap allocation for the common case of a
/// non-overloaded callable.
- overloads: SmallVec<[Signature<'db>; 1]>,
+ pub(crate) overloads: SmallVec<[Signature<'db>; 1]>,
}Here's what the diagnostic would look like for my example with that change applied:
And here's what the diagnostic would look like on @sharkdp's example (in release mode, without the
|
Here's the project I was thinking of. Around 11,000 lines of overloads for a single function: https://github.com/henribru/google-api-python-client-stubs/blob/master/googleapiclient-stubs/discovery.pyi |
Does this already happen as part of |
BurntSushi
force-pushed
the
ag/diag-overload
branch
from
1e0580b to
72f1ccc
Compare
It looks like support for `@overload` has been added since this test was created, so we remove the TODO and add a snippet (from #274).
I found the previous code somewhat harder to read. Namely, a `for` loop was being used to encode "execute zero or one times, but not more." Which is sometimes okay, but it seemed clearer to me to use more explicit case analysis here. This should have no behavioral changes.
These are, after all, specific to function types. The methods on `Type` are more like conveniences that return something when the type *happens* to be a function. But defining them on `FunctionType` itself makes it easy to call them when you have a `FunctionType` instead of a `Type`.
|
@AlexWaygood Thank you! I ended up taking your patch. I'm still a little worried about printing reformatted code, but given that it seems like "tons of overloads" is not altogether uncommon, maybe it does make sense to prioritize concision here. I did also tweak the spans (on the implementation and the first unmatched overload) to cover the entire parameter list, which should at least give good context to address @sharkdp's and @MichaReiser's concerns. I've also added snapshot tests to cover these cases ( As for @dhruvmanila:
I think there are two challenges with surfacing this information at present. The first is information display. If there are a lot of unmatched overloads, then how do we preserve conciseness while also showing the reason why each overload didn't match? That seems tricky. The other challenge here, I think, is how to match up the overload bindings with the overload types returned by |
BurntSushi
force-pushed
the
ag/diag-overload
branch
2 times, most recently
from
5620ce0 to
22854da
Compare
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
Thanks you -- this looks awesome!
...o_matching_overload…_-_No_matching_overload…_-_Call_to_function_wit…_(dd80c593d9136f35).snap
Outdated
Show resolved
Hide resolved
|
I switched the snapshot test from using |
BurntSushi
force-pushed
the
ag/diag-overload
branch
from
22854da to
8ceb6bf
Compare
The diagnostic now includes a pointer to the implementation definition along with each possible overload. This doesn't include information about *why* each overload failed. But given the emphasis on concise output (since there can be *many* unmatched overloads), it's not totally clear how to include that additional information. Fixes #274
BurntSushi
force-pushed
the
ag/diag-overload
branch
from
8ceb6bf to
af56d81
Compare
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
Awesome work, thank you!!
Yeah, this is tricky. I haven't thought about how much information should be displayed for this case. One way would be to group overloads based on the reason. For example, if there are multiple overloads which are not matched because of arity mismatch, they can be grouped under a single sub-diagnostic. We can then have sub-diagnostics for each reason and each of those sub-diagnostics would indicate the overloads that have been excluded for that specific reason. Does that make sense? There are still open questions for the above approach like should we group multiple overloads which have invalid argument type at different positions and if so, how do we highlight the arguments in each of those overloads. If this isn't a feasible solution, we could move back to highlighting each of the overloads like in the first version of this PR and limit the number of overloads that are displayed. The current limit is 50 but that seems a lot as well if we display the code frame as well.
I'm not exactly sure what do you mean here. Can you say more? |
@BurntSushi -- we need to include the full link to the GitHub issue now that the issues are in a different repo ("Fixes astral-sh/ty#274" rather than "Fixes #274") |
There is a sequence of overload bindings and also a sequence of overload types. They might be in correspondence, but it's unclear to me if that's true. (And maybe there is another way to match them up. This could very well be a problem of my own ignorance here!) |
Thank you for the reference. Yes, I think those should be in correspondence. So, every overload should create a corresponding |
This PR takes a crack at improving diagnostics for invalid overloaded
function calls as a result of there being no matching overloads.
I think this is a strict improvement on the status quo, but it's
definitely not as good as we can do. In particular, this only
adds sub-diagnostics that points to each unmatched overload, but
doesn't say why each overload does not match. I didn't do this
because 1) this seemed like an improvement we could merge as-is and
2) I wasn't sure how to match up the overloaded
FunctionTypesreturned by
FunctionType::to_overloadedwith theoverloadsin aCallableBinding. It seems like they are in correspondence withone another, but I'm not sure if this is an API guarantee.
I guess ideally (from my uninformed perspective), it would be nice to
have the overloaded
FunctionTypeon theBinding. But I wasn'tcertain how best to go about that, or if that's the right thing to do.
Fixes astral-sh/ty#274