[ty] Surface matched overload diagnostic directly#18452
Merged
dhruvmanila merged 7 commits intomainfrom Jun 20, 2025
Merged
Conversation
dhruvmanila
added
ty
dhruvmanila
commented
Jun 4, 2025
Comment on lines
112
to
125
Member
Author
There was a problem hiding this comment.
This seems correct to me as the reveal type of x in the following gives us the bound method:
from inspect import getattr_static
from typing import reveal_type
class OverloadExample:
def f(self, x: str) -> int:
return 0
f5 = getattr_static(OverloadExample, "f").__get__
# ty: Revealed type: `bound method Literal[3].f(x: str) -> int` [revealed-type]
reveal_type(f5(3))So, it seems like this test is actually incorrect.
The overload matching did yield a matching overload but still it reported no-matching-overload diagnostic. You can see this in the following logs which suggests that the matching overload is at index 1 after the arity check and the type checking shouldn't fail either because Literal[3] is assignable to ~None:
INFO matching overload index: 1
INFO overload signature: (instance: ~None, owner: type | None = None, /) -> Unknown
INFO argument types: Literal[3]
The reason this was happening earlier is that we'd unconditionally report no-matching-overload without actually checking whether this is the case. This is the branch where that is happening:
Contributor
|
dhruvmanila
force-pushed
the
dhruv/matched-overload-diagnostic
branch
from
f53ee26 to
8cf1e3b
Compare
dhruvmanila
force-pushed
the
dhruv/matched-overload-diagnostic
branch
from
8cf1e3b to
ad34311
Compare
dhruvmanila
force-pushed
the
dhruv/matched-overload-diagnostic
branch
from
ad34311 to
d1a613a
Compare
dhruvmanila
commented
Jun 18, 2025
Comment on lines
+402
to
+405
| ### No matching overloads | ||
|
|
||
| > If argument expansion has been applied to all arguments and one or more of the expanded argument | ||
| > lists cannot be evaluated successfully, generate an error and stop. |
Member
Author
There was a problem hiding this comment.
This test case was missing in the original implementation so just wanted to make sure that we still follow the algorithm correctly with the logic in this PR.
Comment on lines
+1093
to
+1107
| /// The index of the overload that matched for this overloaded callable. | ||
| /// | ||
| /// This is [`Some`] only for step 1 and 4 of the [overload call evaluation algorithm][1]. | ||
| /// | ||
| /// The main use of this field is to surface the diagnostics for a matching overload directly | ||
| /// instead of using the `no-matching-overload` diagnostic. This is mentioned in the spec: | ||
| /// | ||
| /// > If only one candidate overload remains, it is the winning match. Evaluate it as if it | ||
| /// > were a non-overloaded function call and stop. | ||
| /// | ||
| /// Other steps of the algorithm do not set this field because this use case isn't relevant for | ||
| /// them. | ||
| /// | ||
| /// [1]: https://typing.python.org/en/latest/spec/overload.html#overload-call-evaluation | ||
| matching_overload_index: Option<usize>, |
Member
Author
There was a problem hiding this comment.
For step 2, the matching overload would have no error.
For step 3, there would be either multiple matching overloads or no matching overloads. For the former, the return type is populated via overload_call_return_type field.
For step 5, the matching overload is determined by step 6 if it's unambiguous and is the first overload remaining after the filtering process. This is done by the return_type method.
Technically, we could populate this field even for other steps and then it could be used in other methods but I don't think it's worth it right now.
For additional context related to this, the matching_overloads / matching_overloads_mut method differs slightly in that it would not return the matching overload because there was a type checking error but this field would populate the matching overload even in that case. This field could possibly be used in the return_type method instead of relying on overload_call_return_type and matching_overloads but we'd still need to differentiate between union-ing the return types for step 3 and picking the first return type for step 6.
Comment on lines
+1624
to
+1626
| Type::MethodWrapper(MethodWrapperKind::FunctionTypeDunderGet(function)) => { | ||
| Some((FunctionKind::MethodWrapper, function)) | ||
| } |
Member
Author
There was a problem hiding this comment.
I added this as the FunctionType is present right there.
Comment on lines
+2684
to
+2695
| /// Returns the [`OverloadLiteral`] representing this matching overload. | ||
| fn get(&self, db: &'db dyn Db) -> Option<OverloadLiteral<'db>> { | ||
| let (overloads, _) = self.function.overloads_and_implementation(db); | ||
|
|
||
| // TODO: This should actually be safe to index directly but isn't so as of this writing. | ||
| // The main reason is that we've custom overload signatures that are constructed manually | ||
| // and does not belong to any file. For example, the `__get__` method of a function literal | ||
| // has a custom overloaded signature. So, when we try to retrieve the actual overloads | ||
| // above, we get an empty list of overloads because the implementation of that method | ||
| // relies on it existing in the file. | ||
| overloads.get(self.index).copied() | ||
| } |
Member
Author
There was a problem hiding this comment.
This is a bit unfortunate.
We require FunctionType because that will provide us the exact OverloadLiteral to which we can ask for the parameter_span for the invalid-argument-type error.
dhruvmanila
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
carljm
approved these changes
Jun 18, 2025
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
This looks great, thank you! Big improvement in the usefulness of these diagnostics.
Member
Author
|
I ended up adding dedicated snapshot diagnostic tests under |
Member
Author
|
Sorry, I totally missed requesting @BurntSushi review. I'll probably go ahead and merge this but happy to address any feedback in a follow-up PR. |
dcreager
added a commit
that referenced
this pull request
Jun 20, 2025
* main: (21 commits) [`flake8-logging`] Avoid false positive for `exc_info=True` outside `logger.exception` (`LOG014`) (#18737) [`flake8-pie`] Small docs fix to `PIE794` (#18829) [`pylint`] Ignore __init__.py files in (PLC0414) (#18400) Avoid generating diagnostics with per-file ignores (#18801) [`flake8-simplify`] Fix false negatives for shadowed bindings (`SIM910`, `SIM911`) (#18794) [ty] Fix panics when pulling types for `ClassVar` or `Final` parameterized with >1 argument (#18824) [`pylint`] add fix safety section (`PLR1714`) (#18415) [Perflint] Small docs improvement to `PERF401` (#18786) [`pylint`] Avoid flattening nested `min`/`max` when outer call has single argument (`PLW3301`) (#16885) [`ruff`] Added `cls.__dict__.get('__annotations__')` check (`RUF063`) (#18233) [ty] Use `HashTable` in `PlaceTable` (#18819) docs: Correct collections-named-tuple example to use PascalCase assignment (#16884) [ty] ecosystem-analyzer workflow (#18719) [ty] Add support for `@staticmethod`s (#18809) unnecessary_dict_kwargs doc - a note on type checking benefits (#18666) [`flake8-pytest-style`] Mark autofix for `PT001` and `PT023` as unsafe if there's comments in the decorator (#18792) [ty] Surface matched overload diagnostic directly (#18452) [ty] Report when a dataclass contains more than one `KW_ONLY` field (#18731) [`flake8-pie`] Add fix safety section to `PIE794` (#18802) [`pycodestyle`] Add fix safety section to `W291` and `W293` (#18800) ...
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR resolves the way diagnostics are reported for an invalid call to an overloaded function.
If any of the steps in the overload call evaluation algorithm yields a matching overload but it's type checking that failed, the
no-matching-overloaddiagnostic is incorrect because there is a matching overload, it's the arguments passed that are invalid as per the signature. So, this PR improves that by surfacing the diagnostics on the matching overload directly.It also provides additional context, specifically the matching overload where this error occurred and other non-matching overloads. Consider the following example:
We get:
Test Plan
Update test cases, resolve existing todos and validate the updated snapshots.