From 8488c1eca93f9cb45101e683cf9b5f29088a7265 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 22:13:50 +0000 Subject: [PATCH] fix(microflows): keep aggregates whole now that SET is optional MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Making SET optional put `$X = ` in front of every `VARIABLE EQUALS ` statement in the grammar. ANTLR picks the lowest-numbered alternative that matches, so `$Sum = sum($List.Price)` stopped reaching aggregateListStatement and fell through to the generic SET conversion, which joined the list and the attribute into one name. mxbuild rejected the result on both engines: [CE0109] "Undefined variable 'ProductList.Price'." [CE0015] "Aggregate function must specify a valid attribute." Move setStatement last so it only claims what no dedicated rule parses, and make the SET conversion agree with the canonical one — it also dropped the per-item expression of `sum($List, $currentObject/Price * 0.21)`, which buildListAggregateAsFunction never appended in the first place. That half was wrong before the bare form ever reached it, so `set $X = sum($List.Price)` was broken too. Proved by reverting both halves: the new tests fail with the reported symptom, and the same script executed against a blank 11.12.1 project reports 8 errors on a control binary and 0 with the fix. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JXnEgoc2NQP1Y2TWMCMXC4 --- .claude/skills/fix-issue.md | 1 + .../aggregate-after-optional-set.mdl | 62 +++++++++++ mdl/grammar/domains/MDLMicroflow.g4 | 8 +- .../visitor_microflow_aggregate_test.go | 103 ++++++++++++++++++ mdl/visitor/visitor_microflow_expression.go | 7 ++ mdl/visitor/visitor_microflow_statements.go | 88 +++++++-------- 6 files changed, 219 insertions(+), 50 deletions(-) create mode 100644 mdl-examples/bug-tests/aggregate-after-optional-set.mdl create mode 100644 mdl/visitor/visitor_microflow_aggregate_test.go diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index f03e16d82..4998ccfbf 100644 --- a/.claude/skills/fix-issue.md +++ b/.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 '**'` returns nothing | Not a bad write. Declaring and resolving are **separate steps**: the model records the coordinate, and `mx sync-java-dependencies ` 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 = ` overlaps every `VARIABLE EQUALS ` 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 | diff --git a/mdl-examples/bug-tests/aggregate-after-optional-set.mdl b/mdl-examples/bug-tests/aggregate-after-optional-set.mdl new file mode 100644 index 000000000..3960099bd --- /dev/null +++ b/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 = ` 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; +/ diff --git a/mdl/grammar/domains/MDLMicroflow.g4 b/mdl/grammar/domains/MDLMicroflow.g4 index b957e68d0..a4b8ae353 100644 --- a/mdl/grammar/domains/MDLMicroflow.g4 +++ b/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 = ` overlaps every + // `VARIABLE EQUALS ` 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 diff --git a/mdl/visitor/visitor_microflow_aggregate_test.go b/mdl/visitor/visitor_microflow_aggregate_test.go new file mode 100644 index 000000000..7787af2d6 --- /dev/null +++ b/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 = ` in front of every +// `VARIABLE EQUALS ` 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, )` 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 +} diff --git a/mdl/visitor/visitor_microflow_expression.go b/mdl/visitor/visitor_microflow_expression.go index b3e464cb9..b119922c2 100644 --- a/mdl/visitor/visitor_microflow_expression.go +++ b/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 } diff --git a/mdl/visitor/visitor_microflow_statements.go b/mdl/visitor/visitor_microflow_statements.go index 88bd14914..1ca597301 100644 --- a/mdl/visitor/visitor_microflow_statements.go +++ b/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.