You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Unit inference is all-or-nothing per model: a single dimensional conflict anywhere in a model discards every inferred unit for that whole model, blinding subsequent checks to the rest of the variables.
units_infer::infer (src/simlin-engine/src/units_infer.rs:1147) returns a single UnitResult<HashMap<Ident<Canonical>, UnitMap>>, so it aborts and returns Err on the first conflict it finds -- discarding all the metavars it had already resolved up to that point.
db.rs::check_model_units (src/simlin-engine/src/db.rs:1484-1485) then does:
let inferred_units = crate::units_infer::infer(&models_s1, units_ctx, target_model).unwrap_or_else(|err| {// emit one diagnostic, then ...Default::default()// empty inferred-units map for the ENTIRE model});
So when any one equation conflicts, inferred_units collapses to an empty map and every downstream per-variable unit check in that model runs with no inferred units. One bad equation degrades unit checking for the whole model.
This directly parallels the pre-existing TODO at src/simlin-engine/src/units.rs:263:
// TODO: we shouldn't discard the whole context if there are errorsif unit_errors.is_empty(){Ok(ctx)}else{Err(unit_errors)}
-- the same all-or-nothing failure shape, at the unit-context construction layer rather than the inference layer.
Why it matters
Correctness of diagnostics: a single dimensional conflict suppresses unit checking for every other variable in the model, so genuine errors elsewhere go unreported until the first one is fixed -- an iterative, one-at-a-time debugging experience instead of a complete report.
Cascading masking: this is one mechanism behind the C-LEARN unit-error behavior (a model-level conflict makes the per-variable picture incomplete), and it makes residual-diagnostic triage harder because the set surfaced depends on which conflict aborts first (compounded by the nondeterministic ordering tracked in engine: wrld3_resource_unit_alias_should_not_conflict is order-dependent / flaky #474).
Maintainability: the unwrap_or_else(|_| Default::default()) shape is a code smell that hides the partial-result information the inferencer actually computed.
Components affected
src/simlin-engine/src/units_infer.rs (infer -- the single-Result return type)
src/simlin-engine/src/db.rs (check_model_units -- the unwrap_or_else(|_| Default::default()) discard)
src/simlin-engine/src/units.rs:263 (the parallel TODO at the context-construction layer)
Possible approaches
Have infer return partial results alongside conflicts -- e.g. (HashMap<Ident, UnitMap>, Vec<UnitError>) or a struct -- so the metavars it did resolve survive even when some equation conflicts. check_model_units then keeps the resolved units and reports the conflicts, rather than throwing both away.
Apply the same partial-result shape to Context construction (the units.rs:263 TODO) so a duplicate/conflicting unit declaration does not discard the whole context either.
Continue inference past the first conflict (collect all conflicts rather than short-circuiting), which also yields a more complete diagnostic set in one pass.
Context
Identified while fixing the C-LEARN unit-error flood on branch fix-clearn-unit-errors. The four fixes there addressed specific over-flagging root causes; this architectural all-or-nothing discard is a separate, more general robustness issue that is out of scope for that branch. Related: #474 (order-dependence of which conflict surfaces first), #35 (general unit-checking tracker), tech-debt item 2 (unwrap_or_default() usage in simlin-engine).
Description
Unit inference is all-or-nothing per model: a single dimensional conflict anywhere in a model discards every inferred unit for that whole model, blinding subsequent checks to the rest of the variables.
units_infer::infer(src/simlin-engine/src/units_infer.rs:1147) returns a singleUnitResult<HashMap<Ident<Canonical>, UnitMap>>, so it aborts and returnsErron the first conflict it finds -- discarding all the metavars it had already resolved up to that point.db.rs::check_model_units(src/simlin-engine/src/db.rs:1484-1485) then does:So when any one equation conflicts,
inferred_unitscollapses to an empty map and every downstream per-variable unit check in that model runs with no inferred units. One bad equation degrades unit checking for the whole model.This directly parallels the pre-existing TODO at
src/simlin-engine/src/units.rs:263:-- the same all-or-nothing failure shape, at the unit-context construction layer rather than the inference layer.
Why it matters
unwrap_or_else(|_| Default::default())shape is a code smell that hides the partial-result information the inferencer actually computed.Components affected
src/simlin-engine/src/units_infer.rs(infer-- the single-Resultreturn type)src/simlin-engine/src/db.rs(check_model_units-- theunwrap_or_else(|_| Default::default())discard)src/simlin-engine/src/units.rs:263(the parallel TODO at the context-construction layer)Possible approaches
inferreturn partial results alongside conflicts -- e.g.(HashMap<Ident, UnitMap>, Vec<UnitError>)or a struct -- so the metavars it did resolve survive even when some equation conflicts.check_model_unitsthen keeps the resolved units and reports the conflicts, rather than throwing both away.Contextconstruction (theunits.rs:263TODO) so a duplicate/conflicting unit declaration does not discard the whole context either.Context
Identified while fixing the C-LEARN unit-error flood on branch
fix-clearn-unit-errors. The four fixes there addressed specific over-flagging root causes; this architectural all-or-nothing discard is a separate, more general robustness issue that is out of scope for that branch. Related: #474 (order-dependence of which conflict surfaces first), #35 (general unit-checking tracker), tech-debt item 2 (unwrap_or_default()usage in simlin-engine).