Option to disallow explicit Any types (--disallow-any=explicit) #3579

ilinum · 2017-06-21T00:17:11Z

Type aliases are a little tricky to deal with because they get "inlined".
The solution was to unmark Anys as explicit right after we check a type alias
for explicit Anys.

(--disallow-any=explicit) Type aliases are a little tricky to deal with because they get "inlined". The solution was to unmark Anys as explicit right after we check a type alias for explicit Anys.

# Conflicts: # mypy/main.py # mypy/semanal.py

ddfisher

Largely looks good! Thanks for the the thorough testing! Have a few nits.

ddfisher · 2017-06-21T21:36:38Z

mypy/messages.py

@@ -872,6 +872,9 @@ def unimported_type_becomes_any(self, prefix: str, typ: Type, ctx: Context) -> N
        self.fail("{} becomes {} due to an unfollowed import".format(prefix, self.format(typ)),
                  ctx)

+    def explicit_any(self, location: str, ctx: Context) -> None:
+        self.fail('Explicit "Any" is not allowed in a {}'.format(location), ctx)


I wonder if this error would be clearer if it was just Explicit "Any" is not allowed.. This reads to me like it's implying that the Any is not allowed in that particular location, but in fact it's not allowed in any location.

Yeah, actually, I agree. I think I was trying to be too fancy when it wasn't necessary.

ddfisher · 2017-06-21T21:45:26Z

mypy/semanal.py

@@ -4310,3 +4334,21 @@ def find_fixed_callable_return(expr: Expression) -> Optional[CallableType]:
            if isinstance(t.ret_type, CallableType):
                return t.ret_type
    return None
+
+
+def unmark_any_as_explicit(t: Type) -> None:


I think this name (and that of the associated class) needs a bit more thought -- I don't think it's clear what it means outside of the context of this PR.

Yeah, naming things is hard - I went through several iterations of names trying to name this function.

How about unset_any_type_as_explicit? Alternatively, we could try to make the function more generic and add a bool parameter explicit. Something like:

def set_any_type_explicit_attribute(explicit: bool) -> None: ...

ddfisher · 2017-06-21T21:48:51Z

mypy/types.py

                 line: int = -1,
                 column: int = -1) -> None:
        super().__init__(line, column)
        # Was this Any type was inferred without a type annotation?
        self.implicit = implicit
        # Does this come from an unfollowed import? See --disallow-any=unimported option
        self.from_unimported_type = from_unimported_type
+        # Does this Any come from an explicit type annotation?
+        self.explicit = explicit


Is this not always the opposite of implicit? If not, the specifics need to be made very clear in the comment.

Yes, explicit is not always the opposite of implicit, which is confusing.

implicit refers to situations when Any was inferred because a variable or parameter was not annotated.
explicit refers to situations when there was an explicit Any annotation.

However, if this Any comes, for instance, from an unimported type, both explicit and implicit will be False.

It seems to me that implicit has a wrong name. It is currently used for --disallow-untyped-defs to see if this Any comes from a parameter or variable with no explicit type annotation.

I think instead of tweaking comments we should rename implicit. How do you feel about calling it from_unannotated?

Added comment on implicit to make it more clear.

ddfisher · 2017-06-21T21:54:36Z

mypy/semanal.py

+    t.accept(UnmarkAnyAsExplicit())
+
+
+class UnmarkAnyAsExplicit(TypeQuery[None]):


This is a bit of an abuse of the TypeQuery class. None of its parent classes seem to be set up for doing this easily, but I'm not entirely sure how I feel about this overall. @JukkaL do you think this is a reasonable thing to do?

Yeah, I agree, this does not seem right.
Moreover, this mutates a type, which is also not great.

What I need is a TypeVisitor that visit all types within a type. Maybe I can refactor one from TypeQuery and then make TypeQuery inherit the new visitor or something?

ddfisher · 2017-06-21T21:58:46Z

test-data/unit/cmdline.test

+[file m.py]
+from typing import Any, List
+v: Any = ''
+w = ['']  # type: List[Any]


Nit: It's better to vary test cases along one axis at once. Here, you're switching from testing a type annotation to a type comment, but you're also switching from testing a plain Any to an Any in a generic class. (And also the list shouldn't need any contents.)

All right, that makes a lot of sense.

Also, I will split these tests into two.

ddfisher · 2017-06-21T22:00:18Z

test-data/unit/cmdline.test

+
+def foo(x: TupleAny[str]) -> None:  # no error
+    pass
+


Could you also check that TupleAny[Any] produces an error?

Wow, this is an interesting edge case. Will add a test.

ddfisher · 2017-06-21T22:02:06Z

test-data/unit/cmdline.test

+[file m.py]
+from typing import Any, List, NewType
+
+# The commented out line is covered without the flag enabled - it is tested elsewhere


I'd instead be a bit more specific and say "making a NewType of Any is never allowed".

Yep, that wording is definitely better.

However, I think it's better to include the test (Baz = NewType('Baz', Any)).
In a different PR Guido pointed out that tests should follow features, not implementation.

And I think in this case, it's better to have an extra tests. Because if this errors stops being reported --disallow-any=explicit will not work - it won't report an explicit Any.

So I am going to include the test here.

ilinum · 2017-06-22T18:20:40Z

Most feedback should be addressed now.

I also realized that the originally this PR did not report errors when Any was used in TypedDict declaration, so I added that.

# Conflicts: # mypy/messages.py # mypy/semanal.py

ilinum · 2017-06-28T20:43:11Z

mypy/semanal.py

+    t.accept(MarkAnyNonExplicit())
+
+
+class MarkAnyNonExplicit(TypeQuery[None]):


@JukkaL need your advice on this.

