Skip to content

Commit

Permalink
Rename Type.Parameter to Type.Argument, part 2
Browse files Browse the repository at this point in the history
Summary:
**This diff**

This diff is the second half of renaming `Type.Parameter` to `Type.Argument`
(see below for stack description); at this point the type is renamed but I'm
trying to rename all functions and variables that are really talking about
type *arguments* rather than *parameters*.

It is tricker for two reasons:
- lots of code talking about "parameters" is referring to callable parameters,
  which still exist in `Type.t`
- the concept of a type parameter *does* have meaning, and some downstream use
  cases (starting with `ClassHierarchy`) really do mean "type parameters" in
  their name

As a result, the rename is best-effort and I tried *not* to rename anything that's
actually about either callable parameters or type constructor type parameters.

**This stack of 2 diffs**

The `Type.t` data in OCaml primarily represents a (possibly propagated through
analysis) type *annotation*. It does not represent in any meaningful sense a
type *constructor*, which is a concept that has no direct representation in
Pyre but is basically mapped to a "class" in the current architecture.

In terms of modern vocabulary, typically we talk about "parameters" versus
"arguments"; apparently there's also a mostly older vocabulary that calls these
"formal parameters" vs "parameters", but "paramters" / "arguments" is already
the vocabulary that Pyre uses for function calls and type instantiations have
the same distinction.

But because a `Type.t` loosely corresponds to an *annotation*, the values
inside of a `Type.Parametric` form are not actually "parameters", they are
"arguments".

