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

Improve message when function is missing return type #6773

Conversation

@allisonking
Copy link
Contributor

commented May 6, 2019

Fixes #5952

@ilevkivskyi
Copy link
Collaborator

left a comment

Thank you! This looks good, but I have couple small suggestions.

@@ -984,7 +984,12 @@ def is_unannotated_any(t: Type) -> bool:
check_incomplete_defs = self.options.disallow_incomplete_defs and has_explicit_annotation
if show_untyped and (self.options.disallow_untyped_defs or check_incomplete_defs):
if fdef.type is None and self.options.disallow_untyped_defs:
self.fail(message_registry.FUNCTION_TYPE_EXPECTED, fdef)
if (not len(fdef.arguments) or

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi May 6, 2019

Collaborator

You don'y need len(...) here, just not fdef.arguments.

@@ -984,7 +984,12 @@ def is_unannotated_any(t: Type) -> bool:
check_incomplete_defs = self.options.disallow_incomplete_defs and has_explicit_annotation
if show_untyped and (self.options.disallow_untyped_defs or check_incomplete_defs):
if fdef.type is None and self.options.disallow_untyped_defs:
self.fail(message_registry.FUNCTION_TYPE_EXPECTED, fdef)
if (not len(fdef.arguments) or
(len(fdef.arg_names) == 1 and 'self' in fdef.arg_names)):

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi May 6, 2019

Collaborator

fdef.arg_names[0] == 'self' would be more straightforward. Also maybe you should check for both self and cls.

if (not len(fdef.arguments) or
(len(fdef.arg_names) == 1 and 'self' in fdef.arg_names)):
self.fail(message_registry.RETURN_TYPE_EXPECTED_DISALLOW_UNTYPED, fdef)
self.note('Use "-> None" if function does not return a value', fdef)

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi May 6, 2019

Collaborator

I would only show this note if the function body doesn't have non-trivial return statements (i.e. return x not just plain return).

This comment has been minimized.

Copy link
@allisonking

allisonking May 6, 2019

Author Contributor

okay sounds good-- could you point me to how to find if there's a return value?

This comment has been minimized.

Copy link
@gvanrossum

gvanrossum May 6, 2019

Member

You probably have to create a new StatementVisitor that traverses fdef.body looking for any ReturnStmt instances whose expr is not None.

(And don't take my word for it. :-)

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi May 6, 2019

Collaborator

You can use stubgen.has_return_statement() (Maybe also move it to some common file like nodes.py?)

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi May 6, 2019

Collaborator

(Fix: srubgen -> stubgen)

This comment has been minimized.

Copy link
@allisonking

allisonking May 7, 2019

Author Contributor

I took a stab at implementing this in nodes.py... let me know if it's reasonable! 😬

@@ -74,6 +74,8 @@
FUNCTION_TYPE_EXPECTED = "Function is missing a type annotation" # type: Final
ONLY_CLASS_APPLICATION = "Type application is only supported for generic classes" # type: Final
RETURN_TYPE_EXPECTED = "Function is missing a return type annotation" # type: Final
RETURN_TYPE_EXPECTED_DISALLOW_UNTYPED = \
"Function must have a return type annotation ('disallow_untyped_defs' enabled)" # type: Final

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi May 6, 2019

Collaborator

Although it is mentioned in the issue, I would not mention the name of option of the flag in the error message. It may easily get out of sync. So I would remove the part in (...).

@@ -984,7 +984,12 @@ def is_unannotated_any(t: Type) -> bool:
check_incomplete_defs = self.options.disallow_incomplete_defs and has_explicit_annotation
if show_untyped and (self.options.disallow_untyped_defs or check_incomplete_defs):
if fdef.type is None and self.options.disallow_untyped_defs:
self.fail(message_registry.FUNCTION_TYPE_EXPECTED, fdef)
if (not len(fdef.arguments) or
(len(fdef.arg_names) == 1 and 'self' in fdef.arg_names)):

This comment has been minimized.

Copy link
@gvanrossum

gvanrossum May 6, 2019

Member

@JukkaL or @ilevkivskyi Is there a more reliable way to tell whether something's a method? Some (unusual) code bases use a different name instead of self, and class methods also aren't covered by this test.

It's also possible to scan the body to see whether occurrences of return have a value or not on them (though I don't know how practical that is from this context).

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi May 6, 2019

Collaborator

I think for self there is actually one: Var.is_self. I am not sure we have the same for cls.

This comment has been minimized.

Copy link
@allisonking

allisonking May 7, 2019

Author Contributor

Thanks! I tried checking for fdef.arguments[0].variable.is_self here but it seems like I always get False. I couldn't find where is_self was being set-- for now I have this checking for the strings self and cls but agree it's not great

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi May 7, 2019

Collaborator

It may be because if a function is missing type annotation, then we don't analyze it at all during semantic analysis. I think it is OK to rely on the argument name, since this is needed only to tweak the error message, and not to decide whether we need to emit the error.

@ilevkivskyi
Copy link
Collaborator

left a comment

Thanks for updates! This is almost ready, I have few more comments.

@@ -984,7 +984,14 @@ def is_unannotated_any(t: Type) -> bool:
check_incomplete_defs = self.options.disallow_incomplete_defs and has_explicit_annotation
if show_untyped and (self.options.disallow_untyped_defs or check_incomplete_defs):
if fdef.type is None and self.options.disallow_untyped_defs:
self.fail(message_registry.FUNCTION_TYPE_EXPECTED, fdef)
if (not fdef.arguments or (len(fdef.arguments) == 1 and
#(fdef.arguments[0].variable.is_self or fdef.arg_names[0] == 'cls'))):

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi May 7, 2019

Collaborator

This comment is not needed.

@@ -1101,6 +1111,19 @@ def __init__(self, expr: Optional[Expression]) -> None:
def accept(self, visitor: StatementVisitor[T]) -> T:
return visitor.visit_return_stmt(self)

class ReturnSeeker(StatementVisitor):

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi May 7, 2019

Collaborator

StatementVisitor[None]

def visit_return_stmt(self, o: ReturnStmt) -> None:
if (o.expr is None or isinstance(o.expr, NameExpr) and o.expr.name == 'None'):
return
self.found = True

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi May 7, 2019

Collaborator

This causes a bunch of code duplication with stubgen.py. Why don't you remove this class (and corresponding helper) from there and also use the added method there?

@@ -1063,7 +1063,8 @@ disallow_any_generics = True
def get_tasks(self):
return 'whatever'
[out]
a.py:1: error: Function is missing a type annotation
a.py:1: error: Function is missing a return type annotation
a.py:1: note: Use "-> None" if function does not return a value

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi May 7, 2019

Collaborator

This should not be here because method returns a string (see CI failures).

@ilevkivskyi
Copy link
Collaborator

left a comment

Thank you! I have just two more comments.

pass

def visit_exec_stmt(self, o: ExecStmt) -> None:
pass

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi May 8, 2019

Collaborator

Why do you need all these dummy methods? Maybe instead just inherit from mypy.traverser.TraverserVisitor?

This comment has been minimized.

Copy link
@allisonking

allisonking May 8, 2019

Author Contributor

I was having trouble importing the mypy.traverser.TraverserVisitor class, I believe because traverser.py also imports from nodes.py?

mypy/test/testcheck.py:9: in <module>
    from mypy import build
mypy/build.py:36: in <module>
    from mypy.nodes import MypyFile, ImportBase, Import, ImportFrom, ImportAll, SymbolTable
mypy/nodes.py:20: in <module>
    from mypy.traverser import TraverserVisitor
mypy/traverser.py:4: in <module>
    from mypy.nodes import (
E   ImportError: cannot import name 'Block' from 'mypy.nodes' (mypy/nodes.py)

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi May 8, 2019

Collaborator

Hm, yes, this would cause an import cycle. What if you instead keep this a function (not a method) and move it (together with the visitor class) to mypy.traverser and then import it from this common place in both cases (in stubgen.py and in checker.py)? I think this would be a reasonable solution.

@@ -97,6 +97,7 @@ class Options:
This class is mutable to simplify testing.
"""

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi May 8, 2019

Collaborator

This empty line and a bunch below look unrelated to the PR

This comment has been minimized.

Copy link
@allisonking

allisonking May 8, 2019

Author Contributor

whoops-- turned off my autoformatter :)

allisonking added some commits May 9, 2019

@ilevkivskyi
Copy link
Collaborator

left a comment

Thank you for all the work! This looks ready now.

@ilevkivskyi ilevkivskyi merged commit 9c74efd into python:master May 9, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.