[flow] Ban unsafe string index access in indexed access types

Summary: Thanks to George's work that records whether GetPropT comes from annotation, we can limit the ban to annotation only. We should still ban it completely at some point. However, unlike in runtime code, this kind of pattern has no hope of codemodding it into anything correct, so we will start from here. Changelog: [errors] Invalid indexed access types with string index, like `{foo: string}[string]` will now error instead of silently making it any. Reviewed By: panagosg7 Differential Revision: D55457589 fbshipit-source-id: 2945809f279c46c5f23e747d56ad6d93034d4972
facebook · Apr 17, 2024 · fe00f4c · fe00f4c
1 parent bbe7df6
commit fe00f4c
Show file tree

Hide file tree

Showing 10 changed files with 170 additions and 56 deletions.
diff --git a/src/typing/annotation_inference.ml b/src/typing/annotation_inference.ml
@@ -1059,7 +1059,16 @@ module rec ConsGen : S = struct
       ) ->
       Unsoundness.why Constructor reason_op
     | (DefT (reason_obj, ObjT o), Annot_GetPropT (reason_op, use_op, propref)) ->
-      GetPropTKit.read_obj_prop cx dummy_trace ~use_op o propref reason_obj reason_op None
+      GetPropTKit.read_obj_prop
+        cx
+        dummy_trace
+        ~use_op
+        ~from_annot:true
+        o
+        propref
+        reason_obj
+        reason_op
+        None
     | (AnyT _, Annot_GetPropT (reason_op, _, _)) -> AnyT (reason_op, Untyped)
     | ( DefT (reason, ClassT instance),
         Annot_GetPropT (_, _, Named { name = OrdinaryName "prototype"; _ })

diff --git a/src/typing/flow_js.ml b/src/typing/flow_js.ml
@@ -4453,7 +4453,7 @@ struct
           ) ->
           rec_flow_t cx trace ~use_op:unknown_use (Unsoundness.why Constructor reason, OpenT tout)
         | ( DefT (reason_obj, ObjT o),
-            GetPropT { use_op; reason = reason_op; id; from_annot = _; propref; tout; hint = _ }
+            GetPropT { use_op; reason = reason_op; id; from_annot; propref; tout; hint = _ }
           ) ->
           let lookup_info =
             Base.Option.map id ~f:(fun id ->
@@ -4465,7 +4465,17 @@ struct
                 (id, lookup_default_tout)
             )
           in
-          GetPropTKit.read_obj_prop cx trace ~use_op o propref reason_obj reason_op lookup_info tout
+          GetPropTKit.read_obj_prop
+            cx
+            trace
+            ~use_op
+            ~from_annot
+            o
+            propref
+            reason_obj
+            reason_op
+            lookup_info
+            tout
         | ( AnyT (_, src),
             GetPropT { use_op = _; reason; id; from_annot = _; propref = _; tout; hint = _ }
           ) ->
@@ -4490,6 +4500,7 @@ struct
                   cx
                   trace
                   ~use_op
+                  ~from_annot:false
                   o
                   propref
                   reason_obj

diff --git a/src/typing/flow_js_utils.ml b/src/typing/flow_js_utils.ml
@@ -2284,7 +2284,7 @@ module GetPropT_kit (F : Get_prop_helper_sig) = struct
       Some (OrdinaryField { type_ = value; polarity = dict_polarity }, IndexerProperty)
     | _ -> None
 
-  let read_obj_prop cx trace ~use_op o propref reason_obj reason_op lookup_info =
+  let read_obj_prop cx trace ~from_annot ~use_op o propref reason_obj reason_op lookup_info =
     let l = DefT (reason_obj, ObjT o) in
     match get_obj_prop cx trace o propref reason_op with
     | Some (p, _target_kind) ->
@@ -2318,6 +2318,16 @@ module GetPropT_kit (F : Get_prop_helper_sig) = struct
           F.error_type cx trace reason_op
         | AnyT (_, src) -> F.return cx trace ~use_op:unknown_use (AnyT.why src reason_op)
         | GenericT { bound = DefT (_, StrT _); _ }
+        | DefT (_, StrT _)
+          when from_annot ->
+          let reason_prop = reason_of_t elem_t in
+          let kind = Error_message.InvalidObjKey.Other in
+          add_output
+            cx
+            ~trace
+            (Error_message.EObjectComputedPropertyAccess (reason_op, reason_prop, kind));
+          F.error_type cx trace reason_op
+        | GenericT { bound = DefT (_, StrT _); _ }
         | DefT (_, StrT _) ->
           (* string keys are unsoundly allowed, but there's nothing else to
              flow without knowing their literal values. *)

