[ty] Emit "arguments after ** must be a mapping" before overload resolution#22921
Merged
dhruvmanila merged 2 commits intoastral-sh:mainfrom Jan 30, 2026
Merged
Conversation
denyszhak
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
denyszhak
force-pushed
the
ty/kwargs-mapping-error
branch
from
54c1d56 to
1f91ae1
Compare
Typing conformance resultsNo changes detected ✅ |
|
denyszhak
commented
Jan 28, 2026
| for keyword in arguments.keywords.iter().filter(|k| k.arg.is_none()) { | ||
| let mapping_type = self.expression_type(&keyword.value); | ||
|
|
||
| let is_paramspec = matches!( |
Contributor
Author
There was a problem hiding this comment.
Does it make sense to skip the mapping check for ParamSpec and Union[ParamSpec, Unknown] types here?
The test under pep695/paramspec.md would fail otherwise also
Flagging it here would be a premature false positive
Member
There was a problem hiding this comment.
Yeah, we'd need to skip those in here. It's a bit unfortunate that this would lead to duplicate code. If you can find a way to de-duplicate it, that would be great. I think one way would be to have a as_paramspec_typevar method which returns Option<Type> if it's P or P | Unknown.
Contributor
Author
There was a problem hiding this comment.
Updated to use common method across all cases where this duplicated logic was used
Contributor
Author
|
The failing CI is from pre-current version of the commit I believe |
Contributor
Author
|
This new check now points to the actual value (expression after **) not the whole **expr which seems more precise to be and okay? For some cases like cloud-init it's now invalid-argument-type and not no-matching-overload |
Member
Yeah, this seems good to me. |
dhruvmanila
reviewed
Jan 29, 2026
dhruvmanila
changed the title
** must be a mapping" before overload resolution
dhruvmanila
approved these changes
Jan 30, 2026
Member
dhruvmanila
left a comment
dhruvmanila
left a comment
There was a problem hiding this comment.
ty for following through with this additional PR :)
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
Closes astral-sh/ty#2653
Move the KeywordsNotAMapping check from bind.rs to builder.rs, matching the existing pattern for starred argument iterability. This ensures users see "must be a mapping type" instead of "no matching overload" when passing a non-mapping **kwargs to an overloaded function.
Test Plan
Added test case for **kwargs with non-mapping types on overloaded functions in function.md.