Aggregates broke when SET became optional (upstream CI regression)

.claude/skills/fix-issue.md

@@ -401,3 +401,4 @@ extracting `OffsetExpression`/`LimitExpression`.
 | `ALTER MODULE X ADD JAR DEPENDENCY (…)` succeeds, `list jar dependencies` reports it, the build is **green** — and the runtime throws `SQLException: No JDBC driver found in app for URL`. `deployment/build.gradle` has no dependencies block and `find deployment -iname '*<artifact>*'` returns nothing | Not a bad write. Declaring and resolving are **separate steps**: the model records the coordinate, and `mx sync-java-dependencies <project.mpr>` is what downloads it into `vendorlib/`. Studio Pro runs that when you edit Module Settings; nothing headless was running it. Confirmed on 11.12.1 — a full `mxbuild --target=deploy` resolves nothing, and the sync command then fetches the jar | `cmd/mxcli/docker/javadeps.go` (`SyncJavaDependencies`, `UnvendoredJarDependencies`), `cmd/mxcli/cmd_sync_java_deps.go` (`mxcli sync-java-deps [--check]`), `cmd/mxcli/docker/runlocal.go` (vendors before boot), `mdl/executor/cmd_modules.go` (`warnUnvendoredJarDependencies`) | **How to find the missing step**: the reporter's open question was "does mxbuild skip Maven resolution, or does mxcli write it somewhere MxBuild cannot read?" — neither. `strings mx.dll | grep -i dependenc` surfaced `ISyncJavaDependenciesRunner`/`SkipManagedDependencySync`, and `mx --help` listed `sync-java-dependencies`. When a model-level write "works" but the artefact never appears, check whether the **toolset** has a separate command for it before suspecting the write. Wired at three levels so the gap cannot stay silent: the executor says so the moment it writes an unvendored coordinate, `run --local` resolves it before boot, and `--check` exits non-zero as a build gate. Resolution needs network, so every call site is best-effort with an actionable message. Tests `cmd/mxcli/docker/javadeps_test.go`. mxcli-formula1 #12 |
 | `$Total = 5;` does not parse — `no viable alternative at input '$Total=5'` — while `DECLARE $Total Integer = 0;` does, and so do `$X = HEAD($List)`, `$X = create M.E (…)` and `$X = execute database query …`. The error names the token, not the missing keyword | Assignment existed only as a **prefix** on specific activity statements (`(VARIABLE EQUALS)?` on CALL/CREATE/RETRIEVE/…), plus a `SET $Var = expression` statement. A plain value therefore required `SET`, which nothing in the error or the surrounding syntax suggested | `mdl/grammar/domains/MDLMicroflow.g4` (`setStatement : SET? …`), `cmd/mxcli/syntax/features_microflow.go` | Make the guessable form work rather than improve the error: `SET` is now optional and both spellings produce the same `MfSetStmt`/`ChangeVariableAction`. **Prove a grammar relaxation causes no regressions with a control binary, not by reading**: `git stash` the `.g4`, `make grammar`, build `bin/mxcli-control`, sweep every `mdl-examples/**/*.mdl` with both — 13 scripts fail, the *same* 13, all pre-existing. ANTLR's adaptive prediction picks the activity-prefixed alternatives over `setStatement` on its own; no ordering change was needed. Executed against a real .mpr, mxbuild reports 0 errors. Tests `mdl/visitor/visitor_microflow_bare_assign_test.go` (bare and keyword forms must agree on the AST, not merely both parse). mxcli-formula1 #13 |
 | `mxcli test tests/ -p app/App.mpr` fails with "no such file or directory" for a `tests/` that sits right next to the `.mpr` | Test paths resolved against the process CWD only. Defensible in isolation, but mxcli otherwise encourages naming the project (`-p`) rather than standing in its directory, and project auto-discovery searches outward — so the two conventions collide and the failure looks like a missing directory | `cmd/mxcli/cmd_test_run.go` (`resolveTestPaths`) | Fall back to project-relative **only when the CWD-relative path does not exist**: a `tests/` in both places must resolve to the one the user is standing in, since silently preferring the project's copy would run the wrong suite. A path that exists in neither is passed through unchanged so the error names what was typed, not a rewritten path the user never mentioned. Tests `cmd/mxcli/cmd_test_run_paths_test.go`. mxcli-formula1 #13 |
