Skip to content

Commit ce4e8be

Browse files
jnthntatumcopybara-github
authored andcommitted
Fix a bug where type checker would treat the type assignment for the result of a comprehension as flexible / possible to widen.
PiperOrigin-RevId: 748342200
1 parent 26b3a5a commit ce4e8be

File tree

2 files changed

+41
-5
lines changed

2 files changed

+41
-5
lines changed

checker/internal/type_checker_impl.cc

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -800,7 +800,8 @@ void ResolveVisitor::PreVisitComprehension(
800800
void ResolveVisitor::PostVisitComprehension(
801801
const Expr& expr, const ComprehensionExpr& comprehension) {
802802
comprehension_scopes_.pop_back();
803-
types_[&expr] = GetDeducedType(&comprehension.result());
803+
types_[&expr] = inference_context_->FullySubstitute(
804+
GetDeducedType(&comprehension.result()));
804805
}
805806

806807
void ResolveVisitor::PreVisitComprehensionSubexpression(

checker/standard_library_test.cc

Lines changed: 39 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ TEST(StandardLibraryTest, StandardLibraryAddsDecls) {
5555
std::unique_ptr<TypeCheckerBuilder> builder,
5656
CreateTypeCheckerBuilder(GetSharedTestingDescriptorPool()));
5757
EXPECT_THAT(builder->AddLibrary(StandardCheckerLibrary()), IsOk());
58-
EXPECT_THAT(std::move(*builder).Build(), IsOk());
58+
EXPECT_THAT(builder->Build(), IsOk());
5959
}
6060

6161
TEST(StandardLibraryTest, StandardLibraryErrorsIfAddedTwice) {
@@ -91,7 +91,7 @@ TEST(StandardLibraryTest, ComprehensionVarsIndirectCyclicParamAssignability) {
9191
IsOk());
9292

9393
ASSERT_OK_AND_ASSIGN(std::unique_ptr<TypeChecker> type_checker,
94-
std::move(*builder).Build());
94+
builder->Build());
9595

9696
ASSERT_OK_AND_ASSIGN(
9797
auto ast, checker_internal::MakeTestParsedAst(
@@ -106,14 +106,49 @@ TEST(StandardLibraryTest, ComprehensionVarsIndirectCyclicParamAssignability) {
106106
EXPECT_THAT(result.GetIssues(), IsEmpty());
107107
}
108108

109+
TEST(StandardLibraryTest, ComprehensionResultTypeIsSubstituted) {
110+
google::protobuf::Arena arena;
111+
ASSERT_OK_AND_ASSIGN(
112+
std::unique_ptr<TypeCheckerBuilder> builder,
113+
CreateTypeCheckerBuilder(GetSharedTestingDescriptorPool()));
114+
ASSERT_THAT(builder->AddLibrary(StandardCheckerLibrary()), IsOk());
115+
116+
// Test that type for the result list of .map is resolved to a concrete type
117+
// when it is known. Checks for a bug where the result type is considered to
118+
// still be flexible and may widen to dyn.
119+
builder->set_container("cel.expr.conformance.proto2");
120+
ASSERT_OK_AND_ASSIGN(std::unique_ptr<TypeChecker> type_checker,
121+
builder->Build());
122+
123+
ASSERT_OK_AND_ASSIGN(auto ast, checker_internal::MakeTestParsedAst(
124+
"[TestAllTypes{}]"
125+
".map(x, x.repeated_nested_message[0])"
126+
".map(x, x.bb)[0]"));
127+
ASSERT_OK_AND_ASSIGN(ValidationResult result,
128+
type_checker->Check(std::move(ast)));
129+
130+
EXPECT_TRUE(result.IsValid());
131+
132+
EXPECT_THAT(result.GetIssues(), IsEmpty()) << result.FormatError();
133+
134+
ASSERT_OK_AND_ASSIGN(auto checked_ast, result.ReleaseAst());
135+
136+
const ast_internal::AstImpl& checked_impl =
137+
ast_internal::AstImpl::CastFromPublicAst(*checked_ast);
138+
139+
ast_internal::Type type = checked_impl.GetType(checked_impl.root_expr().id());
140+
EXPECT_TRUE(type.has_primitive() &&
141+
type.primitive() == ast_internal::PrimitiveType::kInt64);
142+
}
143+
109144
class StandardLibraryDefinitionsTest : public ::testing::Test {
110145
public:
111146
void SetUp() override {
112147
ASSERT_OK_AND_ASSIGN(
113148
std::unique_ptr<TypeCheckerBuilder> builder,
114149
CreateTypeCheckerBuilder(GetSharedTestingDescriptorPool()));
115150
ASSERT_THAT(builder->AddLibrary(StandardCheckerLibrary()), IsOk());
116-
ASSERT_OK_AND_ASSIGN(stdlib_type_checker_, std::move(*builder).Build());
151+
ASSERT_OK_AND_ASSIGN(stdlib_type_checker_, builder->Build());
117152
}
118153

119154
protected:
@@ -219,7 +254,7 @@ TEST_P(StdLibDefinitionsTest, Runner) {
219254
GetParam().options));
220255
ASSERT_THAT(builder->AddLibrary(StandardCheckerLibrary()), IsOk());
221256
ASSERT_OK_AND_ASSIGN(std::unique_ptr<TypeChecker> type_checker,
222-
std::move(*builder).Build());
257+
builder->Build());
223258

224259
ASSERT_OK_AND_ASSIGN(std::unique_ptr<Ast> ast,
225260
checker_internal::MakeTestParsedAst(GetParam().expr));

0 commit comments

Comments
 (0)