Parameters: unit-aware parameters (an inch parameter reads bare numbers as inches), atan2/mod/trunc and clearer unit errors, cycle paths (a → b → a), rename with every reference following (expressions, feature inputs, dimensions, favourites, comments), deletes refused with the list of users, parameter errors shown on the features using them, parameters.add/rename/delete/comment/users/graph/complete and expr.evaluate; ChangeParameterCommand renames and favourites; hostile test no longer writes stray files

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AS5Tn3xieewUSg3uxrMVdL

crates/doc/src/document.rs

@@ -866,31 +866,54 @@ impl Document {
         self.params.iter().find(|p| p.name == name)
     }
 
-    fn valid_name(name: &str) -> bool {
+    /// Why a name cannot be a parameter name (None = fine).
+    pub fn param_name_problem(name: &str) -> Option<String> {
         let mut cs = name.chars();
-        matches!(cs.next(), Some(c) if c.is_alphabetic() || c == '_')
-            && cs.all(|c| c.is_alphanumeric() || c == '_')
-            && !matches!(name, "mm" | "cm" | "m" | "um" | "in" | "inch" | "mil" | "yd" | "km" | "nm" | "ft" | "deg" | "rad" | "pi" | "PI")
-            && name.len() <= 64
+        if !matches!(cs.next(), Some(c) if c.is_alphabetic() || c == '_') || !cs.all(|c| c.is_alphanumeric() || c == '_') {
+            return Some(format!("`{name}` is not a valid parameter name (letters, digits and _, starting with a letter)"));
+        }
+        if name.len() > 64 {
+            return Some("parameter names are at most 64 characters".into());
+        }
+        if expr::is_reserved(name) {
+            return Some(format!("`{name}` is a unit, constant or function name"));
+        }
+        None
     }
 
-    /// Add or change a parameter. Fails if the result would not evaluate.
+    /// Add or change a parameter. Fails if the result would not evaluate. A new parameter
+    /// without a unit takes the unit of its value (mm, deg or none).
     pub fn set_param(&mut self, name: &str, expr_s: &str, unit: Option<&str>, comment: Option<&str>) -> Result<()> {
-        if !Self::valid_name(name) {
-            return Err(DocError::Invalid(format!("`{name}` is not a valid parameter name")));
+        if let Some(p) = Self::param_name_problem(name) {
+            return Err(DocError::Invalid(p));
         }
         if expr_s.len() > 4096 {
             return Err(DocError::Expr("expression too long".into()));
         }
+        if expr_s.trim().is_empty() {
+            return Err(DocError::Expr("empty expression".into()));
+        }
+        if let Some(u) = unit
+            && expr::unit_info(u).is_none()
+        {
+            return Err(DocError::Expr(format!("unknown unit `{u}` (mm, cm, m, in, ft, deg, rad or none)")));
+        }
+        if comment.is_some_and(|c| c.len() > 4096) {
+            return Err(DocError::Invalid("comment too long".into()));
+        }
         if self.features.iter().any(|f| f.param_names.iter().any(|n| n == name)) {
             return Err(DocError::Invalid(format!("`{name}` is a feature's parameter")));
         }
         let mut next = self.clone();
+        let inferred = match unit {
+            Some(u) => u.trim().to_string(),
+            None => next.infer_unit(expr_s),
+        };
         match next.params.iter_mut().find(|p| p.name == name) {
             Some(p) => {
-                p.expr = expr_s.to_string();
+                p.expr = expr_s.trim().to_string();
                 if let Some(u) = unit {
-                    p.unit = u.to_string();
+                    p.unit = u.trim().to_string();
                 }
                 if let Some(c) = comment {
                     p.comment = c.to_string();
@@ -902,8 +925,8 @@ impl Document {
                 }
                 next.params.push(Param {
                     name: name.into(),
-                    expr: expr_s.into(),
-                    unit: unit.unwrap_or("mm").into(),
+                    expr: expr_s.trim().into(),
+                    unit: inferred,
                     comment: comment.unwrap_or_default().into(),
                     model: false,
                 })
@@ -924,25 +947,6 @@ impl Document {
         name
     }
 
-    /// Remove a user parameter (fails while something references it).
-    pub fn remove_param(&mut self, name: &str) -> Result<()> {
-        if self.param(name).is_none() {
-            return Err(DocError::Unknown(format!("parameter `{name}`")));
-        }
-        let mut probe = self.clone();
-        probe.params.retain(|p| p.name != name);
-        if probe.referenced_names().iter().any(|n| n == name) {
-            return Err(DocError::Invalid(format!("`{name}` is still used")));
-        }
-        *self = probe;
-        Ok(())
-    }
-
-    /// Evaluate all parameters: values (mm / rad / unit-less) and errors by name.
-    pub fn param_values(&self) -> (std::collections::BTreeMap<String, expr::Value>, std::collections::BTreeMap<String, String>) {
-        expr::eval_params(&self.all_param_exprs())
-    }
-
     /// Evaluate an expression against the document's parameters.
     pub fn eval(&self, e: &str, kind: Kind) -> Result<f64> {
         let (vals, _) = self.param_values();

crates/doc/src/eval.rs

@@ -221,18 +221,23 @@ pub struct Model {
     pub last_recomputed: usize,
 }
 
-fn fingerprint(prev: u64, f: &Feature, vals: &BTreeMap<String, Value>, rolled_back: bool) -> u64 {
-    let mut h = std::collections::hash_map::DefaultHasher::new();
-    prev.hash(&mut h);
-    rolled_back.hash(&mut h);
-    serde_json::to_string(f).unwrap_or_default().hash(&mut h);
+/// Parameters a feature uses directly (inputs and sketch dimensions), sorted.
+fn param_names(f: &Feature) -> Vec<String> {
     let mut names: Vec<String> = f.kind.expressions().iter().flat_map(|e| expr::references(e)).collect();
     if let FeatureKind::Sketch { sketch, .. } = &f.kind {
         names.extend(sketch.constraints.iter().filter_map(|c| c.param.clone()));
     }
     names.sort();
     names.dedup();
-    for n in names {
+    names
+}
+
+fn fingerprint(prev: u64, f: &Feature, vals: &BTreeMap<String, Value>, rolled_back: bool) -> u64 {
+    let mut h = std::collections::hash_map::DefaultHasher::new();
+    prev.hash(&mut h);
+    rolled_back.hash(&mut h);
+    serde_json::to_string(f).unwrap_or_default().hash(&mut h);
+    for n in param_names(f) {
         n.hash(&mut h);
         match vals.get(&n) {
             Some(v) => {
@@ -313,9 +318,14 @@ impl Model {
             let t0 = now();
             let mut next = (*state).clone();
             let mut warning = None;
-            let r = kernel::guard(&f.name, || {
-                eval_feature(doc, &vals, f, &mut next, &mut warning).map_err(|e| kernel::KernelError::Failed(e.to_string()))
-            });
+            // A parameter with an error fails the features using it, with that error.
+            let bad_param = param_names(f).into_iter().find_map(|n| self.param_errors.get(&n).map(|e| format!("parameter `{n}`: {e}")));
+            let r = match bad_param {
+                Some(e) => Err(kernel::KernelError::Failed(e)),
+                None => kernel::guard(&f.name, || {
+                    eval_feature(doc, &vals, f, &mut next, &mut warning).map_err(|e| kernel::KernelError::Failed(e.to_string()))
+                }),
+            };
             let ms = t0.elapsed().as_secs_f64() * 1000.0;
             let error = match r {
                 Ok(()) => {

crates/doc/src/lib.rs

@@ -16,7 +16,7 @@ mod project3d;
 pub use assembly::{IDENTITY, Mat, Occurrence, apply_point, apply_vector, is_identity, mat_inverse, mat_mul, rigid};
 pub use document::*;
 pub use eval::{FeatureResult, Model, ModelBody, ModelState, SolvedSketch, ThreadInfo, parse_metric_thread, world_state};
-pub use params::{ParamRow, cycles};
+pub use params::{ParamRow, ParamUser, cycles};
 pub use solvecraft_kernel as kernel;
 pub use solvecraft_sketch as sketch;
 

crates/doc/src/params.rs

@@ -6,10 +6,10 @@
 
 use std::collections::{BTreeMap, BTreeSet};
 
-use serde::Serialize;
+use serde::{Deserialize, Serialize};
 
-use crate::expr::{self, Kind};
-use crate::{DocError, Document, FeatureKind, HoleKind, PatternKind, PlaneRef, Result};
+use crate::expr::{self, Kind, ParamDef, Value};
+use crate::{DocError, Document, FeatureKind, HoleKind, MAX_PARAMS, PatternKind, PlaneRef, Result};
 
 fn plane_inputs_mut<'a>(p: &'a mut PlaneRef, v: &mut Vec<(&'static str, &'a mut String, Kind)>) {
     match p {
@@ -144,6 +144,10 @@ impl FeatureKind {
 
     /// Read-only view of [`FeatureKind::inputs_mut`]: (label, expression, kind).
     pub fn inputs(&self) -> Vec<(&'static str, String, Kind)> {
+        // Imports have no inputs (and their STEP text is large: don't clone it).
+        if matches!(self, FeatureKind::Import { .. } | FeatureKind::MeshImport { .. }) {
+            return Vec::new();
+        }
         let mut c = self.clone();
         c.inputs_mut().into_iter().map(|(l, e, k)| (l, e.clone(), k)).collect()
     }
@@ -271,7 +275,7 @@ impl Document {
                 name: p.name.clone(),
                 expression: p.expr.clone(),
                 unit: p.unit.clone(),
-                value: value(&p.name, p.kind()),
+                value: self.param_display_value(&vals, &p.name),
                 error: errs.get(&p.name).cloned(),
                 comment: p.comment.clone(),
                 source: if p.model { "sketch" } else { "user" },
@@ -385,6 +389,261 @@ impl Document {
     }
 }
 
+/// Something that uses a parameter.
+#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
+#[serde(tag = "type", rename_all = "snake_case")]
+pub enum ParamUser {
+    /// Another parameter's expression (a stored one or a feature input).
+    Parameter { name: String },
+    /// A feature input (`input` is its label, e.g. `Distance`).
+    Feature { feature: u64, name: String, input: String },
+    /// A sketch dimension (`constraint` is its id).
+    Dimension { feature: u64, name: String, constraint: String },
+}
+
+impl ParamUser {
+    pub fn describe(&self) -> String {
+        match self {
+            ParamUser::Parameter { name } => format!("parameter {name}"),
+            ParamUser::Feature { name, input, .. } => format!("{name} ({input})"),
+            ParamUser::Dimension { name, constraint, .. } => format!("{name} dimension {constraint}"),
+        }
+    }
+}
+
+/// Length unit of the document in mm (bare numbers of unit-less parameters next to lengths).
+fn doc_len(units: &str) -> f64 {
+    match expr::unit_info(units) {
+        Some((Kind::Length, s)) => s,
+        _ => 1.0,
+    }
+}
+
+impl Document {
+    /// Every parameter with its unit, for evaluation: stored parameters and named feature
+    /// inputs (mm / deg / none by their kind).
+    pub fn param_defs(&self) -> Vec<ParamDef> {
+        let mut v: Vec<ParamDef> =
+            self.params.iter().map(|p| ParamDef { name: p.name.clone(), expr: p.expr.clone(), unit: p.unit.clone() }).collect();
+        for f in &self.features {
+            for (name, (_, e, k)) in f.param_names.iter().zip(f.kind.inputs()) {
+                if !name.is_empty() {
+                    v.push(ParamDef { name: name.clone(), expr: e, unit: unit_of(k).into() });
+                }
+            }
+        }
+        v
+    }
+
+    /// Evaluate all parameters: values (mm / rad / unit-less) and errors by name. Errors name
+    /// the problem (`circular reference: a → b → a`, `cannot add a length and an angle`).
+    pub fn param_values(&self) -> (BTreeMap<String, Value>, BTreeMap<String, String>) {
+        expr::eval_params(&self.param_defs(), doc_len(&self.units))
+    }
+
+    /// A parameter's value in its own unit (e.g. 2 for a 50.8 mm parameter in inches).
+    pub fn param_display_value(&self, vals: &BTreeMap<String, Value>, name: &str) -> Option<f64> {
+        let v = vals.get(name)?;
+        let unit = match self.param(name) {
+            Some(p) => p.unit.clone(),
+            None => v.kind().map(|k| unit_of(k).to_string()).unwrap_or_default(),
+        };
+        Some(expr::value_in_unit(*v, &unit))
+    }
+
+    /// The unit for a new parameter with this expression: that of its value; a bare number is
+    /// a length (mm).
+    pub(crate) fn infer_unit(&self, e: &str) -> String {
+        let (vals, _) = self.param_values();
+        let v = expr::eval_with(e, &|n| vals.get(n).copied().ok_or_else(|| DocError::Expr(format!("unknown parameter `{n}`"))));
+        match v.ok().and_then(|v| v.kind()) {
+            Some(Kind::Unitless) if !expr::references(e).is_empty() => String::new(),
+            Some(Kind::Angle) => "deg".into(),
+            _ => "mm".into(),
+        }
+    }
+
+    /// Everything that uses a parameter: other parameters (and feature inputs) whose
+    /// expressions refer to it, and the sketch dimensions it drives.
+    pub fn param_users(&self, name: &str) -> Vec<ParamUser> {
+        let mut out = Vec::new();
+        for p in &self.params {
+            if p.name != name && expr::references(&p.expr).iter().any(|r| r == name) {
+                out.push(ParamUser::Parameter { name: p.name.clone() });
+            }
+        }
+        for f in &self.features {
+            for (k, (label, e, _)) in f.kind.inputs().into_iter().enumerate() {
+                if expr::references(&e).iter().any(|r| r == name) {
+                    match f.param_names.get(k).filter(|n| !n.is_empty()) {
+                        Some(n) => out.push(ParamUser::Parameter { name: n.clone() }),
+                        None => out.push(ParamUser::Feature { feature: f.id, name: f.name.clone(), input: label.to_string() }),
+                    }
+                }
+            }
+            if let FeatureKind::Sketch { sketch, .. } = &f.kind {
+                for c in &sketch.constraints {
+                    if c.param.as_deref() == Some(name) {
+                        out.push(ParamUser::Dimension { feature: f.id, name: f.name.clone(), constraint: c.id.clone() });
+                    }
+                }
+            }
+        }
+        out
+    }
+
+    /// Remove a stored parameter; refused while something uses it (the error lists the users).
+    pub fn remove_param(&mut self, name: &str) -> Result<()> {
+        if self.param(name).is_none() {
+            if self.features.iter().any(|f| f.param_names.iter().any(|n| n == name)) {
+                return Err(DocError::Invalid(format!("`{name}` is a feature's input: delete or edit the feature instead")));
+            }
+            return Err(DocError::Unknown(format!("parameter `{name}`")));
+        }
+        let users = self.param_users(name);
+        if !users.is_empty() {
+            let list: Vec<String> = users.iter().map(ParamUser::describe).collect();
+            return Err(DocError::Invalid(format!("`{name}` is used by {}", list.join(", "))));
+        }
+        self.params.retain(|p| p.name != name);
+        self.favorites.remove(name);
+        Ok(())
+    }
+
+    /// Rename a parameter (stored or a feature input); every expression, feature input,
+    /// dimension, favourite and comment that refers to it follows. Returns how many
+    /// references were updated.
+    pub fn rename_param(&mut self, old: &str, new: &str) -> Result<usize> {
+        let new = new.trim();
+        if !self.name_taken(old) {
+            return Err(DocError::Unknown(format!("parameter `{old}`")));
+        }
+        if old == new {
+            return Ok(0);
+        }
+        if let Some(p) = Self::param_name_problem(new) {
+            return Err(DocError::Invalid(p));
+        }
+        if self.name_taken(new) {
+            return Err(DocError::Invalid(format!("there is already a parameter `{new}`")));
+        }
+        let mut n = 0;
+        for p in &mut self.params {
+            if p.name == old {
+                p.name = new.to_string();
+            }
+            let r = expr::rename_reference(&p.expr, old, new);
+            if r != p.expr {
+                p.expr = r;
+                n += 1;
+            }
+        }
+        for f in &mut self.features {
+            for x in &mut f.param_names {
+                if x == old {
+                    *x = new.to_string();
+                }
+            }
+            for (_, e, _) in f.kind.inputs_mut() {
+                let r = expr::rename_reference(e, old, new);
+                if r != *e {
+                    *e = r;
+                    n += 1;
+                }
+            }
+            if let FeatureKind::Sketch { sketch, .. } = &mut f.kind {
+                for c in &mut sketch.constraints {
+                    if c.param.as_deref() == Some(old) {
+                        c.param = Some(new.to_string());
+                        n += 1;
+                    }
+                }
+            }
+        }
+        if self.favorites.remove(old) {
+            self.favorites.insert(new.to_string());
+        }
+        if let Some(c) = self.param_comments.remove(old) {
+            self.param_comments.insert(new.to_string(), c);
+        }
+        Ok(n)
+    }
+
+    /// Set a parameter's comment (stored parameters keep it, feature inputs in `param_comments`).
+    pub fn set_param_comment(&mut self, name: &str, comment: &str) -> Result<()> {
+        if comment.len() > 4096 {
+            return Err(DocError::Invalid("comment too long".into()));
+        }
+        if let Some(p) = self.params.iter_mut().find(|p| p.name == name) {
+            p.comment = comment.to_string();
+            return Ok(());
+        }
+        if !self.name_taken(name) {
+            return Err(DocError::Unknown(format!("parameter `{name}`")));
+        }
+        if comment.is_empty() {
+            self.param_comments.remove(name);
+        } else {
+            self.param_comments.insert(name.to_string(), comment.to_string());
+        }
+        Ok(())
+    }
+
+    /// Dependency edges: (user, parameter used). Users are parameter names, or
+    /// `Feature:<name>` for a feature whose inputs or dimensions use the parameter.
+    pub fn param_graph(&self) -> Vec<(String, String)> {
+        let mut out = expr::param_edges(&self.param_defs());
+        for f in &self.features {
+            let mut names: BTreeSet<String> = f.param_names.iter().filter(|n| !n.is_empty()).cloned().collect();
+            if let FeatureKind::Sketch { sketch, .. } = &f.kind {
+                names.extend(sketch.constraints.iter().filter_map(|c| c.param.clone()));
+            }
+            out.extend(names.into_iter().map(|n| (format!("Feature:{}", f.name), n)));
+        }
+        out
+    }
+
+    /// Features whose result depends on a parameter (directly or through other parameters).
+    pub fn features_using_param(&self, name: &str) -> Vec<u64> {
+        let defs = self.param_defs();
+        let mut affected: BTreeSet<String> = BTreeSet::from([name.to_string()]);
+        for _ in 0..defs.len().min(MAX_PARAMS * 2) {
+            let before = affected.len();
+            for d in &defs {
+                if expr::references(&d.expr).iter().any(|r| affected.contains(r)) {
+                    affected.insert(d.name.clone());
+                }
+            }
+            if affected.len() == before {
+                break;
+            }
+        }
+        let mut out = Vec::new();
+        for f in &self.features {
+            let mut hit = f.param_names.iter().any(|n| affected.contains(n));
+            hit |= f.kind.expressions().iter().any(|e| expr::references(e).iter().any(|r| affected.contains(r)));
+            if let FeatureKind::Sketch { sketch, .. } = &f.kind {
+                hit |= sketch.constraints.iter().any(|c| c.param.as_ref().is_some_and(|p| affected.contains(p)));
+            }
+            if hit {
+                out.push(f.id);
+            }
+        }
+        out
+    }
+
+    /// Add a user parameter (refused when the name exists).
+    pub fn add_user_param(&mut self, name: &str, expr_s: &str, unit: Option<&str>, comment: Option<&str>) -> Result<()> {
+        if self.name_taken(name.trim()) {
+            return Err(DocError::Invalid(format!("there is already a parameter `{}`", name.trim())));
+        }
+        if self.params.len() >= MAX_PARAMS {
+            return Err(DocError::Invalid("too many parameters".into()));
+        }
+        self.set_param(name.trim(), expr_s, unit, comment)
+    }
+}
+
 /// One CSV line with quoted fields.
 fn parse_csv_line(l: &str) -> Vec<String> {
     let mut out = Vec::new();

crates/doc/src/tests.rs

@@ -258,6 +258,9 @@ fn parameter_cycles_are_found() {
     ];
     let c = crate::cycles(&list);
     assert_eq!(c.into_iter().collect::<Vec<_>>(), vec!["a", "b", "c", "e"]);
-    let (vals, errs) = crate::expr::eval_params(&list);
+    let defs: Vec<crate::expr::ParamDef> =
+        list.iter().map(|(n, e, _)| crate::expr::ParamDef { name: n.clone(), expr: e.clone(), unit: "mm".into() }).collect();
+    let (vals, errs) = crate::expr::eval_params(&defs, 1.0);
+    assert!(errs["a"].contains("circular reference: a → b → c → a"), "{}", errs["a"]);
     assert!(vals.contains_key("d") && errs.contains_key("a") && errs.contains_key("e"));
 }