control-toolbox / control-toolbox/CTParser.jl

[Dev] Code improvement and bug fixes

Open
#197 5 comments 0 reactions 1 assignee Claimed by @jbcaillau View on GitHub
internal dev
Dominant language
Julia
Stars
3
Forks
0
Avg merge
4h 22m
Merged PRs (30d)
2

Description

## 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):
```julia
: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:
https://github.com/control-toolbox/CTParser.jl/blob/5aad902df8ce3ccdb07b864781f488c4666c6f0e/src/onepass.jl#L828-L843

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:
```julia
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:
```julia
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:
```julia
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:
```julia
catch
println(...)
rethrow()
end
```
If you need the exception object, do:
```julia
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!`:
```julia
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:
```julia
abstract type Backend end
struct FunBackend <: Backend end
struct ExaBackend <: Backend end
```
Then implement:
```julia
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:
```julia
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.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.