Skip to content

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

Merged
ambv merged 1 commit into
python:masterfrom
Michael0x2a:fix-typevars-with-only-one-constraint
Jan 1, 2017
Merged

ambv merged 1 commit into
python:masterfrom
Michael0x2a:fix-typevars-with-only-one-constraint

Conversation

@Michael0x2a

Copy link
Copy Markdown
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

ambv commented Jan 1, 2017

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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