Skip to content
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

gh-87390: Add tests demonstrating current type variable substitution behaviour #32341

Merged
merged 24 commits into from
Apr 29, 2022

Conversation

mrahtz
Copy link
Contributor

@mrahtz mrahtz commented Apr 5, 2022

Here's a reference of what we currently do and don't allow in terms of type variable substitution in aliases. I've marked the cases that I think the current implementation is incorrect on.

Ideally I'd like to merge this as-is, even with the test cases whose expected results aren't what we think they should be - I think it'll make our job implementing the remaining fixes easier if we can just twiddle one line of an existing test case rather than having to remember to copy-paste the test case that each change fixes.

Some cases I'm unsure about are:

  • tuple[T][*tuple[int, ...]]. My understanding is that when it comes to tuple[int, ...], the current behaviour of type checkers is to "Try to find a way to make it work" - I think this should be tuple[int]?
  • class C(Generic[T1, T2]): ...; C[*tuple[int, ...], *tuple[str, ...]]. By similar reasoning, this should work - but should it be C[int, int], C[int, str], or C[str, str]?

@pradeep90 What do you think?

https://bugs.python.org/issue47006

@bedevere-bot bedevere-bot added the tests Tests in the Lib/test dir label Apr 5, 2022
Lib/test/test_typing.py Outdated Show resolved Hide resolved
@mrahtz mrahtz marked this pull request as ready for review April 10, 2022 09:40
Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is a really helpful list.

Lib/test/test_typing.py Outdated Show resolved Hide resolved
Lib/test/test_typing.py Outdated Show resolved Hide resolved
Lib/test/test_typing.py Outdated Show resolved Hide resolved
@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Apr 11, 2022

All commit authors signed the Contributor License Agreement.
CLA signed

@mrahtz mrahtz changed the title bpo-47006: Add tests demonstrating current type variable substitution behaviour gh-87390: Add tests demonstrating current type variable substitution behaviour Apr 11, 2022
@mrahtz
Copy link
Contributor Author

mrahtz commented Apr 11, 2022

Re @JelleZijlstra's suggestion (if I understood right?) to leave unpacked tuples unsimplified across the board: I just realised this means we might have to ease up on checks for the right number of arguments across the board.

For example:

tuple[T1, T2][*tuple[int, str]]

This should be valid, but I think at the moment it would trigger a TypeError, because it looks like we're passing only a single argument *tuple[int, str] to an alias that expects two arguments.

I guess easing up on type argument number checks should be fine though? The alternative would be to have some fancy logic that tries to detect how many type arguments something like *tuple[int, str] actually counts for, but that seems like it could get messy.

@mrahtz
Copy link
Contributor Author

mrahtz commented Apr 15, 2022

Actually, having thought about it more over the past few days, I don't think leaving unpacked tuples unsimplified in all case will work. For example, if we did:

Alias = tuple[T1, bool, T2]
Alias[*tuple[int, str]]  # Should be tuple[int, bool, str]

There's no way we could properly evaluate the result of that unless we did unpack *tuple[int, str]. I've added another test case for this scenario.

Having said that, Jelle, I'm still bearing in mind your comment:

'generic[T]', '[*tuple_type[int, ...]]', 'generic[*tuple_type[int, ...]]' # Should be generic[int]?

I'd rather leave this as is. If we simplify it, we make it impossible for runtime introspection to even tell the difference.

I do agree that for the particular case of *tuple[int, ...] we should leave it unsimplified. I think the only alias involving *tuple[int, ...] that should be valid at runtime for now is generic[*Ts][*tuple[int, ...]].

I'll update the tests accordingly.

Copy link
Contributor

@pradeep90 pradeep90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Damn, GitHub doesn't make it obvious at all whether you've submitted a review or not. I thought I'd submitted this days ago.)

Lib/test/test_typing.py Outdated Show resolved Hide resolved
Lib/test/test_typing.py Outdated Show resolved Hide resolved
Lib/test/test_typing.py Show resolved Hide resolved
Lib/test/test_typing.py Outdated Show resolved Hide resolved
Lib/test/test_typing.py Outdated Show resolved Hide resolved
Lib/test/test_typing.py Outdated Show resolved Hide resolved
('tuple[T, *Ts]', '[int, str, bool]', 'TypeError'), # Should be tuple[int, str, bool]
('Tuple[T, *Ts]', '[int, str, bool]', 'Tuple[int, str, bool]'),

