Add flag to output error if a typed function is used with an untyped decorator #3555

ilinum · 2017-06-15T23:54:20Z

No description provided.

and the --disallow-untyped-defs flag is enabled

# Conflicts: # mypy/messages.py

ddfisher

Before merging, can you run this against our internal codebases and make sure that there aren't too many errors and that the errors seem like they really should be fixed?

ddfisher · 2017-07-01T00:26:22Z

mypy/checker.py

+        def is_typed_callable(c: Optional[Type]) -> bool:
+            if not c or not isinstance(c, CallableType):
+                return False
+            return all(not isinstance(t, AnyType) or not t.implicit


Nit: I think this would be much clearer as not any(isinstance(t, AnyType) and t.implicit).

(If you disagree, feel free to leave as-is.)

Hmm, I think that's better!

ddfisher · 2017-07-01T00:27:48Z

mypy/checker.py

+                                    func: FuncDef,
+                                    dec_type: Type,
+                                    dec_expr: Expression) -> None:
+        def is_typed_callable(c: Optional[Type]) -> bool:


It's better to not define subfunctions unless they actually use variables from the parent function. I'd make these top-level instead.

All right, that makes sense.

gvanrossum · 2017-07-05T23:24:13Z

The main concern is that this will be seen as a regression by people who have carefully annotated (part of) their codebase but are using untyped decorators (maybe from another part of their codebase). On the one hand these people have a problem (unannotated code persists, and the call sites may have further errors), on the other hand they may not have the bandwidth to solve it quickly, and until it's solved they can no longer use the --disallow-untyped-defs flag at all to require that all functions to be annotated. Which is why I would prefer this to be a separate flag (e.g. --disallow-untyped-decorators).

ilinum · 2017-07-06T17:48:52Z

After thinking about this some more, I agree with you, this change would discourage use of --disallow-untyped-defs, which is something we definitely we want to avoid.

I think it would be reasonable to have a new flag --disallow-untyped-decorators.

Just a note that there is --disallow-any=decorator (460a13b) that disallows functions that have Any in their signature after decorator transformation.

gvanrossum · 2017-07-06T18:21:25Z

OK, let's do the separate flag. Re: `--disallow-any=decorator` -- maybe that should be `decorated` instead? Or are you suggesting that that flag is sufficient for this use case? Maybe it is.

ddfisher · 2017-07-12T20:35:21Z

(The flag is actually --disallow-any=decorated, which is definitely better than --disallow-any=decorator.)

ddfisher · 2017-07-12T20:35:58Z

I think a new flag is the right way forward here. @ilinum can you make that change?

# Conflicts: # mypy/options.py

ddfisher · 2017-07-20T23:24:45Z

(Minor note: PR/commit names should be actions.)

ddfisher

Some functionality and testing changes needed.

ddfisher · 2017-07-20T23:25:55Z

mypy/checker.py

+def is_typed_callable(c: Optional[Type]) -> bool:
+    if not c or not isinstance(c, CallableType):
+        return False
+    return not any(isinstance(t, AnyType) and t.implicit


It looks like this is checking for a fully typed callable, but we should check for a callable with that has any annotations. This case should have appeared in tests.

ddfisher · 2017-07-20T23:27:01Z

test-data/unit/check-flags.test

+    return i
+
+[case testDisallowUntypedDefsMultipleUntypedDecorators]
+# flags: --disallow-untyped-decorators --disallow-untyped-defs


What is the --disallow-untyped-defs part of this test testing?

Hmm, my original thinking was to make sure to interacts well with --disallow-untyped-defs but I think that the flags don't have that much in common. I removed the test and added another one for multiple decorators.

ddfisher · 2017-07-20T23:28:16Z

test-data/unit/cmdline.test

 z.py:1: error: Function is missing a type annotation
 x.py:1: error: Function is missing a type annotation

+[case testDisallowUntypedDefsUntypedFunc]


Why can't this test be in check-flags.test?

Not sure of the original reason.
Moved to check-flags.test.

ddfisher · 2017-07-20T23:28:31Z

test-data/unit/check-flags.test

+
+@d  # no error
+def f(): pass
+


Please add tests for partially typed functions.

ilinum · 2017-08-03T06:19:04Z

Okay, code review feedback should be addressed now!

ddfisher

LGTM! Tiny tiny nit -- feel free to merge if you disagree, otherwise fix and then merge.

ddfisher · 2017-08-03T18:12:53Z

mypy/checker.py


                    self.check_for_missing_annotations(fdef)
                    if 'unimported' in self.options.disallow_any:
                        if fdef.type and isinstance(fdef.type, CallableType):


Nit: I think this may be slightly clearer as not all(... and ...)

Hmm, yep, I think that's better!
I will make the change, make sure it passes the tests and then merge.

ilinum added 2 commits June 15, 2017 16:34

Output error if a typed function is used with an untyped decorator

a107b7c

and the --disallow-untyped-defs flag is enabled

Merge branch 'master' into disallow-untyped-defs-decorated

483b7b6

# Conflicts: # mypy/messages.py

ddfisher approved these changes Jul 1, 2017

View reviewed changes

Address code review feedback

c0c38a9

ilinum added 2 commits July 19, 2017 21:15

Make the flag --disallow-untyped-decorators

70841d1

Merge branch 'master' into disallow-untyped-defs-decorated

bc7184c

# Conflicts: # mypy/options.py

ilinum changed the title ~~Output error if a typed function is used with an untyped decorator and the --disallow-untyped-defs flag is enabled~~ Flag to output error if a typed function is used with an untyped decorator Jul 20, 2017

ddfisher changed the title ~~Flag to output error if a typed function is used with an untyped decorator~~ Add flag to output error if a typed function is used with an untyped decorator Jul 20, 2017

ddfisher suggested changes Jul 20, 2017

View reviewed changes

ilinum added 3 commits August 2, 2017 22:54

Merge branch 'master' into disallow-untyped-defs-decorated

b9bc84f

Report errors if partially annotated defs is used with untyped decorator

6eb9d01

Clean up existing tests + add test for multiple decorators

4febfc0

ddfisher approved these changes Aug 3, 2017

View reviewed changes

any -> not all

1eeac3d

ilinum merged commit 7d7c71d into python:master Aug 3, 2017

ilinum deleted the disallow-untyped-defs-decorated branch August 3, 2017 19:37

Uh oh!

Add flag to output error if a typed function is used with an untyped decorator #3555

Add flag to output error if a typed function is used with an untyped decorator #3555

Uh oh!

Conversation

ilinum commented Jun 15, 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!

gvanrossum commented Jul 5, 2017

Uh oh!

ilinum commented Jul 6, 2017

Uh oh!

gvanrossum commented Jul 6, 2017 via email

Uh oh!

ddfisher commented Jul 12, 2017

Uh oh!

ddfisher commented Jul 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ddfisher commented Jul 20, 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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilinum commented Aug 3, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ddfisher commented Jul 12, 2017 •

edited

Loading