I have two motivations for making this change now:
- We're starting to have discussions about the architecture of how Pyre
  represents types, and it's confusing to distinguish argument vs parameter
  in discussions (which is important, if you cannot name a concept then you
  can't really have a conversation about it) and yet have the code use
  the wrong name. If the fix is easy, why not fix the code?
- In order to improve our handling of type constructors (for now, generic
  classes) when supporting PEP 695, we will probably need a cleaner notion of
  type *parameters*, which means I want that name to be available so that
  we have the option of using it in a refactor.
  - for existing generics, the type *parameters* will be extracted from the
    type *arguments* of the "generic base class"
  - but for PEP 695 classes this will not be the case, the type parameters
    will be explicitly marked as they are in most other languages. This is part
    of why confusing the two concepts is such a problem for 695.

Reviewed By: migeed-z

Differential Revision: D61548861

fbshipit-source-id: a0f655c0791327ca6686e4a41ee18940575f03c5
  • Loading branch information
stroxler authored and facebook-github-bot committed Aug 20, 2024
1 parent bc8270b commit 357943f
Show file tree
Hide file tree
Showing 37 changed files with 705 additions and 725 deletions.
6 changes: 3 additions & 3 deletions source/analysis/analysisError.ml
Original file line number Diff line number Diff line change
Expand Up @@ -1314,8 +1314,8 @@ let rec messages ~concise ~signature location kind =
let expected_message = Format.asprintf "`%a`" pp_type expected in
let actual_message = Format.asprintf "`%a`" pp_type actual in
match expected, actual with
| ( Type.Parametric { parameters = expected_members; name = "Tuple" },
Type.Parametric { parameters = actual_members; name = "Tuple" } ) ->
| ( Type.Parametric { arguments = expected_members; name = "Tuple" },
Type.Parametric { arguments = actual_members; name = "Tuple" } ) ->
let expected_length = List.length expected_members in
let actual_length = List.length actual_members in
if expected_length = actual_length then
Expand Down Expand Up @@ -2893,7 +2893,7 @@ let rec messages ~concise ~signature location kind =
| Parametric
{
name = "BoundMethod";
parameters = [Single (Callable { kind; _ }); Single _];
arguments = [Single (Callable { kind; _ }); Single _];
} );
_;
} -> (
Expand Down
5 changes: 2 additions & 3 deletions source/analysis/annotatedCallable.ml
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,7 @@ let return_annotation_without_applying_decorators
Type.coroutine [Single Type.Any; Single Type.Any; Single annotation]
else if Define.Signature.is_coroutine signature then
match annotation with
| Type.Parametric { name = "typing.Generator"; parameters = [_; _; Single return_annotation] }
->
| Type.Parametric { name = "typing.Generator"; arguments = [_; _; Single return_annotation] } ->
Type.awaitable return_annotation
| _ -> Type.Top
else
Expand Down Expand Up @@ -122,7 +121,7 @@ let create_overload_without_applying_decorators
let parent_type =
let class_annotation = Reference.show parent in
generic_parameters_as_variables class_annotation
>>| List.map ~f:Type.Variable.to_parameter
>>| List.map ~f:Type.Variable.to_argument
>>| Type.parametric class_annotation
|> Option.value ~default:(Type.Primitive class_annotation)
in
Expand Down
113 changes: 56 additions & 57 deletions source/analysis/attributeResolution.ml

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions source/analysis/attributeResolution.mli
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ module AttributeReadOnly : sig

val full_order : ?dependency:DependencyKey.registered -> t -> TypeOrder.order

val check_invalid_type_parameters
val check_invalid_type_arguments
: t ->
?dependency:DependencyKey.registered ->
Type.t ->
Expand Down Expand Up @@ -274,7 +274,7 @@ module AttributeReadOnly : sig
: t ->
?dependency:DependencyKey.registered ->
target:Type.Primitive.t ->
?parameters:Type.Argument.t list ->
?arguments:Type.Argument.t list ->
instantiated:Type.t ->
unit ->
TypeConstraints.Solution.t
Expand Down
2 changes: 1 addition & 1 deletion source/analysis/callgraph.ml
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ module DefaultBuilder : Builder = struct
| Type.Callable.Named direct_target when not (is_protocol ()) ->
let class_name =
if Type.is_meta annotation then
Type.single_parameter annotation
Type.single_argument annotation
else
annotation
in
Expand Down
57 changes: 32 additions & 25 deletions source/analysis/classHierarchy.ml
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,14 @@
(* The classHierarchy module contains an implementation of the C3 superclass linearization
algorithm. This algorithm (adopted in Python 2.3) is a way to determine a method resolution order
(MRO) which maintains certain properties. More can be found on
[wikipedia](https://en.wikipedia.org/wiki/C3_linearization) *)
[wikipedia](https://en.wikipedia.org/wiki/C3_linearization)
Note that there are two blocks of code in this module that convert type *arguments* to type
*parameters*. This is because the pre-PEP-695 syntax for generic classes indicates the type
*parameters* of a class as *arguments* to its generic base class. That is the only reason such
coercion makes sense, in general the two concepts are not interchangeable (parameters are the
placeholders on a class treated as a type constructor, arguments are the actual types provided in
a concrete annotation that specializes the generic). *)
open Core
open Ast
open Pyre
Expand Down Expand Up @@ -36,7 +43,7 @@ end
module Target = struct
type t = {
target: Identifier.t;
parameters: Type.Argument.t list;
arguments: Type.Argument.t list;
}
[@@deriving compare, sexp, show]

Expand Down Expand Up @@ -265,13 +272,13 @@ let generic_parameters_as_variables ?(default = None) (module Handler : Handler)
parents_and_generic_of_target (module Handler) primitive_name
>>= List.find ~f:(fun { Target.target; _ } ->
[%compare.equal: Identifier.t] target generic_primitive)
>>| (fun { Target.parameters; _ } -> parameters_to_variables parameters)
>>| (fun { Target.arguments; _ } -> parameters_to_variables arguments)
|> Option.value ~default


let get_generic_parameters ~generic_primitive edges =
let generic_parameters { Target.target; parameters } =
Option.some_if ([%compare.equal: Identifier.t] generic_primitive target) parameters
let generic_parameters { Target.target; arguments } =
Option.some_if ([%compare.equal: Identifier.t] generic_primitive target) arguments
in
List.find_map ~f:generic_parameters edges

Expand All @@ -283,7 +290,7 @@ let instantiate_successors_parameters ((module Handler : Handler) as handler) ~s
let to_any = function
| Type.Variable.TypeVarVariable _ -> [Type.Argument.Single Type.Any]
| ParamSpecVariable _ -> [CallableParameters Undefined]
| TypeVarTupleVariable _ -> Type.OrderedTypes.to_parameters Type.Variable.TypeVarTuple.any
| TypeVarTupleVariable _ -> Type.OrderedTypes.to_arguments Type.Variable.TypeVarTuple.any
in
target
|> parents_and_generic_of_target (module Handler)
Expand All @@ -294,26 +301,26 @@ let instantiate_successors_parameters ((module Handler : Handler) as handler) ~s
let split =
match Type.split source with
| Primitive primitive, _ when not (Handler.contains primitive) -> None
| Primitive "tuple", [Type.Argument.Single parameter] ->
Some ("tuple", [Type.Argument.Single (Type.weaken_literals parameter)])
| Primitive primitive, parameters -> Some (primitive, parameters)
| Primitive "tuple", [Type.Argument.Single argument] ->
Some ("tuple", [Type.Argument.Single (Type.weaken_literals argument)])
| Primitive primitive, arguments -> Some (primitive, arguments)
| _ ->
(* We can only propagate from those that actually split off a primitive *)
None
in
let handle_split (primitive, parameters) =
let handle_split (primitive, arguments) =
let worklist = Queue.create () in
Queue.enqueue worklist { Target.target = primitive; parameters };
Queue.enqueue worklist { Target.target = primitive; arguments };
let rec iterate worklist =
match Queue.dequeue worklist with
| Some { Target.target = target_index; parameters } ->
| Some { Target.target = target_index; arguments } ->
let instantiated_successors =
(* If a node on the graph has Generic[_T1, _T2, ...] as a supertype and has concrete
parameters, all occurrences of _T1, _T2, etc. in other supertypes need to be
replaced with the concrete parameter corresponding to the type variable. This
function takes a target with concrete parameters and its supertypes, and
arguments, all occurrences of _T1, _T2, etc. in other supertypes need to be
replaced with the concrete argument corresponding to the type variable. This
function takes a target with concrete arguments and its supertypes, and
instantiates the supertypes accordingly. *)
let get_instantiated_successors ~generic_primitive ~parameters successors =
let get_instantiated_successors ~generic_primitive ~arguments successors =
let variables =
get_generic_parameters successors ~generic_primitive
>>= parameters_to_variables
Expand All @@ -328,11 +335,11 @@ let instantiate_successors_parameters ((module Handler : Handler) as handler) ~s
| TypeVarTupleVariable variadic ->
Type.Variable.TypeVarTuplePair (variadic, Type.Variable.TypeVarTuple.any)
in
Type.Variable.zip_variables_with_parameters ~parameters variables
Type.Variable.zip_variables_with_arguments ~arguments variables
|> Option.value ~default:(List.map ~f:to_any variables)
|> TypeConstraints.Solution.create
in
let instantiate_parameters { Target.target; parameters } =
let instantiate_parameters { Target.target; arguments } =
let instantiate = function
| Type.Argument.Single single ->
[
Expand All @@ -347,23 +354,23 @@ let instantiate_successors_parameters ((module Handler : Handler) as handler) ~s
parameters);
]
| Unpacked unpackable ->
Type.OrderedTypes.to_parameters
Type.OrderedTypes.to_arguments
(TypeConstraints.Solution.instantiate_ordered_types
replacement
(Concatenation
(Type.OrderedTypes.Concatenation.create_from_unpackable
unpackable)))
in
{ Target.target; parameters = List.concat_map parameters ~f:instantiate }
{ Target.target; arguments = List.concat_map arguments ~f:instantiate }
in
List.map successors ~f:instantiate_parameters
in
parents_and_generic_of_target (module Handler) target_index
>>| get_instantiated_successors ~generic_primitive ~parameters
>>| get_instantiated_successors ~generic_primitive ~arguments
in
if [%compare.equal: Identifier.t] target_index target then
match target with
| "typing.Callable" -> Some parameters
| "typing.Callable" -> Some arguments
| _ -> instantiated_successors >>= get_generic_parameters ~generic_primitive
else (
instantiated_successors >>| List.iter ~f:(Queue.enqueue worklist) |> ignore;
Expand Down Expand Up @@ -421,10 +428,10 @@ let to_dot (module Handler : Handler) ~class_names =
let add_edges class_name =
parents_of (module Handler) class_name
>>| List.sort ~compare:Target.compare
>>| List.iter ~f:(fun { Target.target = successor; parameters } ->
>>| List.iter ~f:(fun { Target.target = successor; arguments } ->
Format.asprintf " %s -> %s" class_name successor |> Buffer.add_string buffer;
if not (List.is_empty parameters) then
Format.asprintf "[label=\"(%a)\"]" Type.Argument.pp_list parameters
if not (List.is_empty arguments) then
Format.asprintf "[label=\"(%a)\"]" Type.Argument.pp_list arguments
|> Buffer.add_string buffer;
Buffer.add_string buffer "\n")
|> ignore
Expand Down
2 changes: 1 addition & 1 deletion source/analysis/classHierarchy.mli
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ end
module Target : sig
type t = {
target: Ast.Identifier.t;
parameters: Type.Argument.t list;
arguments: Type.Argument.t list;
}
[@@deriving compare, sexp, show]

Expand Down
30 changes: 15 additions & 15 deletions source/analysis/classHierarchyEnvironment.ml
Original file line number Diff line number Diff line change
Expand Up @@ -52,25 +52,25 @@ module IncomingDataComputation = struct
in
List.concat_map ~f:Type.Variable.all_free_variables parsed_bases
|> deduplicate ~equal:Type.Variable.equal
|> List.map ~f:Type.Variable.to_parameter
|> List.map ~f:Type.Variable.to_argument


let compute_generic_base parsed_bases =
let is_generic base_type =
let primitive, _ = Type.split base_type in
Type.is_generic_primitive primitive
in
let extract_protocol_parameters base_type =
let extract_protocol_arguments base_type =
match base_type with
| Type.Parametric { name = "typing.Protocol"; parameters } -> Some parameters
| Type.Parametric { name = "typing.Protocol"; arguments } -> Some arguments
| _ -> None
in
match List.find ~f:is_generic parsed_bases with
| Some _ as generic_base -> generic_base
| None -> (
let create variables = Type.parametric "typing.Generic" variables in
match List.find_map parsed_bases ~f:extract_protocol_parameters with
| Some parameters -> Some (create parameters)
match List.find_map parsed_bases ~f:extract_protocol_arguments with
| Some arguments -> Some (create arguments)
| None ->
(* TODO:(T60673574) Ban propagating multiple type variables *)
let variables = find_propagated_type_variables parsed_bases in
Expand All @@ -97,7 +97,7 @@ module IncomingDataComputation = struct
match Node.value value with
| Expression.Subscript _
| Name _ -> (
let supertype, parameters =
let supertype, arguments =
parse_annotation_without_validating_type_parameters
~allow_untracked:true
value
Expand All @@ -112,8 +112,8 @@ module IncomingDataComputation = struct
| Type.Primitive primitive when not (class_exists primitive) ->
Log.log ~section:`Environment "Superclass annotation %a is missing" Type.pp supertype;
None
| Type.Primitive supertype -> Some (supertype, parameters)
| Type.Any -> Some ("typing.Any", parameters)
| Type.Primitive supertype -> Some (supertype, arguments)
| Type.Any -> Some ("typing.Any", arguments)
| _ -> None)
| _ -> None
in
Expand All @@ -132,32 +132,32 @@ module IncomingDataComputation = struct
handling. This behavior was established in PEP 560 and gets implemented in CPython via
`GenericAlias.__mro_entries__()`. See https://fburl.com/mro_in_pyre for more detailed
explanation. *)
let filter_shadowed_generic_bases name_and_parameters =
let filter_shadowed_generic_bases name_and_arguments =
let is_protocol =
List.exists name_and_parameters ~f:(fun (name, _) -> String.equal name "typing.Protocol")
List.exists name_and_arguments ~f:(fun (name, _) -> String.equal name "typing.Protocol")
in
let process_parent ((name, _) as current) rest =
match name with
| "typing.Generic" ->
(* TODO: type parameters of the `name` class is expected to be non-empty here because
(* TODO: type arguments of the `name` class is expected to be non-empty here because
Python forbids inheriting from `typing.Generic` directly. But we currently can't
check for that since we lack the setup to emit errors from this environment. *)
if is_protocol then
(* Hide `Generic` from MRO if the class also extends from `Protocol` *)
rest
else if List.exists rest ~f:(fun (_, parameters) -> not (List.is_empty parameters)) then
else if List.exists rest ~f:(fun (_, arguments) -> not (List.is_empty arguments)) then
(* Hide `Generic` from MRO if there exist other generic aliases down the base class
list *)
rest
else
current :: rest
| _ -> current :: rest
in
List.fold_right name_and_parameters ~init:[] ~f:process_parent
List.fold_right name_and_arguments ~init:[] ~f:process_parent
in
let is_not_primitive_cycle (parent, _) = not (String.equal name parent) in
let convert_to_targets x =
List.map ~f:(fun (name, parameters) -> { ClassHierarchy.Target.target = name; parameters }) x
List.map ~f:(fun (name, arguments) -> { ClassHierarchy.Target.target = name; arguments }) x
in
let deduplicate targets =
let deduplicate (visited, sofar) ({ ClassHierarchy.Target.target; _ } as edge) =
Expand Down Expand Up @@ -201,7 +201,7 @@ module IncomingDataComputation = struct
compute_generic_base parsed_bases
>>= fun base ->
extract_supertype (Type.expression base)
>>= fun (name, parameters) -> Some { ClassHierarchy.Target.target = name; parameters }
>>= fun (name, arguments) -> Some { ClassHierarchy.Target.target = name; arguments }
in
Some { ClassHierarchy.Edges.parents; generic_base }
end
Expand Down
Loading

0 comments on commit 357943f

Please sign in to comment.