[Dev] Code improvement and bug fixes

[jbcaillau]

Likely errors / bugs (with suggested corrections)

1) Typo/bug: control_fun used without :

In multiple @match branches the code uses control_fun (a bare identifier) instead of the symbol :control_fun.

Example (from your snippet around constraint codegen):

:state_fun || control_fun || :mixed => begin

This will either:

• throw UndefVarError: control_fun not defined at macro-expansion/runtime (depending on how @match expands), or
• silently match something unintended (worst case).

Fix
Replace control_fun with :control_fun everywhere it appears in match patterns.

Search and fix in src/onepass.jl:

• :state_fun || control_fun || :mixed
• :state_fun || control_fun || :mixed in p_constraint_fun! too (I saw a similar pattern earlier: :state_fun || control_fun || :mixed => ...)

Permalink for one occurrence:

CTParser.jl/src/onepass.jl

Lines 828 to 843 in 5aad902

	:state_fun || control_fun || :mixed => begin
	code = :(length($e1) == length($e3) == 1 || throw("this constraint must be scalar")) # (vs. __throw) since raised at runtime
	xt = __symgen(:xt)
	ut = __symgen(:ut)
	e2 = replace_call(e2, [p.x, p.u], p.t, [xt, ut])
	j = __symgen(:j)
	e2 = subs2(e2, xt, p.x, j)
	e2 = subs2(e2, ut, p.u, j)
	e2 = subs(e2, p.t, :($(p.t0) + $j * $(p.dt)))
	concat(
	code,
	:($pref.constraint(
	$p_ocp, $e2 for $j in 0:grid_size; lcon=($e1), ucon=($e3)
	)),
	)
	end

I used code search and results may be incomplete (search UI: https://github.com/control-toolbox/CTParser.jl/search?q=control_fun&type=code).

---

2) Possible semantic bug: as_range returns a Vector, but later code assumes a range-like object

as_range(x) = is_range(x) ? x : [x]

Then in several places you do:

length($e1) == length($e3) == length($rg) || throw("wrong bound dimension")

If rg is a scalar index (like 3), as_range(rg) returns [3], which is OK for length, but later some expressions expect rg to be valid in slicing contexts A[$rg] or in comprehensions for i in $rg.

That works for a Vector{Int} but:

• it changes iteration order/type vs 3:3
• it may allocate and inhibit inference in generated code (minor)
• more importantly, in other places you may rely on rg being an Expr representing a range (not a realized array) if you’re building quoted code.

Safer fix (design + correctness)
Use an expression-level normalization:

• if it’s not a range, turn it into :(($x):($x)) (a 1-length UnitRange) instead of [x].
  • At AST level: Expr(:call, :(:), x, x).

So something like:

as_range_expr(x) = is_range(x) ? x : :($x:$x)

and use that consistently in code generation paths (especially :exa), where rg is interpolated into quoted expressions.

---

3) Potential scoping hazard: __wrap introduces local ex, prints, then throw(ex)

__wrap is:

quote
    local ex
    try
        $e
    catch ex
        println("Line ", $n, ": ", $line)
        throw(ex)
    end
end

This is mostly fine, but:

• it prints to stdout unconditionally (might be undesirable in libraries; better to use @error or rethrow with enriched exception)
• it loses stack context if later code catches/rethrows (it preserves original exception though)

Improvement
Prefer rethrow() inside catch to preserve backtrace:

catch
    println(...)
    rethrow()
end

If you need the exception object, do:

catch ex
    println(...)
    rethrow(ex)
end

In Julia, rethrow() preserves the original backtrace best.

---

4) Alias substitution loop may be order/termination sensitive

In parse!:

for a in keys(p.aliases)
    e = subs(e, a, p.aliases[a])
end

Because aliases can be Expr keys and values can introduce other aliases, a single pass may yield partially-expanded expressions, and alias ordering can change semantics.

Fix / improvement

