From 0de62aaedb9127dc3601bc5ef554a2b8c105be63 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 16:32:23 +0000 Subject: [PATCH] feat(microflow): @position on a flow parameter, and stop moving hand-placed ones MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A Microflows$MicroflowParameter is a stored node with real geometry (RelativeMiddlePoint + Size 30;30) that Studio Pro lets you drag, but no MDL annotation reached it. Two consequences, the second the more serious: a generated flow's parameter block landed wherever the writer put it, and an existing hand-aligned one was MOVED by any rewrite — including a describe -> exec of mxcli's own output. Measured on a real nanoflow, a parameter at -77;0 came back at 200;53; on a 1971-unit project, 20 of 28 parameters sat off the derived grid and so were moved. The cause was a four-point chain: the semantic type had no position field, so neither reader carried one, so both writers could only recompute 200+idx*100;53 inline, and the grammar had no slot to state one. The placement rule is @start's, unchanged (#884, #951): a parameter sitting exactly where the layout would have put it is mxcli's own arithmetic handed back, carries no intent, and is re-derived; one anywhere else was placed by a person, survives a rewrite, and is emitted by DESCRIBE. Carrying stored coordinates over unconditionally is the trap that rule exists to avoid — inserting a parameter would strand the existing ones on the old grid while the new one landed on top. The arbitration lives in the readers, so a non-nil Position means intent everywhere downstream; it is a pointer because 0;0 is a coordinate a person can choose. Syntax needed one grammar line (annotation* on microflowParameter), with no ANTLR ambiguity, so the block-level @parameters(...) alternative is unnecessary. @position is the only annotation a parameter takes; anything else — a typo of it above all — is refused as MDL059 rather than parsing and doing nothing, in check, exec and the LSP. Covers microflows, nanoflows and rules (shared parameter grammar) on both engines. The four near-duplicate describers now share one helper, so the annotation cannot appear from one command and not another. Verified on mxbuild 11.13: authored positions stored verbatim, the unannotated control still deriving to 200;53/300;53, describe -> exec reporting Unchanged, 0 errors. Control: with the read-side carry stubbed, -77;0 -> 200;53 returns and DESCRIBE emits nothing. Refs: ako/mxcli#993 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017s476QkXr9CFMvKspVzcvu --- .../fix-issue/findings/mdl-executor.jsonl | 1 + .../reference/control-flow.md | 8 ++ cmd/mxcli/lsp_diagnostics.go | 12 ++ cmd/mxcli/syntax/features_microflow.go | 12 +- docs/01-project/MDL_QUICK_REFERENCE.md | 1 + .../microflow-993-parameter-position.mdl | 65 +++++++++ mdl/ast/ast_microflow.go | 11 +- mdl/backend/modelsdk/microflow.go | 7 + .../microflow_parameter_position_test.go | 36 +++++ mdl/backend/modelsdk/microflow_write.go | 11 +- mdl/executor/cmd_microflows_create.go | 1 + .../cmd_microflows_parameter_position.go | 73 ++++++++++ .../cmd_microflows_parameter_position_test.go | 129 ++++++++++++++++++ mdl/executor/cmd_microflows_show.go | 60 +++----- mdl/executor/cmd_nanoflows_create.go | 1 + mdl/executor/cmd_rules_create.go | 1 + mdl/executor/validate_flow_parameters.go | 41 ++++++ mdl/executor/validate_program.go | 12 ++ mdl/grammar/domains/MDLMicroflow.g4 | 5 +- .../microflow_parameter_position_test.go | 93 +++++++++++++ mdl/visitor/visitor_microflow.go | 34 +++++ sdk/microflows/microflows.go | 40 ++++++ sdk/mpr/microflow_parameter_position_test.go | 67 +++++++++ sdk/mpr/parser_microflow.go | 13 +- sdk/mpr/parser_nanoflow.go | 4 +- sdk/mpr/parser_rule.go | 2 +- sdk/mpr/writer_microflow.go | 9 +- 27 files changed, 689 insertions(+), 60 deletions(-) create mode 100644 mdl-examples/bug-tests/microflow-993-parameter-position.mdl create mode 100644 mdl/backend/modelsdk/microflow_parameter_position_test.go create mode 100644 mdl/executor/cmd_microflows_parameter_position.go create mode 100644 mdl/executor/cmd_microflows_parameter_position_test.go create mode 100644 mdl/executor/validate_flow_parameters.go create mode 100644 mdl/visitor/microflow_parameter_position_test.go create mode 100644 sdk/mpr/microflow_parameter_position_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index 08ea896a55..21ee42aae0 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -496,3 +496,4 @@ {"area": "mdl/executor", "date": "2026-08-31", "symptom": "`main` goes red on a test that passed in **both** PRs that touched it — here `TestDescribeWorkflow_NoAnnotationEmitsNoComment`: `unexpected violations [MDL-WF05] for a plain jump`", "cause": "Two PRs merged in sequence. One added a validator rule (MDL-WF05, dangling `jump to` target); the other's control test asserted `len(violations) == 0` over a fixture that was **not a valid workflow** — a lone jump whose target did not exist. Each CI run was green because neither saw the other's change", "file": "`mdl/executor/issue1007_annotation_emit_test.go` (the fixture), `mdl/executor/validate_workflow_jump.go` (the rule, which is right)", "insight": "**A test fixture that is not a valid instance of the thing under test is a landmine for the next rule.** \"No violations at all\" is only meaningful over input that *should* have none; over an invalid fixture it silently asserts \"no rule has been written yet that notices this\". Fix the **fixture**, not the rule. Generalise: when a test asserts the ABSENCE of diagnostics, make the input something you would be happy to ship. Two green PRs can still merge to red and **neither PR's CI can detect it** — the only protection is a fixture that does not depend on which rules exist today. Found by running `make test` on an unrelated docs branch cut from the merged main, which is an argument for doing that on any branch cut after a batch merge.", "refs": ["#350", "#351"], "rules": ["MDL-WF04", "MDL-WF05"]} {"area": "mdl/executor", "date": "2026-08-31", "symptom": "`filter($L, Amount > 0)` — or any FILTER/FIND predicate with a bare attribute and an operator other than `=` — fails the build with CE0117 \"Error(s) in expression.\", while the same attribute with `=` builds fine. `mxcli check` and `check --references` both pass. Reported as a Mendix 11.13 regression.", "cause": "The unfinished half of bug #343. Mendix has two filter operations: `Microflows$Filter` takes a member NAME (filter by attribute) and `Microflows$FilterByExpression` takes an expression evaluated per item with the item bound to `$currentObject`. #343 rerouted only `attr = value` to the by-attribute form; every other predicate still fell through to the expression form, where mxcli stored the authored text verbatim — and a bare attribute is not a valid Mendix expression. So the split was on the OPERATOR and invisible to the author: `Status = 'x'` built, `Status != 'x'` did not.", "file": "`mdl/executor/cmd_microflows_builder_actions.go` (`qualifyIteratorAttributes`, `listElementEntity`, `iteratorMemberPath`, `qualifyNamesInSource`); `mdl/executor/validate_microflow_listop_iterator.go` (MDL-LISTOP01); syntax topic `cmd/mxcli/syntax/features_microflow.go`; repros `mdl-examples/bug-tests/1002-filter-find-bare-attribute.mdl` and `1002-filter-bad-iterator.fail.mdl`.", "insight": "Rewrite a bare name that PROVABLY resolves to a member of the list's element entity into `$currentObject/`; refuse one that does not resolve (it used to surface as CE0117 at build time); pass through untouched when the element entity cannot be determined, which proves nothing either way. An association takes its module qualifier in the path, an attribute does not. The predicate is a frozen `SourceExpr` (`buildSourceExpression` in the visitor), so the rewrite has to patch the source TEXT, skipping single-quoted literals — `filter($L, Qty > 0 and Status != 'Amount')` must not rewrite the `'Amount'` inside the literal. Do not blame the Mendix version without running both: the issue reported \"reproduces on 11.13.0, not 11.11.0\" and the identical 7-error probe on mxbuild 11.11.0 and 11.13.0 disproves it — the fork is in mxcli, and nothing in it depends on the version. `mx check -j ` is the attribution tool: the console output names only the activity (\"List operation activity 'Filter by expression'\"), so two errors on one line read as one microflow; the JSON carries `document-name` per location. MDL-LISTOP01 keys on scope, not on the name — `$item` is valid in a predicate when it is the enclosing loop's iterator, which is exactly how CLAUDE.md's O(N) `find` idiom is written, so flagging the name would break the documented pattern. Control for the whole fix: stub `qualifyIteratorAttributes` to return the condition unchanged and the repro goes 0 → 7 x CE0117 with the two `=` cases staying green, which is also the #343 regression guard.", "refs": ["#1002", "#343"], "ce": ["CE0117", "CE0109"], "rules": ["MDL-LISTOP01"]} {"area": "mdl/executor", "date": "2026-08-31", "symptom": "A copied Atlas layout (or any page mxcli authors) fails `mx check` with **CE0463** on its Image widget, and a full field-level diff against Studio Pro's own widget differs in exactly **one line of 1480** — `maxHeight`, mxcli's `0` against the installed package's declared default", "cause": "Same class as the §69 width/height fix, which did not cover it — and why is the point. The reset was applied in a loop over the definition's **property mappings**, and a mapping is what gives a property an MDL keyword: `width`/`height` have one, `maxHeight` has none, so it was never visited and the widget TEMPLATE's captured value stood. Two further halves, each independently load-bearing (stub either and the test fails): a rule whose CONDITION is an unmapped property (`maxHeight` is hidden when `maxHeightUnit` = \"none\") was always indeterminable so never fired — the declared default is the right fallback there, because nothing can have moved an unnamable property off it; and `def.PropertyVisibility` is **empty** for every widget whose rules are lifted live from the `.mpk`, which is most of them, so a lookup keyed on that field silently finds nothing (both consumers now share `visibilityRules()`).", "file": "`mdl/executor/widget_engine.go` (`unmappedHiddenResets`, `visibilityRules`, the condition fallback in `hiddenUnnamedProperties`)", "insight": "The general rule: **the set of properties that must be default-valued is the widget's editorConfig to decide, not mxcli's** — making it a subset of what MDL has words for was the mistake. The third trap was found only end-to-end; the unit test passed while the command still wrote the wrong value. Control, end-to-end on a real 11.13 project with the Image `.mpk` patched to declare `maxHeight` 250 (the 1.6.0 value the finding measured, since that version was not obtainable): pre-fix binary writes 0 and all four mxcli-authored Images fail CE0463; fixed binary writes 250 and all four are clean, while `minHeight` stays at its own declared 0 — per-property from the package, not a blanket value. The project's ~60 Studio Pro-authored Images stay stale under both, which is the package change itself and not mxcli: the Step 0 discrimination diagnose-ce0463.md asks for. Reported as mxcli-ledger FINDINGS §142."} +{"area": "mdl/executor", "date": "2026-09-01", "symptom": "A hand-placed microflow/nanoflow/rule parameter is silently moved onto a grid at 200;53, 300;53, \u2026 by any rewrite \u2014 including a describe \u2192 exec of mxcli's OWN output. Reported as a feature request for `@position` on a parameter; the missing feature and the silent loss are the same defect seen from two sides.", "cause": "Microflows$MicroflowParameter is a stored node with RelativeMiddlePoint + Size, but the semantic type had no position field, so NEITHER reader carried one and BOTH writers could only recompute `200+idx*100;53` inline. The grammar had no slot for the annotation either, so there was no way to state a position and no way to preserve one.", "file": "sdk/microflows/microflows.go (Position + DerivedParameterPosition/AuthoredParameterPosition), mdl/grammar/domains/MDLMicroflow.g4 (annotation* on microflowParameter), mdl/visitor/visitor_microflow.go, mdl/backend/modelsdk/microflow.go + microflow_write.go, sdk/mpr/parser_microflow.go + writer_microflow.go, mdl/executor/cmd_microflows_parameter_position.go", "insight": "The design was already litigated one node family over and should be COPIED, not re-derived: @start's authoredStartPosition (#884 + #951) settles that a node at the layout's own derived spot carries no intent and must be re-derived, while one anywhere else was placed by a person and must survive. Carrying stored coordinates over unconditionally is the trap \u2014 inserting a parameter would strand the existing ones on the old grid while the new one lands on top. Put the arbitration in the READER so a non-nil Position means intent everywhere downstream; Position must be a POINTER because 0;0 is a coordinate a person can choose (two flows in the reference project use it). Measurement that framed the work: 20 of 28 parameters in a real 1971-unit project sit off the derived grid, so nearly every rewrite moved one. Trap when measuring: `Unchanged` on a SECOND exec proves the round trip reaches a fixed point, NOT that the first write changed only the thing you are looking at \u2014 this document also loses ExportLevel and a DestinationControlVector, unrelated and still open. Diff the raw unit BSON sorted by path, since the rebuild reorders ObjectCollection.Objects and a line-diff is then all noise.", "refs": ["ako/mxcli#993", "#951", "#884"], "rules": ["MDL059"]} diff --git a/.claude/skills/mendix/write-microflows/reference/control-flow.md b/.claude/skills/mendix/write-microflows/reference/control-flow.md index 9175a0693f..72c39849f6 100644 --- a/.claude/skills/mendix/write-microflows/reference/control-flow.md +++ b/.claude/skills/mendix/write-microflows/reference/control-flow.md @@ -310,6 +310,14 @@ commit $Product; - `@position` always appears in DESCRIBE output; `@caption` only when custom; `@color` only when not Default - DESCRIBE MICROFLOW shows `@` annotations before their activities - `@start(x, y)` positions the **start event** and goes on the first statement, because the start has no statement of its own. Omit it and the start is derived — one spacing unit (160) left of the first activity, on its centre line — and a rewrite re-derives it so the start follows the activities when they move. A start that is not at the derived spot was placed by hand (in Studio Pro or with `@start`): it survives a rewrite that does not mention it, and DESCRIBE emits `@start` for it. An explicit `@start` overrides both (#951) +- `@position(x, y)` on a **parameter** goes inside the parameter list, ahead of the parameter it places — a parameter is a stored node with its own coordinates, and this is the only annotation it takes. Omit it and the parameters form a row along the top of the canvas (200;53, 300;53, …). The `@start` rule above applies unchanged: a parameter on that derived row is re-derived on a rewrite, one anywhere else was placed by hand, survives, and is emitted by DESCRIBE (#993). Before this, a hand-aligned parameter block was moved back onto the row by any rewrite — including a describe → exec of mxcli's own output: + + ``` + create or modify nanoflow MyModule.ACT_Clear ( + @position(-77, 0) + $Feedback: MyModule.Feedback + ) + ``` ## Error Handling MDL supports error handling for activities that may fail (microflow calls, commits, external service calls, etc.). diff --git a/cmd/mxcli/lsp_diagnostics.go b/cmd/mxcli/lsp_diagnostics.go index c67d00ef1f..e2b48882e8 100644 --- a/cmd/mxcli/lsp_diagnostics.go +++ b/cmd/mxcli/lsp_diagnostics.go @@ -278,6 +278,18 @@ func (s *mdlServer) runSemanticValidation(text string) []protocol.Diagnostic { } if mfStmt, ok := stmt.(*ast.CreateMicroflowStmt); ok { violations = append(violations, executor.ValidateMicroflow(mfStmt)...) + violations = append(violations, executor.ValidateFlowParameterAnnotations( + "microflow '"+mfStmt.Name.String()+"'", mfStmt.Parameters)...) + } + // The editor reports an unusable parameter annotation for the same + // reason `check` does — a typo of @position parses and does nothing. + if nfStmt, ok := stmt.(*ast.CreateNanoflowStmt); ok { + violations = append(violations, executor.ValidateFlowParameterAnnotations( + "nanoflow '"+nfStmt.Name.String()+"'", nfStmt.Parameters)...) + } + if ruleStmt, ok := stmt.(*ast.CreateRuleStmt); ok { + violations = append(violations, executor.ValidateFlowParameterAnnotations( + "rule '"+ruleStmt.Name.String()+"'", ruleStmt.Parameters)...) } if setStmt, ok := stmt.(*ast.AlterSettingsStmt); ok { violations = append(violations, executor.ValidateSettings(setStmt)...) diff --git a/cmd/mxcli/syntax/features_microflow.go b/cmd/mxcli/syntax/features_microflow.go index efdb3e46db..e896ef91c2 100644 --- a/cmd/mxcli/syntax/features_microflow.go +++ b/cmd/mxcli/syntax/features_microflow.go @@ -272,6 +272,7 @@ func init() { "annotation", "caption", "color", "excluded", "bezier", }, Syntax: "@position(x, y) -- the activity's centre point\n" + + "@position(x, y) -- also on a PARAMETER, in the ( … ) list\n" + "@start(x, y) -- the start event, on the FIRST statement\n" + "@anchor(from: right, to: left) -- which SIDE each end of the outgoing flow attaches to\n" + "@curve(from: (40, -90), to: (-40, 90)) -- the flow's bezier control vectors\n" + @@ -288,8 +289,15 @@ func init() { "start is placed one spacing unit left of the first activity, on its centre\n" + "line — and a rewrite MOVES it to follow the activities. A start that is not\n" + "at that derived spot was put there by hand: it survives a rewrite, and\n" + - "DESCRIBE emits @start for it so the description round-trips exactly.", - Example: "create microflow MyModule.ACT_Flow ($In: String)\nreturns String as $Out\nbegin\n" + + "DESCRIBE emits @start for it so the description round-trips exactly.\n\n" + + "A PARAMETER is a stored node with its own coordinates, so it takes\n" + + "@position too — written inside the parameter list, ahead of the parameter\n" + + "it places. It is the only annotation a parameter takes. Omit it and the\n" + + "parameters form a row along the top of the canvas at 200;53, 300;53, … ;\n" + + "the same derived/authored rule as @start then applies, so a parameter on\n" + + "that row is re-derived and one anywhere else survives a rewrite and is\n" + + "emitted by DESCRIBE.", + Example: "create microflow MyModule.ACT_Flow (\n @position(145, 0)\n $In: String\n)\nreturns String as $Out\nbegin\n" + " @start(145, 100)\n @position(200, 100)\n @anchor(from: bottom, to: top)\n" + " @curve(from: (40, -90), to: (-40, 90))\n declare $Tmp String = $In;\n" + " @position(200, 300)\n declare $Out String = $Tmp;\n return $Out;\nend;", diff --git a/docs/01-project/MDL_QUICK_REFERENCE.md b/docs/01-project/MDL_QUICK_REFERENCE.md index 8fcdef36ae..a28209630d 100644 --- a/docs/01-project/MDL_QUICK_REFERENCE.md +++ b/docs/01-project/MDL_QUICK_REFERENCE.md @@ -536,6 +536,7 @@ it is for pages. | Validation | `validation feedback $entity/attribute message 'message';` | Requires attribute path + MESSAGE | | Log | `log info\|warning\|error [node 'name'] 'message';` | | | Position | `@position(x, y)` | Canvas position (before activity) | +| Parameter position | `@position(x, y)` before a parameter, **inside** the `( … )` list | The only annotation a parameter takes. Omit it and parameters form a row at 200;53, 300;53, …; a parameter off that row is treated as hand-placed, survives a rewrite, and is emitted by DESCRIBE (#993) | | Start event | `@start(x, y)` | Canvas position of the start, on the **first** statement. Omit it and the start is placed one spacing unit left of the first activity and MOVES with it on a rewrite; a start that is not at that derived spot is treated as hand-placed, survives a rewrite, and is emitted by DESCRIBE (#951) | | Caption | `@caption 'text'` | Custom caption (before activity) | | Color | `@color Green` | Background color (before activity) | diff --git a/mdl-examples/bug-tests/microflow-993-parameter-position.mdl b/mdl-examples/bug-tests/microflow-993-parameter-position.mdl new file mode 100644 index 0000000000..82e59ba4f1 --- /dev/null +++ b/mdl-examples/bug-tests/microflow-993-parameter-position.mdl @@ -0,0 +1,65 @@ +-- ako/mxcli#993 — @position on a flow parameter. +-- +-- A MicroflowParameter is a stored node with real geometry (RelativeMiddlePoint +-- + Size 30;30), and Studio Pro lets you drag it. Before this, no annotation +-- reached it: a generated flow's parameter block landed on mxcli's derived grid +-- and a hand-aligned one was moved back there by any rewrite. Measured on a real +-- project: a nanoflow parameter at -77;0 came back at 200;53 from a describe → +-- exec of mxcli's own output. +-- +-- Run, then `describe` each flow: NF_ParamPos round-trips its annotations, +-- NF_ParamDerived emits none because its parameters sit exactly where the layout +-- put them. + +create or replace nanoflow MyFirstModule.NF_ParamPos ( + @position(300, 100) + $A: Integer, + @position(200, 100) + $B: Integer +) +returns Integer as $R +begin + @position(300, 200) + declare $R Integer = $A + $B; + @position(500, 200) + return $R; +end; + +-- Control: no annotation, so both parameters go where the layout puts them — +-- 200;53 and 300;53. DESCRIBE must emit no @position for either, or every +-- rewritten flow would come back with its parameters pinned to the grid they +-- happened to be on (the #951 failure, one node family over). +create or replace nanoflow MyFirstModule.NF_ParamDerived ( + $A: Integer, + $B: Integer +) +returns Integer as $R +begin + @position(300, 200) + declare $R Integer = $A + $B; + @position(500, 200) + return $R; +end; + +-- Microflows and rules share the parameter grammar, so they take it too. +create or replace microflow MyFirstModule.MF_ParamPos ( + @position(140, -60) + $A: Integer +) +returns Integer as $R +begin + @position(300, 200) + declare $R Integer = $A + 1; + @position(500, 200) + return $R; +end; + +create or replace rule MyFirstModule.RL_ParamPos ( + @position(60, -40) + $A: Integer +) +returns Boolean +begin + @position(300, 200) + return $A > 0; +end; diff --git a/mdl/ast/ast_microflow.go b/mdl/ast/ast_microflow.go index db6fde1a81..d551f73f78 100644 --- a/mdl/ast/ast_microflow.go +++ b/mdl/ast/ast_microflow.go @@ -33,8 +33,15 @@ type ErrorHandlingClause struct { // MicroflowParam represents a microflow parameter. type MicroflowParam struct { - Name string // Parameter name (without $ prefix) - Type DataType // Parameter type + Name string // Parameter name (without $ prefix) + Type DataType // Parameter type + Position *Position // @position(x, y) on the parameter; nil to let the layout place it + // UnknownAnnotations holds annotation names written on the parameter that + // mxcli does not implement there. Collected rather than dropped so MDL059 + // can refuse them: an annotation that parses and does nothing loses whatever + // it was meant to express, silently (#884, the same reasoning one node + // family over). + UnknownAnnotations []string } // MicroflowReturnType represents a microflow return type. diff --git a/mdl/backend/modelsdk/microflow.go b/mdl/backend/modelsdk/microflow.go index 6559a25792..f18d93ebfd 100644 --- a/mdl/backend/modelsdk/microflow.go +++ b/mdl/backend/modelsdk/microflow.go @@ -377,6 +377,13 @@ func splitFlowObjects(coll element.Element) ([]*microflows.MicroflowParameter, [ if po, ok := el.(*genMf.MicroflowParameter); ok { p := µflows.MicroflowParameter{Name: po.Name(), Type: dataTypeFromGen(po.ParameterType())} p.ID = model.ID(el.ID()) + // Carry the parameter's canvas position, but only when it is not the + // one the layout would derive for this index — a parameter sitting on + // the derived grid carries no intent, and pinning it would strand the + // others the moment a parameter is inserted (#993, and #951 before + // it). Without this the position was never read at all, so a rewrite + // moved every hand-placed parameter back onto the grid. + p.Position = microflows.AuthoredParameterPosition(pointFromGen(el), len(params)) params = append(params, p) continue } diff --git a/mdl/backend/modelsdk/microflow_parameter_position_test.go b/mdl/backend/modelsdk/microflow_parameter_position_test.go new file mode 100644 index 0000000000..e38901c87c --- /dev/null +++ b/mdl/backend/modelsdk/microflow_parameter_position_test.go @@ -0,0 +1,36 @@ +// SPDX-License-Identifier: Apache-2.0 + +package modelsdkbackend + +import ( + "testing" + + "github.com/mendixlabs/mxcli/model" + genMf "github.com/mendixlabs/mxcli/modelsdk/gen/microflows" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// #993, codec engine side. The two engines derive the same grid, so a fix +// applied to one and not the other would make a parameter's placement depend on +// which engine happened to run. +func TestMicroflowParameterToGenKeepsAuthoredPosition(t *testing.T) { + authored := µflows.MicroflowParameter{ + Name: "Feedback", + Position: &model.Point{X: -77, Y: 0}, + } + g := microflowParameterToGen(authored, 0, 11).(*genMf.MicroflowParameter) + if got := g.RelativeMiddlePoint(); got != "-77;0" { + t.Errorf("authored position = %q, want -77;0", got) + } + + // Control: unannotated parameters still derive from the index. + derived := µflows.MicroflowParameter{Name: "Feedback"} + g = microflowParameterToGen(derived, 0, 11).(*genMf.MicroflowParameter) + if got := g.RelativeMiddlePoint(); got != "200;53" { + t.Errorf("derived position at index 0 = %q, want 200;53", got) + } + g = microflowParameterToGen(derived, 1, 11).(*genMf.MicroflowParameter) + if got := g.RelativeMiddlePoint(); got != "300;53" { + t.Errorf("derived position at index 1 = %q, want 300;53", got) + } +} diff --git a/mdl/backend/modelsdk/microflow_write.go b/mdl/backend/modelsdk/microflow_write.go index 20f639e0b6..826a152f81 100644 --- a/mdl/backend/modelsdk/microflow_write.go +++ b/mdl/backend/modelsdk/microflow_write.go @@ -1380,15 +1380,20 @@ func memberChangeToGen(m *microflows.MemberChange) element.Element { return g } -// microflowParameterToGen builds a gen MicroflowParameter (position derives from -// index, matching the legacy serializer). +// microflowParameterToGen builds a gen MicroflowParameter. An authored position +// is written as given; without one the position derives from the index, matching +// the legacy serializer. func microflowParameterToGen(p *microflows.MicroflowParameter, idx, major int) element.Element { g := genMf.NewMicroflowParameter() g.SetID(element.ID(p.ID)) g.SetDocumentation(p.Documentation) g.SetHasVariableNameBeenChanged(false) g.SetName(p.Name) - g.SetRelativeMiddlePoint(fmt.Sprintf("%d;53", 200+idx*100)) + pos := microflows.DerivedParameterPosition(idx) + if p.Position != nil { + pos = *p.Position + } + g.SetRelativeMiddlePoint(fmt.Sprintf("%d;%d", pos.X, pos.Y)) g.SetSize("30;30") if major >= 10 { g.SetDefaultValue("") diff --git a/mdl/executor/cmd_microflows_create.go b/mdl/executor/cmd_microflows_create.go index 2208d143c8..3362eb55b5 100644 --- a/mdl/executor/cmd_microflows_create.go +++ b/mdl/executor/cmd_microflows_create.go @@ -224,6 +224,7 @@ func execCreateMicroflow(ctx *ExecContext, s *ast.CreateMicroflowStmt) error { ContainerID: mf.ID, Name: p.Name, Type: convertASTToMicroflowDataType(p.Type, entityResolver), + Position: positionFromAST(p.Position), } mf.Parameters = append(mf.Parameters, param) } diff --git a/mdl/executor/cmd_microflows_parameter_position.go b/mdl/executor/cmd_microflows_parameter_position.go new file mode 100644 index 0000000000..f015dba593 --- /dev/null +++ b/mdl/executor/cmd_microflows_parameter_position.go @@ -0,0 +1,73 @@ +// SPDX-License-Identifier: Apache-2.0 + +// Package executor - parameter placement across a flow rewrite. +package executor + +import ( + "fmt" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// positionFromAST converts an `@position(x, y)` written on a parameter into the +// semantic model's point. nil in, nil out: no annotation means the layout +// places the parameter, which is what microflows.DerivedParameterPosition does. +func positionFromAST(p *ast.Position) *model.Point { + if p == nil { + return nil + } + return &model.Point{X: p.X, Y: p.Y} +} + +// parameterPositionAnnotation returns the `@position(x, y)` line a description +// needs for the parameter at index idx, or "" when the layout would put it +// exactly where it is. +// +// Emitting only an authored position is what keeps a described flow +// round-tripping: a line restating mxcli's own arithmetic would pin every +// parameter of every rewritten flow to the grid it happened to be on, so +// inserting a parameter would strand the others (the #951 lesson, which the +// StartEvent learned first — see authoredStartPosition). +// +// Readers already normalise, so Position is non-nil only when it is intent; +// the derived comparison here is belt-and-braces for a model assembled in +// memory rather than read from disk. +func parameterPositionAnnotation(p *microflows.MicroflowParameter, idx int, indent string) string { + if p == nil || p.Position == nil { + return "" + } + if *p.Position == microflows.DerivedParameterPosition(idx) { + return "" + } + return fmt.Sprintf("%s@position(%d, %d)", indent, p.Position.X, p.Position.Y) +} + +// describeMicroflowParameters renders the parenthesised parameter list of a +// flow header — one parameter per line, each preceded by its `@position` when +// it has one. +// +// Shared by all four describers (microflow, nanoflow, the generic flow +// describer and the rule describer). They were four copies of the same six +// lines, and a describer that emits the annotation while its twins do not makes +// the round-trip depend on which command the author happened to run — the same +// trap startAnnotationLines calls out. +func describeMicroflowParameters(params []*microflows.MicroflowParameter, formatType func(*microflows.MicroflowParameter) string) []string { + lines := make([]string, 0, len(params)*2) + for i, param := range params { + if ann := parameterPositionAnnotation(param, i, " "); ann != "" { + lines = append(lines, ann) + } + paramType := "Object" + if param.Type != nil { + paramType = formatType(param) + } + comma := "," + if i == len(params)-1 { + comma = "" + } + lines = append(lines, fmt.Sprintf(" $%s: %s%s", param.Name, paramType, comma)) + } + return lines +} diff --git a/mdl/executor/cmd_microflows_parameter_position_test.go b/mdl/executor/cmd_microflows_parameter_position_test.go new file mode 100644 index 0000000000..ba9af032ec --- /dev/null +++ b/mdl/executor/cmd_microflows_parameter_position_test.go @@ -0,0 +1,129 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// The reported symptom (#993): a parameter is a stored node with real geometry, +// and no annotation reached it — so a generated flow's parameter block landed +// wherever the writer put it, and a hand-aligned one was moved back there by any +// rewrite. Measured before the fix on a real project: a nanoflow parameter at +// -77;0 came back at 200;53 from a describe → exec of mxcli's own output. +// +// DerivedParameterPosition is the arithmetic both writers used inline. It is +// pinned here because the whole design rests on a reader being able to recognise +// it: a parameter sitting exactly there is mxcli's own layout handed back and +// carries no intent, so it is re-derived rather than pinned. +func TestDerivedParameterPositionMatchesTheWriters(t *testing.T) { + for idx, want := range []model.Point{{X: 200, Y: 53}, {X: 300, Y: 53}, {X: 400, Y: 53}} { + if got := microflows.DerivedParameterPosition(idx); got != want { + t.Errorf("DerivedParameterPosition(%d) = %v, want %v", idx, got, want) + } + } +} + +// A stored position that is the derived one carries no intent and must not be +// carried over. Carrying it UNCONDITIONALLY is the #951 mistake one node family +// over: inserting a parameter would leave the existing ones on the old grid +// while the new one lands on top of them. +func TestAuthoredParameterPositionIgnoresTheDerivedSpot(t *testing.T) { + if p := microflows.AuthoredParameterPosition(model.Point{X: 200, Y: 53}, 0); p != nil { + t.Errorf("index 0 at the derived spot: got %v, want nil", *p) + } + if p := microflows.AuthoredParameterPosition(model.Point{X: 300, Y: 53}, 1); p != nil { + t.Errorf("index 1 at the derived spot: got %v, want nil", *p) + } + // The same point at a DIFFERENT index is not the derived one, so it is intent. + got := microflows.AuthoredParameterPosition(model.Point{X: 200, Y: 53}, 1) + if got == nil || *got != (model.Point{X: 200, Y: 53}) { + t.Errorf("index 1 at index 0's spot: got %v, want 200;53 kept", got) + } +} + +// 0;0 is a position a person can choose — two flows in the reference project use +// it — so "unset" cannot be spelled as the zero value. This is why Position is a +// pointer, and the test exists because a bool-free `if p.Position != (Point{})` +// would pass every other case here. +func TestAuthoredParameterPositionKeepsOrigin(t *testing.T) { + got := microflows.AuthoredParameterPosition(model.Point{X: 0, Y: 0}, 0) + if got == nil { + t.Fatal("0;0 was dropped as if unset; it is a position a person can choose") + } + if *got != (model.Point{}) { + t.Errorf("got %v, want 0;0", *got) + } +} + +// DESCRIBE emits the annotation only for an authored position. Emitting the +// derived one would restate mxcli's own arithmetic and pin every parameter of +// every rewritten flow — the round-trip failure startAnnotationLines documents. +func TestParameterPositionAnnotation(t *testing.T) { + derived := µflows.MicroflowParameter{Position: &model.Point{X: 200, Y: 53}} + if got := parameterPositionAnnotation(derived, 0, " "); got != "" { + t.Errorf("derived position emitted %q, want no line", got) + } + authored := µflows.MicroflowParameter{Position: &model.Point{X: -77, Y: 0}} + if got := parameterPositionAnnotation(authored, 0, " "); got != " @position(-77, 0)" { + t.Errorf("got %q, want \" @position(-77, 0)\"", got) + } + if got := parameterPositionAnnotation(µflows.MicroflowParameter{}, 0, " "); got != "" { + t.Errorf("unset position emitted %q, want no line", got) + } +} + +// The control for the fix: with Position dropped on the way in — which is +// exactly what both readers did before #993 — the describer emits nothing and +// the writer has only the index to go on, so the annotation cannot round-trip. +// Without this the suite would pass against a build that never had the fix. +func TestDescribeMicroflowParametersCarriesAuthoredPositionOnly(t *testing.T) { + fmtType := func(*microflows.MicroflowParameter) string { return "Integer" } + params := []*microflows.MicroflowParameter{ + {Name: "A", Type: µflows.IntegerType{}, Position: &model.Point{X: 300, Y: 100}}, + {Name: "B", Type: µflows.IntegerType{}}, + } + got := describeMicroflowParameters(params, fmtType) + want := []string{" @position(300, 100)", " $A: Integer,", " $B: Integer"} + if len(got) != len(want) { + t.Fatalf("got %d lines %q, want %d %q", len(got), got, len(want), want) + } + for i := range want { + if got[i] != want[i] { + t.Errorf("line %d: got %q, want %q", i, got[i], want[i]) + } + } + + // Control: strip the positions, as the pre-fix readers did. + for _, p := range params { + p.Position = nil + } + if got := describeMicroflowParameters(params, fmtType); len(got) != 2 { + t.Errorf("with positions dropped, got %q — the annotation must not appear", got) + } +} + +// An annotation a parameter does not take is refused, not ignored. A typo of +// @position is the case that matters: it parses, does nothing, and discards the +// placement the author was trying to state. +func TestValidateFlowParameterAnnotations(t *testing.T) { + params := []ast.MicroflowParam{ + {Name: "A", UnknownAnnotations: []string{"postion"}}, + {Name: "B", Position: &ast.Position{X: 1, Y: 2}}, + } + got := ValidateFlowParameterAnnotations("nanoflow 'M.NF'", params) + if len(got) != 1 { + t.Fatalf("got %d violations, want 1: %+v", len(got), got) + } + if got[0].RuleID != "MDL059" { + t.Errorf("rule = %s, want MDL059", got[0].RuleID) + } + // Control: a parameter with a valid @position and no unknown names is clean. + if v := ValidateFlowParameterAnnotations("x", params[1:]); len(v) != 0 { + t.Errorf("valid @position flagged: %+v", v) + } +} diff --git a/mdl/executor/cmd_microflows_show.go b/mdl/executor/cmd_microflows_show.go index a4de31ef10..395a98b38c 100644 --- a/mdl/executor/cmd_microflows_show.go +++ b/mdl/executor/cmd_microflows_show.go @@ -251,17 +251,10 @@ func describeMicroflow(ctx *ExecContext, name ast.QualifiedName) error { qualifiedName := name.Module + "." + name.Name if len(targetMf.Parameters) > 0 { lines = append(lines, fmt.Sprintf("create or modify microflow %s (", qualifiedName)) - for i, param := range targetMf.Parameters { - paramType := "Object" - if param.Type != nil { - paramType = formatMicroflowDataType(ctx, param.Type, entityNames) - } - comma := "," - if i == len(targetMf.Parameters)-1 { - comma = "" - } - lines = append(lines, fmt.Sprintf(" $%s: %s%s", param.Name, paramType, comma)) - } + lines = append(lines, describeMicroflowParameters(targetMf.Parameters, + func(p *microflows.MicroflowParameter) string { + return formatMicroflowDataType(ctx, p.Type, entityNames) + })...) lines = append(lines, ")") } else { lines = append(lines, fmt.Sprintf("create or modify microflow %s ()", qualifiedName)) @@ -400,17 +393,10 @@ func describeNanoflow(ctx *ExecContext, name ast.QualifiedName) error { qualifiedName := name.Module + "." + name.Name if len(targetNf.Parameters) > 0 { lines = append(lines, fmt.Sprintf("create or modify nanoflow %s (", qualifiedName)) - for i, param := range targetNf.Parameters { - paramType := "Object" - if param.Type != nil { - paramType = formatMicroflowDataType(ctx, param.Type, entityNames) - } - comma := "," - if i == len(targetNf.Parameters)-1 { - comma = "" - } - lines = append(lines, fmt.Sprintf(" $%s: %s%s", param.Name, paramType, comma)) - } + lines = append(lines, describeMicroflowParameters(targetNf.Parameters, + func(p *microflows.MicroflowParameter) string { + return formatMicroflowDataType(ctx, p.Type, entityNames) + })...) lines = append(lines, ")") } else { lines = append(lines, fmt.Sprintf("create or modify nanoflow %s ()", qualifiedName)) @@ -606,17 +592,10 @@ func renderMicroflowMDL( qualifiedName := name.Module + "." + name.Name if len(mf.Parameters) > 0 { lines = append(lines, fmt.Sprintf("create or modify %s %s (", flowType, qualifiedName)) - for i, param := range mf.Parameters { - paramType := "Object" - if param.Type != nil { - paramType = formatMicroflowDataType(ctx, param.Type, entityNames) - } - comma := "," - if i == len(mf.Parameters)-1 { - comma = "" - } - lines = append(lines, fmt.Sprintf(" $%s: %s%s", param.Name, paramType, comma)) - } + lines = append(lines, describeMicroflowParameters(mf.Parameters, + func(p *microflows.MicroflowParameter) string { + return formatMicroflowDataType(ctx, p.Type, entityNames) + })...) lines = append(lines, ")") } else { lines = append(lines, fmt.Sprintf("create or modify %s %s ()", flowType, qualifiedName)) @@ -1535,17 +1514,10 @@ func describeRule(ctx *ExecContext, name ast.QualifiedName) error { qualifiedName := name.Module + "." + name.Name if len(target.Parameters) > 0 { lines = append(lines, fmt.Sprintf("create or modify rule %s (", qualifiedName)) - for i, param := range target.Parameters { - paramType := "Object" - if param.Type != nil { - paramType = formatMicroflowDataType(ctx, param.Type, entityNames) - } - comma := "," - if i == len(target.Parameters)-1 { - comma = "" - } - lines = append(lines, fmt.Sprintf(" $%s: %s%s", param.Name, paramType, comma)) - } + lines = append(lines, describeMicroflowParameters(target.Parameters, + func(p *microflows.MicroflowParameter) string { + return formatMicroflowDataType(ctx, p.Type, entityNames) + })...) lines = append(lines, ")") } else { lines = append(lines, fmt.Sprintf("create or modify rule %s ()", qualifiedName)) diff --git a/mdl/executor/cmd_nanoflows_create.go b/mdl/executor/cmd_nanoflows_create.go index 0c4dd5ed09..4114fbeed1 100644 --- a/mdl/executor/cmd_nanoflows_create.go +++ b/mdl/executor/cmd_nanoflows_create.go @@ -174,6 +174,7 @@ func execCreateNanoflow(ctx *ExecContext, s *ast.CreateNanoflowStmt) error { ContainerID: nf.ID, Name: p.Name, Type: convertASTToMicroflowDataType(p.Type, entityResolver), + Position: positionFromAST(p.Position), } nf.Parameters = append(nf.Parameters, param) } diff --git a/mdl/executor/cmd_rules_create.go b/mdl/executor/cmd_rules_create.go index 5bc2108295..5cf7327355 100644 --- a/mdl/executor/cmd_rules_create.go +++ b/mdl/executor/cmd_rules_create.go @@ -175,6 +175,7 @@ func execCreateRule(ctx *ExecContext, s *ast.CreateRuleStmt) error { ContainerID: rule.ID, Name: p.Name, Type: convertASTToMicroflowDataType(p.Type, entityResolver), + Position: positionFromAST(p.Position), } rule.Parameters = append(rule.Parameters, param) } diff --git a/mdl/executor/validate_flow_parameters.go b/mdl/executor/validate_flow_parameters.go new file mode 100644 index 0000000000..fce9f9dc30 --- /dev/null +++ b/mdl/executor/validate_flow_parameters.go @@ -0,0 +1,41 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/linter" +) + +// ValidateFlowParameterAnnotations refuses an annotation written on a flow +// parameter that mxcli does not implement there. +// +// Same reasoning as checkUnknownAnnotations one node family over (#884): +// `@position` is the only canvas property a parameter has, so `@postion(300, +// 100)` or `@size(30, 30)` on one would parse, do nothing, and discard exactly +// the placement the author was trying to state — which is the whole reason +// someone writes the annotation at all (#993). +// +// A free function rather than a microflowValidator method because the three +// flow flavours do not share a validator: ValidateMicroflow takes a +// *ast.CreateMicroflowStmt, so a check living there would leave nanoflows and +// rules — which share the parameter grammar — unguarded. +func ValidateFlowParameterAnnotations(flow string, params []ast.MicroflowParam) []linter.Violation { + var out []linter.Violation + for _, p := range params { + for _, name := range p.UnknownAnnotations { + out = append(out, linter.Violation{ + RuleID: "MDL059", + Severity: linter.SeverityError, + Message: fmt.Sprintf("%s: unknown annotation `@%s` on parameter `$%s` — it parses "+ + "but does nothing, so whatever it was meant to express is silently lost", + flow, name, p.Name), + Suggestion: fmt.Sprintf("`@position(x, y)` is the only annotation a parameter takes; "+ + "it places the parameter box on the canvas. If `@%s` is a typo of it, correct it.", name), + }) + } + } + return out +} diff --git a/mdl/executor/validate_program.go b/mdl/executor/validate_program.go index 3dae0e6638..792dd753ee 100644 --- a/mdl/executor/validate_program.go +++ b/mdl/executor/validate_program.go @@ -59,6 +59,18 @@ func ValidateProgram(prog *ast.Program, projectPath string) []linter.Violation { // Check microflow body for common issues if mfStmt, ok := stmt.(*ast.CreateMicroflowStmt); ok { violations = append(violations, ValidateMicroflow(mfStmt)...) + violations = append(violations, + ValidateFlowParameterAnnotations("microflow '"+mfStmt.Name.String()+"'", mfStmt.Parameters)...) + } + // Parameter annotations for the two flow flavours that do not go through + // ValidateMicroflow but share the parameter grammar. + if nfStmt, ok := stmt.(*ast.CreateNanoflowStmt); ok { + violations = append(violations, + ValidateFlowParameterAnnotations("nanoflow '"+nfStmt.Name.String()+"'", nfStmt.Parameters)...) + } + if ruleStmt, ok := stmt.(*ast.CreateRuleStmt); ok { + violations = append(violations, + ValidateFlowParameterAnnotations("rule '"+ruleStmt.Name.String()+"'", ruleStmt.Parameters)...) } // Check workflow for constructs MxBuild rejects (missing page, // single-outcome-with-activities, invalid decision outcome names) diff --git a/mdl/grammar/domains/MDLMicroflow.g4 b/mdl/grammar/domains/MDLMicroflow.g4 index 54667f3992..067aca6d59 100644 --- a/mdl/grammar/domains/MDLMicroflow.g4 +++ b/mdl/grammar/domains/MDLMicroflow.g4 @@ -125,8 +125,11 @@ microflowParameterList : microflowParameter (COMMA microflowParameter)* ; +// Annotations on a parameter: `@position(x, y)` places the parameter box on the +// canvas. Parameters are stored nodes with real geometry, so this is the same +// annotation the statements below it take, attached to the thing it positions. microflowParameter - : (parameterName | VARIABLE) COLON dataType + : annotation* (parameterName | VARIABLE) COLON dataType ; // Allow reserved keywords as parameter names (similar to attributeName) diff --git a/mdl/visitor/microflow_parameter_position_test.go b/mdl/visitor/microflow_parameter_position_test.go new file mode 100644 index 0000000000..f107c36fc0 --- /dev/null +++ b/mdl/visitor/microflow_parameter_position_test.go @@ -0,0 +1,93 @@ +// SPDX-License-Identifier: Apache-2.0 + +package visitor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" +) + +// #993: `@position(x, y)` on a parameter declaration. Before the grammar took +// `annotation*` there, the reporter's script was three parse errors starting at +// "extraneous input '@'". +func TestParameterPositionAnnotationParses(t *testing.T) { + script := `create nanoflow M.NF ( + @position(300, 100) + $A: Integer, + @position(200, 100) + $B: Integer, + $C: Integer +) +returns Integer as $R +begin + @position(300, 200) + declare $R Integer = $A + $B; + @position(500, 200) + return $R; +end;` + prog, errs := Build(script) + if len(errs) != 0 { + t.Fatalf("parse errors: %s", errsText(errs)) + } + stmt, ok := prog.Statements[0].(*ast.CreateNanoflowStmt) + if !ok { + t.Fatalf("statement = %T, want *ast.CreateNanoflowStmt", prog.Statements[0]) + } + if len(stmt.Parameters) != 3 { + t.Fatalf("got %d parameters, want 3", len(stmt.Parameters)) + } + if p := stmt.Parameters[0].Position; p == nil || *p != (ast.Position{X: 300, Y: 100}) { + t.Errorf("$A position = %v, want 300,100", p) + } + if p := stmt.Parameters[1].Position; p == nil || *p != (ast.Position{X: 200, Y: 100}) { + t.Errorf("$B position = %v, want 200,100", p) + } + // Control: an unannotated parameter stays unset, so the layout places it. + // Without this the test would pass against a visitor that stamped every + // parameter with the same point. + if p := stmt.Parameters[2].Position; p != nil { + t.Errorf("$C position = %v, want nil — no annotation was written", *p) + } +} + +// An annotation a parameter does not take is recorded, not dropped, so MDL059 +// can refuse it. `@postion` is the case that matters: silently ignoring it +// discards exactly the placement the author asked for. +func TestUnknownParameterAnnotationIsRecorded(t *testing.T) { + prog, errs := Build(`create nanoflow M.NF ( + @postion(300, 100) + $A: Integer +) begin @position(1, 2) return; end;`) + if len(errs) != 0 { + t.Fatalf("parse errors: %s", errsText(errs)) + } + stmt := prog.Statements[0].(*ast.CreateNanoflowStmt) + got := stmt.Parameters[0].UnknownAnnotations + if len(got) != 1 || got[0] != "postion" { + t.Errorf("UnknownAnnotations = %v, want [postion]", got) + } + if stmt.Parameters[0].Position != nil { + t.Errorf("a typo set a position: %v", *stmt.Parameters[0].Position) + } +} + +// `@position` with too few arguments is not a position — it is recorded as +// unknown rather than silently producing 0;0, which would be a real coordinate +// and would look deliberate. +func TestMalformedParameterPositionIsNotSilentlyZero(t *testing.T) { + prog, errs := Build(`create nanoflow M.NF ( + @position(300) + $A: Integer +) begin @position(1, 2) return; end;`) + if len(errs) != 0 { + t.Fatalf("parse errors: %s", errsText(errs)) + } + p := prog.Statements[0].(*ast.CreateNanoflowStmt).Parameters[0] + if p.Position != nil { + t.Errorf("position = %v, want nil", *p.Position) + } + if len(p.UnknownAnnotations) != 1 { + t.Errorf("UnknownAnnotations = %v, want one entry", p.UnknownAnnotations) + } +} diff --git a/mdl/visitor/visitor_microflow.go b/mdl/visitor/visitor_microflow.go index 2d964ebdac..7b167aa427 100644 --- a/mdl/visitor/visitor_microflow.go +++ b/mdl/visitor/visitor_microflow.go @@ -310,12 +310,46 @@ func buildMicroflowParameters(ctx parser.IMicroflowParameterListContext) []ast.M param.Type = buildMicroflowDataType(dt) } + applyParameterAnnotations(¶m, p.AllAnnotation()) + params = append(params, param) } return params } +// applyParameterAnnotations reads the annotations written on a parameter. +// +// Only @position(x, y) means anything on a parameter — it is a stored node with +// its own coordinates, and nothing else about it is a canvas property. Every +// other name is recorded rather than ignored, so MDL059 can refuse it; an +// annotation that parses and does nothing is the failure mode #884 was about, +// and a typo of `@position` is exactly the case that has to be caught. +func applyParameterAnnotations(param *ast.MicroflowParam, annotations []parser.IAnnotationContext) { + for _, annCtx := range annotations { + ann := annCtx.(*parser.AnnotationContext) + name := strings.ToLower(ann.AnnotationName().GetText()) + if name != "position" { + param.UnknownAnnotations = append(param.UnknownAnnotations, name) + continue + } + params := ann.AnnotationParams() + if params == nil { + param.UnknownAnnotations = append(param.UnknownAnnotations, name) + continue + } + all := params.(*parser.AnnotationParamsContext).AllAnnotationParam() + if len(all) < 2 { + param.UnknownAnnotations = append(param.UnknownAnnotations, name) + continue + } + param.Position = &ast.Position{ + X: parseAnnotationParamInt(all[0]), + Y: parseAnnotationParamInt(all[1]), + } + } +} + // buildMicroflowReturnType converts return type context to MicroflowReturnType. func buildMicroflowReturnType(ctx parser.IMicroflowReturnTypeContext) *ast.MicroflowReturnType { if ctx == nil { diff --git a/sdk/microflows/microflows.go b/sdk/microflows/microflows.go index 1ebe1514bf..562f83a2fa 100644 --- a/sdk/microflows/microflows.go +++ b/sdk/microflows/microflows.go @@ -131,6 +131,46 @@ type MicroflowParameter struct { Name string `json:"name"` Documentation string `json:"documentation,omitempty"` Type DataType `json:"type"` + + // Position is the parameter's place on the canvas, set only when those + // coordinates say something DerivedParameterPosition would not have + // produced — see that function for why the distinction is the whole point. + // nil means "wherever the layout puts it", which is why this is a pointer: + // 0;0 is a position a person can choose, and two flows in the reference + // project use it. + Position *model.Point `json:"position,omitempty"` +} + +// DerivedParameterPosition returns where mxcli's own layout puts the parameter +// at index idx: a row of boxes along the top of the canvas, one spacing unit +// apart. Both writers used to compute this inline and unconditionally, which is +// why a hand-placed parameter did not survive a rewrite (#993). +// +// It exists so that both readers can tell an authored position from mxcli's own +// arithmetic handed back. A parameter sitting exactly here carries no intent and +// is re-derived on the next write; one anywhere else was put there by a person +// and is kept. This is the arbitration authoredStartPosition makes for the +// StartEvent, and it is made for the same reason: carrying stored coordinates +// over UNCONDITIONALLY pins the node, so inserting a parameter would leave the +// existing ones stranded on the old grid while the new one lands on top of them +// (the shape of #951, one node family over). +// +// A person who places a parameter exactly where the layout would have is +// indistinguishable from the layout — and re-deriving gives back the same point, +// so the ambiguity costs nothing. +func DerivedParameterPosition(idx int) model.Point { + return model.Point{X: 200 + idx*100, Y: 53} +} + +// AuthoredParameterPosition returns stored, or nil when stored is the point the +// layout would have derived for index idx. Readers call this so that everything +// downstream can treat a non-nil Position as intent. +func AuthoredParameterPosition(stored model.Point, idx int) *model.Point { + if stored == DerivedParameterPosition(idx) { + return nil + } + p := stored + return &p } // GetName returns the parameter's name. diff --git a/sdk/mpr/microflow_parameter_position_test.go b/sdk/mpr/microflow_parameter_position_test.go new file mode 100644 index 0000000000..ea7a0731b7 --- /dev/null +++ b/sdk/mpr/microflow_parameter_position_test.go @@ -0,0 +1,67 @@ +// SPDX-License-Identifier: Apache-2.0 + +package mpr + +import ( + "testing" + + "go.mongodb.org/mongo-driver/bson" + + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +func rmp(t *testing.T, doc bson.D) string { + t.Helper() + v, _ := doc.Map()["RelativeMiddlePoint"].(string) + return v +} + +// #993: a hand-placed parameter must be written where it was placed. Before the +// fix the legacy serializer computed the position from the index and ignored +// anything stored, so a describe → exec of mxcli's own output moved a real +// parameter from -77;0 to 200;53. +func TestSerializeMicroflowParameterKeepsAuthoredPosition(t *testing.T) { + authored := µflows.MicroflowParameter{ + Name: "Feedback", + Position: &model.Point{X: -77, Y: 0}, + } + if got := rmp(t, serializeMicroflowParameter(authored, 0, 11)); got != "-77;0" { + t.Errorf("authored position = %q, want -77;0", got) + } + + // Control: with no authored position the parameter goes where the layout + // puts it — the behaviour every unannotated flow still relies on. Without + // this the test would pass against a writer that had simply stopped + // deriving. + derived := µflows.MicroflowParameter{Name: "Feedback"} + if got := rmp(t, serializeMicroflowParameter(derived, 0, 11)); got != "200;53" { + t.Errorf("derived position at index 0 = %q, want 200;53", got) + } + if got := rmp(t, serializeMicroflowParameter(derived, 2, 11)); got != "400;53" { + t.Errorf("derived position at index 2 = %q, want 400;53", got) + } +} + +// The reader is where the derived/authored arbitration happens, so that +// everything downstream can treat a non-nil Position as intent. A parameter +// stored on the derived grid must come back unset — carrying it over would pin +// it, and inserting a parameter would then strand the others (#951's shape). +func TestParseMicroflowParameterNormalizesDerivedPosition(t *testing.T) { + raw := func(pos string) map[string]any { + return map[string]any{"Name": "A", "RelativeMiddlePoint": pos} + } + if p := parseMicroflowParameter(raw("200;53"), 0); p.Position != nil { + t.Errorf("derived position came back as %v, want nil", *p.Position) + } + if p := parseMicroflowParameter(raw("300;53"), 1); p.Position != nil { + t.Errorf("derived position at index 1 came back as %v, want nil", *p.Position) + } + p := parseMicroflowParameter(raw("-77;0"), 0) + if p.Position == nil { + t.Fatal("authored position was dropped — this is the #993 read-side loss") + } + if *p.Position != (model.Point{X: -77, Y: 0}) { + t.Errorf("position = %v, want -77;0", *p.Position) + } +} diff --git a/sdk/mpr/parser_microflow.go b/sdk/mpr/parser_microflow.go index c61edee650..1da092d909 100644 --- a/sdk/mpr/parser_microflow.go +++ b/sdk/mpr/parser_microflow.go @@ -82,7 +82,7 @@ func ParseMicroflowFromRaw(raw map[string]any, unitID, containerID model.ID) *mi } for _, p := range extractBsonSlice(paramsArray) { if paramMap := extractBsonMap(p); paramMap != nil { - param := parseMicroflowParameter(paramMap) + param := parseMicroflowParameter(paramMap, len(mf.Parameters)) mf.Parameters = append(mf.Parameters, param) } } @@ -109,7 +109,7 @@ func ParseMicroflowFromRaw(raw map[string]any, unitID, containerID model.ID) *mi for _, obj := range extractBsonSlice(ocRaw["Objects"]) { if objMap := extractBsonMap(obj); objMap != nil { if typeName, _ := objMap["$Type"].(string); typeName == "Microflows$MicroflowParameter" { - param := parseMicroflowParameter(objMap) + param := parseMicroflowParameter(objMap, len(mf.Parameters)) mf.Parameters = append(mf.Parameters, param) } } @@ -237,7 +237,11 @@ func parseCaseValue(raw any) microflows.CaseValue { return nil } -func parseMicroflowParameter(raw map[string]any) *microflows.MicroflowParameter { +// parseMicroflowParameter reads one Microflows$MicroflowParameter. idx is the +// parameter's ordinal in its flow, needed to tell a stored position that says +// something from one that is mxcli's own layout arithmetic handed back — see +// microflows.AuthoredParameterPosition. +func parseMicroflowParameter(raw map[string]any, idx int) *microflows.MicroflowParameter { param := µflows.MicroflowParameter{} // Use extractBsonID to handle binary IDs @@ -255,6 +259,9 @@ func parseMicroflowParameter(raw map[string]any) *microflows.MicroflowParameter } else if pt := extractBsonMap(raw["ParameterType"]); pt != nil { param.Type = parseMicroflowDataType(pt) } + if rmp, ok := raw["RelativeMiddlePoint"]; ok { + param.Position = microflows.AuthoredParameterPosition(parsePoint(rmp), idx) + } return param } diff --git a/sdk/mpr/parser_nanoflow.go b/sdk/mpr/parser_nanoflow.go index 11557eced0..bf60f1b930 100644 --- a/sdk/mpr/parser_nanoflow.go +++ b/sdk/mpr/parser_nanoflow.go @@ -62,7 +62,7 @@ func (r *Reader) parseNanoflow(unitID, containerID string, contents []byte) (*mi } for _, p := range extractBsonSlice(paramsArray) { if paramMap := extractBsonMap(p); paramMap != nil { - param := parseMicroflowParameter(paramMap) + param := parseMicroflowParameter(paramMap, len(nf.Parameters)) nf.Parameters = append(nf.Parameters, param) } } @@ -83,7 +83,7 @@ func (r *Reader) parseNanoflow(unitID, containerID string, contents []byte) (*mi for _, obj := range extractBsonSlice(ocRaw["Objects"]) { if objMap := extractBsonMap(obj); objMap != nil { if typeName, _ := objMap["$Type"].(string); typeName == "Microflows$MicroflowParameter" { - param := parseMicroflowParameter(objMap) + param := parseMicroflowParameter(objMap, len(nf.Parameters)) nf.Parameters = append(nf.Parameters, param) } } diff --git a/sdk/mpr/parser_rule.go b/sdk/mpr/parser_rule.go index 50650ee10c..3ba1f01697 100644 --- a/sdk/mpr/parser_rule.go +++ b/sdk/mpr/parser_rule.go @@ -66,7 +66,7 @@ func (r *Reader) parseRule(unitID, containerID string, contents []byte) (*microf for _, obj := range extractBsonSlice(oc["Objects"]) { if objMap := extractBsonMap(obj); objMap != nil { if typeName, _ := objMap["$Type"].(string); typeName == "Microflows$MicroflowParameter" { - rule.Parameters = append(rule.Parameters, parseMicroflowParameter(objMap)) + rule.Parameters = append(rule.Parameters, parseMicroflowParameter(objMap, len(rule.Parameters))) } } } diff --git a/sdk/mpr/writer_microflow.go b/sdk/mpr/writer_microflow.go index de6fa6a22c..8215263984 100644 --- a/sdk/mpr/writer_microflow.go +++ b/sdk/mpr/writer_microflow.go @@ -325,8 +325,13 @@ func serializeAnnotationFlow(af *microflows.AnnotationFlow, majorVersion int) bs // DefaultValue and IsRequired were introduced in Mendix 10; emitting them on a // Mendix 9 project trips the Studio Pro metamodel checker, so they are gated. func serializeMicroflowParameter(p *microflows.MicroflowParameter, posX int, majorVersion int) bson.D { - // Calculate position based on index - parameters appear at the top of the microflow - relativeMiddlePoint := fmt.Sprintf("%d;53", 200+posX*100) + // An authored position is written as given; without one the parameter goes + // where the layout puts it — a row of boxes along the top of the canvas. + pos := microflows.DerivedParameterPosition(posX) + if p.Position != nil { + pos = *p.Position + } + relativeMiddlePoint := pointToString(pos) doc := bson.D{ {Key: "$ID", Value: idToBsonBinary(string(p.ID))},