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

contextmanager decorators require Generators #2773

Closed
wants to merge 1 commit into from
Closed

contextmanager decorators require Generators #2773

wants to merge 1 commit into from

Conversation

asottile
Copy link
Contributor

Resolves #2772

@asottile
Copy link
Contributor Author

the sample from the issue now fails with:

$ mypy --custom-typeshed . t.py 
t.py:4: error: Argument 1 to "contextmanager" has incompatible type "Callable[[], Iterator[int]]"; expected "Callable[..., Generator[<nothing>, Any, Any]]"

I wasn't sure what to put for the other generic parameters for Generator / AsyncGenerator so I set them as Any

Also wasn't sure how else to test this so hopefully CI points me in the right direction if I've messed it up 😆

@asottile
Copy link
Contributor Author

hmmm, CI is failing -- looks like AsyncGenerator was introduced in 3.5.4 -- I don't know what to do to resolve that though :(

@ilevkivskyi
Copy link
Member

Will this allow:

@contextmanager
def func() -> Iterator[int]:
    yield 1  # explicit

If not, then there is no way this can be merged. There are thousands existing annotations like this, and we can't make them all fail because of a false negative in rare corner case.

@asottile
Copy link
Contributor Author

it will not, Iterator[T] isn't the correct type for a generator -- but it's understandable that you wouldn't want to break things. and as far as I know there isn't a warning mechanism to nudge people in the right direction.

my original issue is because of a particularly nasty bug enabled by a developer's misunderstanding of how contextmanager works (that, surprise surprise, was only triggered in an exceptional case). that mypy failed to catch this statically was the real kicker because if the annotation was correct in typeshed this would have been caught before review and long before production. (@contextmanager decorating a function which returns an actual Iterator[T] works in non-exception cases)

@ilevkivskyi
Copy link
Member

and as far as I know there isn't a warning mechanism to nudge people in the right direction

Actually there is basic support for warnings in mypy. But currently warnings still give return code 1. There is some (slowly moving) ongoing work on how to treat errors/warnings more systematically.

@JelleZijlstra
Copy link
Member

I agree with @ilevkivskyi that we can't afford to break all existing annotations that use Iterator for contextmanagered functions. Maybe for now we can just add a comment in the stub to clarify.

Another way forward could be to add special-casing in mypy that detects when a generator function has Iterator[T] as its declared return type and changes it to Generator[T, None, None].

@msullivan
Copy link
Contributor

Another way forward could be to add special-casing in mypy that detects when a generator function has Iterator[T] as its declared return type and changes it to Generator[T, None, None].

This seems like a good idea to me

@srittau srittau added the status: deferred Issue or PR deferred until some precondition is fixed label Oct 10, 2019
@srittau
Copy link
Collaborator

srittau commented Oct 10, 2019

I have marked this as deferred for now. While I think that in general typeshed should have exact types, but backwards compatibility is an issue. Until that is solved/worked around we can't merge this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: deferred Issue or PR deferred until some precondition is fixed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

contextlib.contextmanager allows Iterator[T] -- should probably be Generator[T, None, None]?
5 participants