Skip to content

Option to disallow all expressions of type Any(--disallow-any=expr) #3519

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.

Sign up for GitHub

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
ddfisher merged 17 commits into from
Jun 20, 2017
Merged

Option to disallow all expressions of type Any(--disallow-any=expr) #3519

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
93d4c19
Implement --disallow-any=expr
ilinum Jun 8, 2017
6517ae4
Add return types to functions
ilinum Jun 9, 2017
ee76320
Address code review feedback
ilinum Jun 13, 2017
f059dfd
The rest of the feedback (accidentally committed just one file)
ilinum Jun 13, 2017
ca06a5a
Update error message to be more precise when type contains `Any`
ilinum Jun 13, 2017
edef0b1
Remove redundant tests
ilinum Jun 13, 2017
b5c33d7
Disallow casts to `Any` (explicit or implicit)
ilinum Jun 13, 2017
a07ad12
Disallow calling functions that take parameters of type `Any`
ilinum Jun 13, 2017
ebc51a8
Add test for implicit casts with type comments
ilinum Jun 13, 2017
ff16858
Merge branch 'master' into disallow-any-expr
ilinum Jun 13, 2017
d6d9ecb
Add documentation
ilinum Jun 13, 2017
775c5ec
Update error messages to be more clear
ilinum Jun 16, 2017
a8731e9
Do not report errors for calls to functions that accept Any
ilinum Jun 16, 2017
b0193fb
Remove unneeded always_allow_any = True
ilinum Jun 16, 2017
fd166b1
Update documentation to explain that calling untyped function is allowed
ilinum Jun 19, 2017
a2dbe35
Do not report errors in unchecked functions
ilinum Jun 19, 2017
52b9206
Add test for basic functionality
ilinum Jun 19, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion mypy/checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1736,7 +1736,7 @@ def check_simple_assignment(self, lvalue_type: Type, rvalue: Expression,
# '...' is always a valid initializer in a stub.
return AnyType()
else:
rvalue_type = self.expr_checker.accept(rvalue, lvalue_type)
rvalue_type = self.expr_checker.accept(rvalue, lvalue_type, disallow_any=False)
Copy link
Copy Markdown
Collaborator

@ddfisher ddfisher Jun 9, 2017

Choose a reason for hiding this comment

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

Does this make sure there's an explicit type annotation? Also, I'm not convinced we want to allow this even so.

Copy link
Copy Markdown
Collaborator Author

@ilinum ilinum Jun 12, 2017

Choose a reason for hiding this comment

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

Yeah, that's a good point. I will change that to disallow_any=lvalue_type is None. It seems like lvalue_type is the type from an explicit type annotation.

As for not allowing Any expressions when assigning to a variable with an explicit type annotation, here are my thoughts.

People are going to be calling untyped functions from their code. --disallow-any=expr needs to allow for this, otherwise no one will use it.
In my mind, there are two good ways to do it: explicit cast (call to function cast) and implicit cast (assignment to a variable with an explicit type annotation).
The advantage of an explicit cast is that you can't do it accidentally.
However, the explicit cast syntax makes the code less readable.
Compare with an implicit cast:

# explicit cast
result = cast(float, some_long_method_name(that, (has, many), complicated=params))
# implicit cast
result = some_long_method_name(that, (has, many), complicated=params)  # type: float

A big disadvantage of an implicit cast is that someone can use it accidentally. They won't look at the definition of the function they are calling and just put a type there thinking that mypy will complain if the type is wrong.

This PR has both implicit and explicit cast implemented because I think it's worth having both.
But I would be interested in hearing your reasoning for this!

Copy link
Copy Markdown
Collaborator

@ddfisher ddfisher Jun 13, 2017

Choose a reason for hiding this comment

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

I see the same pros and cons as you -- I just weigh them differently. Mainly, I think it's important for it to be really clear where the Anys are.

That said, IIRC it's not particularly common to annotate the result of a function call, so maybe the type annotation serves that purpose well enough. Let's go with that for now.

if isinstance(rvalue_type, DeletedType):
self.msg.deleted_as_rvalue(rvalue_type, context)
if isinstance(lvalue_type, DeletedType):
Expand Down
30 changes: 26 additions & 4 deletions mypy/checkexpr.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
PartialType, DeletedType, UnboundType, UninhabitedType, TypeType,
true_only, false_only, is_named_instance, function_type, callable_type, FunctionLike,
get_typ_args, set_typ_args,
StarType)
StarType, TypeQuery)
from mypy.nodes import (
NameExpr, RefExpr, Var, FuncDef, OverloadedFuncDef, TypeInfo, CallExpr,
MemberExpr, IntExpr, StrExpr, BytesExpr, UnicodeExpr, FloatExpr,
Expand Down Expand Up @@ -204,7 +204,7 @@ def visit_call_expr(self, e: CallExpr, allow_none_return: bool = False) -> Type:
or isinstance(typ, NameExpr) and node and node.kind == nodes.TYPE_ALIAS):
self.msg.type_arguments_not_allowed(e)
self.try_infer_partial_type(e)
callee_type = self.accept(e.callee)
callee_type = self.accept(e.callee, disallow_any=False)
Copy link
Copy Markdown
Collaborator

