-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
bpo-44653: Support typing types in parameter substitution in the union type. #27247
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
Conversation
bf9d793
to
8f98785
Compare
if (is_arg_unionable == 0) { | ||
PyErr_Format(PyExc_TypeError, | ||
"Each union argument must be a type, got %.100R", arg); | ||
if (nargs == 0) { |
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 am not sure that this situation is possible now, but just for the case...
Py_INCREF(res); | ||
for (Py_ssize_t iarg = 1; iarg < nargs; iarg++) { | ||
PyObject *arg = PyTuple_GET_ITEM(newargs, iarg); | ||
Py_SETREF(res, PyNumber_Or(res, arg)); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
Yes, it has quadratic complexity. But I think that most union types has less than 10 args. If it be a problem (many thousands args in real code) we can add some optimizations, but for now I prefer simplicity.
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.
Good point! Old union de-duplication also had quadratic complexity. So I think it's ok for now. Especially since this is only for substitution, which isn't a common use case either.
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.
LGTM, just a minor comment about tests.
Lib/test/test_types.py
Outdated
eq(x[P], int | P | bytes) | ||
eq(x[typing.Concatenate[int, P]], int | typing.Concatenate[int, P] | bytes) |
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.
ParamSpec
and Concatenate
cannot be unioned (or at least it is undefined behavior). So we don't have to test these. They don't throw TypeError
because we decided to loosen type checks for them. In the future, these tests might break if their implementation changes. So it's best to leave them out.
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.
Then it is a separate 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.
It's not really a bug, we made the decision quite early on to stop advanced runtime type checking new types, and leave it to the type checker or other introspection libraries :). See Guido's comments https://bugs.python.org/msg382858.
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.
Jest verified -- no, defining __or__
for _TypeVarLike
is not a bug (it can make sense for ParamSpec
), but in future we need to add additional checks for substitutions.
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.
Sorry, I wasn't very clear. Yes, __or__
for _TypeVarLike
is not a bug during runtime. But using it with union should make no sense to the static type checker. Please see the grammar for the ParamSpec
PEP:
https://www.python.org/dev/peps/pep-0612/#valid-use-locations
Thanks @serhiy-storchaka for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10. |
GH-27296 is a backport of this pull request to the 3.10 branch. |
…n type. (pythonGH-27247) (cherry picked from commit 2e3744d) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
https://bugs.python.org/issue44653