diff --git a/docs/01-project/MDL_QUICK_REFERENCE.md b/docs/01-project/MDL_QUICK_REFERENCE.md index 3733d30e..0a4117d5 100644 --- a/docs/01-project/MDL_QUICK_REFERENCE.md +++ b/docs/01-project/MDL_QUICK_REFERENCE.md @@ -215,6 +215,7 @@ authentication basic, session | List declaration | `declare $list list of Module.Entity = empty;` | | | Assignment | `set $Var = expression;` | Variable must be declared first | | Create object | `$Var = create Module.Entity (attr = value);` | | +| Duplicate implicit output | `$Var`, `$Var_2`, `$Var_3` | Describe may alias same-position duplicate outputs for round-trip preservation | | Change object | `change $entity (attr = value);` | | | Commit | `commit $entity [with events] [refresh];` | | | Delete | `delete $entity;` | | diff --git a/docs/11-proposals/PROPOSAL_microflow_variable_alias_collision.md b/docs/11-proposals/PROPOSAL_microflow_variable_alias_collision.md new file mode 100644 index 00000000..a5dd14f2 --- /dev/null +++ b/docs/11-proposals/PROPOSAL_microflow_variable_alias_collision.md @@ -0,0 +1,62 @@ +# Microflow Variable Alias Collision + +Status: Draft + +## Summary + +Document the round-trip-only aliasing rule used when `describe` encounters +multiple implicit output variables with the same name at the same microflow +position. + +When the builder detects a duplicate implicit output at the same canvas point, +the later output is renamed with a numeric suffix: + +```mdl +$Item = create SampleModule.Item (); +$Item = create SampleModule.Item (); -- becomes Item_2 internally +``` + +References emitted after the aliased activity are rewritten to the generated +name (`$Item_2`, `$Item_3`, and so on) so the generated Mendix model remains +valid. + +## Motivation + +Some legacy projects contain duplicated or ambiguous implicit output variables +that Studio Pro can keep in the model but MDL cannot represent as repeated +variables in the same scope without ambiguity. Failing the round-trip would +block describe/exec use on those projects. Silently reusing the first variable +would also be wrong because later changes, returns, or association paths would +target the wrong object. + +The aliasing rule preserves a valid model while making the generated MDL +deterministic and reviewable. + +## Semantics + +- Aliasing is position-scoped. A duplicate implicit output only aliases when + the same variable name is produced at the same `@position(x, y)`. +- The first output keeps its original name. +- Later outputs are renamed to the first available suffix: + `Foo_2`, `Foo_3`, and so on. +- Subsequent variable references and attribute paths are rewritten to the active + alias. +- Moving to a different position resets the alias for that variable name. + +This is primarily a describe/round-trip preservation rule. Authored MDL should +prefer explicit unique variable names. + +## Tests And Examples + +- Builder coverage verifies duplicate implicit outputs are emitted as + `SelectedItem` and `SelectedItem_2`, and that downstream references follow + the alias. +- Example script: + `mdl-examples/doctype-tests/variable_alias_collision.test.mdl`. + +## Open Questions + +- Should the builder fail with an explicit disambiguation error instead of + aliasing silently when authored MDL contains this pattern? +- Should `describe` emit a comment near generated aliases so users can + distinguish model-preservation aliases from names authored manually? diff --git a/docs/11-proposals/README.md b/docs/11-proposals/README.md index 3f997476..923b30de 100644 --- a/docs/11-proposals/README.md +++ b/docs/11-proposals/README.md @@ -43,6 +43,7 @@ BSON schema Registry ◄──── multi-version Support |----------|--------|---------|------------| | [MDL Syntax Improvements v1](PROPOSAL_mdl_syntax_improvements.md) | Draft | Go-style assignment, C-style braces, fluent list APIs | — | | [MDL Syntax Improvements v2](PROPOSAL_mdl_syntax_improvements_v2.md) | Proposed | Consolidated v2: unified variable declaration, C-style braces, fluent list ops | Syntax Improvements v1 | +| [Microflow Variable Alias Collision](PROPOSAL_microflow_variable_alias_collision.md) | Draft | Deterministic `Foo_2` aliases for duplicate implicit outputs at the same microflow position | — | | [Page Syntax V2](PROPOSAL_page_syntax_v2.md) | Superseded | Page/widget syntax with `{}` blocks and `->` binding. Superseded by V3 (archived) | — | | [Page Styling Support](page-styling-support.md) | Partial | CSS classes, inline styles, dynamic classes, design properties. Phase 1 (Class/Style) done | — | | [Page Composition](proposal_page_composition.md) | Proposed | Fragment definitions and ALTER PAGE for partial page editing | Page Syntax V2, Page Styling | diff --git a/mdl-examples/bug-tests/variable-alias-collision.mdl b/mdl-examples/bug-tests/variable-alias-collision.mdl new file mode 100644 index 00000000..36073117 --- /dev/null +++ b/mdl-examples/bug-tests/variable-alias-collision.mdl @@ -0,0 +1,52 @@ +-- ============================================================================ +-- Variable alias collision: duplicate implicit output at same position +-- ============================================================================ +-- +-- Symptom (before fix): +-- When a Studio Pro project contained two activities producing the same +-- implicit output variable at the same canvas position (e.g. two +-- `Create Object` actions both writing `$Item` at `@position(100, 200)`), +-- describing it produced MDL with two `$Item =` assignments and the +-- builder collapsed both into one shared variable. Downstream `change` +-- and `return` statements then operated on the wrong object, the +-- resulting BSON was non-equivalent to the original, and roundtrip +-- verification failed. +-- +-- After fix: +-- The flow builder now tracks implicit output positions. When it sees +-- the same variable name produced again at the same `@position(x, y)`, +-- it assigns a deterministic suffix (`Foo_2`, `Foo_3`, …) and rewrites +-- subsequent variable references, attribute paths, change targets, and +-- retrieve start variables through the active alias. Roundtrip +-- describe→exec→describe is a fixpoint and the resulting model is valid. +-- +-- Usage: +-- mxcli exec mdl-examples/bug-tests/variable-alias-collision.mdl -p app.mpr +-- mxcli -p app.mpr -c "describe microflow SampleAliases.ACT_DuplicateOutputPosition" +-- The describe output must contain `$Item_2` for the second create and +-- for the downstream `change`/`return`. Studio Pro `mx check` must +-- report 0 errors. +-- ============================================================================ + +create module SampleAliases; + +create entity SampleAliases.Item ( + Name : string(100) +); +/ + +create microflow SampleAliases.ACT_DuplicateOutputPosition () +returns SampleAliases.Item as $Item +begin + @position(100, 200) + $Item = create SampleAliases.Item (Name = 'first'); + + @position(100, 200) + $Item = create SampleAliases.Item (Name = 'second'); + + change $Item ( + Name = $Item/Name + ' updated'); + + return $Item; +end; +/ diff --git a/mdl-examples/doctype-tests/variable_alias_collision.test.mdl b/mdl-examples/doctype-tests/variable_alias_collision.test.mdl new file mode 100644 index 00000000..9b5ac9f4 --- /dev/null +++ b/mdl-examples/doctype-tests/variable_alias_collision.test.mdl @@ -0,0 +1,17 @@ +create microflow SampleAliases.ACT_DuplicateOutputPosition () +returns SampleAliases.Item as $Item +begin + @position(100, 200) + $Item = create SampleAliases.Item ( + Name = 'first'); + + @position(100, 200) + $Item = create SampleAliases.Item ( + Name = 'second'); + + change $Item ( + Name = $Item/Name + ' updated'); + + return $Item; +end; +/ diff --git a/mdl/executor/cmd_microflows_builder.go b/mdl/executor/cmd_microflows_builder.go index f4d5f4fd..be8f6fb1 100644 --- a/mdl/executor/cmd_microflows_builder.go +++ b/mdl/executor/cmd_microflows_builder.go @@ -46,6 +46,8 @@ type flowBuilder struct { // just emitted an activity, so the next flow's OriginConnectionIndex can // be overridden by the user. Cleared after each flow is created. previousStmtAnchor *ast.FlowAnchors + variableAliases map[string]string + outputVarPositions map[string]model.Point } // addError records a validation error during flow building. @@ -133,6 +135,52 @@ func (fb *flowBuilder) registerResultVariableType(varName string, dt microflows. } } +func (fb *flowBuilder) resolveVariableName(varName string) string { + if varName == "" || fb.variableAliases == nil { + return varName + } + if alias := fb.variableAliases[varName]; alias != "" { + return alias + } + return varName +} + +func (fb *flowBuilder) resolveVariablePath(path string) string { + if before, after, ok := strings.Cut(path, "/"); ok { + return fb.resolveVariableName(before) + "/" + after + } + return fb.resolveVariableName(path) +} + +func (fb *flowBuilder) uniqueImplicitOutputVariable(varName string) string { + if varName == "" { + return "" + } + if fb.outputVarPositions == nil { + fb.outputVarPositions = make(map[string]model.Point) + } + position := model.Point{X: fb.posX, Y: fb.posY} + if previous, ok := fb.outputVarPositions[varName]; ok && + previous.X == position.X && previous.Y == position.Y && + fb.isVariableDeclared(varName) { + if fb.variableAliases == nil { + fb.variableAliases = make(map[string]string) + } + for i := 2; ; i++ { + candidate := fmt.Sprintf("%s_%d", varName, i) + if !fb.isVariableDeclared(candidate) { + fb.variableAliases[varName] = candidate + return candidate + } + } + } + if fb.variableAliases != nil { + delete(fb.variableAliases, varName) + } + fb.outputVarPositions[varName] = position + return varName +} + // lookupMicroflowReturnType resolves the return type of a called microflow by // qualified name so downstream activities can infer variable types. func (fb *flowBuilder) lookupMicroflowReturnType(qualifiedName string) microflows.DataType { @@ -231,10 +279,12 @@ func (fb *flowBuilder) resolveAssociationPaths(expr ast.Expression) ast.Expressi } switch e := expr.(type) { + case *ast.VariableExpr: + return &ast.VariableExpr{Name: fb.resolveVariableName(e.Name)} case *ast.AttributePathExpr: resolved := fb.resolvePathSegments(e.Path) return &ast.AttributePathExpr{ - Variable: e.Variable, + Variable: fb.resolveVariableName(e.Variable), Path: resolved, Segments: e.Segments, } diff --git a/mdl/executor/cmd_microflows_builder_actions.go b/mdl/executor/cmd_microflows_builder_actions.go index 9559eea2..5a54ae33 100644 --- a/mdl/executor/cmd_microflows_builder_actions.go +++ b/mdl/executor/cmd_microflows_builder_actions.go @@ -54,16 +54,17 @@ func (fb *flowBuilder) addCreateVariableAction(s *ast.DeclareStmt) model.ID { // addChangeVariableAction creates a SET statement as a ChangeVariableAction. func (fb *flowBuilder) addChangeVariableAction(s *ast.MfSetStmt) model.ID { + target := fb.resolveVariablePath(s.Target) // Validate that the variable has been declared - if !fb.isVariableDeclared(s.Target) { + if !fb.isVariableDeclared(target) { fb.addErrorWithExample( - fmt.Sprintf("variable '%s' is not declared", s.Target), - errorExampleDeclareVariable(s.Target)) + fmt.Sprintf("variable '%s' is not declared", target), + errorExampleDeclareVariable(target)) } action := µflows.ChangeVariableAction{ BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, - VariableName: s.Target, + VariableName: target, Value: fb.exprToString(s.Value), } @@ -86,9 +87,10 @@ func (fb *flowBuilder) addChangeVariableAction(s *ast.MfSetStmt) model.ID { // addCreateObjectAction creates a CREATE OBJECT statement. func (fb *flowBuilder) addCreateObjectAction(s *ast.CreateObjectStmt) model.ID { + outputVariable := fb.uniqueImplicitOutputVariable(s.Variable) action := µflows.CreateObjectAction{ BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, - OutputVariable: s.Variable, + OutputVariable: outputVariable, Commit: microflows.CommitTypeNo, } // Set entity reference as qualified name (BY_NAME_REFERENCE) @@ -100,7 +102,7 @@ func (fb *flowBuilder) addCreateObjectAction(s *ast.CreateObjectStmt) model.ID { // Register variable type for CHANGE statements if fb.varTypes != nil && entityQN != "" { - fb.varTypes[s.Variable] = entityQN + fb.varTypes[outputVariable] = entityQN } // Build InitialMembers for each SET assignment @@ -239,9 +241,10 @@ func (fb *flowBuilder) addRollbackAction(s *ast.RollbackStmt) model.ID { // addChangeObjectAction creates a CHANGE statement. func (fb *flowBuilder) addChangeObjectAction(s *ast.ChangeObjectStmt) model.ID { + changeVariable := fb.resolveVariableName(s.Variable) action := µflows.ChangeObjectAction{ BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, - ChangeVariable: s.Variable, + ChangeVariable: changeVariable, Commit: microflows.CommitTypeNo, RefreshInClient: false, } @@ -249,7 +252,7 @@ func (fb *flowBuilder) addChangeObjectAction(s *ast.ChangeObjectStmt) model.ID { // Look up entity type from variable scope entityQN := "" if fb.varTypes != nil { - entityQN = fb.varTypes[s.Variable] + entityQN = fb.varTypes[changeVariable] } // Build MemberChange items for each SET assignment @@ -283,6 +286,7 @@ func (fb *flowBuilder) addChangeObjectAction(s *ast.ChangeObjectStmt) model.ID { // addRetrieveAction creates a RETRIEVE statement. func (fb *flowBuilder) addRetrieveAction(s *ast.RetrieveStmt) model.ID { var source microflows.RetrieveSource + outputVariable := fb.uniqueImplicitOutputVariable(s.Variable) if s.StartVariable != "" { // Association retrieve: RETRIEVE $List FROM $Parent/Module.AssocName @@ -296,7 +300,7 @@ func (fb *flowBuilder) addRetrieveAction(s *ast.RetrieveStmt) model.ID { assocInfo := fb.lookupAssociation(s.Source.Module, s.Source.Name) startVarType := "" if fb.varTypes != nil { - startVarType = fb.varTypes[s.StartVariable] + startVarType = fb.varTypes[fb.resolveVariableName(s.StartVariable)] } if assocInfo != nil && assocInfo.Type == domainmodel.AssociationTypeReference && @@ -306,17 +310,17 @@ func (fb *flowBuilder) addRetrieveAction(s *ast.RetrieveStmt) model.ID { dbSource := µflows.DatabaseRetrieveSource{ BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, EntityQualifiedName: assocInfo.parentEntityQN, - XPathConstraint: "[" + assocQN + " = $" + s.StartVariable + "]", + XPathConstraint: "[" + assocQN + " = $" + fb.resolveVariableName(s.StartVariable) + "]", } source = dbSource if fb.varTypes != nil { - fb.varTypes[s.Variable] = "List of " + assocInfo.parentEntityQN + fb.varTypes[outputVariable] = "List of " + assocInfo.parentEntityQN } } else { // Forward traversal or ReferenceSet: use AssociationRetrieveSource source = µflows.AssociationRetrieveSource{ BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, - StartVariable: s.StartVariable, + StartVariable: fb.resolveVariableName(s.StartVariable), AssociationQualifiedName: assocQN, } if fb.varTypes != nil { @@ -326,10 +330,10 @@ func (fb *flowBuilder) addRetrieveAction(s *ast.RetrieveStmt) model.ID { if startVarType == assocInfo.childEntityQN { otherEntity = assocInfo.parentEntityQN } - fb.varTypes[s.Variable] = otherEntity + fb.varTypes[outputVariable] = otherEntity } else { // ReferenceSet or unknown: returns a list - fb.varTypes[s.Variable] = "List of " + assocQN + fb.varTypes[outputVariable] = "List of " + assocQN } } } @@ -403,17 +407,17 @@ func (fb *flowBuilder) addRetrieveAction(s *ast.RetrieveStmt) model.ID { if fb.varTypes != nil { if s.Limit == "1" { // LIMIT 1 returns a single entity - fb.varTypes[s.Variable] = entityQN + fb.varTypes[outputVariable] = entityQN } else { // No LIMIT or LIMIT > 1 returns a list - fb.varTypes[s.Variable] = "List of " + entityQN + fb.varTypes[outputVariable] = "List of " + entityQN } } } action := µflows.RetrieveAction{ BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, - OutputVariable: s.Variable, + OutputVariable: outputVariable, Source: source, } diff --git a/mdl/executor/cmd_microflows_builder_variable_alias_test.go b/mdl/executor/cmd_microflows_builder_variable_alias_test.go new file mode 100644 index 00000000..d52ab00e --- /dev/null +++ b/mdl/executor/cmd_microflows_builder_variable_alias_test.go @@ -0,0 +1,76 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +func TestBuildFlowGraph_DuplicateImplicitOutputAtSamePositionGetsLocalAlias(t *testing.T) { + entityRef := ast.QualifiedName{Module: "Sample", Name: "Item"} + sharedPosition := &ast.ActivityAnnotations{Position: &ast.Position{X: 400, Y: 100}} + body := []ast.MicroflowStatement{ + &ast.CreateObjectStmt{ + Variable: "SelectedItem", + EntityType: entityRef, + Annotations: sharedPosition, + }, + &ast.CreateObjectStmt{ + Variable: "SelectedItem", + EntityType: entityRef, + Annotations: sharedPosition, + }, + &ast.ChangeObjectStmt{ + Variable: "SelectedItem", + Changes: []ast.ChangeItem{{ + Attribute: "Name", + Value: &ast.VariableExpr{Name: "SelectedItem"}, + }}, + }, + &ast.ReturnStmt{Value: &ast.VariableExpr{Name: "SelectedItem"}}, + } + + fb := &flowBuilder{ + posX: 100, posY: 100, baseY: 100, spacing: HorizontalSpacing, + varTypes: map[string]string{}, + declaredVars: map[string]string{}, + } + oc := fb.buildFlowGraph(body, &ast.MicroflowReturnType{Type: ast.DataType{Kind: ast.TypeEntity, EntityRef: &entityRef}}) + + createOutputs := map[string]bool{} + var changeVariable, changeValue string + var returnValue string + for _, obj := range oc.Objects { + switch o := obj.(type) { + case *microflows.ActionActivity: + switch action := o.Action.(type) { + case *microflows.CreateObjectAction: + createOutputs[action.OutputVariable] = true + case *microflows.ChangeObjectAction: + changeVariable = action.ChangeVariable + if len(action.Changes) == 1 { + changeValue = action.Changes[0].Value + } + } + case *microflows.EndEvent: + returnValue = strings.TrimSpace(o.ReturnValue) + } + } + + if !createOutputs["SelectedItem"] || !createOutputs["SelectedItem_2"] { + t.Fatalf("duplicate implicit output should be aliased, got create outputs %#v", createOutputs) + } + if changeVariable != "SelectedItem_2" { + t.Fatalf("change target = %q, want SelectedItem_2", changeVariable) + } + if changeValue != "$SelectedItem_2" { + t.Fatalf("change value = %q, want $SelectedItem_2", changeValue) + } + if returnValue != "$SelectedItem_2" { + t.Fatalf("return value = %q, want $SelectedItem_2", returnValue) + } +}