Disallow implicit Any types from Silent Imports by ilinum · Pull Request #3405 · python/mypy

ilinum · 2017-05-22T22:23:20Z

These types can appear from an unanalyzed module.
If mypy encounters a type annotation that uses such a type,
it will report an error.

Note that this PR is different from #3141 although the flags have similar names. Perhaps, we should figure out the proper names for all flags.

These types can appear from an unanalyzed module. If mypy encounters a type annotation that uses such a type, it will report an error. Fixes python#3205

JukkaL

Did a quick review pass. Looks pretty good! We may want to spend a little more time thinking about the names of various Any-related options, since it looks like we might have a few options that are pretty closely related and it's unclear how to distinguish the different options from each other in a way that is intuitive to users.

JukkaL · 2017-05-23T16:58:33Z

test-data/unit/check-flags.test

+from missing import MyType
+
+def f(x: MyType) -> None:  # E: Argument 1 to 'f' is implicitly converted to 'Any' due to import from unanalyzed module
+  pass


Use a 4-space indent here and elsewhere in tests (you can leave existing tests that use a 2-space indent unmodified).

JukkaL · 2017-05-23T17:01:59Z

test-data/unit/check-flags.test

+  return l
+[builtins fixtures/list.pyi]
+[out]
+main:5: error: Return type is implicitly converted to 'builtins.list[Any]' due to import from unanalyzed module


Would it make sense to update the types to be formatted as List[Any] etc. for consistency with most other messages? builtins.list[Any] and such are pretty verbose and not how users write the types.

JukkaL · 2017-05-23T17:03:07Z

test-data/unit/check-flags.test

+class C(Unchecked): # E: Subclassing type 'Unchecked' that is implicitly converted to 'Any' due to import from unanalyzed module
+  pass
+
+class A(List[Unchecked]): # E: Subclassing a type that is implicitly converted to 'builtins.list[Any]' due to import from unanalyzed module


Similar to above (List[Any] preferred over builtins.list[Any]).

JukkaL · 2017-05-23T17:07:21Z

test-data/unit/check-flags.test

+  pass
+
+class A(List[Unchecked]): # E: Subclassing a type that is implicitly converted to 'builtins.list[Any]' due to import from unanalyzed module
+  pass


Ideas for additional test cases:

Type alias like X = List[Unchecked] or X = Unchecked.

cast(List[Unchecked], foo)

NamedTuple uses an implicit Any type.

T = TypeVar('T', Unchecked, str)

T = TypeVar('T', bound=Unchecked)

X = NewType('X', Unchecked)

Implicitly converted type within a callable type or a tuple type.

ilinum · 2017-05-23T23:58:29Z

The code review feedback should be addressed now.
But we need to decide on a better name for this flag before this PR is ready to be merged.

ilevkivskyi · 2017-05-24T13:43:11Z

FWIW, I think the name of the flag you propose is too generic, e.g. List is annotation is also implicitly converted to List[Any] per PEP 484, but this is not prohibited be this flag. Maybe a better name would be --disallow-imported-any?

ilinum · 2017-05-24T15:06:12Z

@ilevkivskyi Yes, I defintely agree with you. In #3427 I mentioned --disallow-any-from-unanalyzed-module but that's a little more verbose than your option.

ddfisher

I haven't had a chance to look closely at the tests yet, but here's a review of everything else.

Overall, nicely done! Most of my comments are relatively small tweaks. A couple things to note that didn't fit well elsewhere:

This diff includes a typeshed change -- be careful about that! It's super easy to do so accidentally. I found the typeshed subrepo to be a pain to manage until I added the following as a post-checkout and post-merge git hook:

#!/bin/sh
git submodule update

After that, it's basically never been in my way. Highly recommended.

In some comments and error messages, this code talks about types that are "implicitly converted to Any", which isn't quite right. The types aren't being converted, per se, it's more that we don't have any information about them, so they've fallen back to Any. The exact messaging around this (like the name of the flag) probably bears some more thought.

Finally, as a stylistic point, I'd recommend responding individually to all review comments so reviewers can know exactly which things you've addressed in your changes so far, which things you plan to address in future changes, and which things you disagree with. I often write these responses as I'm making the changes, which helps me keep track of things and make sure I'm not missing anything (and sometimes actually making the change provides important context for my response). On the review side, it's always super helpful to know which comments were actually addressed and not, as a "respond to review" commit can mean many things.

