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

Allow yielding result of function that returns None #1933

Open
gvanrossum opened this issue Jul 23, 2016 · 6 comments
Open

Allow yielding result of function that returns None #1933

gvanrossum opened this issue Jul 23, 2016 · 6 comments
Labels
bug mypy got something wrong false-positive mypy gave an error on correct code priority-1-normal

Comments

@gvanrossum
Copy link
Member

gvanrossum commented Jul 23, 2016

UPDATE: see below for current status #1933 (comment)

This is a spin-off from PR #1808. @ddfisher and I cannot agree on what the default type should be in the three methods get_generator_yield_type(), get_generator_receive_type(), get_generator_return_type().

The setting: when the user defines a generator function, they can specify a return type. The return type is required to be a supertype of Generator; this constrains it to Iterator, Iterable, object (and Any, which is special in other ways).

The Generator type has three type parameters:

  • ty, the yield type: the type of y in yield y
  • tc, the receive type: the type of c in c = yield ...
  • tr, the return type: the type of r in return r

Note that tr is fairly new -- it was introduced in Python 3.3 by PEP 380 for the benefit of yield from. tc is older, it dates back to Python 2.5 and PEP 342 (it is the value sent to the generator using the .send() method). ty is the oldest; when yield was originally introduced in Python 2.2 by PEP 255, it was a statement. Before PEP 380, return in a generator function was not allowed to return a value.

When a generator function is just intended to be used as an iterator, there's no need to specify tc and tr, and the recommended type is Iterator[ty]. (Iterable[ty] is also allowed and means roughly the same thing.) In this case the user of the iterator has no access to the .send() method. Hence, yield might as well be considered a statement, since it's never going to return anything other than None. Likewise, there's no point is returning a value with a return statement since the user of the iterator in all likelihood is just going to iterate over it until it raises StopIteration (since that's all that regular iterators do).

This is where @ddfisher and I disagree -- I find it useful if the defaults for tc and tr are None (or perhaps Void), so that mypy will flag an error in the generator body if you either try to use yield ... as an expression, or try to return a value using return <expr>. David (IIUC) believes that both should be Any, giving the reason that "it should be allow to pointless but harmless things." (The code currently has tc default to Void and tr default to Any. Go figure.)

My problem with David's position is that I want mypy to be strict about pointless things that are likely the result of either a misunderstanding or a refactoring gone bad. I have seen enough people make the mistake of accidentally writing return v instead of yield v in a generator that I want this to be flagged as an error. There's also the somewhat analogous case of a regular function declared to return None -- it would be pointless but harmless to return some value here, so maybe David believes that we shouldn't flag that as an error either?

I'm not sure why the existing code (written by David) has Void as the default for tc, but I think it may be because this parameter of Generator is contravariant. So hopefully we at least agree here. :-)

David, please feel free to explain your position more fully (I think you were calling on the formal type system but we ran out of time in our in-person discussion at that point).

@ddfisher
Copy link
Collaborator

To clarify: I do not think that tc should be Any. I had previously thought thattr should be forall T. T, (which we can write as Any, though object would probably be better), but after reading PEP 380 I need to think more about it.

My thoughts on tc:
None of the superclasses of Generator have a send method, so tc can never matter in an external-facing way (i.e. to anything outside of the generator function). Inside the generator function, we know that in all statements of the form c = yield ..., c will have the value None (because no values can be sent into the generator). Therefore, tc should have the type NoneType. Or Void, if you want to parallel the way None-returning functions work (which I think should also be changed, but that's a whole different can of worms). I think tc working in this way is uncontentious, modulo the choice of NoneType vs Void.

My thoughts on tr:
After reading PEP 380, I realized you can write r = yield from some_iterator, which will leave r with the value None. This means that Generators which are used as Iterators had better also return None. This would mean that if I had a Generator[int, None, str] I can't use it as an Iterator[int], which perhaps is reasonable. I'm starting to feel a bit addled by my cold, so I'll finish thinking this through later...

@JukkaL
Copy link
Collaborator

JukkaL commented Jul 27, 2016

I haven't thought about this very carefully, but I'd rather generate errors on questionable code even if doesn't fail at runtime, as in this case the rejected code seems pointless and it's likely almost always an error. Some specifics below.

About tc: Void seems reasonable to me, assuming this results in a reasonable error message. If not, None sounds okay. If the return type is Iterator or Iterable, we can probably outright reject yield expressions.

About tr: If a function returns Iterable or Iterator, I'd probably reject return statements with a value. Again Void or None seems reasonable, but special casing an error message would be reasonable if the error message doesn't look helpful otherwise.

About r = yield from some_iterator: Perhaps we can reject this if the expression is an Iterable or Iterator, and thus we'd sidestep the issue.

About being able to use Generator[int, None, str] as an Iterator[int]: For consistency it seems like it should be allowed, and real code expecting yield from to return None doesn't seem like a common thing.

@gvanrossum
Copy link
Member Author

Currently, the error for x = yield y as well as that for a = yield from b is Function does not return a value if the yield or yield from is
seen as having no value (for yield y this is when the containing
generator is declared as an Iterator or as Generator[..., None, ...]; for
yield from b this is when the called iterator (i.e. b) is declared as
Generator[..., ..., None] (but not if it's an Iterator, because the
current code defaults to Any for rv).

We can keep this issue open as a reminder to improve the error messages. I
can change the default return type to Void/None in phase 2 of my
async/await work (PR #1946).

@ilevkivskyi
Copy link
Member

Here is an update on the issue:

def f() -> None:
    pass

def g() -> Iterator:
    yield from 1  # "yield from" can't be applied to "int", which is a good error message
    x = f()  # "f" does not return a value, which is a reasonable error
    yield f()  # "f" does not return a value, which is a false positive, since we may just want to give control to "f"

I am updating labels and title accordingly.

@ilevkivskyi ilevkivskyi added bug mypy got something wrong false-positive mypy gave an error on correct code and removed feature needs discussion labels May 17, 2018
@ilevkivskyi ilevkivskyi changed the title Discussion about default generator parameter types Allow yielding result of function that returns None May 17, 2018
@JelleZijlstra
Copy link
Member

I don't think the error on yield f() is a false positive; it's basically the same as return f() where f() returns None.

@ilevkivskyi
Copy link
Member

I don't think the error on yield f() is a false positive; it's basically the same as return f() where f() returns None.

I think both are legitimate statements, x = f() is prohibited because it's most likely an error, but we don't want users to write for example:

def g():
    ...
    f()
    yield None

also this error is confusing in case of yield from f(), we should use the same as in yield from 1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong false-positive mypy gave an error on correct code priority-1-normal
Projects
None yet
Development

No branches or pull requests

6 participants