Skip to content

Commit

Permalink
Band-aid for preventMunge in cycle
Browse files Browse the repository at this point in the history
Summary:
When the leader of a cycle has preventMunge then munged properties in other members of the are erroneously checked by assert_ground.

This diff is a band-aid to fix that problem by explicitly passing `should_munge_underscores` from the cycle members directly to `assert_ground`.

Any other file-specific configurations will suffer from this same problem in all merge step phases. The more principled long-term solution is to set up the types of the contexts so that you cannot access file specific information during merge.

Reviewed By: gabelevi

Differential Revision: D17398933

fbshipit-source-id: 6aca1054684cdbadced0273232002c7f30991508
  • Loading branch information
jbrown215 authored and facebook-github-bot committed Sep 17, 2019
1 parent 3c426a0 commit fc39888
Show file tree
Hide file tree
Showing 9 changed files with 36 additions and 8 deletions.
11 changes: 7 additions & 4 deletions src/typing/assert_ground.ml
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class type_finder t =
module Kit (Flow : Flow_common.S) : Flow_common.ASSERT_GROUND = struct
include Flow

class assert_ground_visitor r ~max_reasons =
class assert_ground_visitor r ~max_reasons ~should_munge_underscores =
object (self)
inherit [Marked.t] Type_visitor.t as super

Expand Down Expand Up @@ -289,7 +289,7 @@ module Kit (Flow : Flow_common.S) : Flow_common.ASSERT_GROUND = struct
let props = Context.find_props cx id in
SMap.fold
(fun x p acc ->
if is_munged_prop_name cx x then
if is_munged_prop_name_with_munge x ~should_munge_underscores then
acc
else if SSet.mem x init then
Property.read_t p |> Option.fold ~f:(self#type_ cx pole) ~init:acc
Expand Down Expand Up @@ -357,9 +357,12 @@ module Kit (Flow : Flow_common.S) : Flow_common.ASSERT_GROUND = struct
seen
end

let enforce_strict cx t =
let enforce_strict cx t ~should_munge_underscores =
let visitor =
new assert_ground_visitor (reason_of_t t) ~max_reasons:(Context.max_trace_depth cx)
new assert_ground_visitor
(reason_of_t t)
~max_reasons:(Context.max_trace_depth cx)
~should_munge_underscores
in
let seen = visitor#type_ cx Polarity.Positive Marked.empty t in
ignore (seen : Marked.t)
Expand Down
4 changes: 3 additions & 1 deletion src/typing/flow_common.ml
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ open Type
(* This is here instead of assert_ground.ml to avoid the duplication of the enforce strict
* signature *)
module type ASSERT_GROUND = sig
val enforce_strict : Context.t -> Type.t -> unit
val enforce_strict : Context.t -> Type.t -> should_munge_underscores:bool -> unit
end

module type TRUST_CHECKING = sig
Expand Down Expand Up @@ -64,6 +64,8 @@ module type S = sig

val is_munged_prop_name : Context.t -> name -> bool

val is_munged_prop_name_with_munge : name -> should_munge_underscores:bool -> bool

val lookup_builtin :
Context.t -> ?trace:Trace.t -> string -> reason -> Type.lookup_kind -> Type.t -> unit

Expand Down
7 changes: 6 additions & 1 deletion src/typing/flow_js.ml
Original file line number Diff line number Diff line change
Expand Up @@ -9027,7 +9027,12 @@ struct
* the `munge_underscores` config option is set.
*)
and is_munged_prop_name cx name =
Context.should_munge_underscores cx && Signature_utils.is_munged_property_name name
is_munged_prop_name_with_munge
name
~should_munge_underscores:(Context.should_munge_underscores cx)

and is_munged_prop_name_with_munge name ~should_munge_underscores =
Signature_utils.is_munged_property_name name && should_munge_underscores

and lookup_prop cx trace l reason_prop reason_op strict x action =
let l =
Expand Down
2 changes: 1 addition & 1 deletion src/typing/flow_js.mli
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ val mk_typeof_annotation :
(* strict *)
val types_of : Constraint.constraints -> Type.t list

val enforce_strict : Context.t -> Type.t -> unit
val enforce_strict : Context.t -> Type.t -> should_munge_underscores:bool -> unit

val possible_types : Context.t -> Constraint.ident -> Type.t list

Expand Down
5 changes: 4 additions & 1 deletion src/typing/merge_js.ml
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,10 @@ let detect_invalid_type_assert_calls cx file_sigs cxs tasts =
let force_annotations leader_cx other_cxs =
Core_list.iter
~f:(fun cx ->
Context.module_ref cx |> Flow_js.lookup_module leader_cx |> Flow_js.enforce_strict leader_cx)
let should_munge_underscores = Context.should_munge_underscores cx in
Context.module_ref cx
|> Flow_js.lookup_module leader_cx
|> Flow_js.enforce_strict leader_cx ~should_munge_underscores)
(leader_cx :: other_cxs)

let is_builtin_or_flowlib cx =
Expand Down
2 changes: 2 additions & 0 deletions tests/munge_underscores_assert_ground_cycle/.flowconfig
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[options]
munge_underscores=true
4 changes: 4 additions & 0 deletions tests/munge_underscores_assert_ground_cycle/a.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
//@flow
//@preventMunge
const b = require('./b');
module.exports = b.x;
8 changes: 8 additions & 0 deletions tests/munge_underscores_assert_ground_cycle/b.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
//@flow
const a = require('./a');
class A {
static x: number = 3;
_munged(arg) { return a; }
}

module.exports = A;
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Found 0 errors

0 comments on commit fc39888

Please sign in to comment.