Removed hard-coded .length case #223
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
| @@ -11,4 +11,8 @@ E.js:8:6,10: boolean | |||
| This type is incompatible with | |||
| E.js:4:14,19: number | |||
|
|
|||
| Found 3 errors | |||
| F.js:2:14,17: string | |||
There was a problem hiding this comment.
Hmmm...I don't see an F.js. Did you add it and forget to add it to the commit?
There was a problem hiding this comment.
Oops. Yes, F.js was missed in the commit. I corrected that now. Thanks!
| @@ -1262,9 +1262,6 @@ let rec flow cx (l,u) trace = | |||
| | (_, | |||
| (GetT(_,"toLocaleString",_) | SetT(_,"toLocaleString",_))) -> () | |||
|
|
|||
| | ((ObjT _ | ArrT _), GetT(reason_op,"length",tout)) -> | |||
| unit_flow cx (NumT.why reason_op, tout) | |||
|
|
|||
| | ((ObjT _ | ArrT _), | |||
There was a problem hiding this comment.
Can you also delete this case? It was allowing you to set .length to anything and call .length() with anything. For example this is clearly wrong
var a = { length: "duck" };
a.length = 123;
a.length();
and I think you should get two errors if you delete this case.
Also, sorry for not giving this feedback last time. If you're busy just let me know. I'll merge and update it myself :)
There was a problem hiding this comment.
Very good point! I have just pushed a squashed commit with that case removed.
Enable type checker to correctly type non-standard length properties. Fixes issue facebook#131.
|
Awesome! Thanks for fixing this issue! |
Removed hard-coded .length case
Summary:
The `hh_show` pseudo-function is very useful for debugging Hack type inference: it displays on the terminal the type that Hack believes an expression to have at the point that it type checks the line in which it occurs. (So, for example, it will show multiple times in a loop as Hack seeks a fixed point.)
But there is currently no way to dump any other aspect of Hack internals such as the type parameter environment. This diff rectifies this situation. It introduces a new nullary pseudo-function `hh_show_env` that causes Hack to dump information about its environment, including:
* The types of all locals (`local_types`)
* The lower and upper bounds on all type parameters in scope (`tpenv`)
* The state of the type-variable-to-type variable substitution (`subst`)
* The state of the type-variable-to-type environment (`tenv`)
The last two aren't shown in full but instead as a delta from the previous time that `hh_show_env` was called.
Some other aspects:
* Colour display, with indentation!
* The position information of the `hh_show_env` command is shown above the environment information, with an iteration number in square brackets (because it might be invoked more than once e.g. round a loop).
The `hh_show` command is also improved, to display this position information.
Finally, type variable numbers are shown in types so that the `subst` and `tpenv` parts of the environment can be debugged.
Here is an example: the test `erling_loop5.php` with `hh_show_env()` inserted at the end of the inner loop. The output from `hh_single_type_check` is as follows:
```
File "erling_loop5.php", line 34, characters 5-17:[1]
local_types
$obj: X as X<#216(int | string)>
$x: (int)
$y: (int)
$z: (int | string)
$i: int
subst(changes)
#199: #216; #201: #212; #212: #214; #214: #216;
tenv(changes)
#198: [unresolved]; #199: [unresolved]; #200: [unresolved]; #201: int; #212: int;
#214: int; #216: (int | string);
File "erling_loop5.php", line 34, characters 5-17:[2]
local_types
$obj: X as X<#223(int | string)>
$x: (string | int)
$y: (int | string)
$z: (int | string)
$i: int
subst(changes)
#223: #223; #216: #219; #219: #221; #221: #223;
tenv(changes)
#219: (string | int); #221: (int | string); #223: (int | string);
```
Reviewed By: dabek
Differential Revision: D4068570
fbshipit-source-id: cb96d5178bc41f73a8ddd8776ab7f91f69de2b71
Enable type checker to correctly type non-standard length properties.
Fixes issue #131.