+| After `SET` became optional, `mx check` on a project built from `02-microflow-examples.mdl` reports six errors that no MDL change caused: `[CE0109] "Undefined variable 'ProductList.Price'."` at four Aggregate list activities, and `[CE0015] "Aggregate function must specify a valid attribute."` at the expression-based one. Identical on both engines. `mxcli check` on the same script is silent | Two conversions existed for one syntax. `$Sum = sum($List.Price)` used to reach the dedicated `aggregateListStatement` rule; making `SET` optional put `setStatement` — alternative 5 of ~50 — in front of it, so ANTLR matched the lower-numbered alternative and the statement fell through to `buildSetStatement`'s fallback conversion, which joined list and attribute into one name and dropped the per-item expression entirely. Underneath, `buildListAggregateAsFunction` never appended the expression argument, so the SET path could not have seen it either | `mdl/grammar/domains/MDLMicroflow.g4` (`setStatement` moved LAST in `microflowStatement`), `mdl/visitor/visitor_microflow_statements.go` (`buildSetAggregate` replaces `extractVariableAndAttribute`), `mdl/visitor/visitor_microflow_expression.go` (`buildListAggregateAsFunction` appends the expression argument) | **A permissive alternative belongs last.** `$X = <expr>` overlaps every `VARIABLE EQUALS <function-call>` statement in the rule — aggregates, list operations, RANGE — and ANTLR's ALL(*) picks the lowest-numbered alternative that matches, so a new general form silently steals from every specific one above it. **The measurement trap that let this ship**: the `SET?` change was swept with a control binary over every `mdl-examples/**/*.mdl` and found no difference — but with `mxcli check`, which parses and validates and never serializes. This defect lives between the AST and the BSON, where only `exec` + `mx check` can see it. Sweeping with `check` proves the grammar still *parses*; it proves nothing about what the visitor *builds*. For a grammar change, the control sweep must run the integration gate (`go test -tags integration -run TestMxCheck_DoctypeScripts`), not `check`. Fix proven by reverting both halves and watching the new tests fail with the reported symptom. Tests `mdl/visitor/visitor_microflow_aggregate_test.go`. Upstream CI on the ako→mendixlabs sync PR |

mdl-examples/bug-tests/aggregate-after-optional-set.mdl

@@ -0,0 +1,62 @@
+-- Aggregates must survive SET becoming optional.
+--
+-- `$Sum = sum($List.Price)` used to reach the dedicated aggregateListStatement
+-- rule. Making SET optional put `$X = <expr>` ahead of it in the grammar, so the
+-- statement fell through to the generic SET conversion, which joined the list
+-- and the attribute into one name and dropped the per-item expression:
+--
+--   [CE0109] "Undefined variable 'ProductList.Price'."
+--   [CE0015] "Aggregate function must specify a valid attribute."
+--
+-- Every aggregate below must build with 0 errors, in both spellings.
+
+create entity Bug.Product (
+  Name: string(200),
+  Price: decimal
+);
+
+-- Bare form (no SET keyword) — this is what regressed.
+create microflow Bug.AggregatesBare (
+  $ProductList: list of Bug.Product
+)
+returns decimal as $Total
+begin
+  $Count = count($ProductList);
+  $Total = sum($ProductList.Price);
+  $Average = average($ProductList.Price);
+  $Lowest = minimum($ProductList.Price);
+  $Highest = maximum($ProductList.Price);
+  -- Aggregate over a value computed per item.
+  $Tax = sum($ProductList, $currentObject/Price * 0.21);
+  return $Total;
+end;
+/
+
+-- The SET keyword must produce exactly the same activities. It routes through a
+-- different conversion in the visitor, and that one was wrong even before the
+-- bare form ever reached it.
+create microflow Bug.AggregatesWithSetKeyword (
+  $ProductList: list of Bug.Product
+)
+returns decimal as $Total
+begin
+  set $Count = count($ProductList);
+  set $Total = sum($ProductList.Price);
+  set $Average = average($ProductList.Price);
+  set $Lowest = minimum($ProductList.Price);
+  set $Highest = maximum($ProductList.Price);
+  set $Tax = sum($ProductList, $currentObject/Price * 0.21);
+  return $Total;
+end;
+/
+
+-- A plain value assignment must still be a Change Variable, not an aggregate.
+create microflow Bug.PlainAssignment ()
+returns integer as $N
+begin
+  declare $N integer = 0;
+  $N = 5;
+  $N = $N + 1;
+  return $N;
+end;
+/