ddfisher · 2017-05-24T22:01:52Z

mypy/options.py

    def select_options_affecting_cache(self) -> Mapping[str, bool]:
        return {opt: getattr(self, opt) for opt in self.OPTIONS_AFFECTING_CACHE}
+
+    def silent_mode(self) -> bool:


I'd consider silent_imports_mode instead.

Though actually I think one of the call sites will probably be removed, making this no longer necessary.

ddfisher · 2017-05-24T22:09:49Z

mypy/checker.py

+            elif (self.options.disallow_implicit_any_types
+                  and has_any_from_silent_import(lvalue_type)):
+                prefix = "Type of {}".format(lvalue_name)
+                self.msg.implicit_any_from_silent_import(prefix, lvalue_type, context)


This gives an error on every line where a silent-import-Any-typed variable is assigned to, which is undesirable. We'd prefer to only give an error on the first assignment.

ddfisher · 2017-05-24T22:10:57Z

mypy/checkexpr.py

+        if options.warn_redundant_casts and is_same_type(source_type, target_type):
            self.msg.redundant_cast(target_type, expr)
+        if options.disallow_implicit_any_types and has_any_from_silent_import(target_type):
+            self.msg.implicit_any_from_silent_import("Target type of cast", target_type, expr)


This won't work properly if the variable is casted to a Callable.

I'm not totally sure what you mean. I tried the following code and it gave an error (like it's supposed to).

from typing import cast, Callable from missing import Unchecked foo = [1, 2, 3] call = cast(Callable[[], Unchecked], foo)

Produces the following output:

m.py:6: error: Target type of cast is implicitly converted to Callable[[], Any] due to import from unanalyzed module

Oh, I see what you're saying. Maybe it should saysomething along the lines of Return type is converted Any from Unchecked (not exact wording). Is that what you mean?

I am actually not sure about this. In another case we are typechecking a function definition (where the user types in def foo ...), so there is an error specific to return type and arguments. However, when the user types in Callable, I think talking about Callable as its own type makes sense. Do you have a strong opinion on this?

Ah, that was my misunderstanding. I saw you dissecting Callable types here and assumed that was because has_any_from_silent_import didn't properly handle them. Taking a closer look, you're just trying to give people a better error message. That seems reasonable!

ddfisher · 2017-05-24T22:18:36Z

mypy/semanal.py

-                if self.options.disallow_subclassing_any:
+                # if --disallow-implicit-any-types is set, the issue is reported later
+                implicit = self.options.disallow_implicit_any_types and base.is_from_silent_import
+                if self.options.disallow_subclassing_any and not implicit:


I actually think that emitting both errors here is the correct thing to do.

Yeah, I think it's okay to have both. I will remove the extra check.

As long as it's not overwhelming for the user, it's actually better to have more information than less. The new message would explain more. This is what it outputs now for a simple case:

error: Cannot find module named 'missing' note: (Perhaps setting MYPYPATH or using the "--ignore-missing-imports" flag would help) error: Class cannot subclass 'Unchecked' (has type 'Any') error: Base type Unchecked is implicitly converted to "Any" due to import from unanalyzed module

ddfisher · 2017-05-24T22:20:42Z

mypy/semanal.py

            elif isinstance(base, AnyType):
-                if self.options.disallow_subclassing_any:
+                # if --disallow-implicit-any-types is set, the issue is reported later
+                implicit = self.options.disallow_implicit_any_types and base.is_from_silent_import


implicit isn't a great name for this variable -- that sounds like something that should be only a property of base, but in fact it also depends on an error-related option. I'd consider something like more explicit like disallowed_implicit_any instead.

Yeah, I'll remove this variable as per comment below.

ddfisher · 2017-05-24T22:24:13Z

mypy/semanal.py

+        any_type = AnyType()
+        if self.options.silent_mode():
+            # if silent mode is not enabled, no need to report implicit conversion to Any,
+            # let mypy report an import error.


I think we should still report an errors in this instance, actually -- there's no point in making the flag only work with a very specific combination of other flags when a slightly more general use is perfectly valid. Even if the exact use-case is less clear, having consistent behavior is worthwhile.

Yeah, this makes sense. It's not obvious that the new flag should be used with the silent imports.

ddfisher · 2017-05-25T10:07:50Z

mypy/typeanal.py

                    # is pretty minor.
