Copilot commented on code in PR #13858:
URL: https://github.com/apache/skywalking/pull/13858#discussion_r3178253154
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/v2/compiler/LALValueCodegen.java:
##########
@@ -865,80 +1260,465 @@ static void generateProcessRegistryCall(
// ==================== Utility methods ====================
+ // ==================== Binary expression codegen ====================
+
+ /**
+ * JLS-style numeric type tag used for binary numeric promotion. Ordered
+ * narrow → wide, so {@code commonType(a, b)} can return the wider of two.
+ */
+ private enum ExprType {
+ INT, LONG, FLOAT, DOUBLE,
+ STRING, BOOLEAN, OBJECT, UNKNOWN;
+
+ boolean isNumeric() {
+ return this == INT || this == LONG || this == FLOAT || this ==
DOUBLE;
+ }
+ }
+
+ /**
+ * Parsed representation of a NUMBER literal token. Carries the inferred
+ * Java type and the literal text trimmed/suffixed for direct emission as
+ * Java source.
+ */
+ private static final class NumericLiteral {
+ final ExprType type;
+ /** Java-source representation, e.g. "10000", "10000L", "1.5", "1.5f".
*/
+ final String javaText;
+
+ private NumericLiteral(final ExprType type, final String javaText) {
+ this.type = type;
+ this.javaText = javaText;
+ }
+
+ static NumericLiteral parse(final String numText) {
+ String t = numText;
+ char suffix = 0;
+ if (!t.isEmpty()) {
+ final char last = t.charAt(t.length() - 1);
+ if (last == 'L' || last == 'l' || last == 'F' || last == 'f'
+ || last == 'D' || last == 'd') {
+ suffix = last;
+ t = t.substring(0, t.length() - 1);
+ }
+ }
+ final boolean fractional = t.contains(".") || t.contains("e") ||
t.contains("E");
+ switch (suffix) {
+ case 'L':
+ case 'l':
+ return new NumericLiteral(ExprType.LONG, t + "L");
Review Comment:
`NumericLiteral.parse()` validates the range only for bare integer literals.
An out-of-range `L`-suffixed literal like `999999999999999999999L` now passes
through here and gets emitted as Java source even though it does not fit in a
`long`, so the rule fails later with an opaque compiler error instead of the
clear `IllegalArgumentException` that unsuffixed literals now get.
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/v2/compiler/LALScriptParser.java:
##########
@@ -595,19 +602,75 @@ private static Condition visitConditionExprAsCondition(
// ==================== Value access ====================
private static ValueAccess visitValueAccess(final
LALParser.ValueAccessContext ctx) {
+ return visitValueAccessAdd(ctx.valueAccessAdd());
+ }
+
+ private static ValueAccess visitValueAccessAdd(final
LALParser.ValueAccessAddContext ctx) {
+ final List<LALParser.ValueAccessMulContext> muls =
ctx.valueAccessMul();
+ if (muls.size() == 1) {
+ return visitValueAccessMul(muls.get(0));
+ }
+ final List<ValueAccess> parts = new ArrayList<>(muls.size());
+ for (final LALParser.ValueAccessMulContext mul : muls) {
+ parts.add(visitValueAccessMul(mul));
+ }
+ return binaryExpr(parts, collectInfixOps(ctx,
+ LALLexer.PLUS, LALScriptModel.BinaryOp.PLUS,
+ LALLexer.MINUS, LALScriptModel.BinaryOp.MINUS));
Review Comment:
Because `valueAccess` now includes additive/multiplicative expressions
everywhere, function and method arguments can parse expressions like
`substring(1 + 1)` or `foo(a * 2)`. However the downstream codegen for method
arguments still only knows how to render literals and local variables
(`generateMethodArgs` throws on expression nodes), so these newly accepted
forms will fail during code generation.
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/v2/compiler/LALValueCodegen.java:
##########
@@ -865,80 +1260,465 @@ static void generateProcessRegistryCall(
// ==================== Utility methods ====================
+ // ==================== Binary expression codegen ====================
+
+ /**
+ * JLS-style numeric type tag used for binary numeric promotion. Ordered
+ * narrow → wide, so {@code commonType(a, b)} can return the wider of two.
+ */
+ private enum ExprType {
+ INT, LONG, FLOAT, DOUBLE,
+ STRING, BOOLEAN, OBJECT, UNKNOWN;
+
+ boolean isNumeric() {
+ return this == INT || this == LONG || this == FLOAT || this ==
DOUBLE;
+ }
+ }
+
+ /**
+ * Parsed representation of a NUMBER literal token. Carries the inferred
+ * Java type and the literal text trimmed/suffixed for direct emission as
+ * Java source.
+ */
+ private static final class NumericLiteral {
+ final ExprType type;
+ /** Java-source representation, e.g. "10000", "10000L", "1.5", "1.5f".
*/
+ final String javaText;
+
+ private NumericLiteral(final ExprType type, final String javaText) {
+ this.type = type;
+ this.javaText = javaText;
+ }
+
+ static NumericLiteral parse(final String numText) {
+ String t = numText;
+ char suffix = 0;
+ if (!t.isEmpty()) {
+ final char last = t.charAt(t.length() - 1);
+ if (last == 'L' || last == 'l' || last == 'F' || last == 'f'
+ || last == 'D' || last == 'd') {
+ suffix = last;
+ t = t.substring(0, t.length() - 1);
+ }
+ }
+ final boolean fractional = t.contains(".") || t.contains("e") ||
t.contains("E");
+ switch (suffix) {
+ case 'L':
+ case 'l':
+ return new NumericLiteral(ExprType.LONG, t + "L");
+ case 'F':
+ case 'f':
+ return new NumericLiteral(ExprType.FLOAT,
+ (fractional ? t : (t + ".0")) + "f");
+ case 'D':
+ case 'd':
+ return new NumericLiteral(ExprType.DOUBLE,
+ (fractional ? t : (t + ".0")) + "d");
+ default:
+ if (fractional) {
+ return new NumericLiteral(ExprType.DOUBLE, t);
+ }
+ // Bare integer literal — INT if it fits, else LONG.
+ // Reject values that don't fit in Java's long range
+ // upfront so the parser raises a clear error rather
+ // than letting Javassist fail later on `<huge>L`.
+ try {
+ Integer.parseInt(t);
+ return new NumericLiteral(ExprType.INT, t);
+ } catch (NumberFormatException ignoredInt) {
+ try {
+ Long.parseLong(t);
+ return new NumericLiteral(ExprType.LONG, t + "L");
+ } catch (NumberFormatException ignoredLong) {
+ throw new IllegalArgumentException(
+ "Numeric literal '" + numText
+ + "' exceeds the supported range "
+ + "(must fit in a Java long; use a "
+ + "fractional or 'D'/'F'-suffixed form "
+ + "for larger magnitudes)");
+ }
+ }
+ }
+ }
+ }
+
+ /**
+ * Returns the JLS-style binary numeric promotion result for the two
operand
+ * types. Both must be numeric; otherwise this is a programmer bug (the
+ * caller is responsible for the non-numeric error path).
+ */
+ private static ExprType promote(final ExprType a, final ExprType b) {
+ if (a == ExprType.DOUBLE || b == ExprType.DOUBLE) {
+ return ExprType.DOUBLE;
+ }
+ if (a == ExprType.FLOAT || b == ExprType.FLOAT) {
+ return ExprType.FLOAT;
+ }
+ if (a == ExprType.LONG || b == ExprType.LONG) {
+ return ExprType.LONG;
+ }
+ return ExprType.INT;
+ }
+
+ private static String javaPrimitiveName(final ExprType t) {
+ switch (t) {
+ case LONG:
+ return "long";
+ case FLOAT:
+ return "float";
+ case DOUBLE:
+ return "double";
+ case INT:
+ default:
+ return "int";
+ }
+ }
+
+ private static String javaWrapperName(final ExprType t) {
+ switch (t) {
+ case LONG:
+ return "Long";
+ case FLOAT:
+ return "Float";
+ case DOUBLE:
+ return "Double";
+ case INT:
+ default:
+ return "Integer";
+ }
+ }
+
+ private static Class<?> primitiveClass(final ExprType t) {
+ switch (t) {
+ case LONG:
+ return long.class;
+ case FLOAT:
+ return float.class;
+ case DOUBLE:
+ return double.class;
+ case INT:
+ default:
+ return int.class;
+ }
+ }
+
+ /**
+ * Compile-time inference of an operand's Java type. Operates on the same
+ * AST that codegen will subsequently emit, so the result is consistent.
+ */
+ private static ExprType inferType(final LALScriptModel.ValueAccess part,
+ final LALClassGenerator.GenCtx genCtx) {
+ if (part == null) {
+ return ExprType.UNKNOWN;
+ }
+ if (part.isStringLiteral()) {
+ return ExprType.STRING;
+ }
+ if (part.isNumberLiteral() && part.getChain().isEmpty()) {
+ return NumericLiteral.parse(part.getSegments().get(0)).type;
+ }
+ // (expr as Cast) primary — only trustable when no further chain is
applied.
+ if (part.getParenInner() != null && part.getChain().isEmpty()) {
+ if (part.getParenCast() != null) {
+ return castToType(part.getParenCast());
+ }
+ // Pure grouping (no cast): the parens just preserve precedence.
+ return inferType(part.getParenInner(), genCtx);
+ }
+ // Nested binary expression: recompute via its parts/ops.
+ if (!part.getConcatParts().isEmpty()) {
+ return inferBinaryType(part.getConcatParts(), part.getConcatOps(),
genCtx);
+ }
+ // def variable with a known resolved type.
+ if (!part.getSegments().isEmpty()) {
+ final LALClassGenerator.LocalVarInfo lv =
+ genCtx.localVars.get(part.getSegments().get(0));
+ if (lv != null && part.getChain().isEmpty()) {
+ final Class<?> t = lv.resolvedType;
+ if (t == int.class || t == Integer.class) {
+ return ExprType.INT;
+ }
+ if (t == long.class || t == Long.class) {
+ return ExprType.LONG;
+ }
+ if (t == float.class || t == Float.class) {
+ return ExprType.FLOAT;
+ }
+ if (t == double.class || t == Double.class) {
+ return ExprType.DOUBLE;
+ }
+ if (t == String.class) {
+ return ExprType.STRING;
+ }
+ if (t == boolean.class || t == Boolean.class) {
+ return ExprType.BOOLEAN;
+ }
+ return ExprType.OBJECT;
+ }
+ }
+ // tag() / sourceAttribute() — always return String at runtime.
+ if ("tag".equals(part.getFunctionCallName())
+ || "sourceAttribute".equals(part.getFunctionCallName())) {
+ return ExprType.STRING;
+ }
+ // parsed.* — when the rule has a typed inputType (no JSON/YAML/TEXT
+ // parser), walk the proto getter chain via reflection so primitive
+ // numeric leaves participate in arithmetic and don't fall through
+ // to string concat. Mirrors {@link #generateExtraLogAccess} but
+ // does not emit code or local variables.
+ if (part.isParsedRef()
+ && genCtx.parserType == LALClassGenerator.ParserType.NONE
+ && genCtx.inputType != null) {
+ final Class<?> primitive = walkProtoChainPrimitiveType(
+ part.getChain(), genCtx.inputType);
+ if (primitive != null) {
+ if (primitive == int.class) {
+ return ExprType.INT;
+ }
+ if (primitive == long.class) {
+ return ExprType.LONG;
+ }
+ if (primitive == float.class) {
+ return ExprType.FLOAT;
+ }
+ if (primitive == double.class) {
+ return ExprType.DOUBLE;
+ }
+ if (primitive == boolean.class) {
+ return ExprType.BOOLEAN;
+ }
+ }
+ }
+ return ExprType.UNKNOWN;
+ }
+
+ /**
+ * Walk a {@code parsed.*} field-segment chain over the typed proto root
+ * via reflection, returning the primitive return type of the final getter
+ * if the chain is field-only and ends at a primitive accessor; {@code
null}
+ * otherwise.
+ */
+ private static Class<?> walkProtoChainPrimitiveType(
+ final List<LALScriptModel.ValueAccessSegment> chain,
+ final Class<?> rootType) {
+ if (chain == null || chain.isEmpty()) {
+ return null;
+ }
+ Class<?> currentType = rootType;
+ for (int i = 0; i < chain.size(); i++) {
+ final LALScriptModel.ValueAccessSegment seg = chain.get(i);
+ if (!(seg instanceof LALScriptModel.FieldSegment)) {
+ return null;
+ }
+ final String field = ((LALScriptModel.FieldSegment) seg).getName();
+ final String getter = "get" +
Character.toUpperCase(field.charAt(0))
+ + field.substring(1);
+ try {
+ currentType = currentType.getMethod(getter).getReturnType();
+ } catch (NoSuchMethodException e) {
+ return null;
+ }
+ }
+ return currentType.isPrimitive() ? currentType : null;
+ }
+
+ private static ExprType castToType(final String cast) {
+ if ("Integer".equals(cast)) {
+ return ExprType.INT;
+ }
+ if ("Long".equals(cast)) {
+ return ExprType.LONG;
+ }
+ if ("Float".equals(cast)) {
+ return ExprType.FLOAT;
+ }
+ if ("Double".equals(cast)) {
+ return ExprType.DOUBLE;
+ }
+ if ("String".equals(cast)) {
+ return ExprType.STRING;
+ }
+ if ("Boolean".equals(cast)) {
+ return ExprType.BOOLEAN;
+ }
+ return ExprType.UNKNOWN;
+ }
+
/**
- * Returns {@code true} when every concat part is numeric — i.e. a
- * parenthesized expression with an {@code Integer} or {@code Long} cast,
- * or a bare number literal. If so, {@code +} is arithmetic, not string
- * concatenation.
+ * Walk the operand list applying JLS promotion left-to-right, treating any
+ * {@code +} that touches a String operand as string concat (which sticks).
*/
- private static boolean allPartsNumeric(final
List<LALScriptModel.ValueAccess> parts) {
- for (final LALScriptModel.ValueAccess part : parts) {
- if (part.isNumberLiteral()) {
+ private static ExprType inferBinaryType(
+ final List<LALScriptModel.ValueAccess> parts,
+ final List<LALScriptModel.BinaryOp> ops,
+ final LALClassGenerator.GenCtx genCtx) {
+ ExprType acc = inferType(parts.get(0), genCtx);
+ for (int i = 1; i < parts.size(); i++) {
+ final ExprType rhs = inferType(parts.get(i), genCtx);
+ final LALScriptModel.BinaryOp op = ops.get(i - 1);
+ if (op == LALScriptModel.BinaryOp.PLUS
+ && (acc == ExprType.STRING || rhs == ExprType.STRING)) {
+ acc = ExprType.STRING;
continue;
}
- if (part.getParenInner() != null &&
isNumericCast(part.getParenCast())) {
+ if (acc.isNumeric() && rhs.isNumeric()) {
+ acc = promote(acc, rhs);
continue;
}
- return false;
+ // Anything else for + (object + object) → fallback to string
concat
+ // (preserves long-standing behaviour for `tag("a") + tag("b")`
etc.).
+ if (op == LALScriptModel.BinaryOp.PLUS) {
+ acc = ExprType.STRING;
+ continue;
+ }
+ // - * / on non-numeric — caller emits a compile error.
+ return ExprType.UNKNOWN;
}
- return true;
+ return acc;
}
- private static boolean isNumericCast(final String cast) {
- return "Integer".equals(cast) || "Long".equals(cast);
+ /**
+ * Top-level binary-expression codegen. Mirrors Java's left-to-right
+ * evaluation: adjacent numeric operands accumulate into a primitive
+ * arithmetic expression; once a String operand appears, every subsequent
+ * {@code +} becomes string concatenation. This preserves the semantic
+ * difference between {@code 1 + 2 + "x"} (= {@code "3x"}) and
+ * {@code "" + 1 + 2 + "x"} (= {@code "12x"}).
+ */
+ private static void generateBinaryExpression(
+ final StringBuilder sb,
+ final List<LALScriptModel.ValueAccess> parts,
+ final List<LALScriptModel.BinaryOp> ops,
+ final LALClassGenerator.GenCtx genCtx) {
+ // Render the first operand and seed the accumulator with its type.
+ ExprType accType = inferType(parts.get(0), genCtx);
+ final StringBuilder accBuf = new StringBuilder();
+ appendOperandRaw(accBuf, parts.get(0), accType, genCtx);
+ String accExpr = accBuf.toString();
+
+ for (int i = 1; i < parts.size(); i++) {
+ final LALScriptModel.BinaryOp op = ops.get(i - 1);
+ final ExprType rhsType = inferType(parts.get(i), genCtx);
+ final StringBuilder rhsBuf = new StringBuilder();
+ appendOperandRaw(rhsBuf, parts.get(i), rhsType, genCtx);
+ final String rhsExpr = rhsBuf.toString();
+
+ if (accType.isNumeric() && rhsType.isNumeric()) {
+ final ExprType promoted = promote(accType, rhsType);
+ final String widenedAcc = widenPrimitiveExpr(accExpr, accType,
promoted);
+ final String widenedRhs = widenPrimitiveExpr(rhsExpr, rhsType,
promoted);
+ accExpr = "(" + widenedAcc + " " + opSymbol(op) + " " +
widenedRhs + ")";
+ accType = promoted;
+ continue;
+ }
+ if (op != LALScriptModel.BinaryOp.PLUS) {
+ throw new IllegalArgumentException(
+ "Operator '" + opSymbol(op) + "' requires numeric
operands; "
+ + "got non-numeric expression. Cast operands with "
+ + "'as Integer/Long/Float/Double' to enable
arithmetic.");
+ }
+ // String concat path. Coerce the accumulator to String when
+ // needed: a STRING accumulator is fine; a numeric accumulator
+ // is already in parens (so Java evaluates it before the concat,
+ // preserving `(1 + 2) + "x"` → `"3x"`); anything else needs a
+ // leading "" to make Java accept Object + Object.
+ if (accType != ExprType.STRING && !accType.isNumeric()) {
Review Comment:
The new string-concat fallback still emits raw Java `+` when a numeric
prefix is followed by an operand whose type is unknown/non-String (for example
`1 + parsed.field` in JSON/YAML mode, where `parsed.field` compiles to
`h.mapVal(...)` and has type `Object`). That generates uncompilable Java
because `int + Object` is not legal; the old `"" + ...` coercion handled these
cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