• Either document “single-pass aliasing” explicitly, or
• Apply substitution until fixpoint (with max-iteration guard), or
• Separate “lexical sugar aliases” (like <=) from “user aliases” and apply in phases.

---

Performance / design / refactoring suggestions

A) Reduce duplication via a backend “emitter” object (strategy pattern)

You currently have a pattern:

• p_variable! does generic validation + updates p
• then calls parsing(:variable, backend)(...)
• and separately you maintain p_variable_fun!, p_variable_exa!, etc.

That’s already a strategy pattern, but it’s implemented via global dispatch tables (presumably parsing elsewhere) + many parallel functions.

Refactor idea
Define small backend types:

abstract type Backend end
struct FunBackend <: Backend end
struct ExaBackend <: Backend end

Then implement:

emit_variable!(::FunBackend, p, p_ocp, ...)
emit_variable!(::ExaBackend, p, p_ocp, ...)

This improves:

• discoverability (methods grouped by concept)
• testability (backend behavior can be unit-tested with the same front-end parser calls)
• avoids symbol-based dispatch and reduces runtime branching

B) Consolidate the huge objective parsing match table

The parse! objective section enumerates many algebraic permutations for Bolza/Lagrange with factors and signs, primarily to enforce “the prefactor must not depend on time”.

Refactor idea
Normalize objective expressions first:

• parse the objective as an AST
• extract (mayer_term, lagrange_integrand, sense)
• rewrite e1 - ∫(e2) into (e1, -e2) etc.
• check time-dependence in exactly one place

This will shrink parse! substantially and reduce bug surface.

C) Avoid repeated has(expr, p.t) scans by caching

You repeatedly compute autonomy checks like:

xut = __symgen(:xut)
ee = replace_call(e, [p.x, p.u], p.t, [xut, xut])
has(ee, p.t) && (p.is_autonomous = false)

has likely walks the whole AST each time.

Improvement

• Have replace_call optionally return a flag “time present” while transforming, or
• Maintain a lightweight contains_time::Bool returned by a single traversal.

D) Exa box constraints: accumulate data, not code

For :exa, you build code blocks p.box_x = concat(p.box_x, code_box) etc. That means you’re building large quoted expressions incrementally.

Improvement
Accumulate data in ParsingInfo (e.g., store a vector of (kind, rg, lb_expr, ub_expr, lineinfo)), then at the end generate code once. Benefits:

• less AST concatenation overhead
• easier to validate consistency (e.g., conflicting bounds)
• enables better error messages (“bound set twice”)

E) Consistent “range normalization” across fun/exa

Right now, range/index handling differs by backend (and some comments mention :fun allows i to be a range for dynamics, but :exa doesn’t). Consider defining explicit policy per construct:

• dynamics coord index: must be integer for all backends (or make a separate syntax for ranges)
• constraints range: always accept scalar or range, normalize to UnitRange at AST level

This makes the DSL more predictable.

---

One concrete “must-fix” summary

1. Replace every control_fun in @match patterns with :control_fun.
2. Consider changing as_range to return an AST range (x:x) rather than allocating [x] when generating code (especially for :exa).
3. Prefer rethrow() in __wrap to preserve backtraces.

If you want, paste the remainder of onepass.jl after line ~996 (where your snippet cut off), and I can check the rest for the same control_fun typo and any similar symbol-pattern issues.

[jbcaillau]

Issues / likely bugs in the remainder

1) p_lagrange_exa! uses $ej1 in the :euler_implicit branch but should likely use $ej2

In your pasted block (this is inside p_lagrange_exa!), you have:

elseif scheme ∈ (:euler_implicit, :euler_b) # euler_b is deprecated
    $pref.objective($p_ocp, $(p.dt) * $ej1 for $j1 in 1:grid_size)

If the intent is “implicit Euler uses the value at the right endpoint”, then the integrand should be evaluated at j2 = j1+1 (or, more simply, evaluate at j for j=1:grid_size). That would typically correspond to ej2 (or an expression built from j directly), not ej1.

