Skip to content

Commit

Permalink
Do not use infer_variance with covariant and contravariant flags
Browse files Browse the repository at this point in the history
Summary:
We should not use infer_variance when we've already defined covariance or contravariance flags.

We could not catch this before because the variable definitions are not expressive enough.

In this diff, we fix this issue by extending variable declarations to include the infer_variance flag, as existing variable declarations erased that information. Then when we reach typecheck, we can check the flag, along with the variance information we parsed and raise the right error message.

This diff includes adding the new error message defintion and a unit test.

We can now see the conformance improvement.

Reviewed By: stroxler

Differential Revision: D65041792

fbshipit-source-id: 4b5a1e391b348f8fb786140fd3882aa1289a7a8c
  • Loading branch information
migeed-z authored and facebook-github-bot committed Oct 28, 2024
1 parent 4570f27 commit f6f2a48
Show file tree
Hide file tree
Showing 7 changed files with 64 additions and 5 deletions.
8 changes: 8 additions & 0 deletions source/analysis/analysisError.ml
Original file line number Diff line number Diff line change
Expand Up @@ -788,6 +788,7 @@ and kind =
parameter: type_parameter_name_and_variance;
origin: type_variance_origin;
}
| InvalidVarianceDefinition
| InvalidInheritance of invalid_inheritance
| InvalidOverride of {
parent: Identifier.t;
Expand Down Expand Up @@ -997,6 +998,7 @@ let code_of_kind = function
| NamedTupleMissingDefault -> 74
| InvalidTypeVariableConstraint _ -> 75
| AwaitOutsideAsyncDef -> 76
| InvalidVarianceDefinition -> 77
| ParserFailure _ -> 404
(* Additional errors. *)
| UnawaitedAwaitable _ -> 1001
Expand Down Expand Up @@ -1041,6 +1043,7 @@ let name_of_kind = function
| InvalidTypeParameters _ -> "Invalid type parameters"
| InvalidTypeVariable _ -> "Invalid type variable"
| InvalidTypeVariance _ -> "Invalid type variance"
| InvalidVarianceDefinition -> "Invalid variance definition"
| InvalidInheritance _ -> "Invalid inheritance"
| InvalidOverride _ -> "Invalid override"
| InvalidPositionalOnlyParameter -> "Invalid positional-only parameter"
Expand Down Expand Up @@ -2165,6 +2168,8 @@ let rec messages ~concise ~signature location kind =
base_parameter
in
[formatted; "See `https://pyre-check.org/docs/errors#35-invalid-type-variance` for details."]
| InvalidVarianceDefinition ->
[Format.asprintf "Cannot use infer_variance with predefined variance."]
| InvalidInheritance invalid_inheritance -> (
match invalid_inheritance with
| ProtocolBaseClass ->
Expand Down Expand Up @@ -3383,6 +3388,7 @@ let due_to_analysis_limitations { kind; _ } =
| InvalidTypeParameters _
| InvalidTypeVariable _
| InvalidTypeVariance _
| InvalidVarianceDefinition
| InvalidInheritance _
| InvalidOverride _
| InvalidPositionalOnlyParameter
Expand Down Expand Up @@ -3929,6 +3935,7 @@ let join ~resolution left right =
| InvalidTypeParameters _, _
| InvalidTypeVariable _, _
| InvalidTypeVariance _, _
| InvalidVarianceDefinition, _
| InvalidInheritance _, _
| InvalidOverride _, _
| InvalidPositionalOnlyParameter, _
Expand Down Expand Up @@ -4466,6 +4473,7 @@ let dequalify
origin = dequalify_type_variance_origin origin;
}
| InvalidInheritance name -> InvalidInheritance (dequalify_invalid_inheritance name)
| InvalidVarianceDefinition -> InvalidVarianceDefinition
| InvalidOverride { parent; decorator } ->
InvalidOverride { parent = dequalify_identifier parent; decorator }
| InvalidPositionalOnlyParameter -> InvalidPositionalOnlyParameter
Expand Down
1 change: 1 addition & 0 deletions source/analysis/analysisError.mli
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,7 @@ and kind =
parameter: type_parameter_name_and_variance;
origin: type_variance_origin;
}
| InvalidVarianceDefinition
| InvalidInheritance of invalid_inheritance
| InvalidOverride of {
parent: Identifier.t;
Expand Down
14 changes: 14 additions & 0 deletions source/analysis/test/integration/typeVariableTest.ml
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,20 @@ let test_type_variable_scoping =
`ShouldBeInvariant1[int]` but is used as type `ShouldBeInvariant1[float]`.";
];
labeled_test_case __FUNCTION__ __LINE__
@@ assert_type_errors
{|
from typing import *
S1 = TypeVar("S1", covariant=True, infer_variance=True) # E: cannot use covariant with infer_variance
S2 = TypeVar("S2", contravariant=True, infer_variance=True) # E: cannot use contravariant with infer_variance
|}
[
"Invalid variance definition [77]: Cannot use infer_variance with predefined variance.";
"Invalid variance definition [77]: Cannot use infer_variance with predefined variance.";
];
labeled_test_case __FUNCTION__ __LINE__
@@ assert_type_errors
{|
from typing import Sequence
Expand Down
13 changes: 12 additions & 1 deletion source/analysis/test/typeTest.ml
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ let type_var_declaration_and_variable
?(variance = Type.Record.PreInferenceVariance.P_Invariant)
name
=
( Type.Variable.Declaration.DTypeVar { name; constraints = declaration_constraints; variance },
( Type.Variable.Declaration.DTypeVar
{ name; constraints = declaration_constraints; variance; infer_variance = false },
Type.Variable.TypeVar.create name ~constraints:type_constraints )


Expand Down Expand Up @@ -2697,6 +2698,7 @@ let test_parse_type_variable_declarations _ =
name = "target";
constraints = Unconstrained;
variance = Type.Record.PreInferenceVariance.P_Invariant;
infer_variance = false;
});
assert_parses_declaration
Expand All @@ -2706,6 +2708,7 @@ let test_parse_type_variable_declarations _ =
name = "target";
constraints = Unconstrained;
variance = Type.Record.PreInferenceVariance.P_Covariant;
infer_variance = false;
});
assert_parses_declaration
Expand All @@ -2715,6 +2718,7 @@ let test_parse_type_variable_declarations _ =
name = "target";
constraints = Unconstrained;
variance = Type.Record.PreInferenceVariance.P_Invariant;
infer_variance = false;
});
assert_parses_declaration
Expand All @@ -2724,6 +2728,7 @@ let test_parse_type_variable_declarations _ =
name = "target";
constraints = Unconstrained;
variance = Type.Record.PreInferenceVariance.P_Contravariant;
infer_variance = false;
});
assert_parses_declaration
Expand All @@ -2733,6 +2738,7 @@ let test_parse_type_variable_declarations _ =
name = "target";
constraints = Unconstrained;
variance = Type.Record.PreInferenceVariance.P_Invariant;
infer_variance = false;
});
assert_parses_declaration_with_bounds
"typing.TypeVar('_T', int)"
Expand All @@ -2741,6 +2747,7 @@ let test_parse_type_variable_declarations _ =
name = "target";
constraints = Type.Record.TypeVarConstraints.Explicit [Type.expression Type.integer];
variance = Type.Record.PreInferenceVariance.P_Invariant;
infer_variance = false;
});
assert_parses_declaration_with_bounds
Expand All @@ -2751,6 +2758,7 @@ let test_parse_type_variable_declarations _ =
constraints =
Type.Record.TypeVarConstraints.Bound (Type.expression (Type.Primitive "\"C\""));
variance = Type.Record.PreInferenceVariance.P_Invariant;
infer_variance = false;
});
assert_parses_declaration_with_bounds
Expand All @@ -2762,6 +2770,7 @@ let test_parse_type_variable_declarations _ =
Type.Record.TypeVarConstraints.Explicit
[Type.expression (Type.Primitive "\"C\""); Type.expression (Type.Primitive "X")];
variance = Type.Record.PreInferenceVariance.P_Invariant;
infer_variance = false;
});
assert_parses_declaration_with_bounds
Expand All @@ -2771,6 +2780,7 @@ let test_parse_type_variable_declarations _ =
name = "target";
constraints = Type.Record.TypeVarConstraints.Explicit [Type.expression Type.integer];
variance = Type.Record.PreInferenceVariance.P_Invariant;
infer_variance = false;
});
assert_parses_declaration_with_bounds
Expand All @@ -2782,6 +2792,7 @@ let test_parse_type_variable_declarations _ =
Type.Record.TypeVarConstraints.Bound
(Type.expression (Type.Primitive "\"typing.Callable\""));
variance = Type.Record.PreInferenceVariance.P_Invariant;
infer_variance = false;
});
assert_declaration_does_not_parse "typing.TypeVarTuple('Ts', covariant=True)";
Expand Down
18 changes: 16 additions & 2 deletions source/analysis/type.ml
Original file line number Diff line number Diff line change
Expand Up @@ -3102,6 +3102,7 @@ module Variable = struct
name: Identifier.t;
constraints: Expression.t Record.TypeVarConstraints.t;
variance: Record.PreInferenceVariance.t;
infer_variance: bool;
}
| DTypeVarTuple of { name: Identifier.t }
| DParamSpec of { name: Identifier.t }
Expand Down Expand Up @@ -3160,8 +3161,20 @@ module Variable = struct
List.find_map arguments ~f:variance_definition
|> Option.value ~default:Record.PreInferenceVariance.P_Invariant
in
let infer_variance =
let variance_definition = function
| {
Call.Argument.name = Some { Node.value = name; _ };
value = { Node.value = Constant Constant.True; _ };
}
when String.equal (Identifier.sanitized name) "infer_variance" ->
true
| _ -> false
in
List.exists arguments ~f:variance_definition
in

Some (DTypeVar { name = Reference.show target; constraints; variance })
Some (DTypeVar { name = Reference.show target; constraints; variance; infer_variance })
| {
Node.value =
Expression.Call
Expand All @@ -3179,6 +3192,7 @@ module Variable = struct
name = Reference.show target;
constraints = LiteralIntegers;
variance = Record.PreInferenceVariance.P_Invariant;
infer_variance = false;
})
| {
Node.value =
Expand Down Expand Up @@ -3604,7 +3618,7 @@ module GenericParameter = struct

let of_declaration declaration ~create_type =
match declaration with
| Variable.Declaration.DTypeVar { name; constraints; variance } ->
| Variable.Declaration.DTypeVar { name; constraints; variance; _ } ->
let constraints =
match constraints with
| Bound expression -> Record.TypeVarConstraints.Bound (create_type expression)
Expand Down
1 change: 1 addition & 0 deletions source/analysis/type.mli
Original file line number Diff line number Diff line change
Expand Up @@ -841,6 +841,7 @@ module Variable : sig
name: Identifier.t;
constraints: Expression.t Record.TypeVarConstraints.t;
variance: Record.PreInferenceVariance.t;
infer_variance: bool;
}
| DTypeVarTuple of { name: Identifier.t }
| DParamSpec of { name: Identifier.t }
Expand Down
14 changes: 12 additions & 2 deletions source/analysis/typeCheck.ml
Original file line number Diff line number Diff line change
Expand Up @@ -4643,13 +4643,22 @@ module State (Context : Context) = struct

and forward_variable_alias_definition ~resolution ~location ~errors ~target ~value ~reference =
let global_resolution = Resolution.global_resolution resolution in

(* try to parse as a variable declaration and raise the appropriate errors for variables if we
are successful *)
let parse_as_declaration = Type.Variable.Declaration.parse value ~target:reference in

match parse_as_declaration with
| Some parsed ->
(* if infer_variance is true but variance is not undefined, we raise an error *)
let errors_from_variance_declarations =
match parsed with
| Type.Variable.Declaration.DTypeVar { variance; infer_variance = true; _ }
when not
(Type.Record.PreInferenceVariance.equal
variance
Type.Record.PreInferenceVariance.P_Undefined) ->
emit_error ~errors ~location ~kind:Error.InvalidVarianceDefinition
| _ -> []
in
let create_type =
GlobalResolution.parse_annotation ~validation:NoValidation global_resolution
in
Expand Down Expand Up @@ -4679,6 +4688,7 @@ module State (Context : Context) = struct
errors
| _ -> errors
in
let errors = errors_from_variance_declarations @ errors in
Value resolution, errors
| _ -> Value resolution, errors

Expand Down

0 comments on commit f6f2a48

Please sign in to comment.