diff --git a/src/Strategies/api/describe_registry.jl b/src/Strategies/api/describe_registry.jl index 428f0b44..6ef9e9cc 100644 --- a/src/Strategies/api/describe_registry.jl +++ b/src/Strategies/api/describe_registry.jl @@ -417,14 +417,18 @@ $(TYPEDSIGNATURES) Extract a clean type name from a UnionAll type by removing module prefixes. -This method handles generic types that have not been fully instantiated, preserving -the type parameter variable name. +This method handles generic types that have not been fully instantiated, preserving +the type parameter variable name(s). A `UnionAll` may wrap another `UnionAll` when +more than one type parameter remains unbound (e.g. a 4-parameter strategy with only +its first parameter applied), so this peels every layer rather than assuming exactly +one. # Arguments - `T::UnionAll`: The UnionAll type to format # Returns -- `String`: Clean type name with generic parameter (e.g., `"Exa{P}"` where P is the type variable) +- `String`: Clean type name with generic parameter(s) (e.g., `"Exa{P}"` where P is the + type variable, or `"SciML{O, Q, R}"` when several parameters remain unbound) # Notes - This is a fallback for generic types that are not yet instantiated @@ -433,9 +437,13 @@ the type parameter variable name. See also: [`CTBase.Strategies._strategy_type_name(::DataType)`](@ref) """ function _strategy_type_name(T::UnionAll) - base_name = string(T.body.name.name) - param_name = string(T.var.name) - return "$base_name{$param_name}" + var_names = String[] + body = T + while body isa UnionAll + push!(var_names, string(body.var.name)) + body = body.body + end + return "$(string(body.name.name)){$(join(var_names, ", "))}" end """ @@ -489,7 +497,9 @@ julia> _strategy_base_name(Collocation) # Notes - Used specifically for strategy headers to avoid redundancy with parameter display -- Handles both DataType and UnionAll types +- `UnionAll` (and any other type not covered by the `DataType` method above) falls through to + the `nameof`-based fallback below, which is correct regardless of how many type parameters + remain unbound See also: [`CTBase.Strategies._strategy_type_name`](@ref), [`CTBase.Strategies.describe`](@ref) """ @@ -497,10 +507,6 @@ function _strategy_base_name(T::DataType) return string(T.name.name) end -function _strategy_base_name(T::UnionAll) - return string(T.body.name.name) -end - function _strategy_base_name(T::Type) return string(nameof(T)) end diff --git a/src/Strategies/api/registry.jl b/src/Strategies/api/registry.jl index 093d6e5c..3635c75f 100644 --- a/src/Strategies/api/registry.jl +++ b/src/Strategies/api/registry.jl @@ -243,8 +243,45 @@ function create_registry(pairs::Pair...) ) end - # Create parameterized strategy type - push!(strategies, strategy_type{param_type}) + # Create parameterized strategy type. The strategy parameter must be + # the FIRST declared type parameter — every `Strategies.parameter` + # implementation in the ecosystem reads slot 1 via + # `parameter(::Type{<:X{P}}) where {P} = P`. Round-trip through that + # contract method to validate the binding actually landed there, + # rather than inspecting TypeVar positions/bounds by hand. + applied_type = try + strategy_type{param_type} + catch e + e isa TypeError || rethrow() + throw( + Exceptions.IncorrectArgument( + "Strategy parameter is not compatible with the first type parameter"; + got="$strategy_type applied to $param_type", + expected="$param_type to satisfy the bound of $strategy_type's first type parameter", + suggestion="Declare the strategy parameter as the FIRST type parameter, e.g. struct $strategy_type{P<:AbstractStrategyParameter, ...}", + context="create_registry - binding strategy parameter to type", + ), + ) + end + + bound_param = try + parameter(applied_type) + catch + nothing + end + if bound_param !== param_type + throw( + Exceptions.IncorrectArgument( + "Strategy parameter must be the first type parameter"; + got="parameter($applied_type) returned $bound_param, expected $param_type", + expected="Strategies.parameter to return $param_type for $applied_type", + suggestion="Declare the strategy parameter as the FIRST type parameter, e.g. struct $strategy_type{P<:AbstractStrategyParameter, ...}", + context="create_registry - validating strategy parameter is in the first type-parameter slot", + ), + ) + end + + push!(strategies, applied_type) end else # Non-parameterized strategy: Type (can be UnionAll for parameterized types with default) diff --git a/src/Strategies/contract/abstract_strategy.jl b/src/Strategies/contract/abstract_strategy.jl index 97aa4771..09a992eb 100644 --- a/src/Strategies/contract/abstract_strategy.jl +++ b/src/Strategies/contract/abstract_strategy.jl @@ -39,6 +39,22 @@ Every concrete strategy must provide: 3. **Constructor** accepting keyword arguments (uses `build_strategy_options`) 4. **Instance-level access** to configured options +## Parameter Position Contract + +For strategies registered with a device/backend parameter via `create_registry` +(e.g. `(MyStrategy, [CPU, GPU])`), the parameter **must be the first declared type +parameter**: + +```julia-repl +julia> struct MyStrategy{P<:AbstractStrategyParameter, O} <: AbstractStrategy end # ✓ correct +julia> struct MyStrategy{O, P<:AbstractStrategyParameter} <: AbstractStrategy end # ✗ wrong slot +``` + +This is required because every `Strategies.parameter` implementation in the ecosystem reads +slot 1: `parameter(::Type{<:MyStrategy{P}}) where {P} = P`. `create_registry` validates this +by round-tripping through `parameter`, raising `Exceptions.IncorrectArgument` if the parameter +did not bind to the first slot. + ## Validation Modes The strategy system supports two validation modes for option handling: diff --git a/test/suite/differentiation/test_di_parameter.jl b/test/suite/differentiation/test_di_parameter.jl index cdba02b9..37ba1f06 100644 --- a/test/suite/differentiation/test_di_parameter.jl +++ b/test/suite/differentiation/test_di_parameter.jl @@ -104,6 +104,18 @@ function test_di_parameter() Test.@test Strategies.extract_global_parameter_from_method((:di, :gpu), r) == Strategies.GPU end + + Test.@testset "describe(:di, registry)" begin + r = Strategies.create_registry( + Differentiation.AbstractADBackend => + ((DI, [Strategies.CPU, Strategies.GPU]),), + ) + + buf = IOBuffer() + Test.@test_nowarn Strategies.describe(buf, :di, r) + output = String(take!(buf)) + Test.@test occursin("DifferentiationInterface", output) + end end end diff --git a/test/suite/strategies/test_describe_registry.jl b/test/suite/strategies/test_describe_registry.jl index 1cb0086c..ddbfddd5 100644 --- a/test/suite/strategies/test_describe_registry.jl +++ b/test/suite/strategies/test_describe_registry.jl @@ -10,6 +10,31 @@ const SHOWTIMING = isdefined(Main, :TestData) ? Main.TestData.SHOWTIMING : true struct FakeGenericStrat{P<:Strategies.AbstractStrategyParameter} end struct FakeConcreteStrat end +# Parametric-arity matrix (issue #516): strategies with 1, 2, 3 and 4 total type +# parameters, only the first of which is the strategy parameter. `describe(id, registry)` +# must work for all of them — 3+ used to throw `FieldError` because the private helpers +# assumed a `DataType` sat directly under a single `UnionAll` layer. +abstract type FakeArityFamily <: Strategies.AbstractStrategy end +struct FakeArityP1{P<:Strategies.AbstractStrategyParameter} <: FakeArityFamily end +struct FakeArityP2{P<:Strategies.AbstractStrategyParameter,O} <: FakeArityFamily end +struct FakeArityP3{P<:Strategies.AbstractStrategyParameter,O,Q} <: FakeArityFamily end +struct FakeArityP4{P<:Strategies.AbstractStrategyParameter,O,Q,R} <: FakeArityFamily end + +Strategies.id(::Type{<:FakeArityP1}) = :arity_p1 +Strategies.id(::Type{<:FakeArityP2}) = :arity_p2 +Strategies.id(::Type{<:FakeArityP3}) = :arity_p3 +Strategies.id(::Type{<:FakeArityP4}) = :arity_p4 + +Strategies.parameter(::Type{<:FakeArityP1{P}}) where {P} = P +Strategies.parameter(::Type{<:FakeArityP2{P}}) where {P} = P +Strategies.parameter(::Type{<:FakeArityP3{P}}) where {P} = P +Strategies.parameter(::Type{<:FakeArityP4{P}}) where {P} = P + +Strategies.metadata(::Type{<:FakeArityP1}) = Strategies.StrategyMetadata() +Strategies.metadata(::Type{<:FakeArityP2}) = Strategies.StrategyMetadata() +Strategies.metadata(::Type{<:FakeArityP3}) = Strategies.StrategyMetadata() +Strategies.metadata(::Type{<:FakeArityP4}) = Strategies.StrategyMetadata() + function test_describe_registry() Test.@testset "Describe registry - private helpers" verbose = VERBOSE showtiming = SHOWTIMING begin @@ -42,6 +67,40 @@ function test_describe_registry() Test.@test Strategies._strategy_type_name(Union{Int,String}) == string(Union{Int,String}) end + + # ==================================================================== + # REGRESSION - public describe(id, registry) across type-parameter arity + # ==================================================================== + + for (id_symbol, strat_type, label) in ( + (:arity_p1, FakeArityP1, "1 type parameter"), + (:arity_p2, FakeArityP2, "2 type parameters"), + (:arity_p3, FakeArityP3, "3 type parameters"), + (:arity_p4, FakeArityP4, "4 type parameters"), + ) + Test.@testset "describe(:$id_symbol, registry) - $label" begin + r = Strategies.create_registry( + FakeArityFamily => ((strat_type, [Strategies.CPU, Strategies.GPU]),) + ) + buf = IOBuffer() + Test.@test_nowarn Strategies.describe(buf, id_symbol, r) + output = String(take!(buf)) + Test.@test occursin(string(nameof(strat_type)), output) + end + end + + Test.@testset "describe(:cpu, registry) with a 3-param strategy present" begin + # Blast-radius regression: the parameter branch walks every strategy in the + # registry and previously hit the same UnionAll-depth bug via + # `_strategy_type_name` when rendering "used by strategies". + r = Strategies.create_registry( + FakeArityFamily => ((FakeArityP3, [Strategies.CPU, Strategies.GPU]),) + ) + buf = IOBuffer() + Test.@test_nowarn Strategies.describe(buf, :cpu, r) + output = String(take!(buf)) + Test.@test occursin("FakeArityP3", output) + end end end diff --git a/test/suite/strategies/test_registry_parameters.jl b/test/suite/strategies/test_registry_parameters.jl index 87ec80f5..d1107dde 100644 --- a/test/suite/strategies/test_registry_parameters.jl +++ b/test/suite/strategies/test_registry_parameters.jl @@ -38,6 +38,16 @@ struct FakeStratC <: FakeFamily end Strategies.id(::Type{<:FakeStratC}) = :fakestrata # Same as FakeStratA Strategies.parameter(::Type{<:FakeStratC}) = nothing +# Parameter-position contract (issue #516 item 3): the strategy parameter must be the +# FIRST declared type parameter. These two fakes reproduce the issue's failure shapes. +struct FakeTightStrat{O<:Strategies.StrategyOptions,P<:Strategies.AbstractStrategyParameter} <: + FakeFamily end +Strategies.id(::Type{<:FakeTightStrat}) = :fake_tight + +struct FakeLooseStrat{A,P<:Strategies.AbstractStrategyParameter} <: FakeFamily end +Strategies.id(::Type{<:FakeLooseStrat}) = :fake_loose +Strategies.parameter(::Type{<:FakeLooseStrat{A,P}}) where {A,P} = P + function test_registry_parameters() Test.@testset "Registry with Parameters" verbose=VERBOSE showtiming=SHOWTIMING begin @@ -88,6 +98,22 @@ function test_registry_parameters() ) end + Test.@testset "create_registry validation - parameter not in first slot (TypeError case)" begin + # FakeTightStrat's first type parameter O<:StrategyOptions rejects CPU outright. + Test.@test_throws Exceptions.IncorrectArgument Strategies.create_registry( + FakeFamily => ((FakeTightStrat, [Strategies.CPU]),) + ) + end + + Test.@testset "create_registry validation - parameter not in first slot (silent mismatch case)" begin + # FakeLooseStrat's first type parameter A is unbounded, so CPU binds to A + # instead of the intended P — `parameter(applied)` then returns something + # other than CPU (or throws), which the new check must catch. + Test.@test_throws Exceptions.IncorrectArgument Strategies.create_registry( + FakeFamily => ((FakeLooseStrat, [Strategies.CPU]),) + ) + end + # ==================================================================== # UNIT TESTS - Global ID uniqueness # ====================================================================