Permalink
Browse files

Forbid truthiness tests on string and arraykey

Summary: Truthiness tests on values of type string or arraykey may not behave as expected. Like other dynamic languages, PHP considers `""` to be falsy, but unlike other dynamic languages, PHP considers `"0"` to be falsy. Hack has inherited this behavior. Since the user may have intended only to test for emptiness, we emit an error when testing the truthiness of a string after this change, suggesting `Str\is_empty` instead.

Reviewed By: CatherineGasnier

Differential Revision: D8877182

fbshipit-source-id: 862e9546b65d4f92a1056576f5c2b79cd0e7ede5
  • Loading branch information...
Jake Bailey (Hacklang) authored and hhvm-bot committed Nov 20, 2018
1 parent af1a293 commit 7ec9173a791a31cd6d3ecfc517ef2af4377007d3
@@ -3468,6 +3468,25 @@ let invalid_truthiness_test pos ty =
let sketchy_truthiness_test pos ty truthiness =
add (Typing.err_code Typing.SketchyTruthinessTest) pos @@
match truthiness with
| `String ->
Printf.sprintf
"Sketchy condition: testing the truthiness of %s may not behave as expected.\n\
The values '' and '0' are both considered falsy. \
To check for emptiness, use Str\\is_empty."
ty
| `Arraykey ->
Printf.sprintf
"Sketchy condition: testing the truthiness of %s may not behave as expected.\n\
The values 0, '', and '0' are all considered falsy. \
Test for them explicitly."
ty
| `Stringish ->
Printf.sprintf
"Sketchy condition: testing the truthiness of a %s may not behave as expected.\n\
The values '' and '0' are both considered falsy, \
but objects will be truthy even if their __toString returns '' or '0'.\n\
To check for emptiness, convert to a string and use Str\\is_empty."
ty
| `Traversable ->
(* We have a truthiness test on a value with an interface type which is a
subtype of Traversable, but not a subtype of Container.
@@ -573,7 +573,7 @@ module type S = sig
val shapes_idx_with_non_existent_field: Pos.t -> string -> Pos.t -> [< `Undefined | `Unset] -> unit
val ambiguous_object_access: Pos.t -> string -> Pos.t -> string -> Pos.t -> string -> string -> unit
val invalid_truthiness_test: Pos.t -> string -> unit
val sketchy_truthiness_test: Pos.t -> string -> [< `Traversable ] -> unit
val sketchy_truthiness_test: Pos.t -> string -> [< `String | `Arraykey | `Stringish | `Traversable ] -> unit
val forward_compatibility_not_current: Pos.t -> ForwardCompatibilityLevel.t -> unit
val forward_compatibility_below_minimum: Pos.t -> ForwardCompatibilityLevel.t -> unit
val unserializable_type: Pos.t -> string -> unit
@@ -23,7 +23,17 @@ let rec truthiness_test env ((p, ty), e) =
| Expr_list el -> truthiness_test env (List.last_exn el)
| _ ->
let open Tast_utils in
let prim_to_string prim = Typing_print.error (Typing_defs.Tprim prim) in
List.iter (find_sketchy_types env ty) begin function
| String ->
let tystr = prim_to_string Aast_defs.Tstring in
Errors.sketchy_truthiness_test p tystr `String
| Arraykey ->
let tystr = prim_to_string Aast_defs.Tarraykey in
Errors.sketchy_truthiness_test p tystr `Arraykey
| Stringish ->
let tystr = Utils.strip_ns SN.Classes.cStringish in
Errors.sketchy_truthiness_test p tystr `Stringish
| Traversable_interface (env, ty) ->
Errors.sketchy_truthiness_test p (Env.print_ty env ty) `Traversable
end;
@@ -135,6 +135,11 @@ let rec truthiness env ty =
test, it indicates a potential logic error, since the truthiness of some
values in the type may be surprising. *)
type sketchy_type_kind =
| String | Arraykey | Stringish
(** Truthiness tests on strings may not behave as expected. The user may not
know that the string "0" is falsy, and may have intended only to check for
emptiness. *)
| Traversable_interface of Env.t * Tast.ty
(** Interface types which implement Traversable but not Container may be
always truthy, even when empty. *)
@@ -145,7 +150,11 @@ let rec find_sketchy_types env acc ty =
match snd ty with
| Toption ty -> find_sketchy_types env acc ty
| Tprim Tstring -> String :: acc
| Tprim Tarraykey -> Arraykey :: acc
| Tclass ((_, cid), _, _) ->
if cid = SN.Classes.cStringish then Stringish :: acc else
if tclass_is_falsy_when_empty env ty || not (is_traversable env ty)
then acc
else begin
@@ -1 +1,3 @@
No errors
File "assign_in_conditional.php", line 9, characters 7-37:
Sketchy condition: testing the truthiness of a string may not behave as expected.
The values '' and '0' are both considered falsy. To check for emptiness, use Str\is_empty. (Typing[4276])
@@ -1 +1,3 @@
No errors
File "initialize_property_with_static_literal.php", line 16, characters 29-42:
Sketchy condition: testing the truthiness of a string may not behave as expected.
The values '' and '0' are both considered falsy. To check for emptiness, use Str\is_empty. (Typing[4276])
@@ -1 +1,3 @@
No errors
File "truthiness_test_13.php", line 4, characters 7-8:
Sketchy condition: testing the truthiness of an array key (int/string) may not behave as expected.
The values 0, '', and '0' are all considered falsy. Test for them explicitly. (Typing[4276])
@@ -1 +1,3 @@
No errors
File "truthiness_test_14.php", line 5, characters 7-8:
Sketchy condition: testing the truthiness of a string may not behave as expected.
The values '' and '0' are both considered falsy. To check for emptiness, use Str\is_empty. (Typing[4276])
@@ -1 +1,4 @@
No errors
File "truthiness_test_19.php", line 4, characters 7-8:
Sketchy condition: testing the truthiness of a Stringish may not behave as expected.
The values '' and '0' are both considered falsy, but objects will be truthy even if their __toString returns '' or '0'.
To check for emptiness, convert to a string and use Str\is_empty. (Typing[4276])
@@ -10,7 +10,7 @@ function test(bool $b, int $e): void {
$b = true;
$c = 1;
$d = false;
$e = "";
$e = vec[];
while ($b) {
// a dependency chain to typecheck this loop block multiple times

0 comments on commit 7ec9173

Please sign in to comment.