Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Let/const #431

Closed
wants to merge 46 commits into from
Closed

Let/const #431

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
f962a63
Change block type to record with scope type information
samwgoldman Apr 22, 2015
f5c56c4
Add block-scoped let variables
samwgoldman Apr 22, 2015
2b27342
Allow const variable declarations
samwgoldman Apr 24, 2015
309d0da
Add outstanding let/const features as TODO tests
samwgoldman Apr 25, 2015
760c1e9
Const variables must be initialized
samwgoldman Apr 25, 2015
d07d944
Use scope_kind variant instead of hoist boolean
samwgoldman Apr 29, 2015
87a59ff
Simplify flat_env inner loop definition
samwgoldman Apr 29, 2015
8fb878c
Add pending test for class declaration scope
samwgoldman Apr 29, 2015
d12e8bf
For loops introduce a lexical scope for initialized variables
samwgoldman Apr 30, 2015
222d411
Fix inaccurate comment
samwgoldman Apr 30, 2015
6642876
Add explicit tests for hoisting edge cases
samwgoldman Apr 30, 2015
5e063da
Create new lexical scope let-bound variables in for-in loops
samwgoldman Apr 30, 2015
c6ddef1
Classes are scoped lexically
samwgoldman Apr 30, 2015
6ed936c
Functions are indeed var-scoped
samwgoldman Apr 30, 2015
03c8a4b
Type aliases should be var-scoped
samwgoldman May 1, 2015
40fca43
Classes and interfaces are declared in a lexical scope
samwgoldman May 1, 2015
d1d3b32
Declared variables and functions are var-scoped
samwgoldman May 1, 2015
99e4262
Modules and imports are OK as VarScope
samwgoldman May 1, 2015
882c81a
Merge branch 'master' into es6-let-const-2
samwgoldman May 7, 2015
bbf8193
Merge branch 'master' into es6-let-const-2
samwgoldman May 20, 2015
4659a41
Install member expression refinments in the appropriate scope
samwgoldman May 20, 2015
fb281a2
Merge branch 'master' into es6-let-const-2
samwgoldman May 23, 2015
5c27d4d
Env_js.set_var should not create new entries
samwgoldman May 23, 2015
3c8267d
Add scope_kind field to scope_entry record
samwgoldman May 23, 2015
f109936
Remove redundant scope_kind argument to init_env
samwgoldman May 23, 2015
ab4dd92
TDZ: Prevent use of lexical declaration before initialization
samwgoldman May 23, 2015
c9594e2
Block-scoped variables still hoist
samwgoldman May 24, 2015
222dc44
Return scope along with entry from exists_in_env
samwgoldman May 24, 2015
cc30bd1
Let bindings can not shadow each other in a lexical scope
samwgoldman May 24, 2015
6ad275a
Switch cases are surrounded by a fresh lexical scope
samwgoldman May 24, 2015
f321c2d
Move undeclared const test to its own file
samwgoldman May 24, 2015
614057a
Const bindings can only be declared and assigned once
samwgoldman May 24, 2015
b02c045
Merge branch 'master' into es6-let-const-2
samwgoldman May 30, 2015
dde9473
Clarify assignment_lhs
samwgoldman May 30, 2015
6340fae
Merge branch 'master' into es6-let-const-2
samwgoldman Jun 2, 2015
74934d2
Create lexical scope around for-of loops
samwgoldman Jun 2, 2015
8db94d6
const bindings are valid LHS in ForIn and ForOf loops
samwgoldman Jun 2, 2015
888beed
Fix a few more const bugs
samwgoldman Jun 3, 2015
f0ddfd3
Improve error messages for let/const redeclaration and const assignment
samwgoldman Jun 3, 2015
7bab756
Error on update and assign ops for constant bindings
samwgoldman Jun 3, 2015
ff0c88c
Refactor away boolean blindness
samwgoldman Jun 3, 2015
273fa6f
Rename heap refinement functions to be more specific
samwgoldman Jun 3, 2015
1548e00
Separate concepts of binding_type and scope_kind
samwgoldman Jun 3, 2015
f36b6c1
Do less funky index work in heap_refinement
samwgoldman Jun 3, 2015
47c1e69
Add comments about the use of scope_kind and binding_type
samwgoldman Jun 3, 2015
9e6c28d
Replace for_type flag on scope entries with new binding type
samwgoldman Jun 3, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions src/typing/constraint_js.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,7 +507,16 @@ type scope_entry = {
def_loc: Spider_monkey_ast.Loc.t option;
for_type: bool;
}
type scope = scope_entry SMap.t ref

