Constructive Code Review

[ocots]

CTParser.jl — Constructive Code Review

Reviewed against the sources in utils.jl, onepass.jl, initial_guess.jl, defaults.jl, and the module entry-point.
The package implements a DSL parser for optimal control problems, targeting two backends (:fun via CTModels, :exa via ExaModels).

---

Table of Contents

1. Overall Assessment
2. Architecture & Design
3. defaults.jl
4. utils.jl
5. onepass.jl — ParsingInfo
6. onepass.jl — parse! and dispatch
7. onepass.jl — Backend implementations
8. initial_guess.jl
9. Testing & Reliability
10. Documentation
11. Naming Conventions
12. Summary of Proposals

---

1. Overall Assessment

CTParser.jl is an ambitious and technically sophisticated package. Handling a mathematical DSL that maps seamlessly to both a functional representation (CTModels) and a directly-discretised NLP (ExaModels) in a single macro pass is genuinely difficult, and the general approach — one-pass @match-driven AST transformation producing Julia Expr trees — is sound.

That said, the current implementation accumulates significant incidental complexity: the two backends are deeply entangled, the ParsingInfo struct mixes parsing state with code generation artefacts, and many helper functions share confusingly similar names. The proposals below aim to reduce this complexity without changing the external DSL contract.

---

2. Architecture & Design

2.1 Symbol-based module references are fragile

const PREFIX_FUN = Ref(__default_prefix_fun())  # stores :CTModels

The entire code-generation layer uses Symbols like :CTModels or :ExaModels that are interpolated into Expr trees. At expansion time these resolve to whatever is in scope, making the package sensitive to the import context and impossible to verify statically.

Proposal. Introduce a thin abstract interface:

abstract type AbstractOCPBackend end
struct FunBackend{Mod} <: AbstractOCPBackend end
struct ExaBackend{Mod} <: AbstractOCPBackend end

or, more pragmatically, keep the Ref{Symbol} but add a validation step at backend registration time that checks the symbol resolves to a loaded module, and document clearly that the macro must be expanded in a scope where CTModels (resp. ExaModels) is imported.

2.2 Both backends always generate code

In def_fun:

if is_active_backend(:exa)
    build_exa = def_exa(e; log=log)
    code = concat(code, :($pref.build($p_ocp; build_examodel=($build_exa))))
end

The :exa code is generated unconditionally (unless the user calls deactivate_backend(:exa)). This means every @def invocation runs two full parsing passes and embeds a closure containing all the discretised ExaModels constraints into the compiled output, even when ExaModels is never used. This is wasteful in packages that only need the functional backend.

Proposal. Make :exa opt-in, not opt-out. Ship a separate @def_discrete macro (or a keyword argument @def ... backends=[:fun]) so the default path stays lean. The deactivate_backend API becomes an escape hatch rather than the normal workflow.

2.3 Asymmetric backend activation

backend == :fun && throw("backend :fun is always active")

The fact that :fun cannot be deactivated is a hardcoded assumption. If a user ever wants only the discretised form, they cannot get it. This also means the ACTIVE_PARSING_BACKENDS dict has mixed semantics (:fun is always true, others are toggleable).

Proposal. Either remove :fun from ACTIVE_PARSING_BACKENDS entirely and document that it is always the primary backend, or make all backends truly symmetric.

2.4 The getter closure is generated inside def_exa but used in def_fun

def_exa returns a function that closes over symbolic variable names (p.x, p.u, p.dyn_con, …). These are gensym-ed symbols whose values are only valid within the def_exa-generated closure scope. This works, but the boundary between what is "parsing-time" information and what is "runtime" information is invisible to a reader of def_fun.

Proposal. Extract the getter-building logic into a dedicated _build_getter(p) function with clear documentation of what p fields it consumes, and call it explicitly from def_exa.

---

3. defaults.jl

3.1 $(TYPEDSIGNATURES) on zero-argument functions

Every function in this file uses the $(TYPEDSIGNATURES) docstring extension, which renders as an empty signature (). The extension adds no value here and clutters the rendered documentation.

Proposal. Drop $(TYPEDSIGNATURES) from zero-argument functions, or switch to a plain docstring.

3.2 Ordering mismatch in __default_init_exa

__default_init_exa() = (0.1, 0.1, 0.1) # default init for v, x, u

