Skip to content

Commit 21620e2

Browse files
jnthntatumcopybara-github
jnthntatum
authored and
copybara-github
committed
Avoid duplicating complex expressions in optional macros
cross ref: cel-expr/cel-go#1387 PiperOrigin-RevId: 954922600
1 parent 0160bfd commit 21620e2

2 files changed

Lines changed: 139 additions & 55 deletions

File tree

‎compiler/optional_test.cc‎

Lines changed: 63 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -111,11 +111,66 @@ optional.of(
111111
TestCase{
112112
.expr = "optional.of('foo').optMap(x, x)",
113113
.expected_ast = R"(
114-
_?_:_(
114+
__comprehension__(
115+
// Variable
116+
#unused,
117+
// Target
118+
[]~list(dyn),
119+
// Accumulator
120+
@target,
121+
// Init
115122
optional.of(
116123
"foo"~string
117-
)~optional_type(string)^optional_of.hasValue()~bool^optional_hasValue,
124+
)~optional_type(string)^optional_of,
125+
// LoopCondition
126+
false~bool,
127+
// LoopStep
128+
@target~optional_type(string)^@target,
129+
// Result
130+
_?_:_(
131+
@target~optional_type(string)^@target.hasValue()~bool^optional_hasValue,
132+
optional.of(
133+
__comprehension__(
134+
// Variable
135+
#unused,
136+
// Target
137+
[]~list(dyn),
138+
// Accumulator
139+
x,
140+
// Init
141+
@target~optional_type(string)^@target.value()~string^optional_value,
142+
// LoopCondition
143+
false~bool,
144+
// LoopStep
145+
x~string^x,
146+
// Result
147+
x~string^x)~string
148+
)~optional_type(string)^optional_of,
149+
optional.none()~optional_type(string)^optional_none
150+
)~optional_type(string)^conditional)~optional_type(string)
151+
)",
152+
},
153+
TestCase{
154+
.expr = "optional.of('foo').optFlatMap(x, optional.of(x))",
155+
.expected_ast = R"(
156+
__comprehension__(
157+
// Variable
158+
#unused,
159+
// Target
160+
[]~list(dyn),
161+
// Accumulator
162+
@target,
163+
// Init
118164
optional.of(
165+
"foo"~string
166+
)~optional_type(string)^optional_of,
167+
// LoopCondition
168+
false~bool,
169+
// LoopStep
170+
@target~optional_type(string)^@target,
171+
// Result
172+
_?_:_(
173+
@target~optional_type(string)^@target.hasValue()~bool^optional_hasValue,
119174
__comprehension__(
120175
// Variable
121176
#unused,
@@ -124,48 +179,17 @@ _?_:_(
124179
// Accumulator
125180
x,
126181
// Init
127-
optional.of(
128-
"foo"~string
129-
)~optional_type(string)^optional_of.value()~string^optional_value,
182+
@target~optional_type(string)^@target.value()~string^optional_value,
130183
// LoopCondition
131184
false~bool,
132185
// LoopStep
133186
x~string^x,
134187
// Result
135-
x~string^x)~string
136-
)~optional_type(string)^optional_of,
137-
optional.none()~optional_type(string)^optional_none
138-
)~optional_type(string)^conditional
139-
)",
140-
},
141-
TestCase{
142-
.expr = "optional.of('foo').optFlatMap(x, optional.of(x))",
143-
.expected_ast = R"(
144-
_?_:_(
145-
optional.of(
146-
"foo"~string
147-
)~optional_type(string)^optional_of.hasValue()~bool^optional_hasValue,
148-
__comprehension__(
149-
// Variable
150-
#unused,
151-
// Target
152-
[]~list(dyn),
153-
// Accumulator
154-
x,
155-
// Init
156-
optional.of(
157-
"foo"~string
158-
)~optional_type(string)^optional_of.value()~string^optional_value,
159-
// LoopCondition
160-
false~bool,
161-
// LoopStep
162-
x~string^x,
163-
// Result
164-
optional.of(
165-
x~string^x
166-
)~optional_type(string)^optional_of)~optional_type(string),
167-
optional.none()~optional_type(string)^optional_none
168-
)~optional_type(string)^conditional
188+
optional.of(
189+
x~string^x
190+
)~optional_type(string)^optional_of)~optional_type(string),
191+
optional.none()~optional_type(string)^optional_none
192+
)~optional_type(string)^conditional)~optional_type(string)
169193
)",
170194
},
171195
TestCase{

‎parser/macro.cc‎

Lines changed: 76 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,8 @@ namespace cel {
3939

4040
namespace {
4141

42+
constexpr absl::string_view kOptionalMapVar = "@target";
43+
4244
using google::api::expr::common::CelOperator;
4345

4446
bool IsSimpleIdentifier(const Expr& expr) {
@@ -315,20 +317,50 @@ absl::optional<Expr> ExpandOptMapMacro(MacroExprFactory& factory, Expr& target,
315317
}
316318
auto var_name = args[0].ident_expr().name();
317319

318-
auto target_copy = factory.Copy(target);
319-
std::vector<Expr> call_args;
320-
call_args.reserve(3);
321-
call_args.push_back(factory.NewMemberCall("hasValue", std::move(target)));
320+
if (target.has_ident_expr()) {
321+
auto target_copy = factory.Copy(target);
322+
std::vector<Expr> call_args;
323+
call_args.reserve(3);
324+
call_args.push_back(factory.NewMemberCall("hasValue", std::move(target)));
325+
auto iter_range = factory.NewList();
326+
auto accu_init = factory.NewMemberCall("value", std::move(target_copy));
327+
auto condition = factory.NewBoolConst(false);
328+
auto fold = factory.NewComprehension(
329+
"#unused", std::move(iter_range), std::move(var_name),
330+
std::move(accu_init), std::move(condition), std::move(args[0]),
331+
std::move(args[1]));
332+
call_args.push_back(factory.NewCall("optional.of", std::move(fold)));
333+
call_args.push_back(factory.NewCall("optional.none"));
334+
return factory.NewCall(CelOperator::CONDITIONAL, std::move(call_args));
335+
}
336+
337+
// If the target is complex, use an internal bind expression to avoid
338+
// repeating it and blowing up the AST in the expansion
339+
auto tmp = factory.NewIdent(kOptionalMapVar);
340+
auto tmp_copy = factory.Copy(tmp);
341+
322342
auto iter_range = factory.NewList();
323-
auto accu_init = factory.NewMemberCall("value", std::move(target_copy));
343+
auto accu_init = factory.NewMemberCall("value", std::move(tmp_copy));
324344
auto condition = factory.NewBoolConst(false);
345+
auto loop_step = std::move(args[0]);
325346
auto fold = factory.NewComprehension(
326347
"#unused", std::move(iter_range), std::move(var_name),
327-
std::move(accu_init), std::move(condition), std::move(args[0]),
348+
std::move(accu_init), std::move(condition), std::move(loop_step),
328349
std::move(args[1]));
350+
std::vector<Expr> call_args;
351+
call_args.reserve(3);
352+
call_args.push_back(factory.NewMemberCall("hasValue", std::move(tmp)));
329353
call_args.push_back(factory.NewCall("optional.of", std::move(fold)));
330354
call_args.push_back(factory.NewCall("optional.none"));
331-
return factory.NewCall(CelOperator::CONDITIONAL, std::move(call_args));
355+
auto result = factory.NewCall(CelOperator::CONDITIONAL, std::move(call_args));
356+
357+
iter_range = factory.NewList();
358+
accu_init = std::move(target);
359+
condition = factory.NewBoolConst(false);
360+
loop_step = factory.NewIdent(kOptionalMapVar);
361+
return factory.NewComprehension(
362+
"#unused", std::move(iter_range), kOptionalMapVar, std::move(accu_init),
363+
std::move(condition), loop_step, std::move(result));
332364
}
333365

334366
Macro MakeOptMapMacro() {
@@ -354,19 +386,47 @@ absl::optional<Expr> ExpandOptFlatMapMacro(MacroExprFactory& factory,
354386
}
355387
auto var_name = args[0].ident_expr().name();
356388

357-
auto target_copy = factory.Copy(target);
358-
std::vector<Expr> call_args;
359-
call_args.reserve(3);
360-
call_args.push_back(factory.NewMemberCall("hasValue", std::move(target)));
389+
if (target.has_ident_expr()) {
390+
auto target_copy = factory.Copy(target);
391+
std::vector<Expr> call_args;
392+
call_args.reserve(3);
393+
call_args.push_back(factory.NewMemberCall("hasValue", std::move(target)));
394+
auto iter_range = factory.NewList();
395+
auto accu_init = factory.NewMemberCall("value", std::move(target_copy));
396+
auto condition = factory.NewBoolConst(false);
397+
call_args.push_back(factory.NewComprehension(
398+
"#unused", std::move(iter_range), std::move(var_name),
399+
std::move(accu_init), std::move(condition), std::move(args[0]),
400+
std::move(args[1])));
401+
call_args.push_back(factory.NewCall("optional.none"));
402+
return factory.NewCall(CelOperator::CONDITIONAL, std::move(call_args));
403+
}
404+
405+
auto tmp = factory.NewIdent(kOptionalMapVar);
406+
auto tmp_copy = factory.Copy(tmp);
407+
361408
auto iter_range = factory.NewList();
362-
auto accu_init = factory.NewMemberCall("value", std::move(target_copy));
409+
auto accu_init = factory.NewMemberCall("value", std::move(tmp_copy));
363410
auto condition = factory.NewBoolConst(false);
364-
call_args.push_back(factory.NewComprehension(
411+
auto loop_step = std::move(args[0]);
412+
auto inner = factory.NewComprehension(
365413
"#unused", std::move(iter_range), std::move(var_name),
366-
std::move(accu_init), std::move(condition), std::move(args[0]),
367-
std::move(args[1])));
414+
std::move(accu_init), std::move(condition), std::move(loop_step),
415+
std::move(args[1]));
416+
std::vector<Expr> call_args;
417+
call_args.reserve(3);
418+
call_args.push_back(factory.NewMemberCall("hasValue", std::move(tmp)));
419+
call_args.push_back(std::move(inner));
368420
call_args.push_back(factory.NewCall("optional.none"));
369-
return factory.NewCall(CelOperator::CONDITIONAL, std::move(call_args));
421+
auto result = factory.NewCall(CelOperator::CONDITIONAL, std::move(call_args));
422+
423+
iter_range = factory.NewList();
424+
accu_init = std::move(target);
425+
condition = factory.NewBoolConst(false);
426+
loop_step = factory.NewIdent(kOptionalMapVar);
427+
return factory.NewComprehension(
428+
"#unused", std::move(iter_range), kOptionalMapVar, std::move(accu_init),
429+
std::move(condition), loop_step, std::move(result));
370430
}
371431

372432
Macro MakeOptFlatMapMacro() {

0 commit comments

Comments
 (0)