mdl/grammar/domains/MDLMicroflow.g4

@@ -120,7 +120,6 @@ microflowStatement
     | annotation* caseStatement SEMICOLON
     | annotation* inheritanceSplitStatement SEMICOLON
     | annotation* castObjectStatement SEMICOLON
-    | annotation* setStatement SEMICOLON
     | annotation* createListStatement SEMICOLON       // Must be before createObjectStatement to match "CREATE LIST OF"
     | annotation* createObjectStatement SEMICOLON
     | annotation* changeObjectStatement SEMICOLON
@@ -170,6 +169,13 @@ microflowStatement
     | annotation* openWorkflowStatement SEMICOLON
     | annotation* lockWorkflowStatement SEMICOLON
     | annotation* unlockWorkflowStatement SEMICOLON
+    // LAST on purpose. Since SET became optional, `$X = <expr>` overlaps every
+    // `VARIABLE EQUALS <function-call>` statement above — aggregates, list
+    // operations, RANGE. Those rules must keep winning: a lower-numbered
+    // setStatement swallowed `$Sum = sum($List.Price)` into a Change Variable
+    // whose fallback conversion drops the attribute, which mxbuild rejects
+    // (CE0015 / CE0109). Last means it only claims what nothing else parses.
+    | annotation* setStatement SEMICOLON
     ;
 
 declareStatement

mdl/visitor/visitor_microflow_aggregate_test.go