The comment says (v, x, u), but in p_state_exa! the tuple is accessed as init[2] and in p_control_exa! as init[3], while p_variable_exa! uses init[1]. The indexing is therefore (variable, state, control) — not (v, x, u) as the comment states (same symbols, different conventional ordering).

Proposal. Fix the comment to # (variable, state, control) and add an assertion or @assert in the consuming functions to catch future misalignments.

3.3 Magic number 250 for grid size

The default grid size of 250 is not documented anywhere in terms of accuracy/performance trade-off.

Proposal. Add a note in the docstring explaining that 250 is a pragmatic default that gives roughly first-order accuracy of ~0.4% on smooth problems with the midpoint scheme, and that users should increase it for high-accuracy needs.

---

4. utils.jl

4.1 Confusing naming of the subs* family

The package has four substitution helpers with nearly identical names:

Function	What it does
subs	Replace any expression/symbol e1 by e2 in e
subs2	Replace x[i] → y[i,j] and x[rg] → comprehension
subs2m	Same but with midpoint average (y[i,j]+y[i,j+1])/2
subs3	Replace x[rg] → y[i,j] regardless of rg (loop variable)

The numbering conveys nothing about semantics. A reader encountering subs2m in p_lagrange_exa! has no intuition about what it does.

Proposal. Rename to convey intent:

subs          → expr_replace      # generic AST substitution
subs2         → expr_index_to_2d  # x[i](t) → y[i,j]
subs2m        → expr_index_to_2d_midpoint
subs3         → expr_index_to_2d_loop   # replaces range with loop var

4.2 replace_call accepts Nothing symbols silently

function replace_call(e, x::Vector{<:Union{Nothing,Symbol}}, t, y)
    ...
    if !isnothing(x[i]) && has(eee, x[i])

Allowing Nothing in the symbol vector means callers can pass [p.x, p.u] even when the control has not yet been declared (p.u === nothing). The Nothing is quietly skipped. This is pragmatic but masks a potential semantic error: the constraint/dynamics expression references a control that the parser has not yet seen.

Proposal. Add a pre-call assertion (or at minimum a logging warning) when nothing appears in the symbol vector, and document the contract explicitly. The check isnothing(p.u) && return __throw(...) pattern already exists in p_lagrange! — make it consistent everywhere.

4.3 expr_it bottom-up vs. top-down behavior

expr_it is a bottom-up (post-order) tree walker: it recursively processes children first, then applies _Expr to the reconstructed node. This is the right choice for substitution, but for replace_call — which needs to detect the pattern eee(tt) after children have already been substituted — it can produce unexpected results when substitutions are nested (e.g., ((x^2)(t0) + u[1])(t) in the docstring example shows this subtlety).

Proposal. Document the traversal order explicitly in the docstring of expr_it ("post-order / bottom-up") so callers understand why nested calls work the way they do.

4.4 has is O(n) and called repeatedly

In p_constraint!, p_dynamics!, etc., has(e, p.t) is called on potentially large expressions at every parsed line. has does a full tree walk returning :yes as a sentinel value (mixing data and control flow).

Proposal. The sentinel trick is clever but unusual — consider instead throwing a custom exception (:found) and catching it for an early-exit, or returning a Bool directly from expr_it via short-circuit. For the use-cases here the performance impact is small, but the readability cost is real.

---

5. onepass.jl — ParsingInfo

5.1 Parsing state and code accumulation are mixed in one struct

ParsingInfo currently holds two very different kinds of data:

1. Parser state: t, t0, tf, x, u, v, dim_*, is_autonomous, criterion, aliases, …
2. Code accumulation buffers: box_v, box_x, box_u, l_v, u_v, l_x, u_x, l_u, u_u, dyn_con, dyn_coords, dt

Fields of the second kind are only meaningful for the :exa backend and should not be present when parsing for :fun.

Proposal. Split into two structs:

@with_kw mutable struct ParsingContext     # shared, backend-agnostic
    v, t, t0, tf, x, u, dim_*, aliases, ...
end

@with_kw mutable struct ExaAccumulator     # exa-only code buffers
    dt, dyn_con, dyn_coords, l_v, u_v, box_v, ...
end

Pass an ExaAccumulator only to p_*_exa! functions. This also makes it impossible to accidentally access ExaModels-specific state from :fun handlers.

5.2 __symgen suffix collisions

__symgen(s...) = Symbol(s..., gensym())

