Add --warn-implicit-any by pkch · Pull Request #3141 · python/mypy

pkch · 2017-04-07T09:55:17Z

Experimental: attempt to address parts of #2901

gvanrossum · 2017-04-13T01:58:06Z

Since you say "experimental" I'm not reviewing this yet and I've added "[WIP]" to the subject. (I think GMail may even thread notifications correctly because of the brackets.)

pkch · 2017-04-27T08:59:45Z

It's far from perfect, but since it caught quite a few unintentional Any expansions in my code, maybe it's worth considering.

gvanrossum · 2017-05-10T22:18:58Z

I propose that @ddfisher should review this.

ilinum

I provided some cosmetic feedback but I unfortunately, I can't give any deep review since my knowledge of mypy is limited. Perhaps, @ddfisher can help.

Overall, I think this feature is great! We should try to finish it up and merge it.
Maybe we can name the flag --warn-implicit-any-generic or --disallow-implicit-any-generic.

ilinum · 2017-05-24T18:29:45Z

mypy/semanal.py


    def fail(self, msg: str, ctx: Context, serious: bool = False, *,
-             blocker: bool = False) -> None:
+             blocker: bool = False, implicit_any: bool = False) -> None:


It seems like adding an extra parameter to fail function might not be the best way to do it.
I would try to keep this function generic for any kind of failures.

Perhaps, you can check if the error is in the caller.

I agree. Unfortunately, I need to suppress the error message based on the information (options, and whether the module being checked is a stub) that's unavailable inside TypeAnalyser methods where the error occurs. I am not sure what the best fix for this would be. If I don't add an extra parameter to fail, I'll have to pass all this information into TypeAnalyser, but that's not good either since TypeAnalyser should not care about those details.

ilinum · 2017-05-24T18:31:20Z

mypy/semanal.py


-    def fail(self, msg: str, ctx: Context, *, blocker: bool = False) -> None:
+    def fail(self, msg: str, ctx: Context, *, blocker: bool = False,
+             implicit_any: bool = False) -> None:


Same as above - not sure if it's good to have an implicit_any parameter.

ilinum · 2017-05-24T18:32:05Z

mypy/semanal.py

+            if not self.options.warn_implicit_any or self.errors.file.endswith('.pyi'):
+                return
+            # TempNode, so must have already reported in the first pass
+            if ctx.get_line() == -1:


This seems like a hack but I couldn't find a way to do it better. Perhaps, someone with more mypy experience may help with that?

Are you talking about checking for stubs or checking for temporary nodes? I don't know if I would cosnider either of them a hack (my threshold of hackiness in the error-reporting code is much higher than in normal code); but if anyone has a better solution, it would be nice.

ilinum · 2017-05-24T18:33:52Z

mypy/typeanal.py

                return self.analyze_callable_type(t)
            elif fullname == 'typing.Type':
                if len(t.args) == 0:
+                    self.implicit_any('Type without type args', t)


There doesn's seem to be a test case for this. It's probably worth having one.

ilinum · 2017-05-24T18:36:50Z

mypy/typeanal.py

                if self.nesting_level > 0:
                    self.fail('Invalid type: ClassVar nested inside other type', t)
                if len(t.args) == 0:
+                    self.implicit_any('ClassVar without type args', t)


There doesn's seem to be a test case for this. It's probably worth having one.

ilinum · 2017-05-24T18:37:01Z

mypy/typeanal.py

                    # context. This is slightly problematic as it allows using the type 'Any'
                    # as a base class -- however, this will fail soon at runtime so the problem
                    # is pretty minor.
+                    self.implicit_any('Assigning value of type Any', t)


There doesn's seem to be a test case for this. It's probably worth having one.

ilinum · 2017-05-24T18:40:06Z

mypy/typeanal.py

                                            fallback=instance)
                return instance
        else:
+            self.implicit_any('Fallback', t)


This seems to be a debug statement.

Hmm, how can you tell that this cannot happen in a production run?

ilinum · 2017-05-24T18:45:38Z

mypy/semanal.py

+    def fail(self, msg: str, ctx: Context, *, blocker: bool = False,
+             implicit_any: bool = False) -> None:
+        if implicit_any:
+            if not self.options.warn_implicit_any or self.errors.file.endswith('.pyi'):


