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

[Reverted] Make all single-constraint TypeVars to use bounds #804

Merged

Conversation

Michael0x2a
Copy link
Contributor

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__?)

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__`?)
@ambv
Copy link
Contributor

ambv commented Jan 1, 2017

Also unsure about the unittest case but we can fix it forward if some edge reveals we should have gone with _FT = Callable[[Any], Any] instead.

@ambv ambv merged commit b46366e into python:master Jan 1, 2017
@gvanrossum
Copy link
Member

Now that I've read this over carefully, I unfortunately think that the unittest fix is incorrect. I have this simple test:

from unittest import skip, TestCase

class TC(TestCase):
  @skip("not today")
  def testFoo(self) -> None:
    assert False

Before this PR, that works, but with the change to unittest/__init__.pyi, it gives an error on the @skip line:

__tmp__.py:4: error: Argument 1 has incompatible type Callable[[TC], None]; expected None

I believe the problem is that you currently can't return a generic type from a function without generic arguments. Apparently the old form worked around this (perhaps it just removed the genericity from the return type, which makes sense as an edge case of a TypeVar with value constraints). There's an open issue for this: python/mypy#1551

So sadly I'm going to revert this -- we first need to fix the latter issue. :-(

gvanrossum added a commit that referenced this pull request Jan 2, 2017
Reverts #804.

Reason: until python/mypy#1551 is fixed this gives an error whenever @Skip() is used.

Specifically see #804 (comment).
@Michael0x2a
Copy link
Contributor Author

Bleh, I figured there was something odd afoot. I'm not at a computer right now, but I can poke around/try and find a workaround sometime later today or tomorrow.

@Michael0x2a
Copy link
Contributor Author

@gvanrossum -- I'm slowly poking around with the TypeVar stuff, but in the interim, do you think using _FT = Callable[..., Any] would be a good workaround? (I don't think _FT = Callable[[Any], Any] actually works since then you couldn't add @skip or similar annotations to top-level functions that take in no arguments).

@gvanrossum
Copy link
Member

Yeah, I think _FT = Callable[..., Any] should be fine, as long as we add a TODO comment showing the correct TypeVar definition and a mention of the issue it's waiting for.

@gvanrossum gvanrossum changed the title Make all single-constraint TypeVars to use bounds [Reverted] Make all single-constraint TypeVars to use bounds Mar 29, 2017
@gvanrossum
Copy link
Member

I'm going to attempt a new band-aid fix so we can finally move forward with #2626.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants