Skip to content

Commit 526978f

Browse files
jckingcopybara-github
jcking
authored and
copybara-github
committed
Actually reject non-simple variable names in comprehensions
PiperOrigin-RevId: 933908907
1 parent 071fca0 commit 526978f

2 files changed

Lines changed: 39 additions & 26 deletions

File tree

‎extensions/comprehensions_v2_macros.cc‎

Lines changed: 25 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -14,12 +14,14 @@
1414

1515
#include "extensions/comprehensions_v2_macros.h"
1616

17+
#include <string>
1718
#include <utility>
1819
#include <vector>
1920

2021
#include "absl/base/no_destructor.h"
2122
#include "absl/log/absl_check.h"
2223
#include "absl/status/status.h"
24+
#include "absl/strings/match.h"
2325
#include "absl/strings/str_cat.h"
2426
#include "absl/types/optional.h"
2527
#include "absl/types/span.h"
@@ -38,16 +40,21 @@ namespace {
3840

3941
using ::google::api::expr::common::CelOperator;
4042

43+
bool IsSimpleIdentifier(const Expr& expr) {
44+
return expr.has_ident_expr() && !expr.ident_expr().name().empty() &&
45+
!absl::StartsWith(expr.ident_expr().name(), ".");
46+
}
47+
4148
absl::optional<Expr> ExpandAllMacro2(MacroExprFactory& factory, Expr& target,
4249
absl::Span<Expr> args) {
4350
if (args.size() != 3) {
4451
return factory.ReportError("all() requires 3 arguments");
4552
}
46-
if (!args[0].has_ident_expr() || args[0].ident_expr().name().empty()) {
53+
if (!IsSimpleIdentifier(args[0])) {
4754
return factory.ReportErrorAt(
4855
args[0], "all() first variable name must be a simple identifier");
4956
}
50-
if (!args[1].has_ident_expr() || args[1].ident_expr().name().empty()) {
57+
if (!IsSimpleIdentifier(args[1])) {
5158
return factory.ReportErrorAt(
5259
args[1], "all() second variable name must be a simple identifier");
5360
}
@@ -89,11 +96,11 @@ absl::optional<Expr> ExpandExistsMacro2(MacroExprFactory& factory, Expr& target,
8996
if (args.size() != 3) {
9097
return factory.ReportError("exists() requires 3 arguments");
9198
}
92-
if (!args[0].has_ident_expr() || args[0].ident_expr().name().empty()) {
99+
if (!IsSimpleIdentifier(args[0])) {
93100
return factory.ReportErrorAt(
94101
args[0], "exists() first variable name must be a simple identifier");
95102
}
96-
if (!args[1].has_ident_expr() || args[1].ident_expr().name().empty()) {
103+
if (!IsSimpleIdentifier(args[1])) {
97104
return factory.ReportErrorAt(
98105
args[1], "exists() second variable name must be a simple identifier");
99106
}
@@ -138,11 +145,11 @@ absl::optional<Expr> ExpandExistsOneMacro2(MacroExprFactory& factory,
138145
if (args.size() != 3) {
139146
return factory.ReportError("existsOne() requires 3 arguments");
140147
}
141-
if (!args[0].has_ident_expr() || args[0].ident_expr().name().empty()) {
148+
if (!IsSimpleIdentifier(args[0])) {
142149
return factory.ReportErrorAt(
143150
args[0], "existsOne() first variable name must be a simple identifier");
144151
}
145-
if (!args[1].has_ident_expr() || args[1].ident_expr().name().empty()) {
152+
if (!IsSimpleIdentifier(args[1])) {
146153
return factory.ReportErrorAt(
147154
args[1],
148155
"existsOne() second variable name must be a simple identifier");
@@ -190,12 +197,12 @@ absl::optional<Expr> ExpandTransformList3Macro(MacroExprFactory& factory,
190197
if (args.size() != 3) {
191198
return factory.ReportError("transformList() requires 3 arguments");
192199
}
193-
if (!args[0].has_ident_expr() || args[0].ident_expr().name().empty()) {
200+
if (!IsSimpleIdentifier(args[0])) {
194201
return factory.ReportErrorAt(
195202
args[0],
196203
"transformList() first variable name must be a simple identifier");
197204
}
198-
if (!args[1].has_ident_expr() || args[1].ident_expr().name().empty()) {
205+
if (!IsSimpleIdentifier(args[1])) {
199206
return factory.ReportErrorAt(
200207
args[1],
201208
"transformList() second variable name must be a simple identifier");
@@ -239,12 +246,12 @@ absl::optional<Expr> ExpandTransformList4Macro(MacroExprFactory& factory,
239246
if (args.size() != 4) {
240247
return factory.ReportError("transformList() requires 4 arguments");
241248
}
242-
if (!args[0].has_ident_expr() || args[0].ident_expr().name().empty()) {
249+
if (!IsSimpleIdentifier(args[0])) {
243250
return factory.ReportErrorAt(
244251
args[0],
245252
"transformList() first variable name must be a simple identifier");
246253
}
247-
if (!args[1].has_ident_expr() || args[1].ident_expr().name().empty()) {
254+
if (!IsSimpleIdentifier(args[1])) {
248255
return factory.ReportErrorAt(
249256
args[1],
250257
"transformList() second variable name must be a simple identifier");
@@ -290,12 +297,12 @@ absl::optional<Expr> ExpandTransformMap3Macro(MacroExprFactory& factory,
290297
if (args.size() != 3) {
291298
return factory.ReportError("transformMap() requires 3 arguments");
292299
}
293-
if (!args[0].has_ident_expr() || args[0].ident_expr().name().empty()) {
300+
if (!IsSimpleIdentifier(args[0])) {
294301
return factory.ReportErrorAt(
295302
args[0],
296303
"transformMap() first variable name must be a simple identifier");
297304
}
298-
if (!args[1].has_ident_expr() || args[1].ident_expr().name().empty()) {
305+
if (!IsSimpleIdentifier(args[1])) {
299306
return factory.ReportErrorAt(
300307
args[1],
301308
"transformMap() second variable name must be a simple identifier");
@@ -338,12 +345,12 @@ absl::optional<Expr> ExpandTransformMap4Macro(MacroExprFactory& factory,
338345
if (args.size() != 4) {
339346
return factory.ReportError("transformMap() requires 4 arguments");
340347
}
341-
if (!args[0].has_ident_expr() || args[0].ident_expr().name().empty()) {
348+
if (!IsSimpleIdentifier(args[0])) {
342349
return factory.ReportErrorAt(
343350
args[0],
344351
"transformMap() first variable name must be a simple identifier");
345352
}
346-
if (!args[1].has_ident_expr() || args[1].ident_expr().name().empty()) {
353+
if (!IsSimpleIdentifier(args[1])) {
347354
return factory.ReportErrorAt(
348355
args[1],
349356
"transformMap() second variable name must be a simple identifier");
@@ -388,12 +395,12 @@ absl::optional<Expr> ExpandTransformMapEntry3Macro(MacroExprFactory& factory,
388395
if (args.size() != 3) {
389396
return factory.ReportError("transformMapEntry() requires 3 arguments");
390397
}
391-
if (!args[0].has_ident_expr() || args[0].ident_expr().name().empty()) {
398+
if (!IsSimpleIdentifier(args[0])) {
392399
return factory.ReportErrorAt(
393400
args[0],
394401
"transformMapEntry() first variable name must be a simple identifier");
395402
}
396-
if (!args[1].has_ident_expr() || args[1].ident_expr().name().empty()) {
403+
if (!IsSimpleIdentifier(args[1])) {
397404
return factory.ReportErrorAt(
398405
args[1],
399406
"transformMapEntry() second variable name must be a simple identifier");
@@ -438,12 +445,12 @@ absl::optional<Expr> ExpandTransformMapEntry4Macro(MacroExprFactory& factory,
438445
if (args.size() != 4) {
439446
return factory.ReportError("transformMapEntry() requires 4 arguments");
440447
}
441-
if (!args[0].has_ident_expr() || args[0].ident_expr().name().empty()) {
448+
if (!IsSimpleIdentifier(args[0])) {
442449
return factory.ReportErrorAt(
443450
args[0],
444451
"transformMapEntry() first variable name must be a simple identifier");
445452
}
446-
if (!args[1].has_ident_expr() || args[1].ident_expr().name().empty()) {
453+
if (!IsSimpleIdentifier(args[1])) {
447454
return factory.ReportErrorAt(
448455
args[1],
449456
"transformMapEntry() second variable name must be a simple identifier");

‎parser/macro.cc‎

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
#include "absl/log/absl_check.h"
2626
#include "absl/status/status.h"
2727
#include "absl/status/statusor.h"
28+
#include "absl/strings/match.h"
2829
#include "absl/strings/str_cat.h"
2930
#include "absl/strings/string_view.h"
3031
#include "absl/types/optional.h"
@@ -40,6 +41,11 @@ namespace {
4041

4142
using google::api::expr::common::CelOperator;
4243

44+
bool IsSimpleIdentifier(const Expr& expr) {
45+
return expr.has_ident_expr() && !expr.ident_expr().name().empty() &&
46+
!absl::StartsWith(expr.ident_expr().name(), ".");
47+
}
48+
4349
inline MacroExpander ToMacroExpander(GlobalMacroExpander expander) {
4450
ABSL_DCHECK(expander);
4551
return [expander = std::move(expander)](
@@ -87,7 +93,7 @@ absl::optional<Expr> ExpandAllMacro(MacroExprFactory& factory, Expr& target,
8793
if (args.size() != 2) {
8894
return factory.ReportError("all() requires 2 arguments");
8995
}
90-
if (!args[0].has_ident_expr() || args[0].ident_expr().name().empty()) {
96+
if (!IsSimpleIdentifier(args[0])) {
9197
return factory.ReportErrorAt(
9298
args[0], "all() variable name must be a simple identifier");
9399
}
@@ -119,7 +125,7 @@ absl::optional<Expr> ExpandExistsMacro(MacroExprFactory& factory, Expr& target,
119125
if (args.size() != 2) {
120126
return factory.ReportError("exists() requires 2 arguments");
121127
}
122-
if (!args[0].has_ident_expr() || args[0].ident_expr().name().empty()) {
128+
if (!IsSimpleIdentifier(args[0])) {
123129
return factory.ReportErrorAt(
124130
args[0], "exists() variable name must be a simple identifier");
125131
}
@@ -153,7 +159,7 @@ absl::optional<Expr> ExpandExistsOneMacro(MacroExprFactory& factory,
153159
if (args.size() != 2) {
154160
return factory.ReportError("exists_one() requires 2 arguments");
155161
}
156-
if (!args[0].has_ident_expr() || args[0].ident_expr().name().empty()) {
162+
if (!IsSimpleIdentifier(args[0])) {
157163
return factory.ReportErrorAt(
158164
args[0], "exists_one() variable name must be a simple identifier");
159165
}
@@ -192,7 +198,7 @@ absl::optional<Expr> ExpandMap2Macro(MacroExprFactory& factory, Expr& target,
192198
if (args.size() != 2) {
193199
return factory.ReportError("map() requires 2 arguments");
194200
}
195-
if (!args[0].has_ident_expr() || args[0].ident_expr().name().empty()) {
201+
if (!IsSimpleIdentifier(args[0])) {
196202
return factory.ReportErrorAt(
197203
args[0], "map() variable name must be a simple identifier");
198204
}
@@ -225,7 +231,7 @@ absl::optional<Expr> ExpandMap3Macro(MacroExprFactory& factory, Expr& target,
225231
if (args.size() != 3) {
226232
return factory.ReportError("map() requires 3 arguments");
227233
}
228-
if (!args[0].has_ident_expr() || args[0].ident_expr().name().empty()) {
234+
if (!IsSimpleIdentifier(args[0])) {
229235
return factory.ReportErrorAt(
230236
args[0], "map() variable name must be a simple identifier");
231237
}
@@ -260,7 +266,7 @@ absl::optional<Expr> ExpandFilterMacro(MacroExprFactory& factory, Expr& target,
260266
if (args.size() != 2) {
261267
return factory.ReportError("filter() requires 2 arguments");
262268
}
263-
if (!args[0].has_ident_expr() || args[0].ident_expr().name().empty()) {
269+
if (!IsSimpleIdentifier(args[0])) {
264270
return factory.ReportErrorAt(
265271
args[0], "filter() variable name must be a simple identifier");
266272
}
@@ -298,7 +304,7 @@ absl::optional<Expr> ExpandOptMapMacro(MacroExprFactory& factory, Expr& target,
298304
if (args.size() != 2) {
299305
return factory.ReportError("optMap() requires 2 arguments");
300306
}
301-
if (!args[0].has_ident_expr() || args[0].ident_expr().name().empty()) {
307+
if (!IsSimpleIdentifier(args[0])) {
302308
return factory.ReportErrorAt(
303309
args[0], "optMap() variable name must be a simple identifier");
304310
}
@@ -337,7 +343,7 @@ absl::optional<Expr> ExpandOptFlatMapMacro(MacroExprFactory& factory,
337343
if (args.size() != 2) {
338344
return factory.ReportError("optFlatMap() requires 2 arguments");
339345
}
340-
if (!args[0].has_ident_expr() || args[0].ident_expr().name().empty()) {
346+
if (!IsSimpleIdentifier(args[0])) {
341347
return factory.ReportErrorAt(
342348
args[0], "optFlatMap() variable name must be a simple identifier");
343349
}

0 commit comments

Comments
 (0)