gensym() in Julia returns a symbol like ##123, so __symgen(:p_ocp) produces :p_ocp##123. This is fine for hygiene, but within a single @def expansion several __symgen(:j) calls produce distinct j##n symbols, making the generated code hard to read in macro-expansion debugging (@macroexpand).

Proposal. This is a minor quality-of-life issue. Consider __symgen(s...) = gensym(string(s...)) which produces slightly cleaner names like ##p_ocp#123, or use a counter-based scheme for readability during development.

---

6. onepass.jl — parse! and dispatch

6.1 parse! is a monolithic 120-line match

The top-level parse! function is a single @match block covering all 40+ syntactic forms. While this centralises dispatch, it makes it very hard to:

• Add new syntax without risking shadowing an existing arm
• Test individual arms in isolation
• Understand the precedence of arms (e.g., why ∂($x[$i])($t) must come before ∂($x)($t))

Proposal. Keep parse! as the entry point but extract groups of arms into helper functions with documented priorities:

# In parse!:
_parse_variable!(p, p_ocp, e; ...) ||
_parse_time!(p, p_ocp, e; ...)     ||
_parse_dynamics!(p, p_ocp, e; ...) ||
_parse_constraint!(p, p_ocp, e; ...) ||
_parse_objective!(p, p_ocp, e; ...) ||
__throw("unknown syntax", ...)

Each helper returns nothing if the pattern doesn't match, or the generated code otherwise.

6.2 The lnum counter is fragile

parse! increments p.lnum at the top and decrements it when a LineNumberNode is encountered. This works for a flat block, but could drift if an exception is thrown mid-increment or if parse! is called recursively (the block case at the bottom does call itself recursively, which is correct, but the p.lnum = p.lnum - 1 on entry to the block path relies on the outer call having already incremented).

Proposal. Track lnum via LineNumberNode nodes directly rather than by incrementing a counter. Store the last seen LineNumberNode in p.last_lnn and update it whenever such a node is encountered:

e isa LineNumberNode => (p.last_lnn = e; e)

This makes the line number always accurate regardless of recursion depth.

---

7. onepass.jl — Backend implementations

7.1 p_constraint_exa! is over 120 lines with repeated patterns

The :exa constraint handler repeats a nearly-identical code pattern for :initial, :final, :variable_range, :state_range, :control_range. The pattern is:

1. Expand rg to a default range if nothing
2. Check that bound dimensions match
3. Either accumulate into box_* or emit an ExaModels.constraint call

Proposal. Extract a helper:

function _exa_range_constraint!(p, p_ocp, pref, rg, dim, e2_builder, e1, e3, box_ref)
    rg = isnothing(rg) ? :(1:$dim) : as_range(rg)
    check = :(length($e1) == length($e3) == length($rg) || throw("wrong bound dimension"))
    code = concat(check, e2_builder(rg, e1, e3))
    box_ref !== nothing && (box_ref[] = concat(box_ref[], __wrap(code, ...)))
    return code
end

7.2 Dynamics code generates all schemes but only one runs

In p_dynamics_exa!, the generated code contains an if scheme == :euler ... elseif ... end block that is embedded verbatim. This means the compiler sees all four scheme branches at runtime. Since scheme is a keyword argument to the returned closure (not a type parameter), the compiler cannot specialise on it, and all four code paths are always JIT-compiled.

Proposal. Make scheme a Val-wrapped parameter, or generate scheme-specific closures at ExaModel build time:

function (; scheme=Val(:midpoint), ...)
    _build_dynamics(scheme, ...)
end

_build_dynamics(::Val{:euler}, ...) = ...
_build_dynamics(::Val{:midpoint}, ...) = ...

This is more code but avoids dead branches in the hot path.

7.3 p_dynamics_coord_exa! fallback for non-integer i is an error

function p_dynamics_coord_exa!(p, p_ocp, x, i, t, e)
    return __throw("dynamics coordinate $i should be an integer", p.lnum, p.line)
end

function p_dynamics_coord_exa!(p, p_ocp, x, i::Integer, t, e)
    ...
end

The non-integer fallback always throws. This is correct in spirit but the error message is slightly misleading — if i is a symbolic expression (e.g., a variable range), the user may not understand why their syntax is rejected. The :fun backend supports i as a range via p_dynamics_coord_fun!, creating an undocumented discrepancy between backends.