diff --git a/tests/contextual_typing/contextual_typing.exp b/tests/contextual_typing/contextual_typing.exp
@@ -289,6 +289,14 @@ An annotation on `item` is required because Flow cannot infer its type from loca
                  ^^^^
 
 
+Error ------------------------------------------------------------------------- encountered_placeholder_failure.js:21:13
+
+An annotation on `item` is required because Flow cannot infer its type from local context. [missing-local-annot]
+
+   21| id({[str]: (item) => 1}); // error
+                   ^^^^
+
+
 Error ------------------------------------------------------------------------- encountered_placeholder_failure.js:23:15
 
 An annotation on `item` is required because Flow cannot infer its type from local context. [missing-local-annot]
@@ -1254,4 +1262,4 @@ References:
 
 
 
-Found 90 errors
+Found 91 errors
diff --git a/tests/contextual_typing/encountered_placeholder_failure.js b/tests/contextual_typing/encountered_placeholder_failure.js
@@ -18,7 +18,7 @@ id(async () => (item) => 1); // error
 // Decomp_ObjProp
 id({foo: (item) => 1}); // error
 // Decomp_ObjComputed
-id({[str]: (item) => 1}); // TODO: missing missing-local-annot
+id({[str]: (item) => 1}); // error
 // Decomp_ObjSpread
 id({...{foo: (item) => 1}}); // error
 // Decomp_ArrElement

diff --git a/tests/indexed_access/indexed_access.exp b/tests/indexed_access/indexed_access.exp
@@ -676,127 +676,135 @@ References:
               ^^^^^^ [2]
 
 
-Error ---------------------------------------------------------------------------------------------------- test.js:44:24
+Error ---------------------------------------------------------------------------------------------------- test.js:44:25
 
 Cannot access string literal `bork` on `Obj` because property `bork` is missing in `Obj` [1]. [prop-missing]
 
-   test.js:44:24
-   44| type Nonexistant = Obj['bork']; // Error
-                              ^^^^^^
+   test.js:44:25
+   44| type Nonexistant1 = Obj['bork']; // Error
+                               ^^^^^^
 
 References:
-   test.js:44:20
-   44| type Nonexistant = Obj['bork']; // Error
-                          ^^^ [1]
+   test.js:44:21
+   44| type Nonexistant1 = Obj['bork']; // Error
+                           ^^^ [1]
 
 
-Error ---------------------------------------------------------------------------------------------------- test.js:47:27
+Error ---------------------------------------------------------------------------------------------------- test.js:47:25
+
+Cannot access object with computed property using string [1]. [invalid-computed-prop]
+
+   47| type Nonexistant2 = Obj[string]; // Error
+                               ^^^^^^ [1]
+
+
+Error ---------------------------------------------------------------------------------------------------- test.js:50:27
 
 Cannot access boolean on `Arr` because boolean [1] is not an array index. [incompatible-use]
 
-   47| type ArrNonexistant = Arr[boolean]; // Error
+   50| type ArrNonexistant = Arr[boolean]; // Error
                                  ^^^^^^^ [1]
 
 
-Error ---------------------------------------------------------------------------------------------------- test.js:50:10
+Error ---------------------------------------------------------------------------------------------------- test.js:53:10
 
 Cannot access string literal `x` on undefined because property `x` is missing in undefined [1]. [incompatible-use]
 
-   test.js:50:10
-   50| (1: void['x']); // Error
+   test.js:53:10
+   53| (1: void['x']); // Error
                 ^^^
 
 References:
-   test.js:50:5
-   50| (1: void['x']); // Error
+   test.js:53:5
+   53| (1: void['x']); // Error
            ^^^^ [1]
 
 
-Error ---------------------------------------------------------------------------------------------------- test.js:51:10
+Error ---------------------------------------------------------------------------------------------------- test.js:54:10
 
 Cannot access string literal `x` on null because property `x` is missing in null [1]. [incompatible-use]
 
-   test.js:51:10
-   51| (1: null['x']); // Error
+   test.js:54:10
+   54| (1: null['x']); // Error
                 ^^^
 
 References:
-   test.js:51:5
-   51| (1: null['x']); // Error
+   test.js:54:5
+   54| (1: null['x']); // Error
            ^^^^ [1]
 
 
-Error ----------------------------------------------------------------------------------------------------- test.js:62:2
+Error ----------------------------------------------------------------------------------------------------- test.js:65:2
 
 Cannot cast `'xx'` to indexed access because string [1] is incompatible with number [2]. [incompatible-cast]
 
