From 60d29fd9751d6785f738c419fd9bd1acee997758 Mon Sep 17 00:00:00 2001 From: Dmitri Plotnikov Date: Thu, 10 Sep 2026 22:58:07 -0700 Subject: [PATCH] Avoid intermediate slice views when reading token text Lexing and parsing turn code point ranges into strings constantly: once for every identifier, keyword and literal. Both call sites spelled this as slice(i, j).toString(), which allocates an intermediate CelCodePointArray view solely to copy out of it and then discard it. Add CelCodePointArray.substring(i, j), which builds the String straight from the backing array, and implement it in each of the four subclasses. toString() becomes final and delegates to substring(0, size()), so the subclasses lose their near-duplicate toString() overrides. Lexer.consumeIdent and PrattParser.getTokenText call the new method. This removes exactly one 32-byte object per token whose text is materialized. Measured with CelParserBenchmark (parseOnly, built -c opt), comparing three parsers back to back in one session: ANTLR, the Pratt parser before this change, and the Pratt parser after it. Objects allocated per parse: | Case | ANTLR | Pratt before | Pratt after | Pratt vs ANTLR | Delta this CL | | :--- | ---: | ---: | ---: | ---: | ---: | | SMOKE_TEST | 357 | 123 | 120 | 3.0x smaller | -2.4% | | CHAINED_ORS | 968 | 349 | 339 | 2.9x smaller | -2.9% | | LIST_COMPREHENSION | 512 | 166 | 160 | 3.2x smaller | -3.6% | | MESSAGE_CREATION | 1,253 | 426 | 406 | 3.1x smaller | -4.7% | | LONG_LIST | 81,794 | 19,265 | 18,263 | 4.5x smaller | -5.2% | Bytes allocated per parse: | Case | ANTLR | Pratt before | Pratt after | Pratt vs ANTLR | Delta this CL | | :--- | ---: | ---: | ---: | ---: | ---: | | SMOKE_TEST | 12,256 | 3,608 | 3,512 | 3.5x smaller | -2.7% | | CHAINED_ORS | 32,160 | 9,912 | 9,592 | 3.4x smaller | -3.2% | | LIST_COMPREHENSION | 17,320 | 4,928 | 4,736 | 3.7x smaller | -3.9% | | MESSAGE_CREATION | 43,128 | 13,056 | 12,416 | 3.5x smaller | -4.9% | | LONG_LIST | 2,907,488 | 563,952 | 531,888 | 5.5x smaller | -5.7% | Wall clock, mean of 3 caliper trial medians: | Case | ANTLR | Pratt before | Pratt after | Pratt vs ANTLR | Delta this CL | | :--- | ---: | ---: | ---: | ---: | ---: | | SMOKE_TEST | 4,940 ns | 696 ns | 684 ns | 7.2x faster | -1.8% | | CHAINED_ORS | 14,641 ns | 2,008 ns | 2,006 ns | 7.3x faster | -0.1% | | LIST_COMPREHENSION | 7,514 ns | 1,250 ns | 1,217 ns | 6.2x faster | -2.7% | | MESSAGE_CREATION | 20,979 ns | 3,526 ns | 3,514 ns | 6.0x faster | -0.4% | | LONG_LIST | 1,616,631 ns | 140,500 ns | 146,640 ns | 11.0x faster | +4.4% | Wall clock is unchanged within measurement noise. The per-case deltas run from -2.7% to +4.4% and straddle zero, which is what a change that removes 3-5% of allocations and no actual work should look like. The LONG_LIST row reads as a regression, but that trial was noisy (per-trial medians 144.6us, 155.7us, 139.6us, against a much tighter 142.1us, 141.1us, 138.3us before) and its fastest observed parse, 134.6us, is below the 135.7us baseline. The win here is allocation volume and the GC pressure that follows from it. This is the first in a series of parser changes; the wall-clock improvements come later in that series. The ANTLR column is included for scale, and shows why the Pratt parser exists. ANTLR is slow enough on LONG_LIST that the case exceeds caliper's default 5 minute per-trial budget and has to be measured with a raised --time-limit. PiperOrigin-RevId: 979610652 --- .../common/internal/BasicCodePointArray.java | 11 +- .../common/internal/CelCodePointArray.java | 12 +- .../common/internal/EmptyCodePointArray.java | 13 +- .../common/internal/Latin1CodePointArray.java | 11 +- .../internal/SupplementalCodePointArray.java | 11 +- .../internal/CelCodePointArrayTest.java | 88 ++++ .../src/main/java/dev/cel/parser/BUILD.bazel | 1 + .../src/main/java/dev/cel/parser/Lexer.java | 114 +++--- .../main/java/dev/cel/parser/PrattParser.java | 383 +++++++++--------- .../dev/cel/parser/CelParserImplTest.java | 12 + .../parser/CelParserParameterizedTest.java | 1 + .../java/dev/cel/parser/PrattParserTest.java | 1 + .../resources/parser_core_syntax.baseline | 71 ++++ .../pratt_parser_core_syntax.baseline | 71 ++++ 14 files changed, 522 insertions(+), 278 deletions(-) diff --git a/common/src/main/java/dev/cel/common/internal/BasicCodePointArray.java b/common/src/main/java/dev/cel/common/internal/BasicCodePointArray.java index a54fb65d7..482a4884f 100644 --- a/common/src/main/java/dev/cel/common/internal/BasicCodePointArray.java +++ b/common/src/main/java/dev/cel/common/internal/BasicCodePointArray.java @@ -58,13 +58,14 @@ public BasicCodePointArray slice(int i, int j) { } @Override - public int get(int index) { - checkElementIndex(index, size()); - return codePoints()[offset() + index] & 0xffff; + public String substring(int i, int j) { + checkPositionIndexes(i, j, size()); + return new String(codePoints(), offset() + i, j - i); } @Override - public final String toString() { - return new String(codePoints(), offset(), size()); + public int get(int index) { + checkElementIndex(index, size()); + return codePoints()[offset() + index] & 0xffff; } } diff --git a/common/src/main/java/dev/cel/common/internal/CelCodePointArray.java b/common/src/main/java/dev/cel/common/internal/CelCodePointArray.java index 1f3124c93..a50ce0eea 100644 --- a/common/src/main/java/dev/cel/common/internal/CelCodePointArray.java +++ b/common/src/main/java/dev/cel/common/internal/CelCodePointArray.java @@ -36,6 +36,14 @@ public abstract class CelCodePointArray { /** Returns a new {@link CelCodePointArray} that is a subview of this between [i, j). */ public abstract CelCodePointArray slice(int i, int j); + /** + * Returns the code points between [i, j) as a string. + * + *

Equivalent to {@code slice(i, j).toString()}, but does not materialize the intermediate + * view. Lexing and parsing call this for every literal and identifier. + */ + public abstract String substring(int i, int j); + /** Get the code point at the given index. */ public abstract int get(int index); @@ -55,7 +63,9 @@ public boolean isEmpty() { } @Override - public abstract String toString(); + public final String toString() { + return substring(0, size()); + } public static CelCodePointArray fromString(String text) { if (isNullOrEmpty(text)) { diff --git a/common/src/main/java/dev/cel/common/internal/EmptyCodePointArray.java b/common/src/main/java/dev/cel/common/internal/EmptyCodePointArray.java index 8bca7bf31..32434b02c 100644 --- a/common/src/main/java/dev/cel/common/internal/EmptyCodePointArray.java +++ b/common/src/main/java/dev/cel/common/internal/EmptyCodePointArray.java @@ -14,6 +14,8 @@ package dev.cel.common.internal; +import static com.google.common.base.Preconditions.checkPositionIndexes; + import com.google.common.collect.ImmutableList; import com.google.errorprone.annotations.DoNotCall; import com.google.errorprone.annotations.Immutable; @@ -51,6 +53,12 @@ public int get(int index) { String.format("index (%s) must not be greater than size (0)", index)); } + @Override + public String substring(int i, int j) { + checkPositionIndexes(i, j, 0); + return ""; + } + @Override public int size() { return 0; @@ -60,9 +68,4 @@ public int size() { public ImmutableList lineOffsets() { return ImmutableList.of(1); } - - @Override - public String toString() { - return ""; - } } diff --git a/common/src/main/java/dev/cel/common/internal/Latin1CodePointArray.java b/common/src/main/java/dev/cel/common/internal/Latin1CodePointArray.java index 42cc0445c..1a35ef87f 100644 --- a/common/src/main/java/dev/cel/common/internal/Latin1CodePointArray.java +++ b/common/src/main/java/dev/cel/common/internal/Latin1CodePointArray.java @@ -58,13 +58,14 @@ public Latin1CodePointArray slice(int i, int j) { } @Override - public int get(int index) { - checkElementIndex(index, size()); - return Byte.toUnsignedInt(codePoints()[offset() + index]); + public String substring(int i, int j) { + checkPositionIndexes(i, j, size()); + return new String(codePoints(), offset() + i, j - i, ISO_8859_1); } @Override - public final String toString() { - return new String(codePoints(), offset(), size(), ISO_8859_1); + public int get(int index) { + checkElementIndex(index, size()); + return Byte.toUnsignedInt(codePoints()[offset() + index]); } } diff --git a/common/src/main/java/dev/cel/common/internal/SupplementalCodePointArray.java b/common/src/main/java/dev/cel/common/internal/SupplementalCodePointArray.java index 0c9214410..f66cbf64b 100644 --- a/common/src/main/java/dev/cel/common/internal/SupplementalCodePointArray.java +++ b/common/src/main/java/dev/cel/common/internal/SupplementalCodePointArray.java @@ -59,13 +59,14 @@ public SupplementalCodePointArray slice(int i, int j) { } @Override - public int get(int index) { - checkElementIndex(index, size()); - return codePoints()[offset() + index]; + public String substring(int i, int j) { + checkPositionIndexes(i, j, size()); + return new String(codePoints(), offset() + i, j - i); } @Override - public final String toString() { - return new String(codePoints(), offset(), size()); + public int get(int index) { + checkElementIndex(index, size()); + return codePoints()[offset() + index]; } } diff --git a/common/src/test/java/dev/cel/common/internal/CelCodePointArrayTest.java b/common/src/test/java/dev/cel/common/internal/CelCodePointArrayTest.java index 7cb02c5a8..9340b623b 100644 --- a/common/src/test/java/dev/cel/common/internal/CelCodePointArrayTest.java +++ b/common/src/test/java/dev/cel/common/internal/CelCodePointArrayTest.java @@ -15,6 +15,7 @@ package dev.cel.common.internal; import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.assertThrows; import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableList; @@ -39,6 +40,93 @@ public void computeLineOffset( .inOrder(); } + @Test + public void substring_empty() { + CelCodePointArray empty = CelCodePointArray.fromString(""); + assertThat(empty).isInstanceOf(EmptyCodePointArray.class); + + assertThat(empty.substring(0, 0)).isEmpty(); + assertThrows(IndexOutOfBoundsException.class, () -> empty.substring(0, 1)); + assertThrows(IndexOutOfBoundsException.class, () -> empty.substring(-1, 0)); + assertThrows(IndexOutOfBoundsException.class, () -> empty.substring(1, 0)); + assertThrows(IndexOutOfBoundsException.class, () -> empty.substring(1, 1)); + } + + @Test + public void substring_latin1() { + CelCodePointArray latin1 = CelCodePointArray.fromString("hello world"); + assertThat(latin1).isInstanceOf(Latin1CodePointArray.class); + + assertThat(latin1.substring(0, 5)).isEqualTo("hello"); + assertThat(latin1.substring(6, 11)).isEqualTo("world"); + assertThat(latin1.substring(0, 11)).isEqualTo("hello world"); + assertThat(latin1.substring(3, 3)).isEmpty(); + + assertThrows(IndexOutOfBoundsException.class, () -> latin1.substring(-1, 5)); + assertThrows(IndexOutOfBoundsException.class, () -> latin1.substring(0, 12)); + assertThrows(IndexOutOfBoundsException.class, () -> latin1.substring(5, 4)); + + // Test on a sliced subview to ensure bounds are checked against size(), not the backing buffer + // length + CelCodePointArray sliced = latin1.slice(1, 4); // "ell", size = 3, buffer length = 11 + assertThat(sliced.substring(0, 3)).isEqualTo("ell"); + assertThat(sliced.substring(1, 2)).isEqualTo("l"); + + assertThrows(IndexOutOfBoundsException.class, () -> sliced.substring(-1, 2)); + assertThrows(IndexOutOfBoundsException.class, () -> sliced.substring(0, 4)); + assertThrows(IndexOutOfBoundsException.class, () -> sliced.substring(2, 1)); + } + + @Test + public void substring_basic() { + CelCodePointArray basic = CelCodePointArray.fromString("abc \uff20 def"); + assertThat(basic).isInstanceOf(BasicCodePointArray.class); + + assertThat(basic.substring(0, 3)).isEqualTo("abc"); + assertThat(basic.substring(4, 5)).isEqualTo("\uff20"); + assertThat(basic.substring(6, 9)).isEqualTo("def"); + assertThat(basic.substring(0, 9)).isEqualTo("abc \uff20 def"); + assertThat(basic.substring(3, 3)).isEmpty(); + + assertThrows(IndexOutOfBoundsException.class, () -> basic.substring(-1, 5)); + assertThrows(IndexOutOfBoundsException.class, () -> basic.substring(0, 10)); + assertThrows(IndexOutOfBoundsException.class, () -> basic.substring(5, 4)); + + // Test on a sliced subview to ensure bounds are checked against size(), not the backing buffer + // length + CelCodePointArray sliced = basic.slice(1, 5); // "bc \uff20", size = 4, buffer length = 9 + assertThat(sliced.substring(0, 4)).isEqualTo("bc \uff20"); + + assertThrows(IndexOutOfBoundsException.class, () -> sliced.substring(-1, 2)); + assertThrows(IndexOutOfBoundsException.class, () -> sliced.substring(0, 5)); + assertThrows(IndexOutOfBoundsException.class, () -> sliced.substring(2, 1)); + } + + @Test + public void substring_supplemental() { + CelCodePointArray supp = CelCodePointArray.fromString(" text 가나다 😦😁😑 "); + assertThat(supp).isInstanceOf(SupplementalCodePointArray.class); + + assertThat(supp.substring(0, 5)).isEqualTo(" text"); + assertThat(supp.substring(10, 13)).isEqualTo("😦😁😑"); + assertThat(supp.substring(0, supp.size())).isEqualTo(" text 가나다 😦😁😑 "); + assertThat(supp.substring(3, 3)).isEmpty(); + + assertThrows(IndexOutOfBoundsException.class, () -> supp.substring(-1, 5)); + int greaterThanSize = supp.size() + 1; + assertThrows(IndexOutOfBoundsException.class, () -> supp.substring(0, greaterThanSize)); + assertThrows(IndexOutOfBoundsException.class, () -> supp.substring(5, 4)); + + // Test on a sliced subview to ensure bounds are checked against size(), not the backing buffer + // length + CelCodePointArray sliced = supp.slice(1, 5); // "text", size = 4, buffer length = 15 + assertThat(sliced.substring(0, 4)).isEqualTo("text"); + + assertThrows(IndexOutOfBoundsException.class, () -> sliced.substring(-1, 2)); + assertThrows(IndexOutOfBoundsException.class, () -> sliced.substring(0, 5)); + assertThrows(IndexOutOfBoundsException.class, () -> sliced.substring(2, 1)); + } + @AutoValue abstract static class LineOffsetTestCase { abstract String text(); diff --git a/parser/src/main/java/dev/cel/parser/BUILD.bazel b/parser/src/main/java/dev/cel/parser/BUILD.bazel index 848209380..55183446f 100644 --- a/parser/src/main/java/dev/cel/parser/BUILD.bazel +++ b/parser/src/main/java/dev/cel/parser/BUILD.bazel @@ -118,6 +118,7 @@ java_library( "//common:source_location", "//common/ast", "//common/internal", + "@maven//:com_google_errorprone_error_prone_annotations", "@maven//:com_google_guava_guava", "@maven//:org_jspecify_jspecify", ], diff --git a/parser/src/main/java/dev/cel/parser/Lexer.java b/parser/src/main/java/dev/cel/parser/Lexer.java index 894cda9ce..602a6ef00 100644 --- a/parser/src/main/java/dev/cel/parser/Lexer.java +++ b/parser/src/main/java/dev/cel/parser/Lexer.java @@ -29,8 +29,6 @@ final class Lexer { enum TokenType { ERROR("error"), END("end"), - WHITESPACE("whitespace"), - COMMENT("comment"), // Keywords NULL("null"), @@ -98,11 +96,17 @@ static final class Token { final TokenType type; final int start; final int end; + final @Nullable String text; Token(TokenType type, int start, int end) { + this(type, start, end, null); + } + + Token(TokenType type, int start, int end, @Nullable String text) { this.type = type; this.start = start; this.end = end; + this.text = text; } @Override @@ -149,35 +153,28 @@ static final class LexerError { .buildOrThrow(); private final CelCodePointArray content; + private final int size; private int position; private LexerError error; Lexer(CelCodePointArray content) { this.content = content; + this.size = content.size(); this.position = 0; this.error = null; } Token lex() { + consumeWhitespaceAndComments(); int start = position; - if (position >= content.size()) { + if (position >= size) { return makeToken(TokenType.END, start, start); } int c = content.get(position); switch (c) { - case '\f': - case '\n': - case ' ': - case '\r': - case 0x0B: // \v (vertical tab) - case '\t': - { - consumeWhitespace(); - return makeToken(TokenType.WHITESPACE, start, position); - } case '.': { - if (position + 1 < content.size() && isDigit(content.get(position + 1))) { + if (position + 1 < size && isDigit(content.get(position + 1))) { return consumeNumericLiteral(); } advance(1); @@ -283,10 +280,6 @@ Token lex() { case '/': { advance(1); - if (consume('/')) { - consumeLine(); - return makeToken(TokenType.COMMENT, start, position); - } return makeToken(TokenType.SLASH, start, position); } case '&': @@ -381,6 +374,10 @@ private Token makeToken(TokenType type, int start, int end) { return new Token(type, start, end); } + private Token makeToken(TokenType type, int start, int end, @Nullable String text) { + return new Token(type, start, end, text); + } + private Token setError(int start, int end, String message) { this.error = new LexerError(start, end, message); return new Token(TokenType.ERROR, start, end); @@ -391,7 +388,7 @@ private void advance(int n) { } private boolean match(int c) { - return position < content.size() && content.get(position) == c; + return position < size && content.get(position) == c; } private boolean consume(int c) { @@ -403,7 +400,7 @@ private boolean consume(int c) { } private boolean consumeIf(IntPredicate predicate) { - if (position < content.size()) { + if (position < size) { int cp = content.get(position); if (predicate.test(cp)) { advance(1); @@ -414,7 +411,7 @@ private boolean consumeIf(IntPredicate predicate) { } private void consumeLine() { - while (position < content.size()) { + while (position < size) { if (content.get(position) == '\n') { advance(1); return; @@ -423,8 +420,8 @@ private void consumeLine() { } } - private void consumeWhitespace() { - while (position < content.size()) { + private void consumeWhitespaceAndComments() { + while (position < size) { int c = content.get(position); switch (c) { case '\f': @@ -433,8 +430,15 @@ private void consumeWhitespace() { case '\r': case 11: // \v case '\t': - advance(1); + position++; break; + case '/': + if (position + 1 < size && content.get(position + 1) == '/') { + consumeLine(); + break; + } else { + return; + } default: return; } @@ -442,29 +446,19 @@ private void consumeWhitespace() { } private boolean consumeDigits() { - boolean advanced = false; - while (position < content.size()) { - int c = content.get(position); - if (!isDigit(c)) { - break; - } - advance(1); - advanced = true; + int start = position; + while (position < size && isDigit(content.get(position))) { + position++; } - return advanced; + return position > start; } private boolean consumeHexDigits() { - boolean advanced = false; - while (position < content.size()) { - int c = content.get(position); - if (!isHexDigit(c)) { - break; - } - advance(1); - advanced = true; + int start = position; + while (position < size && isHexDigit(content.get(position))) { + position++; } - return advanced; + return position > start; } private TokenType consumeIntegralSuffix() { @@ -486,7 +480,7 @@ private Token consumeQuotedIdent() { private boolean consumeUntilAfter(int c, boolean isRaw) { int pos = position; boolean escaped = false; - while (pos < content.size()) { + while (pos < size) { int cc = content.get(pos); if (cc == '\n' || cc == '\r') { position = pos; @@ -503,20 +497,20 @@ private boolean consumeUntilAfter(int c, boolean isRaw) { } pos++; } - position = content.size(); + position = size; return false; } private boolean consumeUntilAfterTripleQuote(int quote, boolean isRaw) { int pos = position; boolean escaped = false; - while (pos < content.size()) { + while (pos < size) { int cc = content.get(pos); if (!isRaw && cc == '\\') { escaped = !escaped; } else { if ((isRaw || !escaped) - && pos + 2 < content.size() + && pos + 2 < size && cc == quote && content.get(pos + 1) == quote && content.get(pos + 2) == quote) { @@ -527,16 +521,14 @@ private boolean consumeUntilAfterTripleQuote(int quote, boolean isRaw) { } pos++; } - position = content.size(); + position = size; return false; } private Token consumeStringLiteral(int start, int quote, boolean isBytes, boolean isRaw) { advance(1); boolean isTripleQuote = - position + 1 < content.size() - && content.get(position) == quote - && content.get(position + 1) == quote; + position + 1 < size && content.get(position) == quote && content.get(position + 1) == quote; if (isTripleQuote) { advance(2); if (!consumeUntilAfterTripleQuote(quote, isRaw)) { @@ -556,7 +548,7 @@ private Token consumeStringLiteral(int start, int quote, boolean isBytes, boolea private @Nullable Token consumePrefixedStringLiteral() { int start = position; - if (position >= content.size()) { + if (position >= size) { return null; } int c = content.get(position); @@ -566,7 +558,7 @@ private Token consumeStringLiteral(int start, int quote, boolean isBytes, boolea return null; } int lookahead = 1; - if (position + 1 < content.size()) { + if (position + 1 < size) { int c2 = content.get(position + 1); if (isBytes ? (c2 == 'r' || c2 == 'R') : (c2 == 'b' || c2 == 'B')) { isBytes = true; @@ -574,7 +566,7 @@ private Token consumeStringLiteral(int start, int quote, boolean isBytes, boolea lookahead = 2; } } - if (position + lookahead < content.size()) { + if (position + lookahead < size) { int quote = content.get(position + lookahead); if (quote == '"' || quote == '\'') { advance(lookahead); @@ -612,9 +604,9 @@ private Token consumeNumericLiteral() { return makeToken(tokenType, start, position); } consumeDigits(); - if (position < content.size() + if (position < size && content.get(position) == '.' - && position + 1 < content.size() + && position + 1 < size && isDigit(content.get(position + 1))) { floatingPoint = true; advance(1); @@ -639,19 +631,15 @@ && isDigit(content.get(position + 1))) { private Token consumeIdent() { int start = position; - while (position < content.size()) { - int c = content.get(position); - if (!isIdentTrailing(c)) { - break; - } - advance(1); + while (position < size && isIdentTrailing(content.get(position))) { + position++; } int end = position; - String word = content.slice(start, end).toString(); + String word = content.substring(start, end); TokenType keywordType = KEYWORDS.get(word); if (keywordType != null) { return makeToken(keywordType, start, end); } - return makeToken(TokenType.IDENT, start, end); + return makeToken(TokenType.IDENT, start, end, word); } } diff --git a/parser/src/main/java/dev/cel/parser/PrattParser.java b/parser/src/main/java/dev/cel/parser/PrattParser.java index 17ce514d5..dceaadad6 100644 --- a/parser/src/main/java/dev/cel/parser/PrattParser.java +++ b/parser/src/main/java/dev/cel/parser/PrattParser.java @@ -30,6 +30,7 @@ import java.text.ParseException; import java.util.ArrayDeque; import java.util.ArrayList; +import java.util.Arrays; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -41,6 +42,9 @@ final class PrattParser { private static final String ACCUMULATOR_NAME = "@result"; private static final CelExpr ERROR = CelExpr.newBuilder().setConstant(Constants.ERROR).build(); + private static final Lexer.Token END_TOKEN = new Lexer.Token(Lexer.TokenType.END, 0, 0); + /** Most logical chains are short; 8 avoids resizing for the overwhelming majority. */ + private static final int INITIAL_CHAIN_CAPACITY = 8; private static final class BinaryOpInfo { final int precedence; @@ -56,72 +60,49 @@ private static final class BinaryOpInfo { } } - private static final BinaryOpInfo LOGICAL_OR_OP = - new BinaryOpInfo(1, Operator.LOGICAL_OR.getFunction(), true, Lexer.TokenType.LOGICAL_OR); - private static final BinaryOpInfo LOGICAL_AND_OP = - new BinaryOpInfo(2, Operator.LOGICAL_AND.getFunction(), true, Lexer.TokenType.LOGICAL_AND); - private static final BinaryOpInfo LESS_OP = - new BinaryOpInfo(3, Operator.LESS.getFunction(), false, Lexer.TokenType.LESS); - private static final BinaryOpInfo LESS_EQUAL_OP = - new BinaryOpInfo(3, Operator.LESS_EQUALS.getFunction(), false, Lexer.TokenType.LESS_EQUAL); - private static final BinaryOpInfo GREATER_OP = - new BinaryOpInfo(3, Operator.GREATER.getFunction(), false, Lexer.TokenType.GREATER); - private static final BinaryOpInfo GREATER_EQUAL_OP = - new BinaryOpInfo( - 3, Operator.GREATER_EQUALS.getFunction(), false, Lexer.TokenType.GREATER_EQUAL); - private static final BinaryOpInfo EQUAL_EQUAL_OP = - new BinaryOpInfo(3, Operator.EQUALS.getFunction(), false, Lexer.TokenType.EQUAL_EQUAL); - private static final BinaryOpInfo EXCLAMATION_EQUAL_OP = - new BinaryOpInfo( - 3, Operator.NOT_EQUALS.getFunction(), false, Lexer.TokenType.EXCLAMATION_EQUAL); - private static final BinaryOpInfo IN_OP = - new BinaryOpInfo(3, Operator.IN.getFunction(), false, Lexer.TokenType.IN); - private static final BinaryOpInfo PLUS_OP = - new BinaryOpInfo(4, Operator.ADD.getFunction(), false, Lexer.TokenType.PLUS); - private static final BinaryOpInfo MINUS_OP = - new BinaryOpInfo(4, Operator.SUBTRACT.getFunction(), false, Lexer.TokenType.MINUS); - private static final BinaryOpInfo ASTERISK_OP = - new BinaryOpInfo(5, Operator.MULTIPLY.getFunction(), false, Lexer.TokenType.ASTERISK); - private static final BinaryOpInfo SLASH_OP = - new BinaryOpInfo(5, Operator.DIVIDE.getFunction(), false, Lexer.TokenType.SLASH); - private static final BinaryOpInfo PERCENT_OP = - new BinaryOpInfo(5, Operator.MODULO.getFunction(), false, Lexer.TokenType.PERCENT); - private static final BinaryOpInfo DEFAULT_OP = - new BinaryOpInfo(0, "", false, Lexer.TokenType.ERROR); - - private static BinaryOpInfo getBinaryOpInfo(Lexer.TokenType type) { - switch (type) { - case LOGICAL_OR: - return LOGICAL_OR_OP; - case LOGICAL_AND: - return LOGICAL_AND_OP; - case LESS: - return LESS_OP; - case LESS_EQUAL: - return LESS_EQUAL_OP; - case GREATER: - return GREATER_OP; - case GREATER_EQUAL: - return GREATER_EQUAL_OP; - case EQUAL_EQUAL: - return EQUAL_EQUAL_OP; - case EXCLAMATION_EQUAL: - return EXCLAMATION_EQUAL_OP; - case IN: - return IN_OP; - case PLUS: - return PLUS_OP; - case MINUS: - return MINUS_OP; - case ASTERISK: - return ASTERISK_OP; - case SLASH: - return SLASH_OP; - case PERCENT: - return PERCENT_OP; - default: - return DEFAULT_OP; - } + private static final BinaryOpInfo[] binaryOps = initBinaryOps(); + + // Safe and desirable to use .ordinal() here: + // 1. Safe: This lookup table is strictly private and internal to PrattParser, never serialized or + // persisted. The array is sized to TokenType.values().length, so indexing by ordinal is + // guaranteed to be within bounds even if enum members change. + // 2. Desirable: Expression parsing checks binary operator info on every token in the input; + // direct array indexing by ordinal provides O(1) lookup with zero hashing, indirection, + // or boxing overhead on this critical hot path. + @SuppressWarnings("EnumOrdinal") + private static BinaryOpInfo[] initBinaryOps() { + BinaryOpInfo[] ops = new BinaryOpInfo[Lexer.TokenType.values().length]; + ops[Lexer.TokenType.LOGICAL_OR.ordinal()] = + new BinaryOpInfo(1, Operator.LOGICAL_OR.getFunction(), true, Lexer.TokenType.LOGICAL_OR); + ops[Lexer.TokenType.LOGICAL_AND.ordinal()] = + new BinaryOpInfo(2, Operator.LOGICAL_AND.getFunction(), true, Lexer.TokenType.LOGICAL_AND); + ops[Lexer.TokenType.LESS.ordinal()] = + new BinaryOpInfo(3, Operator.LESS.getFunction(), false, Lexer.TokenType.LESS); + ops[Lexer.TokenType.LESS_EQUAL.ordinal()] = + new BinaryOpInfo(3, Operator.LESS_EQUALS.getFunction(), false, Lexer.TokenType.LESS_EQUAL); + ops[Lexer.TokenType.GREATER.ordinal()] = + new BinaryOpInfo(3, Operator.GREATER.getFunction(), false, Lexer.TokenType.GREATER); + ops[Lexer.TokenType.GREATER_EQUAL.ordinal()] = + new BinaryOpInfo( + 3, Operator.GREATER_EQUALS.getFunction(), false, Lexer.TokenType.GREATER_EQUAL); + ops[Lexer.TokenType.EQUAL_EQUAL.ordinal()] = + new BinaryOpInfo(3, Operator.EQUALS.getFunction(), false, Lexer.TokenType.EQUAL_EQUAL); + ops[Lexer.TokenType.EXCLAMATION_EQUAL.ordinal()] = + new BinaryOpInfo( + 3, Operator.NOT_EQUALS.getFunction(), false, Lexer.TokenType.EXCLAMATION_EQUAL); + ops[Lexer.TokenType.IN.ordinal()] = + new BinaryOpInfo(3, Operator.IN.getFunction(), false, Lexer.TokenType.IN); + ops[Lexer.TokenType.PLUS.ordinal()] = + new BinaryOpInfo(4, Operator.ADD.getFunction(), false, Lexer.TokenType.PLUS); + ops[Lexer.TokenType.MINUS.ordinal()] = + new BinaryOpInfo(4, Operator.SUBTRACT.getFunction(), false, Lexer.TokenType.MINUS); + ops[Lexer.TokenType.ASTERISK.ordinal()] = + new BinaryOpInfo(5, Operator.MULTIPLY.getFunction(), false, Lexer.TokenType.ASTERISK); + ops[Lexer.TokenType.SLASH.ordinal()] = + new BinaryOpInfo(5, Operator.DIVIDE.getFunction(), false, Lexer.TokenType.SLASH); + ops[Lexer.TokenType.PERCENT.ordinal()] = + new BinaryOpInfo(5, Operator.MODULO.getFunction(), false, Lexer.TokenType.PERCENT); + return ops; } private static final class UnaryOp { @@ -138,10 +119,9 @@ private static final class UnaryOp { private final ImmutableMap macros; private final Lexer lexer; private final Map positions; - private final Map macroCalls; + private Map macroCalls = ImmutableMap.of(); + private PrattMacroExprFactory macroExprFactory; private final List issues; - private final PrattMacroExprFactory macroExprFactory; - private Lexer.Token currentToken; private Lexer.Token peekToken; private int recursionDepth; @@ -184,11 +164,9 @@ private PrattParser(CelSource source, CelOptions options, Map this.macros = ImmutableMap.copyOf(macros); this.lexer = new Lexer(source.getContent()); this.positions = new HashMap<>(); - this.macroCalls = new HashMap<>(); this.issues = new ArrayList<>(); - this.macroExprFactory = new PrattMacroExprFactory(); this.nextId = 1; - initTokenStream(); + peekToken = nextSignificantToken(true); } CelExpr run() { @@ -215,40 +193,31 @@ private boolean isRecoveryLimitExceeded() { return errorCount > options.maxParseErrorRecoveryLimit(); } - private void initTokenStream() { - peekToken = nextSignificantToken(true); - } - private String getTokenText(Lexer.Token tok) { + if (tok.text != null) { + return tok.text; + } if (tok.start >= 0 && tok.end >= tok.start && tok.end <= source.getContent().size()) { - return source.getContent().slice(tok.start, tok.end).toString(); + return source.getContent().substring(tok.start, tok.end); } return ""; } private Lexer.Token nextSignificantToken(boolean reportError) { - if (isRecoveryLimitExceeded()) { - return new Lexer.Token(Lexer.TokenType.END, 0, 0); - } - while (true) { - Lexer.Token tok = lexer.lex(); - if (tok.type == Lexer.TokenType.WHITESPACE || tok.type == Lexer.TokenType.COMMENT) { - continue; + Lexer.Token tok = lexer.lex(); + if (tok.type == Lexer.TokenType.ERROR && reportError) { + reportSyntaxError(tok, lexer.getError().message); + if (isRecoveryLimitExceeded()) { + return END_TOKEN; } - if (tok.type == Lexer.TokenType.ERROR && reportError) { - reportSyntaxError(tok, lexer.getError().message); - if (isRecoveryLimitExceeded()) { - return new Lexer.Token(Lexer.TokenType.END, 0, 0); - } - } - return tok; } + return tok; } private Lexer.Token nextToken() { currentToken = peekToken; if (isRecoveryLimitExceeded()) { - peekToken = new Lexer.Token(Lexer.TokenType.END, 0, 0); + peekToken = END_TOKEN; return currentToken; } if (peekToken.type != Lexer.TokenType.END) { @@ -283,7 +252,7 @@ private boolean expect(Lexer.TokenType type, String msg) { private void synchronizeOnDelimiter() { if (isRecoveryLimitExceeded()) { - peekToken = new Lexer.Token(Lexer.TokenType.END, 0, 0); + peekToken = END_TOKEN; return; } while (peekToken.type != Lexer.TokenType.END) { @@ -356,7 +325,7 @@ private void reportError(CelSourceLocation loc, String msg) { CelIssue.formatError( CelSourceLocation.NONE, String.format("More than %d parse errors.", options.maxParseErrorRecoveryLimit()))); - peekToken = new Lexer.Token(Lexer.TokenType.END, 0, 0); + peekToken = END_TOKEN; } if (errorCount <= options.maxParseErrorRecoveryLimit()) { issues.add(CelIssue.formatError(loc, msg)); @@ -369,33 +338,37 @@ private void reportSyntaxError(Lexer.Token token, String msg) { private boolean checkRecursion(int chainDepth, Lexer.Token token) { if (recursionDepth + chainDepth > options.maxParseRecursionDepth()) { - if (!recursionLimitExceeded) { - recursionLimitExceeded = true; - reportError( - token.start, - String.format( - "Expression recursion limit exceeded. limit: %d", - options.maxParseRecursionDepth())); - } + reportRecursionLimit(token.start); return true; } return false; } + private void reportRecursionLimit(int position) { + if (!recursionLimitExceeded) { + recursionLimitExceeded = true; + reportError( + position, + String.format( + "Expression recursion limit exceeded. limit: %d", options.maxParseRecursionDepth())); + } + } + private CelExpr parseExpr() { - if (recursionLimitExceeded || isRecoveryLimitExceeded()) { + if (recursionLimitExceeded || errorCount > options.maxParseErrorRecoveryLimit()) { return ERROR; } - recursionDepth++; - if (checkRecursion(0, peekToken)) { - recursionDepth--; + if (recursionDepth >= options.maxParseRecursionDepth()) { + reportRecursionLimit(peekToken.start); return ERROR; } + recursionDepth++; CelExpr expr = parseBinaryAndTernary(0); recursionDepth--; return expr; } + @SuppressWarnings("EnumOrdinal") // Using ordinal for O(1) binary operator lookup table private CelExpr parseBinaryAndTernary(int minPrec) { CelExpr lhs = parseSelectorChain(); int chainDepth = currentLhsDepth; @@ -406,8 +379,8 @@ private CelExpr parseBinaryAndTernary(int minPrec) { continue; } - BinaryOpInfo opInfo = getBinaryOpInfo(tok); - if (opInfo.precedence < minPrec || opInfo.precedence == 0) { + BinaryOpInfo opInfo = binaryOps[tok.ordinal()]; + if (opInfo == null || opInfo.precedence < minPrec) { break; } @@ -417,7 +390,8 @@ private CelExpr parseBinaryAndTernary(int minPrec) { } Lexer.Token opTok = nextToken(); - if (checkRecursion(chainDepth, opTok)) { + if (recursionDepth + chainDepth > options.maxParseRecursionDepth()) { + reportRecursionLimit(opTok.start); return ERROR; } chainDepth++; @@ -449,51 +423,60 @@ private CelExpr parseTernary(CelExpr lhs) { .build(); } - private CelExpr buildBinaryCall(long opId, String opName, CelExpr lhs, CelExpr rhs) { - return CelExpr.newBuilder() - .setId(opId) - .setCall(CelExpr.CelCall.newBuilder().setFunction(opName).addArgs(lhs).addArgs(rhs).build()) - .build(); - } - private CelExpr parseBalancedLogicalChain(CelExpr lhs, BinaryOpInfo opInfo) { - List terms = new ArrayList<>(); - List ops = new ArrayList<>(); - terms.add(lhs); + Lexer.Token opTok = nextToken(); + long opId = nextId(opTok.start); + CelExpr rhs = parseBinaryAndTernary(opInfo.precedence + 1); + if (peekToken.type != opInfo.type) { + return buildBinaryCall(opId, opInfo.name, lhs, rhs); + } + + CelExpr[] terms = new CelExpr[INITIAL_CHAIN_CAPACITY]; + long[] ops = new long[INITIAL_CHAIN_CAPACITY]; + terms[0] = lhs; + terms[1] = rhs; + ops[0] = opId; + int opsCount = 1; + int termsCount = 2; + while (peekToken.type == opInfo.type) { - Lexer.Token opTok = nextToken(); - long opId = nextId(opTok); - CelExpr rhs = parseBinaryAndTernary(opInfo.precedence + 1); - ops.add(opId); - terms.add(rhs); + opTok = nextToken(); + opId = nextId(opTok.start); + rhs = parseBinaryAndTernary(opInfo.precedence + 1); + if (termsCount == terms.length) { + int newCapacity = terms.length * 2; + ops = Arrays.copyOf(ops, newCapacity); + terms = Arrays.copyOf(terms, newCapacity); + } + ops[opsCount++] = opId; + terms[termsCount++] = rhs; } - return balancedTree(opInfo.name, terms, ops, 0, ops.size() - 1); + return balancedTree(opInfo.name, terms, ops, 0, opsCount - 1); } - private CelExpr balancedTree(String op, List terms, List ops, int lo, int hi) { + private CelExpr balancedTree(String op, CelExpr[] terms, long[] ops, int lo, int hi) { int mid = (lo + hi + 1) / 2; - CelExpr left; - if (mid == lo) { - left = terms.get(mid); - } else { - left = balancedTree(op, terms, ops, lo, mid - 1); - } - CelExpr right; - if (mid == hi) { - right = terms.get(mid + 1); - } else { - right = balancedTree(op, terms, ops, mid + 1, hi); - } + CelExpr left = (mid == lo) ? terms[mid] : balancedTree(op, terms, ops, lo, mid - 1); + CelExpr right = (mid == hi) ? terms[mid + 1] : balancedTree(op, terms, ops, mid + 1, hi); + return buildBinaryCall(ops[mid], op, left, right); + } + + private static CelExpr buildBinaryCall(long id, String function, CelExpr lhs, CelExpr rhs) { return CelExpr.newBuilder() - .setId(ops.get(mid)) - .setCall(CelExpr.CelCall.newBuilder().setFunction(op).addArgs(left).addArgs(right).build()) + .setId(id) + .setCall( + CelExpr.CelCall.newBuilder().setFunction(function).addArgs(lhs).addArgs(rhs).build()) .build(); } private CelExpr parseSelectorChain() { - CelExpr lhs = parseUnary(); - currentLhsDepth = 0; Lexer.TokenType tok = peekToken.type; + CelExpr lhs = + (tok == Lexer.TokenType.EXCLAMATION || tok == Lexer.TokenType.MINUS) + ? parseUnaryOps() + : parsePrimary(); + currentLhsDepth = 0; + tok = peekToken.type; if (tok == Lexer.TokenType.DOT || tok == Lexer.TokenType.LEFT_BRACKET || tok == Lexer.TokenType.LEFT_BRACE) { @@ -538,10 +521,7 @@ private CelExpr parseSelectorChainTail(CelExpr initialLhs) { long opId = nextId(dotTok); CelExpr arg1 = lhs; CelExpr arg2 = - CelExpr.newBuilder() - .setId(nextId(getLeftmostPosition(lhs))) - .setConstant(CelConstant.ofValue(idText)) - .build(); + CelExpr.ofConstant(nextId(getLeftmostPosition(lhs)), CelConstant.ofValue(idText)); lhs = CelExpr.newBuilder() .setId(opId) @@ -572,12 +552,7 @@ private CelExpr parseSelectorChainTail(CelExpr initialLhs) { .build(); } } else { - lhs = - CelExpr.newBuilder() - .setId(nextId(dotTok)) - .setSelect( - CelExpr.CelSelect.newBuilder().setOperand(lhs).setField(idText).build()) - .build(); + lhs = CelExpr.ofSelect(nextId(dotTok), lhs, idText, /* isTestOnly= */ false); } } else if (tok == Lexer.TokenType.LEFT_BRACKET) { if (checkRecursion(chainDepth, peekToken)) { @@ -609,7 +584,7 @@ private CelExpr parseSelectorChainTail(CelExpr initialLhs) { .build()) .build(); } else if (tok == Lexer.TokenType.LEFT_BRACE) { - String structName = extractStructName(lhs).orElse(null); + String structName = extractStructName(lhs); if (structName == null) { break; } @@ -622,14 +597,6 @@ private CelExpr parseSelectorChainTail(CelExpr initialLhs) { return lhs; } - private CelExpr parseUnary() { - Lexer.TokenType tok = peekToken.type; - if (tok == Lexer.TokenType.EXCLAMATION || tok == Lexer.TokenType.MINUS) { - return parseUnaryOps(); - } - return parsePrimary(); - } - private CelExpr parseUnaryOps() { Lexer.Token op = nextToken(); Lexer.TokenType opType = op.type; @@ -783,10 +750,7 @@ private CelExpr parseIdentOrCall() { .build(); } long id = nextId(leadingDot ? firstTok : idTok); - return CelExpr.newBuilder() - .setId(id) - .setIdent(CelExpr.CelIdent.newBuilder().setName(name).build()) - .build(); + return CelExpr.ofIdent(id, name); } private CelExpr parsePrimary() { @@ -807,15 +771,13 @@ private CelExpr parsePrimary() { return expr; } case NULL: - return CelExpr.newBuilder().setId(nextId(nextToken())).setConstant(Constants.NULL).build(); + return CelExpr.ofConstant(nextId(nextToken()), Constants.NULL); case TRUE: case FALSE: { Lexer.Token tok = nextToken(); - return CelExpr.newBuilder() - .setId(nextId(tok)) - .setConstant(tok.type == Lexer.TokenType.TRUE ? Constants.TRUE : Constants.FALSE) - .build(); + return CelExpr.ofConstant( + nextId(tok), tok.type == Lexer.TokenType.TRUE ? Constants.TRUE : Constants.FALSE); } case INT: return parseIntLiteral(/* nodeId= */ -1, /* isNegative= */ false); @@ -990,7 +952,7 @@ private CelExpr parseIntLiteral(long nodeId, boolean isNegative) { long id = nodeId == -1 ? nextId(tok) : nodeId; try { CelConstant constExpr = Constants.parseInt(text); - return CelExpr.newBuilder().setId(id).setConstant(constExpr).build(); + return CelExpr.ofConstant(id, constExpr); } catch (ParseException e) { reportSyntaxError(tok, "invalid int literal: " + text); return CelExpr.newBuilder().setId(nextId(tok)).build(); @@ -1002,7 +964,7 @@ private CelExpr parseUintLiteral() { String value = getTokenText(tok); try { CelConstant constExpr = Constants.parseUint(value); - return CelExpr.newBuilder().setId(nextId(tok)).setConstant(constExpr).build(); + return CelExpr.ofConstant(nextId(tok), constExpr); } catch (ParseException e) { reportSyntaxError(tok, "invalid uint literal: " + value); return CelExpr.newBuilder().setId(nextId(tok)).build(); @@ -1015,7 +977,7 @@ private CelExpr parseDoubleLiteral(long nodeId, boolean isNegative) { long id = nodeId == -1 ? nextId(tok) : nodeId; try { CelConstant constExpr = Constants.parseDouble(text); - return CelExpr.newBuilder().setId(id).setConstant(constExpr).build(); + return CelExpr.ofConstant(id, constExpr); } catch (ParseException e) { reportSyntaxError(tok, "invalid double literal: " + text); return CelExpr.newBuilder().setId(nextId(tok)).build(); @@ -1027,7 +989,7 @@ private CelExpr parseStringLiteral() { String value = getTokenText(tok); try { CelConstant constExpr = Constants.parseString(value); - return CelExpr.newBuilder().setId(nextId(tok)).setConstant(constExpr).build(); + return CelExpr.ofConstant(nextId(tok), constExpr); } catch (ParseException e) { reportError(tok.start, e.getMessage()); return CelExpr.newBuilder().setId(nextId(tok)).build(); @@ -1039,7 +1001,7 @@ private CelExpr parseBytesLiteral() { String value = getTokenText(tok); try { CelConstant constExpr = Constants.parseBytes(value); - return CelExpr.newBuilder().setId(nextId(tok)).setConstant(constExpr).build(); + return CelExpr.ofConstant(nextId(tok), constExpr); } catch (ParseException e) { reportError(tok.start, e.getMessage()); return CelExpr.newBuilder().setId(nextId(tok)).build(); @@ -1084,52 +1046,53 @@ private static boolean isAsciiAlphanumeric(char c) { return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9'); } - private Optional extractStructName(CelExpr expr) { + private @Nullable String extractStructName(CelExpr expr) { if (expr.exprKind().getKind() == CelExpr.ExprKind.Kind.IDENT) { String name = expr.ident().name(); eraseId(expr.id()); - return Optional.of(name); + return name; } if (expr.exprKind().getKind() == CelExpr.ExprKind.Kind.SELECT) { if (expr.select().testOnly()) { - return Optional.empty(); + return null; } CelExpr operand = expr.select().operand(); eraseId(expr.id()); - return extractStructName(operand).map(prefix -> prefix + "." + expr.select().field()); + String prefix = extractStructName(operand); + return prefix != null ? prefix + "." + expr.select().field() : null; } - return Optional.empty(); + return null; } private int getLeftmostPosition(CelExpr expr) { - if (expr.exprKind().getKind() == CelExpr.ExprKind.Kind.IDENT) { - return positions.getOrDefault(expr.id(), 0); - } - if (expr.exprKind().getKind() == CelExpr.ExprKind.Kind.SELECT) { - return getLeftmostPosition(expr.select().operand()); + while (expr.exprKind().getKind() == CelExpr.ExprKind.Kind.SELECT) { + expr = expr.select().operand(); } return positions.getOrDefault(expr.id(), 0); } - private Optional lookupMacro(String id, int argCount, boolean receiverStyle) { + private @Nullable CelMacro lookupMacro(String id, int argCount, boolean receiverStyle) { + if (macros.isEmpty()) { + return null; + } String key = CelMacro.formatKey(id, argCount, receiverStyle); CelMacro macro = macros.get(key); if (macro != null) { - return Optional.of(macro); + return macro; } key = CelMacro.formatVarArgKey(id, receiverStyle); - return Optional.ofNullable(macros.get(key)); + return macros.get(key); } private Optional tryExpandMacro( long exprId, String function, @Nullable CelExpr target, ImmutableList args) { - if (function.isEmpty()) { + if (function.isEmpty() || macros.isEmpty()) { return Optional.empty(); } boolean isReceiver = (target != null); int argCount = args.size(); - Optional macro = lookupMacro(function, argCount, isReceiver); - if (!macro.isPresent()) { + CelMacro macro = lookupMacro(function, argCount, isReceiver); + if (macro == null) { return Optional.empty(); } if (nodeLimitExceeded) { @@ -1139,15 +1102,14 @@ private Optional tryExpandMacro( return Optional.empty(); } - Optional errorArg = args.stream().filter(ERROR::equals).findAny(); - if (errorArg.isPresent() || (target != null && target.equals(ERROR))) { + if ((target != null && target.equals(ERROR)) || hasError(args)) { eraseId(exprId); return Optional.of(ERROR); } int macroPosition = positions.getOrDefault(exprId, 0); CelExpr targetExpr = (target != null ? target : CelExpr.newBuilder().build()); - Optional expandedExpr = expandMacro(macroPosition, macro.get(), targetExpr, args); + Optional expandedExpr = expandMacro(macroPosition, macro, targetExpr, args); if (expandedExpr.isPresent()) { if (options.populateMacroCalls()) { @@ -1159,8 +1121,20 @@ private Optional tryExpandMacro( return Optional.empty(); } + private static boolean hasError(List args) { + for (int i = 0; i < args.size(); i++) { + if (args.get(i).equals(ERROR)) { + return true; + } + } + return false; + } + private Optional expandMacro( int position, CelMacro macro, CelExpr target, ImmutableList arguments) { + if (macroExprFactory == null) { + macroExprFactory = new PrattMacroExprFactory(); + } macroExprFactory.pushPosition(position); try { return macro.getExpander().expandMacro(macroExprFactory, target, arguments); @@ -1171,6 +1145,9 @@ private Optional expandMacro( private void recordMacroCall( long macroId, String function, CelExpr target, ImmutableList args) { + if (!(macroCalls instanceof HashMap)) { + macroCalls = new HashMap<>(); + } CelExpr.CelCall.Builder callExpr = CelExpr.CelCall.newBuilder().setFunction(function); if (target != null) { if (macroCalls.containsKey(target.id())) { @@ -1205,6 +1182,24 @@ private int countGroupingParentheses() { return 0; } + // Fast path: if the next non-whitespace character is not '(', leading open parens is 1. + int pos = peekToken.end; + while (pos < source.getContent().size()) { + int c = source.getContent().get(pos); + if (c != ' ' && c != '\t' && c != '\n' && c != '\r' && c != '\f' && c != 11) { + if (c == '/') { + // A comment might precede another '('. + break; + } + if (c == '(') { + break; + } + // Next significant token is definitely not '('. + return 1; + } + pos++; + } + int savedPos = lexer.savePosition(); try { int leadingOpenParens = 1; diff --git a/parser/src/test/java/dev/cel/parser/CelParserImplTest.java b/parser/src/test/java/dev/cel/parser/CelParserImplTest.java index 37501ec29..5b7f9defb 100644 --- a/parser/src/test/java/dev/cel/parser/CelParserImplTest.java +++ b/parser/src/test/java/dev/cel/parser/CelParserImplTest.java @@ -17,6 +17,7 @@ import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertThrows; +import com.google.common.base.Joiner; import com.google.common.collect.ImmutableSet; import com.google.testing.junit.testparameterinjector.TestParameter; import com.google.testing.junit.testparameterinjector.TestParameterInjector; @@ -27,6 +28,7 @@ import dev.cel.common.CelValidationException; import dev.cel.common.CelValidationResult; import dev.cel.common.ast.CelExpr; +import java.util.Collections; import java.util.Optional; import org.junit.Test; import org.junit.runner.RunWith; @@ -396,4 +398,14 @@ public void toParserBuilder_collectionProperties_copied() { assertThat(newParserBuilder.getMacros()).hasSize(1); assertThat(newParserBuilder.getParserLibraries().build()).hasSize(1); } + + @Test + public void parse_logicalChainLongerThanInitialCapacity_succeeds() { + CelParser parser = newParserBuilder().build(); + for (int operands = 2; operands <= 64; operands++) { + String expr = Joiner.on(" || ").join(Collections.nCopies(operands, "true")); + CelValidationResult result = parser.parse(expr); + assertThat(result.hasError()).isFalse(); + } + } } diff --git a/parser/src/test/java/dev/cel/parser/CelParserParameterizedTest.java b/parser/src/test/java/dev/cel/parser/CelParserParameterizedTest.java index 7e19e24f8..0f9fb36b7 100644 --- a/parser/src/test/java/dev/cel/parser/CelParserParameterizedTest.java +++ b/parser/src/test/java/dev/cel/parser/CelParserParameterizedTest.java @@ -372,6 +372,7 @@ public void parser_core_syntax() { runTest("a || b || c || d || e || f"); runTest("a < 5 || a > 10"); runTest("a && b && c && d || e && f && g && h"); + runTest("a || b && c || d && e || f && g || h && i || j && k || l"); // Conditional operator runTest("a?b:c"); diff --git a/parser/src/test/java/dev/cel/parser/PrattParserTest.java b/parser/src/test/java/dev/cel/parser/PrattParserTest.java index e9394b362..710ff2fcf 100644 --- a/parser/src/test/java/dev/cel/parser/PrattParserTest.java +++ b/parser/src/test/java/dev/cel/parser/PrattParserTest.java @@ -295,6 +295,7 @@ public void pratt_parser_core_syntax() { runTest("a || b || c || d || e || f"); runTest("a < 5 || a > 10"); runTest("a && b && c && d || e && f && g && h"); + runTest("a || b && c || d && e || f && g || h && i || j && k || l"); // Conditional operator runTest("a?b:c"); diff --git a/parser/src/test/resources/parser_core_syntax.baseline b/parser/src/test/resources/parser_core_syntax.baseline index 7c05685f3..34997c9d8 100644 --- a/parser/src/test/resources/parser_core_syntax.baseline +++ b/parser/src/test/resources/parser_core_syntax.baseline @@ -1061,6 +1061,77 @@ L: _||_( )^#12[1,27]# )^#8[1,17]# +I: a || b && c || d && e || f && g || h && i || j && k || l +=====> +P: _||_( + _||_( + _||_( + a^#1:Expr.Ident#, + _&&_( + b^#3:Expr.Ident#, + c^#5:Expr.Ident# + )^#4:Expr.Call# + )^#2:Expr.Call#, + _||_( + _&&_( + d^#7:Expr.Ident#, + e^#9:Expr.Ident# + )^#8:Expr.Call#, + _&&_( + f^#11:Expr.Ident#, + g^#13:Expr.Ident# + )^#12:Expr.Call# + )^#10:Expr.Call# + )^#6:Expr.Call#, + _||_( + _||_( + _&&_( + h^#15:Expr.Ident#, + i^#17:Expr.Ident# + )^#16:Expr.Call#, + _&&_( + j^#19:Expr.Ident#, + k^#21:Expr.Ident# + )^#20:Expr.Call# + )^#18:Expr.Call#, + l^#23:Expr.Ident# + )^#22:Expr.Call# +)^#14:Expr.Call# +L: _||_( + _||_( + _||_( + a^#1[1,0]#, + _&&_( + b^#3[1,5]#, + c^#5[1,10]# + )^#4[1,7]# + )^#2[1,2]#, + _||_( + _&&_( + d^#7[1,15]#, + e^#9[1,20]# + )^#8[1,17]#, + _&&_( + f^#11[1,25]#, + g^#13[1,30]# + )^#12[1,27]# + )^#10[1,22]# + )^#6[1,12]#, + _||_( + _||_( + _&&_( + h^#15[1,35]#, + i^#17[1,40]# + )^#16[1,37]#, + _&&_( + j^#19[1,45]#, + k^#21[1,50]# + )^#20[1,47]# + )^#18[1,42]#, + l^#23[1,55]# + )^#22[1,52]# +)^#14[1,32]# + I: a?b:c =====> P: _?_:_( diff --git a/parser/src/test/resources/pratt_parser_core_syntax.baseline b/parser/src/test/resources/pratt_parser_core_syntax.baseline index 02f44e87c..fb9d94e58 100644 --- a/parser/src/test/resources/pratt_parser_core_syntax.baseline +++ b/parser/src/test/resources/pratt_parser_core_syntax.baseline @@ -1043,6 +1043,77 @@ L: _||_( )^#12[1,27]# )^#8[1,17]# +I: a || b && c || d && e || f && g || h && i || j && k || l +=====> +P: _||_( + _||_( + _||_( + a^#1:Expr.Ident#, + _&&_( + b^#3:Expr.Ident#, + c^#5:Expr.Ident# + )^#4:Expr.Call# + )^#2:Expr.Call#, + _||_( + _&&_( + d^#7:Expr.Ident#, + e^#9:Expr.Ident# + )^#8:Expr.Call#, + _&&_( + f^#11:Expr.Ident#, + g^#13:Expr.Ident# + )^#12:Expr.Call# + )^#10:Expr.Call# + )^#6:Expr.Call#, + _||_( + _||_( + _&&_( + h^#15:Expr.Ident#, + i^#17:Expr.Ident# + )^#16:Expr.Call#, + _&&_( + j^#19:Expr.Ident#, + k^#21:Expr.Ident# + )^#20:Expr.Call# + )^#18:Expr.Call#, + l^#23:Expr.Ident# + )^#22:Expr.Call# +)^#14:Expr.Call# +L: _||_( + _||_( + _||_( + a^#1[1,0]#, + _&&_( + b^#3[1,5]#, + c^#5[1,10]# + )^#4[1,7]# + )^#2[1,2]#, + _||_( + _&&_( + d^#7[1,15]#, + e^#9[1,20]# + )^#8[1,17]#, + _&&_( + f^#11[1,25]#, + g^#13[1,30]# + )^#12[1,27]# + )^#10[1,22]# + )^#6[1,12]#, + _||_( + _||_( + _&&_( + h^#15[1,35]#, + i^#17[1,40]# + )^#16[1,37]#, + _&&_( + j^#19[1,45]#, + k^#21[1,50]# + )^#20[1,47]# + )^#18[1,42]#, + l^#23[1,55]# + )^#22[1,52]# +)^#14[1,32]# + I: a?b:c =====> P: _?_:_(