Skip to content

Allow returning inferred None from functions #5663

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

Merged
merged 4 commits into from
Sep 26, 2018

Conversation

ilevkivskyi
Copy link
Member

Fixes #4214

As discussed in the issue, assigning an inferred (from generics, overloads, or lambdas) None type should be always allowed, while prohibiting assigning function calls that have an explicit None return type may have some value (at least people don't complain much about this).

Note: this error is quite nasty for dataclasses, where it flags a legitimate typical use case (see tests).

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using NoneTyp attribute to handle an edge case such as this seems like an overkill. This increases complexity and adds some overhead with a pretty small gain.

Instead, I'd rather rewrite the None return check to look at whether the callable refers to something with an explicit declared None return type (look for the annotation in the node, not the inferred type). It's fine if the semantics of the check change slightly as this is not required for correctness.

@ilevkivskyi
Copy link
Member Author

OK, I have rewritten it without a flag and I like the new semantics even more, it looks more consistent.

This however required fixing an unrelated issue: previously all Vars at global and local scopes were unconditionally marked as inferred, this is a one line fix however, so I don't want to make a separate PR for this.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update, I like how the new check works. There seems to be a problem with methods though (see my comment).

o = a() # E: Function does not return a value
o = A().g(a) # E: "g" of "A" does not return a value
o = a()
o = A().g(a)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this should still generate an error, since A.g has an explicit None return type.

@@ -936,8 +936,8 @@ main:3: error: "A" not callable

a, o = None, None # type: (A, object)
a = f() # E: "f" does not return a value
o = a() # E: Function does not return a value
o = A().g(a) # E: "g" of "A" does not return a value
o = a()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be okay for this to still generate an error, since __call__ has an explicit None return, but since this is an edge case it's not that important. Might be worth adding an issues about this though.

@ilevkivskyi
Copy link
Member Author

@JukkaL
Yes, makes sense, I added some more special-casing. I would however avoid further special-casing as this is (as you mentioned before) not required for correctness.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this is enough special casing for now. Looks good now!

@ilevkivskyi ilevkivskyi merged commit 8df6adc into python:master Sep 26, 2018
@ilevkivskyi ilevkivskyi deleted the no-complain-none branch September 26, 2018 02:14
@msullivan
Copy link
Collaborator

This seems to have introduced a regression where we will emit the Function does not return a value error message inside of functions that shouldn't be type checked:

def foo() -> None:
    pass

def lol():
    x = foo()

produces an error

@ilevkivskyi
Copy link
Member Author

Hm, interesting, I will check this.

msullivan added a commit that referenced this pull request Sep 26, 2018
msullivan added a commit that referenced this pull request Sep 26, 2018
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