@ddfisher ddfisher Jun 9, 2017

Choose a reason for hiding this comment

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

Why are Any exprs allowed here?

Copy link
Copy Markdown
Collaborator Author

@ilinum ilinum Jun 12, 2017

Choose a reason for hiding this comment

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

This allows us to call function that are one of:

  • unresolved
  • unannotated or take Any as a parameter

Should we disallow this? My general thinking is that we want to make --disallow-any=expr easy to enable per-module. That way you can turn the option on for current module even if other modules are not well annotated.

As for functions that take explicitly annotated Any parameters, I think in a world with perfect stubs, we would disallow calling those. However, that's not the case (at least now) and, for instance, print function accept arguments of type Any. This would mean that you can't call print with --disallow-any=expr enabled.

Copy link
Copy Markdown
Member

@gvanrossum gvanrossum Jun 12, 2017

Choose a reason for hiding this comment

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

However nothing would be lost if print's args were converted to object.

Copy link
Copy Markdown
Collaborator Author

@ilinum ilinum Jun 12, 2017

Choose a reason for hiding this comment

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

Did you mean updated in typeshed?

Or silently treat them as object if this flag is enabled? I think the second is a good idea but would be really hard to implement, from a technical viewpoint.

Copy link
Copy Markdown
Collaborator

@ddfisher ddfisher Jun 13, 2017

Choose a reason for hiding this comment

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

I think he means updated in typeshed. Type checking-wise, from the caller's perspective, there's no difference between an an argument of type Any and and argument of type object.

Anyway, I don't think this should be allowed. The primary purpose of this flag is not to be something that's easy to enable -- it's to provide clear, strong guarantees about how thoroughly a module is type checked. In my mind, that sets a high bar for any exceptions we'd consider making.

Copy link
Copy Markdown
Collaborator Author

@ilinum ilinum Jun 13, 2017

Choose a reason for hiding this comment

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

That means we should probably go through all stubs in typeshed and parameters of type Any to object. I can look into it. Just to confirm, it shouldn't break anything, right?

I see your point.
I still think we should not give an error if a user is calling a function that takes Any. Often, they can't modify the stubs of the function they are calling.
That being said, I'm okay with disallowing this and then seeing if it's a problem for the users.

How should we handle unresolved functions? (I imagine this would be the case when --disallow-any=expr is used alongside --ignore-missing-imports)

Copy link
Copy Markdown
Collaborator

@ddfisher ddfisher Jun 15, 2017

Choose a reason for hiding this comment

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

It wouldn't break anything, but that's not a change we want to make: a function taking Any and a function taking object implies pretty different things about what that function supports.

I think it's reasonable for now to disallow functions that have Any as an argument. We may need to relax this requirement later, but it's easier to start strict and then remove errors if they're too much.

Unresolved functions should definitely be errors in this mode. They're unambiguously expressions with type Any.

if (self.chk.options.disallow_untyped_calls and
self.chk.in_checked_function() and
isinstance(callee_type, CallableType)
Expand Down Expand Up @@ -1670,7 +1670,8 @@ def visit_enum_index_expr(self, enum_type: TypeInfo, index: Expression,

def visit_cast_expr(self, expr: CastExpr) -> Type:
"""Type check a cast expression."""
source_type = self.accept(expr.expr, type_context=AnyType(), allow_none_return=True)
source_type = self.accept(expr.expr, type_context=AnyType(), allow_none_return=True,
disallow_any=False)
target_type = expr.type
options = self.chk.options
if options.warn_redundant_casts and is_same_type(source_type, target_type):
Expand Down Expand Up @@ -2191,7 +2192,8 @@ def visit_backquote_expr(self, e: BackquoteExpr) -> Type:
def accept(self,
node: Expression,
type_context: Type = None,
allow_none_return: bool = False
allow_none_return: bool = False,
disallow_any: bool = True,
Copy link
Copy Markdown
Collaborator

@ddfisher ddfisher Jun 9, 2017

Choose a reason for hiding this comment

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

I'd consider naming this always_allow_any (or similar) instead. It's a bit misleading as-is.

Copy link
Copy Markdown
Collaborator Author

@ilinum ilinum Jun 12, 2017

Choose a reason for hiding this comment

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

I was looking for a better name. always_allow_any is good, I think.

) -> Type:
"""Type check a node in the given type context. If allow_none_return
is True and this expression is a call, allow it to return None. This
Expand All @@ -2211,6 +2213,13 @@ def accept(self,
self.type_context.pop()
assert typ is not None
self.chk.store_type(node, typ)

if (disallow_any and
not self.chk.is_stub and
has_any_type(typ) and
'expr' in self.chk.options.disallow_any):
Copy link
Copy Markdown
Collaborator

@ddfisher ddfisher Jun 9, 2017

Choose a reason for hiding this comment

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

Nit: The last conditional should be first here. It's standard (in this codebase at least) for conditionals involving flags for the flag to be checked first. (And I think that makes the most conceptual sense, though I'm not sure I can well explain why.)

Copy link
Copy Markdown
Collaborator Author

@ilinum ilinum Jun 12, 2017

Choose a reason for hiding this comment

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

Yeah, I think it's easier when you're looking through the code.
Also, it might be a little faster.

self.msg.disallowed_any_type(typ, node)

if not self.chk.in_checked_function():
return AnyType()
else:
Expand Down Expand Up @@ -2429,6 +2438,19 @@ def narrow_type_from_binder(self, expr: Expression, known_type: Type) -> Type:
return known_type


def has_any_type(t: Type) -> bool:
"""Whether t contains an Any type"""
return t.accept(HasAnyType())


class HasAnyType(TypeQuery[bool]):
Copy link
Copy Markdown
Collaborator

@ddfisher ddfisher Jun 9, 2017

Choose a reason for hiding this comment

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

Elsewhere in the file TypeQuery is referred to as the partially qualified types.TypeQuery. I'd do that here for consistency.

Copy link
Copy Markdown
Collaborator Author

@ilinum ilinum Jun 12, 2017

Choose a reason for hiding this comment

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

Yep!

def __init__(self) -> None:
super().__init__(any)

def visit_any(self, t: AnyType) -> bool:
return True


def has_coroutine_decorator(t: Type) -> bool:
"""Whether t came from a function decorated with `@coroutine`."""
return isinstance(t, Instance) and t.type.fullname() == 'typing.AwaitableGenerator'
Expand Down
3 changes: 1 addition & 2 deletions mypy/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ def type_check_only(sources: List[BuildSource], bin_dir: str, options: Options)
options=options)


disallow_any_options = ['unimported']
disallow_any_options = ['unimported', 'expr']


def disallow_any_argument_type(raw_options: str) -> List[str]:
Expand Down Expand Up @@ -201,7 +201,6 @@ def process_options(args: List[str],

strict_flag_names = [] # type: List[str]
strict_flag_assignments = [] # type: List[Tuple[str, bool]]
disallow_any_options = ['unimported']

def add_invertible_flag(flag: str,
*,
Expand Down
6 changes: 5 additions & 1 deletion mypy/messages.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import re
import difflib

from typing import cast, List, Dict, Any, Sequence, Iterable, Tuple
from typing import cast, List, Dict, Any, Sequence, Iterable, Tuple, Optional
Copy link
Copy Markdown
Collaborator

@ddfisher ddfisher Jun 9, 2017

Choose a reason for hiding this comment

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

Unused import.

Copy link
Copy Markdown
Collaborator Author

@ilinum ilinum Jun 12, 2017

Choose a reason for hiding this comment

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

Good catch!
I changed my PyCharm settings to complain if there's an unused import when I'm committing.


from mypy.erasetype import erase_type
from mypy.errors import Errors
Expand Down Expand Up @@ -897,6 +897,10 @@ def typeddict_item_name_not_found(self,
def type_arguments_not_allowed(self, context: Context) -> None:
self.fail('Parameterized generics cannot be used with class or instance checks', context)

def disallowed_any_type(self, typ: Type, context: Context) -> None:
self.fail('Expressions of type "Any" are disallowed '
Copy link
Copy Markdown
Collaborator

@ddfisher ddfisher Jun 9, 2017

Choose a reason for hiding this comment

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

This is a bit confusing, as the expression may not have the type "Any".

Copy link
Copy Markdown
Collaborator Author

@ilinum ilinum Jun 12, 2017

Choose a reason for hiding this comment

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

I'm not sure what you mean.
Do you mean a situation when a type contains Any (e.g. List[Any])?

Copy link
Copy Markdown
Collaborator

@ddfisher ddfisher Jun 13, 2017

Choose a reason for hiding this comment

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

Yep, exactly.

Copy link
Copy Markdown
Collaborator Author

@ilinum ilinum Jun 13, 2017

Choose a reason for hiding this comment

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

Updated the error message to say Expressions of type that contains type "Any" are disallowed (has type {}) if type is not Any.

'(has type {})'.format(self.format(typ)), context)


def capitalize(s: str) -> str:
"""Capitalize the first character of a string."""
Expand Down
47 changes: 47 additions & 0 deletions test-data/unit/check-flags.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,3 +509,50 @@ x, y = 1, 2 # type: Unchecked, Unchecked
[out]
main:4: error: Type of variable becomes "Any" due to an unfollowed import
main:6: error: A type on this line becomes "Any" due to an unfollowed import

Copy link
Copy Markdown
Collaborator

@ddfisher ddfisher Jun 16, 2017

Choose a reason for hiding this comment

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

It'd be nice to have some tests for the basic functionality. These tests do a good job of exercising the more interesting edge cases, but it's worth testing putting an Any type in a variety of expressions to make sure it's caught/the behavior is what we expect.

Copy link
Copy Markdown
Collaborator Author

@ilinum ilinum Jun 19, 2017

Choose a reason for hiding this comment

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

Done!

Let me know if you have any other test ideas.

[case testDisallowAnyBasic]
# flags: --disallow-any=expr
from typing import List, Any, cast

def foo(z: int):
return z

def print(s: Any) -> None:
s = s + 1
pass

class Foo:
g: Any = 2

x: List[str] = ['hello']

f: int = Foo.doo()
k = Foo.doo()

z = cast(int, Foo().g)
m = cast(Any, Foo().g)
y = Foo().g
print(x[0])
print(x[foo(0)])
[builtins fixtures/list.pyi]
[out]
main:8: error: Expressions of type "Any" are disallowed (has type "Any")
Copy link
Copy Markdown
Collaborator

@ddfisher ddfisher Jun 9, 2017

Choose a reason for hiding this comment

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

It'd be better to use the inline error syntax here (which you may not have seen). Basically you can put # E: ERROR MESSAGE at the end of a line in a test, and it will be understood as indicating the appropriate error output on that line.

Copy link
Copy Markdown
Collaborator

@ddfisher ddfisher Jun 9, 2017

Choose a reason for hiding this comment

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

You'll have to break this up into multiple tests so you can do this (because there are multiple errors on some lines), but it'd be worthwhile.

Copy link
Copy Markdown
Collaborator

@ddfisher ddfisher Jun 9, 2017

Choose a reason for hiding this comment

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

Thinking some more about this: I'd like to see this split up into multiple tests anyway, each testing a specific aspect (both positive and negative) of this PR (and named appropriately). E.g. testDisallowAnyAllowsAnyInCast or similar.

Copy link
Copy Markdown
Collaborator Author

@ilinum ilinum Jun 12, 2017

Choose a reason for hiding this comment

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

Yes, I will split them up! :)

main:16: error: Type[Foo] has no attribute "doo"
main:17: error: Type[Foo] has no attribute "doo"
main:17: error: Expressions of type "Any" are disallowed (has type "Any")
main:20: error: Expressions of type "Any" are disallowed (has type "Any")
main:21: error: Expressions of type "Any" are disallowed (has type "Any")
main:23: error: Expressions of type "Any" are disallowed (has type "Any")

[case testDisallowAnyExprGeneric]
# flags: --disallow-any=expr
from typing import List

blargle: List = []
Copy link
Copy Markdown
Collaborator

@ddfisher ddfisher Jun 9, 2017

Choose a reason for hiding this comment

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

Nit: as much as I like blargle, l is probably better here.

Copy link
Copy Markdown
Collaborator Author

@ilinum ilinum Jun 12, 2017

Choose a reason for hiding this comment

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

oh well, yeah, l is more understandable.

blargle.append(1)
k = blargle[0]
[builtins fixtures/list.pyi]
[out]
main:5: error: Expressions of type "Any" are disallowed (has type List[Any])
main:6: error: Expressions of type "Any" are disallowed (has type List[Any])
main:6: error: Expressions of type "Any" are disallowed (has type "Any")