From c20655064700746e8f1a11b53e30336733dbcfb3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rui=20Gon=C3=A7alves?= Date: Thu, 26 Oct 2023 04:05:29 -0700 Subject: [PATCH] Add custom type checking for element/2 Summary: This would have caught the issue in D50360629, where the usage of `element` was clearly unsafe. Reviewed By: ilya-klyuchnikov Differential Revision: D50412145 fbshipit-source-id: a93a679027f7de3a2f469cfcf78fbcd53237d10c --- .../eqwalizer/tc/ElabApplyCustom.scala | 35 +++++- .../com/whatsapp/eqwalizer/tc/Narrow.scala | 98 +++++++++++++++ .../whatsapp/eqwalizer/tc/TcDiagnostics.scala | 5 + eqwalizer/test_projects/_cli/otp_funs.cli | 4 +- eqwalizer/test_projects/check/src/custom.erl | 86 +++++++++++++ .../test_projects/check/src/custom.erl.check | 116 ++++++++++++++++++ .../check_gradual/src/gradual_custom.erl | 22 ++++ .../src/gradual_custom.erl.check | 27 ++++ 8 files changed, 389 insertions(+), 4 deletions(-) diff --git a/eqwalizer/src/main/scala/com/whatsapp/eqwalizer/tc/ElabApplyCustom.scala b/eqwalizer/src/main/scala/com/whatsapp/eqwalizer/tc/ElabApplyCustom.scala index 81be2819..574c5251 100644 --- a/eqwalizer/src/main/scala/com/whatsapp/eqwalizer/tc/ElabApplyCustom.scala +++ b/eqwalizer/src/main/scala/com/whatsapp/eqwalizer/tc/ElabApplyCustom.scala @@ -7,10 +7,10 @@ package com.whatsapp.eqwalizer.tc import scala.annotation.tailrec -import com.whatsapp.eqwalizer.ast.Exprs.{AtomLit, Cons, Expr, Lambda, NilLit} +import com.whatsapp.eqwalizer.ast.Exprs.{AtomLit, Cons, Expr, IntLit, Lambda, NilLit} import com.whatsapp.eqwalizer.ast.Types._ import com.whatsapp.eqwalizer.ast.{Exprs, Pos, RemoteId} -import com.whatsapp.eqwalizer.tc.TcDiagnostics.{ExpectedSubtype, UnboundVar, UnboundRecord} +import com.whatsapp.eqwalizer.tc.TcDiagnostics.{ExpectedSubtype, IndexOutOfBounds, UnboundVar, UnboundRecord} import com.whatsapp.eqwalizer.ast.CompilerMacro class ElabApplyCustom(pipelineContext: PipelineContext) { @@ -28,6 +28,7 @@ class ElabApplyCustom(pipelineContext: PipelineContext) { private lazy val custom: Set[RemoteId] = Set( + RemoteId("erlang", "element", 2), RemoteId("erlang", "map_get", 2), RemoteId("file", "open", 2), RemoteId("lists", "filtermap", 2), @@ -296,6 +297,36 @@ class ElabApplyCustom(pipelineContext: PipelineContext) { (valTy, env1) } + /* + `-spec element(N :: NumberType, Tup :: TupleType) -> Out`, where `Out` is: + - `Tup[N]` when `N` is an integer literal corresponding to a valid index + - Union of element types of `Tup` when `N` is not a literal + - An error otherwise (index out of bounds or unexpected type) + */ + case RemoteId("erlang", "element", 2) => + val List(index, tuple) = args + val List(indexTy, tupleTy) = argTys + + def validate(): Unit = { + if (!subtype.subType(indexTy, NumberType)) + throw ExpectedSubtype(index.pos, index, expected = NumberType, got = indexTy) + if (!subtype.subType(tupleTy, AnyTupleType)) + throw ExpectedSubtype(tuple.pos, tuple, expected = AnyTupleType, got = tupleTy) + } + validate() + + val elemTy = index match { + case IntLit(Some(n)) => + narrow.getTupleElement(tupleTy, n) match { + case Right(elemTy) => elemTy + case Left(tupLen) => throw IndexOutOfBounds(callPos, index, n, tupLen) + } + case _ => + narrow.getAllTupleElements(tupleTy) + } + + (elemTy, env1) + case RemoteId("maps", "get", 3) => val List(key, map, defaultVal) = args val List(keyTy, mapTy, defaultValTy) = argTys diff --git a/eqwalizer/src/main/scala/com/whatsapp/eqwalizer/tc/Narrow.scala b/eqwalizer/src/main/scala/com/whatsapp/eqwalizer/tc/Narrow.scala index ca62403a..468bba2a 100644 --- a/eqwalizer/src/main/scala/com/whatsapp/eqwalizer/tc/Narrow.scala +++ b/eqwalizer/src/main/scala/com/whatsapp/eqwalizer/tc/Narrow.scala @@ -351,6 +351,104 @@ class Narrow(pipelineContext: PipelineContext) { case _ => List() } + /** + * Given a type (required to be a subtype of `AnyTupleType`) and an index, returns the type of the tuple element at + * the index wrapped in a `Right`. If the index can be possibly out of bounds (in at least one of the options in a + * union) the function returns `Left(tupLen)`, where `tupLen` is the minimum index value for which this operation would + * type check. + */ + def getTupleElement(t: Type, idx: Int): Either[Int, Type] = t match { + case NoneType => + Right(NoneType) + case DynamicType => + Right(DynamicType) + case AnyTupleType if pipelineContext.gradualTyping => + Right(DynamicType) + case AnyTupleType => + Right(AnyType) + case BoundedDynamicType(t) if subtype.subType(t, AnyTupleType) => + Right(BoundedDynamicType(getTupleElement(t, idx).getOrElse(NoneType))) + case BoundedDynamicType(t) => + Right(BoundedDynamicType(NoneType)) + case TupleType(elemTys) if idx >= 1 && idx <= elemTys.length => + Right(elemTys(idx - 1)) + case TupleType(elemTys) => + Left(elemTys.length) + case r: RecordType => + recordToTuple(r) match { + case Some(tupTy) => getTupleElement(tupTy, idx) + case None if pipelineContext.gradualTyping => Right(DynamicType) + case None => Right(AnyType) + } + case r: RefinedRecordType => + refinedRecordToTuple(r) match { + case Some(tupTy) => getTupleElement(tupTy, idx) + case None if pipelineContext.gradualTyping => Right(DynamicType) + case None => Right(AnyType) + } + case UnionType(tys) => + val res = tys.map(getTupleElement(_, idx)).foldLeft[Either[Int, Set[Type]]](Right(Set.empty)) { + case (Right(accTy), Right(elemTy)) => Right(accTy + elemTy) + case (Left(n1), Left(n2)) => Left(n1.min(n2)) + case (Left(n1), _) => Left(n1) + case (_, Left(n2)) => Left(n2) + } + res.map { optionTys => UnionType(util.flattenUnions(UnionType(optionTys)).toSet) } + case RemoteType(rid, args) => + val body = util.getTypeDeclBody(rid, args) + getTupleElement(body, idx) + case _ => + throw new IllegalStateException() + } + + /** + * Given a type (required to be a subtype of `AnyTupleType`), returns the union of all its element types. + */ + def getAllTupleElements(t: Type): Type = t match { + case NoneType => + NoneType + case DynamicType => + DynamicType + case AnyTupleType if pipelineContext.gradualTyping => + DynamicType + case AnyTupleType => + AnyType + case BoundedDynamicType(t) if subtype.subType(t, AnyTupleType) => + BoundedDynamicType(getAllTupleElements(t)) + case BoundedDynamicType(t) => + BoundedDynamicType(NoneType) + case TupleType(elemTys) => + UnionType(elemTys.toSet) + case r: RecordType => + recordToTuple(r) match { + case Some(tupTy) => getAllTupleElements(tupTy) + case None if pipelineContext.gradualTyping => DynamicType + case None => AnyType + } + case r: RefinedRecordType => + refinedRecordToTuple(r) match { + case Some(tupTy) => getAllTupleElements(tupTy) + case None if pipelineContext.gradualTyping => DynamicType + case None => AnyType + } + case UnionType(tys) => + UnionType(util.flattenUnions(UnionType(tys.map(getAllTupleElements))).toSet) + case RemoteType(rid, args) => + val body = util.getTypeDeclBody(rid, args) + getAllTupleElements(body) + case _ => + throw new IllegalStateException() + } + + private def recordToTuple(r: RecordType): Option[TupleType] = + refinedRecordToTuple(RefinedRecordType(r, Map())) + + private def refinedRecordToTuple(r: RefinedRecordType): Option[TupleType] = + util.getRecord(r.recType.module, r.recType.name).map { recDecl => + val elemTys = AtomLitType(r.recType.name) :: recDecl.fields.map(f => r.fields.getOrElse(f._1, f._2.tp)).toList + TupleType(elemTys) + } + private def adjustShapeMap(t: ShapeMap, keyT: Type, valT: Type): Type = keyT match { case AtomLitType(key) => diff --git a/eqwalizer/src/main/scala/com/whatsapp/eqwalizer/tc/TcDiagnostics.scala b/eqwalizer/src/main/scala/com/whatsapp/eqwalizer/tc/TcDiagnostics.scala index 121b485b..14b8fafc 100644 --- a/eqwalizer/src/main/scala/com/whatsapp/eqwalizer/tc/TcDiagnostics.scala +++ b/eqwalizer/src/main/scala/com/whatsapp/eqwalizer/tc/TcDiagnostics.scala @@ -56,6 +56,11 @@ object TcDiagnostics { def errorName = "fun_arity_mismatch" override def erroneousExpr: Option[Expr] = Some(expr) } + case class IndexOutOfBounds(pos: Pos, expr: Expr, index: Int, tupleArity: Int) extends TypeError { + override val msg: String = s"Tried to access element $index of a tuple with $tupleArity elements" + def errorName = "index_out_of_bounds" + override def erroneousExpr: Option[Expr] = Some(expr) + } case class NotSupportedLambdaInOverloadedCall(pos: Pos, expr: Expr) extends TypeError { override val msg: String = s"Lambdas are not allowed as args to overloaded functions" def errorName = "fun_in_overload_arg" diff --git a/eqwalizer/test_projects/_cli/otp_funs.cli b/eqwalizer/test_projects/_cli/otp_funs.cli index 6395912d..0384989d 100644 --- a/eqwalizer/test_projects/_cli/otp_funs.cli +++ b/eqwalizer/test_projects/_cli/otp_funs.cli @@ -16,8 +16,8 @@ gb_sets 26 proplists 51 maps 149 lists 169 -erlang 374 +erlang 396 Per app stats: kernel 21 -erts 374 +erts 396 stdlib 471 diff --git a/eqwalizer/test_projects/check/src/custom.erl b/eqwalizer/test_projects/check/src/custom.erl index c230f3f4..1d7b31b8 100644 --- a/eqwalizer/test_projects/check/src/custom.erl +++ b/eqwalizer/test_projects/check/src/custom.erl @@ -8,6 +8,92 @@ -import(maps, [get/2, get/3]). -compile([export_all, nowarn_export_all]). +-record(foo, { + a :: ok | error, + b :: number(), + c :: string() +}). + +% element/2 - basic examples + +-spec element_2_basic_1({atom(), number(), string()}) -> atom(). +element_2_basic_1(Tup) -> + element(1, Tup). + +-spec element_2_basic_2_neg({atom(), number(), string(), map()}) -> atom(). +element_2_basic_2_neg(Tup) -> + element(4, Tup). + +-spec element_2_basic_3_neg({atom(), number(), string()}) -> atom(). +element_2_basic_3_neg(Tup) -> + element(42, Tup). + +% element/2 - union examples + +-spec element_2_union_1({atom(), number() | string()} | {number(), atom()}) -> number() | string() | atom(). +element_2_union_1(Tup) -> + element(2, Tup). + +-spec element_2_union_2_neg({atom(), number() | string()} | {number(), atom()}) -> map(). +element_2_union_2_neg(Tup) -> + element(2, Tup). + +-spec element_2_union_3_neg({atom(), string()} | list()) -> string(). +element_2_union_3_neg(Tup) -> + element(2, Tup). + +-spec element_2_union_4_neg({c, d, e, f} | {a, b} | {b, c, d}) -> atom(). +element_2_union_4_neg(Tup) -> + element(42, Tup). + +% element/2 - dynamic index examples + +-spec element_2_dynindex_1_neg(pos_integer(), {atom(), number(), string()}) -> map(). +element_2_dynindex_1_neg(N, Tup) -> + element(N, Tup). + +-spec element_2_dynindex_2_neg(pos_integer(), {atom(), atom()} | {atom(), atom(), number()}) -> atom(). +element_2_dynindex_2_neg(N, Tup) -> + element(N, Tup). + +% element/2 - tuple() examples + +-spec element_2_anytuple_1_neg(tuple()) -> atom(). +element_2_anytuple_1_neg(Tup) -> + element(1, Tup). + +-spec element_2_anytuple_2_neg(tuple() | {number(), atom()}) -> atom(). +element_2_anytuple_2_neg(Tup) -> + element(1, Tup). + +% element/2 - record examples + +-spec element_2_record_1(#foo{}) -> foo. +element_2_record_1(Rec) -> + element(1, Rec). + +-spec element_2_record_2(#foo{}) -> ok | error. +element_2_record_2(Rec) -> + element(2, Rec). + +-spec element_2_record_3(#foo{}) -> ok. +element_2_record_3(Rec) when Rec#foo.a =/= error -> + element(2, Rec). + +-spec element_2_record_4_neg(pos_integer(), #foo{}) -> atom(). +element_2_record_4_neg(N, Rec) -> + element(N, Rec). + +% element/2 - none examples + +-spec element_2_none_1(none()) -> number(). +element_2_none_1(Tup) -> + element(42, Tup). + +-spec element_2_none_2(pos_integer(), none()) -> number(). +element_2_none_2(N, Tup) -> + element(N, Tup). + -spec map_get_2_1( pid(), #{pid() => atom()} ) -> atom(). diff --git a/eqwalizer/test_projects/check/src/custom.erl.check b/eqwalizer/test_projects/check/src/custom.erl.check index bca4cd9a..8fba682f 100644 --- a/eqwalizer/test_projects/check/src/custom.erl.check +++ b/eqwalizer/test_projects/check/src/custom.erl.check @@ -8,6 +8,122 @@ -import(maps, [get/2, get/3]). | | -compile([export_all, nowarn_export_all]). | | | | +-record(foo, { | | + a :: ok | error, | | + b :: number(), | | + c :: string() | | +}). | | + | | +% element/2 - basic examples | | + | | +-spec element_2_basic_1({atom(), number(),…… | +element_2_basic_1(Tup) -> | OK | + element(1, Tup). | | + | | +-spec element_2_basic_2_neg({atom(), numbe…… | +element_2_basic_2_neg(Tup) -> | ERROR | + element(4, Tup). | | erlang:element(4, Tup). + | | Expression has type: #D{eqwalizer:dynamic() => eqwalizer:dynamic()} + | | Context expected type: atom() + | | +-spec element_2_basic_3_neg({atom(), numbe…… | +element_2_basic_3_neg(Tup) -> | ERROR | + element(42, Tup). | | 42. + | | Tried to access element 42 of a tuple with 3 elements + | | +% element/2 - union examples | | + | | +-spec element_2_union_1({atom(), number() …… | +element_2_union_1(Tup) -> | OK | + element(2, Tup). | | + | | +-spec element_2_union_2_neg({atom(), numbe…… | +element_2_union_2_neg(Tup) -> | ERROR | + element(2, Tup). | | erlang:element(2, Tup). + | | Expression has type: number() | string() | atom() + | | Context expected type: #D{eqwalizer:dynamic() => eqwalizer:dynamic()} + | | +-spec element_2_union_3_neg({atom(), strin…… | +element_2_union_3_neg(Tup) -> | ERROR | + element(2, Tup). | | Tup. + | | Expression has type: {atom(), string()} | [eqwalizer:dynamic()] + | | Context expected type: tuple() + | | + | | {atom(), string()} | [eqwalizer:dynamic()] is not compatible with tuple() + | | because + | | [eqwalizer:dynamic()] is not compatible with tuple() + | | +-spec element_2_union_4_neg({c, d, e, f} |…… | +element_2_union_4_neg(Tup) -> | ERROR | + element(42, Tup). | | 42. + | | Tried to access element 42 of a tuple with 2 elements + | | +% element/2 - dynamic index examples | | + | | +-spec element_2_dynindex_1_neg(pos_integer…… | +element_2_dynindex_1_neg(N, Tup) -> | ERROR | + element(N, Tup). | | erlang:element(N, Tup). + | | Expression has type: atom() | number() | string() + | | Context expected type: #D{eqwalizer:dynamic() => eqwalizer:dynamic()} + | | +-spec element_2_dynindex_2_neg(pos_integer…… | +element_2_dynindex_2_neg(N, Tup) -> | ERROR | + element(N, Tup). | | erlang:element(N, Tup). + | | Expression has type: atom() | number() + | | Context expected type: atom() + | | + | | atom() | number() is not compatible with atom() + | | because + | | number() is not compatible with atom() + | | +% element/2 - tuple() examples | | + | | +-spec element_2_anytuple_1_neg(tuple()) ->…… | +element_2_anytuple_1_neg(Tup) -> | ERROR | + element(1, Tup). | | erlang:element(1, Tup). + | | Expression has type: term() + | | Context expected type: atom() + | | +-spec element_2_anytuple_2_neg(tuple() | {…… | +element_2_anytuple_2_neg(Tup) -> | ERROR | + element(1, Tup). | | erlang:element(1, Tup). + | | Expression has type: term() | number() + | | Context expected type: atom() + | | +% element/2 - record examples | | + | | +-spec element_2_record_1(#foo{}) -> foo. | | +element_2_record_1(Rec) -> | OK | + element(1, Rec). | | + | | +-spec element_2_record_2(#foo{}) -> ok | e…… | +element_2_record_2(Rec) -> | OK | + element(2, Rec). | | + | | +-spec element_2_record_3(#foo{}) -> ok. | | +element_2_record_3(Rec) when Rec#foo.a =/=…… OK | + element(2, Rec). | | + | | +-spec element_2_record_4_neg(pos_integer()…… | +element_2_record_4_neg(N, Rec) -> | ERROR | + element(N, Rec). | | erlang:element(N, Rec). + | | Expression has type: 'foo' | 'ok' | 'error' | number() | string() + | | Context expected type: atom() + | | + | | 'foo' | 'ok' | 'error' | number() | string() is not compatible with atom() + | | because + | | string() is not compatible with atom() + | | +% element/2 - none examples | | + | | +-spec element_2_none_1(none()) -> number()…… | +element_2_none_1(Tup) -> | OK | + element(42, Tup). | | + | | +-spec element_2_none_2(pos_integer(), none…… | +element_2_none_2(N, Tup) -> | OK | + element(N, Tup). | | + | | -spec map_get_2_1( | | pid(), #{pid() => atom()} | | ) -> atom(). | | diff --git a/eqwalizer/test_projects/check_gradual/src/gradual_custom.erl b/eqwalizer/test_projects/check_gradual/src/gradual_custom.erl index 00940b7c..c8d6a789 100644 --- a/eqwalizer/test_projects/check_gradual/src/gradual_custom.erl +++ b/eqwalizer/test_projects/check_gradual/src/gradual_custom.erl @@ -7,6 +7,28 @@ -compile([export_all, nowarn_export_all]). +% element/2 - dynamic tuple examples + +-spec element_2_dynamic_1_print(eqwalizer:dynamic()) -> none(). +element_2_dynamic_1_print(Tup) -> + eqwalizer:reveal_type(element(42, Tup)). + +-spec element_2_dynamic_2_print(eqwalizer:dynamic({number(), atom()})) -> none(). +element_2_dynamic_2_print(Tup) -> + eqwalizer:reveal_type(element(2, Tup)). + +-spec element_2_dynamic_3_print(eqwalizer:dynamic({number(), atom()})) -> none(). +element_2_dynamic_3_print(Tup) -> + eqwalizer:reveal_type(element(1, Tup)). + +-spec element_2_dynamic_4_print(eqwalizer:dynamic(number())) -> none(). +element_2_dynamic_4_print(Tup) -> + eqwalizer:reveal_type(element(1, Tup)). + +-spec element_2_dynamic_5_print(pos_integer(), eqwalizer:dynamic({number(), atom()})) -> none(). +element_2_dynamic_5_print(N, Tup) -> + eqwalizer:reveal_type(element(N, Tup)). + -spec app_env1_gradual() -> number(). app_env1_gradual() -> Res = application:get_env(app1), diff --git a/eqwalizer/test_projects/check_gradual/src/gradual_custom.erl.check b/eqwalizer/test_projects/check_gradual/src/gradual_custom.erl.check index 0b0f464c..73a115af 100644 --- a/eqwalizer/test_projects/check_gradual/src/gradual_custom.erl.check +++ b/eqwalizer/test_projects/check_gradual/src/gradual_custom.erl.check @@ -6,6 +6,33 @@ -module(gradual_custom). | | | | -compile([export_all, nowarn_export_all]). | | + | | +% element/2 - dynamic tuple examples | | + | | +-spec element_2_dynamic_1_print(eqwalizer:…… | +element_2_dynamic_1_print(Tup) -> | ERROR | + eqwalizer:reveal_type(element(42, Tup)…… | dynamic() + + | | +-spec element_2_dynamic_2_print(eqwalizer:…… | +element_2_dynamic_2_print(Tup) -> | ERROR | + eqwalizer:reveal_type(element(2, Tup))…… | dynamic(atom()) + + | | +-spec element_2_dynamic_3_print(eqwalizer:…… | +element_2_dynamic_3_print(Tup) -> | ERROR | + eqwalizer:reveal_type(element(1, Tup))…… | dynamic(number()) + + | | +-spec element_2_dynamic_4_print(eqwalizer:…… | +element_2_dynamic_4_print(Tup) -> | ERROR | + eqwalizer:reveal_type(element(1, Tup))…… | dynamic(none()) + + | | +-spec element_2_dynamic_5_print(pos_intege…… | +element_2_dynamic_5_print(N, Tup) -> | ERROR | + eqwalizer:reveal_type(element(N, Tup))…… | dynamic(number() | atom()) + | | -spec app_env1_gradual() -> number(). | | app_env1_gradual() -> | OK |