-                    return AnyType()
+                    any_type = AnyType()
+                    any_type.is_from_silent_import = sym.node.type.is_from_silent_import


This suggests is_from_silent_import to me that should be an optional argument to AnyType's constructor.

Yes, I was thinking about it. Would probably make it much cleaner.

What stopped me was that there is already an parameter implicit in the constructor from this commit.
It should be fine if I comment it though.

ddfisher · 2017-05-25T10:08:58Z

mypy/typeanal.py

            return []


+def has_any_from_silent_import(t: Type) -> bool:


Rather than special-casing Callable at some of the call sites, it'd be better to handle it here.

Responded to this in this discussion: #3405 (comment)

ilinum · 2017-05-25T20:25:31Z

Thanks for the feedback, David!

Yeah, I realized I commited typeshed after I submitted the PR. It should be good now! Thanks for the advice.

I responded to all your comments - and will follow it in the future. Thanks!

For now I addressed all of the feedback, except for the Callable type.
And we still need to discuss good name for the flag and how to refer to it in comments/variable names.

Also, handle multiple declaration

ddfisher

This is looking good. I had a few minor nitpicks, but after we figure out naming/messaging it should be good to go!

ddfisher · 2017-05-26T23:11:52Z

mypy/checker.py

+        if (s.type is not None and
+                self.options.disallow_implicit_any_types and
+                has_any_from_silent_import(s.type)):
+            if isinstance(s.lvalues[-1], TupleExpr):  # is multiple


Nitpick: this comment should be a bit more explicit. In general, I'd err on the side of trying to be very very clear in comments, even if that feels a little verbose. In this case, I'd consider moving the comment to the next line and writing something like:

# This is a multiple assignment. Instead of figuring out which type is problematic, give a generic error message.

Yep, that makes sense! Agree.

ddfisher · 2017-05-26T23:15:51Z

mypy/checkexpr.py

+        if options.warn_redundant_casts and is_same_type(source_type, target_type):
            self.msg.redundant_cast(target_type, expr)
+        if options.disallow_implicit_any_types and has_any_from_silent_import(target_type):
+            self.msg.implicit_any_from_silent_import("Target type of cast", target_type, expr)


Ah, that was my misunderstanding. I saw you dissecting Callable types here and assumed that was because has_any_from_silent_import didn't properly handle them. Taking a closer look, you're just trying to give people a better error message. That seems reasonable!

ddfisher · 2017-05-26T23:18:42Z

mypy/types.py


-    def __init__(self, implicit: bool = False, line: int = -1, column: int = -1) -> None:
+    def __init__(self, implicit: bool = False, from_silent_import: bool = False,
+                 line: int = -1, column: int = -1) -> None:


Nitpick: if you have to wrap a function def line, put each argument on a line of its own. There's a lot of existing code in mypy which doesn't do this, but it's much easier to read if done this way, so we're trying to do this for new code.

Seems reasonable!

ddfisher · 2017-05-26T23:19:32Z

mypy/types.py

+                 line: int = -1, column: int = -1) -> None:
        super().__init__(line, column)
-        self.implicit = implicit
+        self.implicit = implicit  # Was this Any type was inferred without a type annotation?


Nitpick: probably better to just put this comment on the preceding line for consistency with the next member variable initalization.

ddfisher · 2017-05-26T23:20:01Z

mypy/types.py

        super().__init__(line, column)
-        self.implicit = implicit
+        self.implicit = implicit  # Was this Any type was inferred without a type annotation?
+        # Does this come from an unresolved import? See--disallow-implicit-any-types flag


Trivial: See-- -> See --

Didn't notice when I was committing! :)

ddfisher · 2017-05-26T23:23:23Z

test-data/unit/check-flags.test

+def f(x: X) -> None:  # E: Argument 1 to "f" is implicitly converted to List[Any] due to import from unanalyzed module
+    pass
+[builtins fixtures/list.pyi]
+[case testDisallowImplicitAnyCast]


Trivial: missing blank line before this line. It's a lot harder to read the tests if there isn't a blank line before each new testcase.

Yes! Updated my other tests too.

and wording in variable names and comments to reflect the name change.

ilinum · 2017-06-06T02:06:06Z