As David pointed out, this use of TypeQuery seems like abuse of this class.
What do you think is a good way of dealing with this?

We could refactor a child of TypeQuery to have that. Or maybe there's another way.

What about having a TypeQuery that returns all Any types that are part of a type, and then iterate over the results of the query and set explicit? I.e. something like this:

for any_type in find_all_any_types(t): any_type.explicit = False

Hmm actually that might not be a good idea, since mutating types is only safe if you are certain that there is ever only a single reference to the type object. What about a type transform that replaces Any instances with a copy that has explicit set to False? You should be able to use TypeTranslator.

Yes, that's a really good idea!

Done!

ilinum · 2017-06-28T20:44:25Z

All feedback should be addressed now, except for using TypeQuery to set AnyType as non-explicit.

JukkaL · 2017-06-29T16:38:20Z

mypy/checker.py

@@ -627,6 +627,11 @@ def is_implicit_any(t: Type) -> bool:
                                if has_any_from_unimported_type(arg_type):
                                    prefix = "Argument {} to \"{}\"".format(idx + 1, fdef.name())
                                    self.msg.unimported_type_becomes_any(prefix, arg_type, fdef)
+                    if ('explicit' in self.options.disallow_any and


This same piece of code gets repeated almost identically many times. Can you refactor this into a function or a few functions?

to check for explicit Any

# Conflicts: # mypy/semanal.py

ilinum · 2017-06-29T19:32:43Z

All feedback is addressed now, this PR should be ready to be merged.

ddfisher

LGTM!

Implement option to disallow explicit Any types

fa8e7a3

(--disallow-any=explicit) Type aliases are a little tricky to deal with because they get "inlined". The solution was to unmark Anys as explicit right after we check a type alias for explicit Anys.

ilinum changed the title ~~Implement option to disallow explicit Any types~~ Option to disallow explicit Any types (--disallow-any=explicit) Jun 21, 2017

Merge branch 'master' into disallow-any=explicit

95a75f5

# Conflicts: # mypy/main.py # mypy/semanal.py

ddfisher suggested changes Jun 21, 2017

View reviewed changes

ilinum added 5 commits June 21, 2017 15:30

Report errors for explicit Any in TypedDict declaration

e3cd6eb

Make error message simpler (just say that there's Any on this line)

1bf6c61

Simplify test by splitting it into two

398b2d8

Add test to check for generic alias with generic type "Any"

4b9643f

Uncomment test for NewType with explicit Any

8bcaf99

Improve wording in a comment

3d87808

ilinum force-pushed the disallow-any=explicit branch from 4332307 to 3d87808 Compare June 22, 2017 20:00

ilinum added 5 commits June 26, 2017 17:25

Merge branch 'master' into disallow-any=explicit

ddc12e3

# Conflicts: # mypy/messages.py # mypy/semanal.py

Rename function to mark_any_non_explicit

4394530

Update comment about explicit and implicit attributes on AnyType

022684c

Add comment to the test about explicit Any in NewType

f4c87fe

Merge branch 'master' into disallow-any=explicit

9380796

ilinum commented Jun 28, 2017

View reviewed changes

JukkaL reviewed Jun 29, 2017

View reviewed changes

ilinum added 3 commits June 29, 2017 11:52

Use TypeTranslator to make Any non-explicit + extract method

5ffbd1e

to check for explicit Any

remove unneeded assert

24c88c1

Merge branch 'master' into disallow-any=explicit

4384491

# Conflicts: # mypy/semanal.py

Add documentation

8f90d49

ddfisher approved these changes Jul 1, 2017

View reviewed changes

ilinum merged commit b7636cb into python:master Jul 1, 2017

ilinum deleted the disallow-any=explicit branch July 1, 2017 04:03

ilinum mentioned this pull request Jul 20, 2017

Flag for disallowing Any #3470

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Option to disallow explicit Any types (--disallow-any=explicit) #3579

Option to disallow explicit Any types (--disallow-any=explicit) #3579

ilinum commented Jun 21, 2017 •

edited

Loading

ddfisher left a comment

ddfisher Jun 21, 2017

ilinum Jun 21, 2017

ddfisher Jun 21, 2017

ilinum Jun 21, 2017

ddfisher Jun 21, 2017

ilinum Jun 21, 2017

ilinum Jun 28, 2017

ddfisher Jun 21, 2017

ilinum Jun 21, 2017

ddfisher Jun 21, 2017

ilinum Jun 22, 2017

ddfisher Jun 21, 2017

ilinum Jun 22, 2017

ddfisher Jun 21, 2017

ilinum Jun 22, 2017

ilinum commented Jun 22, 2017

ilinum Jun 28, 2017 •

edited

Loading

JukkaL Jun 29, 2017

JukkaL Jun 29, 2017

ilinum Jun 29, 2017

ilinum commented Jun 28, 2017

JukkaL Jun 29, 2017

ilinum Jun 29, 2017

ilinum commented Jun 29, 2017

ddfisher left a comment

		t.accept(UnmarkAnyAsExplicit())


		class UnmarkAnyAsExplicit(TypeQuery[None]):

		t.accept(MarkAnyNonExplicit())


		class MarkAnyNonExplicit(TypeQuery[None]):

Option to disallow explicit Any types (--disallow-any=explicit) #3579

Option to disallow explicit Any types (--disallow-any=explicit) #3579

Conversation

ilinum commented Jun 21, 2017 • edited Loading

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

ilinum commented Jun 22, 2017

ilinum Jun 28, 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

ilinum commented Jun 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilinum commented Jun 29, 2017

ddfisher left a comment

Choose a reason for hiding this comment

ilinum commented Jun 21, 2017 •

edited

Loading

ilinum Jun 28, 2017 •

edited

Loading