Allow overloads in source files, not just stubs #2603

sixolet · 2016-12-23T05:36:21Z

This should fix #1136

The implementation must directly follow all the overloads
The implementation is typechecked exactly according to its own declared types
Indicates an error if the implementation's argument list is not more general than every override variant
Also indicates an error if the implementation's return type is also not more general than the return type of every override variant

It also limits overloads to the decorators that are specifically geared towards providing overload-type functionality -- @overload, but also @property and its cousins @funcname.setter and @funcname.deleter. All other decorators are treated purely as decorators, and now provide redefinition errors if you repeat a function definition with them, instead of errors about an overload you probably did not mean.

gvanrossum

I am really excited about this! Here are some nits, nothing major except the missing case of the decorated implementation (which actually needs to be fixed in three places).

gvanrossum · 2017-01-03T22:15:43Z

test-data/unit/semanal-errors.test

@@ -1056,25 +1056,25 @@ from typing import overload
 def dec(x): pass
 @overload
 def f(): pass
-@dec  # E: 'overload' decorator expected
+@dec # E: Name 'f' already defined


We actually prefer two spaces before the # E: -- this follows PEP 8 (there are still many examples with one space from long ago).

gvanrossum · 2017-01-03T22:23:18Z

test-data/unit/check-overloading.test

+def f(x: 'B') -> 'A': ...
+
+def f(x: Any) -> Any:
+   if isinstance(x, A):


Since test code never gets executed, you might as well leave the body pass.

gvanrossum · 2017-01-03T22:26:01Z

test-data/unit/check-overloading.test

+[builtins fixtures/isinstance.pyi]
+
+[case testTypeCheckOverloadWithImplementationFastparse]
+# flags: --fast-parser


Honestly I would be happy if this only worked with --fast-parser -- we're going to phase out the slow parser soon(-ish) and I don't see the need to double the number of tests for this feature just to ensure it works with both parsers. (However it would be good to also have some PY2 tests.)

Turning doubled tests into py2 tests, now that they're fast-parser by default.

gvanrossum · 2017-01-03T22:42:20Z

test-data/unit/check-overloading.test

+def f(x: Any) -> Any:
+   if isinstance(x, A):
+       y = x
+       y = B() # E: Incompatible types in assignment (expression has type "B", variable has type "A")


Is it important that the error refers to the classes used in the overload? Assuming we don't do anything special for checking the body of the implementation, I'd only care about a few things:

for a body without annotations, errors are not reported

for a body with annotations (even just -> Any), errors are reported as usual

Simplified the test and then re-complicated it by testing both the things you suggest.

gvanrossum · 2017-01-03T23:16:33Z

mypy/semanal.py

@@ -2906,6 +2909,16 @@ def visit_overloaded_func_def(self, func: OverloadedFuncDef) -> None:
        func._fullname = self.sem.qualified_name(func.name())
        if kind == GDEF:
            self.sem.globals[func.name()] = SymbolTableNode(kind, func, self.sem.cur_mod_id)
+        if func.impl:
+            # Also analyze the function body (in case there are conditional imports).


Hmm... Refactor this to merge with the nearly identical block in the previous method?

gvanrossum · 2017-01-03T23:25:42Z

mypy/fastparse.py

@@ -190,19 +192,27 @@ def as_block(self, stmts: List[ast35.stmt], lineno: int) -> Block:

    def fix_function_overloads(self, stmts: List[Statement]) -> List[Statement]:
        ret = []  # type: List[Statement]
-        current_overload = []
+        current_overload = []  # type: List[Decorator]
        current_overload_name = None
        # mypy doesn't actually check that the decorator is literally @overload


Maybe update this comment a bit? It's still technically correct but less than before...

gvanrossum · 2017-01-03T23:32:32Z

mypy/fastparse.py

+                  and stmt.name() is not None):
+                ret.append(OverloadedFuncDef(current_overload, stmt))
+                current_overload = []
+                current_overload_name = None
            else:


Looks like there's a missing case here: a Decorator that's not an overload but does match the current overload name. That would indicate a decorated implementation, which I think we ought to support.

gvanrossum · 2017-01-03T23:35:10Z

mypy/sharedparse.py

+
+
+def _is_overload_decorator(dec):
+    if isinstance(dec, NameExpr) and dec.name in {'overload', 'property', 'abstractproperty'}:


Can you add a comment explaining why properties are also considered overloads? Is this just because the same parse tree structure is used to support a sequence of @property, @f.getter and/or @f.deleter?

