Skip to content

Commit febb2fa

Browse files
committed
fix: trim empty top-level else branches
Symptom: describe could emit an empty top-level `else` when the false branch pointed at a continuation that had already been emitted while traversing the true branch. Re-executing the MDL removed that empty branch, leaving a roundtrip mismatch. Root cause: top-level IF traversal decided whether to emit `else` only by comparing `falseFlow.DestinationID` to the selected merge. That misses cases where the false destination is not the merge but traversal still emits no statements because the continuation is already visited. Fix: mirror nested IF traversal: emit the `else` line tentatively, traverse the false branch, and remove the line again if the traversal produced no statements. Tests: added a synthetic traversal regression test; `go test ./mdl/executor -run 'TopLevelIfRemovesEmptyElse|SequentialIfWithoutElse' -v`; `make build`; `make lint-go`; `make test`.
1 parent 2991c12 commit febb2fa

2 files changed

Lines changed: 65 additions & 6 deletions

File tree

mdl/executor/cmd_microflows_show_helpers.go

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -724,18 +724,20 @@ func traverseFlow(
724724
traverseFlowUntilMerge(ctx, trueFlow.DestinationID, mergeID, activityMap, flowsByOrigin, flowsByDest, splitMergeMap, visited, entityNames, microflowNames, lines, indent+1, sourceMap, headerLineCount, annotationsByTarget)
725725
}
726726

727-
// Emit the ELSE branch only if it has statements. When the false
728-
// flow jumps straight to the merge (the MDL was `if X then ... end if`
729-
// with no else), emitting `else` with no body produces an empty
730-
// branch that normalizes away on re-parse.
731-
falseHasBody := falseFlow != nil && falseFlow.DestinationID != mergeID
732-
if falseHasBody {
727+
if falseFlow != nil {
728+
elseLineIdx := len(*lines)
733729
*lines = append(*lines, indentStr+"else")
734730
visitedFalseBranch := make(map[model.ID]bool)
735731
for id := range visited {
736732
visitedFalseBranch[id] = true
737733
}
738734
traverseFlowUntilMerge(ctx, falseFlow.DestinationID, mergeID, activityMap, flowsByOrigin, flowsByDest, splitMergeMap, visitedFalseBranch, entityNames, microflowNames, lines, indent+1, sourceMap, headerLineCount, annotationsByTarget)
735+
// Remove empty else block. A false branch can point at a
736+
// continuation already emitted through the true branch, so checking
737+
// only falseFlow.DestinationID != mergeID is not enough.
738+
if len(*lines) == elseLineIdx+1 {
739+
*lines = (*lines)[:elseLineIdx]
740+
}
739741
}
740742

741743
*lines = append(*lines, indentStr+"end if;")

mdl/executor/cmd_microflows_traverse_test.go

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -561,6 +561,63 @@ func TestTraverseFlow_SequentialIfWithoutElseKeepsContinuationOutsideFirstIf(t *
561561
}
562562
}
563563

564+
func TestTraverseFlow_TopLevelIfRemovesEmptyElseAfterVisitedContinuation(t *testing.T) {
565+
e := newTestExecutor()
566+
567+
logAction := func(id, message string) *microflows.ActionActivity {
568+
return &microflows.ActionActivity{
569+
BaseActivity: microflows.BaseActivity{BaseMicroflowObject: mkObj(id)},
570+
Action: &microflows.LogMessageAction{
571+
LogLevel: "Info",
572+
LogNodeName: "'Synthetic'",
573+
MessageTemplate: &model.Text{Translations: map[string]string{"en_US": message}},
574+
},
575+
}
576+
}
577+
578+
activityMap := map[model.ID]microflows.MicroflowObject{
579+
mkID("start"): &microflows.StartEvent{BaseMicroflowObject: mkObj("start")},
580+
mkID("outer_split"): &microflows.ExclusiveSplit{
581+
BaseMicroflowObject: mkObj("outer_split"),
582+
SplitCondition: &microflows.ExpressionSplitCondition{Expression: "$UseNestedPath"},
583+
},
584+
mkID("inner_split"): &microflows.ExclusiveSplit{
585+
BaseMicroflowObject: mkObj("inner_split"),
586+
SplitCondition: &microflows.ExpressionSplitCondition{Expression: "$HasContinuation"},
587+
},
588+
mkID("inner_return"): &microflows.EndEvent{BaseMicroflowObject: mkObj("inner_return")},
589+
mkID("tail_log"): logAction("tail_log", "shared tail"),
590+
mkID("end"): &microflows.EndEvent{BaseMicroflowObject: mkObj("end")},
591+
}
592+
flowsByOrigin := map[model.ID][]*microflows.SequenceFlow{
593+
mkID("start"): {mkFlow("start", "outer_split")},
594+
mkID("outer_split"): {
595+
mkBranchFlow("outer_split", "inner_split", &microflows.ExpressionCase{Expression: "true"}),
596+
mkBranchFlow("outer_split", "tail_log", &microflows.ExpressionCase{Expression: "false"}),
597+
},
598+
mkID("inner_split"): {
599+
mkBranchFlow("inner_split", "tail_log", &microflows.ExpressionCase{Expression: "true"}),
600+
mkBranchFlow("inner_split", "inner_return", &microflows.ExpressionCase{Expression: "false"}),
601+
},
602+
mkID("tail_log"): {mkFlow("tail_log", "end")},
603+
}
604+
605+
splitMergeMap := map[model.ID]model.ID{
606+
mkID("outer_split"): mkID("end"),
607+
mkID("inner_split"): mkID("tail_log"),
608+
}
609+
var lines []string
610+
e.traverseFlow(mkID("start"), activityMap, flowsByOrigin, splitMergeMap, make(map[model.ID]bool), nil, nil, &lines, 0, nil, 0, nil)
611+
612+
out := strings.Join(lines, "\n")
613+
if strings.Contains(out, "\nelse\nend if;") {
614+
t.Fatalf("empty top-level else should be removed:\n%s", out)
615+
}
616+
if got := strings.Count(out, "shared tail"); got != 1 {
617+
t.Fatalf("shared tail emitted %d times, want once:\n%s", got, out)
618+
}
619+
}
620+
564621
func TestTraverseFlow_GuardBranchWithMultipleActivitiesKeepsContinuationOutsideElse(t *testing.T) {
565622
e := newTestExecutor()
566623

0 commit comments

Comments
 (0)