Skip to content

Add an optional warning for returning Any from typed functions #2854

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 7 commits into from
Feb 14, 2017

Conversation

lincolnq
Copy link
Contributor

@lincolnq lincolnq commented Feb 12, 2017

I noticed that I often wanted to write "safe" wrappers around untyped library code. There's an example in the mypy source, mypy/git.py:

def git_revision(dir: str) -> bytes:
    """Get the SHA-1 of the HEAD of a git repository."""
    return subprocess.check_output(["git", "rev-parse", "HEAD"], cwd=dir).strip()

Note that subprocess.check_output returns an Any for convenience (its actual type depends on the encoding parameter). If this author had accidentally written .split() instead of .strip(), this error would pass silently but the calling code would fail at runtime, since a caller would receive a List[bytes] when they expected a stripped bytes. I realize that use of Any invites runtime type errors, but it would be nice to at least know when the possibility of runtime errors is "leaking" out of a function definition.

This PR adds an optional warning which lets you know when you're returning Any from a function which is documented to return a non-Any type. The warning flags the above code. The user could then silence the warning by changing their code to:

def git_revision(dir: str) -> bytes:
    """Get the SHA-1 of the HEAD of a git repository."""
    output = subprocess.check_output(["git", "rev-parse", "HEAD"], cwd=dir) # type: bytes
    return output.strip()

For the curious: the output from turning on this warning and running it on the mypy source is reasonable in size, and indeed flags things worth looking-at:

mypy/stubutil.py:90: warning: Returning Any from function declared with return type builtins.bool
mypy/git.py:50: warning: Returning Any from function declared with return type builtins.bytes
mypy/git.py:57: warning: Returning Any from function declared with return type builtins.bytes
mypy/git.py:63: warning: Returning Any from function declared with return type builtins.bool
mypy/git.py:69: warning: Returning Any from function declared with return type builtins.bool
mypy/nodes.py:217: warning: Returning Any from function declared with return type mypy.nodes.SymbolNode
mypy/types.py:60: warning: Returning Any from function declared with return type mypy.types.Type
mypy/fastparse.py:84: warning: Returning Any from function declared with return type mypy.nodes.MypyFile
mypy/fastparse.py:95: warning: Returning Any from function declared with return type mypy.types.Type
mypy/fastparse.py:914: warning: Returning Any from function declared with return type mypy.nodes.Node
mypy/fastparse2.py:101: warning: Returning Any from function declared with return type mypy.nodes.MypyFile
mypy/fastparse2.py:925: warning: Returning Any from function declared with return type mypy.nodes.Expression
mypy/parse.py:1427: warning: Returning Any from function declared with return type mypy.nodes.Expression

The stubutil, nodes and types ones are doing something very dynamic (accessing __dict__.) The git ones are calling subprocess.check_output. I don't understand what fastparse.py is doing yet to comment on those instances.

However, parse.py is extremely interesting! The expr variable's type is declared to be Expression at the beginning of that function. Adding reveal_type(expr) at various places indicates that after the block of if/elif/... where it dispatches to self.parse_* and assigns the result to expr, the type of expr has been magically changed to Any. I am mystified by what mypy is doing here; I tried to go source-spelunking but am a bit too new to the project to figure out what exactly is going on. If I do manage to reproduce it in a smaller test case I'll file a separate issue.

mypy/messages.py Outdated
@@ -27,6 +27,7 @@
MISSING_RETURN_STATEMENT = 'Missing return statement'
INVALID_IMPLICIT_RETURN = 'Implicit return in function which does not return'
INCOMPATIBLE_RETURN_VALUE_TYPE = 'Incompatible return value type'
RETURN_ANY = 'Returning Any from function declared with return type {}'
Copy link
Member

Choose a reason for hiding this comment

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

Normally, types and type names are put in quotes, e.g. like this: '... with return type "{}"'

Copy link
Collaborator

@ddfisher ddfisher left a comment

Choose a reason for hiding this comment

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

Good idea! The implementation looks good, too -- just have a few small nits and then will merge!

Thinking out loud here: this helps catch accidental Anys in one particular circumstance, but it might be nice to have something a bit more comprehensive as well. Perhaps we should consider adding a flag which disallows use of Any-typed values except as the right hand side of an assignment with a type comment (i.e. forcing all Anys to be immediately typed).

mypy/main.py Outdated
@@ -221,6 +221,8 @@ def add_invertible_flag(flag: str,
help="warn about casting an expression to its inferred type")
add_invertible_flag('--no-warn-no-return', dest='warn_no_return', default=True,
help="do not warn about functions that end without returning")
add_invertible_flag('--warn-return-any', default=False, strict_flag=True,
help="warn about returning objects of type Any")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add "from non-Any typed functions".

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, "objects" -> "values".

mypy/messages.py Outdated
@@ -27,6 +27,7 @@
MISSING_RETURN_STATEMENT = 'Missing return statement'
INVALID_IMPLICIT_RETURN = 'Implicit return in function which does not return'
INCOMPATIBLE_RETURN_VALUE_TYPE = 'Incompatible return value type'
RETURN_ANY = 'Returning Any from function declared with return type "{}"'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Tiny nitpick: I think this reads better as "...with declared return type...".

from typing import Any
def g() -> Any: pass
def f(): return g()
[out]
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 also be good to have a test for returning Any from a function declared to return Any.

@lincolnq
Copy link
Contributor Author

Good feedback. Addressed all of the above.

@lincolnq
Copy link
Contributor Author

(And regarding a more broad check around use of Any, I agree that seems potentially very useful. I may look into that at some point if I get some more time to work on stuff!)

@ddfisher ddfisher merged commit 95fd5fb into python:master Feb 14, 2017
@ddfisher
Copy link
Collaborator

Thanks!

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