Skip to content

Commit

Permalink
Abnormals should pop lex scope in catch clause
Browse files Browse the repository at this point in the history
This resolves an issue which I misdiagnosed, causing me to add
unnecessary cases to Env_js.merge_env, which could mask other real
errors.

What's happening here: We enter a catch clause, which pushes a new lex
scope. Before we can pop the lex scope, we hit an abnormal and jump
away. Then, we merge the env that has a dangling lex scope on top.

In general, we run this risk whenever we push a lex scope and then
process statements that might raise. I have convinced myself that no
other code that calls merge_env will hit this specific case, but a
generalized approach would be nice, too.
  • Loading branch information
samwgoldman committed Sep 9, 2015
1 parent b3f9ba7 commit 2c04e7f
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 7 deletions.
5 changes: 0 additions & 5 deletions src/typing/env_js.ml
Original file line number Diff line number Diff line change
Expand Up @@ -765,11 +765,6 @@ let merge_env =
()
| None, Some e, None when is_lex scope1 && Entry.is_lex e ->
()
(* walk through uneven lex scopes *)
| Some _, Some _, None when is_lex scope2 ->
merge_entry cx reason (env0, env1, env2_) name
| Some _, None, Some _ when is_lex scope1 ->
merge_entry cx reason (env0, env1_, env2) name
(* otherwise, non-refinement uneven distributions are asserts. *)
| orig, child1, child2 ->
let print_entry_kind_opt = function
Expand Down
9 changes: 7 additions & 2 deletions src/typing/type_inference_js.ml
Original file line number Diff line number Diff line change
Expand Up @@ -1350,8 +1350,13 @@ and statement cx type_params_map = Ast.Statement.(
Env_js.push_lex ();
Scope.Entry.(Env_js.bind_implicit_let
~state:Initialized CatchParamBinding cx name t r);
List.iter (statement_decl cx type_params_map) b.Block.body;
toplevels cx type_params_map b.Block.body;
Abnormal.exception_handler
(fun () ->
List.iter (statement_decl cx type_params_map) b.Block.body;
toplevels cx type_params_map b.Block.body)
(fun exn ->
Env_js.pop_lex ();

This comment has been minimized.

Copy link
@bhosmer

bhosmer Sep 9, 2015

Contributor

Yes! Perfect. This is an occupational hazard when using exceptions as part of the normal control flow - similar problems show up in the speculative plumbing for unions and intersections in flow_js.

Oops, not quite perfect... I think you need to pop back to the topmost VarScope, right?

This comment has been minimized.

Copy link
@bhosmer

bhosmer Sep 9, 2015

Contributor

Ugh, no it's worse than that. You need to remember the LexScope you pushed on 1350, and pop back to (and including) that. Because a) the try/catch might of course be in a lex scope, and more importantly b) an arbitrary stack of lex and var scopes might have been pushed at the point where the throw happens.

This comment has been minimized.

Copy link
@samwgoldman

samwgoldman Sep 9, 2015

Author Member

I don't think so. We catch the exception right outside the sole call to catch_clause, so we should unwrap to the right place. We just need to be careful to stay balanced against the push_lex ~8 lines up.

If you're right, it should be simple to come up with a failing test, but I'm not able to. Can you?

This comment has been minimized.

Copy link
@samwgoldman

samwgoldman Sep 9, 2015

Author Member

(What you're saying in your second comment does make sense, now that I see you're referring to possibly pushed lex scopes inside of b.Block.body... still can't make Flow blow up on it though.)

This comment has been minimized.

Copy link
@bhosmer

bhosmer Sep 9, 2015

Contributor

I'm having a hard time visualizing a way to provoke an immediate blowup. The problem would be dead layers piling up. I guess a test would be something that got wrecked by merging the dead scopes after the try/catch... but easier would just be to print something that showed the scopes piling up, maybe the depth of the scope stack.

Wondering if this unwinding is needed in the other places where exceptions are used to trap control flow as well.

(When I wrote the comment I was thinking that a throw from the middle of a local function might also contribute a dead VarScope, but on second thought, control constructs wouldn't throw past a function boundary. So I do think we're talking about LexScopes only.)

Abnormal.raise_exn exn);
Env_js.pop_lex ()

| loc, Identifier (_, { Ast.Identifier.name; _ }) ->
Expand Down

0 comments on commit 2c04e7f

Please sign in to comment.