Fix case where type checker would infer a recursively defined type.

PiperOrigin-RevId: 688181907

Author: jnthntatum

checker/BUILD

@@ -143,9 +143,12 @@ cc_test(
         "//checker/internal:test_ast_helpers",
         "//common:ast",
         "//common:constant",
+        "//common:decl",
+        "//common:type",
         "//internal:testing",
         "@com_google_absl//absl/status",
         "@com_google_absl//absl/status:status_matchers",
+        "@com_google_protobuf//:protobuf",
     ],
 )
 

checker/internal/type_checker_impl_test.cc

@@ -759,6 +759,53 @@ TEST(TypeCheckerImplTest, ComprehensionVarsFollowQualifiedIdentPriority) {
               Contains(Pair(_, IsVariableReference("x.y"))));
 }
 
+TEST(TypeCheckerImplTest, ComprehensionVarsCyclicParamAssignability) {
+  TypeCheckEnv env;
+  google::protobuf::Arena arena;
+  ASSERT_THAT(RegisterMinimalBuiltins(&arena, env), IsOk());
+
+  TypeCheckerImpl impl(std::move(env));
+  // This is valid because the list construction in the transform will resolve
+  // to list(dyn) since candidates E1 -> E2 and list(E1) -> E2 don't agree.
+  ASSERT_OK_AND_ASSIGN(auto ast, MakeTestParsedAst("[].map(c, [ c, [c] ])"));
+  ASSERT_OK_AND_ASSIGN(ValidationResult result, impl.Check(std::move(ast)));
+
+  EXPECT_TRUE(result.IsValid());
+
+  EXPECT_THAT(result.GetIssues(), IsEmpty());
+
+  // Remainder are conceptually the same, but confirm generality.
+  ASSERT_OK_AND_ASSIGN(ast, MakeTestParsedAst("[].map(c, [ c, [[c]] ])"));
+  ASSERT_OK_AND_ASSIGN(result, impl.Check(std::move(ast)));
+
+  EXPECT_TRUE(result.IsValid());
+
+  ASSERT_OK_AND_ASSIGN(ast, MakeTestParsedAst("[].map(c, [ [c], [[c]] ])"));
+  ASSERT_OK_AND_ASSIGN(result, impl.Check(std::move(ast)));
+
+  EXPECT_TRUE(result.IsValid());
+
+  ASSERT_OK_AND_ASSIGN(ast, MakeTestParsedAst("[].map(c, [ c, c ])"));
+  ASSERT_OK_AND_ASSIGN(result, impl.Check(std::move(ast)));
+
+  EXPECT_TRUE(result.IsValid());
+
+  ASSERT_OK_AND_ASSIGN(ast, MakeTestParsedAst("[].map(c, [ [c], c ])"));
+  ASSERT_OK_AND_ASSIGN(result, impl.Check(std::move(ast)));
+
+  EXPECT_TRUE(result.IsValid());
+
+  ASSERT_OK_AND_ASSIGN(ast, MakeTestParsedAst("[].map(c, [ [[c]], c ])"));
+  ASSERT_OK_AND_ASSIGN(result, impl.Check(std::move(ast)));
+
+  EXPECT_TRUE(result.IsValid());
+
+  ASSERT_OK_AND_ASSIGN(ast, MakeTestParsedAst("[].map(c, [ c, type(c) ])"));
+  ASSERT_OK_AND_ASSIGN(result, impl.Check(std::move(ast)));
+
+  EXPECT_TRUE(result.IsValid());
+}
+
 struct PrimitiveLiteralsTestCase {
   std::string expr;
   ast_internal::PrimitiveType expected_type;

checker/internal/type_inference_context.cc

@@ -128,21 +128,6 @@ FunctionOverloadInstance InstantiateFunctionOverload(
   return result;
 }
 
-bool OccursWithin(absl::string_view var_name, Type t) {
-  // This is difficult to trigger without lambdas in CEL, but we still check
-  // to guarantee that we don't introduce a recursive type definition (a cycle
-  // in the substitution map).
-  if (t.kind() == TypeKind::kTypeParam && t.AsTypeParam()->name() == var_name) {
-    return true;
-  }
-  for (const auto& param : t.GetParameters()) {
-    if (OccursWithin(var_name, param)) {
-      return true;
-    }
-  }
-  return false;
-}
-
 // Converts a wrapper type to its corresponding primitive type.
 // Returns nullopt if the type is not a wrapper type.
 absl::optional<Type> WrapperToPrimitive(const Type& t) {
@@ -205,7 +190,7 @@ Type TypeInferenceContext::InstantiateTypeParams(
       if (auto it = substitutions.find(name); it != substitutions.end()) {
         return TypeParamType(it->second);
       }
-      absl::string_view substitution = NewTypeVar();
+      absl::string_view substitution = NewTypeVar(name);
       substitutions[type.AsTypeParam()->name()] = substitution;
       return TypeParamType(substitution);
     }
@@ -360,8 +345,8 @@ Type TypeInferenceContext::Substitute(
     }
     if (auto it = type_parameter_bindings_.find(t.name());
         it != type_parameter_bindings_.end()) {
-      if (it->second.has_value()) {
-        subs = *it->second;
+      if (it->second.type.has_value()) {
+        subs = *it->second.type;
         continue;
       }
     }
@@ -370,6 +355,33 @@ Type TypeInferenceContext::Substitute(
   return subs;
 }
 
+bool TypeInferenceContext::OccursWithin(
+    absl::string_view var_name, const Type& type,
+    const SubstitutionMap& substitutions) const {
+  // This is difficult to trigger in normal CEL expressions, but may
+  // happen with comprehensions where we can potentially reference a variable
+  // with a free type var in different ways.
+  //
+  // This check guarantees that we don't introduce a recursive type definition
+  // (a cycle in the substitution map).
+  if (type.kind() == TypeKind::kTypeParam) {
+    if (type.AsTypeParam()->name() == var_name) {
+      return true;
+    }
+    auto typeSubs = Substitute(type, substitutions);
+    if (typeSubs != type && OccursWithin(var_name, typeSubs, substitutions)) {
+      return true;
+    }
+  }
+
+  for (const auto& param : type.GetParameters()) {
+    if (OccursWithin(var_name, param, substitutions)) {
+      return true;
+    }
+  }
+  return false;
+}
+
 bool TypeInferenceContext::IsAssignableWithConstraints(
     const Type& from, const Type& to,
     SubstitutionMap& prospective_substitutions) {
@@ -384,16 +396,16 @@ bool TypeInferenceContext::IsAssignableWithConstraints(
 
   if (to.kind() == TypeKind::kTypeParam) {
     absl::string_view name = to.AsTypeParam()->name();
-    if (!OccursWithin(name, from)) {
-      prospective_substitutions[to.AsTypeParam()->name()] = from;
+    if (!OccursWithin(name, from, prospective_substitutions)) {
+      prospective_substitutions[name] = from;
       return true;
     }
   }
 
   if (from.kind() == TypeKind::kTypeParam) {
     absl::string_view name = from.AsTypeParam()->name();
-    if (!OccursWithin(name, to)) {
-      prospective_substitutions[from.AsTypeParam()->name()] = to;
+    if (!OccursWithin(name, to, prospective_substitutions)) {
+      prospective_substitutions[name] = to;
       return true;
     }
   }
@@ -465,7 +477,7 @@ void TypeInferenceContext::UpdateTypeParameterBindings(
        iter != prospective_substitutions.end(); ++iter) {
     if (auto binding_iter = type_parameter_bindings_.find(iter->first);
         binding_iter != type_parameter_bindings_.end()) {
-      binding_iter->second = iter->second;
+      binding_iter->second.type = iter->second;
     } else {
       ABSL_LOG(WARNING) << "Uninstantiated type parameter: " << iter->first;
     }

checker/internal/type_inference_context.h

@@ -87,13 +87,14 @@ class TypeInferenceContext {
   std::string DebugString() const {
     return absl::StrCat(
         "type_parameter_bindings: ",
-        absl::StrJoin(type_parameter_bindings_, "\n ",
-                      [](std::string* out, const auto& binding) {
-                        absl::StrAppend(
-                            out, binding.first, " -> ",
-                            binding.second.value_or(Type(TypeParamType("none")))
-                                .DebugString());
-                      }));
+        absl::StrJoin(
+            type_parameter_bindings_, "\n ",
+            [](std::string* out, const auto& binding) {
+              absl::StrAppend(
+                  out, binding.first, " (", binding.second.name, ") -> ",
+                  binding.second.type.value_or(Type(TypeParamType("none")))
+                      .DebugString());
+            }));
   }
 
  private:
@@ -102,10 +103,15 @@ class TypeInferenceContext {
   // Used for prospective substitutions during type inference.
   using SubstitutionMap = absl::flat_hash_map<absl::string_view, Type>;
 
-  absl::string_view NewTypeVar() {
+  struct TypeVar {
+    absl::optional<Type> type;
+    absl::string_view name;
+  };
+
+  absl::string_view NewTypeVar(absl::string_view name = "") {
     next_type_parameter_id_++;
     auto inserted = type_parameter_bindings_.insert(
-        {absl::StrCat("T%", next_type_parameter_id_), absl::nullopt});
+        {absl::StrCat("T%", next_type_parameter_id_), {absl::nullopt, name}});
     ABSL_DCHECK(inserted.second);
     return inserted.first->first;
   }
@@ -134,6 +140,9 @@ class TypeInferenceContext {
 
   Type Substitute(const Type& type, const SubstitutionMap& substitutions) const;
 
+  bool OccursWithin(absl::string_view var_name, const Type& type,
+                    const SubstitutionMap& substitutions) const;
+
   void UpdateTypeParameterBindings(
       const SubstitutionMap& prospective_substitutions);
 
@@ -150,8 +159,7 @@ class TypeInferenceContext {
   // instance.
   //
   // nullopt signifies a free type variable.
-  absl::node_hash_map<std::string, absl::optional<Type>>
-      type_parameter_bindings_;
+  absl::node_hash_map<std::string, TypeVar> type_parameter_bindings_;
   int64_t next_type_parameter_id_ = 0;
   google::protobuf::Arena* arena_;
   bool enable_legacy_null_assignment_;

checker/internal/type_inference_context_test.cc

@@ -485,7 +485,7 @@ TEST(TypeInferenceContextTest, DebugString) {
   ASSERT_TRUE(resolution.has_value());
   EXPECT_TRUE(resolution->result_type.IsList());
 
-  EXPECT_EQ(context.DebugString(), "type_parameter_bindings: T%1 -> int");
+  EXPECT_EQ(context.DebugString(), "type_parameter_bindings: T%1 (A) -> int");
 }
 
 struct TypeInferenceContextWrapperTypesTestCase {

checker/standard_library_test.cc

@@ -28,7 +28,10 @@
 #include "checker/validation_result.h"
 #include "common/ast.h"
 #include "common/constant.h"
+#include "common/decl.h"
+#include "common/type.h"
 #include "internal/testing.h"
+#include "google/protobuf/arena.h"
 
 namespace cel {
 namespace {
@@ -56,6 +59,43 @@ TEST(StandardLibraryTest, StandardLibraryErrorsIfAddedTwice) {
               StatusIs(absl::StatusCode::kAlreadyExists));
 }
 
+TEST(StandardLibraryTest, ComprehensionVarsIndirectCyclicParamAssignability) {
+  google::protobuf::Arena arena;
+  TypeCheckerBuilder builder;
+  ASSERT_THAT(builder.AddLibrary(StandardLibrary()), IsOk());
+
+  // Note: this is atypical -- parameterized variables aren't well supported
+  // outside of built-in syntax.
+  // e.g. `list : Type(List(A))` is instantiated per reference to bind A to
+  // the concrete type of a list in the same assignability context.
+  //
+  // Validate that parameterization is sanitized to be contextual
+  // List(V) -> List(T%1)
+  // Map(K, V) -> Map(T%2, T%3)
+  Type list_type = ListType(&arena, TypeParamType("V"));
+  Type map_type = MapType(&arena, TypeParamType("K"), TypeParamType("V"));
+
+  ASSERT_THAT(builder.AddVariable(MakeVariableDecl("list_var", list_type)),
+              IsOk());
+  ASSERT_THAT(builder.AddVariable(MakeVariableDecl("map_var", map_type)),
+              IsOk());
+
+  ASSERT_OK_AND_ASSIGN(std::unique_ptr<TypeChecker> type_checker,
+                       std::move(builder).Build());
+
+  ASSERT_OK_AND_ASSIGN(
+      auto ast, checker_internal::MakeTestParsedAst(
+                    "list_var.exists(v,"
+                    "  map_var.filter(k, map_var[k] > 1.0).size() > int(v)"
+                    ")"));
+  ASSERT_OK_AND_ASSIGN(ValidationResult result,
+                       type_checker->Check(std::move(ast)));
+
+  EXPECT_TRUE(result.IsValid());
+
+  EXPECT_THAT(result.GetIssues(), IsEmpty());
+}
+
 class StandardLibraryDefinitionsTest : public ::testing::Test {
  public:
   void SetUp() override {

common/types/type_type.cc

@@ -14,7 +14,10 @@
 
 #include "common/type.h"
 
+#include <string>
+
 #include "absl/base/nullability.h"
+#include "absl/strings/str_cat.h"
 #include "absl/types/span.h"
 #include "google/protobuf/arena.h"
 
@@ -41,6 +44,14 @@ struct TypeTypeData final {
 
 }  // namespace common_internal
 
+std::string TypeType::DebugString() const {
+  std::string s(name());
+  if (!GetParameters().empty()) {
+    absl::StrAppend(&s, "(", GetParameters().front().DebugString(), ")");
+  }
+  return s;
+}
+
 TypeType::TypeType(absl::Nonnull<google::protobuf::Arena*> arena, const Type& parameter)
     : TypeType(common_internal::TypeTypeData::Create(arena, parameter)) {}
 

common/types/type_type.h

@@ -57,7 +57,7 @@ class TypeType final {
 
   TypeParameters GetParameters() const ABSL_ATTRIBUTE_LIFETIME_BOUND;
 
-  std::string DebugString() const { return std::string(name()); }
+  std::string DebugString() const;
 
   Type GetType() const;