There should be a better way for checking for stub files but I can't seem to find it.

Why are you limiting the flag to non-stub files anyway?

I also can't find it.

In stub files, a lot of the times the type argument in genertic types is omitted (e.g., Union[type, Tuple], def f() -> tuple:, def g() -> Iterable:, etc.). There'll be hundreds of warnings like this, making the flag unusable.
I am sure we don't want to change the stubs to avoid implicit Any (and even if we did, that change would have to happen first, before warn_implicit_any starts paying attention to stubs).

ilinum · 2017-05-24T19:40:26Z

Just remembered this doesn't cover the cases when not all type parameters are specified.

So like Dict[str] would not be covered.

ilevkivskyi · 2017-05-28T07:44:42Z

@ilinum

So like Dict[str] would not be covered.

What do you mean by this? Dict[str] is already an error.

ilinum · 2017-05-30T17:57:38Z

@ilevkivskyi you are right. I looked at the code and it didn't seem like it would handle these kind of errors. But I just ran mypy now and it is an error.

ilinum · 2017-06-14T02:33:55Z

@pkch are you interested in working on this?

pkch · 2017-06-14T09:05:53Z

Ah yes, I'm going to make the fixes I'm aware of. However, I'm still not sure if the overall approach is acceptable.

(--disallow-any=generics) This code is based on python#3141 by pkch. This option disallows implicit Anys from omitted type parameters to generic types. For instance, `def x() -> List` would produce an error while `def x() -> List[Any]` is allowed. Note that with the flag enabled builtin generic types such as `list` and `set` are also forbidden.

ilinum · 2017-06-30T05:14:27Z

I reworked this PR and submitted it as #3637.

Closing this PR in favor of the new one.

(--disallow-any=generics) This code is based on #3141 by pkch. This option disallows implicit Anys from omitted type parameters to generic types. For instance, `def x() -> List` would produce an error while `def x() -> List[Any]` is allowed. Note that with the flag enabled builtin generic types such as `list` and `set` are also forbidden.

pkch mentioned this pull request Apr 7, 2017

--warn-return-any is too specialized #2901

Closed

gvanrossum changed the title ~~Add --warn-implicit-any~~ [WIP] Add --warn-implicit-any Apr 13, 2017

pkch mentioned this pull request Apr 26, 2017

Reliable version of os.replace for Windows (wait/retry loop) #3239

Closed

Add --warn-implicit-any

6292edc

pkch force-pushed the noargs-any branch from 468bd49 to 6292edc Compare April 27, 2017 08:57

pkch changed the title ~~[WIP] Add --warn-implicit-any~~ Add --warn-implicit-any Apr 27, 2017

gvanrossum requested a review from ddfisher May 10, 2017 22:19

This was referenced May 22, 2017

Disallow implicit Any types from Silent Imports #3405

Merged

Unbound parameters of type aliases silently default to Any #3386

Closed

Flags for disabling Any types #3427

Closed

ilinum reviewed May 24, 2017

View reviewed changes

pkch mentioned this pull request May 24, 2017

Fix crash when annotating callable definition location #3375

Merged

ilinum mentioned this pull request May 27, 2017

Flag for disallowing Any #3470

Closed

ilinum mentioned this pull request May 30, 2017

Omitted Generics Should Be Disallowed #3469

Closed

gvanrossum assigned ddfisher Jun 5, 2017

pkch added 2 commits June 24, 2017 09:39

Merge branch 'master' into noargs-any

abe4132

CR: add tests

aceeca3

ilinum mentioned this pull request Jun 30, 2017

Option to disallow omitting type parameters of generic types #3637

Merged

ilinum closed this Jun 30, 2017

Uh oh!

Conversation

pkch commented Apr 7, 2017

Uh oh!

gvanrossum commented Apr 13, 2017

Uh oh!

pkch commented Apr 27, 2017

Uh oh!

gvanrossum commented May 10, 2017

Uh oh!

ilinum 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

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilinum commented May 24, 2017

Uh oh!

ilevkivskyi commented May 28, 2017

Uh oh!

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

Uh oh!

Uh oh!

ilinum commented Jun 14, 2017

Uh oh!

pkch commented Jun 14, 2017

Uh oh!

ilinum commented Jun 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ilinum commented May 30, 2017 •

edited

Loading