Proposal. Make the discrepancy explicit in the error message:

"dynamics coordinate index $i must be a literal integer for the :exa backend (range indices are supported only by the :fun backend)"

And add a test that verifies this error is thrown with the right message.

---

8. initial_guess.jl

This is the cleanest file in the package. The design of _collect_init_specs with cross-spec substitution is well-thought-out. A few smaller points:

8.1 spec_subs grows linearly and is applied naively

for (pattern, replacement) in spec_subs
    rhs = subs(rhs, pattern, replacement)
end

Each new spec applies all previous substitutions in a linear scan. For n specs this is O(n²) tree walks. In practice n is small, so this is not a performance concern, but it could cause a subtlety: if spec A produces a new symbol that happens to match the pattern of an earlier (already-applied) substitution, it will be silently substituted again.

Proposal. Apply substitutions in a single combined pass (build a map and walk the tree once) or document the single-pass-per-spec semantics clearly.

8.2 The @init macro silently ignores unknown trailing rest arguments shape

if length(rest) == 1
    opt = rest[1]
    if opt isa Expr && opt.head == :(=) && opt.args[1] == :log

If the user writes @init ocp begin ... end true (bare Bool without log =), opt.head will fail the isa Expr test and the macro will throw a ParsingError about "Unsupported trailing argument". The error message should mention the correct syntax.

Proposal. The error message already says "Use log = true or log = false", which is good. Additionally, handle the opt isa Bool case with a more helpful message:

elseif opt isa Bool
    error("@init: use `log = $opt` (keyword syntax) instead of a bare `$opt`")

8.3 __gen_temporal_value runtime error message is very verbose

The generated error string (lines 80-100 of initial_guess.jl) constructs a multi-sentence diagnostic. While informative, it is generated via string concatenation with * in the expression tree, which means it allocates even in the non-error path (the string is built before the if check).

Proposal. Move the string construction inside the error branch:

if $arg_quoted != _expected
    error(lazy"Incorrect time variable in @init: used :$($arg_quoted) but expected :$(_expected)")
end

---

9. Testing & Reliability

The onepass.jl header lists several open TODOs:

# - add tests to compare runs of exa with scalar vs vec dynamics
# - tests exceptions (parsing and semantics/runtime)
# - add tests on ParsingError + run time errors
# - add assert for pre/post conditions and invariants

These are not minor items — they are the primary safety net for a macro-based DSL where a wrong @match arm silently produces bad generated code.

Proposal (priority order):

1. Post-condition checks: After each p_*_fun! call, assert that the generated Expr contains no residual occurrences of p.x, p.u, or p.t (as mentioned in the TODO). This catches incomplete substitutions early.
2. Exception tests: For every __throw(...) call, add a test that exercises the condition and checks the error message string (use @test_throws with a regex on the message).
3. Round-trip tests: Parse a known OCP, evaluate the generated code, and check that calling the resulting functions with known inputs returns known outputs.
4. Backend parity tests: For every supported syntax form, run both :fun and :exa backends and verify that the discretised solution converges to the functional one as grid_size → ∞.

---

10. Documentation

10.1 $(TYPEDSIGNATURES) on zero-argument functions

Mentioned above (§3.1). Also affects prefix_fun(), prefix_exa(), e_prefix() in onepass.jl — all zero-argument getters. The rendered docs will show an empty signature, which is unhelpful.

10.2 Internal functions exported implicitly

Functions like parsing, activate_backend, deactivate_backend, is_active_backend, prefix_fun!, prefix_exa!, e_prefix! are all public (no _ prefix, accessible as CTParser.foo). The module's EXPORTS doc-marker is mentioned in the module docstring but the actual export list is not shown in the provided sources. It is unclear which functions are intentionally public API vs. internal extension points.

Proposal. Add a section to the README/docs that clearly distinguishes:

• User API: @def, @init
• Extension points: prefix_fun!, activate_backend, etc. (for package authors building on CTParser)
• Internal: parse!, p_*_fun!, etc. (prefix with _ or document as internal)

10.3 The DSL syntax itself is not documented in-package

The @def docstring example shows valid syntax but doesn't document the full grammar. A BNF-like grammar table in the docs would help users understand what forms are supported.

---

11. Naming Conventions