Given how dynamics were done earlier:

• explicit Euler used ej1 (at j)
• implicit Euler used ej2 (at j+1)

So this looks like a copy/paste bug.

Suggested correction
Either:

• change $ej1 to $ej2 and keep the same indexing semantics as dynamics, or
• redefine the loop variable so the integrand is evaluated at the correct grid points.

For example (conceptually):

elseif scheme ∈ (:euler_implicit, :euler_b)
    $pref.objective($p_ocp, $(p.dt) * $ej2 for $j1 in 0:grid_size-1)

or if you want j directly:

$pref.objective($p_ocp, $(p.dt) * (integrand at j) for j in 1:grid_size)

As written, it’s inconsistent and likely wrong numerically.

---

2) Precedence / range parsing ambiguity: 0:grid_size-1 etc.

You use:

for $j1 in 0:grid_size-1

and similarly 1:grid_size-1.

In Julia, : has lower precedence than -? Actually : is an operator with fairly low precedence, but the safe/readable form is always parenthesized. The current form is easy to misread and is brittle.

Suggested correction (readability + safety)
Write:

for $j1 in 0:(grid_size - 1)
for $j1 in 1:(grid_size - 1)

Even if the current parse is correct, this avoids accidental changes later.

---

3) p_mayer_exa!: redundant/possibly inconsistent substitutions for x0 / xf

You do both:

e = subs2(e, x0, p.x, 0)
e = subs(e, x0, :([$(p.x)[$k, 0] for $k ∈ 1:$(p.dim_x)]))

and similarly for xf.

That implies:

• first, replace x[i](t0) with x[i,0] (good)
• then, replace occurrences of bare x0 with a full vector comprehension (also reasonable)

But doing both can be risky if subs2 also introduces x0 in some structured way (depending on what subs2 does), or if x0 appears in nested contexts.

Suggested improvement
If the goal is:

• x(t0) (vector call) becomes x[:,0]-like,
• x[i](t0) becomes x[i,0],
  then it’s cleaner to normalize both cases explicitly, e.g.:
• replace x(t0) with a vector expression
• replace x[i](t0) with scalar indexing

Without seeing replace_call/subs2, I can’t prove it’s wrong, but it’s a hotspot for subtle substitution bugs. At minimum, add tests:

• objective with x(t0) vector form
• objective with x[3](t0) scalar form
• mixed use in same expression

---

4) p_mayer_exa!: objective sense (min/max) seemingly ignored

In p_mayer_exa!(..., type) you do:

code = :($pref.objective($p_ocp, $e))

No sign flip for :max, no minimize= flag here.

Later in def_exa, you construct:

$pref.ExaCore(...; minimize=($p.criterion == :min))

So the “sense” is handled at the model level, not per-objective call. That can be fine, but then p_mayer_exa! shouldn’t even accept type (or should assert it matches p.criterion once set). Otherwise it’s misleading and easy to break if ExaModels’ API changes.

Suggested correction

• Either remove type from p_mayer_exa! signature and rely on p.criterion,
• Or assert consistency:

(!isnothing(p.criterion) && p.criterion != type) && throw(...)

Same remark applies to p_bolza_exa! passing type through when ExaCore already decides minimize/maximize globally.

---

5) Comment typo: “Summary … (:fun backend)” repeated for EXA

Minor, but you have:

# Summary of available parsing subfunctions (:fun backend)
const PARSING_EXA = ...

Should say (:exa backend).

---

6) macro def_exa(e) calls def_exa(e) inside: name shadowing risk / confusion

You define:

macro def_exa(e)
    try
        code = def_exa(e)

There is also a function def_exa(e; log=false).

This is legal (macro and function in different namespaces), but it is confusing and can lead to accidental recursion if refactoring changes resolution.

Suggested improvement
Rename one side, e.g.:

• macro @def_exa calls def_exa_fun(e) or build_exa_closure(e)

