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: _?_:_(