Skip to content

Commit

Permalink
Differentiate overload stubs and implementations
Browse files Browse the repository at this point in the history
Summary:
The typechecker needs to ignore the existence of a signature if that signature is overloaded with typing.overload decorated defines. In other words, when selecting signatures, the overloaded define (containing the actual implementation of the function) should never be selected.

typing.overload designates a definition that is purely for the typechecker's benefit, and is ignored at runtime because it is immediately overwritten with a definition that contains the actual implementation. Calling a function decorated with typing.overload throws at runtime:
```
>>> import typing
>>> typing.overload
... def foo(x):
...     return 1
...
>>> foo(1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/fbcode/gcc-5-glibc-2.23/lib/python3.6/typing.py", line 1591, in _overload_dummy
    "You should not call an overloaded function. "
NotImplementedError: You should not call an overloaded function. A series of The controller you requested could not be found. functions outside a stub module should always be followed by an implementation that is not The controller you requested could not be found..
```

Therefore, the purpose of typing.overload defines is simply to clarify the type signature when the return annotation should vary depending on the parameter annotations (but maybe not in a way specifiable with type vars):

```
typing.overload
def foo(x: int) -> str: ...

typing.overload
def foo(x:str) -> int:...

def foo(x: Union[int, str]) -> Union[int, str]
```

Currently, when pyre sees the above set of signatures and then tries to resolve a call to `foo(x)` where `x` is an `int`, we look at all overloads equally and can select the `Union[int, str] -> Union[int, str]` signature and end up assuming the return type of `foo(x)` is `Union[int, str]` rather than the correct and more specific `str`.

If `typing.overload` definitions exist, we should be selecting only from those, and not from the implementation signature.

**NodeAPI Bug (T35334236)**
```
    overload
    staticmethod
    def get(roots):
        # type: (TRoot) -> NodeGetCity[NodeCity]
        pass

    overload  # noqa
    staticmethod
    def get(roots):
        # type: (TRoots) -> NodeGetCity[List[NodeCity]]
        pass

    staticmethod  # noqa
    def get(roots):
        # type: (Union[TRoot, TRoots]) -> NodeGetCity
        """
        Get the node object/s corresponding to the roots.
        param roots: Node|fbid, or iterable<Node|fbid>.
        return A Query object that will return a Node if a Node or NodeId
                was passed as roots otherwise will return a list of Nodes.
        """
        return NodeGetCity(roots, nodeapi_ext.NodeErrorControllerType.ENFORCE)
```

Pyre selects the third method, which has a return type of NodeGetCity with an unknown type parameter (which inherits from awaitable), so awaiting on this returns an unknown type. Pyre *should* realize that the overloaded methods are there to refine the type signature of get depending on the input type. Merging with a pre-existing task to handle overloads this way.

**More context:**
Pep:
https://docs.python.org/3/library/typing.html#typing.overload
Discussion:
python/typing#253
python/mypy#3160
python/typing#14

**Next steps - not urgent (T35517446):**
1. When typechecking a define, if it is decorated with an typing.overload and there does not exist a matching definition *not* decorated with typing.overload. (overload functions throw when called at runtime)
2. When typechecking a define, if there exist other same name defines decorated with typing.overload, throw if overloading functions do not comprehensively cover the type signature of this overloaded function

Reviewed By: dkgi

Differential Revision: D10494018

fbshipit-source-id: 48973008d94cc539198ec13552b8108412413f2a
  • Loading branch information