Current	Issue	Proposed
__symgen	Double underscore is Python convention; unusual in Julia	_gensym or fresh_sym
subs, subs2, subs3, subs2m	Number suffix conveys no semantics	See §4.1
p_time_fun!, p_time_exa!	p_ prefix stands for "parse" but this is not obvious	parse_time_fun! or _emit_time_fun!
__throw, __wrap	Double underscore; also confusable with Julia's Base.throw	_parse_error, _wrap_with_context
PARSING_FUN, PARSING_EXA, PARSING_DIR	All-caps for mutable dicts that are not true constants	_backend_parsers_fun, etc., or use a struct
ParsingInfo	Fine, but "Info" is vague	ParserState or ParseContext
dyn_coords	Unclear whether this tracks "which coords have been defined"	defined_dyn_coords

---

12. Summary of Proposals

#	Priority	Location	Proposal
P1	High	Architecture	Make :exa opt-in, not opt-out (or add @def_discrete)
P2	High	onepass.jl	Split ParsingInfo into ParserState + ExaAccumulator
P3	High	Testing	Add post-condition assertions (has(generated_code, p.x) == false)
P4	High	Testing	Add exception tests for every __throw call site
P5	Medium	utils.jl	Rename subs2, subs3, subs2m with semantic names
P6	Medium	onepass.jl	Replace lnum increment/decrement with LineNumberNode tracking
P7	Medium	onepass.jl	Extract repeated p_constraint_exa! pattern into a helper
P8	Medium	onepass.jl	Use Val{scheme} dispatch to avoid dead scheme branches
P9	Medium	defaults.jl	Fix __default_init_exa comment ordering
P10	Medium	initial_guess.jl	Move error string construction inside the error branch
P11	Low	defaults.jl	Remove $(TYPEDSIGNATURES) from zero-argument functions
P12	Low	Architecture	Validate module symbols at backend registration time
P13	Low	Naming	Adopt _ prefix for internal helpers; parse_* prefix for parse sub-functions
P14	Low	Docs	Add a BNF/grammar table for the @def DSL

---

Review written against the sources provided. The package is well-conceived and the DSL is expressive; the proposals above are aimed at making the internals as clean and trustworthy as the public API already appears to be.

[ocots]

CTParser.jl — Design Principles Review (Part 2)

This comment complements the structural critique in the first comment.
It applies standard software design principles — SOLID, DRY, KISS, YAGNI — and Julia-specific patterns to the same codebase, with concrete refactoring proposals for each finding.

---

1. Single Responsibility Principle (SRP)

1.1 ParsingInfo is a God Object

📋 Applying Architecture Rule: Single Responsibility / God Object anti-pattern

ParsingInfo currently holds 25+ fields covering at least three distinct responsibilities:

Responsibility	Fields
Parser state (what has been declared)	t, t0, tf, x, u, v, dim_*, aliases, is_autonomous, criterion
Parse-progress bookkeeping	lnum, line, is_global_dyn, is_coord_dyn, dyn_coords
ExaModels code-generation buffers	dt, dyn_con, l_v, u_v, box_v, l_x, u_x, box_x, l_u, u_u, box_u

The third group is never read by the :fun backend — it exists solely to accumulate ExaModels variable names and box-constraint expressions. Yet every parse! call creates and carries these fields regardless of the active backend.

Proposal — split into three focused structs:

# 1. Shared parser state (both backends)
@with_kw mutable struct ParseContext
    v::Symbol = __symgen(:unset)
    t::Union{Symbol,Nothing} = nothing
    t0::Union{Real,Symbol,Expr,Nothing} = nothing
    tf::Union{Real,Symbol,Expr,Nothing} = nothing
    x::Union{Symbol,Nothing} = nothing
    u::Union{Symbol,Nothing} = nothing
    dim_v::Union{Integer,Symbol,Expr,Nothing} = nothing
    dim_x::Union{Integer,Symbol,Expr,Nothing} = nothing
    dim_u::Union{Integer,Symbol,Expr,Nothing} = nothing
    is_autonomous::Bool = true
    criterion::Union{Symbol,Nothing} = nothing
    aliases::OrderedDict = __init_aliases()
end

# 2. Parse-progress bookkeeping (both backends)
@with_kw mutable struct ParseProgress
    lnum::Int = 0
    line::String = ""
    is_global_dyn::Bool = false
    is_coord_dyn::Bool = false
    dyn_coords::Vector{Int} = Int[]
end