type scope_kind =
| VarScope (* var, functions hoisted up to this point *)
| LexicalScope (* let, const, classes *)

type scope = {
kind: scope_kind;
entries: scope_entry SMap.t ref;
}

type stack = int list

let create_env_entry ?(for_type=false) specific general loc =
Expand Down Expand Up @@ -1988,8 +1997,9 @@ let string_of_scope_entry cx entry =
entry.for_type

let string_of_scope cx scope =
let entries = scope.entries in
SMap.fold (fun k v acc ->
(Utils.spf "%s: %s" k (string_of_scope_entry cx v))::acc
) !scope []
) !entries []
|> String.concat ";\n "
|> Utils.spf "{\n %s\n}"
11 changes: 10 additions & 1 deletion src/typing/constraint_js.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,16 @@ type scope_entry = {
def_loc: Spider_monkey_ast.Loc.t option;
for_type: bool;
}
type scope = scope_entry SMap.t ref

type scope_kind =
| VarScope (* var, functions hoisted up to this point *)
| LexicalScope (* let, const, classes *)

type scope = {
kind: scope_kind;
entries: scope_entry SMap.t ref;
}

type stack = int list

val create_env_entry :
Expand Down
184 changes: 129 additions & 55 deletions src/typing/env_js.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,10 @@ let swap_changeset f =
changeset := newset;
oldset

let global_scope: scope = ref SMap.empty
let global_scope: scope = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair warning, the env/global_scope situation was tweaked recently (actually, it's the stuff @mroch did that makes it possible to refine globals). Shouldn't be too hard to merge, but nontrivial.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Err, I already merged that locally. Thought I pushed, but apparently I didn't. I'll push that up now.

kind = VarScope;
entries = ref SMap.empty;
}

let global_poison = ["eval"; "arguments"]
let global_lexicals = [
Expand All @@ -60,12 +63,14 @@ let cache_global cx x reason =
else
Flow_js.get_builtin cx x reason
)) in
global_scope := SMap.add x (create_env_entry t t None) !global_scope;
let global_entries = global_scope.entries in
global_entries := SMap.add x (create_env_entry t t None) !global_entries;
cx.globals <- SSet.add x cx.globals;
global_scope

