From 8ecad006033a7e6647712c2586c401f4dcc4aa33 Mon Sep 17 00:00:00 2001 From: Dmitri Plotnikov Date: Thu, 10 Sep 2026 23:49:35 -0700 Subject: [PATCH] Remove throwaway builders, boxed positions and eager maps from the parse path Four related allocation reductions. They land together because the last two are coupled: the parser's new one-shot position map only pays off once the source builder can adopt an immutable map instead of copying it. CelExpr. Every newBuilder() allocated a fresh CelNotSet plus an ExprKind wrapper purely to be overwritten a moment later; those become shared NOT_SET_KIND and NOT_SET_EXPR singletons. CelSelect paid that once per node and CelComprehension five times. addArgs, addElements and addEntries used Arrays.asList(...) followed by forEach(list::add), allocating a list view and a capturing lambda per call, and now use Collections.addAll and Iterables.addAll. The ofCall, ofList, ofStruct and ofMap factories hand an already-immutable collection straight to the value class rather than copying it through the builder's mutable ArrayList. PrattParser. Builds calls, lists, maps and structs through those factories, which also shortens the call sites. The expression position map becomes an int[] indexed by expression id rather than a Map; ids are dense and handed out sequentially by nextId, so this drops two boxed objects and a hash insert per node. copyPositionsTo then builds the map once, presized. The source content is cached in a field rather than re-fetched through the accessor, and nextSignificantToken loses a loop over WHITESPACE and COMMENT tokens that could never iterate, since Lexer.lex() consumes those internally and never emits them. CelSource.Builder. positions and macroCalls start as empty immutable maps and are copied into a HashMap only when something actually mutates them. Two builders exist per parse, so this removes up to four hash maps. addPositionsMap and addAllMacroCalls adopt an already-immutable argument outright while the builder is still pristine, which makes build()'s copyOf a no-op and removes the second copy of the parser's position map. extensions is created lazily, and addAllExtensions short-circuits on an empty argument, which matters because toBuilder() always calls it. CelValidationResult. Hoists its issue comparator into a constant and replaces a stream().anyMatch(...) with an indexed loop. Every successful parse paid both. Measured with CelParserBenchmark (parseOnly, built -c opt). "Pratt before" is this CL's parent, so the last column is this CL's own contribution. The ANTLR column is the series baseline and is unaffected by any of these changes. Objects allocated per parse: | Case | ANTLR | Pratt before | Pratt after | Pratt vs ANTLR | Delta this CL | | :--- | ---: | ---: | ---: | ---: | ---: | | SMOKE_TEST | 357 | 120 | 70 | 5.1x smaller | -41.7% | | CHAINED_ORS | 968 | 339 | 201 | 4.8x smaller | -40.7% | | LIST_COMPREHENSION | 512 | 160 | 100 | 5.1x smaller | -37.5% | | MESSAGE_CREATION | 1,253 | 406 | 272 | 4.6x smaller | -33.0% | | LONG_LIST | 81,794 | 18,263 | 13,005 | 6.3x smaller | -28.8% | Bytes allocated per parse: | Case | ANTLR | Pratt before | Pratt after | Pratt vs ANTLR | Delta this CL | | :--- | ---: | ---: | ---: | ---: | ---: | | SMOKE_TEST | 12,256 | 3,512 | 2,040 | 6.0x smaller | -41.9% | | CHAINED_ORS | 32,160 | 9,592 | 5,656 | 5.7x smaller | -41.0% | | LIST_COMPREHENSION | 17,320 | 4,736 | 2,928 | 5.9x smaller | -38.2% | | MESSAGE_CREATION | 43,128 | 12,416 | 8,696 | 5.0x smaller | -30.0% | | LONG_LIST | 2,907,488 | 531,888 | 390,992 | 7.4x smaller | -26.5% | 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 | 692 ns | 388 ns | 12.7x faster | -43.9% | | CHAINED_ORS | 14,641 ns | 2,055 ns | 1,111 ns | 13.2x faster | -45.9% | | LIST_COMPREHENSION | 7,514 ns | 1,231 ns | 797 ns | 9.4x faster | -35.3% | | MESSAGE_CREATION | 20,979 ns | 3,583 ns | 2,524 ns | 8.3x faster | -29.6% | | LONG_LIST | 1,616,631 ns | 142,764 ns | 117,823 ns | 13.7x faster | -17.5% | PiperOrigin-RevId: 979625851 --- .../main/java/dev/cel/common/CelSource.java | 66 +- .../dev/cel/common/CelValidationResult.java | 17 +- .../main/java/dev/cel/common/ast/CelExpr.java | 106 ++-- .../common/internal/BasicCodePointArray.java | 11 +- .../common/internal/CelCodePointArray.java | 12 +- .../common/internal/EmptyCodePointArray.java | 13 +- .../common/internal/Latin1CodePointArray.java | 11 +- .../internal/SupplementalCodePointArray.java | 11 +- .../java/dev/cel/common/CelSourceTest.java | 35 ++ .../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 | 581 +++++++++--------- .../dev/cel/parser/CelParserImplTest.java | 38 ++ .../parser/CelParserParameterizedTest.java | 1 + .../java/dev/cel/parser/PrattParserTest.java | 1 + .../resources/parser_core_syntax.baseline | 71 +++ .../pratt_parser_core_syntax.baseline | 71 +++ 18 files changed, 801 insertions(+), 447 deletions(-) diff --git a/common/src/main/java/dev/cel/common/CelSource.java b/common/src/main/java/dev/cel/common/CelSource.java index d64049f61..81687e77f 100644 --- a/common/src/main/java/dev/cel/common/CelSource.java +++ b/common/src/main/java/dev/cel/common/CelSource.java @@ -33,6 +33,7 @@ import java.util.List; import java.util.Map; import java.util.Optional; +import org.jspecify.annotations.Nullable; /** Represents the source content of an expression and related metadata. */ @Immutable @@ -162,9 +163,13 @@ public static final class Builder { private final CelCodePointArray codePoints; private final List lineOffsets; - private final Map positions; - private final Map macroCalls; - private final ImmutableSet.Builder extensions; + // Both maps start out immutable and empty, and are only copied into a mutable map once they are + // actually modified. This keeps the common case (a source that is either never populated, or + // populated in bulk from an already immutable map) allocation free. + private Map positions; + private Map macroCalls; + // Null until the first extension is added; extensions are rare. + private ImmutableSet.@Nullable Builder extensions; private final boolean lineOffsetsAlreadyComputed; private String description; @@ -176,9 +181,8 @@ private Builder() { private Builder(CelCodePointArray codePoints, List lineOffsets) { this.codePoints = checkNotNull(codePoints); this.lineOffsets = checkNotNull(lineOffsets); - this.positions = new HashMap<>(); - this.macroCalls = new HashMap<>(); - this.extensions = ImmutableSet.builder(); + this.positions = ImmutableMap.of(); + this.macroCalls = ImmutableMap.of(); this.description = ""; this.lineOffsetsAlreadyComputed = !lineOffsets.isEmpty(); } @@ -207,39 +211,66 @@ public Builder addAllLineOffsets(Iterable lineOffsets) { return this; } + /** Returns {@code map} as a mutable map, copying it first if it is still immutable. */ + private static Map ensureMutable(Map map) { + return map instanceof HashMap ? map : new HashMap<>(map); + } + + /** + * Returns a map containing every entry of {@code map} plus every entry of {@code additions}. + * + *

If {@code map} is still the empty immutable placeholder a builder starts with, and {@code + * additions} is already immutable, then {@code additions} is adopted as-is and no copy is made. + * That is the common case: a source populated in bulk exactly once, which lets {@link #build()} + * reuse the argument directly. Otherwise the entries are merged into a mutable copy. + */ + private static Map augmentedMap(Map map, Map additions) { + if (map instanceof ImmutableMap && map.isEmpty() && additions instanceof ImmutableMap) { + return additions; + } + Map merged = ensureMutable(map); + merged.putAll(additions); + return merged; + } + @CanIgnoreReturnValue public Builder addPositionsMap(Map positionsMap) { checkNotNull(positionsMap); - this.positions.putAll(positionsMap); + positions = augmentedMap(positions, positionsMap); return this; } @CanIgnoreReturnValue public Builder addPositions(long exprId, int position) { - this.positions.put(exprId, position); + positions = ensureMutable(positions); + positions.put(exprId, position); return this; } @CanIgnoreReturnValue public Builder removePositions(long exprId) { - this.positions.remove(exprId); + if (positions.containsKey(exprId)) { + positions = ensureMutable(positions); + positions.remove(exprId); + } return this; } @CanIgnoreReturnValue public Builder addMacroCalls(long exprId, CelExpr expr) { - this.macroCalls.put(exprId, expr); + macroCalls = ensureMutable(macroCalls); + macroCalls.put(exprId, expr); return this; } @CanIgnoreReturnValue public Builder addAllMacroCalls(Map macroCalls) { - this.macroCalls.putAll(macroCalls); + this.macroCalls = augmentedMap(this.macroCalls, macroCalls); return this; } public ImmutableSet getExtensions() { - return extensions.build(); + return extensions == null ? ImmutableSet.of() : extensions.build(); } /** @@ -249,6 +280,9 @@ public ImmutableSet getExtensions() { @CanIgnoreReturnValue public Builder addAllExtensions(Iterable extensions) { checkNotNull(extensions); + if (this.extensions == null) { + this.extensions = ImmutableSet.builder(); + } this.extensions.addAll(extensions); return this; } @@ -287,13 +321,17 @@ public Optional getOffsetLocation(int offset) { return CelSourceHelper.getOffsetLocation(codePoints, offset); } + /** Returns a live, mutable view of the positions recorded so far. */ @CheckReturnValue public Map getPositionsMap() { - return this.positions; + positions = ensureMutable(positions); + return positions; } + /** Returns a live, mutable view of the macro calls recorded so far. */ @CheckReturnValue public Map getMacroCalls() { + macroCalls = ensureMutable(macroCalls); return macroCalls; } @@ -310,7 +348,7 @@ public CelSource build() { ImmutableList.copyOf(lineOffsets), ImmutableMap.copyOf(positions), ImmutableMap.copyOf(macroCalls), - extensions.build()); + getExtensions()); } } diff --git a/common/src/main/java/dev/cel/common/CelValidationResult.java b/common/src/main/java/dev/cel/common/CelValidationResult.java index 61152c493..f1f218336 100644 --- a/common/src/main/java/dev/cel/common/CelValidationResult.java +++ b/common/src/main/java/dev/cel/common/CelValidationResult.java @@ -22,6 +22,7 @@ import com.google.errorprone.annotations.Immutable; import com.google.errorprone.annotations.InlineMe; import dev.cel.common.annotations.Internal; +import java.util.Comparator; import org.jspecify.annotations.Nullable; /** @@ -31,6 +32,9 @@ @Immutable public final class CelValidationResult { + private static final Comparator BY_SOURCE_LOCATION = + comparing(CelIssue::getSourceLocation); + @SuppressWarnings("Immutable") private final @Nullable Throwable failure; @@ -64,11 +68,20 @@ private CelValidationResult( @Nullable Throwable failure) { this.ast = ast; this.source = source; - this.issues = ImmutableList.sortedCopyOf(comparing(CelIssue::getSourceLocation), issues); - this.hasError = issues.stream().anyMatch(CelValidationResult::issueIsError) || failure != null; + this.issues = ImmutableList.sortedCopyOf(BY_SOURCE_LOCATION, issues); + this.hasError = failure != null || containsError(issues); this.failure = failure; } + private static boolean containsError(ImmutableList issues) { + for (int i = 0; i < issues.size(); i++) { + if (issueIsError(issues.get(i))) { + return true; + } + } + return false; + } + /** * Returns the validated {@code CelAbstractSyntaxTree} if one exists. * diff --git a/common/src/main/java/dev/cel/common/ast/CelExpr.java b/common/src/main/java/dev/cel/common/ast/CelExpr.java index cac968686..0f238b63d 100644 --- a/common/src/main/java/dev/cel/common/ast/CelExpr.java +++ b/common/src/main/java/dev/cel/common/ast/CelExpr.java @@ -20,11 +20,13 @@ import com.google.auto.value.AutoOneOf; import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableList; +import com.google.common.collect.Iterables; import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.errorprone.annotations.CheckReturnValue; import com.google.errorprone.annotations.Immutable; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.Optional; /** @@ -38,6 +40,16 @@ @SuppressWarnings("unchecked") // Class ensures only the super type is used public abstract class CelExpr implements Expression { + /** + * Shared instance of the {@link ExprKind.Kind#NOT_SET} kind. {@link CelNotSet} carries no state, + * so a single instance can back every unset expression. + */ + private static final ExprKind NOT_SET_KIND = + AutoOneOf_CelExpr_ExprKind.notSet(new AutoValue_CelExpr_CelNotSet()); + + /** Shared instance of an expression with an unset kind and a zero id. */ + private static final CelExpr NOT_SET_EXPR = ofNotSet(0L); + @Override public abstract long id(); @@ -340,9 +352,7 @@ public Builder setComprehension(CelComprehension comprehension) { public abstract Builder toBuilder(); public static Builder newBuilder() { - return new AutoValue_CelExpr.Builder() - .setId(0) - .setExprKind(AutoOneOf_CelExpr_ExprKind.notSet(new AutoValue_CelExpr_CelNotSet())); + return new AutoValue_CelExpr.Builder().setId(0).setExprKind(NOT_SET_KIND); } /** Denotes the kind of the expression. An expression can only be of one kind. */ @@ -457,7 +467,7 @@ public abstract static class Builder { public static Builder newBuilder() { return new AutoValue_CelExpr_CelSelect.Builder() .setField("") - .setOperand(CelExpr.newBuilder().build()) + .setOperand(NOT_SET_EXPR) .setTestOnly(false); } } @@ -525,13 +535,14 @@ public Builder clearTarget() { @CanIgnoreReturnValue public Builder addArgs(CelExpr... args) { checkNotNull(args); - return addArgs(Arrays.asList(args)); + Collections.addAll(mutableArgs, args); + return this; } @CanIgnoreReturnValue public Builder addArgs(Iterable args) { checkNotNull(args); - args.forEach(mutableArgs::add); + Iterables.addAll(mutableArgs, args); return this; } @@ -604,13 +615,14 @@ public Builder setElement(int index, CelExpr element) { @CanIgnoreReturnValue public Builder addElements(CelExpr... elements) { checkNotNull(elements); - return addElements(Arrays.asList(elements)); + Collections.addAll(mutableElements, elements); + return this; } @CanIgnoreReturnValue public Builder addElements(Iterable elements) { checkNotNull(elements); - elements.forEach(mutableElements::add); + Iterables.addAll(mutableElements, elements); return this; } @@ -696,13 +708,14 @@ public Builder setEntry(int index, CelStruct.Entry entry) { @CanIgnoreReturnValue public Builder addEntries(CelStruct.Entry... entries) { checkNotNull(entries); - return addEntries(Arrays.asList(entries)); + Collections.addAll(mutableEntries, entries); + return this; } @CanIgnoreReturnValue public Builder addEntries(Iterable entries) { checkNotNull(entries); - entries.forEach(mutableEntries::add); + Iterables.addAll(mutableEntries, entries); return this; } @@ -815,13 +828,14 @@ public Builder setEntry(int index, CelMap.Entry entry) { @CanIgnoreReturnValue public Builder addEntries(CelMap.Entry... entries) { checkNotNull(entries); - return addEntries(Arrays.asList(entries)); + Collections.addAll(mutableEntries, entries); + return this; } @CanIgnoreReturnValue public Builder addEntries(Iterable entries) { checkNotNull(entries); - entries.forEach(mutableEntries::add); + Iterables.addAll(mutableEntries, entries); return this; } @@ -963,20 +977,17 @@ public static Builder newBuilder() { return new AutoValue_CelExpr_CelComprehension.Builder() .setIterVar("") .setIterVar2("") - .setIterRange(CelExpr.newBuilder().build()) + .setIterRange(NOT_SET_EXPR) .setAccuVar("") - .setAccuInit(CelExpr.newBuilder().build()) - .setLoopCondition(CelExpr.newBuilder().build()) - .setLoopStep(CelExpr.newBuilder().build()) - .setResult(CelExpr.newBuilder().build()); + .setAccuInit(NOT_SET_EXPR) + .setLoopCondition(NOT_SET_EXPR) + .setLoopStep(NOT_SET_EXPR) + .setResult(NOT_SET_EXPR); } } public static CelExpr ofNotSet(long id) { - return newBuilder() - .setId(id) - .setExprKind(AutoOneOf_CelExpr_ExprKind.notSet(new AutoValue_CelExpr_CelNotSet())) - .build(); + return newBuilder().setId(id).setExprKind(NOT_SET_KIND).build(); } public static CelExpr ofConstant(long id, CelConstant celConstant) { @@ -1007,46 +1018,45 @@ public static CelExpr ofSelect(long id, CelExpr operandExpr, String field, boole .build(); } + /** Creates a global (non receiver-style) call expression. */ + public static CelExpr ofCall(long id, String function, ImmutableList arguments) { + return ofCall(id, Optional.empty(), function, arguments); + } + public static CelExpr ofCall( long id, Optional targetExpr, String function, ImmutableList arguments) { - - CelCall.Builder celCallBuilder = CelCall.newBuilder().setFunction(function).addArgs(arguments); - targetExpr.ifPresent(celCallBuilder::setTarget); - return newBuilder() - .setId(id) - .setExprKind(AutoOneOf_CelExpr_ExprKind.call(celCallBuilder.build())) - .build(); + // setArgs/autoBuild are used in place of addArgs/build so that the already-immutable argument + // list is handed straight to the value class, skipping a copy through the builder's mutable + // list. This is on the hot path of every parse. + CelCall celCall = + CelCall.newBuilder() + .setFunction(function) + .setTarget(targetExpr) + .setArgs(arguments) + .autoBuild(); + return newBuilder().setId(id).setExprKind(AutoOneOf_CelExpr_ExprKind.call(celCall)).build(); } public static CelExpr ofList( long id, ImmutableList elements, ImmutableList optionalIndices) { - return newBuilder() - .setId(id) - .setExprKind( - AutoOneOf_CelExpr_ExprKind.list( - CelList.newBuilder() - .addElements(elements) - .addOptionalIndices(optionalIndices) - .build())) - .build(); + CelList celList = + CelList.newBuilder() + .setElements(elements) + .addOptionalIndices(optionalIndices) + .autoBuild(); + return newBuilder().setId(id).setExprKind(AutoOneOf_CelExpr_ExprKind.list(celList)).build(); } public static CelExpr ofStruct( long id, String messageName, ImmutableList entries) { - return newBuilder() - .setId(id) - .setExprKind( - AutoOneOf_CelExpr_ExprKind.struct( - CelStruct.newBuilder().setMessageName(messageName).addEntries(entries).build())) - .build(); + CelStruct celStruct = + CelStruct.newBuilder().setMessageName(messageName).setEntries(entries).autoBuild(); + return newBuilder().setId(id).setExprKind(AutoOneOf_CelExpr_ExprKind.struct(celStruct)).build(); } public static CelExpr ofMap(long id, ImmutableList entries) { - return newBuilder() - .setId(id) - .setExprKind( - AutoOneOf_CelExpr_ExprKind.map(CelMap.newBuilder().addEntries(entries).build())) - .build(); + CelMap celMap = CelMap.newBuilder().setEntries(entries).autoBuild(); + return newBuilder().setId(id).setExprKind(AutoOneOf_CelExpr_ExprKind.map(celMap)).build(); } public static CelStruct.Entry ofStructEntry( 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/CelSourceTest.java b/common/src/test/java/dev/cel/common/CelSourceTest.java index d8b3701e3..24eded8fd 100644 --- a/common/src/test/java/dev/cel/common/CelSourceTest.java +++ b/common/src/test/java/dev/cel/common/CelSourceTest.java @@ -18,10 +18,12 @@ import static org.antlr.v4.runtime.IntStream.UNKNOWN_SOURCE_NAME; import static org.junit.Assert.assertThrows; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; import dev.cel.common.CelSource.Extension; import dev.cel.common.CelSource.Extension.Component; import dev.cel.common.CelSource.Extension.Version; +import dev.cel.common.ast.CelExpr; import dev.cel.common.internal.BasicCodePointArray; import dev.cel.common.internal.CodePointStream; import dev.cel.common.internal.Latin1CodePointArray; @@ -192,4 +194,37 @@ public void source_lineOffsetsAlreadyComputed_throws() { .hasMessageThat() .contains("Line offsets were already been computed through the provided code points."); } + + @Test + public void builder_getPositionsMap_isMutable() { + CelSource.Builder builder = CelSource.newBuilder(); + builder.getPositionsMap().put(1L, 10); + assertThat(builder.build().getPositionsMap()).containsExactly(1L, 10); + } + + @Test + public void builder_getMacroCalls_isMutable() { + CelSource.Builder builder = CelSource.newBuilder(); + CelExpr macroCall = CelExpr.ofIdent(1, "foo"); + builder.getMacroCalls().put(1L, macroCall); + assertThat(builder.build().getMacroCalls()).containsExactly(1L, macroCall); + } + + @Test + public void builder_addPositionsMap_mergesWithExisting() { + CelSource.Builder builder = CelSource.newBuilder(); + builder.addPositions(1L, 10); + builder.addPositionsMap(ImmutableMap.of(2L, 20)); + assertThat(builder.build().getPositionsMap()).containsExactly(1L, 10, 2L, 20); + } + + @Test + public void builder_addAllMacroCalls_mergesWithExisting() { + CelSource.Builder builder = CelSource.newBuilder(); + CelExpr macro1 = CelExpr.ofIdent(1, "foo"); + CelExpr macro2 = CelExpr.ofIdent(2, "bar"); + builder.addMacroCalls(1L, macro1); + builder.addAllMacroCalls(ImmutableMap.of(2L, macro2)); + assertThat(builder.build().getMacroCalls()).containsExactly(1L, macro1, 2L, macro2); + } } 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..2e148c9a8 100644 --- a/parser/src/main/java/dev/cel/parser/PrattParser.java +++ b/parser/src/main/java/dev/cel/parser/PrattParser.java @@ -26,10 +26,12 @@ import dev.cel.common.Operator; import dev.cel.common.ast.CelConstant; import dev.cel.common.ast.CelExpr; +import dev.cel.common.internal.CelCodePointArray; import dev.cel.common.internal.Constants; 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; @@ -39,8 +41,16 @@ /** Pratt parser implementation for CEL. */ final class PrattParser { + /** Sentinel stored in {@link #positions} for expression ids that have no source position. */ + private static final int NO_POSITION = -1; + 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, NO_POSITION, NO_POSITION); + + /** 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 +66,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 { @@ -134,14 +121,21 @@ private static final class UnaryOp { } private final CelSource source; + private final CelCodePointArray content; private final CelOptions options; private final ImmutableMap macros; private final Lexer lexer; - private final Map positions; - private final Map macroCalls; - private final List issues; - private final PrattMacroExprFactory macroExprFactory; + /** + * Code point offset of each expression node, indexed by expression id, with {@link #NO_POSITION} + * for nodes that have none. Ids are dense and handed out sequentially by {@link #nextId}, so an + * array avoids the boxing and hashing a {@code Map} would cost on every node. + */ + private int[] positions; + + private Map macroCalls = ImmutableMap.of(); + private PrattMacroExprFactory macroExprFactory; + private final List issues; private Lexer.Token currentToken; private Lexer.Token peekToken; private int recursionDepth; @@ -170,7 +164,7 @@ static CelValidationResult parse( } CelSource.Builder sourceBuilder = source.toBuilder(); - sourceBuilder.addPositionsMap(prattParser.positions); + prattParser.copyPositionsTo(sourceBuilder); sourceBuilder.addAllMacroCalls(prattParser.macroCalls); return new CelValidationResult( @@ -180,15 +174,14 @@ static CelValidationResult parse( private PrattParser(CelSource source, CelOptions options, Map macros) { this.source = source; + this.content = source.getContent(); this.options = options; this.macros = ImmutableMap.copyOf(macros); - this.lexer = new Lexer(source.getContent()); - this.positions = new HashMap<>(); - this.macroCalls = new HashMap<>(); + this.lexer = new Lexer(content); + this.positions = new int[Math.max(16, Math.min(content.size() + 1, 1024))]; this.issues = new ArrayList<>(); - this.macroExprFactory = new PrattMacroExprFactory(); this.nextId = 1; - initTokenStream(); + peekToken = nextSignificantToken(true); } CelExpr run() { @@ -215,40 +208,32 @@ private boolean isRecoveryLimitExceeded() { return errorCount > options.maxParseErrorRecoveryLimit(); } - private void initTokenStream() { - peekToken = nextSignificantToken(true); - } - private String getTokenText(Lexer.Token tok) { - if (tok.start >= 0 && tok.end >= tok.start && tok.end <= source.getContent().size()) { - return source.getContent().slice(tok.start, tok.end).toString(); + if (tok.text != null) { + return tok.text; + } + if (tok.start >= 0 && tok.end >= tok.start && tok.end <= content.size()) { + return content.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; - } - if (tok.type == Lexer.TokenType.ERROR && reportError) { - reportSyntaxError(tok, lexer.getError().message); - if (isRecoveryLimitExceeded()) { - return new Lexer.Token(Lexer.TokenType.END, 0, 0); - } + // The lexer skips whitespace and comments itself, so every token it returns is significant. + Lexer.Token tok = lexer.lex(); + if (tok.type == Lexer.TokenType.ERROR && reportError) { + reportSyntaxError(tok, lexer.getError().message); + if (isRecoveryLimitExceeded()) { + return END_TOKEN; } - 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) { @@ -281,11 +266,8 @@ private boolean expect(Lexer.TokenType type, String msg) { return false; } + // Find the next delimiter to prevent a cascade of spurious secondary errors. private void synchronizeOnDelimiter() { - if (isRecoveryLimitExceeded()) { - peekToken = new Lexer.Token(Lexer.TokenType.END, 0, 0); - return; - } while (peekToken.type != Lexer.TokenType.END) { if (peekToken.type == Lexer.TokenType.COMMA || peekToken.type == Lexer.TokenType.RIGHT_PAREN @@ -307,7 +289,7 @@ private long nextId(int position) { nodeLimitExceeded = true; } if (!nodeLimitExceeded && position >= 0) { - positions.put(id, position); + setPosition(id, position); } return id; } @@ -317,25 +299,60 @@ private long nextId(Lexer.Token token) { } private long nextId() { - return nextId(-1); + return nextId(NO_POSITION); } private void setPosition(long id, Lexer.Token token) { if (token.start >= 0) { - positions.put(id, token.start); + setPosition(id, token.start); + } + } + + private void setPosition(long id, int position) { + int index = (int) id; + if (index >= positions.length) { + int oldLength = positions.length; + positions = Arrays.copyOf(positions, Math.max(index + 1, oldLength * 2)); + Arrays.fill(positions, oldLength, positions.length, NO_POSITION); } + positions[index] = position; + } + + /** Returns the recorded position of {@code id}, or {@link #NO_POSITION} if it has none. */ + private int getPosition(long id) { + int index = (int) id; + return index >= 0 && index < positions.length ? positions[index] : NO_POSITION; + } + + /** Returns the recorded position of {@code id}, or {@code 0} if it has none. */ + private int getPositionOrZero(long id) { + return Math.max(getPosition(id), 0); + } + + private void copyPositionsTo(CelSource.Builder sourceBuilder) { + ImmutableMap.Builder positionsMap = + ImmutableMap.builderWithExpectedSize((int) nextId); + for (long id = 1; id < nextId; id++) { + int position = getPosition(id); + if (position != NO_POSITION) { + positionsMap.put(id, position); + } + } + sourceBuilder.addPositionsMap(positionsMap.buildOrThrow()); } private long copyId(long id) { if (id == 0) { return 0; } - int pos = positions.getOrDefault(id, 0); - return nextId(pos); + return nextId(getPositionOrZero(id)); } private void eraseId(long id) { - positions.remove(id); + int index = (int) id; + if (index >= 0 && index < positions.length) { + positions[index] = NO_POSITION; + } if (nextId == id + 1) { --nextId; } @@ -356,7 +373,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 +386,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 +427,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 +438,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++; @@ -437,63 +459,64 @@ private CelExpr parseTernary(CelExpr lhs) { return lhs; } CelExpr falseExpr = parseExpr(); - return CelExpr.newBuilder() - .setId(opId) - .setCall( - CelExpr.CelCall.newBuilder() - .setFunction(Operator.CONDITIONAL.getFunction()) - .addArgs(lhs) - .addArgs(trueExpr) - .addArgs(falseExpr) - .build()) - .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(); + return CelExpr.ofCall( + opId, Operator.CONDITIONAL.getFunction(), ImmutableList.of(lhs, trueExpr, falseExpr)); } 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); - } - return CelExpr.newBuilder() - .setId(ops.get(mid)) - .setCall(CelExpr.CelCall.newBuilder().setFunction(op).addArgs(left).addArgs(right).build()) - .build(); + 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.ofCall(id, function, ImmutableList.of(lhs, rhs)); + } + + private static CelExpr buildUnaryCall(long id, String function, CelExpr operand) { + return CelExpr.ofCall(id, function, ImmutableList.of(operand)); } 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) { @@ -536,48 +559,20 @@ private CelExpr parseSelectorChainTail(CelExpr initialLhs) { String idText = normalizeIdent(idTok, /* allowQuoted= */ !isMemberCall); if (optional) { long opId = nextId(dotTok); - CelExpr arg1 = lhs; - CelExpr arg2 = - CelExpr.newBuilder() - .setId(nextId(getLeftmostPosition(lhs))) - .setConstant(CelConstant.ofValue(idText)) - .build(); - lhs = - CelExpr.newBuilder() - .setId(opId) - .setCall( - CelExpr.CelCall.newBuilder() - .setFunction(Operator.OPTIONAL_SELECT.getFunction()) - .addArgs(arg1) - .addArgs(arg2) - .build()) - .build(); + CelExpr field = + CelExpr.ofConstant(nextId(getLeftmostPosition(lhs)), CelConstant.ofValue(idText)); + lhs = buildBinaryCall(opId, Operator.OPTIONAL_SELECT.getFunction(), lhs, field); } else if (peekToken.type == Lexer.TokenType.LEFT_PAREN) { Lexer.Token lparen = nextToken(); long callId = nextId(lparen); ImmutableList args = parseArguments(Lexer.TokenType.RIGHT_PAREN); Optional expanded = tryExpandMacro(callId, idText, lhs, args); - if (expanded.isPresent()) { - lhs = expanded.get(); - } else { - lhs = - CelExpr.newBuilder() - .setId(callId) - .setCall( - CelExpr.CelCall.newBuilder() - .setFunction(idText) - .setTarget(lhs) - .addArgs(args) - .build()) - .build(); - } - } else { lhs = - CelExpr.newBuilder() - .setId(nextId(dotTok)) - .setSelect( - CelExpr.CelSelect.newBuilder().setOperand(lhs).setField(idText).build()) - .build(); + expanded.isPresent() + ? expanded.get() + : CelExpr.ofCall(callId, Optional.of(lhs), idText, args); + } else { + lhs = CelExpr.ofSelect(nextId(dotTok), lhs, idText, /* isTestOnly= */ false); } } else if (tok == Lexer.TokenType.LEFT_BRACKET) { if (checkRecursion(chainDepth, peekToken)) { @@ -598,18 +593,9 @@ private CelExpr parseSelectorChainTail(CelExpr initialLhs) { expect(Lexer.TokenType.RIGHT_BRACKET, "expected ']'"); String opName = optional ? Operator.OPTIONAL_INDEX.getFunction() : Operator.INDEX.getFunction(); - lhs = - CelExpr.newBuilder() - .setId(opId) - .setCall( - CelExpr.CelCall.newBuilder() - .setFunction(opName) - .addArgs(lhs) - .addArgs(index) - .build()) - .build(); + lhs = buildBinaryCall(opId, opName, lhs, index); } else if (tok == Lexer.TokenType.LEFT_BRACE) { - String structName = extractStructName(lhs).orElse(null); + String structName = extractStructName(lhs); if (structName == null) { break; } @@ -622,14 +608,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; @@ -662,10 +640,7 @@ private CelExpr parseUnaryOps() { (opType == Lexer.TokenType.EXCLAMATION) ? Operator.LOGICAL_NOT.getFunction() : Operator.NEGATE.getFunction(); - return CelExpr.newBuilder() - .setId(opId) - .setCall(CelExpr.CelCall.newBuilder().setFunction(opName).addArgs(operand).build()) - .build(); + return buildUnaryCall(opId, opName, operand); } private CelExpr parseUnaryOpsChain(Lexer.Token firstOp) { @@ -739,11 +714,7 @@ private CelExpr parseUnaryOpsChain(Lexer.Token firstOp) { (ops.get(i).token.type == Lexer.TokenType.EXCLAMATION) ? Operator.LOGICAL_NOT.getFunction() : Operator.NEGATE.getFunction(); - operand = - CelExpr.newBuilder() - .setId(ops.get(i).id) - .setCall(CelExpr.CelCall.newBuilder().setFunction(opName).addArgs(operand).build()) - .build(); + operand = buildUnaryCall(ops.get(i).id, opName, operand); } return operand; @@ -777,16 +748,10 @@ private CelExpr parseIdentOrCall() { if (expanded.isPresent()) { return expanded.get(); } - return CelExpr.newBuilder() - .setId(callId) - .setCall(CelExpr.CelCall.newBuilder().setFunction(name).addArgs(args).build()) - .build(); + return CelExpr.ofCall(callId, name, args); } 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 +772,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); @@ -853,7 +816,8 @@ private CelExpr parsePrimary() { private CelExpr parseList() { Lexer.Token openTok = nextToken(); long listId = nextId(openTok); - CelExpr.CelList.Builder listBuilder = CelExpr.CelList.newBuilder(); + ImmutableList.Builder elements = ImmutableList.builder(); + ImmutableList.Builder optionalIndices = ImmutableList.builder(); int elemIndex = 0; while (peekToken.type != Lexer.TokenType.RIGHT_BRACKET && peekToken.type != Lexer.TokenType.END) { @@ -865,9 +829,9 @@ private CelExpr parseList() { reportError(q.start, "unsupported syntax '?'"); } } - listBuilder.addElements(parseExpr()); + elements.add(parseExpr()); if (optional) { - listBuilder.addOptionalIndices(elemIndex); + optionalIndices.add(elemIndex); } elemIndex++; if (peekToken.type == Lexer.TokenType.COMMA) { @@ -877,13 +841,13 @@ private CelExpr parseList() { } } expect(Lexer.TokenType.RIGHT_BRACKET, "expected ']'"); - return CelExpr.newBuilder().setId(listId).setList(listBuilder.build()).build(); + return CelExpr.ofList(listId, elements.build(), optionalIndices.build()); } private CelExpr parseMap() { Lexer.Token openTok = nextToken(); long mapId = nextId(openTok); - CelExpr.CelMap.Builder mapBuilder = CelExpr.CelMap.newBuilder(); + ImmutableList.Builder entries = ImmutableList.builder(); while (peekToken.type != Lexer.TokenType.RIGHT_BRACE && peekToken.type != Lexer.TokenType.END) { boolean optional = false; Lexer.Token keyStart = peekToken; @@ -903,13 +867,7 @@ private CelExpr parseMap() { } setPosition(entryId, colon); CelExpr value = parseExpr(); - mapBuilder.addEntries( - CelExpr.CelMap.Entry.newBuilder() - .setId(entryId) - .setKey(key) - .setValue(value) - .setOptionalEntry(optional) - .build()); + entries.add(CelExpr.ofMapEntry(entryId, key, value, optional)); if (peekToken.type == Lexer.TokenType.COMMA) { nextToken(); } else { @@ -917,13 +875,12 @@ private CelExpr parseMap() { } } expect(Lexer.TokenType.RIGHT_BRACE, "expected '}'"); - return CelExpr.newBuilder().setId(mapId).setMap(mapBuilder.build()).build(); + return CelExpr.ofMap(mapId, entries.build()); } private CelExpr parseStruct(long objId, String structName) { nextToken(); - CelExpr.CelStruct.Builder structBuilder = - CelExpr.CelStruct.newBuilder().setMessageName(structName); + ImmutableList.Builder entries = ImmutableList.builder(); while (peekToken.type != Lexer.TokenType.RIGHT_BRACE && peekToken.type != Lexer.TokenType.END) { boolean optional = false; if (peekToken.type == Lexer.TokenType.QUESTION) { @@ -947,13 +904,7 @@ private CelExpr parseStruct(long objId, String structName) { } long fieldId = nextId(colon); CelExpr value = parseExpr(); - structBuilder.addEntries( - CelExpr.CelStruct.Entry.newBuilder() - .setId(fieldId) - .setFieldKey(fieldName) - .setValue(value) - .setOptionalEntry(optional) - .build()); + entries.add(CelExpr.ofStructEntry(fieldId, fieldName, value, optional)); if (peekToken.type == Lexer.TokenType.COMMA) { nextToken(); } else { @@ -961,7 +912,7 @@ private CelExpr parseStruct(long objId, String structName) { } } expect(Lexer.TokenType.RIGHT_BRACE, "expected '}'"); - return CelExpr.newBuilder().setId(objId).setStruct(structBuilder.build()).build(); + return CelExpr.ofStruct(objId, structName, entries.build()); } private ImmutableList parseArguments(Lexer.TokenType closeToken) { @@ -990,7 +941,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 +953,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 +966,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 +978,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 +990,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,70 +1035,69 @@ 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); + return getPositionOrZero(expr.id()); } - 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) { reportError( - positions.getOrDefault(exprId, 0), - "could not expand macro: expression node limit exceeded"); + getPositionOrZero(exprId), "could not expand macro: expression node limit exceeded"); 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); + int macroPosition = getPositionOrZero(exprId); + CelExpr targetExpr = (target != null ? target : CelExpr.ofNotSet(0)); + Optional expandedExpr = expandMacro(macroPosition, macro, targetExpr, args); if (expandedExpr.isPresent()) { if (options.populateMacroCalls()) { @@ -1159,8 +1109,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 +1133,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 +1170,25 @@ private int countGroupingParentheses() { return 0; } + // Fast path: if the next non-whitespace character is not '(', leading open parens is 1. + int pos = peekToken.end; + int size = content.size(); + while (pos < size) { + int c = content.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; @@ -1285,8 +1269,7 @@ public String getAccumulatorVarName() { @Override protected CelSourceLocation getSourceLocation(long exprId) { - int pos = positions.getOrDefault(exprId, -1); - return source.getOffsetLocation(pos).orElse(CelSourceLocation.NONE); + return source.getOffsetLocation(getPosition(exprId)).orElse(CelSourceLocation.NONE); } @Override diff --git a/parser/src/test/java/dev/cel/parser/CelParserImplTest.java b/parser/src/test/java/dev/cel/parser/CelParserImplTest.java index 37501ec29..d6173f538 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,40 @@ 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(); + } + } + + @Test + public void parse_lexerErrorExceedsRecoveryLimit_stopsParsing() { + if (!enablePrattParser) { + return; + } + CelParser parser = + newParserBuilder() + .setOptions( + CelOptions.newBuilder() + .enablePrattParser(enablePrattParser) + .maxParseErrorRecoveryLimit(2) + .build()) + .build(); + CelValidationResult result = parser.parse("[ @, @, @ ]"); + assertThat(result.hasError()).isTrue(); + assertThat(result.getErrors()).hasSize(3); + } + + @Test + public void parse_largeExpression_expandsPositionsArray() { + CelParser parser = newParserBuilder().build(); + String expr = "[" + Joiner.on(", ").join(Collections.nCopies(1025, "1")) + "]"; + 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: _?_:_(