Make TypeVars with only one constraint illegal #2626
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
At runtime, doing something like `T = TypeVar('T', str)` results in the
following exception:
TypeError: A single constraint is not allowed
However, mypy itself seems to ignore this particular error, which feels
incongruous.
This commit slightly tweaks the semanal phase to emit an error message
in this case to mirror the existing runtime behavior implemented in the
typing module.
This commit fixes some broken unit tests that appeared to have been using TypeVar incorrectly. This commit does not cause the entire test suite to pass due to existing errors within typeshed that need to be fixed first.
Michael0x2a
added a commit
to Michael0x2a/typeshed
that referenced
this pull request
Jan 1, 2017
According to the documentation in the typing module, TypeVars cannot have only a single constraint. Attempting to do so will actually result in an exception at runtime. (However, this error is currently ignored by mypy -- see python/mypy#2626 for a related pending pull request). This commit changes all instances of TypeVars using a single constraint (e.g. `T = TypeVar('T', Foo)`) to use bounds instead (e.g. `T = TypeVar('T', bound=Foo)`. This seems to be the correct fix for plistlib after reading the module docs, but it's less obvious this is correct for unittest. The unittest module originally had `_FT = TypeVar('_FT', Callable[[Any], Any])` -- an alternative fix would have been to do `_FT = Callable[[Any], Any]`. Although I'm not entirely sure what it means to have a bound be a Callable, I decided to make the assumption that the original authors probably meant to use TypeVars instead of type aliases for a reason (possibly to handle classes implementing `__call__`?)
|
...well, I thought this would be a quick fix, but it turned out to be a little more involved then I expected. The tests are currently failing because typeshed is using single-constraint TypeVars in a few places. I have a pull request to fix this at python/typeshed#804. Once that's accepted and mypy syncs typeshed, I can rebase this pull request and hopefully that'll make the tests pass. |
ambv
pushed a commit
to python/typeshed
that referenced
this pull request
Jan 1, 2017
According to the documentation in the typing module, TypeVars cannot have only a single constraint. Attempting to do so will actually result in an exception at runtime. (However, this error is currently ignored by mypy -- see python/mypy#2626 for a related pending pull request). This commit changes all instances of TypeVars using a single constraint (e.g. `T = TypeVar('T', Foo)`) to use bounds instead (e.g. `T = TypeVar('T', bound=Foo)`. This seems to be the correct fix for plistlib after reading the module docs, but it's less obvious this is correct for unittest. The unittest module originally had `_FT = TypeVar('_FT', Callable[[Any], Any])` -- an alternative fix would have been to do `_FT = Callable[[Any], Any]`. Although I'm not entirely sure what it means to have a bound be a Callable, I decided to make the assumption that the original authors probably meant to use TypeVars instead of type aliases for a reason (possibly to handle classes implementing `__call__`?)
|
I merged your PR in typeshed. |
|
Can you do the typeshed sync in this PR? Then the tests will hopefully pass and we can merge it. |
|
Sorry, there's a complication. See my comments in python/typeshed#804, which I am going to revert. :-( |
gvanrossum
pushed a commit
to python/typeshed
that referenced
this pull request
Mar 29, 2017
gvanrossum
added a commit
to python/typeshed
that referenced
this pull request
Mar 29, 2017
…1118) Another attempt for #804, to unblock python/mypy#2626. There's a work-around here for python/mypy#1551.
|
I guess I need to sync typeshed and then try again. |
But with synced typeshed.
|
Whee! Finally this is merged. Sorry for the long delay! |
|
@gvanrossum wow, I haven't had time to think about this at all this quarter -- I can't believe this actually got pushed through! Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
At runtime, doing something like
T = TypeVar('T', str)results in the following exception:However, mypy itself seems to ignore this particular error, which feels incongruous.
This pull request slightly tweaks the semanal phase to emit an error message in this case to mirror the existing runtime behavior implemented in the typing module.