diff --git a/CHANGELOG.md b/CHANGELOG.md index d62669594..fc594b786 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,12 @@ Versions follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html). --- +## [Unreleased] + +### 🐛 Bug Fixes + +- **`init` and `initial_guess` together raise "Conflicting aliases" in descriptive mode too** ([#964](https://github.com/control-toolbox/OptimalControl.jl/issues/964)). Explicit mode already reported the conflict; descriptive mode, the common case, routed the leftover `init` to the strategies and rejected it as an unknown option, with unrelated suggestions. Both modes now run the same check before any routing. + ## [2.2.1-beta] — 2026-09-02 Documentation-infrastructure follow-up to the 2.2.0-beta site rewrite. No change to OptimalControl's API or runtime behaviour; nothing to migrate — see [BREAKING.md](BREAKING.md). diff --git a/docs/src/solve/initial-guess.md b/docs/src/solve/initial-guess.md index 5d5c9e1cb..83700d8dc 100644 --- a/docs/src/solve/initial-guess.md +++ b/docs/src/solve/initial-guess.md @@ -389,8 +389,8 @@ variable accept an initial guess. ## `init` or `initial_guess` `init` is an alias for `initial_guess`; use whichever reads better, but not both in the same -call. In descriptive mode, passing both is reported as an unknown option `:init` rather than -as a conflict ([OptimalControl#964](https://github.com/control-toolbox/OptimalControl.jl/issues/964)). +call. Passing both raises an `IncorrectArgument` "Conflicting aliases for the same option", in +descriptive and explicit mode alike. ## See also diff --git a/src/helpers/descriptive_routing.jl b/src/helpers/descriptive_routing.jl index cb20313f8..7841626d4 100644 --- a/src/helpers/descriptive_routing.jl +++ b/src/helpers/descriptive_routing.jl @@ -254,6 +254,9 @@ function _route_descriptive_options( registry::CTBase.Strategies.StrategyRegistry, kwargs, ) + # `route_all_options` consumes the first alias it finds and routes the other one to + # the strategies, where it is reported as an unknown option: check the conflict first. + _check_action_aliases(kwargs, _INITIAL_GUESS_ALIASES) families = _descriptive_families() action_defs = _descriptive_action_defs() return CTBase.Orchestration.route_all_options( diff --git a/src/helpers/kwarg_extraction.jl b/src/helpers/kwarg_extraction.jl index 9d50ce143..cb1d6b454 100644 --- a/src/helpers/kwarg_extraction.jl +++ b/src/helpers/kwarg_extraction.jl @@ -76,15 +76,49 @@ true See also: [`_extract_kwarg`](@ref), [`solve_explicit`](@ref), [`solve_descriptive`](@ref) """ function _extract_action_kwarg(kwargs::Base.Pairs, names::Tuple{Vararg{Symbol}}, default) - present = [n for n in names if haskey(kwargs, n)] + present = _check_action_aliases(kwargs, names) if isempty(present) return default, kwargs - elseif length(present) == 1 + else name = present[1] value = kwargs[name] remaining = Base.pairs(NamedTuple(k => v for (k, v) in kwargs if k != name)) return value, remaining - else + end +end + +""" +$(TYPEDSIGNATURES) + +Return the names of `names` present in `kwargs`, and raise if there is more than one. + +`names` lists the primary name of an action option and its aliases. Passing two of them +at once is ambiguous, whichever mode the call uses: the check is shared by +[`_extract_action_kwarg`](@ref) (explicit mode) and [`_route_descriptive_options`](@ref) +(descriptive mode), so both modes raise the same error. + +# Arguments +- `kwargs`: Keyword arguments from a `solve` call +- `names::Tuple{Vararg{Symbol}}`: Primary name and aliases of one action option + +# Returns +- `Vector{Symbol}`: The names found in `kwargs`, empty or of length one + +# Throws +- [`CTBase.Exceptions.IncorrectArgument`](@extref): If more than one alias is provided at the same time + +# Example +```julia +julia> OptimalControl._check_action_aliases(pairs((; init=1, display=false)), (:initial_guess, :init)) +1-element Vector{Symbol}: + :init +``` + +See also: [`_extract_action_kwarg`](@ref), [`_route_descriptive_options`](@ref) +""" +function _check_action_aliases(kwargs, names::Tuple{Vararg{Symbol}})::Vector{Symbol} + present = Symbol[n for n in names if haskey(kwargs, n)] + if length(present) > 1 throw( CTBase.IncorrectArgument( "Conflicting aliases for the same option"; @@ -95,4 +129,5 @@ function _extract_action_kwarg(kwargs::Base.Pairs, names::Tuple{Vararg{Symbol}}, ), ) end + return present end diff --git a/test/suite/helpers/test_kwarg_extraction.jl b/test/suite/helpers/test_kwarg_extraction.jl index c0699d80d..98918263e 100644 --- a/test/suite/helpers/test_kwarg_extraction.jl +++ b/test/suite/helpers/test_kwarg_extraction.jl @@ -162,6 +162,41 @@ function test_kwarg_extraction() end end + # ==================================================================== + # UNIT TESTS - Action alias conflict check + # ==================================================================== + + Test.@testset "_check_action_aliases" begin + names = OptimalControl._INITIAL_GUESS_ALIASES + + Test.@testset "No alias present" begin + present = OptimalControl._check_action_aliases( + pairs((; display=false)), names + ) + Test.@test present == Symbol[] + end + + Test.@testset "One alias present" begin + present = OptimalControl._check_action_aliases( + pairs((; init=1, display=false)), names + ) + Test.@test present == [:init] + end + + Test.@testset "Both aliases present" begin + err = try + OptimalControl._check_action_aliases( + pairs((; init=1, initial_guess=2)), names + ) + nothing + catch e + e + end + Test.@test err isa CTBase.IncorrectArgument + Test.@test occursin("Conflicting aliases", sprint(showerror, err)) + end + end + # ==================================================================== # PERFORMANCE TESTS # ==================================================================== diff --git a/test/suite/solve/test_descriptive_routing.jl b/test/suite/solve/test_descriptive_routing.jl index 5743c4776..5ea4ae968 100644 --- a/test/suite/solve/test_descriptive_routing.jl +++ b/test/suite/solve/test_descriptive_routing.jl @@ -254,6 +254,23 @@ function test_descriptive_routing() ) end + Test.@testset "_route_descriptive_options - error on init and initial_guess" begin + # Regression test for #964: the leftover alias used to be routed to the + # strategies and reported as an unknown option. + err = try + OptimalControl._route_descriptive_options( + MOCK_METHOD, MOCK_REGISTRY, pairs((; init=1, initial_guess=2)) + ) + nothing + catch e + e + end + Test.@test err isa CTBase.IncorrectArgument + message = sprint(showerror, err) + Test.@test occursin("Conflicting aliases", message) + Test.@test !occursin("Unknown option", message) + end + Test.@testset "_route_descriptive_options - error on ambiguous option" begin Test.@test_throws CTBase.IncorrectArgument OptimalControl._route_descriptive_options( MOCK_METHOD, MOCK_REGISTRY, pairs((; backend=:sparse)) @@ -564,6 +581,27 @@ function test_descriptive_routing() ) Test.@test sol isa MockSolution2 end + + Test.@testset "solve_descriptive - init and initial_guess together" begin + ocp = MockOCP2() + err = try + OptimalControl.solve_descriptive( + ocp, + :collocation, + :adnlp, + :ipopt; + init=MockInit2(), + initial_guess=MockInit2(), + display=false, + registry=MOCK_REGISTRY, + ) + nothing + catch e + e + end + Test.@test err isa CTBase.IncorrectArgument + Test.@test occursin("Conflicting aliases", sprint(showerror, err)) + end end end