---

7) Getter closure in def_exa: base_type[] looks suspicious

Inside getter:

isnothing($(p.dim_v)) ? base_type[] : $pref.solution(sol, $(p.v))

base_type is a keyword argument defaulting to __default_base_type_exa(); likely a Type (e.g. Float64). Doing base_type[] is not valid for a type. It would be valid for a Ref or 0-d array.

Maybe base_type is expected to be Ref{DataType}? But in the function signature it’s passed as a kwarg, and then used earlier as:

$pref.ExaCore(base_type; backend=backend, minimize=...)

which suggests base_type is a type, not a ref.

This is a likely runtime error if dim_v is nothing and someone asks val=:variable or variable multipliers.

Suggested correction
Return something appropriate:

• an empty vector base_type[] if you meant “empty Vector{base_type}()”, then write:

  Vector{base_type}()   # not valid either: base_type is a value, but you can do:
  Vector{T}() where T

  In generated code, you can do:

  :(Vector{$(base_type)}())

  But since base_type is a runtime kwarg, you’d do:

  Vector{base_type}()  # this works if base_type is a Type

• or zero(base_type) if you meant a scalar default
• or simply nothing

So one concrete fix could be:

isnothing($(p.dim_v)) ? Vector{base_type}() : $pref.solution(sol, $(p.v))

and similarly for variable_l / variable_u.

---

Refactoring / design improvements (specific to this remainder)

A) Replace OrderedDict{Symbol,Function} backend registries with multiple dispatch

You have:

const PARSING_FUN = OrderedDict{Symbol,Function}()
...
function parsing(s, backend)
    return PARSING_DIR[backend][s]
end

This is flexible but:

• not type-stable (dictionary lookup of Function)
• harder for the compiler to inline
• harder to navigate

Improvement
Use multiple dispatch as mentioned earlier, or at least store Val-based dispatch:

parsing(::Val{:variable}, ::Val{:fun}) = p_variable_fun!

This becomes compile-time resolvable.

B) Consider factoring scheme-dependent quadrature into one utility

You now have scheme logic in dynamics and objective integration. They should share a single discretization policy object (or function) so you can’t fix Euler implicit in one place and forget the other.

For example:

• grid_nodes(scheme, grid_size) returning which indices are used
• eval_time(j, scheme) etc.

C) Make def_exa build a structured model then emit code

def_exa currently does parsing and directly emits a big quoted closure that:

• declares box arrays
• creates ExaCore
• emits constraints/objectives
• validates dynamics coverage
• returns model + getter

This works, but is hard to test.

Refactor direction
Let parsing produce an intermediate representation (IR):

• declarations (dims, names)
• constraints list
• objective spec
• dynamics specs
  Then have a separate “codegen” step for :exa. That makes it much easier to:
• unit-test IR extraction without evaluating generated code
• run consistency checks (like dynamics completeness) without quote-walking

---

Most important fixes to implement first

1. Fix p_lagrange_exa! implicit Euler objective branch (ej1 → likely ej2 / correct indexing).
2. Parenthesize 0:(grid_size-1) etc.
3. Fix base_type[] in getter (likely Vector{base_type}()).

If you tell me what base_type is supposed to be (a Type like Float64, or a Ref, etc.) and how pref.objective is defined for ExaModels in this project, I can propose the most consistent return types and objective discretization for each scheme.

[jbcaillau]

Yes—your concern is correct in general:

• If you generate code after the macro (e.g., at runtime) and then eval it, you can hit world-age issues (and also module scoping issues).
• Generating code inside the macro expansion (or returning a closure built from already-compiled functions) avoids that entire class of problems.

That said, there are a couple of nuances.

Why eval tends to cause world-age trouble

Macros run at parse/expand time and return an expression that becomes part of the caller’s code. That code is compiled in the current “world” and can call newly-defined methods normally.

If instead you do something like:

1. build IR at runtime,
2. turn IR into an Expr,
3. eval it (possibly inside a function),

