From cf1292b5d7c04b6cfa11095de689f51479f267d6 Mon Sep 17 00:00:00 2001 From: Dmitri Plotnikov Date: Mon, 14 Sep 2026 16:57:22 -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: 981440440 --- .../main/java/dev/cel/common/CelSource.java | 72 ++++-- .../dev/cel/common/CelValidationResult.java | 17 +- .../main/java/dev/cel/common/ast/CelExpr.java | 106 +++++---- .../java/dev/cel/common/CelSourceTest.java | 35 +++ .../src/main/java/dev/cel/parser/BUILD.bazel | 1 - .../main/java/dev/cel/parser/PrattParser.java | 218 +++++++++--------- .../dev/cel/parser/CelParserImplTest.java | 83 ++++++- 7 files changed, 355 insertions(+), 177 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..4ea2b7a1e 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,72 @@ public Builder addAllLineOffsets(Iterable lineOffsets) { return this; } + /** + * 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 = map instanceof HashMap ? map : new HashMap<>(map); + merged.putAll(additions); + return merged; + } + + private Map mutablePositions() { + if (!(positions instanceof HashMap)) { + positions = new HashMap<>(positions); + } + return positions; + } + @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); + mutablePositions().put(exprId, position); return this; } @CanIgnoreReturnValue public Builder removePositions(long exprId) { - this.positions.remove(exprId); + if (positions.containsKey(exprId)) { + mutablePositions().remove(exprId); + } return this; } + private Map mutableMacroCalls() { + if (!(macroCalls instanceof HashMap)) { + macroCalls = new HashMap<>(macroCalls); + } + return macroCalls; + } + @CanIgnoreReturnValue public Builder addMacroCalls(long exprId, CelExpr expr) { - this.macroCalls.put(exprId, expr); + mutableMacroCalls().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 +286,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,14 +327,16 @@ 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; + return mutablePositions(); } + /** Returns a live, mutable view of the macro calls recorded so far. */ @CheckReturnValue public Map getMacroCalls() { - return macroCalls; + return mutableMacroCalls(); } @CheckReturnValue @@ -310,7 +352,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/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/parser/src/main/java/dev/cel/parser/BUILD.bazel b/parser/src/main/java/dev/cel/parser/BUILD.bazel index 55183446f..848209380 100644 --- a/parser/src/main/java/dev/cel/parser/BUILD.bazel +++ b/parser/src/main/java/dev/cel/parser/BUILD.bazel @@ -118,7 +118,6 @@ 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/PrattParser.java b/parser/src/main/java/dev/cel/parser/PrattParser.java index dceaadad6..1fe2a4bab 100644 --- a/parser/src/main/java/dev/cel/parser/PrattParser.java +++ b/parser/src/main/java/dev/cel/parser/PrattParser.java @@ -26,6 +26,7 @@ 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; @@ -40,9 +41,14 @@ /** 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, 0, 0); + 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; @@ -115,10 +121,18 @@ 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; + + /** + * 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; @@ -150,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( @@ -160,10 +174,12 @@ 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.lexer = new Lexer(content); + this.positions = new int[Math.max(16, Math.min(content.size() + 1, 1024))]; + Arrays.fill(this.positions, NO_POSITION); this.issues = new ArrayList<>(); this.nextId = 1; peekToken = nextSignificantToken(true); @@ -197,13 +213,14 @@ 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().substring(tok.start, tok.end); + 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) { + // 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); @@ -250,6 +267,7 @@ 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 = END_TOKEN; @@ -276,7 +294,7 @@ private long nextId(int position) { nodeLimitExceeded = true; } if (!nodeLimitExceeded && position >= 0) { - positions.put(id, position); + setPosition(id, position); } return id; } @@ -286,32 +304,65 @@ 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; + } + + 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(getPosition(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; } } private void reportError(int position, String msg) { - CelSourceLocation loc = source.getOffsetLocation(position).orElse(CelSourceLocation.NONE); + CelSourceLocation loc = + position >= 0 + ? source.getOffsetLocation(position).orElse(CelSourceLocation.NONE) + : CelSourceLocation.NONE; reportError(loc, msg); } @@ -411,16 +462,8 @@ 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(); + return CelExpr.ofCall( + opId, Operator.CONDITIONAL.getFunction(), ImmutableList.of(lhs, trueExpr, falseExpr)); } private CelExpr parseBalancedLogicalChain(CelExpr lhs, BinaryOpInfo opInfo) { @@ -462,11 +505,11 @@ private CelExpr balancedTree(String op, CelExpr[] terms, long[] ops, int lo, int } private static CelExpr buildBinaryCall(long id, String function, CelExpr lhs, CelExpr rhs) { - return CelExpr.newBuilder() - .setId(id) - .setCall( - CelExpr.CelCall.newBuilder().setFunction(function).addArgs(lhs).addArgs(rhs).build()) - .build(); + 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() { @@ -519,38 +562,18 @@ private CelExpr parseSelectorChainTail(CelExpr initialLhs) { String idText = normalizeIdent(idTok, /* allowQuoted= */ !isMemberCall); if (optional) { long opId = nextId(dotTok); - CelExpr arg1 = lhs; - CelExpr arg2 = + CelExpr field = CelExpr.ofConstant(nextId(getLeftmostPosition(lhs)), CelConstant.ofValue(idText)); - lhs = - CelExpr.newBuilder() - .setId(opId) - .setCall( - CelExpr.CelCall.newBuilder() - .setFunction(Operator.OPTIONAL_SELECT.getFunction()) - .addArgs(arg1) - .addArgs(arg2) - .build()) - .build(); + 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(); - } + lhs = + expanded.isPresent() + ? expanded.get() + : CelExpr.ofCall(callId, Optional.of(lhs), idText, args); } else { lhs = CelExpr.ofSelect(nextId(dotTok), lhs, idText, /* isTestOnly= */ false); } @@ -573,16 +596,7 @@ 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); if (structName == null) { @@ -629,10 +643,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) { @@ -706,11 +717,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; @@ -744,10 +751,7 @@ 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.ofIdent(id, name); @@ -815,7 +819,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) { @@ -827,9 +832,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) { @@ -839,13 +844,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; @@ -865,13 +870,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 { @@ -879,13 +878,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) { @@ -909,13 +907,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 { @@ -923,7 +915,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) { @@ -1068,7 +1060,7 @@ private int getLeftmostPosition(CelExpr expr) { while (expr.exprKind().getKind() == CelExpr.ExprKind.Kind.SELECT) { expr = expr.select().operand(); } - return positions.getOrDefault(expr.id(), 0); + return getPosition(expr.id()); } private @Nullable CelMacro lookupMacro(String id, int argCount, boolean receiverStyle) { @@ -1097,8 +1089,7 @@ private Optional tryExpandMacro( } if (nodeLimitExceeded) { reportError( - positions.getOrDefault(exprId, 0), - "could not expand macro: expression node limit exceeded"); + getPosition(exprId), "could not expand macro: expression node limit exceeded"); return Optional.empty(); } @@ -1107,8 +1098,8 @@ private Optional tryExpandMacro( return Optional.of(ERROR); } - int macroPosition = positions.getOrDefault(exprId, 0); - CelExpr targetExpr = (target != null ? target : CelExpr.newBuilder().build()); + int macroPosition = getPosition(exprId); + CelExpr targetExpr = (target != null ? target : CelExpr.ofNotSet(0)); Optional expandedExpr = expandMacro(macroPosition, macro, targetExpr, args); if (expandedExpr.isPresent()) { @@ -1184,8 +1175,9 @@ private int countGroupingParentheses() { // 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); + 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 '('. @@ -1280,7 +1272,10 @@ public String getAccumulatorVarName() { @Override protected CelSourceLocation getSourceLocation(long exprId) { - int pos = positions.getOrDefault(exprId, -1); + int pos = getPosition(exprId); + if (pos < 0) { + return CelSourceLocation.NONE; + } return source.getOffsetLocation(pos).orElse(CelSourceLocation.NONE); } @@ -1289,7 +1284,10 @@ protected CelSourceLocation currentSourceLocationForMacro() { int pos = !macroPositions.isEmpty() ? peekPosition() - : (currentToken != null ? currentToken.start : 0); + : (currentToken != null ? currentToken.start : NO_POSITION); + if (pos < 0) { + return CelSourceLocation.NONE; + } return source.getOffsetLocation(pos).orElse(CelSourceLocation.NONE); } diff --git a/parser/src/test/java/dev/cel/parser/CelParserImplTest.java b/parser/src/test/java/dev/cel/parser/CelParserImplTest.java index 5b7f9defb..09d578a36 100644 --- a/parser/src/test/java/dev/cel/parser/CelParserImplTest.java +++ b/parser/src/test/java/dev/cel/parser/CelParserImplTest.java @@ -18,6 +18,7 @@ import static org.junit.Assert.assertThrows; import com.google.common.base.Joiner; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.testing.junit.testparameterinjector.TestParameter; import com.google.testing.junit.testparameterinjector.TestParameterInjector; @@ -27,9 +28,11 @@ import dev.cel.common.CelSource; import dev.cel.common.CelValidationException; import dev.cel.common.CelValidationResult; +import dev.cel.common.ast.CelConstant; import dev.cel.common.ast.CelExpr; import java.util.Collections; import java.util.Optional; +import org.junit.Assume; import org.junit.Test; import org.junit.runner.RunWith; @@ -51,6 +54,7 @@ public void build_withMacros_containsAllMacros() { CelParserImpl parser = (CelParserImpl) newParserBuilder().setStandardMacros(CelStandardMacro.STANDARD_MACROS).build(); + assertThat(parser.findMacro("has:1:false")).hasValue(CelStandardMacro.HAS.getDefinition()); assertThat(parser.findMacro("all:2:true")).hasValue(CelStandardMacro.ALL.getDefinition()); assertThat(parser.findMacro("exists:2:true")).hasValue(CelStandardMacro.EXISTS.getDefinition()); @@ -67,6 +71,7 @@ public void build_withStandardMacros_containsAllMacros() { CelParserImpl parser = (CelParserImpl) newParserBuilder().setStandardMacros(CelStandardMacro.STANDARD_MACROS).build(); + assertThat(parser.findMacro("has:1:false")).hasValue(CelStandardMacro.HAS.getDefinition()); assertThat(parser.findMacro("all:2:true")).hasValue(CelStandardMacro.ALL.getDefinition()); assertThat(parser.findMacro("exists:2:true")).hasValue(CelStandardMacro.EXISTS.getDefinition()); @@ -106,6 +111,7 @@ public void build_withStandardMacrosAndCustomMacros_containsAllMacros() { public void build_withMacro_containsMacro() { CelParserImpl parser = (CelParserImpl) newParserBuilder().setStandardMacros(CelStandardMacro.HAS).build(); + assertThat(parser.findMacro("has:1:false")).hasValue(CelStandardMacro.HAS.getDefinition()); } @@ -113,6 +119,7 @@ public void build_withMacro_containsMacro() { public void build_withStandardMacro_containsMacro() { CelParserImpl parser = (CelParserImpl) newParserBuilder().setStandardMacros(CelStandardMacro.HAS).build(); + assertThat(parser.findMacro("has:1:false")).hasValue(CelStandardMacro.HAS.getDefinition()); } @@ -146,6 +153,7 @@ public void build_standardMacroKeyConflictsWithCustomMacro_throws() { @Test public void build_containsNoMacros() { CelParserImpl parser = (CelParserImpl) newParserBuilder().build(); + assertThat(parser.findMacro("has:1:false")).isEmpty(); } @@ -180,7 +188,9 @@ public void parse_throwsWhenExpressionSizeCodePointLimitExceeded() { .maxExpressionCodePointSize(2) .build()) .build(); + CelValidationResult parseResult = parser.parse(CelSource.newBuilder("foo").build()); + CelValidationException exception = assertThrows(CelValidationException.class, parseResult::getAst); assertThat(exception.getErrors()).hasSize(1); @@ -261,7 +271,6 @@ public void parse_largeExprHitsMaxRecursionLimit_throws( public void parse_exprUnderMaxRecursionLimit_doesNotThrow( @TestParameter MaxParseRecursionDepthTestCase testCase) throws CelValidationException { int maxParseRecursionLimit = MaxParseRecursionDepthTestCase.MAX_RECURSION_LIMIT + 1; - CelParser parser = newParserBuilder() .setOptions( @@ -270,7 +279,9 @@ public void parse_exprUnderMaxRecursionLimit_doesNotThrow( .maxParseRecursionDepth(maxParseRecursionLimit) .build()) .build(); + CelValidationResult parseResult = parser.parse(CelSource.newBuilder(testCase.source).build()); + assertThat(parseResult.hasError()).isFalse(); assertThat(parseResult.getAst()).isNotNull(); } @@ -285,6 +296,7 @@ public void parse_nodeLimitExceeded_throws() { .maxParseExpressionNodeCount(2) .build()) .build(); + CelValidationResult parseResult = parser.parse("a + b + c"); CelValidationException exception = @@ -304,6 +316,7 @@ public void parse_macroExpansionNodeLimitExceeded_throws() { .maxParseExpressionNodeCount(5) .build()) .build(); + CelValidationResult parseResult = parser.parse("[1, 2, 3, 4, 5].map(x, x * 2)"); CelValidationException exception = @@ -330,7 +343,9 @@ public void parse_macroExpansionNodeLimitNotExceeded_success() throws CelValidat .maxParseExpressionNodeCount(100) .build()) .build(); + CelValidationResult parseResult = parser.parse("[1, 2, 3, 4, 5].map(x, x * 2)"); + assertThat(parseResult.hasError()).isFalse(); assertThat(parseResult.getAst()).isNotNull(); } @@ -402,10 +417,76 @@ public void toParserBuilder_collectionProperties_copied() { @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() { + Assume.assumeTrue(enablePrattParser); + 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() throws Exception { + CelParser parser = newParserBuilder().build(); + String expr = "[" + Joiner.on(", ").join(Collections.nCopies(1025, "1")) + "]"; + ImmutableMap.Builder expectedPositions = + ImmutableMap.builderWithExpectedSize(1026); + expectedPositions.put(1L, 0); + for (int i = 0; i < 1025; i++) { + expectedPositions.put((long) (i + 2), 1 + 3 * i); + } + + CelValidationResult result = parser.parse(expr); + + assertThat(result.hasError()).isFalse(); + assertThat(result.getAst().getSource().getPositionsMap()) + .containsExactlyEntriesIn(expectedPositions.buildOrThrow()); + } + + @Test + public void parse_macroCopiesNodeWithoutPosition_noSourcePositionRecorded() throws Exception { + CelMacro macro = + CelMacro.newGlobalMacro( + "copy_macro", + 0, + (exprFactory, target, args) -> { + CelExpr nodeWithoutPosition = + CelExpr.newBuilder() + .setId(5L) + .setConstant(CelConstant.ofValue(10L)) + .build(); + return Optional.of(exprFactory.copy(nodeWithoutPosition)); + }); + CelParser parser = newParserBuilder().addMacros(macro).build(); + + CelValidationResult result = parser.parse("copy_macro()"); + + assertThat(result.hasError()).isFalse(); + // The contract for nodes without a source position is -1 (NO_POSITION). Unpositioned + // nodes must not be assigned position 0 (which is a valid source offset) and thus should + // not have an entry recorded in the positions map. + assertThat(result.getAst().getSource().getPositionsMap()).isEmpty(); + } } + +