('C[T, *Ts]', '[*tuple_type[int, ...]]', 'C[*tuple_type[int, ...]]'), # Should be C[int, *tuple_type[int, ...]]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what it's worth, there won't be an observable difference from the type checking point of view. For example, if someone tries to pass in this instantiated alias C[*tuple[int, ...]] to a function that expects C[A, *Rs], they will still get A=int, Rs=tuple[int, ...].

So, we have some freedom in what to do here. If getting C[int, *tuple[int, ...]] is hard, it might be ok to keep C[*tuple[int, ...]]

Copy link
Contributor Author

@mrahtz mrahtz Apr 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a case I updated since your review. The vibe I'm getting from @JelleZijlstra is that the kind of logic complexity he'd be happy with (correct me if I'm wrong, @JelleZijlstra) wouldn't be up to dealing with C[T, *Ts][*tuple[int, ...]] - either figuring out the 'right' answer of C[int, *tuple[int, ...]], or accepting it as valid and giving the mostly-right answer of C[*tuple[int, ...]].

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, according to the updated tentative spec at #91162, we'd disallow this altogether - we wouldn't allow any cases where *tuple[int, ...] has to be split among multiple things.

Lib/test/test_typing.py Show resolved Hide resolved
Lib/test/test_typing.py Outdated Show resolved Hide resolved
@JelleZijlstra
Copy link
Member

I came across a case in the typing-extensions tests that is not covered here yet:

from typing import *
Ts = TypeVarTuple('Ts')
T1 = TypeVar('T')
T2 = TypeVar('T')
class C(Generic[T1, T2, Unpack[Ts]]): pass
with self.assertRaises(TypeError):
    C[int]

typing.py currently accepts this without errors.

JelleZijlstra added a commit to JelleZijlstra/typing that referenced this pull request Apr 15, 2022
- Defer to the PEP 646 implementation in typing.py on 3.11
- Adjust some tests accordingly. Noted a bug in
  python/cpython#32341 (comment)
- typing._type_check() is more lenient in 3.11 and no longer rejects ints
- The representation of the empty tuple type changed

Tests pass for me on a 3.11 build from today now.
@mrahtz
Copy link
Contributor Author

mrahtz commented Apr 16, 2022

[@JelleZijlstra]

I came across a case in the typing-extensions tests that is not covered here yet:

from typing import *
Ts = TypeVarTuple('Ts')
T1 = TypeVar('T')
T2 = TypeVar('T')
class C(Generic[T1, T2, Unpack[Ts]]): pass
with self.assertRaises(TypeError):
    C[int]

typing.py currently accepts this without errors.

For the time being, this intentional - we decided not to check arity at runtime for variadics (https://github.com/python/cpython/blob/main/Lib/typing.py#L1331).

JelleZijlstra added a commit to python/typing that referenced this pull request Apr 16, 2022
- Defer to the PEP 646 implementation in typing.py on 3.11
- Adjust some tests accordingly. Noted a bug in
  python/cpython#32341 (comment)
- typing._type_check() is more lenient in 3.11 and no longer rejects ints
- The representation of the empty tuple type changed

Tests pass for me on a 3.11 build from today now.
@JelleZijlstra
Copy link
Member

Going to merge this now so we have the test framework. We'll likely adjust some of the cases though.

@JelleZijlstra JelleZijlstra merged commit f665616 into python:main Apr 29, 2022
@mrahtz mrahtz deleted the subst-tests branch April 29, 2022 20:15
JelleZijlstra added a commit to python/typing_extensions that referenced this pull request May 19, 2022
- Defer to the PEP 646 implementation in typing.py on 3.11
- Adjust some tests accordingly. Noted a bug in
  python/cpython#32341 (comment)
- typing._type_check() is more lenient in 3.11 and no longer rejects ints
- The representation of the empty tuple type changed

Tests pass for me on a 3.11 build from today now.
JelleZijlstra added a commit to python/typing_extensions that referenced this pull request May 19, 2022
- Defer to the PEP 646 implementation in typing.py on 3.11
- Adjust some tests accordingly. Noted a bug in
  python/cpython#32341 (comment)
- typing._type_check() is more lenient in 3.11 and no longer rejects ints
- The representation of the empty tuple type changed

Tests pass for me on a 3.11 build from today now.
JelleZijlstra added a commit to python/typing_extensions that referenced this pull request May 19, 2022
- Defer to the PEP 646 implementation in typing.py on 3.11
- Adjust some tests accordingly. Noted a bug in
  python/cpython#32341 (comment)
- typing._type_check() is more lenient in 3.11 and no longer rejects ints
- The representation of the empty tuple type changed

Tests pass for me on a 3.11 build from today now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news tests Tests in the Lib/test dir topic-typing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants