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

ilinum · 2017-06-09T19:19:59Z

The option disallows all expressions of type Any except:

if a value of type Any used as a second parameter to cast
if a value of type Any is assigned to a variable with an explicit type annotation

The option disallows all expressions of type Any except: * if a value of type Any used as a second parameter to `cast` * if a value of type Any is assigned to a variable with an explicit type annotation

(Was causing failures because of --disallow-untyped-defs)

ddfisher

This looks good overall! I think all my comments are either nits or about naming or testing.

ddfisher · 2017-06-09T22:23:10Z

mypy/checker.py

@@ -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)


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

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!

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.

ddfisher · 2017-06-09T22:24:56Z

mypy/messages.py

@@ -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 '


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

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

Yep, exactly.

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

ddfisher · 2017-06-09T23:26:24Z

test-data/unit/check-flags.test

+print(x[foo(0)])
+[builtins fixtures/list.pyi]
+[out]
+main:8: error: Expressions of type "Any" are disallowed (has type "Any")


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.

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.

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.

Yes, I will split them up! :)

ddfisher · 2017-06-09T23:28:58Z

test-data/unit/check-flags.test

+# flags: --disallow-any=expr
+from typing import List
+
+blargle: List = []


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

oh well, yeah, l is more understandable.

ddfisher · 2017-06-09T23:31:24Z

mypy/checkexpr.py

+    return t.accept(HasAnyType())
+
+
+class HasAnyType(TypeQuery[bool]):


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

ddfisher · 2017-06-09T23:37:47Z

mypy/checkexpr.py

@@ -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,


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

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

ddfisher · 2017-06-09T23:39:39Z

mypy/checkexpr.py

+        if (disallow_any and
+                not self.chk.is_stub and
+                has_any_type(typ) and
+                'expr' in self.chk.options.disallow_any):


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.)

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

ddfisher · 2017-06-09T23:40:47Z

mypy/checkexpr.py

@@ -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)


Why are Any exprs allowed here?

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.

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

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.

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.

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)

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.

ddfisher · 2017-06-09T23:42:49Z

mypy/messages.py

@@ -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


Unused import.

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

ilinum · 2017-06-13T00:07:08Z

Addressed first round of code review feedback!
The following still need discussion:

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

ddfisher · 2017-06-13T19:47:07Z

mypy/checker.py

-                                         rvalue)
+                is_cast = isinstance(rvalue, CallExpr) and rvalue.is_cast()
+                init_type = self.expr_checker.accept(rvalue, always_allow_any=is_cast)
+                self.infer_variable_type(inferred, lvalue, init_type, rvalue)


What's the purpose of this change?

For the following case:

m = cast(Any, Foo().g)

After splitting up the test cases like you suggested, I realized that that case doesn't work.

Actually, I am going to roll back this particular change because of what we discussed here.

ddfisher · 2017-06-13T19:48:19Z

mypy/messages.py

+        else:
+            infix = 'type that contains type'
+        self.fail('Expressions of {} "Any" are disallowed '
+                  '(has type {})'.format(infix, self.format(typ)), context)


I think this may still need some wordsmithing. "Expressions of type that contains type 'Any' are disallowed" is not very easy to parse, IMO.

Also Expressions of type "Any" are disallowed (has type "Any") is redundant, which could be confusing.

How about Expression type contains "Any" (has type {})?
If the actual type is Any, we can say Expressions has type "Any".

That's great!

ddfisher · 2017-06-13T19:50:30Z

test-data/unit/check-flags.test

+    return s  # E: Expressions of type "Any" are disallowed (has type "Any")
+
+g(0)
+g('hello')


What's the purpose of this line? It seems like it's testing the same thing as the previous.

Yep, it does test the same thing. I will remove this line.

ddfisher · 2017-06-13T19:51:28Z

test-data/unit/check-flags.test

+def g(s):
+    return s  # E: Expressions of type "Any" are disallowed (has type "Any")
+
+g(0)


It'd be nice to put a short comment on this line explaining what it's testing and why it's not an error.

After what we discussed here, it will be an error.

ddfisher · 2017-06-13T19:52:26Z

test-data/unit/check-flags.test

+v = g(1)  # E: Expressions of type "Any" are disallowed (has type "Any")
+w: int = g(1)
+
+[case testDisallowAnyExprExplicitAnyParam]


