diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-10-22-branch-error-handler-rejoins-outside-branch.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-22-branch-error-handler-rejoins-outside-branch.json new file mode 100644 index 0000000000..acd5963622 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-22-branch-error-handler-rejoins-outside-branch.json @@ -0,0 +1 @@ +{"area": "mdl/executor (microflow builder)", "date": "2026-10-22", "symptom": "A fall-through custom error handler (no return / raise error) on an activity inside an IF, CASE or split type branch was wired outside the branch. With a result variable it rejoined on the flow INTO the split, so a failing call re-ran the decision and called again (a loop; DESCRIBE shows `merge rejoin1;` before the `if`). Without one it ended the microflow at an end event of its own (DESCRIBE shows a `return;` the script did not contain). In a branch ending in return it rejoined after the IF. `mx check` reports 0 errors for all of them (mendixlabs/mxcli#1565).", "cause": "The builder keeps the handler pending until the next flow is drawn from the failing activity. buildFlowGraph settles it on every flow it draws, but the branch loops settled only an EMPTY handler (`if fb.emptyErrorHandlerFrom == lastID`), and the IF-without-ELSE and split type loops not even that, so a handler with a body left the branch pending. The parent draws the flow into the split only after building it, and a handler with a result variable settles on the next flow whatever its origin: that flow was the one into the split. Without a result variable nothing matched, and the end-of-flow pass gave it an end event.", "file": "mdl/executor/cmd_microflows_builder_flows.go (enterBranchErrorHandlers / takeBranchErrorHandlers / settleBranchErrorHandlersOn / addBranchErrorHandlerFlowForStatement / queueBranchErrorHandlersAt; addPendingErrorHandlerFlowTo removed), cmd_microflows_builder_control.go (IF: all three branch loops settle on each flow and on the exit flow to the merge; leftovers rejoin on the flow leaving the IF; thenNeedsErrorMerge removed), cmd_microflows_builder_actions.go (CASE and split type branches); test cmd_microflows_builder_branch_handler_test.go; example mdl-examples/bug-tests/1565-branch-error-handler-rejoin.mdl", "insight": "Making the branch loops settle on every flow is not enough on its own. (1) The handlers pending when a split starts belong to the activity BEFORE it, and a result-variable handler settles on any flow, so it would rejoin in the middle of the first branch: each branch sets the outer handlers aside and restores them. (2) In a branch that ends in return, a result-variable handler is deliberately sent to the code after the split, the shared error path of a success tail built on the result (TestBuildFlowGraph_OutputHandlerInReturningBranchSkipsDerivedSuccessTail); left pending, it would settle on the next flow the parent draws, the one INTO the split. Such leftovers are pinned to the split exit (merge, continuing branch, or the split itself when every branch returns) by making that exit their source and clearing the skip variable, so they settle on the flow leaving the split. Assert the graph (where the handler tail reaches through merges, end-event count, no cycle), not mx check, which accepts every broken shape.", "refs": ["mendixlabs/mxcli#1565"], "ce": []} diff --git a/CHANGELOG.md b/CHANGELOG.md index ea8ca9c59d..47b8cce384 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **An error handler inside a branch continues after its failing activity** (mendixlabs/mxcli#1565) — a custom error handler that does not end in `return` or `raise error`, on an activity inside an IF, CASE or split type branch, was wired outside the branch. With a result variable (`$N = call microflow … on error without rollback begin … end error;`) it rejoined on the flow into the split, so a failing call re-ran the decision and called again, for as long as it kept failing; without one it ended the microflow at an end event of its own, skipping the rest of the branch and everything after it; in a branch ending in `return` it skipped to the code after the IF. `mx check` accepts all three. Each branch now rejoins its own handlers right after the failing activity, as the top level does, or on the branch's way out when the activity is its last. In a branch that ends in `return`, a handler on an activity with a result still goes to the code after the split, the branch's shared error path, but on the flow leaving the split rather than the one into it. A handler on an activity before the split is unchanged. The third case is a behaviour change for a handler on an activity without a result: it now runs the rest of its branch, `return` included, rather than the code after the IF. - **A specialization of a non-persistent entity reads as non-persistent** (mendixlabs/mxcli#1600) — `describe entity Probe.Child` printed `create or modify persistent entity Probe.Child extends Probe.Base (` when `Probe.Base` is non-persistent, so a reader took the child for a table and the output stated a combination Mendix does not have. A specialization stores no persistable flag of its own; it takes its root generalization's, and the reader defaulted it to persistent. The flag is now resolved along the generalization chain, across modules and into the System module (whose `HttpRequest`, `HttpResponse` and `SoapFault` read persistent for the same reason), and the fix reaches everything that reads it: `describe`, the catalog's `EntityType`, the domain-model diagrams, and `alter entities … where persistent`, which used to add its attribute to such a child as well. Re-running the describe output leaves the entity unchanged. - **`go mod tidy` runs again, and the expression round-trip test runs for the first time** — `mdl/exprcheck/roundtrip_test.go` imported `mdl/backend/mpr` (the legacy engine, deleted) plus `executor.GetHierarchyForMining` and `executor.DescribeMicroflowToString` (both removed). It never broke a build because it sat behind `//go:build roundtrip`, a tag no Makefile target and no CI job has ever passed — the only file in the tree using it. `go mod tidy` considers every build tag, so it was the one command that still saw the file, and it failed with "no matching versions for query latest" on the deleted package, which reads as a dependency problem rather than a dead test. The test is ported to `mdl/executor/exprcheck_roundtrip_test.go` under the `integration` tag, where `setupTestEnv`/`describeMDL` already provide the real-project harness and `make test-integration-executor` actually runs it; the removed hierarchy helper is replaced by building the container-to-module map from `backend.ListModules()`. Its assertion is worth keeping: describing every microflow and re-parsing each expression with the robust parser catches grammar gaps — a missing `parsePrimary` branch, a missing `funcTable` entry, a missing slot constraint — that the unit tests cannot, since those only feed the parser expressions someone thought to write down. It now also fails rather than passing silently when the fixture project yields no microflows or no expression slots. diff --git a/mdl-examples/bug-tests/1565-branch-error-handler-rejoin.mdl b/mdl-examples/bug-tests/1565-branch-error-handler-rejoin.mdl new file mode 100644 index 0000000000..fa03f91895 --- /dev/null +++ b/mdl-examples/bug-tests/1565-branch-error-handler-rejoin.mdl @@ -0,0 +1,92 @@ +mdl 1; +-- ============================================================================ +-- mendixlabs/mxcli#1565: a fall-through error handler inside a branch +-- ============================================================================ +-- +-- A custom error handler that does not end in `return` or `raise error` +-- continues with the statement after the failing activity. Inside a branch +-- (IF, CASE, split type) it was left pending past the branch: +-- +-- * with a result variable it rejoined on the flow INTO the split, so a +-- failing call re-ran the decision and called again: a loop for as long +-- as the call kept failing (DESCRIBE: `merge rejoin1;` before the `if`); +-- * without one it ended the microflow at an end event of its own, so the +-- rest of the branch and everything after it never ran (DESCRIBE: a +-- `return;` in the handler that the script did not contain); +-- * in a branch ending in `return`, it rejoined after the IF instead. +-- +-- `mx check` reports 0 errors for all three, so only the flow shows it. After +-- the fix, DESCRIBE prints each handler without `return;` or `join`, with the +-- rejoin merge folded onto the failing call as `@merge(x, y)`, and re-running +-- the DESCRIBE output changes nothing. + +create module BugTest1565; + +create enumeration BugTest1565.Color (Red 'Red', Green 'Green'); + +create microflow BugTest1565.Sub () returns Integer +begin + return 1; +end; + +create microflow BugTest1565.SubNoResult () +begin + log info node 'BugTest1565' 'sub'; +end; + +-- Rejoins at 'after call'; no loop back to the split. +create microflow BugTest1565.WithResult ($Flag: Boolean) +begin + log info node 'BugTest1565' 'before'; + if $Flag then + $N = call microflow BugTest1565.Sub() on error without rollback begin + log warning node 'BugTest1565' 'failed'; + end error; + log info node 'BugTest1565' 'after call'; + end if; + log info node 'BugTest1565' 'end'; +end; + +-- Rejoins at 'after call'; one end event. +create microflow BugTest1565.WithoutResult ($Flag: Boolean) +begin + log info node 'BugTest1565' 'before'; + if $Flag then + call microflow BugTest1565.SubNoResult() on error without rollback begin + log warning node 'BugTest1565' 'failed'; + end error; + log info node 'BugTest1565' 'after call'; + end if; + log info node 'BugTest1565' 'end'; +end; + +-- Rejoins at 'after call', inside the branch that returns. +create microflow BugTest1565.ReturningBranch ($Flag: Boolean) returns Boolean +begin + if $Flag then + call microflow BugTest1565.SubNoResult() on error without rollback begin + log warning node 'BugTest1565' 'failed'; + end error; + log info node 'BugTest1565' 'after call'; + return true; + else + log info node 'BugTest1565' 'else'; + end if; + log info node 'BugTest1565' 'end'; + return false; +end; + +-- Rejoins at 'after call' in the CASE branch. +create microflow BugTest1565.CaseBranch ($C: BugTest1565.Color) +begin + case $C + when Red then + call microflow BugTest1565.SubNoResult() on error without rollback begin + log warning node 'BugTest1565' 'failed'; + end error; + log info node 'BugTest1565' 'after call'; + when Green, (empty) then + log info node 'BugTest1565' 'green'; + end case; + log info node 'BugTest1565' 'end'; +end; diff --git a/mdl/executor/cmd_microflows_builder_actions.go b/mdl/executor/cmd_microflows_builder_actions.go index 818487e212..15fafe7157 100644 --- a/mdl/executor/cmd_microflows_builder_actions.go +++ b/mdl/executor/cmd_microflows_builder_actions.go @@ -570,6 +570,7 @@ func (fb *flowBuilder) addEnumSplit(s *ast.EnumSplitStmt) model.ID { savedEndsWithReturn := fb.endsWithReturn allBranchesReturn := len(branches) > 0 + var caseHandlers []pendingErrorHandlerState for i, br := range branches { branchY := branchYs[i] fb.posX = splitX + SplitWidth + HorizontalSpacing/2 @@ -579,6 +580,7 @@ func (fb *flowBuilder) addEnumSplit(s *ast.EnumSplitStmt) model.ID { lastID := model.ID("") pendingCase := "" var prevAnchor *ast.FlowAnchors + outerHandlers := fb.enterBranchErrorHandlers() for j, stmt := range br.body { thisAnchor := stmtOwnAnchor(stmt) actID := fb.addStatement(stmt) @@ -629,9 +631,7 @@ func (fb *flowBuilder) addEnumSplit(s *ast.EnumSplitStmt) model.ID { } applyUserAnchors(flow, prevAnchor, thisAnchor) fb.flows = append(fb.flows, flow) - if fb.emptyErrorHandlerFrom == lastID { - fb.addPendingErrorHandlerFlowForStatement(lastID, actID, stmt, statementsReferenceVar(br.body[j+1:], fb.errorHandlerSkipVar)) - } + fb.addBranchErrorHandlerFlowForStatement(lastStmtIsReturn(br.body), lastID, actID, stmt, statementsReferenceVar(br.body[j+1:], fb.errorHandlerSkipVar)) } prevAnchor = thisAnchor if fb.nextConnectionPoint != "" { @@ -648,6 +648,7 @@ func (fb *flowBuilder) addEnumSplit(s *ast.EnumSplitStmt) model.ID { } if lastStmtIsReturn(br.body) { + caseHandlers = append(caseHandlers, fb.takeBranchErrorHandlers(outerHandlers)...) continue } allBranchesReturn = false @@ -664,7 +665,9 @@ func (fb *flowBuilder) addEnumSplit(s *ast.EnumSplitStmt) model.ID { tail.DestinationConnectionIndex = mergeSideFor(fb.objectPosition(lastID), fb.objectPosition(ensureMerge().ID)) } fb.flows = append(fb.flows, tail) + fb.addPendingErrorHandlerFlowForStatement(lastID, ensureMerge().ID, nil) } + caseHandlers = append(caseHandlers, fb.takeBranchErrorHandlers(outerHandlers)...) } fb.posX = mergeX + HorizontalSpacing/2 @@ -678,6 +681,12 @@ func (fb *flowBuilder) addEnumSplit(s *ast.EnumSplitStmt) model.ID { // merge — as on an IF (#767). It was accepted and dropped (#1587). fb.nextFlowAnchor = mergeExitFlowAnchor(s) } + // The flow leaving the split: see queueBranchErrorHandlersAt. + exitID := splitID + if !allBranchesReturn { + exitID = ensureMerge().ID + } + fb.queueBranchErrorHandlersAt(caseHandlers, exitID) return splitID } @@ -748,11 +757,15 @@ func (fb *flowBuilder) addStructuredInheritanceSplit(s *ast.InheritanceSplitStmt fromSplit bool order int anchor *ast.FlowAnchors + // handlers are the branch's error handlers still to rejoin, on the + // flow from id to the merge. + handlers []pendingErrorHandlerState } var branchTails []branchTail savedEndsWithReturn := fb.endsWithReturn allBranchesReturn := len(s.Cases) > 0 && len(s.ElseBody) > 0 + var splitHandlers []pendingErrorHandlerState branchIndex := 0 addBranch := func(caseValue string, body []ast.MicroflowStatement) { @@ -772,7 +785,8 @@ func (fb *flowBuilder) addStructuredInheritanceSplit(s *ast.InheritanceSplitStmt var lastID model.ID var prevAnchor *ast.FlowAnchors pendingCase := "" - for _, stmt := range body { + outerHandlers := fb.enterBranchErrorHandlers() + for j, stmt := range body { thisAnchor := stmtOwnAnchor(stmt) actID := fb.addStatement(stmt) if fb.takeBranchJoin(lastID, splitID, caseValue, pendingCase, prevAnchor, nil) { @@ -808,6 +822,7 @@ func (fb *flowBuilder) addStructuredInheritanceSplit(s *ast.InheritanceSplitStmt applyUserAnchors(flow, prevAnchor, thisAnchor) fb.flows = append(fb.flows, flow) } + fb.addBranchErrorHandlerFlowForStatement(lastStmtIsReturn(body), lastID, actID, stmt, statementsReferenceVar(body[j+1:], fb.errorHandlerSkipVar)) } prevAnchor = thisAnchor if fb.nextConnectionPoint != "" { @@ -826,9 +841,11 @@ func (fb *flowBuilder) addStructuredInheritanceSplit(s *ast.InheritanceSplitStmt if !lastStmtIsReturn(body) { allBranchesReturn = false if lastID != "" { - branchTails = append(branchTails, branchTail{id: lastID, caseValue: pendingCase, anchor: prevAnchor}) + branchTails = append(branchTails, branchTail{id: lastID, caseValue: pendingCase, anchor: prevAnchor, handlers: fb.takeBranchErrorHandlers(outerHandlers)}) + return } } + splitHandlers = append(splitHandlers, fb.takeBranchErrorHandlers(outerHandlers)...) } for _, c := range s.Cases { @@ -864,6 +881,8 @@ func (fb *flowBuilder) addStructuredInheritanceSplit(s *ast.InheritanceSplitStmt fb.posX = mergeX + MergeSize + HorizontalSpacing/2 fb.posY = centerY fb.endsWithReturn = savedEndsWithReturn + // The flow leaving the split: see queueBranchErrorHandlersAt. + exitID := splitID if allBranchesReturn { fb.endsWithReturn = true } else if len(branchTails) > 0 { @@ -895,10 +914,13 @@ func (fb *flowBuilder) addStructuredInheritanceSplit(s *ast.InheritanceSplitStmt applyUserAnchors(flow, tail.anchor, nil) fb.flows = append(fb.flows, flow) } + splitHandlers = append(splitHandlers, fb.settleBranchErrorHandlersOn(tail.handlers, tail.id, merge.ID)...) } } fb.nextConnectionPoint = merge.ID + exitID = merge.ID } + fb.queueBranchErrorHandlersAt(splitHandlers, exitID) return splitID } diff --git a/mdl/executor/cmd_microflows_builder_branch_handler_test.go b/mdl/executor/cmd_microflows_builder_branch_handler_test.go new file mode 100644 index 0000000000..82c26a1324 --- /dev/null +++ b/mdl/executor/cmd_microflows_builder_branch_handler_test.go @@ -0,0 +1,342 @@ +package executor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// A fall-through custom error handler on an activity inside a branch continues +// with the statement after the failing activity, as it does at the top level +// (mendixlabs/mxcli#1565). It rejoined on the flow INTO the split instead — a +// retry loop for as long as the call kept failing — when the call assigned a +// variable, and ended the microflow at an end event of its own when it did not. +func TestBranchErrorHandlerRejoinsAfterTheFailingActivity(t *testing.T) { + cases := []struct { + name string + src string + // rejoin is the log the handler's tail must reach through merges only. + rejoin string + ends int + }{ + { + name: "if without else, call with a result", + src: `create microflow M.A ($Flag: Boolean) +begin + log info node 'P' 'before'; + if $Flag then + $N = call microflow M.Sub() on error without rollback begin + log warning node 'P' 'failed'; + end error; + log info node 'P' 'after call'; + end if; + log info node 'P' 'end'; +end;`, + rejoin: "after call", + ends: 1, + }, + { + name: "if without else, call without a result", + src: `create microflow M.B ($Flag: Boolean) +begin + log info node 'P' 'before'; + if $Flag then + call microflow M.Sub() on error without rollback begin + log warning node 'P' 'failed'; + end error; + log info node 'P' 'after call'; + end if; + log info node 'P' 'end'; +end;`, + rejoin: "after call", + ends: 1, + }, + { + name: "then branch that returns", + src: `create microflow M.C ($Flag: Boolean) returns Boolean +begin + if $Flag then + call microflow M.Sub() on error without rollback begin + log warning node 'P' 'failed'; + end error; + log info node 'P' 'after call'; + return true; + else + log info node 'P' 'else'; + end if; + log info node 'P' 'end'; + return false; +end;`, + rejoin: "after call", + ends: 2, + }, + { + name: "else branch", + src: `create microflow M.D ($Flag: Boolean) returns Boolean +begin + if $Flag then + return true; + else + call microflow M.Sub() on error without rollback begin + log warning node 'P' 'failed'; + end error; + log info node 'P' 'after call'; + end if; + log info node 'P' 'end'; + return false; +end;`, + rejoin: "after call", + ends: 2, + }, + { + name: "handler on the branch's last activity", + src: `create microflow M.E ($Flag: Boolean) +begin + if $Flag then + log info node 'P' 'first'; + $N = call microflow M.Sub() on error without rollback begin + log warning node 'P' 'failed'; + end error; + end if; + log info node 'P' 'end'; +end;`, + rejoin: "end", + ends: 1, + }, + { + name: "result used by the next branch statement", + src: `create microflow M.F ($Flag: Boolean) +begin + if $Flag then + $N = call microflow M.Sub() on error without rollback begin + log warning node 'P' 'failed'; + end error; + log info node 'P' 'n=' + toString($N); + log info node 'P' 'after call'; + end if; + log info node 'P' 'end'; +end;`, + rejoin: "after call", + ends: 1, + }, + { + // The rest of the branch builds on the result, so the handler skips + // it for the code after the IF, the branch's shared error path + // (TestBuildFlowGraph_OutputHandlerInReturningBranchSkipsDerivedSuccessTail), + // on the flow leaving the IF, not the one into it. + name: "result used by the rest of a branch that returns", + src: `create microflow M.K ($Flag: Boolean) +begin + if $Flag then + $N = call microflow M.Sub() on error without rollback begin + log warning node 'P' 'failed'; + end error; + log info node 'P' 'n=' + toString($N); + return; + end if; + log info node 'P' 'end'; +end;`, + rejoin: "end", + ends: 2, + }, + { + name: "nested if", + src: `create microflow M.G ($Flag: Boolean, $Other: Boolean) +begin + if $Flag then + if $Other then + call microflow M.Sub() on error without rollback begin + log warning node 'P' 'failed'; + end error; + log info node 'P' 'after call'; + end if; + log info node 'P' 'outer'; + end if; + log info node 'P' 'end'; +end;`, + rejoin: "after call", + ends: 1, + }, + { + name: "case branch", + src: `create microflow M.H ($C: M.Color) +begin + case $C + when Red then + call microflow M.Sub() on error without rollback begin + log warning node 'P' 'failed'; + end error; + log info node 'P' 'after call'; + when Green, (empty) then + log info node 'P' 'green'; + end case; + log info node 'P' 'end'; +end;`, + rejoin: "after call", + ends: 1, + }, + { + name: "split type branch", + src: `create microflow M.I ($In: M.Base) +begin + split type $In + when M.Special then + call microflow M.Sub() on error without rollback begin + log warning node 'P' 'failed'; + end error; + when M.Base then + log info node 'P' 'base'; + end split; + log info node 'P' 'end'; +end;`, + rejoin: "end", + ends: 1, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + oc := buildMicroflowFromMDL(t, tc.src) + logs := logActivitiesByText(oc) + failed, want := logs["failed"], logs[tc.rejoin] + if failed == "" || want == "" { + t.Fatalf("missing log activities in %v", logs) + } + if got := throughMerges(oc, failed); got != want { + t.Errorf("handler rejoins at %s, want the %q log %s", describeObject(oc, got), tc.rejoin, want) + } + if n := countEndEvents(oc); n != tc.ends { + t.Errorf("got %d end events, want %d", n, tc.ends) + } + if hasCycle(oc) { + t.Error("the flow graph has a cycle") + } + }) + } +} + +// A handler still pending when an IF starts belongs to the activity before +// it: the branches must leave it alone, so it rejoins on the flow into the +// split, as before. +func TestErrorHandlerBeforeAnIfStillRejoinsAtTheSplit(t *testing.T) { + oc := buildMicroflowFromMDL(t, `create microflow M.J ($Flag: Boolean) +begin + $N = call microflow M.Sub() on error without rollback begin + log warning node 'P' 'failed'; + end error; + if $Flag then + log info node 'P' 'a'; + log info node 'P' 'b'; + end if; + log info node 'P' 'end'; +end;`) + got := throughMerges(oc, logActivitiesByText(oc)["failed"]) + if _, ok := objectByID(oc.Objects, got).(*microflows.ExclusiveSplit); !ok { + t.Errorf("handler rejoins at %s, want the IF's split", describeObject(oc, got)) + } + if hasCycle(oc) { + t.Error("the flow graph has a cycle") + } +} + +func logActivitiesByText(oc *microflows.MicroflowObjectCollection) map[string]model.ID { + ids := map[string]model.ID{} + for _, obj := range oc.Objects { + activity, ok := obj.(*microflows.ActionActivity) + if !ok { + continue + } + if action, ok := activity.Action.(*microflows.LogMessageAction); ok && action.MessageTemplate != nil { + for _, text := range action.MessageTemplate.Translations { + ids[text] = activity.ID + } + } + } + return ids +} + +// throughMerges follows the single outgoing flow from id past any merges and +// returns the first object that is not one. +func throughMerges(oc *microflows.MicroflowObjectCollection, id model.ID) model.ID { + for range len(oc.Objects) { + var next model.ID + for _, f := range oc.Flows { + if f.OriginID == id { + next = f.DestinationID + break + } + } + if next == "" { + return "" + } + if _, ok := objectByID(oc.Objects, next).(*microflows.ExclusiveMerge); !ok { + return next + } + id = next + } + return "" +} + +func describeObject(oc *microflows.MicroflowObjectCollection, id model.ID) string { + if id == "" { + return "nothing" + } + for text, logID := range logActivitiesByText(oc) { + if logID == id { + return "the " + text + " log" + } + } + switch objectByID(oc.Objects, id).(type) { + case *microflows.EndEvent: + return "an end event" + case *microflows.ExclusiveSplit: + return "a split" + } + return string(id) +} + +func countEndEvents(oc *microflows.MicroflowObjectCollection) int { + n := 0 + for _, obj := range oc.Objects { + if _, ok := obj.(*microflows.EndEvent); ok { + n++ + } + } + return n +} + +func hasCycle(oc *microflows.MicroflowObjectCollection) bool { + next := map[model.ID][]model.ID{} + for _, f := range oc.Flows { + next[f.OriginID] = append(next[f.OriginID], f.DestinationID) + } + const ( + unseen = iota + open + done + ) + state := map[model.ID]int{} + var visit func(model.ID) bool + visit = func(id model.ID) bool { + switch state[id] { + case open: + return true + case done: + return false + } + state[id] = open + for _, to := range next[id] { + if visit(to) { + return true + } + } + state[id] = done + return false + } + for id := range next { + if visit(id) { + return true + } + } + return false +} diff --git a/mdl/executor/cmd_microflows_builder_control.go b/mdl/executor/cmd_microflows_builder_control.go index 7cb5719e80..74dac8827d 100644 --- a/mdl/executor/cmd_microflows_builder_control.go +++ b/mdl/executor/cmd_microflows_builder_control.go @@ -34,8 +34,6 @@ func (fb *flowBuilder) addIfStatement(s *ast.IfStmt) model.ID { hasElseBody := s.HasElse || len(s.ElseBody) > 0 elseReturns := hasElseBody && lastStmtIsReturn(s.ElseBody) bothReturn := hasElseBody && thenReturns && elseReturns - thenNeedsErrorMerge := thenReturns && bodyHasContinuingCustomErrorHandler(s.ThenBody) - elseNeedsErrorMerge := elseReturns && bodyHasContinuingCustomErrorHandler(s.ElseBody) // The last statement of a loop body: an IF/ELSE whose branches both carry // on there joins nothing, since both end the iteration, and a loop-body @@ -136,7 +134,9 @@ func (fb *flowBuilder) addIfStatement(s *ast.IfStmt) model.ID { needMerge := false if !bothReturn && !tailWithoutMerge { if hasElseBody { - needMerge = (!thenReturns && !elseReturns) || thenNeedsErrorMerge || elseNeedsErrorMerge + // A fall-through handler in a returning branch rejoins inside that + // branch, after its failing activity, so it needs no merge here. + needMerge = !thenReturns && !elseReturns } else { needMerge = !thenReturns // THEN continues + FALSE path → 2 inputs } @@ -158,6 +158,7 @@ func (fb *flowBuilder) addIfStatement(s *ast.IfStmt) model.ID { thenStartX := splitX + SplitWidth + HorizontalSpacing/2 var noMergeExitID model.ID + var branchHandlers []pendingErrorHandlerState var noMergeExitCase string var noMergeExitAnchor *ast.FlowAnchors @@ -171,6 +172,7 @@ func (fb *flowBuilder) addIfStatement(s *ast.IfStmt) model.ID { var prevThenAnchor *ast.FlowAnchors var pendingThenCase string var pendingThenAnchor *ast.FlowAnchors + outerHandlers := fb.enterBranchErrorHandlers() for i, stmt := range s.ThenBody { thisAnchor := stmtOwnAnchor(stmt) fb.atBodyTail = tailWithoutMerge && i == len(s.ThenBody)-1 @@ -207,9 +209,7 @@ func (fb *flowBuilder) addIfStatement(s *ast.IfStmt) model.ID { } applyUserAnchors(flow, originAnchor, destAnchor) fb.flows = append(fb.flows, flow) - if fb.emptyErrorHandlerFrom == lastThenID { - fb.addPendingErrorHandlerFlowForStatement(lastThenID, actID, stmt, statementsReferenceVar(s.ThenBody[i+1:], fb.errorHandlerSkipVar)) - } + fb.addBranchErrorHandlerFlowForStatement(thenReturns, lastThenID, actID, stmt, statementsReferenceVar(s.ThenBody[i+1:], fb.errorHandlerSkipVar)) } prevThenAnchor = thisAnchor // For nested compound statements, use their exit point @@ -245,6 +245,7 @@ func (fb *flowBuilder) addIfStatement(s *ast.IfStmt) model.ID { } applyUserAnchors(flow, originAnchor, destAnchor) fb.flows = append(fb.flows, flow) + fb.addPendingErrorHandlerFlowForStatement(lastThenID, mergeID, nil) } else { // Empty THEN body - connect split directly to merge with true case. // Pass trueBranchAnchor as destination too so the @anchor(true: (..., to: Y)) @@ -253,9 +254,8 @@ func (fb *flowBuilder) addIfStatement(s *ast.IfStmt) model.ID { applyUserAnchors(flow, trueBranchAnchor, trueBranchAnchor) fb.flows = append(fb.flows, flow) } - } else if thenReturns && needMerge { - fb.addPendingErrorHandlerFlowTo(mergeID) } + branchHandlers = fb.takeBranchErrorHandlers(outerHandlers) // Process ELSE body (below the THEN path) thenH := max(thenBounds.Height, ActivityHeight) @@ -269,6 +269,7 @@ func (fb *flowBuilder) addIfStatement(s *ast.IfStmt) model.ID { var prevElseAnchor *ast.FlowAnchors var pendingElseCase string var pendingElseAnchor *ast.FlowAnchors + outerHandlers = fb.enterBranchErrorHandlers() for i, stmt := range s.ElseBody { thisAnchor := stmtOwnAnchor(stmt) fb.atBodyTail = tailWithoutMerge && i == len(s.ElseBody)-1 @@ -314,9 +315,7 @@ func (fb *flowBuilder) addIfStatement(s *ast.IfStmt) model.ID { } applyUserAnchors(flow, originAnchor, destAnchor) fb.flows = append(fb.flows, flow) - if fb.emptyErrorHandlerFrom == lastElseID { - fb.addPendingErrorHandlerFlowForStatement(lastElseID, actID, stmt, statementsReferenceVar(s.ElseBody[i+1:], fb.errorHandlerSkipVar)) - } + fb.addBranchErrorHandlerFlowForStatement(elseReturns, lastElseID, actID, stmt, statementsReferenceVar(s.ElseBody[i+1:], fb.errorHandlerSkipVar)) } prevElseAnchor = thisAnchor // For nested compound statements, use their exit point @@ -352,6 +351,7 @@ func (fb *flowBuilder) addIfStatement(s *ast.IfStmt) model.ID { } applyUserAnchors(flow, originAnchor, destAnchor) fb.flows = append(fb.flows, flow) + fb.addPendingErrorHandlerFlowForStatement(lastElseID, mergeID, nil) } else { // Explicit empty ELSE body: the split still needs a configured // false case when both branches converge at the merge. @@ -359,9 +359,8 @@ func (fb *flowBuilder) addIfStatement(s *ast.IfStmt) model.ID { applyUserAnchors(flow, falseBranchAnchor, falseBranchAnchor) fb.flows = append(fb.flows, flow) } - } else if elseReturns && needMerge { - fb.addPendingErrorHandlerFlowTo(mergeID) } + branchHandlers = append(branchHandlers, fb.takeBranchErrorHandlers(outerHandlers)...) if !needMerge { if thenReturns && !elseReturns { if lastElseID != "" { @@ -419,7 +418,8 @@ func (fb *flowBuilder) addIfStatement(s *ast.IfStmt) model.ID { var prevThenAnchor *ast.FlowAnchors var pendingThenCase string var pendingThenAnchor *ast.FlowAnchors - for _, stmt := range s.ThenBody { + outerHandlers := fb.enterBranchErrorHandlers() + for i, stmt := range s.ThenBody { thisAnchor := stmtOwnAnchor(stmt) actID := fb.addStatement(stmt) if fb.takeBranchJoin(lastThenID, splitID, "true", pendingThenCase, prevThenAnchor, trueBranchAnchor) { @@ -450,6 +450,7 @@ func (fb *flowBuilder) addIfStatement(s *ast.IfStmt) model.ID { } applyUserAnchors(flow, originAnchor, destAnchor) fb.flows = append(fb.flows, flow) + fb.addBranchErrorHandlerFlowForStatement(thenReturns, lastThenID, actID, stmt, statementsReferenceVar(s.ThenBody[i+1:], fb.errorHandlerSkipVar)) } prevThenAnchor = thisAnchor // For nested compound statements, use their exit point @@ -485,6 +486,7 @@ func (fb *flowBuilder) addIfStatement(s *ast.IfStmt) model.ID { } applyUserAnchors(flow, originAnchor, destAnchor) fb.flows = append(fb.flows, flow) + fb.addPendingErrorHandlerFlowForStatement(lastThenID, mergeID, nil) } else { // Empty THEN body - connect split directly to merge going down and back up. // Pass trueBranchAnchor as destination too so @anchor(true: (..., to: Y)) @@ -494,6 +496,7 @@ func (fb *flowBuilder) addIfStatement(s *ast.IfStmt) model.ID { fb.flows = append(fb.flows, flow) } } + branchHandlers = fb.takeBranchErrorHandlers(outerHandlers) if !needMerge { noMergeExitID = splitID noMergeExitCase = "false" @@ -501,6 +504,18 @@ func (fb *flowBuilder) addIfStatement(s *ast.IfStmt) model.ID { } } + // A handler a branch could not settle — one with a result, in a branch that + // returns — rejoins on the flow leaving the IF. When both branches return + // nothing leaves it, and the split never starts another flow, so the + // handler ends at an end event as before. + exitID := noMergeExitID + if needMerge { + exitID = mergeID + } else if exitID == "" { + exitID = splitID + } + fb.queueBranchErrorHandlersAt(branchHandlers, exitID) + // If both branches end with RETURN, the flow terminates here if bothReturn { fb.endsWithReturn = true diff --git a/mdl/executor/cmd_microflows_builder_flows.go b/mdl/executor/cmd_microflows_builder_flows.go index 1d281cbf3e..6b025f6569 100644 --- a/mdl/executor/cmd_microflows_builder_flows.go +++ b/mdl/executor/cmd_microflows_builder_flows.go @@ -326,33 +326,85 @@ func (fb *flowBuilder) rewritePendingErrorHandlers(rewrite func(pendingErrorHand fb.setActivePendingErrorHandler(active) } -func (fb *flowBuilder) addPendingErrorHandlerFlowForStatement(originID, destinationID model.ID, stmt ast.MicroflowStatement, futureReferencesSkipVar ...bool) { - futureReferences := len(futureReferencesSkipVar) > 0 && futureReferencesSkipVar[0] - fb.rewritePendingErrorHandlers(func(state pendingErrorHandlerState) pendingErrorHandlerState { - return fb.addPendingErrorHandlerFlowForState(state, originID, destinationID, stmt, futureReferences) - }) +// branchErrorHandlerScope holds the handlers still pending when a branch of a +// split starts: they belong to the activities before the split, not to it. +type branchErrorHandlerScope struct { + active pendingErrorHandlerState + queued []pendingErrorHandlerState } -func (fb *flowBuilder) addPendingErrorHandlerFlowTo(destinationID model.ID) { - if destinationID == "" { - return +// enterBranchErrorHandlers sets the pending handlers aside while a branch is +// built, so the branch settles its own handlers on its own flows, as the top +// level does. A fall-through handler in a branch was left pending past the +// branch and rejoined on the flow INTO the split (a retry loop when the call +// assigns a variable) or ended the microflow (mendixlabs/mxcli#1565). +func (fb *flowBuilder) enterBranchErrorHandlers() branchErrorHandlerScope { + outer := branchErrorHandlerScope{active: fb.activePendingErrorHandler(), queued: fb.pendingErrorHandlers} + fb.setActivePendingErrorHandler(pendingErrorHandlerState{}) + fb.pendingErrorHandlers = nil + return outer +} + +// takeBranchErrorHandlers restores the handlers set aside and returns those of +// the branch it could not settle. +func (fb *flowBuilder) takeBranchErrorHandlers(outer branchErrorHandlerScope) []pendingErrorHandlerState { + inner := fb.pendingErrorHandlers + if active := fb.activePendingErrorHandler(); !active.activeIsEmpty() { + inner = append(inner, active) } - fb.rewritePendingErrorHandlers(func(state pendingErrorHandlerState) pendingErrorHandlerState { - if state.emptyFrom != "" { - fb.addEmptyErrorHandlerRejoinFlowFrom(state.emptyFrom, state.emptyFrom, destinationID) - state.emptyFrom = "" - } - if state.source != "" && state.tailFrom != "" { - fb.addErrorHandlerRejoinFlowForState(state, state.source, destinationID) - state.source = "" - state.tailFrom = "" + fb.pendingErrorHandlers = outer.queued + fb.setActivePendingErrorHandler(outer.active) + return inner +} + +// queueBranchErrorHandlersAt queues the handlers the branches of a split could +// not settle, to rejoin on the flow that leaves the split from exitID — its +// merge, or the last activity of its one continuing branch — which the parent +// draws later. A handler with a result settles on whatever flow comes next, +// and the next flow the parent draws is the one INTO the split: a loop. +func (fb *flowBuilder) queueBranchErrorHandlersAt(handlers []pendingErrorHandlerState, exitID model.ID) { + for _, state := range handlers { + if state.skipVar != "" && state.tailFrom != "" { state.skipVar = "" - state.tailCase = "" - state.tailAnchor = nil - state.tailIsSource = false - state.returnValue = "" + state.source = exitID } - return state + fb.pendingErrorHandlers = append(fb.pendingErrorHandlers, state) + } +} + +// settleBranchErrorHandlersOn settles handlers a branch left pending on the +// flow that leaves it, originID → destinationID, which must already be drawn: +// a handler on the branch's last activity rejoins there. It returns those it +// could not settle. +func (fb *flowBuilder) settleBranchErrorHandlersOn(handlers []pendingErrorHandlerState, originID, destinationID model.ID) []pendingErrorHandlerState { + if len(handlers) == 0 { + return nil + } + outer := fb.enterBranchErrorHandlers() + fb.pendingErrorHandlers = handlers + fb.addPendingErrorHandlerFlowForStatement(originID, destinationID, nil) + return fb.takeBranchErrorHandlers(outer) +} + +// addBranchErrorHandlerFlowForStatement settles the branch's handlers on a flow +// drawn inside the branch, as addPendingErrorHandlerFlowForStatement does at the +// top level. In a branch that ends in a return, a handler on an activity with a +// result is left for the code after the split: it is the shared error path of a +// branch whose remaining statements build on the result +// (TestBuildFlowGraph_OutputHandlerInReturningBranchSkipsDerivedSuccessTail). +func (fb *flowBuilder) addBranchErrorHandlerFlowForStatement(branchReturns bool, originID, destinationID model.ID, stmt ast.MicroflowStatement, futureReferencesSkipVar bool) { + fb.rewritePendingErrorHandlers(func(state pendingErrorHandlerState) pendingErrorHandlerState { + if branchReturns && state.skipVar != "" { + return state + } + return fb.addPendingErrorHandlerFlowForState(state, originID, destinationID, stmt, futureReferencesSkipVar) + }) +} + +func (fb *flowBuilder) addPendingErrorHandlerFlowForStatement(originID, destinationID model.ID, stmt ast.MicroflowStatement, futureReferencesSkipVar ...bool) { + futureReferences := len(futureReferencesSkipVar) > 0 && futureReferencesSkipVar[0] + fb.rewritePendingErrorHandlers(func(state pendingErrorHandlerState) pendingErrorHandlerState { + return fb.addPendingErrorHandlerFlowForState(state, originID, destinationID, stmt, futureReferences) }) }