shannonzhu authored and facebook-github-bot committed Oct 25, 2018
1 parent 81f27f9 commit 27250ce
Show file tree
Hide file tree
Showing 14 changed files with 196 additions and 71 deletions.
1 change: 1 addition & 0 deletions analysis/annotatedAccess.ml
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,7 @@ let fold ~resolution ~initial ~f access =
|> Option.value ~default:false
in
match getattr with
| Some (Callable { overload_stubs = [overload]; _ })
| Some (Callable { overloads = [overload]; _ })
when correct_getattr_arity overload ->
Some (
Expand Down
22 changes: 16 additions & 6 deletions analysis/annotatedCallable.ml
Original file line number Diff line number Diff line change
Expand Up @@ -89,14 +89,24 @@ let create defines ~resolution =
else
Type.Callable.Function
in
let to_overload ({ Define.parameters; _ } as define) =
{
annotation = return_annotation ~define ~resolution;
parameters = Defined (List.map parameters ~f:parameter);
}
let overloads, overload_stubs =
let to_overload (implementations, stubs) ({ Define.parameters; _ } as define) =
let overload =
{
annotation = return_annotation ~define ~resolution;
parameters = Defined (List.map parameters ~f:parameter);
}
in
if Define.is_overloaded_method define then
implementations, overload :: stubs
else
overload :: implementations, stubs
in
List.fold ~init:([],[]) ~f:to_overload defines
in
{
kind = Named name;
overloads = List.map defines ~f:to_overload;
overloads;
overload_stubs;
implicit;
}
15 changes: 10 additions & 5 deletions analysis/annotatedClass.ml
Original file line number Diff line number Diff line change
Expand Up @@ -637,10 +637,13 @@ let attributes
match
Annotation.annotation existing_annotation,
Annotation.annotation new_annotation with
| Type.Callable ({ overloads; _ } as existing_callable),
Type.Callable { overloads = new_overloads; _ } ->
| Type.Callable ({ overloads; overload_stubs; _ } as existing_callable),
Type.Callable
{ overloads = new_overloads; overload_stubs = new_overload_stubs; _ } ->
Annotation.create (Type.Callable {
existing_callable with overloads = new_overloads @ overloads })
existing_callable with
overloads = new_overloads @ overloads;
overload_stubs = new_overload_stubs @ overload_stubs })
| _ ->
existing_annotation
in
Expand Down Expand Up @@ -777,6 +780,7 @@ let fallback_attribute ~resolution ~access definition =
in
begin
match annotation with
| Type.Callable { Type.Callable.overload_stubs = overload :: _; _ }
| Type.Callable { Type.Callable.overloads = overload :: _; _ } ->
let return_annotation = Type.Callable.Overload.return_annotation overload in
let location = Attribute.location fallback in
Expand Down Expand Up @@ -867,7 +871,7 @@ let constructor definition ~resolution =
|> Attribute.annotation
|> Annotation.annotation
|> function
| Type.Callable ({ Type.Callable.overloads; _ } as callable) ->
| Type.Callable ({ Type.Callable.overloads; overload_stubs; _ } as callable) ->
let open Type.Callable in
(* __new__ requires a metaclass to be passed in as the first argument; unannotate to
prevent extraneous errors. *)
Expand All @@ -880,7 +884,8 @@ let constructor definition ~resolution =
overload
in
let overloads = List.map overloads ~f:unannotate_first_parameter in
Type.Callable { callable with Type.Callable.overloads }
let overload_stubs = List.map overload_stubs ~f:unannotate_first_parameter in
Type.Callable { callable with Type.Callable.overloads; overload_stubs }
| annotation ->
annotation
in
Expand Down
20 changes: 18 additions & 2 deletions analysis/annotatedSignature.ml
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,22 @@ type signature_match = {
}