Not sure I fully understand what this test is testing.

It's the same as testDisallowAnyExprUnannotatedFunction except this time function is annotated but type parameter is explicitly Any.

ddfisher · 2017-06-13T19:53:28Z

test-data/unit/check-flags.test

+    g: Any = 2
+
+z = cast(int, Foo().g)
+m = cast(Any, Foo().g)


I don't think this should be allowed. You won't be able to use m without error!

Good point, will change.

ddfisher · 2017-06-13T19:55:15Z

test-data/unit/check-flags.test

+    g: Any = 2
+
+z: int = Foo().g
+m: Any = Foo().g


This probably shouldn't be allowed either. Also, could you add a comment-style annotation to this test?

Good point, will change.

ddfisher · 2017-06-13T19:57:47Z

test-data/unit/check-flags.test

+m: Any = Foo().g
+[builtins fixtures/list.pyi]
+
+[case testDisallowAnyExprAllowsNonExistentMembers]


Is this test really unique? Given the behavior of the other tests, it seems like this is primarily testing that calling a nonexistent function returns Any after the error, which doesn't need to be tested here.

Yes, you are right. I will remove the test :)

ilinum · 2017-06-13T23:03:09Z

I pushed some commits that should address almost all of second round of code review.
What we have left on this PR:

Decide on error message wording.
How to handle unresolved references (Option to disallow all expressions of type Any(--disallow-any=expr) #3519 (comment)).

ilinum · 2017-06-13T23:06:58Z

test-data/unit/check-flags.test

+
+# because expected type is List[Any] the inferred type of [''] is List[Any], which causes
+# more than one error on the line below
+g([''])


As stated in the comment above, expecged type is List[Any], which causes the inferred type of [''] to be List[Any].

Honestly, this behavior seems correct but the user gets a whole bunch of errors that are hard to understand.

@ddfisher what do you think?

I've changed my mind on the functions-with-arguments-of-type-Any issue -- I think it's too hard for users to understand, and isn't necessarily in the sprit of this flag. I think it'd be better to allow those instead. That should help with a lot of confusing errors.

Just a note that g(['']) is still an going to be an error because type of [''] is inferred with expected type of List[Any]. and [''] technically has type List[Any]

# Conflicts: # mypy/main.py

graingert · 2017-06-14T10:26:30Z

docs/source/command_line.rst

+  mypy will output an error unless the expression is immediately
+  used as an argument to ``cast`` or assigned to a variable with an
+  explicit type annotation. Note that declaring a variable of type ``Any``
+  or casting to type ``Any`` is not allowed.


should this be "is allowed"

No, cast(Any, foo) and x: Any = foo are not allowed (we discussed it here and here).

What do you think of the wording?

ddfisher

A few nitpicks and a functionality change. Sorry for the churn.
Remaining things to do (per in person convo):

Remove error message for functions with Any arguments.
Update error messages with the better wording you came up with.
Fix comment nitpicks.

ddfisher · 2017-06-15T23:28:05Z

mypy/checkexpr.py

@@ -185,7 +185,7 @@ def visit_call_expr(self, e: CallExpr, allow_none_return: bool = False) -> Type:
        """Type check a call expression."""
        if e.analyzed:
            # It's really a special form that only looks like a call.
-            return self.accept(e.analyzed, self.type_context[-1])
+            return self.accept(e.analyzed, self.type_context[-1], always_allow_any=True)


What, exactly, is this allowing? At a minimum, I think this needs a comment about why always_allow_any=True.

I'm not totally sure. I removed it and none of the tests failed, so I am going to remove it. I also played around with it a little bit and didn't find any difference.

I did some investigation what it could be and I think it was me trying to allow cast(Any, foo).
But since then we decided to not allow that.

ddfisher · 2017-06-15T23:28:32Z

mypy/checkexpr.py

@@ -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, always_allow_any=True)


Is always_allow_any=True here so we can give a more specific error message in check_arg? If so, could you add a comment about that?

Yes it is.
I think we are not giving any errors for calls to function that accept Any anymore.

Should I still add a comment?

I think it's still worth a quick comment, yeah.

After talking about this in person, we decided that adding info about calling untyped functions to documentation would be enough.

ddfisher · 2017-06-15T23:44:22Z

test-data/unit/check-flags.test

