Type variable tuple variance - #741
Conversation
Sorry, something went wrong.
JelleZijlstra
left a comment
There was a problem hiding this comment.
- Can you also set tvt.__bound__ = bound for consistency?
- Can you add tests and a changelog entry?
Sorry, something went wrong.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #741 +/- ##
==========================================
+ Coverage 97.19% 97.26% +0.06%
==========================================
Files 3 3
Lines 7815 7862 +47
==========================================
+ Hits 7596 7647 +51
+ Misses 219 215 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Sorry, something went wrong.
| def __init__(self, name, *, bound=None, covariant=False, contravariant=False, | ||
| infer_variance=False, default=NoDefault): | ||
| self.__name__ = name | ||
| self.__covariant__ = bool(covariant) |
There was a problem hiding this comment.
Why does this branch call bool() on the variance-related arguments and _type_check on bound and the above one doesn't? We should have things behave the same way across versions.
Sorry, something went wrong.
There was a problem hiding this comment.
the implementation of ParamSpec also has this discrepancy, i will update it accordingly
should _set_default also invoke _type_check?
Sorry, something went wrong.
There was a problem hiding this comment.
Sorry I didn't come back here for a while. I think bool() is right for the boolean fields since it matches what we do for TypeVar. However, _type_check is actually wrong for ParamSpec and TypeVarTuple bounds, since it assumes a single type. This is actually a bug in CPython too, it rejects TypeVarTuple bounds like (int, str) that should be valid.
Sorry, something went wrong.
There was a problem hiding this comment.
it rejects TypeVarTuple bounds like (int, str) that should be valid.
is this in the spec? perhaps this is unrelated to this change
Sorry, something went wrong.
There was a problem hiding this comment.
Ah right, we're not actually adding support for bounds here, only for variance. Might as well get this fixed in the runtime though.
Sorry, something went wrong.
There was a problem hiding this comment.
Sorry, something went wrong.
| self.assertEqual(repr(Ts_contra), '-Ts_contra') | ||
| self.assertEqual(repr(Ts_infer), 'Ts_infer') | ||
| else: | ||
| # Not worth creating our own version of TypeVarTuple |
There was a problem hiding this comment.
I don't understand this comment, we do create our own version in order to add the variance arguments, so we might as well update the repr too.
In general the tests ideally shouldn't have version-dependent branches: it's a goal for typing-extensions for all versions to behave the same.
Sorry, something went wrong.
There was a problem hiding this comment.
it's adapted from the ParamSpec test which does similar. we do create a class for TypeVarTuple, but it doesn't return Self, so we can't update the repr
Sorry, something went wrong.
There was a problem hiding this comment.
typing_extensions/src/test_typing_extensions.py
Line 5769 in 83caa59
Sorry, something went wrong.
python/cpython#148212