[ty] Avoid overload errors when detecting dataclass-on-tuple#22687
Merged
charliermarsh merged 2 commits intomainfrom Jan 19, 2026
Merged
[ty] Avoid overload errors when detecting dataclass-on-tuple#22687charliermarsh merged 2 commits intomainfrom
charliermarsh merged 2 commits intomainfrom
Conversation
Typing conformance resultsNo changes detected ✅ |
|
charliermarsh
commented
Jan 18, 2026
| // (possibly with semantic errors), report its errors directly instead | ||
| // of the generic "no matching overload" message. | ||
| if let MatchingOverloadIndex::Single(matching_overload_index) = | ||
| self.matching_overload_index() |
Member
Author
There was a problem hiding this comment.
I... think this was impossible on main? Because if we had exactly one matching overload without errors, we'd never call report_diagnostics? But now, we can have one overload that matches but still emits an error.
Member
There was a problem hiding this comment.
Yeah, I think that's correct, I reached the same conclusion in #22688 (comment).
I think the body of this if branch is same as the one right above it, so both of these can be merged? Edit: Oh, merging that might be difficult but maybe we could extract out the logic into an inner function.
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-parameter-default |
0 | 0 | 7 |
invalid-return-type |
0 | 1 | 6 |
invalid-argument-type |
0 | 1 | 4 |
invalid-assignment |
0 | 0 | 5 |
possibly-missing-attribute |
0 | 3 | 1 |
invalid-await |
0 | 2 | 0 |
unresolved-attribute |
0 | 0 | 2 |
unused-ignore-comment |
1 | 0 | 0 |
| Total | 1 | 7 | 25 |
Member
Author
|
So in But not the following: On main, we show both. |
Member
Author
|
I think that's actually... correct? Consider In this branch, we have |
charliermarsh
force-pushed
the
charlie/data-decorator
branch
2 times, most recently
from
ec73f1f to
6abafd1
Compare
Member
Author
|
I am going to see if I can fix that separately here. |
charliermarsh
force-pushed
the
charlie/data-decorator
branch
from
6abafd1 to
c008e7c
Compare
Member
Author
|
I moved that change into #22688. |
charliermarsh
force-pushed
the
charlie/data-decorator
branch
from
c008e7c to
6993da4
Compare
charliermarsh
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
dhruvmanila
approved these changes
Jan 19, 2026
Comment on lines
2276
to
2277
| // If any matching overload has errors, we have binding errors. | ||
| if !first_overload.errors.is_empty() { |
Member
There was a problem hiding this comment.
These errors would be "semantic" errors as defined in affects_overload_resolution, right? If so, the comment could be updated
Comment on lines
3934
to
3942
| fn as_result(&self) -> Result<(), CallErrorKind> { | ||
| if !self.errors.is_empty() { | ||
| if self | ||
| .errors | ||
| .iter() | ||
| .any(BindingError::affects_overload_resolution) | ||
| { | ||
| return Err(CallErrorKind::BindingError); | ||
| } | ||
| Ok(()) |
Member
There was a problem hiding this comment.
The as_result name for this method seems a bit misleading now that we don't account for all errors here (only the ones that affect overload resolution), maybe we should rename this? e.g., has_errors_affecting_overload_resolution (too long), failed_overload_resolution, etc. This method is also only called in matching_overloads and matching_overloads_mut so it seems ok to rename it to be specific to this usage.
| // (possibly with semantic errors), report its errors directly instead | ||
| // of the generic "no matching overload" message. | ||
| if let MatchingOverloadIndex::Single(matching_overload_index) = | ||
| self.matching_overload_index() |
Member
There was a problem hiding this comment.
Yeah, I think that's correct, I reached the same conclusion in #22688 (comment).
I think the body of this if branch is same as the one right above it, so both of these can be merged? Edit: Oh, merging that might be difficult but maybe we could extract out the logic into an inner function.
charliermarsh
force-pushed
the
charlie/data-decorator
branch
from
6993da4 to
fc557c5
Compare
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
3 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
Fixes some TODOs introduced in #22672 around cases like the following:
On main,
dataclass(NT)emits# error: [no-matching-overload]instead, which is wrong -- the overload does match! On main, the logic proceeds as follows:dataclasshas two overloads:dataclass(cls: type[_T], ...) -> type[_T]dataclass(cls: None = None, ...) -> Callable[[type[_T]], type[_T]]dataclass(NT)is called:NTmatchestype[_T]... but theninvalid_dataclass_target()runs and addsInvalidDataclassApplicationerrorNTdoesn't matchNone, so we have a type error.matching_overload_index()filters byoverload.as_result().is_ok(), which checks iferrors.is_empty(). Since both overloads have errors, neither matches...Instead, we now differentiate between non-matching errors, and errors that occur when we do match, but the call has some other semantic failure.