-   test.js:62:2
-   62| ('xx': O['bar']); // Error
+   test.js:65:2
+   65| ('xx': O['bar']); // Error
         ^^^^ [1]
 
 References:
-   test.js:57:8
-   57|   bar: number;
+   test.js:60:8
+   60|   bar: number;
               ^^^^^^ [2]
 
 
-Error ----------------------------------------------------------------------------------------------------- test.js:71:2
+Error ----------------------------------------------------------------------------------------------------- test.js:74:2
 
 Cannot cast `'xx'` to indexed access because string [1] is incompatible with number [2]. [incompatible-cast]
 
-   test.js:71:2
-   71| ('xx': C['bar']); // Error
+   test.js:74:2
+   74| ('xx': C['bar']); // Error
         ^^^^ [1]
 
 References:
-   test.js:66:8
-   66|   bar: number;
+   test.js:69:8
+   69|   bar: number;
               ^^^^^^ [2]
 
 
-Error ----------------------------------------------------------------------------------------------------- test.js:80:2
+Error ----------------------------------------------------------------------------------------------------- test.js:83:2
 
 Cannot cast `'xx'` to indexed access because string [1] is incompatible with number [2]. [incompatible-cast]
 
-   test.js:80:2
-   80| ('xx': I['bar']); // ERROR
+   test.js:83:2
+   83| ('xx': I['bar']); // ERROR
         ^^^^ [1]
 
 References:
-   test.js:75:8
-   75|   bar: number;
+   test.js:78:8
+   78|   bar: number;
               ^^^^^^ [2]
 
 
-Error ----------------------------------------------------------------------------------------------------- test.js:86:8
+Error ----------------------------------------------------------------------------------------------------- test.js:89:8
 
 Cannot cast function to indexed access because boolean [1] is incompatible with number [2] in the return value.
 [incompatible-cast]
 
-   test.js:86:8
-   86| (() => true) as M['method']; // ERROR
+   test.js:89:8
+   89| (() => true) as M['method']; // ERROR
               ^^^^ [1]
 
 References:
-   test.js:83:13
-   83|   method(): number;
+   test.js:86:13
+   86|   method(): number;
                    ^^^^^^ [2]
 
 
-Error ----------------------------------------------------------------------------------------------------- test.js:91:5
+Error ----------------------------------------------------------------------------------------------------- test.js:94:5
 
 Cannot cast `f()` to empty because number [1] is incompatible with empty [2]. [incompatible-cast]
 
-   test.js:91:5
-   91|     f() as empty; // ERROR
+   test.js:94:5
+   94|     f() as empty; // ERROR
            ^^^
 
 References:
-   test.js:83:13
-   83|   method(): number;
+   test.js:86:13
+   86|   method(): number;
                    ^^^^^^ [1]
-   test.js:91:12
-   91|     f() as empty; // ERROR
+   test.js:94:12
+   94|     f() as empty; // ERROR
                   ^^^^^ [2]
 
 
@@ -871,7 +879,7 @@ References:
 
 
 
-Found 53 errors
+Found 54 errors
 
 Only showing the most relevant union/intersection branches.
 To see all branches, re-run Flow with --show-all-branches
diff --git a/tests/indexed_access/test.js b/tests/indexed_access/test.js
@@ -41,8 +41,11 @@ declare var o: Obj;
 (f({foo: true, baz: 'hi', bort: 1}): boolean); // OK
 (f(o): string); // Error
 
-type Nonexistant = Obj['bork']; // Error
-(1: Nonexistant);
+type Nonexistant1 = Obj['bork']; // Error
+(1: Nonexistant1);
+
+type Nonexistant2 = Obj[string]; // Error
+(1: Nonexistant2);
 
 type ArrNonexistant = Arr[boolean]; // Error
 (1: ArrNonexistant);

diff --git a/tests/speculation_errors/bad_interaction_with_evalt.js b/tests/speculation_errors/bad_interaction_with_evalt.js
@@ -0,0 +1,5 @@
+import type { Bad, Obj2 } from "./exported_bad_evalt";
+
+declare const bad: Bad;
+bad.get() as Obj2; // error: TODO should not error here, since annotation error should be non-speculative
+bad.get(1) as Obj2; // error: TODO spurious incompatible with empty
diff --git a/tests/speculation_errors/exported_bad_evalt.js b/tests/speculation_errors/exported_bad_evalt.js
@@ -0,0 +1,8 @@
+declare export class C<T> {
+  get(): T | null;
+  get(v: T): T;
+}
+type Obj = {foo: string};
+export type Obj2 = {...Obj}
+
+export type Bad = C<Obj2[string]>;