Skip to content

Commit

Permalink
Fix ParamSpec bug where we do not propagate type vars in typecheck
Browse files Browse the repository at this point in the history
Summary:
I'm going to try migrating some of IG's code. In the process, I found a bug where we do not properly propagate type vars in defines.

Specifically, we need to collect the type variables from PEP695 in the typechecker in various places, and there was a place where did not do that. If we do not collect them, the parser will return "unknown" giving incorrect typechecking results.

I added a legacy example as well to show that they both have the correct behavior and are matching (modulo qualification).

Note: the bug I got on IG was actually just "invalid syntax". Here's the diff with the error msg D65066009. This is strange though, since that sound like a parsing error and we should not be getting those.

Reviewed By: grievejia

Differential Revision: D65078436

fbshipit-source-id: b8e297ba24751bea85f0ab173775a8bb1b560a19
  • Loading branch information
migeed-z authored and facebook-github-bot committed Oct 28, 2024
1 parent 43ba1fe commit 7661930
Show file tree
Hide file tree
Showing 2 changed files with 58 additions and 6 deletions.
35 changes: 33 additions & 2 deletions source/analysis/test/integration/typeVariableTest.ml
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,6 @@ let test_type_variable_scoping =

|}
[];
(* TODO migeedz: Why do we need to express the domain of Callable as a list for this to
typecheck? *)
labeled_test_case __FUNCTION__ __LINE__
@@ assert_type_errors
{|
Expand All @@ -68,6 +66,39 @@ let test_type_variable_scoping =
...
|}
[];
labeled_test_case __FUNCTION__ __LINE__
@@ assert_type_errors
{|
from typing import Callable, Awaitable

def outer[**TParams, TReturn](
inner: Callable[TParams, Awaitable[TReturn]],
) -> Callable[TParams, Awaitable[TReturn]]:
async def _func(
*args: TParams.args, **kwargs: TParams.kwargs
) -> TReturn:
return await inner(*args, **kwargs)
return _func
|}
[];
labeled_test_case __FUNCTION__ __LINE__
@@ assert_type_errors
{|
from typing import Callable, Awaitable, ParamSpec, TypeVar
TParams = ParamSpec("TParams")
TReturn = TypeVar("TReturn")
def outer(
inner: Callable[TParams, Awaitable[TReturn]],
) -> Callable[TParams, Awaitable[TReturn]]:
async def _func(
*args: TParams.args, **kwargs: TParams.kwargs
) -> TReturn:
return await inner(*args, **kwargs)
return _func
|}
[];
(* PEP695 generic methods from non-generic classes *)
labeled_test_case __FUNCTION__ __LINE__
@@ assert_type_errors
Expand Down
29 changes: 25 additions & 4 deletions source/analysis/typeCheck.ml
Original file line number Diff line number Diff line change
Expand Up @@ -481,7 +481,6 @@ module State (Context : Context) = struct

let get_type_params_as_variables type_params global_resolution =
let create_type = GlobalResolution.parse_annotation global_resolution in
(* TODO migeedz: why does parse_annotation return Top for types of the form A[int]? . *)
let validate_bound bound =
match bound.Node.value with
| Expression.Tuple elements ->
Expand Down Expand Up @@ -6141,6 +6140,15 @@ module State (Context : Context) = struct
in
Value resolution, validate_return expression ~resolution ~errors ~actual ~is_implicit
| Define { signature = { Define.Signature.name; parent; type_params; _ } as signature; _ } ->
let type_params, type_params_errors =
get_type_params_as_variables type_params global_resolution
in
let resolution =
type_params
|> List.fold ~init:resolution ~f:(fun resolution variable ->
Resolution.add_type_variable resolution ~variable)
in

let resolution =
match parent with
| NestingContext.Function _ ->
Expand All @@ -6152,7 +6160,6 @@ module State (Context : Context) = struct
Resolution.new_local resolution ~reference:name ~type_info:annotation
| _ -> resolution
in
let _, type_params_errors = get_type_params_as_variables type_params global_resolution in
Value resolution, type_params_errors
| Import { Import.from; imports } ->
let get_export_kind = function
Expand Down Expand Up @@ -6990,7 +6997,15 @@ module State (Context : Context) = struct
|> add_typeguard_error return_annotation return_type
in
let add_capture_annotations ~outer_scope_type_variables resolution errors =
let process_signature ({ Define.Signature.parent; _ } as signature) =
let process_signature ({ Define.Signature.parent; type_params; _ } as signature) =
let type_params, _ = get_type_params_as_variables type_params global_resolution in

let resolution =
type_params
|> List.fold ~init:resolution ~f:(fun resolution variable ->
Resolution.add_type_variable resolution ~variable)
in

match parent with
| NestingContext.Function _ ->
type_of_signature ~module_name:Context.qualifier ~resolution signature
Expand Down Expand Up @@ -8060,7 +8075,12 @@ module State (Context : Context) = struct
>>| List.map ~f:extract
|> Option.value ~default:[]
in
let type_variables_of_define signature_of_nesting_function =
let type_variables_of_define
({ Define.Signature.type_params; _ } as signature_of_nesting_function)
=
let local_scope_function_type_params, _ =
get_type_params_as_variables type_params global_resolution
in
let parser = GlobalResolution.nonvalidating_annotation_parser global_resolution in
let generic_parameters_as_variables =
GlobalResolution.generic_parameters_as_variables global_resolution
Expand All @@ -8074,6 +8094,7 @@ module State (Context : Context) = struct
|> Type.Variable.all_free_variables
|> List.dedup_and_sort ~compare:Type.Variable.compare
in
let define_variables = define_variables @ local_scope_function_type_params in
let containing_class_variables =
(* PEP484 specifies that scope of the type variables of the outer class doesn't cover the
inner one. We are able to inspect only 1 level of nesting class as a result. *)
Expand Down

0 comments on commit 7661930

Please sign in to comment.