+
+# because expected type is List[Any] the inferred type of [''] is List[Any], which causes
+# more than one error on the line below
+g([''])


I've changed my mind on the functions-with-arguments-of-type-Any issue -- I think it's too hard for users to understand, and isn't necessarily in the sprit of this flag. I think it'd be better to allow those instead. That should help with a lot of confusing errors.

ddfisher

This is almost there! Just a couple more things to fix. Thanks for bearing through this super long review.

ddfisher · 2017-06-16T23:47:33Z

test-data/unit/check-flags.test

+[case testDisallowAnyExprUnannotatedFunction]
+# flags: --disallow-any=expr
+def g(s):
+    return s  # E: Expression has type "Any"


Sorry for not noticing this earlier, but I don't think we should give this warning in unannotated functions. Should be easy to fix, though!

Yes, not reporting errors in unannotated functions makes sense.
Do you think it's worth mentioning it in documentation?

@ddfisher I think Github may have hidden this comment. I think that's something worth including in documentation.

What's your opinion on this?

ddfisher · 2017-06-16T23:48:23Z

test-data/unit/check-flags.test

@@ -515,3 +515,59 @@ 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
+


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.

Done!

Let me know if you have any other test ideas.

ilinum · 2017-06-19T20:59:47Z

Thanks for reviewing so many changed and iterations!
Addressed the most recent round of code review.

ddfisher · 2017-06-20T19:26:49Z

Looks good! Thanks again for bearing through the long review! 🎉

ilinum added 2 commits June 8, 2017 17:28

Implement --disallow-any=expr

93d4c19

The option disallows all expressions of type Any except: * if a value of type Any used as a second parameter to `cast` * if a value of type Any is assigned to a variable with an explicit type annotation

Add return types to functions

6517ae4

(Was causing failures because of --disallow-untyped-defs)

ddfisher suggested changes Jun 9, 2017

View reviewed changes

ilinum added 2 commits June 12, 2017 17:02

Address code review feedback

ee76320

The rest of the feedback (accidentally committed just one file)

f059dfd

Update error message to be more precise when type contains Any

ca06a5a

ddfisher reviewed Jun 13, 2017

View reviewed changes

ilinum added 4 commits June 13, 2017 14:15

Remove redundant tests

edef0b1

Disallow casts to Any (explicit or implicit)

b5c33d7

Disallow calling functions that take parameters of type Any

a07ad12

Add test for implicit casts with type comments

ebc51a8

ilinum commented Jun 13, 2017

View reviewed changes

ilinum added 2 commits June 13, 2017 16:19

Merge branch 'master' into disallow-any-expr

ff16858

# Conflicts: # mypy/main.py

Add documentation

d6d9ecb

graingert reviewed Jun 14, 2017

View reviewed changes

ddfisher reviewed Jun 15, 2017

View reviewed changes

ilinum added 3 commits June 15, 2017 17:35

Update error messages to be more clear

775c5ec

Do not report errors for calls to functions that accept Any

a8731e9

Remove unneeded always_allow_any = True

b0193fb

ddfisher reviewed Jun 16, 2017

View reviewed changes

Update documentation to explain that calling untyped function is allowed

fd166b1

ilinum added 2 commits June 19, 2017 13:31

Do not report errors in unchecked functions

a2dbe35

Add test for basic functionality

52b9206

ddfisher approved these changes Jun 20, 2017

View reviewed changes

ddfisher merged commit f19e6e4 into python:master Jun 20, 2017

ilinum deleted the disallow-any-expr branch June 20, 2017 19:28

ilinum mentioned this pull request Jul 20, 2017

Flag for disallowing Any #3470

Closed

nipunn1313 mentioned this pull request Feb 25, 2020

Disallow Any on public interface types pytest-dev/pytest#6815

Closed

		return t.accept(HasAnyType())


		class HasAnyType(TypeQuery[bool]):

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

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

Conversation

ilinum commented Jun 9, 2017

ddfisher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilinum commented Jun 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilinum Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilinum commented Jun 13, 2017 • edited Loading

ilinum Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddfisher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilinum Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddfisher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilinum Jun 13, 2017 •

edited

Loading

ilinum commented Jun 13, 2017 •

edited

Loading

ilinum Jun 13, 2017 •

edited

Loading

ilinum Jun 19, 2017 •

edited

Loading