refactor(computed): replace meval with built-in expression engine#77
Merged
Conversation
meval has been unmaintained since 2017 and its transitive nom 1.2.4 will be rejected by a future version of Rust. Evaluated evalexpr, fasteval, and exmex as replacements; each silently changes the value or validity of formulas meval accepts today (evalexpr: integer division and math::-namespaced functions; fasteval: itself unmaintained, missing sqrt/exp/ln/atan2; exmex: -2^2 == +4 and left-associative ^). Instead, src/expression/engine.rs is a self-contained tokenizer + shunting-yard + RPN evaluator that mirrors meval's grammar exactly, verified with 200k differentially-fuzzed expressions against meval (0 mismatches, including accept/reject agreement on malformed input) plus an embedded golden-value corpus. Formulas now compile once and evaluate per record against a value slice (previously a fresh meval Context was rebuilt for every record). On the 223k-record Haltech example log: 15-31x faster for normal formulas, and z-score formulas drop from 14 s to 6.6 ms because channel statistics resolve to constants at compile time. Time-shift syntax (RPM[-1], RPM@-0.1s) and the public expression API are unchanged. Also: MCP/IPC formula evaluation now computes channel statistics when a formula uses _mean_/_stdev_/_min_/_max_/_range_ variables (previously it silently produced all zeros); log2/log10/trunc/fract/pow, tau, and phi now work (their names were already reserved); added the ignored benchmark test tests/expression_bench.rs for before/after comparisons.
There was a problem hiding this comment.
Pull request overview
This PR removes the unmaintained meval dependency and replaces computed-channel formula evaluation with an in-repo expression engine, while updating callers (UI + IPC) to correctly handle statistics-backed formulas and adding supporting tests/benchmarks.
Changes:
- Replaced
mevalwith a built-in tokenizer + shunting-yard + RPN evaluator (src/expression/engine.rs) and refactored the expression module to compile once and evaluate per-record with slot filling. - Introduced
formula_uses_statistics()and updated UI + IPC paths to compute/pass channel statistics only when needed. - Added benchmark + additional unit tests, and updated docs/dependencies to reflect the new module layout and removal of
meval.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/expression_bench.rs | Adds an ignored benchmark to measure computed-formula evaluation performance over a large log. |
| tests/core/computed_channels_tests.rs | Updates scientific-notation test comment to reflect validation behavior. |
| tests/computed_channels_tests.rs | Same comment update in the non-core test suite copy. |
| src/ui/computed_channels_manager.rs | Uses shared formula_uses_statistics() helper instead of ad-hoc substring checks. |
| src/ipc/handler.rs | Computes channel statistics before evaluation when formulas reference statistical variables. |
| src/expression/mod.rs | Refactors validation + evaluation around compiled expressions, adds formula_uses_statistics(), and integrates the new engine. |
| src/expression/engine.rs | New self-contained expression compiler/evaluator implementing the intended operator semantics and function set. |
| CLAUDE.md | Updates module layout docs and records the “no external expression crate” decision. |
| Cargo.toml | Removes meval dependency. |
| Cargo.lock | Removes meval and transitive deps (nom 1.2.4, fnv), updates lock entries accordingly. |
Comments suppressed due to low confidence (2)
src/expression/mod.rs:365
resolve_slotsfalls back to channel index 0 when a binding is missing (unwrap_or(0)), which can silently evaluate the wrong channel instead of surfacing a clear error. Sinceevaluate_all_records_with_statsis a public API, it should error if required bindings are absent.
src/expression/mod.rs:10- The module-level docs (and PR description) claim meval grammar compatibility, but
validate_formulastill rejects scientific-notation literals like1e2becauseextract_channel_referenceswill treat the exponent fragment (e2) as an unquoted channel identifier. Consider documenting this limitation here (or updating the extractor/validation) so users don’t assumee-notation works end-to-end.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+396
to
+415
| let mut instrs = Vec::with_capacity(rpn.len()); | ||
| let mut var_names: Vec<String> = Vec::new(); | ||
|
|
||
| for token in rpn { | ||
| let instr = match token { | ||
| Token::Number(value) => Instr::Const(value), | ||
| Token::Var(name) => { | ||
| if let Some(value) = constant(&name) { | ||
| Instr::Const(value) | ||
| } else { | ||
| let slot = var_names | ||
| .iter() | ||
| .position(|v| *v == name) | ||
| .unwrap_or_else(|| { | ||
| var_names.push(name); | ||
| var_names.len() - 1 | ||
| }); | ||
| Instr::Var(slot) | ||
| } | ||
| } |
- Use a HashMap for variable slot lookup during compilation instead of a linear scan (O(n) instead of O(n^2) in distinct variables) - Error on a missing channel binding in resolve_slots instead of silently falling back to channel index 0 - Recognize scientific-notation exponent fragments in the channel extractor so literals like 1e2 validate and evaluate end-to-end (previously the "e2" was treated as an unknown channel at validation)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
meval has been unmaintained since 2017 and its transitive
nom1.2.4 triggers cargo's "will be rejected by a future version of Rust" warning — a ticking time bomb under the computed channels feature.Alternatives evaluated (and rejected)
Tested evalexpr 13, fasteval 0.2, and exmex 0.21 against meval's actual grammar with a compatibility matrix + benchmarks:
1/2does integer division (silently wrong); math functions aremath::-namespaced sosin(X)fails; nopi/e; slowest optionsqrt,exp,ln,atan2,signum,pi/e-2^2= +4 (meval: −4),2^3^2= 64 (meval: 512, right-assoc); breaks e.g.exp(-((X-a)/b)^2); not fixable via its operator factoryEvery candidate silently mis-evaluates formulas meval accepts today, so the replacement is a self-contained ~550-line tokenizer + shunting-yard + RPN evaluator (
src/expression/engine.rs) mirroring meval's grammar exactly. Zero new dependencies — this class of problem can't recur.Correctness evidence
%, variadicmin/max, constants, number formats)tests/computed_channels_tests.rs+ all suites, 930+ tests)Performance (84 MB Haltech log, 223,559 records, best of 3)
Formulas now compile once to an RPN instruction list; the per-record loop just fills variable slots — no parsing, hashing, or allocation (previously a fresh meval
Contextwas rebuilt per record).RPM * 2 + 1RPM - RPM[-1]("Manifold Pressure" - "Manifold Pressure"@-0.1s) * 10sqrt(abs(RPM)) + sin(RPM / 1000)(RPM - _mean_RPM) / _stdev_RPMThe z-score row: statistics now resolve to constants at compile time instead of being re-injected (114 channels × 5 variables) on every record. Reproduce with
cargo test --release --test expression_bench -- --ignored --nocapture.Notes
expressionunchanged; time-shift regex preprocessing (RPM[-1],RPM@-0.1s) untouched_mean_X-style variables evaluated without statistics now error instead of silently producing zeros; the MCP/IPC handler now computes statistics when needed (newformula_uses_statistics()helper shared with the UI) — z-score formulas over MCP work nowlog2,log10,trunc,fract,pow(a,b),tau,phi(all errored under meval);logintentionally errors with a hint towardln/log10