Skip to content

Commit

Permalink
Ban new on functions
Browse files Browse the repository at this point in the history
Summary:
Before this change, we allowed functions to act as constructors. This was done in order to support the pattern that existed before ES6 classes.

```
function C(a) {
  this.a = a;
}
const c = new C(1);
```

We are motivated to remove support for this pattern not only because it has been obviated by ES6 classes, but also because it relies on unsealed objects to model `this`, which we are are in the process of eliminating.

Changelog: [error] Banned usage of `new` on functions. Move usages of this pattern to use ES6 classes instead. If the pattern exists in third-party that cannot be changed directly, you can use a [declaration file](https://flow.org/en/docs/declarations/) to type the legacy pattern with a `declare class`.

Reviewed By: samwgoldman

Differential Revision: D35037868

fbshipit-source-id: 252e726212b0c09be4bbc77e505bc44082d18286
  • Loading branch information
gkz authored and facebook-github-bot committed Mar 29, 2022
1 parent 25e9495 commit 5aba8ec
Show file tree
Hide file tree
Showing 35 changed files with 137 additions and 559 deletions.
2 changes: 2 additions & 0 deletions src/common/errors/error_codes.ml
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ type error_code =
| InvalidCharsetTypeArg
| InvalidCompare
| InvalidComputedProp
| InvalidConstructor
| InvalidEnumAccess
| InvalidExact
| InvalidExhaustiveCheck
Expand Down Expand Up @@ -241,6 +242,7 @@ let string_of_code : error_code -> string = function
| InvalidCharsetTypeArg -> "invalid-charset-type-arg"
| InvalidCompare -> "invalid-compare"
| InvalidComputedProp -> "invalid-computed-prop"
| InvalidConstructor -> "invalid-constructor"
| InvalidEnumAccess -> "invalid-enum-access"
| InvalidExact -> "invalid-exact"
| InvalidExhaustiveCheck -> "invalid-exhaustive-check"
Expand Down
2 changes: 1 addition & 1 deletion src/typing/debug_js.ml
Original file line number Diff line number Diff line change
Expand Up @@ -1211,7 +1211,6 @@ let dump_error_message =
| IncompatibleMethodT _ -> "IncompatibleMethodT"
| IncompatibleCallT -> "IncompatibleCallT"
| IncompatibleMixedCallT -> "IncompatibleMixedCallT"
| IncompatibleConstructorT -> "IncompatibleConstructorT"
| IncompatibleGetElemT _ -> "IncompatibleGetElemT"
| IncompatibleSetElemT _ -> "IncompatibleSetElemT"
| IncompatibleCallElemT _ -> "IncompatibleCallElemT"
Expand Down Expand Up @@ -1694,6 +1693,7 @@ let dump_error_message =
in
spf "ESketchyNumberLint (%s) (%s)" kind_str (dump_reason cx reason)
)
| EInvalidConstructor reason -> spf "EInvalidConstructor (%s)" (dump_reason cx reason)
| EInvalidPrototype (loc, reason) ->
spf "EInvalidPrototype (%s) (%s)" (string_of_aloc loc) (dump_reason cx reason)
| EUnnecessaryOptionalChain (loc, _) -> spf "EUnnecessaryOptionalChain (%s)" (string_of_aloc loc)
Expand Down
36 changes: 25 additions & 11 deletions src/typing/errors/error_message.ml
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,7 @@ and 'loc t' =
valid: 'loc virtual_reason;
use_op: 'loc virtual_use_op;
}
| EInvalidConstructor of 'loc virtual_reason
| EUnsupportedKeyInObjectType of 'loc
| EPredAnnot of 'loc
| ERefineAnnot of 'loc
Expand Down Expand Up @@ -556,7 +557,6 @@ and 'loc upper_kind =
| IncompatibleMethodT of 'loc * name option
| IncompatibleCallT
| IncompatibleMixedCallT
| IncompatibleConstructorT
| IncompatibleGetElemT of 'loc
| IncompatibleSetElemT of 'loc
| IncompatibleCallElemT of 'loc
Expand Down Expand Up @@ -598,13 +598,13 @@ let rec map_loc_of_error_message (f : 'a -> 'b) : 'a t' -> 'b t' =
| IncompatibleSetElemT loc -> IncompatibleSetElemT (f loc)
| IncompatibleCallElemT loc -> IncompatibleCallElemT (f loc)
| ( IncompatibleGetPrivatePropT | IncompatibleSetPrivatePropT | IncompatibleCallT
| IncompatibleMixedCallT | IncompatibleConstructorT | IncompatibleElemTOfArrT
| IncompatibleObjAssignFromTSpread | IncompatibleObjAssignFromT | IncompatibleObjRestT
| IncompatibleObjSealT | IncompatibleArrRestT | IncompatibleSuperT | IncompatibleMixinT
| IncompatibleSpecializeT | IncompatibleThisSpecializeT | IncompatibleVarianceCheckT
| IncompatibleGetKeysT | IncompatibleGetValuesT | IncompatibleUnaryMinusT
| IncompatibleMapTypeTObject | IncompatibleTypeAppVarianceCheckT | IncompatibleGetStaticsT
| IncompatibleBindT | IncompatibleUnclassified _ ) as u ->
| IncompatibleMixedCallT | IncompatibleElemTOfArrT | IncompatibleObjAssignFromTSpread
| IncompatibleObjAssignFromT | IncompatibleObjRestT | IncompatibleObjSealT
| IncompatibleArrRestT | IncompatibleSuperT | IncompatibleMixinT | IncompatibleSpecializeT
| IncompatibleThisSpecializeT | IncompatibleVarianceCheckT | IncompatibleGetKeysT
| IncompatibleGetValuesT | IncompatibleUnaryMinusT | IncompatibleMapTypeTObject
| IncompatibleTypeAppVarianceCheckT | IncompatibleGetStaticsT | IncompatibleBindT
| IncompatibleUnclassified _ ) as u ->
u
in
let map_unsupported_syntax = function
Expand Down Expand Up @@ -730,6 +730,7 @@ let rec map_loc_of_error_message (f : 'a -> 'b) : 'a t' -> 'b t' =
| EInvalidCharSet { invalid = (ir, set); valid; use_op } ->
EInvalidCharSet
{ invalid = (map_reason ir, set); valid = map_reason valid; use_op = map_use_op use_op }
| EInvalidConstructor r -> EInvalidConstructor (map_reason r)
| EIncompatibleWithShape (l, u, use_op) ->
EIncompatibleWithShape (map_reason l, map_reason u, map_use_op use_op)
| EInvalidObjectKit { reason; reason_op; use_op } ->
Expand Down Expand Up @@ -1206,6 +1207,7 @@ let util_use_op_of_msg nope util = function
| EInstanceofRHS _
| EObjectComputedPropertyAccess (_, _)
| EObjectComputedPropertyAssign (_, _)
| EInvalidConstructor _
| EInvalidLHSInAssignment _
| EUnsupportedImplements _
| EReactElementFunArity (_, _, _)
Expand Down Expand Up @@ -1325,6 +1327,7 @@ let loc_of_msg : 'loc t' -> 'loc option = function
| EEnumNotIterable { reason; _ }
| ERecursiveDefinition { reason; _ }
| EDefinitionCycle ((reason, _), _)
| EInvalidConstructor reason
| EInvalidDeclaration { declaration = reason; _ } ->
Some (poly_loc_of_reason reason)
| EExponentialSpread
Expand Down Expand Up @@ -3251,6 +3254,18 @@ let friendly_message_of_msg : Loc.t t' -> Loc.t friendly_message_recipe =
]
in
Normal { features }
| EInvalidConstructor reason ->
Normal
{
features =
[
text "Cannot use ";
code "new";
text " on ";
ref reason;
text ". Only classes can be constructed.";
];
}
| EInvalidPrototype (_, reason) ->
Normal
{
Expand Down Expand Up @@ -3904,9 +3919,7 @@ let error_code_of_use_op use_op ~default =
Base.Option.first_some (fold_use_op code_of_root code_of_frame use_op) (Some default)

let error_code_of_upper_kind = function
| IncompatibleConstructorT
| IncompatibleCallT ->
Some NotAFunction
| IncompatibleCallT -> Some NotAFunction
| IncompatibleObjAssignFromTSpread
| IncompatibleArrRestT ->
Some NotAnArray
Expand Down Expand Up @@ -4027,6 +4040,7 @@ let error_code_of_message err : error_code option =
(* We don't want these to be suppressible *)
| EInternal (_, _) -> None
| EInvalidCharSet _ -> Some InvalidCharsetTypeArg
| EInvalidConstructor _ -> Some InvalidConstructor
| EInvalidLHSInAssignment _ -> Some InvalidLhs
| EInvalidObjectKit _ -> Some NotAnObject
| EInvalidPrototype _ -> Some NotAnObject
Expand Down
4 changes: 1 addition & 3 deletions src/typing/errors/flow_error.ml
Original file line number Diff line number Diff line change
Expand Up @@ -1032,9 +1032,7 @@ let rec make_error_printable ?(speculation = false) (error : Loc.t t) : Loc.t Er
use_loc
use_op
[text "the parameter types of an "; ref lower; text " are unknown"]
| IncompatibleCallT
| IncompatibleConstructorT ->
nope "is not a function"
| IncompatibleCallT -> nope "is not a function"
| IncompatibleObjAssignFromTSpread
| IncompatibleArrRestT ->
nope "is not an array"
Expand Down
44 changes: 4 additions & 40 deletions src/typing/flow_js.ml
Original file line number Diff line number Diff line change
Expand Up @@ -3321,46 +3321,6 @@ struct
in
(* return this *)
rec_flow cx trace (ret, ObjTestT (annot_reason ~annot_loc reason_op, this, t))
(****************************************************************)
(* function types derive objects through explicit instantiation *)
(****************************************************************)
| ( DefT (lreason, _, FunT (_, proto, ({ this_t = (this, _); return_t = ret; _ } as ft))),
ConstructorT (use_op, reason_op, targs, args, t)
) ->
(* TODO: closure *)
(* create new object **)
let reason_c = replace_desc_reason RNewObject reason_op in
let objtype =
let obj_kind = UnsealedInFile (ALoc.source (loc_of_t proto)) in
let flags = { default_flags with obj_kind } in
let call = None in
let pmap = Context.generate_property_map cx NameUtils.Map.empty in
mk_objecttype ~flags ~call pmap proto
in
let new_obj = DefT (reason_c, bogus_trust (), ObjT objtype) in
(* error if type arguments are provided to non-polymorphic constructor **)
Base.Option.iter targs ~f:(fun _ ->
add_output
cx
~trace
Error_message.(
ECallTypeArity
{
call_loc = aloc_of_reason reason_op;
is_new = true;
reason_arity = lreason;
expected_arity = 0;
}
)
);

(* call function with this = new_obj, params = args **)
rec_flow_t cx trace ~use_op:unknown_use (new_obj, this);
multiflow_call cx trace ~use_op reason_op args ft;

(* if ret is object-like, return ret; otherwise return new_obj **)
let reason_o = replace_desc_reason RConstructorReturn reason_op in
rec_flow cx trace (ret, ObjTestT (reason_o, new_obj, t))
| (AnyT _, ConstructorT (use_op, reason_op, targs, args, t)) ->
ignore targs;

Expand All @@ -3369,6 +3329,10 @@ struct
(fun t -> rec_flow cx trace (t, UseT (use_op, AnyT.untyped reason_op)))
args;
rec_flow_t cx trace ~use_op:unknown_use (AnyT.untyped reason_op, t)
(* Only classes (and `any`) can be constructed. *)
| (_, ConstructorT (use_op, reason_op, _, _, t)) ->
add_output cx ~trace Error_message.(EInvalidConstructor (reason_of_t l));
rec_flow_t cx trace ~use_op (AnyT.error reason_op, t)
(* Since we don't know the signature of a method on AnyT, assume every
parameter is an AnyT. *)
| ( AnyT _,
Expand Down
1 change: 0 additions & 1 deletion src/typing/flow_js_utils.ml
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,6 @@ let error_message_kind_of_upper = function
Error_message.IncompatibleMethodT (aloc_of_reason r, Some name)
| MethodT (_, _, _, Computed t, _, _) -> Error_message.IncompatibleMethodT (loc_of_t t, None)
| CallT _ -> Error_message.IncompatibleCallT
| ConstructorT _ -> Error_message.IncompatibleConstructorT
| GetElemT (_, _, t, _) -> Error_message.IncompatibleGetElemT (loc_of_t t)
| SetElemT (_, _, t, _, _, _) -> Error_message.IncompatibleSetElemT (loc_of_t t)
| CallElemT (_, _, t, _) -> Error_message.IncompatibleCallElemT (loc_of_t t)
Expand Down
28 changes: 14 additions & 14 deletions tests/ast_error_description/ast_error_description.exp
Original file line number Diff line number Diff line change
Expand Up @@ -1132,18 +1132,18 @@ References:
^^^^^ [2]


Error ----------------------------------------------------------------------------------------------- expression.js:48:4
Error ----------------------------------------------------------------------------------------------- expression.js:48:8

Cannot cast `new f()` to empty because new object [1] is incompatible with empty [2]. [incompatible-cast]
Cannot use `new` on function [1]. Only classes can be constructed. [invalid-constructor]

expression.js:48:4
expression.js:48:8
48| (((new f(): empty): T).p: empty);
^^^^^^^ [1]
^

References:
expression.js:48:13
48| (((new f(): empty): T).p: empty);
^^^^^ [2]
expression.js:9:11
9| const f = (x: mixed): mixed => x;
^^^^^^^^^^^^^^^^^^^^^^ [1]


Error ----------------------------------------------------------------------------------------------- expression.js:49:2
Expand All @@ -1163,18 +1163,18 @@ References:
^^^^^ [2]


Error ----------------------------------------------------------------------------------------------- expression.js:49:4
Error ----------------------------------------------------------------------------------------------- expression.js:49:8

Cannot cast `new f(...)` to empty because new object [1] is incompatible with empty [2]. [incompatible-cast]
Cannot use `new` on function [1]. Only classes can be constructed. [invalid-constructor]

expression.js:49:4
expression.js:49:8
49| (((new f(42): empty): T).p: empty);
^^^^^^^^^ [1]
^

References:
expression.js:49:15
49| (((new f(42): empty): T).p: empty);
^^^^^ [2]
expression.js:9:11
9| const f = (x: mixed): mixed => x;
^^^^^^^^^^^^^^^^^^^^^^ [1]


Error ----------------------------------------------------------------------------------------------- expression.js:50:2
Expand Down
2 changes: 0 additions & 2 deletions tests/async/async_parse.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ class C {
var e = async function () {};
var et = async function<T> (a: T) {};

var n = new async function() {};

var o = { async m() {} };
var ot = { async m<T>(a: T) {} };
var oz = { async async() {} };
Expand Down
2 changes: 0 additions & 2 deletions tests/async/await_parse.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ class C {
var e = async function () { await 1; };
var et = async function<T> (a: T) { await 1; };

var n = new async function() { await 1; };

var o = { async m() { await 1; } };
var ot = { async m<T>(a: T) { await 1; } };
var oz = { async async(async) { await async; } };
Expand Down
15 changes: 0 additions & 15 deletions tests/autocomplete/autocomplete.exp
Original file line number Diff line number Diff line change
Expand Up @@ -2235,21 +2235,6 @@ import_source.js:3:17
Flags: --pretty
{"result":[]}

object-with-prototype.js:9:11
Flags: --pretty
{
"result":[
{"name":"bar","type":"string"},
{"name":"baz","type":"string"},
{"name":"hasOwnProperty","type":"(prop: mixed) => boolean"},
{"name":"isPrototypeOf","type":"(o: mixed) => boolean"},
{"name":"propertyIsEnumerable","type":"(prop: mixed) => boolean"},
{"name":"toLocaleString","type":"() => string"},
{"name":"toString","type":"() => string"},
{"name":"valueOf","type":"() => mixed"}
]
}

object-spread-1.js:9:3
Flags: --pretty
{
Expand Down
10 changes: 0 additions & 10 deletions tests/autocomplete/object-with-prototype.js

This file was deleted.

16 changes: 15 additions & 1 deletion tests/constructor/constructor.exp
Original file line number Diff line number Diff line change
Expand Up @@ -43,5 +43,19 @@ References:
^ [3]


Error -------------------------------------------------------------------------------------------------- invalid.js:5:15

Found 3 errors
Cannot use `new` on function [1]. Only classes can be constructed. [invalid-constructor]

invalid.js:5:15
5| const c = new C(1); // ERROR
^

References:
invalid.js:1:1
1| function C(a: number) {
^^^^^^^^^^^^^^^^^^^^^ [1]



Found 4 errors
9 changes: 9 additions & 0 deletions tests/constructor/invalid.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
function C(a: number) {
this.a = a;
}

const c = new C(1); // ERROR

// `any` is allowed
declare var A: any;
const a = new A(1); // OK
2 changes: 1 addition & 1 deletion tests/core_tests/core_tests.exp
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
Error ------------------------------------------------------------------------------------------------------ json.js:4:5

Cannot call `JSON` because object type [1] is not a function. [not-a-function]
Cannot use `new` on object type [1]. Only classes can be constructed. [invalid-constructor]

json.js:4:5
4| new JSON(); // ERROR
Expand Down
2 changes: 1 addition & 1 deletion tests/declare_export/declare_export.exp
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ References:

Error ----------------------------------------------------------------------------------------------- es6modules.js:70:5

Cannot call `CJS_Clobb_Class_NS` because module `CommonJS_Clobbering_Class` [1] is not a function. [not-a-function]
Cannot use `new` on module `CommonJS_Clobbering_Class` [1]. Only classes can be constructed. [invalid-constructor]

es6modules.js:70:5
70| new CJS_Clobb_Class_NS(); // Error: Namespace object isn't constructable
Expand Down
2 changes: 1 addition & 1 deletion tests/demo/demo.exp
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ References:

Error ------------------------------------------------------------------------------------------------------ 2/B.js:3:13

Cannot call `A` because exports [1] is not a function. [not-a-function]
Cannot use `new` on exports [1]. Only classes can be constructed. [invalid-constructor]

3| var z = new A("42").getX();
^
Expand Down
2 changes: 1 addition & 1 deletion tests/es6modules/es6modules.exp
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ References:

Error ----------------------------------------------------------------------------------------------- es6modules.js:70:5

Cannot call `CJS_Clobb_Class_NS` because module `CommonJS_Clobbering_Class` [1] is not a function. [not-a-function]
Cannot use `new` on module `CommonJS_Clobbering_Class` [1]. Only classes can be constructed. [invalid-constructor]

es6modules.js:70:5
70| new CJS_Clobb_Class_NS(); // Error: Namespace object isn't constructable
Expand Down
Loading

0 comments on commit 5aba8ec

Please sign in to comment.