fix(fit): the fit's own name must not already mean something
Copilot's fifth pass, one finding and it is right. `into` was checked against `measure_item_names()`, and `Expr::Ref` does not ask that question: `Model::find_item_category` finds a **regular category's** item before it falls back into `_Measure` (gotcha 7). So naming the fit after an item of some category installed cleanly, and every later formula referring to the fitted measure then read the category coordinate instead — silently, because both resolve. The check now asks the document the same question `Expr::Ref` will ask, which is both stronger than the measure-only form and simpler than it. Two regressions: one that a category item's name is refused, and one that earns the refusal by asserting the property it protects — a second formula `Doubled = |Score (fit)| * 2` reads twice the fit, per record, against a real document. 1232 passing (+2); clippy, fmt and the wasm gate clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -379,10 +379,17 @@ pub fn fit_measure(
|
||||
"a fit of {target:?} cannot be stored as {target:?} — the formula would shadow the \
|
||||
cells it was fitted to. Name a different measure"
|
||||
);
|
||||
// **Any** existing item, not only an existing measure. `Expr::Ref` resolves
|
||||
// through `Model::find_item_category`, which finds a *regular* category's
|
||||
// item before it falls back into `_Measure` (gotcha 7) — so naming the fit
|
||||
// after an item of some category installs cleanly and then makes every
|
||||
// later formula that references it read the category coordinate instead
|
||||
// (Copilot on PR #25). Asking the document the same question `Expr::Ref`
|
||||
// will ask is both stronger than the measure-only check and simpler.
|
||||
anyhow::ensure!(
|
||||
!model.measure_item_names().iter().any(|m| m == into),
|
||||
"{into:?} is already a measure; a formula on it would replace what is there. \
|
||||
Name a measure that does not exist yet"
|
||||
model.find_item_category(into).is_none(),
|
||||
"{into:?} already names an item of this document, so a formula referring to the fit \
|
||||
would resolve to that instead. Name something that does not exist yet"
|
||||
);
|
||||
let table = feature_table(model, view, target)?;
|
||||
// The width guard runs on the RAW table, before `preprocess` materialises
|
||||
@@ -1249,7 +1256,52 @@ mod tests {
|
||||
let (model, view) = document();
|
||||
let err =
|
||||
fit_measure(&model, &view, "Score", "Cost", &request()).expect_err("Cost holds data");
|
||||
assert!(err.to_string().contains("already a measure"), "{err}");
|
||||
assert!(err.to_string().contains("already names an item"), "{err}");
|
||||
}
|
||||
|
||||
/// Copilot on PR #25. The check used to ask only about *measures*, and
|
||||
/// `Expr::Ref` does not: `Model::find_item_category` finds a regular
|
||||
/// category's item **before** falling back into `_Measure` (gotcha 7). So
|
||||
/// naming the fit after some category's item installed cleanly and then
|
||||
/// made every later formula referring to it read the category coordinate.
|
||||
#[test]
|
||||
fn a_fit_named_after_a_category_item_is_refused() {
|
||||
let (mut model, view) = document();
|
||||
model.category_mut(REGION).expect("Region exists").kind = CategoryKind::Regular;
|
||||
assert!(
|
||||
model.find_item_category("North").is_some(),
|
||||
"the fixture must really have an item named North"
|
||||
);
|
||||
let err = fit_measure(&model, &view, "Score", "North", &request())
|
||||
.expect_err("North is an item of Region");
|
||||
assert!(err.to_string().contains("already names an item"), "{err}");
|
||||
}
|
||||
|
||||
/// And the property that refusal protects, asserted against a real
|
||||
/// document: a second formula referring to the fitted measure reads the
|
||||
/// **fit**, not something else that happens to share its name.
|
||||
#[test]
|
||||
fn a_later_formula_referring_to_the_fit_reads_the_fit() {
|
||||
let (mut model, view) = document();
|
||||
let outcome =
|
||||
fit_measure(&model, &view, "Score", FIT_INTO, &request()).expect("the fit lands");
|
||||
model.add_formula(outcome.formula.clone());
|
||||
model.add_formula(
|
||||
improvise_formula::parse_formula(
|
||||
&format!("Doubled = {} * 2", improvise_formula::quote_name(FIT_INTO)),
|
||||
MEASURE,
|
||||
)
|
||||
.expect("parses"),
|
||||
);
|
||||
model.recompute_formulas(&[]);
|
||||
for i in 0..12u32 {
|
||||
let fitted = model.evaluate(&cell(&format!("r{i}"), FIT_INTO));
|
||||
let doubled = model.evaluate(&cell(&format!("r{i}"), "Doubled"));
|
||||
let (Some(CellValue::Number(a)), Some(CellValue::Number(b))) = (fitted, doubled) else {
|
||||
panic!("record r{i} did not evaluate both measures");
|
||||
};
|
||||
assert!((b - a * 2.0).abs() < 1e-6, "r{i}: {b} is not twice {a}");
|
||||
}
|
||||
}
|
||||
|
||||
/// improvise-8xhy reaching a real user, and the reason improvise-4zl7
|
||||
|
||||
Reference in New Issue
Block a user