From 9bbf194e436612ff56ed16a5374eb503ff533ffb Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 19:53:31 +0000 Subject: [PATCH 01/10] fix(widget): resolve concatenated hide lists in editorConfig MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `widget describe` said nothing about 23 properties Combo box's editor hides, and its MDL example offered two database-only properties for a combobox with `source: 'context'`, where the editor shows neither. hideTargetKeys read a hide call's property list by trimming a leading `[` and a trailing `]`. Combo box builds its biggest lists by concatenating module-level arrays onto the literal one: "context" === t.source && hidePropertiesIn(e, t, [ …3 literals… ].concat(N)) where N is ten database properties. The trim never found the `]` (the argument ends in `)`), so the split stopped at the first element and every concatenated name was dropped — silently, because the call still counted as recognized. The counter reported full coverage of a list it had read a third of. Parse the array with a balanced scan and resolve the trailing `.concat(...)` against a map of module-level string-array bindings. Resolution withholds rather than guesses: an argument that is neither an inline string array nor a known identifier — `.concat(n(b.static))`, a call on a computed key — makes the chain unresolvable and the literal keys stand alone; an identifier bound twice to different arrays is dropped, since the minifier reuses short names. Measured over the 42 describable widgets (population from the .mpk files, not the 33 .def.json — Combo box has none): displayed rules 332 -> 355 (+23, all Combo box) rules lost 0 (set comparison; a line diff reports five false losses from re-ordering) per-widget regressions 0 example blocks changed 1 — Combo box, losing exactly the two wrong bindings recognized counter 180 -> 180, correctly: this changes what a recognized call contributes, not which are readable Each gain traces to a concat list: 10 = N under `source === "context"`, 9 = W+z under the enumeration/boolean branch, 2+2 = z under the two caption-type else branches. 14 concat sites exist in the fixture; 6 resolve, 8 are computed at runtime and keep today's literal-only behaviour. Controls: the .mpk regression test fails without the fix naming all eight properties; each of the three refusals fails its own case when stubbed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KTdvGwVewkdgNQGDCxpQPZ --- .../fix-issue/findings/mdl-executor.jsonl | 1 + .../src/reference/query/describe-widget.md | 9 ++ mdl/executor/editorconfig_extract.go | 133 +++++++++++++++++- mdl/executor/editorconfig_shapes_test.go | 109 ++++++++++++++ 4 files changed, 249 insertions(+), 3 deletions(-) diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index 603291ee00..9f359be044 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -572,3 +572,4 @@ {"area": "mdl/executor", "date": "2026-09-09", "symptom": "A widget-describe change measured as 'zero rules lost' had in fact dropped SIX visibility rules from Combo box — the widget the work was justified by. The before/after sweep enumerated widgets from `.mxcli/widgets/*.def.json` (33 of them) but `mxcli widget describe` also serves widgets straight from their `.mpk`, and Combo box has no def.json. The real describable set is 42; the missing 9 included the one that mattered.", "cause": "The sweep's widget list was derived from an artifact of the pipeline (generated def.json files) rather than from the capability under test (what `widget describe` can describe). Nothing compared the two lists, so the sample silently excluded a whole class — the same 'two lists, nothing comparing them' shape as the defects being fixed.", "file": "mdl/executor/editorconfig_extract.go, mdl/executor/editorconfig_shapes_test.go", "insight": "Derive a sweep's population from the CAPABILITY, never from a convenient artifact, and state the population in the claim: 'zero rules lost across the 33 widgets carrying a def.json' would have been true and would have invited the question. A second trap immediately after: the regression test written to lock the fix used a hand-written editorConfig snippet of the same APPARENT shape, and it passed with the fix reverted — the nesting that triggers the drop is three levels deep and specific, so the synthetic case was never flagged conjunctive and the test proved nothing. Pointing the test at the real committed .mpk made the control fail with all six names. Rule of thumb: when a defect was found in real vendor input, the regression test takes the real input; a reconstructed minimal case must be shown to fail without the fix BEFORE it is trusted, and here it did not. The fix itself is the policy that should have been there from the start: conjunction support may WITHHOLD a rule the extractor never produced before (emitting one conjunct over-fires), but may never drop one the older vocabulary already lifted — that rule's accuracy is unchanged by the new work.", "refs": ["mendixlabs/mxcli#1036"]} {"area": "mdl/executor", "date": "2026-09-09", "symptom": "Six of Combo box's editorConfig hide-rules could not be lifted with their full condition, and its coverage counter sat at 21 of 32. The rules sit inside `\"association\"===t.optionsSourceType && ( … )`, itself the ELSE branch of a ternary inside `\"context\"===t.source ? ( … )` — a chained ternary, where each branch's BODY is parenthesised but each branch's CONDITION is not.", "cause": "groupGuard read the condition of a group opened after `&&`/`||` with trailingExpr, which stops at a STATEMENT separator. A chained ternary contains none, so it returned the whole `A ? (…) : B` expression as the group's condition. That is not a comparison, guardToCondition refused it, and enclosingGroupConditions reported the chain unreadable.", "file": "mdl/executor/editorconfig_extract.go (groupGuard, operandBefore)", "insight": "The characterisation written into the PR body — 'ternary chains without parentheses, which the outward walk does not traverse' — was WRONG, and instrumenting the walk rather than re-reading it is what showed so: the walk reaches these groups fine; the failure is guard EXTRACTION at the group, one function away. A one-line ceiling written from reasoning is worth re-deriving before anyone builds on it. The fix is not a straight swap to lastGuardExpr, which bounds at `{` and hands back a fragment with an unbalanced `}` where the expression follows a block (ProgressCircle's ternary follows a whole switch); take lastGuardExpr's answer only when it stopped at an INSIDE-expression boundary (`:`, `?`, `,`) and fall back to trailingExpr otherwise. Also a measurement note: the 'before' number quoted from an earlier session (16 of 32) was stale — the branch had been restarted from a main that already carried the previous fix, so the real baseline was 21. Re-measure the baseline in the tree you are actually editing rather than quoting a figure from memory.", "refs": ["mendixlabs/mxcli#1036"]} {"area": "mdl/executor", "date": "2026-09-09", "symptom": "`alter page P { set NoSuchProperty = 10 on dgProducts; }` and `set PageSize = 12 on noSuchWidget;` both passed `mxcli check -p app.mpr --references` (exit 0, \"All references valid\") and were then refused by `exec`: `pluggable property \"NoSuchProperty\" not found` / `widget \"noSuchWidget\" not found`. exec applies statements one at a time, so the script had already written everything before the typo", "cause": "ValidateWidgetProperties resolves the properties of widgets a statement CARRIES \u2014 CREATE PAGE's tree, and ALTER's InsertWidgetOp/ReplaceWidgetOp trees. SetPropertyOp carries no widget: it names one already stored, so its property can only be resolved against the DOCUMENT, which that pass never opens. Same inversion validate_alter_target.go closed for the ALTER's target document, one level further in", "file": "`mdl/executor/validate_alter_set.go` (new), `mdl/backend/pagemutator/probe.go` (new)", "insight": "**Do not re-derive what a mutation accepts \u2014 run the mutation.** The vocabulary of an ALTER SET is partly a switch in `setRawWidgetPropertyMut` and partly the STORED widget's own PropertyTypes, which belong to whatever widget package the project installed; no registry in this repo can state it for an arbitrary project. So the check opens the document, runs the real setter against a throwaway deep copy (`Mutator.Probe`, whose `Save` is refused), and keeps only the error \u2014 check and exec cannot drift because there is one resolver. The author also gets exec's exact wording from the pre-flight. **Two false-positive sources, both measured, both silence rather than a finding**: a page the script CREATEs (nothing stored \u2014 skip, and do not even open it) and a widget an INSERT in the same script adds. The second cannot be a name match: a DataGrid 2 column is inserted as `colBrand` and addressed as `Brand` (derived from the bound attribute), so the rule is `ResolvesTarget` \u2014 suppress only when the document does not carry the target AND the script adds widgets to that document. **Gating on an optional interface assertion, not on backend.PageMutator**, keeps the pass off the MCP mutator, which has no pluggable path and would have reported its own difference as the author's mistake. Controls: 4 casings of a real property pass (a case-sensitive pre-flight would have re-broken #1069); insert-then-set passes check AND exec in both the same-statement and across-statement forms; 17 ALTER SET examples in mdl-examples show 0 new errors vs the baseline binary; the project's files are byte-identical (md5 over .mpr + mprcontents) after five check runs. Revert control: stubbing the pass makes the three gap tests fail", "refs": []} +{"area": "mdl/executor", "date": "2026-09-09", "symptom": "`mxcli widget describe COMBOBOX -p app.mpr` said nothing about 23 properties the widget's editor hides, and its generated MDL example offered two database-only properties (`optionsSourceDatabaseCaptionType`, `optionsSourceDatabaseCustomContentType`) for a combobox with `source: 'context'`, where the editor shows neither. The coverage counter read \"24 of 32 editor hide-rules recognized\" and did NOT move, because those calls were recognized — only their property lists were short", "cause": "`hideTargetKeys` read a hide call's property list by trimming a leading `[` and a trailing `]` off the argument. Combo box builds its biggest lists by concatenating module-level arrays onto the literal one — `hidePropertiesIn(e, t, [\"a\",\"b\"].concat(N))`, where `N` is ten database properties — so the trim never found the `]` (the argument ends in `)`), the top-level split stopped at the first element boundary, and every concatenated name was dropped silently. The call still incremented `stats.Recognized`, so the counter reported full coverage of a list it had read a third of", "file": "`mdl/executor/editorconfig_extract.go` (`stringArrayConsts`, `stringArrayMembers`, `concatMembers`; `hideTargetKeys` takes the consts map)", "insight": "**A coverage counter that counts CALL SITES hides a defect in what each call contributed.** \"Recognized\" meant \"I read the condition\", not \"I read the properties\", and the two came apart at exactly the calls that matter most — the branch-level ones hiding ten properties at a time, where a wrong answer is widest. What made it findable was not the counter but **the user-visible artifact**: the MDL example proposed properties the editor does not show, visible only by reading the example against the widget's own `editorConfig.js`. Measure the artifact, not the metric. **Resolution must withhold, not guess**: an argument that is not an inline string array or a known identifier (`.concat(n(b.static))` — a call on a computed key) makes the whole chain unresolvable and the literal keys stand alone; an identifier bound twice to different arrays is dropped, because the minifier reuses short names across scopes. Each of those three refusals needed its own **stubbed control** to show it was load-bearing — withhold-don't-guess is only real if a test fails when the guard is removed. **Measure lost rules as a SET, not a line diff**: `diff` on the sorted rule block reported five false losses that were re-ordering from the 23 insertions. Measured over the 42 describable widgets (population from the `.mpk` files, NOT the 33 `.def.json` — Combo box has none): 332 → 355 rules, 0 lost, 0 per-widget regressions, and the only example block that changed was Combo box's, losing exactly the two wrong bindings. 14 concat sites exist in the fixture; 6 resolve, 8 stay unresolvable and keep today's literal-only behaviour. **Still open after this**: rules inside the `? (…)` database branch and the `: else` branches keep only their own guard and lose the branch condition, so e.g. `selectedItemsSorting hidden when optionsSourceDatabaseItemSelection ≠ \"Multi\"` over-fires for a context-source combobox", "refs": []} diff --git a/docs-site/src/reference/query/describe-widget.md b/docs-site/src/reference/query/describe-widget.md index 38cf9247f9..e8b4d3984b 100644 --- a/docs-site/src/reference/query/describe-widget.md +++ b/docs-site/src/reference/query/describe-widget.md @@ -103,6 +103,15 @@ widget hides under the configuration the example picked is left out, so what you see is what that configuration actually supports. The footer reports how many of the widget's hide-rules were recognised; an unrecognised rule never prunes. +The count is of **hide-calls whose condition was read**, which is not the same +as every property being accounted for. A widget editor often builds one call's +property list by concatenating shared arrays onto a literal one — Combo box +hides ten database properties that way in a single call. Those are resolved, so +the rules appear; but where the concatenated argument is computed at runtime +(`.concat(n(b.static))`) it cannot be resolved statically, and only the literal +names are reported. Nothing is guessed: an unresolvable list contributes its +literal members and no more. + A rule can carry several conditions, joined with `and`: ``` diff --git a/mdl/executor/editorconfig_extract.go b/mdl/executor/editorconfig_extract.go index f559460d86..5bf4eea6d4 100644 --- a/mdl/executor/editorconfig_extract.go +++ b/mdl/executor/editorconfig_extract.go @@ -138,6 +138,7 @@ func extractVisibilityRulesFromJS(js string) ([]types.WidgetVisibilityRule, edit var stats editorConfigExtractStats var pending []ternaryThenCandidate // resolved after the loop seen := map[string]bool{} // dedupe propertyKey+condition + consts := stringArrayConsts(js) // module-level arrays, for `[…].concat(N)` for _, loc := range hideCallRE.FindAllStringIndex(js, -1) { stats.TotalHideCalls++ @@ -147,7 +148,7 @@ func extractVisibilityRulesFromJS(js string) ([]types.WidgetVisibilityRule, edit stats.SkippedComplex++ continue } - listKey, keys, condKeys, ok := hideTargetKeys(args) + listKey, keys, condKeys, ok := hideTargetKeys(args, consts) if !ok || (len(keys) == 0 && len(condKeys) == 0) { stats.SkippedComplex++ continue @@ -387,7 +388,7 @@ func negate(c types.WidgetVisibilityCondition) (types.WidgetVisibilityCondition, // // ok is false for a shape it does not recognize; the caller counts it as skipped // and emits no rule, which degrades to "not hidden". -func hideTargetKeys(args string) (listKey string, keys []string, condKeys []ternaryKey, ok bool) { +func hideTargetKeys(args string, consts map[string][]string) (listKey string, keys []string, condKeys []ternaryKey, ok bool) { parts := splitTopLevelCommas(args) // Collect string-literal positional args and any array literal. var stringArgs []string @@ -395,7 +396,20 @@ func hideTargetKeys(args string) (listKey string, keys []string, condKeys []tern for _, p := range parts { p = strings.TrimSpace(p) if strings.HasPrefix(p, "[") { - for _, el := range splitTopLevelCommas(strings.TrimSuffix(strings.TrimPrefix(strings.TrimSpace(p), "["), "]")) { + // The argument may be `[…].concat(N, W)`, so read the literal with a + // balanced scan and resolve the concatenated arrays separately — + // trimming a trailing "]" would stop at the first element and drop + // everything after it. + body, bodyOK := balancedArgs(p, 1) + if !bodyOK { + body = strings.TrimSuffix(strings.TrimPrefix(p, "["), "]") + } + if bodyOK { + if extra, extraOK := concatMembers(p[len(body)+2:], consts); extraOK { + arrayKeys = append(arrayKeys, extra...) + } + } + for _, el := range splitTopLevelCommas(body) { if tk, ok := ternaryElement(el); ok { // `cond ? "a" : "b"` as an array ELEMENT. Harvesting its string // literals would invent a key from the comparison value and mark @@ -433,6 +447,119 @@ func hideTargetKeys(args string) (listKey string, keys []string, condKeys []tern } } +// arrayDeclRE finds a module-level array binding — `z=["lazyLoading",…]` in the +// minified bundle. Combo box's biggest hide lists are built by concatenating +// three such arrays onto a literal one, so without resolving them the call is +// counted as recognized while most of its properties are silently dropped. +var arrayDeclRE = regexp.MustCompile(`(?:^|[,;({=\s])([A-Za-z_$][\w$]*)\s*=\s*\[`) + +// concatCallRE matches the `.concat(` that follows a hide call's array argument. +var concatCallRE = regexp.MustCompile(`^\s*\.concat\(`) + +// stringArrayConsts maps each identifier bound to an array of STRING LITERALS to +// its members. An identifier bound more than once is dropped rather than +// guessed at: the minifier reuses short names across scopes, and resolving one +// to the wrong array would invent hide rules for properties the editor shows. +// Arrays holding anything but string literals (an element list, a data URI +// built by a call) are skipped for the same reason. +func stringArrayConsts(js string) map[string][]string { + out := map[string][]string{} + ambiguous := map[string]bool{} + for _, loc := range arrayDeclRE.FindAllStringSubmatchIndex(js, -1) { + name := js[loc[2]:loc[3]] + if ambiguous[name] { + continue + } + body, ok := balancedArgs(js, loc[1]) // loc[1] is just past the '[' + if !ok { + continue + } + members, ok := stringArrayMembers(body) + if !ok { + continue + } + if prev, seen := out[name]; seen && !sameStrings(prev, members) { + delete(out, name) + ambiguous[name] = true + continue + } + out[name] = members + } + return out +} + +// stringArrayMembers reads an array literal's body as a list of string literals. +// ok is false if ANY element is something else — a partial read would drop +// members without saying so, which is the defect this exists to fix. +func stringArrayMembers(body string) ([]string, bool) { + if strings.TrimSpace(body) == "" { + return nil, false + } + var out []string + for _, el := range splitTopLevelCommas(body) { + el = strings.TrimSpace(el) + m := stringLitRE.FindStringSubmatch(el) + if m == nil || !strings.HasPrefix(el, `"`) || len(m[0]) != len(el) { + return nil, false + } + out = append(out, m[1]) + } + return out, true +} + +func sameStrings(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} + +// concatMembers reads the `.concat(a, b)` chain trailing an array literal and +// returns the members its arguments contribute. Each argument must be either an +// inline string array or an identifier in consts; anything else (a call such as +// `.concat(i(a))`) yields ok=false, and the caller keeps the literal keys alone +// rather than emitting a list it knows to be short. +func concatMembers(tail string, consts map[string][]string) ([]string, bool) { + var out []string + for { + m := concatCallRE.FindStringIndex(tail) + if m == nil { + return out, true + } + args, ok := balancedArgs(tail, m[1]) + if !ok { + return out, false + } + for _, a := range splitTopLevelCommas(args) { + a = strings.TrimSpace(a) + switch { + case strings.HasPrefix(a, "["): + body, ok := balancedArgs(a, 1) + if !ok { + return out, false + } + members, ok := stringArrayMembers(body) + if !ok { + return out, false + } + out = append(out, members...) + default: + members, ok := consts[a] + if !ok { + return out, false + } + out = append(out, members...) + } + } + tail = tail[m[1]+len(args)+1:] // past the closing ')', for a chained .concat + } +} + // ternaryKey is an array element of the form `cond ? "a" : "b"` — a hide whose // TARGET depends on a condition, rather than the whole call being guarded. Each // branch becomes its own rule carrying that condition (the then-branch when the diff --git a/mdl/executor/editorconfig_shapes_test.go b/mdl/executor/editorconfig_shapes_test.go index d4884ccb62..23f9bf88f8 100644 --- a/mdl/executor/editorconfig_shapes_test.go +++ b/mdl/executor/editorconfig_shapes_test.go @@ -382,3 +382,112 @@ func TestChainedTernaryBranchConditionIsLifted(t *testing.T) { } } } + +func TestConcatenatedHideListsAreResolved(t *testing.T) { + js, err := mpk.ReadEditorConfig( + "../../testdata/expr-checker/widgets/com.mendix.widget.web.Combobox.mpk", + "com.mendix.widget.web.combobox.Combobox") + if err != nil || js == "" { + t.Fatalf("read Combo box editorConfig: %v", err) + } + rules, _ := extractVisibilityRulesFromJS(js) + + // Combo box builds its biggest hide lists by concatenating module-level + // arrays onto the literal one: + // + // "context" === t.source && hidePropertiesIn(e, t, [ …3 literals… ].concat(N)) + // + // where N is ten database properties. Reading only the literal array counts + // the call as recognized and silently drops the rest, so the describe output + // says nothing about them — and the generated example then offers properties + // the editor does not show. + want := []struct { + prop, guard string + }{ + // …].concat(N) under `"context" === t.source` + {"optionsSourceDatabaseDataSource", "source"}, + {"optionsSourceDatabaseItemSelection", "source"}, + {"onChangeDatabaseEvent", "source"}, + {"optionsSourceDatabaseCaptionType", "source"}, + {"databaseAttributeString", "source"}, + // …].concat(W, z) under `["enumeration","boolean"].includes(optionsSourceType)` + {"attributeAssociation", "optionsSourceType"}, + {"optionsSourceAssociationDataSource", "optionsSourceType"}, + {"lazyLoading", "optionsSourceType"}, + } + for _, w := range want { + var found bool + for _, r := range rules { + if r.PropertyKey != w.prop { + continue + } + for _, c := range r.Conditions() { + if c.PropertyKey == w.guard { + found = true + } + } + } + if !found { + t.Errorf("no rule lifted for %s guarded by %s — concat member dropped", w.prop, w.guard) + } + } +} + +func TestConcatResolverRefusesWhatItCannotRead(t *testing.T) { + cases := []struct { + name string + js string + want []string // property keys that MUST be lifted + reject []string // keys that must NOT appear — inventing one is worse than missing it + }{{ + name: "inline array literal", + js: `f=function(t,e){"a"===t.mode&&hidePropertiesIn(e,t,["p"].concat(["q","r"]))}`, + want: []string{"p", "q", "r"}, + }, { + name: "chained concat of two consts", + js: `A=["q"],B=["r"];f=function(t,e){"a"===t.mode&&hidePropertiesIn(e,t,["p"].concat(A).concat(B))}`, + want: []string{"p", "q", "r"}, + }, { + // `.concat(n(x))` — a call on a computed key. Resolving it would mean + // evaluating the widget's own code, so the literal keys stand alone and + // nothing is invented. Eight such sites exist across the fixture. + name: "computed concat argument", + js: `A=["q"];f=function(t,e){"a"===t.mode&&hidePropertiesIn(e,t,["p"].concat(n(A[t.kind])))}`, + want: []string{"p"}, + reject: []string{"q"}, + }, { + // The minifier reuses short names across scopes. A name bound to two + // different arrays is dropped rather than guessed at — picking either + // would hide properties the editor shows. + name: "identifier bound twice to different arrays", + js: `A=["q"];g=function(){A=["zzz"]};f=function(t,e){"a"===t.mode&&hidePropertiesIn(e,t,["p"].concat(A))}`, + want: []string{"p"}, + reject: []string{"q", "zzz"}, + }, { + // Not every array is a property list. + name: "array of non-strings", + js: `A=[1,2];f=function(t,e){"a"===t.mode&&hidePropertiesIn(e,t,["p"].concat(A))}`, + want: []string{"p"}, + reject: []string{"1"}, + }} + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + rules, _ := extractVisibilityRulesFromJS(tc.js) + got := map[string]bool{} + for _, r := range rules { + got[r.PropertyKey] = true + } + for _, w := range tc.want { + if !got[w] { + t.Errorf("missing rule for %q (got %v)", w, got) + } + } + for _, b := range tc.reject { + if got[b] { + t.Errorf("invented a rule for %q — the resolver must withhold, not guess (got %v)", b, got) + } + } + }) + } +} From d6148072fd277bb40d02b9ea7e2ae11708e86d4e Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 16:53:32 +0000 Subject: [PATCH 02/10] fix(check): report a container-typed widget property written as a value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mendixlabs/mxcli#1056. `customAllSelected: []` on a pluggable widget passed `mxcli check -p` with zero diagnostics, exec printed "Created page", the stored page carried 0 CustomWidgets$WidgetProperty entries, and mxbuild then reported 3x CE0642 "Property 'All selected' is required." #999 built `[(k: v)]` into its own AST type so MDL-WIDGET27 could report it, but keyed the rule on that type — which the visitor only produces when the brackets contain a parenthesised entry. An empty `[]` falls through to the generic `[expr, …]` branch and becomes an empty []string no writer claims: #999's silent drop reached by a different spelling. A scalar never looked like a list at all. The rule now covers all three, with deliberately different gating: p: [(k: v)] shape error (unchanged) p: [] shape error — writes nothing under every current writer, so it needs no project, which matters because check-mdl runs .fail.mdl files without one p: 'x' type error ONLY when the widget resolves — without a definition this is the ordinary property form and flagging it would be a guess Keyed on EMPTINESS, never on the brackets: `visible: [$x != '']`, `editable: [true]` and a filter's `attributes: [Name]` are how MDL spells those properties, and a guard test pins that they stay silent. A child slot and an object list spell their remedy differently — `kw name { … }` against `kw name (…)` — so containerKeyword reports which, or the message would print an example that does not parse. The printed remedy is verified end to end: pasted verbatim it checks, execs, and adds 0 errors to an mx check (11.6.6) where the page written before this rule existed still contributes its 3 CE0642 — the control pair in one build. The reporter only reached the wrong spelling because the right one was rejected; that half was already fixed by bca5466e, four days after their build. Confirmed by building their exact commit in a worktree: it reproduces their error verbatim and HEAD parses, execs and writes the same five WidgetProperty entries Studio Pro does. A guard test pins that the slot form `widget docs` emits parses, so the two halves cannot regress independently — a parser that rejects the right spelling would turn this error into a dead end. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KTdvGwVewkdgNQGDCxpQPZ --- .../fix-issue/findings/mdl-executor.jsonl | 1 + .claude/skills/mendix/custom-widgets/SKILL.md | 23 ++++ ...056-widget-slot-as-property-value.fail.mdl | 42 ++++++ .../validate_widget_object_property.go | 127 +++++++++++++++--- .../validate_widget_object_property_test.go | 102 ++++++++++++++ 5 files changed, 278 insertions(+), 17 deletions(-) create mode 100644 mdl-examples/bug-tests/1056-widget-slot-as-property-value.fail.mdl diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index 9f359be044..4d0a40ec61 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -573,3 +573,4 @@ {"area": "mdl/executor", "date": "2026-09-09", "symptom": "Six of Combo box's editorConfig hide-rules could not be lifted with their full condition, and its coverage counter sat at 21 of 32. The rules sit inside `\"association\"===t.optionsSourceType && ( … )`, itself the ELSE branch of a ternary inside `\"context\"===t.source ? ( … )` — a chained ternary, where each branch's BODY is parenthesised but each branch's CONDITION is not.", "cause": "groupGuard read the condition of a group opened after `&&`/`||` with trailingExpr, which stops at a STATEMENT separator. A chained ternary contains none, so it returned the whole `A ? (…) : B` expression as the group's condition. That is not a comparison, guardToCondition refused it, and enclosingGroupConditions reported the chain unreadable.", "file": "mdl/executor/editorconfig_extract.go (groupGuard, operandBefore)", "insight": "The characterisation written into the PR body — 'ternary chains without parentheses, which the outward walk does not traverse' — was WRONG, and instrumenting the walk rather than re-reading it is what showed so: the walk reaches these groups fine; the failure is guard EXTRACTION at the group, one function away. A one-line ceiling written from reasoning is worth re-deriving before anyone builds on it. The fix is not a straight swap to lastGuardExpr, which bounds at `{` and hands back a fragment with an unbalanced `}` where the expression follows a block (ProgressCircle's ternary follows a whole switch); take lastGuardExpr's answer only when it stopped at an INSIDE-expression boundary (`:`, `?`, `,`) and fall back to trailingExpr otherwise. Also a measurement note: the 'before' number quoted from an earlier session (16 of 32) was stale — the branch had been restarted from a main that already carried the previous fix, so the real baseline was 21. Re-measure the baseline in the tree you are actually editing rather than quoting a figure from memory.", "refs": ["mendixlabs/mxcli#1036"]} {"area": "mdl/executor", "date": "2026-09-09", "symptom": "`alter page P { set NoSuchProperty = 10 on dgProducts; }` and `set PageSize = 12 on noSuchWidget;` both passed `mxcli check -p app.mpr --references` (exit 0, \"All references valid\") and were then refused by `exec`: `pluggable property \"NoSuchProperty\" not found` / `widget \"noSuchWidget\" not found`. exec applies statements one at a time, so the script had already written everything before the typo", "cause": "ValidateWidgetProperties resolves the properties of widgets a statement CARRIES \u2014 CREATE PAGE's tree, and ALTER's InsertWidgetOp/ReplaceWidgetOp trees. SetPropertyOp carries no widget: it names one already stored, so its property can only be resolved against the DOCUMENT, which that pass never opens. Same inversion validate_alter_target.go closed for the ALTER's target document, one level further in", "file": "`mdl/executor/validate_alter_set.go` (new), `mdl/backend/pagemutator/probe.go` (new)", "insight": "**Do not re-derive what a mutation accepts \u2014 run the mutation.** The vocabulary of an ALTER SET is partly a switch in `setRawWidgetPropertyMut` and partly the STORED widget's own PropertyTypes, which belong to whatever widget package the project installed; no registry in this repo can state it for an arbitrary project. So the check opens the document, runs the real setter against a throwaway deep copy (`Mutator.Probe`, whose `Save` is refused), and keeps only the error \u2014 check and exec cannot drift because there is one resolver. The author also gets exec's exact wording from the pre-flight. **Two false-positive sources, both measured, both silence rather than a finding**: a page the script CREATEs (nothing stored \u2014 skip, and do not even open it) and a widget an INSERT in the same script adds. The second cannot be a name match: a DataGrid 2 column is inserted as `colBrand` and addressed as `Brand` (derived from the bound attribute), so the rule is `ResolvesTarget` \u2014 suppress only when the document does not carry the target AND the script adds widgets to that document. **Gating on an optional interface assertion, not on backend.PageMutator**, keeps the pass off the MCP mutator, which has no pluggable path and would have reported its own difference as the author's mistake. Controls: 4 casings of a real property pass (a case-sensitive pre-flight would have re-broken #1069); insert-then-set passes check AND exec in both the same-statement and across-statement forms; 17 ALTER SET examples in mdl-examples show 0 new errors vs the baseline binary; the project's files are byte-identical (md5 over .mpr + mprcontents) after five check runs. Revert control: stubbing the pass makes the three gap tests fail", "refs": []} {"area": "mdl/executor", "date": "2026-09-09", "symptom": "`mxcli widget describe COMBOBOX -p app.mpr` said nothing about 23 properties the widget's editor hides, and its generated MDL example offered two database-only properties (`optionsSourceDatabaseCaptionType`, `optionsSourceDatabaseCustomContentType`) for a combobox with `source: 'context'`, where the editor shows neither. The coverage counter read \"24 of 32 editor hide-rules recognized\" and did NOT move, because those calls were recognized — only their property lists were short", "cause": "`hideTargetKeys` read a hide call's property list by trimming a leading `[` and a trailing `]` off the argument. Combo box builds its biggest lists by concatenating module-level arrays onto the literal one — `hidePropertiesIn(e, t, [\"a\",\"b\"].concat(N))`, where `N` is ten database properties — so the trim never found the `]` (the argument ends in `)`), the top-level split stopped at the first element boundary, and every concatenated name was dropped silently. The call still incremented `stats.Recognized`, so the counter reported full coverage of a list it had read a third of", "file": "`mdl/executor/editorconfig_extract.go` (`stringArrayConsts`, `stringArrayMembers`, `concatMembers`; `hideTargetKeys` takes the consts map)", "insight": "**A coverage counter that counts CALL SITES hides a defect in what each call contributed.** \"Recognized\" meant \"I read the condition\", not \"I read the properties\", and the two came apart at exactly the calls that matter most — the branch-level ones hiding ten properties at a time, where a wrong answer is widest. What made it findable was not the counter but **the user-visible artifact**: the MDL example proposed properties the editor does not show, visible only by reading the example against the widget's own `editorConfig.js`. Measure the artifact, not the metric. **Resolution must withhold, not guess**: an argument that is not an inline string array or a known identifier (`.concat(n(b.static))` — a call on a computed key) makes the whole chain unresolvable and the literal keys stand alone; an identifier bound twice to different arrays is dropped, because the minifier reuses short names across scopes. Each of those three refusals needed its own **stubbed control** to show it was load-bearing — withhold-don't-guess is only real if a test fails when the guard is removed. **Measure lost rules as a SET, not a line diff**: `diff` on the sorted rule block reported five false losses that were re-ordering from the 23 insertions. Measured over the 42 describable widgets (population from the `.mpk` files, NOT the 33 `.def.json` — Combo box has none): 332 → 355 rules, 0 lost, 0 per-widget regressions, and the only example block that changed was Combo box's, losing exactly the two wrong bindings. 14 concat sites exist in the fixture; 6 resolve, 8 stay unresolvable and keep today's literal-only behaviour. **Still open after this**: rules inside the `? (…)` database branch and the `: else` branches keep only their own guard and lose the branch condition, so e.g. `selectedItemsSorting hidden when optionsSourceDatabaseItemSelection ≠ \"Multi\"` over-fires for a context-source combobox", "refs": []} +{"area": "mdl/executor", "date": "2026-09-13", "symptom": "`customAllSelected: []` on a pluggable widget passed `mxcli check -p` with ZERO diagnostics, exec printed \"Created page\", and the stored page carried 0 `CustomWidgets$WidgetProperty` entries — then mxbuild reported 3x CE0642 \"Property 'All selected' is required.\" The same hole swallowed `attributes: []` on an object list and any scalar written for a container-typed property (mendixlabs/mxcli#1056)", "cause": "#999 built `[(k: v)]` into its own AST type so MDL-WIDGET27 could report it, but keyed the rule on that type — which the visitor only produces when the brackets contain a parenthesised entry. An EMPTY `[]` falls through to the generic `[expr, …]` branch and becomes an empty []string that no writer claims, which is #999's silent drop reached by a different spelling. A scalar never looked like a list at all, so nothing examined it", "file": "`mdl/executor/validate_widget_object_property.go` (`isEmptyListValue`, `isDeclaredContainer`, `containerKeyword` resolving child slots as well as object lists)", "insight": "**The reporter only reached the wrong spelling because the right one was rejected.** #1056 reads as one bug and is two: the slot syntax their own generated docs showed did not parse (fixed four days after their build by bca5466e, verified by building their exact commit 89824921 in a worktree — it reproduces their error verbatim and HEAD passes), and the wrong spelling stayed silent. Fixing only the parser would have left every author who had already worked around it with a page that builds broken. **Key on emptiness, never on the brackets**: `visible: [$x != '']`, `editable: [true]` and a filter's `attributes: [Name]` are all how MDL spells those properties, so a rule keyed on `[` would have broken the corpus — the guard test asserts those stay silent. **Gate the scalar case on the definition and the empty case on shape**: `p: 'x'` is the ordinary property form, so calling it wrong without knowing the property is container-typed would be a guess, while `p: []` writes nothing under every current writer and needs no project (which matters because `make check-mdl` runs without one). **A child slot and an object list spell their remedy differently** — `kw name { … }` vs `kw name (…)` — so one message for both would print an example that does not parse; that is why containerKeyword returns isSlot. Verified the printed remedy end-to-end: pasting it verbatim checks, execs, and adds 0 errors to a build where the pre-fix page still contributes its 3 CE0642 — the control pair in one mxbuild run. **Measurement trap hit again**: the first corpus sweep piped `2>/dev/null` while mxcli prints diagnostics to stderr, so it scored 0 of 547 on BOTH sides; the before-side positive control (the #999 file must fire) is what caught it", "refs": []} diff --git a/.claude/skills/mendix/custom-widgets/SKILL.md b/.claude/skills/mendix/custom-widgets/SKILL.md index 95accdd086..675bdf3500 100644 --- a/.claude/skills/mendix/custom-widgets/SKILL.md +++ b/.claude/skills/mendix/custom-widgets/SKILL.md @@ -71,6 +71,29 @@ vanished from storage, while the multi-key shape died as `missing ')' at ','`. The error now names the container keyword and rewrites your entry into the form that works. +The same rule covers the two spellings that carry no entry to key on +(`mendixlabs/mxcli#1056`): + +```sql +selectionhelper sh (renderStyle: 'custom', customAllSelected: []) -- MDL-WIDGET27 +selectionhelper sh (renderStyle: 'custom', customAllSelected: 'something') -- MDL-WIDGET27 +``` + +A **widgets**-typed property such as `customAllSelected` holds child widgets, so +it is written as a block with widgets in it rather than entries: + +```sql +selectionhelper sh (renderStyle: 'custom') { + customallselected s1 { dynamictext d1 (Content: 'All') } +} +``` + +The empty form is reported from its shape, with no project needed. The scalar +form is reported only when the widget resolves, because without a definition +`p: 'x'` is the ordinary property form and flagging it would be a guess. Both +matter because a required slot left empty is not a silent no-op at build time — +it is `CE0642 "Property '…' is required."`, one per slot. + `describe widget -p ` lists a widget's container keywords under **Body containers**. diff --git a/mdl-examples/bug-tests/1056-widget-slot-as-property-value.fail.mdl b/mdl-examples/bug-tests/1056-widget-slot-as-property-value.fail.mdl new file mode 100644 index 0000000000..0f9740c477 --- /dev/null +++ b/mdl-examples/bug-tests/1056-widget-slot-as-property-value.fail.mdl @@ -0,0 +1,42 @@ +-- mendixlabs/mxcli#1056 — a container-typed widget property written as a VALUE. +-- +-- This file is expected to FAIL `mxcli check` (.fail.mdl). Every statement below +-- is a spelling that used to pass check, exec successfully, and write nothing. +-- +-- The reporter arrived here because the slot syntax their own generated widget +-- docs showed was rejected by the parser. That half is fixed — the grammar takes +-- generic container names since bca5466e, so `customallselected s1 { … }` now +-- parses, execs, and produces the same five CustomWidgets$WidgetProperty entries +-- Studio Pro writes. What remained was that the WRONG spelling stayed silent: +-- +-- customAllSelected: [] -> check clean, exec "Created page", 0 properties +-- written, then 3x CE0642 at build time because +-- the slot is required. +-- +-- #999 fixed the `[(k: v)]` shape. It keyed on there being at least one +-- parenthesised entry, so the empty and scalar forms fell through the same hole. +-- +-- Expected: MDL-WIDGET27 (error) on each of the three widgets below. + +-- 1. Empty list on a widgets-typed CHILD SLOT — the reporter's exact form. +create or replace page BugTest1056.EmptySlot (Title: 'Empty slot', Layout: Atlas_Core.Atlas_Default) { + pluggablewidget 'com.mendix.widget.web.selectionhelper.SelectionHelper' sh1 ( + renderStyle: 'custom', + customAllSelected: [] + ) +} + +-- 2. Empty list on an OBJECT LIST. Same hole, reached from the #999 side. +create or replace page BugTest1056.EmptyList (Title: 'Empty list', Layout: Atlas_Core.Atlas_Default) { + htmlelement frame1 (tagName: 'div', attributes: []) +} + +-- 3. A scalar on a child slot. No value in a property list can reach a property +-- that holds child widgets; this one is only knowable from the definition, so it +-- is reported when the widget resolves and stays silent without a project. +create or replace page BugTest1056.ScalarSlot (Title: 'Scalar slot', Layout: Atlas_Core.Atlas_Default) { + pluggablewidget 'com.mendix.widget.web.selectionhelper.SelectionHelper' sh2 ( + renderStyle: 'custom', + customAllSelected: 'something' + ) +} diff --git a/mdl/executor/validate_widget_object_property.go b/mdl/executor/validate_widget_object_property.go index c5db5ed657..c4c3e5ada5 100644 --- a/mdl/executor/validate_widget_object_property.go +++ b/mdl/executor/validate_widget_object_property.go @@ -74,23 +74,97 @@ func validateObjectEntryProperties(w *ast.WidgetV3, registry *WidgetRegistry, lo var out []linter.Violation for _, key := range keys { - entries, ok := w.Properties[key].(*ast.ObjectEntryListV3) - if !ok || entries == nil { - continue + value := w.Properties[key] + // One violation per property, so the three shapes below are exclusive: + // an empty list on a declared container matches two of them. + switch { + case isObjectEntryList(value): + entries, _ := value.(*ast.ObjectEntryListV3) + out = append(out, linter.Violation{ + RuleID: "MDL-WIDGET27", + Severity: linter.SeverityError, + Message: fmt.Sprintf( + "%s: widget `%s` property `%s` is a repeated entry written as a property value — "+ + "MDL writes these as %s in the widget body, and a value here is discarded on write", + locationPrefix, w.Name, key, objectEntryRemedy(w, registry, key)), + Suggestion: objectEntryExample(w, registry, key, entries), + }) + case isEmptyListValue(value): + // `p: []`. The generic `[expr, …]` branch of the visitor turns this + // into an empty []string that no writer claims, so it is #999's + // silent drop reached by a different spelling (mendixlabs/mxcli#1056). + // Keyed on EMPTINESS, never on the brackets: `visible: [expr]` and a + // filter's `attributes: [Name]` are how MDL spells those properties. + out = append(out, linter.Violation{ + RuleID: "MDL-WIDGET27", + Severity: linter.SeverityError, + Message: fmt.Sprintf( + "%s: widget `%s` property `%s` is an empty list value, which writes nothing — "+ + "%s", + locationPrefix, w.Name, key, emptyListRemedy(w, registry, key)), + Suggestion: objectEntryExample(w, registry, key, nil), + }) + case isDeclaredContainer(w, registry, key): + // The definition says this property holds repeated entries or child + // widgets, so NO value in the property list can reach storage. Only + // knowable with a project; without one `p: 'x'` is the ordinary + // property form and flagging it would be a guess. + out = append(out, linter.Violation{ + RuleID: "MDL-WIDGET27", + Severity: linter.SeverityError, + Message: fmt.Sprintf( + "%s: widget `%s` property `%s` holds %s, not a value — "+ + "a value here is discarded on write", + locationPrefix, w.Name, key, containerNoun(w, registry, key)), + Suggestion: objectEntryExample(w, registry, key, nil), + }) } - out = append(out, linter.Violation{ - RuleID: "MDL-WIDGET27", - Severity: linter.SeverityError, - Message: fmt.Sprintf( - "%s: widget `%s` property `%s` is a repeated entry written as a property value — "+ - "MDL writes these as %s in the widget body, and a value here is discarded on write", - locationPrefix, w.Name, key, objectEntryRemedy(w, registry, key)), - Suggestion: objectEntryExample(w, registry, key, entries), - }) } return out } +func isObjectEntryList(v any) bool { + entries, ok := v.(*ast.ObjectEntryListV3) + return ok && entries != nil +} + +// isEmptyListValue reports `p: []`. The visitor renders a bracketed value with +// no elements as an empty []string — the one shape that can produce one. +func isEmptyListValue(v any) bool { + items, ok := v.([]string) + return ok && len(items) == 0 +} + +// isDeclaredContainer reports whether the widget's definition declares this +// property as an object list or a child slot. False without a definition, which +// is what keeps the scalar case project-gated. +func isDeclaredContainer(w *ast.WidgetV3, registry *WidgetRegistry, propertyKey string) bool { + kw, _ := containerKeyword(w, registry, propertyKey) + return kw != "" +} + +// containerNoun describes what the property holds, for the message. +func containerNoun(w *ast.WidgetV3, registry *WidgetRegistry, propertyKey string) string { + if _, slot := containerKeyword(w, registry, propertyKey); slot { + return "child widgets" + } + return "repeated entries" +} + +// emptyListRemedy names the container when the definition declares one, and +// otherwise says only what is certain: the value does not reach storage. +func emptyListRemedy(w *ast.WidgetV3, registry *WidgetRegistry, propertyKey string) string { + kw, slot := containerKeyword(w, registry, propertyKey) + switch { + case kw != "" && slot: + return fmt.Sprintf("`%s` holds child widgets, written as `%s { … }` in the widget body", propertyKey, kw) + case kw != "": + return fmt.Sprintf("`%s` holds repeated entries, written as `%s (…)` in the widget body", propertyKey, kw) + } + return "remove it, or — if this property takes repeated entries or child widgets — " + + "write them as blocks in the widget body" +} + // objectEntryRemedy names the container keyword when the widget's definition // declares one, and describes the shape when it does not — with no project there // is no definition to consult, and naming a keyword that might be wrong is worse @@ -105,28 +179,47 @@ func objectEntryRemedy(w *ast.WidgetV3, registry *WidgetRegistry, propertyKey st // objectEntryKeyword resolves the property key to the MDL container keyword the // widget declares for it, or "" when it cannot be known. func objectEntryKeyword(w *ast.WidgetV3, registry *WidgetRegistry, propertyKey string) string { + kw, _ := containerKeyword(w, registry, propertyKey) + return kw +} + +// containerKeyword resolves a property key to the MDL container keyword the +// widget's definition declares for it, and reports whether that container is a +// CHILD SLOT (holds widgets, written as `kw name { … }`) rather than an object +// list (holds entries, written as `kw name (…)`). The two spell their remedy +// differently, so a rule that conflated them would print an example that does +// not parse. Returns "" when there is no definition to consult. +func containerKeyword(w *ast.WidgetV3, registry *WidgetRegistry, propertyKey string) (keyword string, isSlot bool) { def := lookupWidgetDef(w, registry) if def == nil { - return "" + return "", false } for _, ol := range def.ObjectLists { if strings.EqualFold(ol.PropertyKey, propertyKey) { - return strings.ToLower(ol.MDLContainer) + return strings.ToLower(ol.MDLContainer), false + } + } + for _, cs := range def.ChildSlots { + if strings.EqualFold(cs.PropertyKey, propertyKey) { + return strings.ToLower(cs.MDLContainer), true } } - return "" + return "", false } // objectEntryExample rewrites what the author wrote into the form that works, so // the fix is a copy rather than a translation exercise. Falls back to naming the // discovery command when the keyword is unknown. func objectEntryExample(w *ast.WidgetV3, registry *WidgetRegistry, propertyKey string, entries *ast.ObjectEntryListV3) string { - kw := objectEntryKeyword(w, registry, propertyKey) + kw, slot := containerKeyword(w, registry, propertyKey) if kw == "" { return "move the entries into the widget body as container blocks; " + "`mxcli widget describe -p ` lists the container keywords" } - if len(entries.Entries) == 0 { + if slot { + return fmt.Sprintf("write `%s %s1 { … }` inside the widget body, holding the child widgets", kw, kw) + } + if entries == nil || len(entries.Entries) == 0 { return fmt.Sprintf("write `%s %s1 (…)` inside the widget body", kw, kw) } var parts []string diff --git a/mdl/executor/validate_widget_object_property_test.go b/mdl/executor/validate_widget_object_property_test.go index c82db220bb..d0d5e5956e 100644 --- a/mdl/executor/validate_widget_object_property_test.go +++ b/mdl/executor/validate_widget_object_property_test.go @@ -206,3 +206,105 @@ func TestOrdinaryArrayKeepsItsAstShape(t *testing.T) { w2.Properties["DesignProperties"]) } } + +// mendixlabs/mxcli#1056. #999's fix keyed on the `[(k: v)]` shape, so it needed +// at least one parenthesised entry. The reporter of #1056 reached for the EMPTY +// form instead — `customAllSelected: []` — after the slot syntax their own +// generated docs showed was rejected by the parser (that half is fixed; the +// grammar took generic container names in bca5466e). The empty form fell +// through to the generic `[expr, …]` branch, became a nil []string no writer +// claims, and reproduced #999 exactly: check clean, exec successful, nothing +// written — then 3x CE0642 at build time because the slots are required. +func TestEmptyListProperty_IsReported(t *testing.T) { + got := widget27(t, strings.Replace(page27, "%s", `attributes: []`, 1)) + if len(got) != 1 { + t.Fatalf("got %d MDL-WIDGET27 for `attributes: []`, want 1: %+v", len(got), got) + } + if got[0].Severity != linter.SeverityError { + t.Errorf("severity = %v, want error — a warning lets exec write the page "+ + "with the property discarded, which is the whole defect", got[0].Severity) + } + if !strings.Contains(got[0].Suggestion, "attribute") { + t.Errorf("suggestion must name the container keyword, got %q", got[0].Suggestion) + } +} + +// A widgets-typed slot is the same mistake wearing a different type: the +// property holds CHILD WIDGETS, so no value written in the property list can +// ever reach storage. This is the exact property from #1056. +func TestEmptyListOnWidgetSlot_IsReported(t *testing.T) { + src := `create page M.P (Title: 'x', Layout: Atlas_Core.Atlas_Default) { + pluggablewidget 'com.mendix.widget.web.selectionhelper.SelectionHelper' sh ( + renderStyle: 'custom', customAllSelected: [] + ) +}` + got := widget27(t, src) + if len(got) != 1 { + t.Fatalf("got %d MDL-WIDGET27 for `customAllSelected: []`, want 1: %+v", len(got), got) + } + if !strings.Contains(got[0].Suggestion, "customallselected") { + t.Errorf("suggestion must name the slot's container keyword, got %q", got[0].Suggestion) + } +} + +// A scalar cannot reach a container-typed property either, and this one is only +// knowable from the definition — which is why it is reported ONLY when the +// widget resolves. Without that, `p: 'x'` is the ordinary property form and +// flagging it would be a guess. +func TestScalarOnContainerProperty_IsReported(t *testing.T) { + src := `create page M.P (Title: 'x', Layout: Atlas_Core.Atlas_Default) { + pluggablewidget 'com.mendix.widget.web.selectionhelper.SelectionHelper' sh ( + renderStyle: 'custom', customAllSelected: 'something' + ) +}` + got := widget27(t, src) + if len(got) != 1 { + t.Fatalf("got %d MDL-WIDGET27 for `customAllSelected: 'something'`, want 1: %+v", len(got), got) + } +} + +// The bracket form is how MDL writes a conditional expression and a filter's +// attribute list. Reporting those would break the corpus, so the rule must key +// on EMPTINESS, never on the brackets. +func TestNonEmptyBracketValuesAreNotReported(t *testing.T) { + for _, prop := range []string{ + `visible: [$currentObject/Name != '']`, + `editable: [true]`, + } { + if got := widget27(t, strings.Replace(page27, "%s", prop, 1)); len(got) != 0 { + t.Errorf("%s: got %d MDL-WIDGET27, want 0: %+v", prop, len(got), got) + } + } + filter := `create page M.P (Title: 'x', Layout: Atlas_Core.Atlas_Default) { + datagrid2 dg (DataSource: database, Entity: MyFirstModule.Expense) { + column c1 (Attribute: Name) { textfilter tf (attributes: [Name]) } + } +}` + if got := widget27(t, filter); len(got) != 0 { + t.Errorf("filter attribute list: got %d MDL-WIDGET27, want 0: %+v", len(got), got) + } +} + +// The other half of mendixlabs/mxcli#1056: the slot syntax the widget docs +// generate must PARSE. It did not on the reporter's build (`mismatched input +// 'customallselected' expecting '}'`), which is why they reached for +// `customAllSelected: []` at all. bca5466e made generic container names parse; +// this pins it, so the two halves cannot regress independently — a parser that +// rejects the right spelling turns MDL-WIDGET27 into a dead end. +func TestDocumentedSlotSyntaxParses(t *testing.T) { + src := `create page M.P (Title: 'x', Layout: Atlas_Core.Atlas_Default) { + pluggablewidget 'com.mendix.widget.web.selectionhelper.SelectionHelper' sh ( + renderStyle: 'custom' + ) { + customallselected s1 { dynamictext d1 (Content: 'All') } + customsomeselected s2 { dynamictext d2 (Content: 'Some') } + customnoneselected s3 { dynamictext d3 (Content: 'None') } + } +}` + if _, errs := visitor.Build(src); len(errs) > 0 { + t.Fatalf("the slot form `mxcli widget docs` emits must parse, got: %v", errs) + } + if got := widget27(t, src); len(got) != 0 { + t.Errorf("the correct form must not be reported, got %d: %+v", len(got), got) + } +} From f0488ced5e763dcba97f049e10006592e78fd041 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 17:38:02 +0000 Subject: [PATCH 03/10] test(widget): derive fixture widget definitions instead of reading gitignored ones MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The three MDL-WIDGET27 tests added in d6148072 passed locally and failed in CI on the same commit: two got the fallback remedy instead of the container keyword, the third found 0 violations. They resolved the widget through LoadWidgetRegistry(fixtureProject(t)), which reads .def.json files from testdata/expr-checker/.mxcli/widgets/. That directory is GITIGNORED — the definitions are derived, not tracked — so they exist for any developer who has run `mxcli widget docs` against the fixture (I generated them earlier in the same session, investigating the issue) and never exist on the runner. With no definition, containerKeyword returns "" and the two definition-dependent branches degrade exactly as designed. fixtureProjectWithDefs copies the fixture to a temp dir, removes any .mxcli the developer's tree carries, and derives the definitions from the tracked .mpk files with RefreshWidgetDefinitions. Local and CI now see identical inputs, and the generated files stay out of the fixture other tests copy. Also tightens the #999 assertion, which is why the suite did not catch this: strings.Contains(msg, "attribute") on a widget whose property is named `attributes` is satisfied by the property name alone, so it passed with no definition loaded. It now asserts the remedy shape, which only a resolved definition produces. Reproduced by moving .mxcli aside — same three failures, same messages. Control: with the derivation stubbed, all FOUR fail, where before only the three new ones did. Restored, full suite green. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KTdvGwVewkdgNQGDCxpQPZ --- .../fix-issue/findings/mdl-executor.jsonl | 1 + .../validate_widget_object_property_test.go | 40 +++++++++++++++++-- 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index d80838e06f..f8fb070d65 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -606,3 +606,4 @@ {"area": "mdl/executor", "date": "2026-09-13", "symptom": "A boundary event path written from MDL fails the build with CE0105 'Call microflow cannot be the last object of a flow, it should end with a jump or end activity' (interrupting), or builds at 0 errors and the runtime refuses to start: 'Expected the flow to end with an end event' (non-interrupting). After the fix, `create or modify` of the same workflow was refused by the dropped-construct guard", "cause": "Mendix ends every boundary event path with `EndOfBoundaryEventPathActivity`; mxcli never wrote one, and MDL has no end statement. The rewrite guard then counted `$Type`s containing 'BoundaryEvent', which includes the marker, so a stored workflow with one event counted two", "file": "`sdk/workflows/workflow.go` (`EndBoundaryEventPath`), `mdl/executor/cmd_workflows_write.go` (`buildBoundaryEvents`), `mdl/backend/wfmutator/mutator.go` + `mdl/backend/mcp/workflow.go` (`InsertBoundaryEvent`), `mdl/executor/validate_workflow_rewrite.go` (`countRawBoundaryEvents`)", "insight": "**Same class as the parallel-split marker: a Studio Pro terminal marker that no MDL statement spells.** Grep the writers for every `EndOf*` type the reader knows before assuming one is the only one. The guard miscount was only visible by re-running the fixed script against the fixed workflow \u2014 a substring match on `$Type` is wrong the moment a sibling type shares the stem; match the suffix, and keep a control asserting the substring count so the test says why", "fix": "Append the marker unless the path ends in a jump or end-of-workflow (CE6692 otherwise); count boundary events by `strings.HasSuffix($Type, \"BoundaryEvent\")`"} {"area": "mdl/executor", "date": "2026-09-13", "symptom": "A workflow with `boundary event timer '\u2026'` (no interrupting / non interrupting) passes check and builds at 0 errors; the runtime then fails to start: `Class 'Workflows$TimerBoundaryEvent' could not be found`", "cause": "The bare form maps to `Workflows$TimerBoundaryEvent`, which exists in no cached 11.x runtime (only Interrupting/NonInterruptingTimerBoundaryEvent). mxbuild tolerates the unknown type. It was the documented syntax example", "file": "`mdl/executor/validate_workflow_refs.go` (`bareTimerBoundaryEventErrors`, MDL-WF07), `cmd/mxcli/syntax/features_workflow.go`", "insight": "**A type mxbuild accepts is not a type the runtime has.** Found only because a verification boot of an unrelated fix loaded it. When a grammar has a default branch that maps to a storage type, check that type against the runtime's class list, not against `mx check`", "fix": "Refuse the bare form on 11+ at check and exec, CREATE and every ALTER op that can carry a boundary event; update syntax help, skill table and the ako/mxcli#415 bug-test script to name the kind"} {"area": "mdl/executor", "date": "2026-09-13", "symptom": "A view entity whose association column is also declared as an attribute (`MeterRef: Trends.Meter` or `MeterRef: Trends.Meter.ID` beside `select m.ID as MeterRef`) passes `mxcli check`; `check -p` says 'OQL select has 1 columns but 2 attributes declared'; exec writes `Enumeration(Trends.Meter)` and mx check reports CE1613, or throws 'An error occurred when trying to set the Enumeration property' for the three-part form", "cause": "A bare qualified name parses as TypeEnumeration (the entity/enum ambiguity), and execCreateViewEntity converted it with convertDataType without asking what it names. The alias-to-attribute alignment skips association columns, so the declared attribute had no column and was compared against the next one", "file": "`mdl/executor/oql_view_associations.go` (`ValidateViewAttributeDeclarations` MDL080, `viewAttributeEntityTypeErrors`), `mdl/executor/cmd_entities.go` (`execCreateViewEntity`), `mdl/executor/validate.go`, `mdl/executor/validate_program.go`, `cmd/mxcli/lsp_diagnostics.go`", "insight": "**The TypeEnumeration/TypeEntity ambiguity has a consumer wherever a data type becomes a stored type, and view entity attributes were one nobody had listed.** Split the refusal by what it needs: an association column's alias and a three-part name are decidable from the script, so they belong in the no-project phase that exec's pre-check also runs; entity-vs-enum needs the project, so it goes in check -p AND the handler, because exec --no-check skips both phases. Verify the handler refusal by counting changed files, not by the error text", "fix": "Refuse in ValidateProgram/LSP (MDL080) and at the top of execCreateViewEntity before any backend call; report an attribute once"} +{"area": "mdl/executor", "date": "2026-09-13", "symptom": "Three new MDL-WIDGET27 tests passed locally and failed in CI on the same commit: two reported the fallback remedy (\"move the entries into the widget body as container blocks\") instead of naming the container keyword, and the third found 0 violations where it wanted 1", "cause": "The tests resolved the widget through `LoadWidgetRegistry(fixtureProject(t))`, which reads `.def.json` files from `testdata/expr-checker/.mxcli/widgets/`. That directory is GITIGNORED — the definitions are derived, not tracked — so they exist for any developer who has ever run `mxcli widget docs` against the fixture (I generated them earlier in the same session, while investigating) and never exist on the runner. With no definition, `containerKeyword` returns \"\" and the two definition-dependent branches degrade exactly as designed: fallback wording, and silence for the scalar case", "file": "`mdl/executor/validate_widget_object_property_test.go` (`fixtureProjectWithDefs`)", "insight": "**A gitignored fixture makes a test environment-dependent in the one direction nobody checks** — the developer's tree is a superset of the runner's, so the test is green exactly where it is not being tested. The fix is to DERIVE the artifact from tracked inputs inside the test (`RefreshWidgetDefinitions` over the fixture's tracked `.mpk` files, into a temp copy, after removing any `.mxcli` the developer's tree carries), so local and CI see identical inputs. Reproduce by moving the gitignored directory aside before believing any diagnosis. **The sibling lesson is why this was not caught by the existing suite**: #999's test asserted `strings.Contains(msg, \"attribute\")` on a widget whose property is named `attributes`, so the property name alone satisfied it and the assertion passed with NO definition loaded — a substring assertion whose needle is a substring of the data it is meant to distinguish from proves nothing. Tightened to the remedy shape (`` `attribute (…)` blocks ``) and verified with the derivation stubbed: all four then fail, where before only the three new ones did", "refs": []} diff --git a/mdl/executor/validate_widget_object_property_test.go b/mdl/executor/validate_widget_object_property_test.go index d0d5e5956e..56348e3b68 100644 --- a/mdl/executor/validate_widget_object_property_test.go +++ b/mdl/executor/validate_widget_object_property_test.go @@ -3,6 +3,8 @@ package executor import ( + "os" + "path/filepath" "strings" "testing" @@ -11,13 +13,41 @@ import ( "github.com/mendixlabs/mxcli/mdl/visitor" ) +// fixtureProjectWithDefs copies the fixture to a temp dir and DERIVES its widget +// definitions from the tracked `.mpk` files. +// +// `testdata/expr-checker/.mxcli/` is gitignored: the `.def.json` files are +// generated, and a developer who has ever run `mxcli widget docs` against the +// fixture has them while CI never does. Reading them ambiently makes a test pass +// locally and fail on the runner — which is exactly how the three tests below +// first shipped red. Deriving them here depends only on tracked inputs, and the +// copy keeps the generated files out of the fixture other tests copy. +func fixtureProjectWithDefs(t *testing.T) string { + t.Helper() + src := filepath.Dir(fixtureProject(t)) + dst := t.TempDir() + if err := os.CopyFS(dst, os.DirFS(src)); err != nil { + t.Fatalf("copy fixture: %v", err) + } + // Start from no definitions whatever the developer's tree holds, so the + // test sees the same inputs here and on the runner. + if err := os.RemoveAll(filepath.Join(dst, ".mxcli")); err != nil { + t.Fatalf("clear derived definitions: %v", err) + } + proj := filepath.Join(dst, "minimal.mpr") + if _, err := RefreshWidgetDefinitions(proj, true, nil); err != nil { + t.Fatalf("derive widget definitions from the tracked .mpk files: %v", err) + } + return proj +} + func widget27(t *testing.T, src string) []linter.Violation { t.Helper() prog, errs := visitor.Build(src) if len(errs) > 0 { t.Fatalf("parsing: %v", errs) } - registry := LoadWidgetRegistry(fixtureProject(t)) + registry := LoadWidgetRegistry(fixtureProjectWithDefs(t)) if registry == nil { t.Fatal("no registry") } @@ -57,8 +87,12 @@ func TestObjectEntryProperty_IsReported(t *testing.T) { "page and discard the entries, which is the bug", got[0].Severity) } // The message has to name the container keyword, or it says "wrong" without - // saying what right looks like. - if !strings.Contains(got[0].Message, "attribute") { + // saying what right looks like. Assert the KEYWORD IN ITS REMEDY, not a bare + // "attribute": the property is called `attributes`, so a substring check for + // "attribute" is satisfied by the property name and passes with no definition + // loaded at all — which is how the definition-dependent tests below shipped + // green locally and red in CI. + if !strings.Contains(got[0].Message, "`attribute (…)` blocks") { t.Errorf("message does not name the container keyword: %q", got[0].Message) } if !strings.Contains(got[0].Message, "attributes") { From 8368b2fa13e132f6c48bc7e35bb5f98272349302 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 17:54:31 +0000 Subject: [PATCH 04/10] docs: settle four open questions on the microflow-description proposal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two runtime/storage measurements and the bookkeeping Phase E earned. SHORT-CIRCUIT (was Q1, blocked Mode 3). Both `and` and `or` short-circuit on Mendix 11.14.0, so folding a guard cannot introduce an evaluation the original avoided and Mode 3 is unblocked — the contemplated fallback ("restrict to conditions proven total, or drop") is not needed. The purity/ordering precondition is untouched. The probe is integer division by zero in the right operand, an expression's only observable given Mendix expressions are pure. What makes it a measurement is the pair of POSITIVE CONTROLS that force the right operand to be reached and must therefore throw: without them, the short-circuit tests passing is equally well explained by `1 div 0` being harmless. Both operands read microflow parameters rather than literals so mxbuild cannot constant-fold the expression, which would have measured its folding instead of the runtime's evaluation order. Shipped as a fixture pair, verified end to end on a clean project: 4/4. TRANSPLANTIDS ON UNNAMED MERGES (was Q4). IDs survive; positional matching does not churn them. Measured four ways — re-exec of identical MDL (elided), a FORCED write under MXCLI_ALWAYS_WRITE=1, a describe -> exec round trip, and an edit inserting an activity before the merges so every position shifts. The forced write is the load-bearing one: without it a pass only shows the write was skipped, not that anything was preserved. Also struck: fall-through (Q2), verb choice (Q5) and loops (Q6), all settled when Phase E shipped. KNOWN LIMITATION, recorded not fixed. labelRejoinMerges labels only error-rejoin merges; the nested describer represents an if/else join implicitly and walks through a single-input merge without representing it, so such a merge is deleted by a describe -> exec round trip with no warning. Measured: two authored merges describe to one, and executing that leaves one. It predates merge/join (DESCRIBE never emitted merges) but Phase E makes it reachable from MDL. It does not touch the Phase E fixpoint claim — D1 = D2 = D3, the loss is on the first step from the authored graph. Remedies weighed, undecided. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018hifgRSawfaRWXS44YKtSJ --- ...OPOSAL_structured_microflow_description.md | 109 ++++++++++++++---- .../bug-tests/923-short-circuit-semantics.mdl | 52 +++++++++ .../923-short-circuit-semantics.test.mdl | 42 +++++++ 3 files changed, 183 insertions(+), 20 deletions(-) create mode 100644 mdl-examples/bug-tests/923-short-circuit-semantics.mdl create mode 100644 mdl-examples/bug-tests/923-short-circuit-semantics.test.mdl diff --git a/docs/11-proposals/PROPOSAL_structured_microflow_description.md b/docs/11-proposals/PROPOSAL_structured_microflow_description.md index 5188e491a7..7d36296851 100644 --- a/docs/11-proposals/PROPOSAL_structured_microflow_description.md +++ b/docs/11-proposals/PROPOSAL_structured_microflow_description.md @@ -12,8 +12,12 @@ are shipped; the prevalence scan that gates the rest is [measured below](#measured-2026-09-12) and selects Mode 3. Mode 2 for the *non*-error irreducible graphs — crossed branches with no error handler — is the remaining piece: those still describe to flattened MDL with the MDL-FLOW01 -warning. Phases 1–2 otherwise unscheduled. -**Date:** 2026-08-20 (scan: 2026-09-12; Phase E: 2026-09-13) +warning. **Mode 3 is unblocked**: `and`/`or` were measured to short-circuit +(2026-09-13), so guard folding is semantics-preserving. Phases 1–2 otherwise +unscheduled. One [known limitation](#known-limitation-a-merge-outside-both-described-forms-is-dropped) +is open: a merge that is neither an error rejoin nor an `if`/`else` join is +dropped by a describe → exec round trip. +**Date:** 2026-08-20 (scan: 2026-09-12; Phase E and the runtime measurements: 2026-09-13) `DESCRIBE MICROFLOW` renders a microflow's control flow as nested `if/then/else`. That works only for graphs that are *properly nested*. A Mendix microflow is an @@ -513,26 +517,91 @@ every supported version; this is a describe/parse-side concern. No entry in - **`mx check`** — every generated form at 0 errors. - **Runtime (Phase 2 only)** — the short-circuit question above, settled by booting an app and observing whether an erroring right operand is evaluated. -- **Fixtures** — `mdl-examples/bug-tests/923-irreducible-microflow-graph.mdl`. + **Done** (2026-09-13): both operators short-circuit. Any such runtime probe + needs the positive control that forces the right operand to be reached — + otherwise a pass is explained just as well by the probe never throwing. +- **Fixtures** — `mdl-examples/bug-tests/923-irreducible-microflow-graph.mdl`; + `923-short-circuit-semantics{,.test}.mdl` (shipped). ## Open Questions -1. **Short-circuit semantics of `and`/`or`** — undocumented; blocks Mode 3. Settle - on a runtime before designing further. -2. **Fall-through into a `merge`** — this proposal requires an explicit terminator. - The permissive alternative (fall-through means an implicit `join` to the next - declared merge) is friendlier to hand-authors and shorter to read, at the cost - of ambiguity in exactly the construct that exists to remove ambiguity. -3. **`normalized` as an MDL modifier vs a CLI flag.** The modifier keeps it in the +1. **`normalized` as an MDL modifier vs a CLI flag.** The modifier keeps it in the language and works in the REPL; a flag keeps a rendering option out of the grammar. No existing `DESCRIBE` modifier sets precedent. -4. **`canon.TransplantIDs` on unnamed merges** — measure whether merge `$ID`s - survive a describe → exec round-trip, or whether positional matching churns - them. Affects whether Phase 1 output is genuinely idempotent. -5. **Verb choice** — `join ` vs `goto `. `join` matches Mendix's own - vocabulary and ADR-0003; `goto` is more immediately obvious to a developer and - was the reporter's own instinct in the issue discussion. -6. **Loops.** This proposal addresses acyclic split/merge structure. Retry loops - (issue #281) already make the flow graph cyclic and are handled by separate - pass-through logic; whether the detector needs to exclude back-edges explicitly - is unverified. + +### Settled + +- **Short-circuit semantics of `and`/`or`** (was Q1, blocked Mode 3) — **both + short-circuit**, so folding a guard does not introduce an evaluation the + original avoided and **Mode 3 is unblocked**. Measured on Mendix 11.14.0 on a + local runtime, 2026-09-13; fixtures + `mdl-examples/bug-tests/923-short-circuit-semantics{,.test}.mdl`. + + The probe is integer division by zero in the right operand — the only + observable an expression can have, Mendix expressions being pure. What makes + it a measurement rather than an assumption is the pair of **positive + controls**: two tests that force the right operand to be reached and therefore + must throw. Without them, the short-circuit tests passing is equally well + explained by `1 div 0` being harmless, which would say nothing about + evaluation order. Both operands read microflow *parameters*, not literals, so + mxbuild cannot constant-fold the expression and turn the measurement into one + of its own folding. + + This removes the restriction the proposal contemplated ("if `or` turns out to + be eager, Mode 3 is restricted to conditions proven total, or dropped"). The + *purity and ordering* precondition is untouched and still applies: the region + being folded must contain only splits and merges. + +- **Fall-through into a `merge`** (was Q2) — settled **permissive** when Phase E + shipped: a path that has already ended (`return`, `throw`, `join`) does not + fall through into a following `merge`, but an ordinary path does. The + ambiguity the strict form guarded against turned out to sit elsewhere — a + *terminated* path emitting a duplicate edge into a following merge — and that + is what is enforced. + +- **`canon.TransplantIDs` on unnamed merges** (was Q4) — **IDs survive**; + positional matching does not churn them, and Phase E's output is genuinely + idempotent. Measured 2026-09-13 on Mendix 11.14.0 four ways, each stronger + than the last: re-exec of identical MDL (write elided, `Unchanged + microflow`); a **forced write** under `MXCLI_ALWAYS_WRITE=1` (`Replaced` — so + elision is bypassed and the transplant is what preserves the IDs); a + describe → exec round trip; and an edit inserting an activity *before* the + merges, shifting every position. The forced-write run is the load-bearing one: + without it a passing test only shows the write was skipped. + +- **Verb choice** (was Q5) — `join`, shipped in Phase E. + +- **Loops** (was Q6) — a retry loop is expressible as a **backward `join`** to a + `merge` declared earlier, which is what Phase E's forward-and-backward label + resolution is for. `merge`/`join` inside a `loop`/`while` *body* is refused + (MDL-FLOW04): a `LoopedActivity` owns its own object collection and a sequence + flow cannot leave it, so there is no graph to build. + +## Known limitation: a merge outside both described forms is dropped + +`labelRejoinMerges` labels only the merges an error handler reaches *and* the +normal path also reaches. Every other merge is left to the nested describer, +which represents an `if`/`else` join implicitly and **walks straight through a +merge with a single incoming path without representing it at all**. Such a merge +is therefore deleted by a describe → exec round trip, silently — no +`MDL-FLOW01`, no warning on exec, just `Replaced microflow` one node lighter. + +Measured 2026-09-13: a microflow authored with two merges (one a 2-input error +rejoin, one a 1-input pass-through) describes to **one** `merge`, and executing +that output leaves the model with one. The ordinary `if`/`else` merge is +unaffected and round-trips with its `$ID` intact and the write elided. + +This predates `merge`/`join` — `DESCRIBE` never emitted merges before, so a +Studio Pro-authored 1-input merge has always been dropped — but Phase E makes it +reachable from MDL, so a user can now write `MERGE done;` and have their own +syntax disappear on the next round trip. It is behaviourally harmless (a 1-input +merge is a no-op) and does **not** affect the Phase E fixpoint claim: describe → +exec → describe converges after one step (verified D1 = D2 = D3). The loss is on +the first step, from the *authored graph* to D1. + +Deleting a node the user drew is what guard-don't-drop exists to prevent, so the +candidate remedies are a **describe-time warning** when an `ExclusiveMerge` is +not represented in the output (cheap, no behaviour change, consistent with +MDL-FLOW01 warning rather than silently flattening) or emitting a label for every +unrepresented merge (more faithful, noisier on every describe of a real +microflow). Undecided. diff --git a/mdl-examples/bug-tests/923-short-circuit-semantics.mdl b/mdl-examples/bug-tests/923-short-circuit-semantics.mdl new file mode 100644 index 0000000000..6c34c70c8f --- /dev/null +++ b/mdl-examples/bug-tests/923-short-circuit-semantics.mdl @@ -0,0 +1,52 @@ +-- ============================================================================ +-- Do Mendix `and` / `or` short-circuit? Measured, not reasoned about. +-- ============================================================================ +-- +-- Open Question 1 of PROPOSAL_structured_microflow_description.md. Mode 3 of +-- that proposal folds the guards of nested exclusive splits into one condition +-- -- `c1 and c2` on nested splits becomes `not(c1) or c2` -- which is only +-- semantics-preserving if the right operand is NOT evaluated once the left has +-- decided the result. Mendix's reference guide documents both operators +-- WITHOUT stating whether they short-circuit, so the question could not be +-- settled from the docs. +-- +-- The probe is integer division by zero in the right operand: the only +-- observable an expression can have is whether it throws, since Mendix +-- expressions are pure and cannot carry a side effect. +-- +-- Both operands read microflow PARAMETERS rather than literals. That is +-- deliberate: `1 div 0` written literally could be folded at build time, and a +-- folded expression would measure mxbuild's constant folding instead of the +-- runtime's evaluation order. +-- +-- Run it: +-- mxcli exec mdl-examples/bug-tests/923-short-circuit-semantics.mdl -p app.mpr +-- mxcli test mdl-examples/bug-tests/923-short-circuit-semantics.test.mdl \ +-- -p app.mpr --local +-- +-- Result (Mendix 11.14.0, local runtime, 2026-09-13): BOTH short-circuit. +-- The two CONTROL tests in the .test.mdl are what make that a measurement +-- rather than an assumption -- see the header there. + +create module ShortCircuit; +/ + +create or replace microflow ShortCircuit.MF_ScOr (Guard: Boolean, Zero: Integer) returns String +begin + if $Guard or (1 div $Zero > 0) then + return 'taken'; + else + return 'not taken'; + end if; +end; +/ + +create or replace microflow ShortCircuit.MF_ScAnd (Guard: Boolean, Zero: Integer) returns String +begin + if $Guard and (1 div $Zero > 0) then + return 'taken'; + else + return 'not taken'; + end if; +end; +/ diff --git a/mdl-examples/bug-tests/923-short-circuit-semantics.test.mdl b/mdl-examples/bug-tests/923-short-circuit-semantics.test.mdl new file mode 100644 index 0000000000..0a78bc2a22 --- /dev/null +++ b/mdl-examples/bug-tests/923-short-circuit-semantics.test.mdl @@ -0,0 +1,42 @@ +/** + * Do Mendix `and` / `or` short-circuit? Open Question 1 of + * PROPOSAL_structured_microflow_description.md, which blocks Mode 3. + * + * Setup: run 923-short-circuit-semantics.mdl against the project first. + * + * Tests 1 and 3 are POSITIVE CONTROLS. They force the right operand to be + * reached, so they MUST throw. Without them a pass on tests 2 and 4 is + * worthless: it would be equally well explained by division by zero being + * harmless in Mendix, which would say nothing at all about evaluation order. + * The controls are the difference between measuring short-circuiting and + * measuring nothing. + * + * Measured on Mendix 11.14.0 (local runtime, 2026-09-13): all four pass, so + * both operators short-circuit. + * + * @test CONTROL or right operand reached so division by zero throws + * @throws + */ +$result = call microflow ShortCircuit.MF_ScOr(Guard = false, Zero = 0); +/ + +/** + * @test or short circuits when the left operand is true + * @expect $result = 'taken' + */ +$result = call microflow ShortCircuit.MF_ScOr(Guard = true, Zero = 0); +/ + +/** + * @test CONTROL and right operand reached so division by zero throws + * @throws + */ +$result = call microflow ShortCircuit.MF_ScAnd(Guard = true, Zero = 0); +/ + +/** + * @test and short circuits when the left operand is false + * @expect $result = 'not taken' + */ +$result = call microflow ShortCircuit.MF_ScAnd(Guard = false, Zero = 0); +/ From 3195a45c5363b6ef07dbe6a8fd3ba81ef1e69242 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 18:15:59 +0000 Subject: [PATCH 05/10] fix(describe): warn when a merge the description omits will be deleted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DESCRIBE walks straight through an ExclusiveMerge with a single incoming path without emitting anything for it, so describe -> exec DELETES the node. Silently: no warning, no MDL-FLOW01 (the graph is perfectly reducible, so the irreducibility detector is right to stay quiet), and mx check clean either way. Measured on a blank 11.14 app plus FeedbackModule and Administration, comparing merge $IDs across a round trip over 21 microflows: in-degree 1 deleted 7 of 7, everything else survived with its $ID intact. It is not a corner case — VAL_Feedback loses 5 of its 10 merges and SUB_Feedback_SendToServer 3 of 5. Behaviourally harmless (a one-input merge is a no-op) but it deletes a node the user drew, which is what guard-don't-drop exists to prevent. The obvious rule is wrong on real code. "Represented" cannot just mean the join point findSplitMergePoints pairs with a split: findMergeForSplit needs a join common to ALL branches, so a split where one branch returns pairs with nothing — and yet its merge is emitted as the continuation after `end split` and survives. Administration.ManageMyAccount is that shape and was a false positive until the in-degree clause went in; a warning that fires on ordinary Marketplace code is worse than no warning. Four negative controls pin it, one per quiet shape. Deliberately out of scope: a merge lost to the flattening of an IRREDUCIBLE graph (one two-input merge of SUB_Feedback_SendToServer goes that way). Those microflows already carry MDL-FLOW01, which says the whole description is not equivalent and must not be re-executed — strictly stronger than this warning, and the right owner. After the fix, every microflow in the corpus that loses a merge is flagged and no microflow that does not is. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018hifgRSawfaRWXS44YKtSJ --- .../fix-issue/findings/mdl-executor.jsonl | 1 + mdl/executor/cmd_microflows_show.go | 2 + .../cmd_microflows_show_dropped_merge_test.go | 174 ++++++++++++++++++ mdl/executor/cmd_microflows_show_merge.go | 100 ++++++++++ 4 files changed, 277 insertions(+) create mode 100644 mdl/executor/cmd_microflows_show_dropped_merge_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index f8fb070d65..c9c5d22adf 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -607,3 +607,4 @@ {"area": "mdl/executor", "date": "2026-09-13", "symptom": "A workflow with `boundary event timer '\u2026'` (no interrupting / non interrupting) passes check and builds at 0 errors; the runtime then fails to start: `Class 'Workflows$TimerBoundaryEvent' could not be found`", "cause": "The bare form maps to `Workflows$TimerBoundaryEvent`, which exists in no cached 11.x runtime (only Interrupting/NonInterruptingTimerBoundaryEvent). mxbuild tolerates the unknown type. It was the documented syntax example", "file": "`mdl/executor/validate_workflow_refs.go` (`bareTimerBoundaryEventErrors`, MDL-WF07), `cmd/mxcli/syntax/features_workflow.go`", "insight": "**A type mxbuild accepts is not a type the runtime has.** Found only because a verification boot of an unrelated fix loaded it. When a grammar has a default branch that maps to a storage type, check that type against the runtime's class list, not against `mx check`", "fix": "Refuse the bare form on 11+ at check and exec, CREATE and every ALTER op that can carry a boundary event; update syntax help, skill table and the ako/mxcli#415 bug-test script to name the kind"} {"area": "mdl/executor", "date": "2026-09-13", "symptom": "A view entity whose association column is also declared as an attribute (`MeterRef: Trends.Meter` or `MeterRef: Trends.Meter.ID` beside `select m.ID as MeterRef`) passes `mxcli check`; `check -p` says 'OQL select has 1 columns but 2 attributes declared'; exec writes `Enumeration(Trends.Meter)` and mx check reports CE1613, or throws 'An error occurred when trying to set the Enumeration property' for the three-part form", "cause": "A bare qualified name parses as TypeEnumeration (the entity/enum ambiguity), and execCreateViewEntity converted it with convertDataType without asking what it names. The alias-to-attribute alignment skips association columns, so the declared attribute had no column and was compared against the next one", "file": "`mdl/executor/oql_view_associations.go` (`ValidateViewAttributeDeclarations` MDL080, `viewAttributeEntityTypeErrors`), `mdl/executor/cmd_entities.go` (`execCreateViewEntity`), `mdl/executor/validate.go`, `mdl/executor/validate_program.go`, `cmd/mxcli/lsp_diagnostics.go`", "insight": "**The TypeEnumeration/TypeEntity ambiguity has a consumer wherever a data type becomes a stored type, and view entity attributes were one nobody had listed.** Split the refusal by what it needs: an association column's alias and a three-part name are decidable from the script, so they belong in the no-project phase that exec's pre-check also runs; entity-vs-enum needs the project, so it goes in check -p AND the handler, because exec --no-check skips both phases. Verify the handler refusal by counting changed files, not by the error text", "fix": "Refuse in ValidateProgram/LSP (MDL080) and at the top of execCreateViewEntity before any backend call; report an attribute once"} {"area": "mdl/executor", "date": "2026-09-13", "symptom": "Three new MDL-WIDGET27 tests passed locally and failed in CI on the same commit: two reported the fallback remedy (\"move the entries into the widget body as container blocks\") instead of naming the container keyword, and the third found 0 violations where it wanted 1", "cause": "The tests resolved the widget through `LoadWidgetRegistry(fixtureProject(t))`, which reads `.def.json` files from `testdata/expr-checker/.mxcli/widgets/`. That directory is GITIGNORED — the definitions are derived, not tracked — so they exist for any developer who has ever run `mxcli widget docs` against the fixture (I generated them earlier in the same session, while investigating) and never exist on the runner. With no definition, `containerKeyword` returns \"\" and the two definition-dependent branches degrade exactly as designed: fallback wording, and silence for the scalar case", "file": "`mdl/executor/validate_widget_object_property_test.go` (`fixtureProjectWithDefs`)", "insight": "**A gitignored fixture makes a test environment-dependent in the one direction nobody checks** — the developer's tree is a superset of the runner's, so the test is green exactly where it is not being tested. The fix is to DERIVE the artifact from tracked inputs inside the test (`RefreshWidgetDefinitions` over the fixture's tracked `.mpk` files, into a temp copy, after removing any `.mxcli` the developer's tree carries), so local and CI see identical inputs. Reproduce by moving the gitignored directory aside before believing any diagnosis. **The sibling lesson is why this was not caught by the existing suite**: #999's test asserted `strings.Contains(msg, \"attribute\")` on a widget whose property is named `attributes`, so the property name alone satisfied it and the assertion passed with NO definition loaded — a substring assertion whose needle is a substring of the data it is meant to distinguish from proves nothing. Tightened to the remedy shape (`` `attribute (…)` blocks ``) and verified with the derivation stubbed: all four then fail, where before only the three new ones did", "refs": []} +{"area": "mdl-executor", "date": "2026-09-13", "symptom": "DESCRIBE silently deletes an ExclusiveMerge: describe -> exec leaves the microflow with fewer merge nodes than the stored graph, with no warning, no MDL-FLOW01 and mx check clean", "cause": "The nested describer walks straight through a merge with a single incoming path without emitting anything for it, so the rebuild has no reason to create it. Only two merge shapes were represented: a split's join point (rendered by `end if`) and a labelled error rejoin (`merge