I updated the commit to use the new option name and syntax (for more, see #3470).

In the near future, this option will accept a list of parameters but for now it only accepts one (unimported). After this PR is merged, I will rename --disallow-untyped-defs to --disallow-any=untyped.

ilinum · 2017-06-06T02:09:48Z

mypy/messages.py

        self.note('Redundant cast to {}'.format(self.format(typ)), context)

+    def unimported_type_becomes_any(self, prefix: str, typ: Type, ctx: Context) -> None:
+        self.fail("{} becomes {} due to an unfollowed import (such imports occur either "


I'm not sure it is good to have an error message that long.

We could remove the explanation in parens but I'm not sure that most users will understand what an unfollowed import is otherwise.

@ddfisher what do you think?

I agree -- this error message is too long. I also agree that it'll probably be somewhat confusing. I'd say: remove the parenthetical explanation for now, and we can consider some other mechanism of explaining to the user if we think it's a big enough problem. (But we can/should do that in another PR.) We could consider outputting one Note: at the end if there are any of these errors explaining what an unfollowed import is.

Yeah, a note sounds like a better option!

ddfisher

This is looking good -- just a few things to fix up in the new code!

ddfisher · 2017-06-07T00:26:48Z

mypy/main.py

+        current_options = raw_options.split(',')
+        for option in current_options:
+            if option not in valid_disallow_any_options:
+                formatted_opts = ', '.join(map("'{}'".format, valid_disallow_any_options))


Nit: strongly prefer list comprehensions to map (or filter) in Python. I'm also a fan of functional programming, but list comprehensions are considered more pythonic.

I'd also consider calling this "formatted_valid_options" in order to disambiguate from the other "*_options" variables. I'd also write out "options" fully here -- it's a bit more verbose, but is more consistent with the rest of the variable names.

Yep!

The reason I chose map in that situation was that it was shorter than using a comprehension.
But it's probably better to use whatever is more pythonic :)

If you're interested, talk to Guido about his views on map vs list comprehensions. (Hint: they are strong views.)

I will, thanks :)

ddfisher · 2017-06-07T00:27:46Z

mypy/main.py

            strict_flag_assignments.append((dest, not default))

+    def disallow_any_argument_type(raw_options: str) -> List[str]:
+        current_options = raw_options.split(',')


I'm not sure this is named quite right. "current_options" implies that the options will be changing over time, which doesn't really happen. Maybe consider just "options"?

Good point. I was trying to make it clear that these options are the ones provided by user. I think just options works too!

ddfisher · 2017-06-07T00:32:57Z

mypy/main.py


    strict_flag_names = []  # type: List[str]
    strict_flag_assignments = []  # type: List[Tuple[str, bool]]
+    valid_disallow_any_options = {'unimported'}


Tiny nit: I think the "valid_" part of this name doesn't really add anything (and is very slightly misleading because it isn't only used in a checking-for-validity context: it's also displayed to the user).

More importantly, this should be a list instead of a set, because it's displayed to our users in a couple places. This is important for two reasons: 1) We don't want a nondeterministic --help output. 2) We're going to want to specify the options in a particular order that we think makes the most sense to read them in.

A set is canonically the "right" data structure to use here, because you're mainly checking for membership. However, it's so small that it doesn't matter (especially because it's not going to be run in a tight loop or anything).

Yep, all that makes sense!

Yeah, I didn't think about the fact that the help output could be in different order -- that's a good point!

ddfisher · 2017-06-07T00:39:08Z

mypy/main.py