let select ~resolution ~arguments ~callable:({ Type.Callable.overloads; _ } as callable) =
let select
~resolution
~arguments
~callable:({ Type.Callable.overloads; overload_stubs; _ } as callable) =
let open Type.Callable in
let removed_stubbed_implementations ~implementations ~stubs =
let unstubbed_implementations =
let not_stubbed implementation =
let ignore_annotation _ _ = true in
let arity_equal = Type.Callable.equal_overload ignore_annotation in
not (List.exists ~f:(arity_equal implementation) stubs)
in
List.filter ~f:not_stubbed implementations
in
unstubbed_implementations @ stubs
in
let match_arity ({ parameters = all_parameters; _ } as overload) =
let base_signature_match =
{
Expand Down Expand Up @@ -542,6 +556,7 @@ let select ~resolution ~arguments ~callable:({ Type.Callable.overloads; _ } as c
Type.Callable {
Type.Callable.kind = Anonymous;
overloads;
overload_stubs = [];
implicit = Function;
}
|> Type.variables
Expand Down Expand Up @@ -620,7 +635,8 @@ let select ~resolution ~arguments ~callable:({ Type.Callable.overloads; _ } as c
>>| determine_reason
|> Option.value ~default:(NotFound { callable; reason = None })
in
List.filter_map ~f:match_arity overloads
removed_stubbed_implementations ~implementations:overloads ~stubs:overload_stubs
|> List.filter_map ~f:match_arity
|> List.map ~f:check_annotations
|> List.map ~f:calculate_rank
|> find_closest
Expand Down
25 changes: 13 additions & 12 deletions analysis/test/annotatedCallableTest.ml
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ let test_apply_decorators _ =


let test_create _ =
let assert_callable ?parent source expected =
let assert_callable ?parent ?expected ?expected_stubs source =
let resolution =
populate source
|> fun environment -> Environment.resolution environment ()
Expand All @@ -120,34 +120,36 @@ let test_create _ =
assert_equal
~printer:Type.show
~cmp:Type.equal
(parse_callable expected)
(parse_callable_with_stubs expected expected_stubs)
callable
in

assert_callable "def foo() -> int: ..." "typing.Callable('foo')[[], int]";
assert_callable "async def foo() -> int: ..." "typing.Callable('foo')[[], typing.Awaitable[int]]";
assert_callable "def foo() -> int: ..." ~expected:"typing.Callable('foo')[[], int]";
assert_callable
"async def foo() -> int: ..."
~expected:"typing.Callable('foo')[[], typing.Awaitable[int]]";

assert_callable
"def foo(a, b) -> str: ..."
"typing.Callable('foo')[[Named(a, $unknown), Named(b, $unknown)], str]";
~expected:"typing.Callable('foo')[[Named(a, $unknown), Named(b, $unknown)], str]";
assert_callable
"def foo(a: int, b) -> str: ..."
"typing.Callable('foo')[[Named(a, int), Named(b, $unknown)], str]";
~expected:"typing.Callable('foo')[[Named(a, int), Named(b, $unknown)], str]";
assert_callable
"def foo(a: int = 1) -> str: ..."
"typing.Callable('foo')[[Named(a, int, default)], str]";
~expected:"typing.Callable('foo')[[Named(a, int, default)], str]";

assert_callable
"def foo(a, *args, **kwargs) -> str: ..."
"typing.Callable('foo')[[Named(a, $unknown), Variable(args), Keywords(kwargs)], str]";
~expected:"typing.Callable('foo')[[Named(a, $unknown), Variable(args), Keywords(kwargs)], str]";
assert_callable
"def foo(**kwargs: typing.Dict[str, typing.Any]) -> str: ..."
"typing.Callable('foo')[[Keywords(kwargs, typing.Dict[str, typing.Any])], str]";
~expected:"typing.Callable('foo')[[Keywords(kwargs, typing.Dict[str, typing.Any])], str]";

assert_callable
~parent:"module.Foo"
"def module.Foo.foo(a: int, b) -> str: ..."
"typing.Callable('module.Foo.foo')[[Named(a, int), Named(b, $unknown)], str]";
~expected:"typing.Callable('module.Foo.foo')[[Named(a, int), Named(b, $unknown)], str]";

assert_callable
~parent:"module.Foo"
Expand All @@ -157,8 +159,7 @@ let test_create _ =
@overload
def module.Foo.foo(a: str) -> str: ...
|}
(* Note that the overload order is reversed from the declaration - shouldn't matter. *)
"typing.Callable('module.Foo.foo')[[Named(a, str)], str][[Named(a, int)], int]";
~expected_stubs:"typing.Callable('module.Foo.foo')[[Named(a, int)], int][[Named(a, str)], str]";

let assert_implicit_argument ?parent source expected =
let resolution =
Expand Down
8 changes: 4 additions & 4 deletions analysis/test/annotatedClassTest.ml
Original file line number Diff line number Diff line change
Expand Up @@ -273,8 +273,8 @@ let test_constructors _ =
def Foo.__init__(self, b: str) -> None: pass
|}
(Some
("typing.Callable('Foo.__init__')[[Named(self, $unknown), Named(b, str)], Foo]" ^
"[[Named(self, $unknown), Named(a, int)], Foo]"));
("typing.Callable('Foo.__init__')[[Named(self, $unknown), Named(a, int)], Foo]" ^
"[[Named(self, $unknown), Named(b, str)], Foo]"));

(* Generic classes. *)
assert_constructor
Expand Down Expand Up @@ -758,8 +758,8 @@ let test_class_attributes _ =
~expected_attribute:(
create_expected_attribute
"baz"
("typing.Callable('Attributes.baz')[[Named(self, $unknown), Named(x, str)], str]" ^
"[[Named(self, $unknown), Named(x, int)], int]"))
("typing.Callable('Attributes.baz')[[Named(self, $unknown), Named(x, int)], int]" ^
"[[Named(self, $unknown), Named(x, str)], str]"))


