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.
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
Use TypeVarTuple in our APIs #2881
Use TypeVarTuple in our APIs #2881
Changes from 15 commits
bd5a451
97d6e0e
9bcb185
6f8153f
52c69c8
13d5d63
9ed1d08
0933718
a0e9966
5899ad4
e12273f
5a39c8b
5b0215c
342733c
749ef94
1a0daa6
ff01a82
b91b4b9
972961e
bbd436d
bac6558
9783f8b
e1edda0
176092b
2903a59
d9d7a3d
580627d
e012611
9f94f97
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
edit: I thought that would work (and it does if
*args: Unpack[PosArgT]
is gone) but it doesn't. I believe the issue is that the type var tuple can have a finite amount of arguments and then... kinda overflow, at least in some cases right? So like, calling withint, str, TaskStatus[str]
whenPosArgT
is just[int, str]
would mean thatStatusT_co
would bestr
right?Errr, so we're promising that anything with this protocol can call it as such. But we're also passing functions that don't allow that? (actually, maybe not, and that's an actual bug. But as is our protocol shouldn't accept
(a: int, *, task_status: TaskStatus[str])
)edit edit: ok my analysis above is totally incorrect, only keeping it to prevent someone from going down the same path i went down.
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.
Unfortunately no, the
*args
already makes it keyword-only.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.
Yeah sorry about the repeated confusion with this thread,
TypeVarTuple
is getting to my head :(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.
Yeah it's real confusing. Since
task_status
is keyword-only it shouldn't get mixed up. As you can see with the type-tests, if it's being assigned to an annotated type, Mypy can do bidirectional inference and figure it out. But if it's not annotated or just a bare call, it gets confused and assumes it'sNever
. I think it's a bug.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.
I agree. I took a look at mypy's code for the last 45 minutes and spotted:
UninhabitedType
(str value:Never
... not literallyNever
) to us)TypeVarTuple
gets erased totuple[Any, ...]
and our callable doesn't take in infinite parameters.Callable[[Unpack[Ts], T], None]
has an implicitUnpack
) but means that mypy focuses on the typevartuple, not making a constraint for our type variable.I'm not really sure how to solve this myself, a bug report would probably work nicely.
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.
@A5rocks probably worth cross-posting your analysis in the issue that teamspen opened.