# 3. ExaModels code buffers (:exa backend only)
@with_kw mutable struct ExaCodeBuffers
    dt::Symbol = __symgen(:dt)
    dyn_con::Union{Symbol,Nothing} = nothing
    l_v::Symbol = __symgen(:l_v)
    u_v::Symbol = __symgen(:u_v)
    box_v::Expr = :(LineNumberNode(0, "box constraints: variable"))
    # ... etc.
end

ExaCodeBuffers is only instantiated when the :exa backend is active, and only passed to p_*_exa! functions. The :fun handler signatures never see it.

---

1.2 parse! handles syntax recognition, dispatch, and error formatting

📋 Applying Architecture Rule: Single Responsibility — functions longer than 50 lines, multiple if-else branches handling different concerns

parse! currently does three things in one 120-line @match block:

1. Alias substitution on the incoming expression
2. Pattern recognition (what kind of declaration is this?)
3. Dispatch to the backend-specific handler

Proposal — separate these three steps:

function parse!(ctx, progress, p_ocp, e; log=false, backend=:fun)
    e = _apply_aliases(ctx.aliases, e)          # step 1: pure substitution
    kind, args = _recognize(e, progress)        # step 2: pattern → tagged tuple
    return _dispatch(kind, args, ctx, progress, p_ocp; log, backend)  # step 3
end

_recognize is a pure function (no side effects, no mutation) that returns a tagged tuple like (:constraint, e1, e2, e3, label). This is independently testable without any backend machinery.

---

1.3 def_fun orchestrates too much

📋 Applying Architecture Rule: Single Responsibility — function names with implicit "and"

def_fun currently: parses the expression for :fun, conditionally parses it again for :exa, wraps both results, and assembles the final Expr. That is at least three responsibilities.

Proposal:

function def_fun(e; log=false)
    fun_code  = _gen_fun_code(e; log)
    exa_block = is_active_backend(:exa) ? _gen_exa_code(e; log) : nothing
    return _assemble_def(fun_code, exa_block)
end

Each helper has one job and can be tested in isolation.

---

2. Open/Closed Principle (OCP)

2.1 Adding a new backend requires modifying three constants

📋 Applying Architecture Rule: Open/Closed — avoid hard-coded type checks, design for extension without modification

const PARSING_BACKENDS = (:fun, :exa)           # must be modified
const PARSING_DIR = OrderedDict(...)             # must be modified
const ACTIVE_PARSING_BACKENDS = OrderedDict(...) # must be modified

Adding a hypothetical :jump backend (JuMP.jl) requires touching all three. The activate_backend / deactivate_backend functions also contain hardcoded guards:

backend == :fun && throw("backend :fun is always active")

This hard-codes the asymmetry between :fun and all other backends.

Proposal — a registration API:

abstract type AbstractParsingBackend end

struct BackendEntry
    handlers::OrderedDict{Symbol,Function}
    always_active::Bool
end

const BACKEND_REGISTRY = OrderedDict{Symbol,BackendEntry}()

function register_backend!(name::Symbol, handlers; always_active=false)
    BACKEND_REGISTRY[name] = BackendEntry(handlers, always_active)
end

# At load time:
register_backend!(:fun, PARSING_FUN; always_active=true)
register_backend!(:exa, PARSING_EXA)

A downstream package can now add a :jump backend by calling register_backend!(:jump, PARSING_JUMP) without touching CTParser's source. This is the Open/Closed principle in practice.

---

2.2 is_active_backend check in def_fun is a hard-coded branch

📋 Applying Architecture Rule: Open/Closed — avoid isa/typeof checks

if is_active_backend(:exa)
    build_exa = def_exa(e; log=log)
    ...
end

Every new backend would add another if is_active_backend(:new_backend) branch. With the registry above, this becomes:

for (name, entry) in BACKEND_REGISTRY
    entry.always_active && continue  # :fun already handled
    is_active_backend(name) && (code = concat(code, _gen_backend_code(name, e; log)))
end

---

3. Liskov Substitution Principle (LSP)

3.1 The two backends are not contract-compatible

📋 Applying Architecture Rule: Liskov Substitution — subtypes must honor parent contracts

The :fun backend for p_dynamics_coord! accepts any index type (integer or range). The :exa backend silently rejects non-integer indices with a throw. This means the two backends do not honour the same contract for the same DSL syntax:

