Skip to content

Commit

Permalink
[flow] Explicitly unsoundly unbind methods during cjs exports extraction
Browse files Browse the repository at this point in the history
Summary:
Normally, given something like

```
declare class Foo {
  bar(): void
}

declare const foo: Foo;
const {bar} = foo;
bar();
```

we will get a method-unbinding error. However, this error is hidden if the code is in the following form due to CJS-ESM export extraction magic (likely unintentional).

```
// a.js
declare class Foo {
  bar(): void
}

declare const foo: Foo;
module.exports = foo;

// b.js
import {bar} from './a';
bar();
```

Right now, there are way too many existing code that depends on this unsoundness issue, which is blocking me from fixing the issue of inconsistent this-typing for class methods.

Therefore, in this diff, I decide to formally codify the unsoundness issue in the code for CJS exports extraction, instead of relying on other bugs that accidentally does this.

Changelog: [internal]

Reviewed By: gkz

Differential Revision: D68961697

fbshipit-source-id: 36b33d3b359952ef780b62da5c4fc9fe19f27b27
  • Loading branch information
SamChou19815 authored and facebook-github-bot committed Jan 31, 2025
1 parent fa03a66 commit 60cfefd
Show file tree
Hide file tree
Showing 4 changed files with 41 additions and 9 deletions.
10 changes: 1 addition & 9 deletions src/typing/flow_js_utils.ml
Original file line number Diff line number Diff line change
Expand Up @@ -2081,14 +2081,6 @@ end
(* GetPropT helper *)
(*******************)

let rec unbind_this_method = function
| DefT (r, FunT (static, ({ this_t = (this_t, This_Method { unbound = false }); _ } as ft))) ->
DefT (r, FunT (static, { ft with this_t = (this_t, This_Method { unbound = true }) }))
| DefT (r, PolyT { tparams_loc; tparams; t_out; id }) ->
DefT (r, PolyT { tparams_loc; tparams; t_out = unbind_this_method t_out; id })
| IntersectionT (r, rep) -> IntersectionT (r, InterRep.map unbind_this_method rep)
| t -> t

let check_method_unbinding cx ~use_op ~method_accessible ~reason_op ~propref ~hint p =
match p with
| Method { key_loc; type_ = t }
Expand Down Expand Up @@ -2122,7 +2114,7 @@ let check_method_unbinding cx ~use_op ~method_accessible ~reason_op ~propref ~hi
add_output
cx
(Error_message.EMethodUnbinding { use_op; reason_op; reason_prop = reason_of_t t });
Method { key_loc; type_ = unbind_this_method t })
Method { key_loc; type_ = Type.Properties.unbind_this_method t })
| _ -> p

(* e.g. `0`, `-123, `234234` *)
Expand Down
28 changes: 28 additions & 0 deletions src/typing/type.ml
Original file line number Diff line number Diff line change
Expand Up @@ -2064,6 +2064,8 @@ and Properties : sig

val string_of_id : id -> string

val unbind_this_method : TypeTerm.t -> TypeTerm.t

val extract_named_exports : t -> Exports.t

val iter_t : (TypeTerm.t -> unit) -> t -> unit
Expand Down Expand Up @@ -2117,6 +2119,14 @@ end = struct

let add_method x key_loc type_ = NameUtils.Map.add x (Method { key_loc; type_ })

let rec unbind_this_method = function
| DefT (r, FunT (static, ({ this_t = (this_t, This_Method { unbound = false }); _ } as ft))) ->
DefT (r, FunT (static, { ft with this_t = (this_t, This_Method { unbound = true }) }))
| DefT (r, PolyT { tparams_loc; tparams; t_out; id }) ->
DefT (r, PolyT { tparams_loc; tparams; t_out = unbind_this_method t_out; id })
| IntersectionT (r, rep) -> IntersectionT (r, InterRep.map unbind_this_method rep)
| t -> t

let extract_named_exports pmap =
NameUtils.Map.fold
(fun x p tmap ->
Expand All @@ -2127,6 +2137,24 @@ end = struct
| Field { preferred_def_locs; _ } -> preferred_def_locs
| _ -> None
in
let type_ =
match p with
(* The following code explicitly encodes the following unsound behavior:
```
// a.js
declare class Foo { bar(): void }
declare const foo: Foo;
module.exports = foo;
// b.js
import {bar} from './a'; // no method unbinding error
bar();
```
*)
| Method _ -> unbind_this_method type_
| _ -> type_
in
NameUtils.Map.add x { name_loc = Property.read_loc p; preferred_def_locs; type_ } tmap
| None -> tmap)
pmap
Expand Down
10 changes: 10 additions & 0 deletions tests/cjs_esm_interop/unsound_extract_exports.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
class Foo {
#v: string = '';

foo(): string {
return this.#v;
}
}

const instance: Foo = new Foo();
module.exports = instance;
2 changes: 2 additions & 0 deletions tests/cjs_esm_interop/use_unsound_extract_exports.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
import {foo} from './unsound_extract_exports'; // TODO unsound: no method-unbinding error
foo();

0 comments on commit 60cfefd

Please sign in to comment.