@@ -0,0 +1,103 @@
+// SPDX-License-Identifier: Apache-2.0
+
+package visitor
+
+import (
+	"testing"
+
+	"github.com/mendixlabs/mxcli/mdl/ast"
+)
+
+// Making SET optional put `$X = <expr>` in front of every
+// `VARIABLE EQUALS <function-call>` statement in the grammar, so
+// `$Sum = sum($List.Price)` stopped reaching aggregateListStatement and fell
+// through to the SET conversion instead — which joined the list and the
+// attribute into one name. mxbuild rejected the result:
+//
+//	[CE0109] "Undefined variable 'ProductList.Price'."
+//	[CE0015] "Aggregate function must specify a valid attribute."
+//
+// Both spellings must produce the same aggregate, whichever rule claims them.
+func TestAggregateSplitsListFromAttribute(t *testing.T) {
+	cases := []struct {
+		name, src string
+		wantOp    ast.AggregateListOperationType
+		wantAttr  string
+	}{
+		{"bare sum", "$T = sum($ProductList.Price);", ast.AggregateSum, "Price"},
+		{"bare average", "$T = average($ProductList.Price);", ast.AggregateAverage, "Price"},
+		{"bare minimum", "$T = minimum($ProductList.Price);", ast.AggregateMinimum, "Price"},
+		{"bare maximum", "$T = maximum($ProductList.Price);", ast.AggregateMaximum, "Price"},
+		// The SET keyword routes through a different conversion; it was wrong
+		// there before the bare form ever reached it.
+		{"set sum", "set $T = sum($ProductList.Price);", ast.AggregateSum, "Price"},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			got := parseSingleAggregate(t, tc.src)
+			if got.Operation != tc.wantOp {
+				t.Errorf("operation = %v, want %v", got.Operation, tc.wantOp)
+			}
+			if got.InputVariable != "ProductList" {
+				t.Errorf("input variable = %q, want ProductList (the list, without the attribute)", got.InputVariable)
+			}
+			if got.Attribute != tc.wantAttr {
+				t.Errorf("attribute = %q, want %q", got.Attribute, tc.wantAttr)
+			}
+		})
+	}
+}
+
+// `sum($List, <expression>)` aggregates a value computed per item. Losing the
+// expression leaves an aggregate with nothing to aggregate — CE0015.
+func TestAggregateKeepsPerItemExpression(t *testing.T) {
+	for _, src := range []string{
+		"$T = sum($ProductList, $currentObject/Price * 0.21);",
+		"set $T = sum($ProductList, $currentObject/Price * 0.21);",
+	} {
+		got := parseSingleAggregate(t, src)
+		if got.InputVariable != "ProductList" {
+			t.Errorf("%s: input variable = %q, want ProductList", src, got.InputVariable)
+		}
+		if !got.IsExpression || got.Expression == nil {
+			t.Errorf("%s: expression dropped (IsExpression=%v, Expression=%v)", src, got.IsExpression, got.Expression)
+		}
+	}
+}
+
+// COUNT takes the list alone and must not acquire an attribute.
+func TestAggregateCountTakesTheListAlone(t *testing.T) {
+	got := parseSingleAggregate(t, "$N = count($ProductList);")
+	if got.Operation != ast.AggregateCount || got.InputVariable != "ProductList" || got.Attribute != "" {
+		t.Errorf("got %+v, want COUNT over ProductList with no attribute", got)
+	}
+}
+
+func parseSingleAggregate(t *testing.T, stmt string) *ast.AggregateListStmt {
+	t.Helper()
+	src := "create microflow M.A ($ProductList: list of M.Product)\nbegin\n  " + stmt + "\nend;"
+	prog, errs := Build(src)
+	if len(errs) > 0 {
+		t.Fatalf("parse errors for %q: %v", stmt, errs)
+	}
+	for _, s := range prog.Statements {
+		cm, ok := s.(*ast.CreateMicroflowStmt)
+		if !ok {
+			continue
+		}
+		for _, st := range cm.Body {
+			if agg, ok := st.(*ast.AggregateListStmt); ok {
+				return agg
+			}
+			// A SET means the statement was swallowed as a plain value
+			// assignment — that is the regression, and it reads better as a
+			// failure here than as a nil dereference below.
+			if set, ok := st.(*ast.MfSetStmt); ok {
+				t.Fatalf("%q produced a Change Variable (target %q), not an aggregate", stmt, set.Target)
+			}
+		}
+	}
+	t.Fatalf("no AggregateListStmt produced by %q", stmt)
+	return nil
+}

mdl/visitor/visitor_microflow_expression.go

@@ -526,6 +526,13 @@ func buildListAggregateAsFunction(ctx parser.IListAggregateOperationContext) ast
 		}
 	}
 
+	// The per-item expression of `sum($List, $currentObject/Price * 0.21)`.
+	// Without it the call reads as a one-argument aggregate, and whoever
+	// consumes it builds an aggregate with nothing to aggregate — CE0015.
+	if exprCtx := aggrCtx.Expression(); exprCtx != nil {
+		funcExpr.Arguments = append(funcExpr.Arguments, buildSourceExpression(exprCtx))
+	}
+
 	return funcExpr
 }
 

mdl/visitor/visitor_microflow_statements.go

@@ -808,37 +808,13 @@ func buildSetStatement(ctx parser.ISetStatementContext) ast.MicroflowStatement {
 				InputVariable:  extractVariableName(funcCall.Arguments, 0),
 			}
 		case "SUM":
-			inputVar, attr := extractVariableAndAttribute(funcCall.Arguments, 0)
-			return &ast.AggregateListStmt{
-				OutputVariable: targetVar,
-				Operation:      ast.AggregateSum,
-				InputVariable:  inputVar,
-				Attribute:      attr,
-			}
+			return buildSetAggregate(targetVar, ast.AggregateSum, funcCall.Arguments)
 		case "AVERAGE":
-			inputVar, attr := extractVariableAndAttribute(funcCall.Arguments, 0)
-			return &ast.AggregateListStmt{
-				OutputVariable: targetVar,
-				Operation:      ast.AggregateAverage,
-				InputVariable:  inputVar,
-				Attribute:      attr,
-			}
+			return buildSetAggregate(targetVar, ast.AggregateAverage, funcCall.Arguments)
 		case "MINIMUM":