then the newly-evaluated methods/functions live in a newer world than the currently-running code. Calling them immediately from the older world can trigger the classic error:

“MethodError … the applicable method may be too new: running in world age …”

You can work around it with Base.invokelatest, but that has overhead and is usually not what you want for a modeling DSL.

Also, eval always evaluates in a module’s global scope (not local), which is often awkward for capturing local variables / hygiene.

How IR + codegen can still be macro-safe (no runtime eval)

You can absolutely do IR production + codegen inside the macro:

• Macro parses the DSL block → produces an IR object (could be a Julia struct literal embedded in the returned AST, or even computed during expansion).
• Macro then emits backend code by “lowering” that IR to an expression.
• The returned expression contains normal Julia code (closures, calls to backend constructors, etc.) and no eval.

So the pipeline is:

DSL Expr → (macro-time) IR → (macro-time) generated Expr → compiled normally

No world-age issue.

A good pattern: build IR at macro-time, but keep it data-oriented

If your IR is a Julia struct, you have two macro-friendly options:

1. Build IR as Julia values during macro expansion
   The macro code runs in the macro’s module, so it can construct IR objects then immediately emit code from them. You do not need to serialize the IR into the output AST unless you want it for debugging.

2. Embed the IR as a literal in the expanded code (optional)
   E.g. return something like:

   let ir = CTParser.IR(...literal data...)
       CTParser.codegen(ir; backend=:exa)
   end

   This still avoids eval as long as codegen(ir) returns runnable code without defining new methods (i.e., it constructs model objects / closures, not new top-level function definitions that must be compiled via eval).

However, note: if codegen(ir) itself returns an Expr that you then want to execute, you’re back to needing eval. So in that style, codegen should produce executable objects directly, or the macro should splice the expression produced by codegen.

One nuance: do you really need to “define functions” in generated code?

In your current design, you often generate:

function fun(args...)
   ...
end
pref.constraint!(... f=fun ...)

That’s fine inside macro expansion: those are local function definitions inside the returned quoted block; no eval needed.

If you moved codegen to runtime, and it tried to create new methods by eval, that’s where world age bites.

Practical takeaway

• IR + codegen is compatible with macro expansion and does not imply eval.
• Runtime eval is the common source of world age issues, and avoiding it is generally the right move for a DSL like this.
• If you ever must generate runtime code, you can use invokelatest as a fallback, but it’s usually slower and less pleasant.

If you sketch how you envision the IR (fields, contents), I can suggest a macro-friendly architecture that keeps your current “backend :fun and :exa” split while eliminating most of the duplication without introducing eval/world-age problems.

[jbcaillau]

One concrete “must-fix” summary

1. Replace every control_fun in @match patterns with :control_fun.

nice catch. corrected. some parsing errors where not detected as everything parsed to control_fun; usually fine as :control_fun and :mixed lead to the same code, but missed to raise errors on :other type constraint

2. Consider changing as_range to return an AST range (x:x) rather than allocating [x] when generating code (especially for :exa).

done

3. Prefer rethrow() in __wrap to preserve backtraces.

If you want, paste the remainder of onepass.jl after line ~996 (where your snippet cut off), and I can check the rest for the same control_fun typo and any similar symbol-pattern issues.

done

[jbcaillau]

Most important fixes to implement first

1. Fix p_lagrange_exa! implicit Euler objective branch (ej1 → likely ej2 / correct indexing).

no. this is actually correct

2. Parenthesize 0:(grid_size-1) etc.

no. useless (and added by formatter)

3. Fix base_type[] in getter (likely Vector{base_type}()).

If you tell me what base_type is supposed to be (a Type like Float64, or a Ref, etc.) and how pref.objective is defined for ExaModels in this project, I can propose the most consistent return types and objective discretization for each scheme.

no. useless (it works like that and is equivalent to the proposed tweak)

[jbcaillau]

critical corrections above included in #182