let find_global cx x reason =
if (SMap.mem x !global_scope)
let global_entries = global_scope.entries in
if (SMap.mem x !global_entries)
then
global_scope
else
Expand All @@ -75,33 +80,58 @@ let find_env ?(for_type=false) cx x reason =
let rec loop = function
| [] -> find_global cx x reason
| scope::scopes ->
(match SMap.get x !scope with
let entries = scope.entries in
(match SMap.get x !entries with
| Some entry when (not entry.for_type || for_type) -> scope
| _ -> loop scopes)
in
loop (!env)

let get_from_scope x scope =
SMap.find_unsafe x !scope
let entries = scope.entries in
SMap.find_unsafe x !entries

let set_in_scope x entry scope =
scope := !scope |> SMap.add x entry
let entries = scope.entries in
entries := !entries |> SMap.add x entry

let exists_in_scope x scope =
SMap.get x !scope
let entries = scope.entries in
SMap.get x !entries

let set_in_env x entry scope_kind =
let rec loop = function
| [] -> set_in_scope x entry global_scope
| scope::scopes ->
if (scope_kind = LexicalScope || scope.kind = scope_kind) then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this cause vars to get set in the first lexical scope they hit (if they hit a lexical scope first)?

Thinking about this a bit, I actually think we need to introduce one LexicalScope directly under each VarScope when we create the scopes. Consider these test cases:

let thing = 42;

function foo() {
  let thing = 42; // This gets scoped to the function env
}

var thing = 42;

while (cond) {
  var thing = 43;
}

thing; // 43

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, if the entry is var-scoped, then scope_kind will be VarScope, so the condition will fail.

Also, this same condition ensures that let-scoped variables will be installed into any kind of scope. (That's what the first part is all about, the second part ensures VarScope entries are installed only into VarScope scopes.)

You'll find that both of those test cases work today with the existing changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I think you're right -- I was getting scope_kind and scope.kind reversed in my head. Cool

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mind adding some [regression] tests covering those scenarios at some point?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do. I'm confident existing tests indirectly cover this stuff, but it doesn't hurt to be explicit.

set_in_scope x entry scope
else
loop scopes in
loop (!env)

let exists_in_env x scope_kind =
let rec loop = function
| [] -> exists_in_scope x global_scope
| scope::scopes ->
if (scope_kind = LexicalScope || scope.kind = scope_kind) then
exists_in_scope x scope
else
loop scopes in
loop (!env)

let update_scope ?(for_type=false) x (specific_t, general_t) scope =
let values = !scope in
let new_entry = match SMap.get x values with
let entries = scope.entries in
let new_entry = match SMap.get x !entries with
| Some {def_loc; for_type; _;} ->
create_env_entry ~for_type specific_t general_t def_loc
| None ->
create_env_entry ~for_type specific_t general_t None
in
scope := values |> SMap.add x new_entry
entries := !entries |> SMap.add x new_entry

let unset_in_scope x scope =
scope := !scope |> SMap.remove x
let entries = scope.entries in
entries := !entries |> SMap.remove x

let read_env ?(for_type=false) cx x reason =
find_env ~for_type cx x reason |> get_from_scope x
Expand All @@ -116,9 +146,11 @@ let pop_env () =
env := List.tl !env

let flat_env () =
List.fold_left (fun acc x ->
SMap.union acc !x
) !global_scope !env
let rec loop acc = function
| { entries; _ }::bs -> loop (SMap.union acc !entries) bs
| [] -> acc in
let global_entries = global_scope.entries in
loop !global_entries !env

let switch_env scope =
pop_env ();
Expand All @@ -127,10 +159,9 @@ let switch_env scope =
let peek_env () =
List.hd !env

let init_env cx x shape =
let scope = peek_env() in
match exists_in_scope x scope with
| None -> set_in_scope x shape scope
let init_env cx x shape scope_kind =
match exists_in_env x scope_kind with
| None -> set_in_env x shape scope_kind
| Some { general; for_type; def_loc; _; } when for_type <> shape.for_type ->
(* When we have a value var shadowing a type var, replace the type var
* with the value var *)
Expand All @@ -146,7 +177,7 @@ let init_env cx x shape =
shadower_reason, "This binding is shadowing";
shadowed_reason, "which is a different sort of binding";
];
set_in_scope x shape scope;
set_in_env x shape scope_kind;
Flow_js.unify cx shape.general general
| Some { general; _ } ->
Flow_js.unify cx general shape.general
Expand Down Expand Up @@ -179,8 +210,7 @@ let var_ref ?(for_type=false) cx x reason =
mod_reason_of_t (repos_reason p) t

let get_refinement cx key r =
let scope = peek_env () in
match exists_in_scope key scope with
match exists_in_env key VarScope with
| Some { specific; _ } ->
let pos = pos_of_reason r in
let t = mod_reason_of_t (repos_reason pos) specific in
Expand All @@ -195,7 +225,10 @@ let set_var ?(for_type=false) cx x specific_t reason =
write_env ~for_type cx x (specific_t, general) reason

let clone_env ctx =
List.map (fun scope -> ref !scope) ctx
List.map (fun scope ->
let { kind; entries } = scope in
{ kind; entries = ref !entries }
) ctx

let update_frame cx ctx =
let current_frame = List.hd !Flow_js.frames in
Expand All @@ -214,12 +247,15 @@ let rec merge_env cx reason (ctx, ctx1, ctx2) changeset =
match (ctx, ctx1, ctx2) with
| ([],[],[]) -> ()
| (scope::ctx, scope1::ctx1, scope2::ctx2) ->
let entries = scope.entries in
let entries1 = scope1.entries in
let entries2 = scope2.entries in
(* merge scope, scope1, scope2 *)
let not_found = SSet.fold (fun x not_found ->
match SMap.get x !scope with
match SMap.get x !entries with
| Some {specific;general;def_loc;for_type;} ->
let shape1 = SMap.find_unsafe x !scope1 in
let shape2 = SMap.find_unsafe x !scope2 in
let shape1 = SMap.find_unsafe x !entries1 in
let shape2 = SMap.find_unsafe x !entries2 in
let s1 = shape1.specific in
let s2 = shape2.specific in
let (s, g) =
Expand All @@ -236,7 +272,7 @@ let rec merge_env cx reason (ctx, ctx1, ctx2) changeset =
(tvar,general)
in
let for_type = for_type || shape1.for_type || shape2.for_type in
scope := SMap.add x (create_env_entry ~for_type s g def_loc) !scope;
scope.entries := SMap.add x (create_env_entry ~for_type s g def_loc) !entries;
not_found
| None ->
SSet.add x not_found
Expand All @@ -246,26 +282,35 @@ let rec merge_env cx reason (ctx, ctx1, ctx2) changeset =
| _ -> assert false

let widen_env cx reason =
let scope = List.hd !env in
let vars = !scope in
if SMap.cardinal vars < 32 then
scope := vars |> SMap.mapi (fun x {specific;general;def_loc;for_type;} ->
if specific = general
then create_env_entry ~for_type specific general def_loc
else
let reason = replace_reason x reason in
let tvar = Flow_js.mk_tvar cx reason in
Flow_js.unit_flow cx (specific,tvar);
Flow_js.unit_flow cx (tvar,general);
create_env_entry ~for_type tvar general def_loc
)
let rec loop = function
| [] -> ()
| scope::scopes ->
let entries = scope.entries in
let entries = !entries in
if SMap.cardinal entries < 32 then
scope.entries := entries |> SMap.mapi (fun x {specific;general;def_loc;for_type;} ->
if specific = general
then create_env_entry ~for_type specific general def_loc
else
let reason = replace_reason x reason in
let tvar = Flow_js.mk_tvar cx reason in
Flow_js.unit_flow cx (specific,tvar);
Flow_js.unit_flow cx (tvar,general);
create_env_entry ~for_type tvar general def_loc
);
if scope.kind = VarScope then ()
else loop scopes
in
loop !env

let rec copy_env_ cx reason x = function
| ([],[]) -> ()
| (scope1::ctx1, scope2::ctx2) ->
(match SMap.get x !scope1 with
let entries1 = scope1.entries in
let entries2 = scope2.entries in
(match SMap.get x !entries1 with
| Some {specific=s1;_} ->
let s2 = (SMap.find_unsafe x !scope2).specific in
let s2 = (SMap.find_unsafe x !entries2).specific in
Flow_js.unit_flow cx (s2,s1)
| None -> copy_env_ cx reason x (ctx1,ctx2)
)
Expand All @@ -281,16 +326,18 @@ let copy_env cx reason (ctx1,ctx2) xs =

let havoc_env () =
List.iter (fun scope ->
scope := SMap.mapi (fun x {specific=_;general;def_loc;for_type;} ->
let entries = scope.entries in
entries := SMap.mapi (fun x {specific=_;general;def_loc;for_type;} ->
create_env_entry ~for_type general general def_loc
) !scope
) !entries
) !env

let rec havoc_env2_ x = function
| scope::scopes ->
(match SMap.get x !scope with
let entries = scope.entries in
(match SMap.get x !entries with
| Some {specific=_;general;def_loc;for_type;} ->
scope := !scope |>
entries := !entries |>
SMap.add x (create_env_entry ~for_type general general def_loc)
| None ->
havoc_env2_ x scopes
Expand All @@ -302,21 +349,48 @@ let havoc_env2 xs =

let havoc_heap_refinements () =
List.iter (fun scope ->
scope := SMap.mapi (fun x ({specific=_;general;def_loc;for_type;} as entry) ->
let entries = scope.entries in
entries := SMap.mapi (fun x ({specific=_;general;def_loc;for_type;} as entry) ->
if is_refinement x
then create_env_entry ~for_type general general def_loc
else entry
) !scope
) !entries
) !env

let clear_env reason =
let scope = List.hd !env in
scope := !scope |> SMap.mapi (fun x {specific;general;def_loc;for_type;} ->
(* internal names (.this, .super, .return, .exports) are read-only *)
if is_internal_name x
then create_env_entry ~for_type specific general def_loc
else create_env_entry ~for_type (UndefT reason) general def_loc
)
let rec loop = function
| [] -> ()
| scope::scopes ->
let entries = scope.entries in
entries := !entries |> SMap.mapi (fun x {specific;general;def_loc;for_type;} ->
(* internal names (.this, .super, .return, .exports) are read-only *)
if is_internal_name x
then create_env_entry ~for_type specific general def_loc
else create_env_entry ~for_type (UndefT reason) general def_loc
);
if scope.kind = VarScope then ()
else loop scopes
in
loop !env

let string_of_scope_entry cx entry =
let pos = match entry.def_loc with
| Some loc -> (string_of_pos (pos_of_loc loc))
| None -> "(none)"
in
Utils.spf "{ specific: %s; general: %s; def_loc: %s; for_type: %b }"
(dump_t cx entry.specific)
(dump_t cx entry.general)
pos
entry.for_type

let string_of_scope cx scope =
let entries = scope.entries in
SMap.fold (fun k v acc ->
(Utils.spf "%s: %s" k (string_of_scope_entry cx v))::acc
) !entries []
|> String.concat ";\n "
|> Utils.spf "{\n %s\n}"

let string_of_env cx ctx =
String.concat "\n" (List.map (string_of_scope cx) ctx)
Expand All @@ -327,7 +401,7 @@ let string_of_env cx ctx =
let install_refinement cx x xtypes =
if exists_in_scope x (peek_env ()) = None then
let t = SMap.find_unsafe x xtypes in
init_env cx x (create_env_entry t t None)
init_env cx x (create_env_entry t t None) VarScope (* TODO: verify correct hoist behavior *)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure that the scope of the refinement depends on the type of entry corresponding to x. Furthermore, code that relies on exists_in_scope/peek_env instead of exists_in_env/scope_kind is generally suspect.

Unfortunately, we don't generally know the scope kind of the entry here, and I'm not even sure we can thread it through. The refinement logic is still a bit opaque to me, but I'll try to come up with some test cases to stress this code. Help here would be appreciated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so I think I have two tests that show the refinement scope needs to depend on the scope of the thing being refined.

Here is why just VarScope doesn't work:

function scope() {
  {
    let x: {p:?string} = {p:"xxx"};

    if (x.p == null) {
      return;
    }
  }
  x.p // should fail, but $REFI x.p is hoisted to here, so there is no warning
}

Here is why just LexicalScope doesn't work:

function scope() {
  var x: {p:?string} = {p:"xxx"};
  {
    if (x.p == null) {
      return;
    }
  }
  var y: string = x.p // should pass, but refinement doesn't live long enough
}

@gabelevi, you helped me understand this refinement stuff in the first place (thanks!). Do you see any problem with my reasoning here? It seems like the scope_kind of the refinement needs to depend on the scope_kind of the thing being refined (x in this case).

If that's true, we currently don't store that information beyond init_env, so I believe I'd need to add scope_kind to the scope_entry struct. (If true, this isn't so bad, because I think we need to add this kind of information to support TDZ use cases anyway.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've implemented this in 4659a41. Mostly sure it's correct :)


let refine_with_pred cx reason pred xtypes =
SMap.iter (fun x predx ->
Expand Down
2 changes: 1 addition & 1 deletion src/typing/env_js.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ val var_ref : ?for_type:bool -> context -> string -> reason -> Type.t

val set_var : ?for_type:bool -> context -> string -> Type.t -> reason -> unit

val init_env : context -> string -> scope_entry -> unit
val init_env : context -> string -> scope_entry -> scope_kind -> unit

val clone_env : scope list -> scope list

Expand Down
5 changes: 3 additions & 2 deletions src/typing/flow_js.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,12 +200,13 @@ let rec havoc_ctx cx i j =
and havoc_ctx_ = function
| (scope::scopes_, x1::stack1_, x2::stack2_) when x1 = x2 ->
(if modes.verbose then prerr_endlinef "HAVOC::%d" x1);
scope := SMap.mapi (fun x {specific;general;def_loc;for_type} ->
let entries = scope.entries in
entries := SMap.mapi (fun x {specific;general;def_loc;for_type} ->
(* internal names (.this, .super, .return, .exports) are read-only *)
if is_internal_name x
then create_env_entry ~for_type specific general def_loc
else create_env_entry ~for_type general general def_loc
) !scope;
) !entries;
havoc_ctx_ (scopes_,stack1_,stack2_)
| _ -> ()

Expand Down
Loading