-			inputVar, attr := extractVariableAndAttribute(funcCall.Arguments, 0)
-			return &ast.AggregateListStmt{
-				OutputVariable: targetVar,
-				Operation:      ast.AggregateMinimum,
-				InputVariable:  inputVar,
-				Attribute:      attr,
-			}
+			return buildSetAggregate(targetVar, ast.AggregateMinimum, funcCall.Arguments)
 		case "MAXIMUM":
-			inputVar, attr := extractVariableAndAttribute(funcCall.Arguments, 0)
-			return &ast.AggregateListStmt{
-				OutputVariable: targetVar,
-				Operation:      ast.AggregateMaximum,
-				InputVariable:  inputVar,
-				Attribute:      attr,
-			}
+			return buildSetAggregate(targetVar, ast.AggregateMaximum, funcCall.Arguments)
 		}
 	}
 
@@ -903,31 +879,45 @@ func getArgumentExpression(args []ast.Expression, index int) ast.Expression {
 	return args[index]
 }
 
-// extractVariableAndAttribute extracts variable and attribute from $Var/Attr or $Var, Attr.
-func extractVariableAndAttribute(args []ast.Expression, index int) (varName string, attrName string) {
-	if index >= len(args) {
-		return "", ""
+// buildSetAggregate builds an aggregate activity from a SET whose value is a
+// SUM/AVERAGE/MINIMUM/MAXIMUM call.
+//
+// It mirrors buildAggregateListStatement, which handles the same two spellings
+// when they arrive through the aggregateListStatement rule: one argument is a
+// list plus an attribute (`sum($List.Price)`), two arguments are a list plus an
+// expression evaluated per item (`sum($List, $currentObject/Price * 0.21)`).
+// Two conversions for one syntax is how the attribute went missing in the first
+// place, so the two must agree.
+func buildSetAggregate(targetVar string, op ast.AggregateListOperationType, args []ast.Expression) *ast.AggregateListStmt {
+	stmt := &ast.AggregateListStmt{OutputVariable: targetVar, Operation: op}
+	if len(args) == 0 {
+		return stmt
 	}
-	// Check for attribute path like $Var/Attr
-	if pathExpr, ok := args[index].(*ast.AttributePathExpr); ok {
-		varName = pathExpr.Variable
-		if len(pathExpr.Path) > 0 {
-			attrName = pathExpr.Path[len(pathExpr.Path)-1]
+
+	switch arg := args[0].(type) {
+	case *ast.AttributePathExpr:
+		stmt.InputVariable = arg.Variable
+		if len(arg.Path) > 0 {
+			stmt.Attribute = arg.Path[len(arg.Path)-1]
 		}
-		return
-	}
-	// Check for simple variable
-	if varExpr, ok := args[index].(*ast.VariableExpr); ok {
-		varName = varExpr.Name
-		// Look for attribute in next argument
-		if index+1 < len(args) {
-			if identExpr, ok := args[index+1].(*ast.IdentifierExpr); ok {
-				attrName = identExpr.Name
-			}
+	case *ast.VariableExpr:
+		// `sum($List.Price)` reaches the expression parser as one variable whose
+		// name carries the dot, not as an attribute path. Left joined, mxbuild
+		// reports the whole thing as an undefined variable (CE0109).
+		stmt.InputVariable = arg.Name
+		if list, attr, ok := strings.Cut(arg.Name, "."); ok {
+			stmt.InputVariable, stmt.Attribute = list, attr
 		}
-		return
 	}
-	return "", ""
+
+	// Any second argument is the per-item expression. Dropping it leaves an
+	// aggregate with nothing to aggregate, which mxbuild rejects with CE0015.
+	if len(args) > 1 {
+		stmt.IsExpression = true
+		stmt.Expression = args[1]
+		stmt.Attribute = ""
+	}
+	return stmt
 }
 
 // extractSortSpecs extracts sort specifications from function arguments.