# Works with :fun, throws with :exa
∂(x[1:2])(t) == [v(t), u(t)]

There is no LSP violation per se (the backends are not subtypes of a common abstract type), but the inconsistency is the symptom of a missing shared contract. If backends were required to implement a declared interface, this gap would be caught at registration time rather than at the user's @def.

Proposal. Define a trait or a check_capabilities function that each backend must implement, returning the set of syntax forms it supports. parse! can then emit a clear error — "this syntax is not supported by the :exa backend" — instead of a cryptic throw deep in p_dynamics_coord_exa!.

---

4. Dependency Inversion Principle (DIP)

4.1 Backends are referenced by Symbol, not by abstraction

📋 Applying Architecture Rule: Dependency Inversion — depend on abstractions, not concrete implementations

const PREFIX_FUN = Ref(:CTModels)
const PREFIX_EXA = Ref(:ExaModels)

Generated code does things like:

:($pref.variable!($p_ocp, $q, $vv))
# expands to: CTModels.variable!(p_ocp, q, vv)

The dependency on CTModels and ExaModels is inverted through string interpolation rather than through a proper abstraction. There is no way to statically verify that CTModels actually exports variable!, that its signature matches, or that the module is in scope at expansion time.

Proposal. Define a small module-level protocol as a @doc-annotated checklist (since Julia does not have interfaces in the formal sense):

# In CTParser:
"""
Backend module protocol. A module `M` qualifies as a CTParser functional
backend if it exports:
  - M.PreModel() → an OCP builder object
  - M.variable!(ocp, q, name) → nothing
  - M.state!(ocp, n, name) → nothing
  - M.control!(ocp, m, name) → nothing
  - M.time!(ocp; t0, tf, ...) → nothing
  - M.dynamics!(ocp, f) → nothing
  - M.constraint!(ocp, kind; ...) → nothing
  - M.objective!(ocp, type; ...) → nothing
  - M.build(ocp; ...) → Model
"""

Pair this with a validate_backend_protocol(mod::Module) function that checks the presence of each required name via isdefined, callable at package load time or in tests. This makes the implicit contract explicit and detectable.

---

5. DRY — Don't Repeat Yourself

5.1 Euler/midpoint/trapeze branches are copy-pasted across four functions

📋 Applying Architecture Rule: DRY — every piece of knowledge should have a single representation

The scheme-selection if scheme == :euler ... elseif scheme == :midpoint ... block appears identically (modulo variable names) in:

• p_dynamics_exa!
• p_dynamics_coord_exa!
• p_lagrange_exa!

Each repetition means a bug fix or a new scheme (e.g., :runge_kutta4) must be applied in three places.

Proposal. Extract the scheme logic into a single function that takes the pre-computed ej1, ej2, ej12, dxj expressions and the scheme name:

function _exa_scheme_expr(scheme_sym, dt, ej1, ej2, ej12, lhs, j1, grid_size)
    quote
        if $scheme_sym == :euler
            $pref.constraint($p_ocp, $lhs - $dt * $ej1 for $j1 in 0:(grid_size-1))
        elseif $scheme_sym == :midpoint
            $pref.constraint($p_ocp, $lhs - $dt * $ej12 for $j1 in 0:(grid_size-1))
        elseif $scheme_sym ∈ (:trapeze, :trapezoidal)
            $pref.constraint($p_ocp, $lhs - $dt * ($ej1 + $ej2)/2 for $j1 in 0:(grid_size-1))
        else
            throw("unknown scheme: $scheme_sym")
        end
    end
end

Adding :runge_kutta4 then requires touching exactly one function.

---

5.2 Box-constraint accumulation is repeated for v, x, and u

📋 Applying Architecture Rule: DRY

In p_variable_exa!, p_state_exa!, and p_control_exa!, the pattern

code_box = :($(p.l_x) = -Inf * ones($n); $(p.u_x) = Inf * ones($n))
p.box_x = concat(p.box_x, code_box)

is repeated three times with different field names. Similarly in p_constraint_exa! for :state_range, :control_range, :variable_range.

Proposal. A single helper:

function _init_box_code!(buffers, which::Symbol, dim)
    l = getfield(buffers, Symbol(:l_, which))
    u = getfield(buffers, Symbol(:u_, which))
    code = :($l = -Inf * ones($dim); $u = Inf * ones($dim))
    old = getfield(buffers, Symbol(:box_, which))
    setfield!(buffers, Symbol(:box_, which), concat(old, code))