+        for option in current_options:
+            if option not in valid_disallow_any_options:
+                formatted_opts = ', '.join(map("'{}'".format, valid_disallow_any_options))
+                message = "Unrecognized option '{}' (valid options are: {}).".format(


This line needs to mention that the option is related to --disallow-any. Much less importantly, I'd also consider "Invalid" instead of "Unrecognized", as it seems slightly more accurate.

I'd consider something like "Invalid '--disallow-any' option ...".

Oh wow, yeah good catch. I assumed when the error would be thrown, it would say the option name but I guess not.

Yep!

It's always good to run these things on the command-line yourself to make sure the output really looks as expected! (And sometimes the way you feel about phrasing, etc, can change when you see it in context.)

ddfisher · 2017-06-07T00:44:41Z

mypy/messages.py

        self.note('Redundant cast to {}'.format(self.format(typ)), context)

+    def unimported_type_becomes_any(self, prefix: str, typ: Type, ctx: Context) -> None:
+        self.fail("{} becomes {} due to an unfollowed import (such imports occur either "


I agree -- this error message is too long. I also agree that it'll probably be somewhat confusing. I'd say: remove the parenthetical explanation for now, and we can consider some other mechanism of explaining to the user if we think it's a big enough problem. (But we can/should do that in another PR.) We could consider outputting one Note: at the end if there are any of these errors explaining what an unfollowed import is.

ddfisher · 2017-06-07T18:17:24Z

mypy/main.py

+        for option in options:
+            if option not in disallow_any_options:
+                formatted_valid_options = ', '.join(
+                    "'{}'".format(option) for option in disallow_any_options)


option here shadows option defined on line 209 -- you really don't want to do that. I think a one-letter o would be fine as a name here (because the context you need to see what that means is all right on the same line).

Hmm, good point.
Actually, variable options on line 208 overshadows options from outer scope (line 372).
Will fix!

ddfisher · 2017-06-07T18:18:42Z

mypy/main.py

+        for option in current_options:
+            if option not in valid_disallow_any_options:
+                formatted_opts = ', '.join(map("'{}'".format, valid_disallow_any_options))
+                message = "Unrecognized option '{}' (valid options are: {}).".format(


It's always good to run these things on the command-line yourself to make sure the output really looks as expected! (And sometimes the way you feel about phrasing, etc, can change when you see it in context.)

ddfisher

This looks good now! There's just one final style nit that I think is important enough to address before merging (see inline comment).

ilinum · 2017-06-07T18:36:18Z

Should be good now!

ddfisher · 2017-06-07T20:25:59Z

Thanks!

Add flag to disallow implicit Any types.

29848f8

These types can appear from an unanalyzed module. If mypy encounters a type annotation that uses such a type, it will report an error. Fixes python#3205

ilinum force-pushed the disallow-implicit-any-types branch from 704a7b7 to 29848f8 Compare May 22, 2017 23:15

JukkaL reviewed May 23, 2017

View reviewed changes

ilinum changed the title ~~Add flag to disallow implicit Any types.~~ Add flag to disallow implicit Any types from Silent Imports May 23, 2017

ilinum changed the title ~~Add flag to disallow implicit Any types from Silent Imports~~ Disallow implicit Any types from Silent Imports May 23, 2017

ilinum mentioned this pull request May 23, 2017

Flags for disabling Any types #3427

Closed

ilinum added 2 commits May 23, 2017 15:07

Update error messages to be more user-friendly

e93f7aa

Expand error checking on several more cases

001d4c2

Fix issue with subclassing explicit Any not being reported

3b05b9c

ddfisher suggested changes May 25, 2017

View reviewed changes

ilinum added 3 commits May 25, 2017 12:19

Address code review comments from David

f3ce4e6

revert typeshed commit

7abf396

revert typeshed commit

16f8d75

ilinum added 3 commits May 25, 2017 13:30

Inline silent_mode method

e3b04f4

Fix typo (was using wrong option)

4a2eca5

Only report an error if this is a first assignment

7aea346

Also, handle multiple declaration

ddfisher suggested changes May 26, 2017

View reviewed changes

Address code review feedback from David

952c63b

ilinum mentioned this pull request May 27, 2017

Flag for disallowing Any #3470

Closed

gvanrossum assigned ddfisher May 29, 2017

Rename option to --disallow-any=unimported, modify error messages

b764a50

and wording in variable names and comments to reflect the name change.

ilinum commented Jun 6, 2017

View reviewed changes

ddfisher suggested changes Jun 7, 2017

View reviewed changes

Address code review feedback

845a671

ddfisher reviewed Jun 7, 2017

View reviewed changes

Rename variables that were shadowing other variables from outer scope

feeba7b

ddfisher approved these changes Jun 7, 2017

View reviewed changes

ddfisher merged commit 72168fa into python:master Jun 7, 2017

ilinum deleted the disallow-implicit-any-types branch June 7, 2017 20:39

Uh oh!

Conversation

ilinum commented May 22, 2017

Uh oh!

JukkaL left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilinum commented May 23, 2017

Uh oh!

ilevkivskyi commented May 24, 2017

Uh oh!

ilinum commented May 24, 2017

Uh oh!

ddfisher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilinum May 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilinum May 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilinum May 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilinum commented May 25, 2017

Uh oh!

ddfisher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ilinum May 25, 2017 •

edited

Loading

ilinum May 25, 2017 •

edited

Loading

ilinum May 25, 2017 •

edited

Loading