diff --git a/binding.go b/binding.go index 6b479c8..8dccc7c 100644 --- a/binding.go +++ b/binding.go @@ -3,7 +3,6 @@ package commandkit import ( "errors" "fmt" - "sort" "strconv" commandmeta "go.maximumdirect.net/commandkit/internal/commandmeta" @@ -298,7 +297,14 @@ func prepareInputWithOccurrences[I any](spec CommandSpec[I], flagValues, flagSpe fallbackOrdinal++ } if occurrence.apply != nil { - ordered = append(ordered, occurrence) + if occurrences == nil { + ordered = append(ordered, occurrence) + } else { + if ordered == nil { + ordered = make([]preparedOccurrence[I], len(occurrences)) + } + ordered[*occurrence.ordinal] = occurrence + } } } if enforceRequired && binding.meta.Required && !binding.deferredRequired && !value.supplied && missingRequired == "" { @@ -313,9 +319,12 @@ func prepareInputWithOccurrences[I any](spec CommandSpec[I], flagValues, flagSpe return preparedInput[I]{}, err } preparedValues := append([]preparedValue[I](nil), preparedFlags...) - sort.SliceStable(ordered, func(i, j int) bool { return *ordered[i].ordinal < *ordered[j].ordinal }) + // Parsed occurrences use dense parser positions; without a parser log the + // fallback ordinals were assigned in the same order as the append above. for _, occurrence := range ordered { - preparedValues = append(preparedValues, preparedValue[I]{apply: occurrence.apply}) + if occurrence.apply != nil { + preparedValues = append(preparedValues, preparedValue[I]{apply: occurrence.apply}) + } } for i, argument := range spec.Arguments { binding := argument.binding diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index b95b294..93db99c 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,6 +1,6 @@ # Shared reporting and PromptKit adoption remediation -Status: two open findings; R1–R5 complete. R1–R3 are from the architectural review of +Status: one open finding; R1–R6 complete. R1–R3 are from the architectural review of `f83866e44a8a9066db52cbd2aa5cccde0d69a839`. R4–R7 are from the algorithmic audit of `f5b2d1f80c4720b762f3301c29d5050710c210d0`; R4 is a correctness issue and R5–R7 are efficiency opportunities for consideration. @@ -243,6 +243,15 @@ threshold or benchmark is needed to establish the complexity difference. ## R6 — P3: Ordered inputs use general sorting of already indexed streams +**Complete.** Initial preparation places ordered setters at parser positions +and scans them once, using O(M+F) traversal and O(M) ordering storage. Captured +replay uses a standard-library heap to merge K monotone streams in O(K+E log K) +work for K greater than one, O(E) for one stream, and O(K) merge storage. +Selection order breaks ties, preserving duplicate handles. Core regressions cover +nested capture, reversed/subset/single/empty/duplicate selections, both replay +methods, conversion counts, and repeated invocations. The independent core and +aligned ckconfig/ckprompt suites pass with Go 1.27.0. + **Location:** [binding.go](../../binding.go), prepareInputWithOccurrences line 316, and [flag_values.go](../../flag_values.go), FlagValues.applyTo line 122. diff --git a/flag_values.go b/flag_values.go index eb3eec4..d9ca1b3 100644 --- a/flag_values.go +++ b/flag_values.go @@ -1,9 +1,9 @@ package commandkit import ( + "container/heap" "errors" "fmt" - "sort" ) // ErrFlagInput identifies a missing required captured flag selected for an input. @@ -108,24 +108,74 @@ func (values FlagValues[I]) applyTo(target *I, suppliedOnly bool, selected ...Fl } preparedValues = append(preparedValues, prepared) } - var ordered []preparedOccurrence[I] - for _, prepared := range preparedValues { + var ordered occurrenceHeap[I] + for index, prepared := range preparedValues { if prepared.apply != nil { prepared.apply(target) } - for _, occurrence := range prepared.ordered { - if occurrence.apply != nil { - ordered = append(ordered, occurrence) - } + stream := occurrenceStream[I]{events: prepared.ordered, order: index} + stream.skipUnapplied() + if len(stream.events) > 0 { + ordered = append(ordered, stream) } } - sort.SliceStable(ordered, func(i, j int) bool { return *ordered[i].ordinal < *ordered[j].ordinal }) - for _, occurrence := range ordered { - occurrence.apply(target) + // One selected stream already has the required order. + if len(ordered) == 1 { + for _, event := range ordered[0].events { + if event.apply != nil { + event.apply(target) + } + } + return nil + } + heap.Init(&ordered) + for len(ordered) > 0 { + stream := &ordered[0] + stream.events[0].apply(target) + stream.events = stream.events[1:] + stream.skipUnapplied() + if len(stream.events) == 0 { + heap.Pop(&ordered) + } else { + heap.Fix(&ordered, 0) + } } return nil } +// Each binding supplies a monotone stream. The selection index breaks ordinal +// ties in the same order as concatenation followed by a stable sort. +type occurrenceStream[I any] struct { + events []preparedOccurrence[I] + order int +} + +func (stream *occurrenceStream[I]) skipUnapplied() { + for len(stream.events) > 0 && stream.events[0].apply == nil { + stream.events = stream.events[1:] + } +} + +type occurrenceHeap[I any] []occurrenceStream[I] + +func (h occurrenceHeap[I]) Len() int { return len(h) } +func (h occurrenceHeap[I]) Less(i, j int) bool { + left, right := *h[i].events[0].ordinal, *h[j].events[0].ordinal + if left == right { + return h[i].order < h[j].order + } + return left < right +} +func (h occurrenceHeap[I]) Swap(i, j int) { h[i], h[j] = h[j], h[i] } +func (h *occurrenceHeap[I]) Push(value any) { *h = append(*h, value.(occurrenceStream[I])) } +func (h *occurrenceHeap[I]) Pop() any { + last := len(*h) - 1 + value := (*h)[last] + (*h)[last] = occurrenceStream[I]{} + *h = (*h)[:last] + return value +} + func classifyInputError(err error) Outcome { if errors.Is(err, ErrFlagWiring) { return Failure(err.Error()).WithCause(err) diff --git a/occurrence_test.go b/occurrence_test.go index 7dfe018..a1f49c1 100644 --- a/occurrence_test.go +++ b/occurrence_test.go @@ -150,3 +150,65 @@ func TestOccurrenceRequiredAndAliasedConversion(t *testing.T) { t.Fatalf("help: %#v", help) } } + +func TestNestedCapturedOccurrencesMergeSelections(t *testing.T) { + calls := 0 + a := occurrenceFor("alpha", false, &calls) + b := RenameFlag(occurrenceFor("beta", false, &calls), "beta") + c := RenameFlag(occurrenceFor("gamma", false, &calls), "gamma") + type middle struct{ values FlagValues[occurrenceInput] } + type outer struct{ values FlagValues[middle] } + inner := CaptureFlags([]Flag[occurrenceInput]{a, b, c}, func(m *middle) *FlagValues[occurrenceInput] { return &m.values }) + flags := CaptureFlags(inner, func(o *outer) *FlagValues[middle] { return &o.values }) + var captured FlagValues[occurrenceInput] + app, err := New(Identity{Name: "Example"}, NewCommand(CommandSpec[outer]{Name: "run", Flags: flags, Run: func(_ Invocation, input outer) Outcome { + var m middle + if err := input.values.ApplyTo(&m, inner...); err != nil { + return Failure("middle replay failed").WithCause(err) + } + captured = m.values + return Success() + }})) + if err != nil { + t.Fatal(err) + } + for _, args := range [][]string{ + {"run", "--gamma=c1", "--alpha=a1", "--beta=b1", "--alpha=a2", "--gamma=c2", "--beta=b2"}, + {"run"}, + } { + result := app.Execute(t.Context(), args, Streams{}) + if result.Status != 0 { + t.Fatalf("capture=%#v", result) + } + for _, tc := range []struct { + name string + flags []Flag[occurrenceInput] + want []string + }{ + {"all reversed", []Flag[occurrenceInput]{c, b, a}, []string{"gamma=c1", "alpha=a1", "beta=b1", "alpha=a2", "gamma=c2", "beta=b2"}}, + {"subset", []Flag[occurrenceInput]{a, c}, []string{"gamma=c1", "alpha=a1", "alpha=a2", "gamma=c2"}}, + {"single", []Flag[occurrenceInput]{b}, []string{"beta=b1", "beta=b2"}}, + {"duplicate handle", []Flag[occurrenceInput]{b, a, b}, []string{"alpha=a1", "beta=b1", "beta=b1", "alpha=a2", "beta=b2", "beta=b2"}}, + {"empty", nil, nil}, + } { + for _, suppliedOnly := range []bool{false, true} { + var got occurrenceInput + if suppliedOnly { + err = captured.ApplySuppliedTo(&got, tc.flags...) + } else { + err = captured.ApplyTo(&got, tc.flags...) + } + want := tc.want + if len(args) == 1 { + want = nil + } + if err != nil || !reflect.DeepEqual(got.events, want) { + t.Fatalf("%s suppliedOnly=%t args=%v events=%v want=%v err=%v", tc.name, suppliedOnly, args, got.events, want, err) + } + } + } + } + if calls != 6 { + t.Fatalf("replay reconverted captured input: %d parses", calls) + } +}