end

called as _init_box_code!(exa_buffers, :x, n).

---

6. KISS — Keep It Simple

6.1 has uses a sentinel value to simulate early-exit

📋 Applying Architecture Rule: KISS — prefer simple solutions

has(e, e1) = begin
    foo(e1) = (h, args...) -> begin
        ee = Expr(h, args...)
        if :yes ∈ args        # ← sentinel check on every node
            :yes
        else
            ee == e1 ? :yes : ee
        end
    end
    expr_it(e, foo(e1), x -> x == e1 ? :yes : x) == :yes
end

The sentinel :yes bubbles up through the entire tree even after the match is found — there is no early exit. The approach also conflates the AST node type (Expr or Symbol) with a control-flow signal (:yes).

Proposal. Use a thrown exception for early exit — a well-known Julia pattern for tree search:

struct _Found end

function has(e, e1)
    try
        _has_walk(e, e1)
        return false
    catch ::_Found
        return true
    end
end

_has_walk(e, e1) = (e == e1 && throw(_Found()); nothing)
_has_walk(e::Expr, e1) = (e == e1 && throw(_Found()); foreach(a -> _has_walk(a, e1), e.args))

This exits immediately on the first match, is type-stable, and does not require expr_it at all for this use case.

---

6.2 __init_aliases pre-populates 20 entries that are almost never used

📋 Applying Architecture Rule: KISS / YAGNI

for i in 1:max_dim   # max_dim = 20
    al[Symbol(:R, CTBase.ctupperscripts(i))] = :(R^$i)
end

This creates aliases R¹ through R²⁰ eagerly at every @def expansion, regardless of whether the user ever writes R³ in their problem. The max_dim=20 keyword argument is also never called with a non-default value anywhere in the codebase.

Proposal. Either make the alias resolution lazy (look up Rⁿ on demand), or simply hard-code the most commonly used values (up to R⁶, say) and remove the max_dim parameter. Applying YAGNI: if nobody has needed R¹⁷ yet, don't generate it.

---

7. Julia-Specific Pattern: Multiple Dispatch for Backend Dispatch

7.1 Dictionary-based dispatch misses Julia's native extensibility

📋 Applying Architecture Rule: Multiple Dispatch for extensibility and clarity

Backend dispatch currently goes through a dictionary lookup:

function parsing(s, backend)
    return PARSING_DIR[backend][s]
end

# Usage:
parsing(:constraint, backend)(p, p_ocp, ...)

This is extensible (add entries to the dict) but bypasses Julia's method dispatch entirely, which means:

• No MethodError if a handler is missing — you get a KeyError at runtime instead
• No specialisation by the compiler
• No possibility to use @which or methods() to introspect what will be called

Proposal. Encode the backend as a type and dispatch natively:

struct FunBackend end
struct ExaBackend end

# Each handler is a method, not a dict entry:
_emit_constraint!(::FunBackend, p, p_ocp, e1, e2, e3, c_type, label) = ...
_emit_constraint!(::ExaBackend, p, p_ocp, e1, e2, e3, c_type, label) = ...

A new backend adds methods; it does not touch any existing dict. @which _emit_constraint!(ExaBackend(), ...) works for debugging. Missing methods produce a MethodError with a useful message.

---

Summary

Principle	Finding	Impact
SRP	ParsingInfo is a God Object (3 responsibilities)	High
SRP	parse! mixes recognition, substitution, dispatch	Medium
OCP	New backend requires editing 3 constants	High
OCP	is_active_backend check will grow with each new backend	Medium
LSP	:fun/:exa expose different syntax contracts silently	Medium
DIP	Backends referenced by Symbol, no static contract	High
DRY	Scheme branches copy-pasted across 3 functions	High
DRY	Box-constraint init repeated for v, x, u	Low
KISS	has sentinel trick — no early exit, mixes types	Low
KISS/YAGNI	20 eagerly-generated Rⁿ aliases	Low
Multiple Dispatch	Dict-based backend dispatch bypasses Julia's native mechanism	Medium

The highest-leverage refactoring is the combination of P1 from the first comment (make :exa opt-in) + the registration API above (OCP §2.1) + multiple dispatch for handlers (§7.1): together they decouple the two backends completely, make CTParser extensible to new targets, and align the package with idiomatic Julia.