Instead I deferred it to semanalysis time, where it belongs

gvanrossum · 2017-01-03T23:54:52Z

mypy/messages.py

@@ -807,6 +807,14 @@ def overloaded_signatures_overlap(self, index1: int, index2: int,
        self.fail('Overloaded function signatures {} and {} overlap with '
                  'incompatible return types'.format(index1, index2), context)

+    def overloaded_signatures_arg_specific(self, index1: int, context: Context) -> None:
+        self.fail('Overloaded function implementation cannot accept all possible arguments '


"cannot" -> "does not" or "should". (But the next one is correct IMO.)

gvanrossum · 2017-01-03T23:58:52Z

test-data/unit/parse.test

          FuncDef:3(
            g
            Block:3(
              PassStmt:3())))
        Decorator:4(
          Var(g)
-          NameExpr(foo)
+          NameExpr(overload)


I feel this file should also grow some additional tests that show the representation of an overload with implementation.

They've stopped being interesting in the parse phase, but the semantic analysis phase files now have such tests.

The docs say `@override` doesn't work in user code, but it seems to work in mypy 0.470. The update may be waiting on python#2603, but that PR does not seem to include doc updates, so feel free to put this patch in that PR.

…ub files

sixolet

Ok, I think this is now mostly updated to reflect these comments.

sixolet · 2017-02-17T06:26:59Z

mypy/sharedparse.py

+
+
+def _is_overload_decorator(dec):
+    if isinstance(dec, NameExpr) and dec.name in {'overload', 'property', 'abstractproperty'}:


Instead I deferred it to semanalysis time, where it belongs

sixolet · 2017-02-17T06:28:21Z

test-data/unit/check-overloading.test

+def f(x: 'B') -> 'A': ...
+
+def f(x: Any) -> Any:
+   if isinstance(x, A):


sixolet · 2017-02-17T06:29:20Z

test-data/unit/check-overloading.test

+[builtins fixtures/isinstance.pyi]
+
+[case testTypeCheckOverloadWithImplementationFastparse]
+# flags: --fast-parser


Turning doubled tests into py2 tests, now that they're fast-parser by default.

sixolet · 2017-02-17T06:45:27Z

test-data/unit/check-overloading.test

+def f(x: Any) -> Any:
+   if isinstance(x, A):
+       y = x
+       y = B() # E: Incompatible types in assignment (expression has type "B", variable has type "A")


Simplified the test and then re-complicated it by testing both the things you suggest.

sixolet · 2017-02-17T06:46:13Z

test-data/unit/parse.test

          FuncDef:3(
            g
            Block:3(
              PassStmt:3())))
        Decorator:4(
          Var(g)
-          NameExpr(foo)
+          NameExpr(overload)


They've stopped being interesting in the parse phase, but the semantic analysis phase files now have such tests.

sixolet · 2017-02-17T07:06:53Z

test-data/unit/check-abstract.test

@@ -410,6 +410,8 @@ class B(object, A): pass \
      # E: Cannot determine consistent method resolution order (MRO) for "B"

 [case testOverloadedAbstractMethod]
+from foo import *


This chicanery is now endemic to the previously-existing tests, to make them all test overloads in stub files, so I don't have to introduce an implementation to each.

sixolet · 2017-02-17T07:10:08Z

NB: The rules used to determine whether an implementation is valid for a series of overloads take into account its argument names, the same way function subtyping does. That strictness is technically correct (my favorite kind of correct), but may be annoying.

ilevkivskyi · 2017-02-17T17:45:19Z

@sixolet Hm, Appveyor build failed due to / vs \ issue on Windows. Maybe this is related to your extensive use of from foo import * in tests?

sixolet · 2017-02-17T17:56:58Z

Well, I just implemented the syntax for inline errors in non-main files. I've already got another commit prepped that I theorize will process them right, though I don't have a Windows box on hand. I'll post it in a sec, once my local run finishes.

…

On Feb 17, 2017, at 9:45 AM, Ivan Levkivskyi ***@***.***> wrote: @sixolet Hm, Appveyor build failed due to / vs \ issue on Windows. Maybe this is related to your extensive use of from foo import * in tests? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

gvanrossum

Can you also merge master? (We're trying out a new policy -- it seems merging is actually preferable for the reviewer over rebasing.)

gvanrossum · 2017-03-13T17:10:22Z

test-data/unit/check-unsupported.test

-main:5: error: Name 'f' already defined
-main:7: error: Name 'g' already defined
+tmp/foo.pyi:7: error: Name 'g' already defined
+tmp/foo.pyi:5: error: Name 'f' already defined


This is really weird. Why are the error messages not in line-number order? (I thought there was some sorting in errors.py???)

I spent a while tracking down the reason for it:

For objects we initially thought were part of an overload series, but turn out not to be, we give the already-defined error in semantic analysis phase two, now.

For objects that are plain redefinitions, we give the already-defined error in phase 1.

We were setting an import_context object differently between these two phases

Error sorting groups by import_context, not just file.

I've made a change to set import_context the same way between the two phases.

gvanrossum · 2017-03-13T17:16:55Z

test-data/unit/check-overloading.test

+@overload
+def f(x: 'B') -> 'A': ...
+
+def f(x: 'A') -> Any: # E: Overloaded function implementation does not accept all possible arguments of signature 2


I'd love to see a line number reference here for the specific signature that we're incompatible with here. I know it's probably hard to do, so maybe in a follow-up PR?

gvanrossum · 2017-03-13T19:57:27Z

mypy/checker.py

+                if impl_type is None or sig1 is None:
+                    return
+
+                assert isinstance(impl_type, CallableType)


I can get this to crash. Here's a hint on the repro:

@overload def f(a: int) -> int: return a+1 @overload def f(s: str) -> str: return str(int(s) + 1) @deco def f(a: Any) -> Any: pass

According to pdb impl_type is Any; defn.impl is a Decorator.

Thank you, adding a test and a fix. The fix treats an Any-typed implementation as fine no matter what.

gvanrossum · 2017-03-13T20:00:52Z

mypy/fastparse.py

        for stmt in stmts:
-            if isinstance(stmt, Decorator) and stmt.name() == current_overload_name:
+            if (isinstance(stmt, Decorator)
+                    and stmt.name() == current_overload_name):


I think this should double-check that one of the decorators is actually @overload (or @typing.overload).

And if there are decorators but none of them are "overload" and the name matches, this should be considered the implementation.

And there should be tests for that.

We can't check that here -- we don't know until the semantic analysis step. There's code there to compensate for that.

Making sure we have a test for all the various cases of that and that they give solid error messages.

(Yes, it gives you the redefinition error message, correctly)

gvanrossum · 2017-03-13T20:01:32Z

mypy/fastparse.py

                current_overload.append(stmt)
+            elif (isinstance(stmt, FuncDef)
+                  and stmt.name() == current_overload_name
+                  and stmt.name() is not None):


This would make more sense as checking whether current_overload_name is not None before even calling isinstance() or comparing stmt.name() to it.

Cool, yeah.

gvanrossum · 2017-03-13T21:07:37Z

mypy/semanal.py

+                    # Some of them were overloads, but not all.
+                    for idx in non_overload_indexes:
+                        if self.is_stub_file:
+                            self.fail("Implementations of overloaded functions "


Why the plural (here and in the next error)? Surely at most one implementation is allowed anyway. I think all three plurals in the sentence should become singulars.

gvanrossum · 2017-03-13T21:15:03Z

mypy/semanal.py

        t = []  # type: List[CallableType]
+        non_overload_indexes = []


Maybe this variable should have "error" somewhere in its name?

It's not an error as long as the only non-overload is the last one.

gvanrossum · 2017-03-13T21:16:17Z

mypy/semanal.py

+
+            elif not self.is_stub_file and not non_overload_indexes:
+                self.fail(
+                    "Overload outside a stub must have implementation",


Either remove "a" or add "an" before "implementation".

gvanrossum · 2017-03-13T21:18:35Z

mypy/semanal.py

+                    "Overload outside a stub must have implementation",
+                    defn)
+
+        if t:


I think it's time for t to get a longer name.

gvanrossum · 2017-03-13T21:57:39Z

mypy/semanal.py

+                sem.function_stack.append(impl.func)
+                sem.errors.push_function(func.name())
+                sem.enter()
+                impl.func.body.accept(self)


I think this deserves at least an else: assert False since otherwise the stack management would be out of sync.

sixolet

Ok, I think I handled most of these comments now

sixolet · 2017-03-15T01:03:06Z

mypy/checker.py

+                if impl_type is None or sig1 is None:
+                    return
+
+                assert isinstance(impl_type, CallableType)


Thank you, adding a test and a fix. The fix treats an Any-typed implementation as fine no matter what.

sixolet · 2017-03-15T01:09:49Z

mypy/fastparse.py

        for stmt in stmts:
-            if isinstance(stmt, Decorator) and stmt.name() == current_overload_name:
+            if (isinstance(stmt, Decorator)
+                    and stmt.name() == current_overload_name):


We can't check that here -- we don't know until the semantic analysis step. There's code there to compensate for that.

Making sure we have a test for all the various cases of that and that they give solid error messages.

sixolet · 2017-03-15T01:15:35Z

mypy/fastparse.py

        for stmt in stmts:
-            if isinstance(stmt, Decorator) and stmt.name() == current_overload_name:
+            if (isinstance(stmt, Decorator)
+                    and stmt.name() == current_overload_name):


(Yes, it gives you the redefinition error message, correctly)

sixolet · 2017-03-15T01:21:53Z

mypy/fastparse.py

                current_overload.append(stmt)
+            elif (isinstance(stmt, FuncDef)
+                  and stmt.name() == current_overload_name
+                  and stmt.name() is not None):


Cool, yeah.

sixolet · 2017-03-15T01:23:37Z

mypy/nodes.py

@@ -535,6 +545,9 @@ def name(self) -> str:
    def accept(self, visitor: StatementVisitor[T]) -> T:
        return visitor.visit_func_def(self)

+    def get_body(self) -> Optional['Block']:


... Thank you. Cruft from a previous way of doing it.

sixolet · 2017-03-15T18:45:04Z

mypy/semanal.py

+                    # Some of them were overloads, but not all.
+                    for idx in non_overload_indexes:
+                        if self.is_stub_file:
+                            self.fail("Implementations of overloaded functions "


sixolet · 2017-03-15T18:45:09Z

mypy/semanal.py

+
+            elif not self.is_stub_file and not non_overload_indexes:
+                self.fail(
+                    "Overload outside a stub must have implementation",


sixolet · 2017-03-15T18:45:14Z

mypy/semanal.py

+                    "Overload outside a stub must have implementation",
+                    defn)
+
+        if t:


sixolet · 2017-03-15T18:46:32Z

mypy/semanal.py

+                sem.function_stack.append(impl.func)
+                sem.errors.push_function(func.name())
+                sem.enter()
+                impl.func.body.accept(self)


sixolet · 2017-03-16T00:34:59Z

test-data/unit/check-unsupported.test

-main:5: error: Name 'f' already defined
-main:7: error: Name 'g' already defined
+tmp/foo.pyi:7: error: Name 'g' already defined
+tmp/foo.pyi:5: error: Name 'f' already defined


I spent a while tracking down the reason for it:

For objects we initially thought were part of an overload series, but turn out not to be, we give the already-defined error in semantic analysis phase two, now.

For objects that are plain redefinitions, we give the already-defined error in phase 1.

We were setting an import_context object differently between these two phases

Error sorting groups by import_context, not just file.

I've made a change to set import_context the same way between the two phases.

gvanrossum

Major progress -- I found nothing else except some excess blank lines and a suggestion for a new test. We also discussed over lunch that there should be a bunch of tests involving generic functions in various positions, checking both correct examples and error cases.

gvanrossum · 2017-03-20T21:20:59Z

mypy/nodes.py


    This node has no explicit representation in the source program.
    Overloaded variants must be consecutive in the source file.
+


This blank line should not be here.

gvanrossum · 2017-03-20T22:30:20Z

test-data/unit/check-overloading.test

+class B: pass
+[builtins fixtures/isinstance.pyi]
+
+


One blank line is enough.

gvanrossum · 2017-03-20T22:32:13Z

test-data/unit/check-overloading.test

+class B: pass
+[builtins fixtures/isinstance.pyi]
+
+[case testTypeCheckOverloadWithDecoratedImplementation]


How about another test that checks for errors where the implementation doesn't match the overloads.

There are two so far.

gvanrossum

Thanks for adding the new tests! There are still two blank lines I'd like to kill, and on re-reviewing the major recent diff I found one questionable issue.

Otherwise LGTM.

gvanrossum · 2017-03-24T21:05:37Z

mypy/semanal.py

+                    item.accept(self)
+                # TODO support decorated overloaded functions properly
+                if isinstance(item, Decorator):
+                    item.func.is_overload = True


Do you need this also for the implementation? Maybe move this into the else: block at line 470?

But then why is it not set for non-decorated implementations? Something's still fishy.

Never mind, I was wrong, confusing item.is_overload and item.func.is_overload. You're right.

gvanrossum · 2017-03-25T02:19:12Z

Yee-haw! Now we're just waiting for a clean run of mypy against our internal codebases.

gvanrossum · 2017-03-27T15:28:01Z

Yee-haw! Thanks so much Naomi.

pkch · 2017-03-28T22:41:06Z

Is it already the time to update the docs (they still say that overloading isn't allowed outside the stubs)? Or we need to wait until it becomes part of an official mypy version?

ilevkivskyi · 2017-03-28T22:42:33Z

@pkch FYI there is already a PR #2792 for this.

gvanrossum · 2017-03-28T22:50:33Z

Policy is to update the docs in master at the same time as the code, or as soon after as we can (but always before the next release). People wanting docs for the latest released version can go to http://mypy.readthedocs.io/en/stable/.

The docs say `@override` doesn't work in user code, but it seems to work in mypy 0.470. The update may be waiting on #2603, but that PR does not seem to include doc updates, so feel free to put this patch in that PR.

@sixolet

…es) (#3159) These are the doc updates corresponding to PR #2603 by @sixolet (specified in a revision of PEP 484: https://www.python.org/dev/peps/pep-0484/#function-method-overloading). The docs got started in PR #PR #2792 by @jkleint.

gvanrossum reviewed Jan 3, 2017

View reviewed changes

jkleint mentioned this pull request Feb 2, 2017

Docs: show how to @overload function annotations in user code #2792

Closed

ilevkivskyi mentioned this pull request Feb 13, 2017

Question about usage of union of Tuples python/typing#391

Closed

sixolet force-pushed the overload branch from 354ef42 to 37ba7d4 Compare February 17, 2017 06:20

sixolet added 8 commits February 16, 2017 22:57

Overloads in source files, not just stub files

e59f264

Properties count as overloads too

bb5fdae

tests and fixes and types and such

ecb5f73

Add impl & test for fastparse2

435751d

Temporary checkpoint -- most tests pass but not overloads in classes

7b3c08d

Partly-programatically modify all non-impl overload tests to be in st…

6296587

…ub files

Refactor so that overload decides on its impl at semanal time

91b1726

Guido comments and polish

dbc770c

sixolet force-pushed the overload branch from 04991b7 to dbc770c Compare February 17, 2017 06:57

sixolet commented Feb 17, 2017

View reviewed changes

s/cannot/does not/

80fc979

sixolet commented Feb 17, 2017

View reviewed changes

Fix windows paths when expanding errors in non-main files

045be3e

sixolet added 2 commits February 17, 2017 11:08

Another theory about how to make windows paths work

95555e6

It is simpler to have OverloadPart be a union

c12a9fd

gvanrossum requested changes Mar 13, 2017

View reviewed changes

sixolet added 2 commits March 14, 2017 15:25

Merge master

143692a

Many of Guidos comments

04ce906

sixolet commented Mar 16, 2017

View reviewed changes

gvanrossum reviewed Mar 20, 2017

View reviewed changes

sixolet added 2 commits March 23, 2017 16:14

Test implementation with type variables, and fix the bugs

a191620

lint

b24d2ec

gvanrossum reviewed Mar 24, 2017

View reviewed changes

sixolet added 2 commits March 24, 2017 18:37

remove extra blank linkes

9fa66f4

Put is_overload in correct else clause

c6c6d03

gvanrossum approved these changes Mar 25, 2017

View reviewed changes

gvanrossum merged commit e674e25 into python:master Mar 27, 2017

elazarg mentioned this pull request Mar 29, 2017

Strip type objects when constructing TypeType #2832

Merged

elazarg mentioned this pull request Apr 13, 2017

[Refactoring] Remove Decorator.type #3166

Merged



		def _is_overload_decorator(dec):
		if isinstance(dec, NameExpr) and dec.name in {'overload', 'property', 'abstractproperty'}:


		This node has no explicit representation in the source program.
		Overloaded variants must be consecutive in the source file.

Allow overloads in source files, not just stubs #2603

Allow overloads in source files, not just stubs #2603

Conversation

sixolet commented Dec 23, 2016 • edited Loading

gvanrossum 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

sixolet 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

sixolet commented Feb 17, 2017

ilevkivskyi commented Feb 17, 2017

sixolet commented Feb 17, 2017 via email

gvanrossum 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

sixolet 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

gvanrossum 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

gvanrossum 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

gvanrossum commented Mar 25, 2017

gvanrossum commented Mar 27, 2017

pkch commented Mar 28, 2017

sixolet commented Dec 23, 2016 •

edited

Loading