Skip to content

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

Merged
merged 2 commits into from
Jul 22, 2021

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Jul 19, 2021

if (is_arg_unionable == 0) {
PyErr_Format(PyExc_TypeError,
"Each union argument must be a type, got %.100R", arg);
if (nargs == 0) {
Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member

@Fidget-Spinner Fidget-Spinner left a 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.

Comment on lines 805 to 806
eq(x[P], int | P | bytes)
eq(x[typing.Concatenate[int, P]], int | typing.Concatenate[int, P] | bytes)
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

@Fidget-Spinner Fidget-Spinner Jul 20, 2021

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.

Copy link
Member Author

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.

Copy link
Member

@Fidget-Spinner Fidget-Spinner Jul 20, 2021

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

@ambv ambv merged commit 2e3744d into python:main Jul 22, 2021
@miss-islington
Copy link
Contributor

Thanks @serhiy-storchaka for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-27296 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Jul 22, 2021
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 22, 2021
…n type. (pythonGH-27247)

(cherry picked from commit 2e3744d)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
ambv pushed a commit that referenced this pull request Jul 22, 2021
…n type. (GH-27247) (#27296)

(cherry picked from commit 2e3744d)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants