Skip to content

Commit

Permalink
[flow][match] Simple check for match expression exhaustiveness based …
Browse files Browse the repository at this point in the history
…on refinements

Summary:
Adds a simple check for match expression exhaustiveness based on the argument being refined to `empty`.

In the future, once we have a more advanced analysis, this could serve as an optimization (if the refinements systems has already refined the argument to `empty` by the end of the `match`, we don't need to run the advanced analysis).

Changelog: [internal]

Reviewed By: SamChou19815

Differential Revision: D67261340

fbshipit-source-id: e22f918dc958f140dd36a104b6652f244d92b225
  • Loading branch information
gkz authored and facebook-github-bot committed Dec 17, 2024
1 parent 9dc295b commit f48b56d
Show file tree
Hide file tree
Showing 21 changed files with 287 additions and 243 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -7492,7 +7492,7 @@ let%expect_test "match_object_pattern" =
[%expect {|
[
(2, 1) to (2, 6) => {
(2, 1) to (2, 6): (`<match_root>`)
{refinement = Or (Not (And (object, Not (Null))), Not (SentinelR type)); writes = {refinement = Or (Or (Not (And (object, Not (Null))), Not (SentinelR type)), Not (PropExistsR (value))); writes = (2, 1) to (2, 6): (`<match_root>`)}}
};
(2, 8) to (2, 9) => {
Global x
Expand All @@ -7519,7 +7519,7 @@ let%expect_test "match_array_pattern" =
[%expect {|
[
(2, 1) to (2, 6) => {
(2, 1) to (2, 6): (`<match_root>`)
{refinement = Or (Not (And (isArray, array length === 1)), Not (SentinelR 0)); writes = {refinement = Or (Not (And (isArray, array length === 2)), Not (SentinelR 0)); writes = (2, 1) to (2, 6): (`<match_root>`)}}
};
(2, 8) to (2, 9) => {
Global x
Expand Down
10 changes: 9 additions & 1 deletion src/analysis/env_builder/find_providers.ml
Original file line number Diff line number Diff line change
Expand Up @@ -987,7 +987,15 @@ end = struct
expr

method! match_expression loc x =
let { Ast.Expression.Match.arg_internal; arg = _; cases = _; comments = _ } = x in
let {
Ast.Expression.Match.arg_internal;
arg = _;
cases = _;
match_keyword_loc = _;
comments = _;
} =
x
in
ignore
@@ this#pattern_identifier
~kind:Ast.Variable.Const
Expand Down
3 changes: 1 addition & 2 deletions src/analysis/env_builder/name_def.ml
Original file line number Diff line number Diff line change
Expand Up @@ -3122,8 +3122,7 @@ class def_finder ~autocomplete_hooks ~react_jsx env_info toplevel_scope =

method private visit_match_expression x =
let open Ast.Expression.Match in
let { arg; cases; arg_internal; comments = _ } = x in
ignore @@ this#expression arg;
let { arg; cases; arg_internal; match_keyword_loc = _; comments = _ } = x in
this#add_ordinary_binding
arg_internal
(mk_reason RMatchExpression arg_internal)
Expand Down
5 changes: 4 additions & 1 deletion src/analysis/env_builder/name_def_ordering.ml
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,9 @@ struct
super#yield loc yield

method! match_expression _ x =
let { Ast.Expression.Match.arg; cases; arg_internal; comments = _ } = x in
let { Ast.Expression.Match.arg; cases; arg_internal; match_keyword_loc; comments = _ } =
x
in
ignore @@ this#expression arg;
ignore
@@ this#pattern_identifier
Expand All @@ -286,6 +288,7 @@ struct
ignore @@ this#identifier (Flow_ast_utils.match_root_ident case_loc);
ignore @@ super#match_expression_case (case_loc, case)
);
ignore @@ this#identifier (Flow_ast_utils.match_root_ident match_keyword_loc);
x

(* In order to resolve a def containing a variable write, the
Expand Down
10 changes: 8 additions & 2 deletions src/analysis/env_builder/name_resolver.ml
Original file line number Diff line number Diff line change
Expand Up @@ -2913,7 +2913,7 @@ module Make (Context : C) (FlowAPIUtils : F with type cx = Context.t) :

method! match_expression match_loc x =
let open Flow_ast.Expression.Match in
let { arg; cases; arg_internal; comments = _ } = x in
let { arg; cases; arg_internal; match_keyword_loc; comments = _ } = x in
let match_root_ident = Flow_ast_utils.match_root_ident in
ignore @@ this#expression arg;
let env0 = this#env_snapshot in
Expand Down Expand Up @@ -2969,7 +2969,13 @@ module Make (Context : C) (FlowAPIUtils : F with type cx = Context.t) :
this#negate_new_refinements ()
))
()
))
);
let arg_out =
( match_keyword_loc,
Ast.Expression.Identifier (Flow_ast_utils.match_root_ident match_keyword_loc)
)
in
ignore @@ this#expression arg_out)
();
let completion_states = !completion_states |> List.rev in
this#reset_env env0;
Expand Down
2 changes: 2 additions & 0 deletions src/common/errors/error_codes.ml
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ type error_code =
| InvalidTempType
| LintSetting
| MalformedPackage
| MatchNotExhaustive
| MethodUnbinding
| MissingLocalAnnot
| MissingThisAnnot
Expand Down Expand Up @@ -319,6 +320,7 @@ let string_of_code : error_code -> string = function
| InvalidTempType -> "invalid-temp-type"
| LintSetting -> "lint-setting"
| MalformedPackage -> "malformed-package"
| MatchNotExhaustive -> "match-not-exhaustive"
| MethodUnbinding -> "method-unbinding"
| MissingLocalAnnot -> "missing-local-annot"
| MissingThisAnnot -> "missing-this-annot"
Expand Down
2 changes: 1 addition & 1 deletion src/parser/estree_translator.ml
Original file line number Diff line number Diff line change
Expand Up @@ -640,7 +640,7 @@ with type t = Impl.t = struct
| (loc, Class c) -> class_expression (loc, c)
| (loc, JSXElement element) -> jsx_element (loc, element)
| (loc, JSXFragment fragment) -> jsx_fragment (loc, fragment)
| (loc, Match { Match.arg; cases; comments; arg_internal = _ }) ->
| (loc, Match { Match.arg; cases; comments; arg_internal = _; match_keyword_loc = _ }) ->
node
?comments
"MatchExpression"
Expand Down
9 changes: 5 additions & 4 deletions src/parser/expression_parser.ml
Original file line number Diff line number Diff line change
Expand Up @@ -1337,16 +1337,16 @@ module Expression
let leading = Peek.comments env in
let start_loc = Peek.loc env in
(* Consume `match` as an identifier, in case it's a call expression. *)
let id = Parse.identifier env in
let ((id_loc, _) as id) = Parse.identifier env in
(* Allows trailing comma. *)
let args = arguments env in
(* `match (<expr>) {` *)
if (not (Peek.is_line_terminator env)) && Peek.token env = T_LCURLY then
let arg = Parser_common.reparse_arguments_as_match_argument env args in
Cover_expr (match_expression ~start_loc ~leading ~arg env)
Cover_expr (match_expression ~start_loc ~id_loc ~leading ~arg env)
else
(* It's actually a call expression of the form `match(...)` *)
let callee = (fst id, Expression.Identifier id) in
let callee = (id_loc, Expression.Identifier id) in
let (args_loc, _) = args in
let loc = Loc.btwn start_loc args_loc in
let comments = Flow_ast_utils.mk_comments_opt ~leading () in
Expand Down Expand Up @@ -1382,7 +1382,7 @@ module Expression

and primary env = as_expression env (primary_cover env)

and match_expression env ~start_loc ~leading ~arg =
and match_expression env ~start_loc ~id_loc ~leading ~arg =
let case env =
let leading = Peek.comments env in
let pattern = Parse.match_pattern env in
Expand Down Expand Up @@ -1422,6 +1422,7 @@ module Expression
Expression.Match.arg;
cases;
arg_internal = start_loc;
match_keyword_loc = id_loc;
comments = Flow_ast_utils.mk_comments_opt ~leading ~trailing ();
})
env
Expand Down
3 changes: 3 additions & 0 deletions src/parser/flow_ast.ml
Original file line number Diff line number Diff line change
Expand Up @@ -1758,6 +1758,9 @@ and Expression : sig
arg: ('M, 'T) Expression.t;
cases: ('M, 'T) Case.t list;
arg_internal: 'M;
(* The type here is used to store the resulting type after the patterns
refine the arg type. *)
match_keyword_loc: 'T;
comments: ('M, unit) Syntax.t option;
}
[@@deriving show]
Expand Down
4 changes: 2 additions & 2 deletions src/parser/flow_ast_mapper.ml
Original file line number Diff line number Diff line change
Expand Up @@ -2521,14 +2521,14 @@ class ['loc] mapper =

method match_expression _loc (expr : ('loc, 'loc) Ast.Expression.Match.t) =
let open Ast.Expression.Match in
let { arg; cases; arg_internal; comments } = expr in
let { arg; cases; arg_internal; match_keyword_loc; comments } = expr in
let arg' = this#expression arg in
let cases' = map_list this#match_expression_case cases in
let comments' = this#syntax_opt comments in
if arg == arg' && cases == cases' && comments == comments' then
expr
else
{ arg = arg'; cases = cases'; arg_internal; comments = comments' }
{ arg = arg'; cases = cases'; arg_internal; match_keyword_loc; comments = comments' }

method match_expression_case (case : ('loc, 'loc) Ast.Expression.Match.Case.t) =
let open Ast.Expression.Match.Case in
Expand Down
11 changes: 9 additions & 2 deletions src/parser_utils/flow_polymorphic_ast_mapper.ml
Original file line number Diff line number Diff line change
Expand Up @@ -2062,12 +2062,19 @@ class virtual ['M, 'T, 'N, 'U] mapper =
method match_expression (x : ('M, 'T) Ast.Expression.Match.t) : ('N, 'U) Ast.Expression.Match.t
=
let open Ast.Expression.Match in
let { arg; cases; arg_internal; comments } = x in
let { arg; cases; arg_internal; match_keyword_loc; comments } = x in
let arg' = this#expression arg in
let cases' = List.map ~f:(this#on_loc_annot * this#match_expression_case) cases in
let arg_internal' = this#on_loc_annot arg_internal in
let match_keyword_loc' = this#on_type_annot match_keyword_loc in
let comments' = this#syntax_opt comments in
{ arg = arg'; cases = cases'; arg_internal = arg_internal'; comments = comments' }
{
arg = arg';
cases = cases';
arg_internal = arg_internal';
match_keyword_loc = match_keyword_loc';
comments = comments';
}

method match_expression_case (case : ('M, 'T) Ast.Expression.Match.Case.t')
: ('N, 'U) Ast.Expression.Match.Case.t' =
Expand Down
2 changes: 1 addition & 1 deletion src/parser_utils/output/js_layout_generator.ml
Original file line number Diff line number Diff line change
Expand Up @@ -1270,7 +1270,7 @@ and expression ?(ctxt = normal_context) ~opts (root_expr : (Loc.t, Loc.t) Ast.Ex
| Some arg -> fuse [space; expression ~ctxt ~opts arg]
| None -> Empty);
]
| E.Match { E.Match.arg; cases; comments; arg_internal = _ } ->
| E.Match { E.Match.arg; cases; comments; arg_internal = _; match_keyword_loc = _ } ->
let cases =
List.map
(fun ((loc, _) as case) ->
Expand Down
2 changes: 2 additions & 0 deletions src/typing/debug_js.ml
Original file line number Diff line number Diff line change
Expand Up @@ -1898,6 +1898,8 @@ let dump_error_message =
spf "ECannotCallReactComponent (%s)" (dump_reason cx reason)
| ENegativeTypeGuardConsistency { reason; _ } ->
spf "ENegativeTypeGuardConsistency (%s)" (dump_reason cx reason)
| EMatchNotExhaustive { loc; reason } ->
spf "EMatchNotExhaustive (%s) (%s)" (string_of_aloc loc) (dump_reason cx reason)
| EDevOnlyRefinedLocInfo { refined_loc; refining_locs = _ } ->
spf "EDevOnlyRefinedLocInfo {refined_loc=%s}" (string_of_aloc refined_loc)
| EDevOnlyInvalidatedRefinementInfo { read_loc; invalidation_info = _ } ->
Expand Down
14 changes: 13 additions & 1 deletion src/typing/errors/error_message.ml
Original file line number Diff line number Diff line change
Expand Up @@ -613,6 +613,12 @@ and 'loc t' =
arg: 'loc virtual_reason;
}
| ECannotCallReactComponent of { reason: 'loc virtual_reason }
(* Match *)
| EMatchNotExhaustive of {
loc: 'loc;
reason: 'loc virtual_reason;
}
(* Dev only *)
| EDevOnlyRefinedLocInfo of {
refined_loc: 'loc;
refining_locs: 'loc list;
Expand Down Expand Up @@ -1413,6 +1419,8 @@ let rec map_loc_of_error_message (f : 'a -> 'b) : 'a t' -> 'b t' =
| ECannotCallReactComponent { reason } -> ECannotCallReactComponent { reason = map_reason reason }
| EDevOnlyRefinedLocInfo { refined_loc; refining_locs } ->
EDevOnlyRefinedLocInfo { refined_loc = f refined_loc; refining_locs = List.map f refining_locs }
| EMatchNotExhaustive { loc; reason } ->
EMatchNotExhaustive { loc = f loc; reason = map_reason reason }
| EDevOnlyInvalidatedRefinementInfo { read_loc; invalidation_info } ->
EDevOnlyInvalidatedRefinementInfo
{
Expand Down Expand Up @@ -1709,7 +1717,8 @@ let util_use_op_of_msg nope util = function
| EUnionPartialOptimizationNonUniqueKey _
| EUnionOptimization _
| EUnionOptimizationOnNonUnion _
| ECannotCallReactComponent _ ->
| ECannotCallReactComponent _
| EMatchNotExhaustive _ ->
nope

(* Not all messages (i.e. those whose locations are based on use_ops) have locations that can be
Expand Down Expand Up @@ -1910,6 +1919,7 @@ let loc_of_msg : 'loc t' -> 'loc option = function
| EDuplicateClassMember { loc; _ } -> Some loc
| EEmptyArrayNoProvider { loc } -> Some loc
| EUnusedPromise { loc; _ } -> Some loc
| EMatchNotExhaustive { loc; _ } -> Some loc
| EDevOnlyRefinedLocInfo { refined_loc; refining_locs = _ } -> Some refined_loc
| EDevOnlyInvalidatedRefinementInfo { read_loc; invalidation_info = _ } -> Some read_loc
| EUnableToSpread _
Expand Down Expand Up @@ -2857,6 +2867,7 @@ let friendly_message_of_msg = function
| EUnionOptimizationOnNonUnion { loc = _; arg } ->
Normal (MessageInvalidUseOfFlowEnforceOptimized arg)
| ECannotCallReactComponent { reason } -> Normal (MessageCannotCallReactComponent reason)
| EMatchNotExhaustive { loc = _; reason } -> Normal (MessageMatchNotExhaustive reason)

let defered_in_speculation = function
| EUntypedTypeImport _
Expand Down Expand Up @@ -3193,3 +3204,4 @@ let error_code_of_message err : error_code option =
| EUnionOptimization _ -> Some UnionUnoptimizable
| EUnionOptimizationOnNonUnion _ -> Some UnionUnoptimizable
| ECannotCallReactComponent _ -> Some ReactRuleCallComponent
| EMatchNotExhaustive _ -> Some MatchNotExhaustive
7 changes: 7 additions & 0 deletions src/typing/errors/flow_intermediate_error.ml
Original file line number Diff line number Diff line change
Expand Up @@ -3982,6 +3982,13 @@ let to_printable_error :
code ": null";
text " to disambiguate.";
]
| MessageMatchNotExhaustive reason ->
[
code "match";
text " is not exhaustively checked: ";
ref reason;
text " has not been fully checked against by the match patterns below.";
]
in
let rec convert_error_message { kind; loc; error_code; root; message; misplaced_source_file = _ }
=
Expand Down
1 change: 1 addition & 0 deletions src/typing/errors/flow_intermediate_error_types.ml
Original file line number Diff line number Diff line change
Expand Up @@ -897,6 +897,7 @@ type 'loc message =
reason: 'loc virtual_reason;
null_loc: 'loc option;
}
| MessageMatchNotExhaustive of 'loc virtual_reason

type 'loc intermediate_error = {
kind: Flow_errors_utils.error_kind;
Expand Down
30 changes: 30 additions & 0 deletions src/typing/merge_js.ml
Original file line number Diff line number Diff line change
Expand Up @@ -618,6 +618,35 @@ let check_spread_prop_keys cx tast =
let (_ : _ Ast.Program.t) = checker#program tast in
()

let check_match_exhaustiveness cx tast =
let checker =
object
inherit
[ALoc.t, ALoc.t * Type.t, ALoc.t, ALoc.t * Type.t] Flow_polymorphic_ast_mapper.mapper as super

method on_type_annot x = x

method on_loc_annot x = x

method! match_expression x =
let { Ast.Expression.Match.match_keyword_loc = (loc, t); _ } = x in
(match Flow_js.possible_concrete_types_for_inspection cx (TypeUtil.reason_of_t t) t with
| [] -> ()
| remaining_ts ->
Base.List.iter remaining_ts ~f:(fun remaining_t ->
Flow_js.add_output
cx
(Error_message.EMatchNotExhaustive
{ loc; reason = TypeUtil.reason_of_t remaining_t }
)
));
super#match_expression x
end
in
if Context.enable_pattern_matching_expressions cx then
let (_ : _ Ast.Program.t) = checker#program tast in
()

let emit_refinement_information_as_errors =
let open Loc_collections in
let emit_refined_locations_info cx =
Expand Down Expand Up @@ -680,6 +709,7 @@ let post_merge_checks cx ast tast metadata =
detect_unused_promises cx;
check_union_opt cx;
check_spread_prop_keys cx tast;
check_match_exhaustiveness cx tast;
emit_refinement_information_as_errors cx

(* Check will lazily create types for the checked file's dependencies. These
Expand Down
16 changes: 14 additions & 2 deletions src/typing/statement.ml
Original file line number Diff line number Diff line change
Expand Up @@ -2867,7 +2867,7 @@ module Make
);
let t = AnyT.at (AnyError None) loc in
((loc, t), TSSatisfies (Tast_utils.error_mapper#ts_satisfies cast))
| Match { Match.arg; cases; arg_internal; comments } ->
| Match { Match.arg; cases; arg_internal; match_keyword_loc; comments } ->
if not @@ Context.enable_pattern_matching_expressions cx then (
Flow.add_output
cx
Expand Down Expand Up @@ -2920,9 +2920,21 @@ module Make
(case_ast :: cases, ts, all_throws)
)
in
let match_keyword_loc =
( match_keyword_loc,
Type_env.var_ref
~lookup_mode:ForValue
cx
(OrdinaryName Flow_ast_utils.match_root_name)
match_keyword_loc
)
in
let match_t = union_of_ts reason (List.rev ts_rev) in
let ast =
((loc, match_t), Match { Match.arg; cases = List.rev cases_rev; arg_internal; comments })
( (loc, match_t),
Match
{ Match.arg; cases = List.rev cases_rev; arg_internal; match_keyword_loc; comments }
)
in
if (not (List.is_empty cases)) && all_throws then
Abnormal.throw_expr_control_flow_exception loc ast
Expand Down
Loading

0 comments on commit f48b56d

Please sign in to comment.