Skip to content

Commit

Permalink
Make disjointness check in equality check fire even when dynamic is p…
Browse files Browse the repository at this point in the history
…resent

Summary:
The equality disjointness check doesn't fire if one of the types is a like-type. So, for example, this is accepted:
```
function make_tuple():(int,string) { ... }
function testit(): bool {
  return make_tuple() === 1;
}
```
In contrast, the user-controllable `<<__NonDisjoint>>` feature *does* see through likes. This diff makes equality behave the same way.

Reviewed By: CatherineGasnier

Differential Revision: D66362246

fbshipit-source-id: 67e122b16aa5b3643b352056ea732c91b8dca7d7
  • Loading branch information
Andrew Kennedy authored and facebook-github-bot committed Nov 25, 2024
1 parent 688937e commit aeb075f
Show file tree
Hide file tree
Showing 6 changed files with 75 additions and 37 deletions.
26 changes: 6 additions & 20 deletions hphp/hack/src/typing/tast_check/disjoint_types.ml
Original file line number Diff line number Diff line change
Expand Up @@ -41,26 +41,12 @@ let add_warning env p name ty1 ty2 ~dynamic ~as_lint =
} )

let check_non_disjoint ~is_dynamic_call ~as_lint env p name ty1 ty2 =
let tenv = Tast_env.tast_env_as_typing_env env in
if
(not is_dynamic_call)
&& Typing_utils.(
(not (is_nothing tenv ty1))
&& (not (is_nothing tenv ty2))
&& is_type_disjoint tenv ty1 ty2)
then
add_warning env p name ty1 ty2 ~dynamic:false ~as_lint
else
let ty1 = Tast_env.strip_dynamic env ty1 in
let ty2 = Tast_env.strip_dynamic env ty2 in
(* We don't flag ~null because typically this is a false positive *)
if
Typing_utils.(
(not (is_nothing tenv ty1 || is_null tenv ty1))
&& (not (is_nothing tenv ty2 || is_null tenv ty2))
&& is_type_disjoint tenv ty1 ty2)
then
add_warning env p name ty1 ty2 ~dynamic:true ~as_lint
match Env.is_disjoint ~is_dynamic_call env ty1 ty2 with
| Env.NonDisjoint -> ()
| Env.DisjointIgnoringDynamic (ty1, ty2) ->
add_warning env p name ty1 ty2 ~dynamic:true ~as_lint
| Env.Disjoint ->
add_warning env p name ty1 ty2 ~dynamic:is_dynamic_call ~as_lint

let rec check_non_disjoint_tyl ~is_dynamic_call ~as_lint env p name tyl =
match tyl with
Expand Down
21 changes: 9 additions & 12 deletions hphp/hack/src/typing/tast_check/equality_check.ml
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ open Ast_defs
open Typing_defs
module Cls = Folded_class
module Env = Tast_env
module MakeType = Typing_make_type
module SN = Naming_special_names

let warning_kind = Typing_warning.Equality_check
Expand Down Expand Up @@ -57,10 +56,6 @@ let opaque_enum_expander =
| _ -> super#on_tnewtype env r cid tyl cstr
end

let is_nothing env ty =
let nothing = MakeType.nothing Reason.none in
Tast_env.is_sub_type env ty nothing

let add_warning env ~as_lint pos kind ty1 ty2 =
Typing_warning_utils.add_for_migration
(Env.get_tcopt env)
Expand All @@ -78,15 +73,17 @@ let error_if_inequatable env ty1 ty2 err =
let expand_enum = opaque_enum_expander#on_type in
(* Break all type abstractions *)
let expand env ty =
let env = Env.tast_env_as_typing_env env in
let (env, ty) = expand_enum env ty in
expand_tydef env ty
let ((env, _), ty, _) = expand_tydef env ty in
(Tast_env.typing_env_as_tast_env env, ty)
in
let typing_env = Env.tast_env_as_typing_env env in
let ((typing_env, _), ety1, _) = expand typing_env ty1 in
let ((typing_env, _), ety2, _) = expand typing_env ty2 in
if is_nothing env ety1 || is_nothing env ety2 then
()
else if Typing_subtype.is_type_disjoint typing_env ety1 ety2 then
let (env, ety1) = expand env ty1 in
let (env, ety2) = expand env ty2 in
match Env.is_disjoint ~is_dynamic_call:false env ety1 ety2 with
| Env.NonDisjoint -> ()
| Env.Disjoint -> err (Env.print_ty env ty1) (Env.print_ty env ty2)
| Env.DisjointIgnoringDynamic (ty1, ty2) ->
err (Env.print_ty env ty1) (Env.print_ty env ty2)

let ensure_valid_equality_check env ~as_lint p bop e1 e2 =
Expand Down
28 changes: 28 additions & 0 deletions hphp/hack/src/typing/tast_env.ml
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,34 @@ let expand_type = Typing_env.expand_type

let strip_dynamic env ty = snd (Typing_dynamic_utils.strip_dynamic env ty)

type is_disjoint_result =
| NonDisjoint
| Disjoint
| DisjointIgnoringDynamic of Typing_defs.locl_ty * Typing_defs.locl_ty

let is_disjoint ~is_dynamic_call env ty1 ty2 =
if
(not is_dynamic_call)
&& Typing_utils.(
(not (is_nothing env ty1))
&& (not (is_nothing env ty2))
&& is_type_disjoint env ty1 ty2)
then
Disjoint
else
let ty1 = strip_dynamic env ty1 in
let ty2 = strip_dynamic env ty2 in
(* We don't flag ~null because typically this is a false positive *)
if
Typing_utils.(
(not (is_nothing env ty1 || is_null env ty1))
&& (not (is_nothing env ty2 || is_null env ty2))
&& is_type_disjoint env ty1 ty2)
then
DisjointIgnoringDynamic (ty1, ty2)
else
NonDisjoint

let strip_supportdyn env ty =
let (sd, _, ty) = Typing_utils.strip_supportdyn env ty in
(sd, ty)
Expand Down
12 changes: 12 additions & 0 deletions hphp/hack/src/typing/tast_env.mli
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,18 @@ val fully_expand : env -> Tast.ty -> Tast.ty
(** Strip ~ from type *)
val strip_dynamic : env -> Tast.ty -> Tast.ty

type is_disjoint_result =
| NonDisjoint (** Types are possibly overlapping *)
| Disjoint (** Types are definitely non-overlapping *)
| DisjointIgnoringDynamic of Tast.ty * Tast.ty
(** Types overlap in dynamic, but as like-stripped types are disjoint *)

(** Are types disjoint? If is_dynamic_call is true, then
we are checking a dynamic call through a like type, so
result cannot be precise (i.e. won't return Disjoint) *)
val is_disjoint :
is_dynamic_call:bool -> env -> Tast.ty -> Tast.ty -> is_disjoint_result

(** Strip supportdyn from type, return whether it was there or not *)
val strip_supportdyn : env -> Tast.ty -> bool * Tast.ty

Expand Down
16 changes: 14 additions & 2 deletions hphp/hack/test/typecheck/not_trivial_strict_eq.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,13 @@
*
*/

function f() /* : TAny */ { return 42; }
function f() /* : TAny */ {
return 42;
}

function getArr(): varray<int> { return vec[]; }
function getArr(): varray<int> {
return vec[];
}

function g(): void {
$a = getArr();
Expand All @@ -36,3 +40,11 @@ function h(?int $a, ?string $b): bool {
// Not a trivial comparison since both $a and $b can be null
return $a === $b;
}

function make_tuple(): (int, string) {
return tuple(3, 'a');
}

function test2(): bool {
return make_tuple() === 2;
}
9 changes: 6 additions & 3 deletions hphp/hack/test/typecheck/not_trivial_strict_eq.php.exp
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
WARN: File "not_trivial_strict_eq.php", line 37, characters 10-18:
WARN: File "not_trivial_strict_eq.php", line 41, characters 10-18:
This expression is likely always `false` (Warn[12001])
File "not_trivial_strict_eq.php", line 35, characters 13-15:
File "not_trivial_strict_eq.php", line 39, characters 13-15:
This is an int
File "not_trivial_strict_eq.php", line 35, characters 22-27:
File "not_trivial_strict_eq.php", line 39, characters 22-27:
This is a string
WARN: File "not_trivial_strict_eq.php", line 49, characters 10-27:
Invalid comparison: This expression will always return `false`.
A value of type `(int, string)` can never be equal to a value of type `int` (Warn[12007])
ERROR: File "not_trivial_strict_eq.php", line 12, characters 10-10:
Was expecting a return type hint (Typing[4030])

0 comments on commit aeb075f

Please sign in to comment.