Skip to content

Commit d6e6421

Browse files
ruippeixotogfacebook-github-bot
ruippeixotog
authored and
facebook-github-bot
committed
Add custom eqWAlizer type checking for maps:merge
Summary: `maps:merge` currently loses all type information of the maps being merged. On this diff I'm adding custom handling of that function to deal with more map manipulation cases in WASERVER. EqWAlizer already had some nice utilities to merge map types, but the existing implementation was a bit confusing and it the merging logic didn't assume any overwrite behavior (in `maps:merge`, when there are conflicting keys the right-ahnd side map's values are used). I expanded the existing merge logic to support this case, making the code more readable (I hope?) in the process. Reviewed By: VLanvin Differential Revision: D59677724 fbshipit-source-id: d0664130eda5e607a92aa110b221d83db95f6a19
1 parent 3ff553c commit d6e6421

File tree

9 files changed

+115
-25
lines changed

9 files changed

+115
-25
lines changed

‎eqwalizer/src/main/resources/application.conf

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,4 +21,6 @@ eqwalizer {
2121
clause_coverage = ${?EQWALIZER_CLAUSE_COVERAGE}
2222
overloaded_spec_dynamic_result = false
2323
overloaded_spec_dynamic_result = ${?EQWALIZER_OVERLOADED_SPEC_DYNAMIC_RESULT}
24+
custom_maps_merge = false
25+
custom_maps_merge = ${?EQWALIZER_CUSTOM_MAPS_MERGE}
2426
}

‎eqwalizer/src/main/scala/com/whatsapp/eqwalizer/package.scala

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ package object eqwalizer {
4848
checkRedundantGuards: Boolean,
4949
clauseCoverage: Boolean,
5050
overloadedSpecDynamicResult: Boolean,
51+
customMapsMerge: Boolean,
5152
mode: Mode.Mode,
5253
errorDepth: Int,
5354
) {
@@ -80,6 +81,7 @@ package object eqwalizer {
8081
checkRedundantGuards = config.getBoolean("check_redundant_guards"),
8182
clauseCoverage = config.getBoolean("clause_coverage"),
8283
overloadedSpecDynamicResult = config.getBoolean("overloaded_spec_dynamic_result"),
84+
customMapsMerge = config.getBoolean("custom_maps_merge"),
8385
mode,
8486
errorDepth = config.getInt("error_depth"),
8587
)

‎eqwalizer/src/main/scala/com/whatsapp/eqwalizer/tc/ElabApplyCustom.scala

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,11 @@ class ElabApplyCustom(pipelineContext: PipelineContext) {
5050
RemoteId("maps", "with", 2),
5151
RemoteId("maps", "without", 2),
5252
RemoteId(CompilerMacro.fake_module, "record_info", 2),
53-
)
53+
) ++ experimentalCustom
54+
55+
private def experimentalCustom: Set[RemoteId] =
56+
if (pipelineContext.customMapsMerge) Set(RemoteId("maps", "merge", 2))
57+
else Set()
5458

5559
private lazy val customPredicate: Set[RemoteId] =
5660
Set(RemoteId("lists", "member", 2))
@@ -458,6 +462,15 @@ class ElabApplyCustom(pipelineContext: PipelineContext) {
458462
val pairTys = narrow.asMapType(coerce(args.head, argTys.head, anyMapTy))
459463
(mapToList(pairTys), env1)
460464

465+
case RemoteId("maps", "merge", 2) =>
466+
val List(mapTy1, mapTy2) = args
467+
.zip(argTys)
468+
.map { case (arg, ty) => narrow.asMapType(coerce(arg, ty, anyMapTy)) }
469+
val resMapTys = util
470+
.cartesianProduct(mapTy1, mapTy2)
471+
.map { case (ty1, ty2) => narrow.joinAndMergeShapes(List(ty1, ty2), true) }
472+
(subtype.join(resMapTys), env1)
473+
461474
case RemoteId("maps", "with", 2) =>
462475
@tailrec
463476
def toKey(ty: Type)(implicit pos: Pos): Option[String] = ty match {

‎eqwalizer/src/main/scala/com/whatsapp/eqwalizer/tc/Narrow.scala

Lines changed: 24 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,6 @@ import com.whatsapp.eqwalizer.ast.Forms.RecDeclTyped
1010
import com.whatsapp.eqwalizer.ast.{RemoteId, TypeVars}
1111
import com.whatsapp.eqwalizer.ast.Types._
1212

13-
import scala.collection.mutable
14-
1513
class Narrow(pipelineContext: PipelineContext) {
1614
private val subtype = pipelineContext.subtype
1715
private val util = pipelineContext.util
@@ -606,30 +604,32 @@ class Narrow(pipelineContext: PipelineContext) {
606604
case _ => None
607605
}
608606

609-
private def mergeShapes(s1: ShapeMap, s2: ShapeMap): ShapeMap = {
610-
var optProps = mutable.HashMap.empty[String, Type]
611-
var reqProps = mutable.HashMap.empty[String, Type]
612-
val commonReqKeys = s1.props.collect { case ReqProp(key, _) => key }.toSet & s2.props.collect {
613-
case ReqProp(key, _) => key
614-
}.toSet
615-
for (p <- s1.props.toSet ++ s2.props.toSet) {
616-
p match {
617-
case OptProp(key, tp) => optProps.updateWith(key)(ty => Some(subtype.join(tp, ty.getOrElse(NoneType))))
618-
case ReqProp(key, tp) =>
619-
if (commonReqKeys.contains(key)) {
620-
reqProps.updateWith(key)(ty => Some(subtype.join(tp, ty.getOrElse(NoneType))))
621-
} else {
622-
optProps.updateWith(key)(ty => Some(subtype.join(tp, ty.getOrElse(NoneType))))
623-
}
624-
}
607+
private def mergeShapes(s1: ShapeMap, s2: ShapeMap, inOrder: Boolean): ShapeMap = {
608+
ShapeMap {
609+
(s1.props ++ s2.props)
610+
.groupBy(_.key)
611+
.values
612+
.map {
613+
// prop is only defined in one of the maps
614+
case List(p) if inOrder => p
615+
case List(p) => OptProp(p.key, p.tp)
616+
// prop is optional on both sides
617+
case List(OptProp(key, tp1), OptProp(_, tp2)) => OptProp(key, subtype.join(tp1, tp2))
618+
// prop is required on both sides
619+
case List(ReqProp(key, tp1), ReqProp(_, tp2)) if inOrder => ReqProp(key, tp2)
620+
case List(ReqProp(key, tp1), ReqProp(_, tp2)) => ReqProp(key, subtype.join(tp1, tp2))
621+
// prop is required on one side and optional on the other
622+
case List(OptProp(key, tp1), ReqProp(_, tp2)) if inOrder => ReqProp(key, tp2)
623+
case List(ReqProp(key, tp1), OptProp(_, tp2)) if inOrder => ReqProp(key, subtype.join(tp1, tp2))
624+
case List(p1, p2) => OptProp(p1.key, subtype.join(p1.tp, p2.tp))
625+
626+
case _ => throw new IllegalStateException()
627+
}
628+
.toList
625629
}
626-
val allProps = optProps.map { case (k, t) => OptProp(k, t) }.toList ++ reqProps.map { case (k, t) =>
627-
ReqProp(k, t)
628-
}.toList
629-
ShapeMap(allProps)
630630
}
631631

632-
def joinAndMergeShapes(tys: Iterable[Type]): Type = {
632+
def joinAndMergeShapes(tys: Iterable[Type], inOrder: Boolean = false): Type = {
633633
val (shapes, notShapes) = tys.partition {
634634
case s: ShapeMap => true
635635
case _ => false
@@ -640,7 +640,7 @@ class Narrow(pipelineContext: PipelineContext) {
640640
joinedNotShapes
641641
} else {
642642
subtype.join(
643-
shapesCoerced.tail.foldLeft(shapesCoerced.head)((acc, shape) => mergeShapes(acc, shape)),
643+
shapesCoerced.tail.foldLeft(shapesCoerced.head)((acc, shape) => mergeShapes(acc, shape, inOrder)),
644644
joinedNotShapes,
645645
)
646646
}

‎eqwalizer/src/main/scala/com/whatsapp/eqwalizer/tc/Util.scala

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,4 +139,12 @@ class Util(pipelineContext: PipelineContext) {
139139
case BoundedDynamicType(_) => true
140140
case _ => false
141141
}
142+
143+
def cartesianProduct(ty1: Type, ty2: Type): List[(Type, Type)] = {
144+
def expand(ty: Type): List[Type] = ty match {
145+
case UnionType(tys) => tys.toList.flatMap(expand)
146+
case ty => List(ty)
147+
}
148+
for (t1 <- expand(ty1); t2 <- expand(ty2)) yield (t1, t2)
149+
}
142150
}

‎eqwalizer/src/main/scala/com/whatsapp/eqwalizer/tc/package.scala

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,5 +74,6 @@ package object tc {
7474
options.errorDepth.getOrElse(config.errorDepth)
7575
val clauseCoverage: Boolean = config.clauseCoverage
7676
val overloadedSpecDynamicResult: Boolean = config.overloadedSpecDynamicResult
77+
val customMapsMerge: Boolean = config.customMapsMerge
7778
}
7879
}

‎eqwalizer/src/test/resources/application.conf

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,4 +17,6 @@ eqwalizer {
1717
clause_coverage = ${?EQWALIZER_CLAUSE_COVERAGE}
1818
overloaded_spec_dynamic_result = false
1919
overloaded_spec_dynamic_result = ${?EQWALIZER_OVERLOADED_SPEC_DYNAMIC_RESULT}
20+
custom_maps_merge = true
21+
custom_maps_merge = ${?EQWALIZER_CUSTOM_MAPS_MERGE}
2022
}

‎eqwalizer/test_projects/check/src/custom.erl

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -646,6 +646,36 @@ maps_to_list_6(M) -> maps:to_list(M).
646646
-spec maps_to_list_7_neg(number()) -> dynamic().
647647
maps_to_list_7_neg(Num) -> maps:to_list(Num).
648648

649+
-spec maps_merge_1(#{a => string(), b => number()}, #{b => number(), c => atom()}) ->
650+
#{a => string(), b => number(), c => atom()}.
651+
maps_merge_1(M1, M2) -> maps:merge(M1, M2).
652+
653+
-spec maps_merge_2(#{a => string(), b => number()}, #{b => boolean(), c => atom()}) ->
654+
#{a => string(), b => number() | boolean(), c => atom()}.
655+
maps_merge_2(M1, M2) -> maps:merge(M1, M2).
656+
657+
-spec maps_merge_3(#{a := string(), b => number()}, #{b := boolean(), c => atom()}) ->
658+
#{a := string(), b := boolean(), c => atom()}.
659+
maps_merge_3(M1, M2) -> maps:merge(M1, M2).
660+
661+
-spec maps_merge_4(#{a => string(), b => number()}, #{atom() => boolean()}) ->
662+
#{atom() => boolean() | string() | number()}.
663+
maps_merge_4(M1, M2) -> maps:merge(M1, M2).
664+
665+
-spec maps_merge_5(#{string() => number()}, #{atom() => boolean()}) ->
666+
#{string() | atom() => boolean() | number()}.
667+
maps_merge_5(M1, M2) -> maps:merge(M1, M2).
668+
669+
-spec maps_merge_6(#{a => binary()}, map()) -> map().
670+
maps_merge_6(M1, M2) -> maps:merge(M1, M2).
671+
672+
-spec maps_merge_7_neg(#{a => binary()}, number()) -> term().
673+
maps_merge_7_neg(M1, M2) -> maps:merge(M1, M2).
674+
675+
-spec maps_merge_8(#{a := atom()}, #{b := number()} | #{}) ->
676+
#{a := atom(), b := number()} | #{a := atom()}.
677+
maps_merge_8(M1, M2) -> maps:merge(M1, M2).
678+
649679
-spec lists_filtermap_1() -> [number()].
650680
lists_filtermap_1() ->
651681
lists:filtermap(

‎eqwalizer/test_projects/check/src/custom.erl.check

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -787,6 +787,38 @@ maps_to_list_7_neg(Num) -> maps:to_list(Nu…… ERROR | Num.
787787
| | Expression has type: number()
788788
| | Context expected type: #D{term() => term()}
789789
| |
790+
-spec maps_merge_1(#{a => string(), b => n…… |
791+
#{a => string(), b => number(), c => a…… |
792+
maps_merge_1(M1, M2) -> maps:merge(M1, M2)…… OK |
793+
| |
794+
-spec maps_merge_2(#{a => string(), b => n…… |
795+
#{a => string(), b => number() | boole…… |
796+
maps_merge_2(M1, M2) -> maps:merge(M1, M2)…… OK |
797+
| |
798+
-spec maps_merge_3(#{a := string(), b => n…… |
799+
#{a := string(), b := boolean(), c => …… |
800+
maps_merge_3(M1, M2) -> maps:merge(M1, M2)…… OK |
801+
| |
802+
-spec maps_merge_4(#{a => string(), b => n…… |
803+
#{atom() => boolean() | string() | num…… |
804+
maps_merge_4(M1, M2) -> maps:merge(M1, M2)…… OK |
805+
| |
806+
-spec maps_merge_5(#{string() => number()}…… |
807+
#{string() | atom() => boolean() | num…… |
808+
maps_merge_5(M1, M2) -> maps:merge(M1, M2)…… OK |
809+
| |
810+
-spec maps_merge_6(#{a => binary()}, map()…… |
811+
maps_merge_6(M1, M2) -> maps:merge(M1, M2)…… OK |
812+
| |
813+
-spec maps_merge_7_neg(#{a => binary()}, n…… |
814+
maps_merge_7_neg(M1, M2) -> maps:merge(M1,…… ERROR | M2.
815+
| | Expression has type: number()
816+
| | Context expected type: #D{term() => term()}
817+
| |
818+
-spec maps_merge_8(#{a := atom()}, #{b := …… |
819+
#{a := atom(), b := number()} | #{a :=…… |
820+
maps_merge_8(M1, M2) -> maps:merge(M1, M2)…… OK |
821+
| |
790822
-spec lists_filtermap_1() -> [number()]. | |
791823
lists_filtermap_1() -> | OK |
792824
lists:filtermap( | |

0 commit comments

Comments
 (0)