let test_fallback_attribute _ =
Expand Down
31 changes: 15 additions & 16 deletions analysis/test/environmentTest.ml
Original file line number Diff line number Diff line change
Expand Up @@ -567,50 +567,49 @@ let test_register_functions _ =
assert_is_none (Handler.function_definitions (access ["nested_in_function"]));
assert_is_none (Handler.function_definitions (access ["Class.property_method"]));
let assert_global access expected =
let assert_global ?expected ?expected_stubs access =
let actual =
Access.create access
|> Handler.globals
>>| Node.value
>>| Annotation.annotation
in
let expected =
expected
>>| parse_single_expression
>>| Type.create ~aliases:(fun _ -> None)
match expected, expected_stubs with
| None, None -> None
| _ -> Some (parse_callable_with_stubs expected expected_stubs)
in
assert_equal
~printer:(function | Some annotation -> Type.show annotation | _ -> "None")
~cmp:(Option.equal Type.equal)
expected
actual
in
assert_global "function" (Some "typing.Callable('function')[[], int]");
assert_global "function" ~expected:"typing.Callable('function')[[], int]";
assert_global
"function_with_arguments"
(Some "typing.Callable('function_with_arguments')[[Named(i, int)], None]");
~expected:"typing.Callable('function_with_arguments')[[Named(i, int)], None]";
assert_global
"Class.__init__"
(Some "typing.Callable('Class.__init__')[[Named(self, $unknown)], None]");
~expected:"typing.Callable('Class.__init__')[[Named(self, $unknown)], None]";
assert_global
"Class.method"
(Some "typing.Callable('Class.method')[[Named(self, $unknown), Named(i, int)], int]");
~expected:"typing.Callable('Class.method')[[Named(self, $unknown), Named(i, int)], int]";
assert_global
"Class.Nested.nested_class_method"
(Some "typing.Callable('Class.Nested.nested_class_method')[[Named(self, $unknown)], str]");
~expected:"typing.Callable('Class.Nested.nested_class_method')[[Named(self, $unknown)], str]";
assert_global
"overloaded"
(Some
("typing.Callable('overloaded')" ^
"[[Named(i, str)], None][[Named(i, float)], None][[Named(i, int)], None]"));
~expected:"typing.Callable('overloaded')[[Named(i, str)], None][[Named(i, float)], None]"
~expected_stubs:"typing.Callable('overloaded')[[Named(i, int)], None]";
assert_global
"ClassWithOverloadedConstructor.__init__"
(Some
("typing.Callable('ClassWithOverloadedConstructor.__init__')" ^
"[[Named(self, $unknown), Named(i, int)], None]" ^
"[[Named(self, $unknown), Named(s, str)], None]"))
~expected:("typing.Callable('ClassWithOverloadedConstructor.__init__')" ^
"[[Named(self, $unknown), Named(i, int)], None]")
~expected_stubs:("typing.Callable('ClassWithOverloadedConstructor.__init__')" ^
"[[Named(self, $unknown), Named(s, str)], None]")
let test_populate _ =
Expand Down
39 changes: 39 additions & 0 deletions analysis/test/typeCheckTest.ml
Original file line number Diff line number Diff line change
Expand Up @@ -1432,6 +1432,45 @@ let test_check _ =
@overload
def derp(self, x: str) -> str:
pass
def derp(self, x: typing.Union[int, str]) -> typing.Union[int, str]:
if isinstance(x, int):
return 0
else:
return ""

def herp(x: Foo) -> int:
return x.derp(5)
|}
[];

(* Technically invalid; all @overload stubs must be followed by implementation *)
assert_type_errors
{|
class Foo:
@overload
def derp(self, x: int) -> int:
pass
@overload
def derp(self, x: str) -> str:
pass

def herp(x: Foo) -> int:
return x.derp(5)
|}
[];

(* Technically invalid; @overload stubs must comprehensively cover implementation *)
assert_type_errors
{|
class Foo:
@overload
def derp(self, x: int) -> int:
pass
def derp(self, x: typing.Union[int, str]) -> typing.Union[int, str]:
if isinstance(x, int):
return 0
else:
return ""

def herp(x: Foo) -> int:
return x.derp(5)
Expand Down
13 changes: 9 additions & 4 deletions analysis/test/typeTest.ml
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,10 @@ let test_create _ =
assert_create "typing.Union[int, typing.Optional[$bottom], str, typing.Tuple[int, str]]"
(Type.optional
(Type.union [
Type.integer;
Type.string;
Type.tuple [Type.integer; Type.string];
]
Type.integer;
Type.string;
Type.tuple [Type.integer; Type.string];
]
));

(* Nested renaming. *)
Expand Down Expand Up @@ -188,6 +188,7 @@ let test_create _ =
(Type.Callable {
kind = Type.Callable.Named (Access.create "name");
overloads = [ { annotation = Type.integer; parameters = Undefined }];
overload_stubs = [];
implicit = Type.Callable.Function;
});
assert_create
Expand All @@ -207,6 +208,7 @@ let test_create _ =
];
};
];
overload_stubs = [];
implicit = Type.Callable.Function;
});
assert_create
Expand Down Expand Up @@ -236,6 +238,7 @@ let test_create _ =
];
};
];
overload_stubs = [];
implicit = Type.Callable.Function;
});
assert_create
Expand All @@ -260,6 +263,7 @@ let test_create _ =
];
};
];
overload_stubs = [];
implicit = Type.Callable.Function;
});
assert_create
Expand All @@ -278,6 +282,7 @@ let test_create _ =
];
};
];
overload_stubs = [];
implicit = Type.Callable.Function;
});
assert_create "typing.Callable[int]" (Type.callable ~annotation:Type.Top ())
Expand Down
Loading

0 comments on commit 27250ce

Please sign in to comment.