Independent critique: mixed rename planning

2026-09-24. Read-only source review at ee500908e3d77182ddd54e12524f9b6c6eb43b60. Reviewed RENAME-PLAN-SOURCE-MAP.md, SHA256 ed868523110b6eef996132589450042c6c649984cb870494d1c1e0c05d118c77. No implementation, Cargo, commits or Gitea activity. Paths below begin at crates/oa-core/src/. Parent owns the accepted contract.

Recommendation: adopt the consumed-source/borrowed-target planning design, subject to the precise rules below. The original-expression-site scratch bound is valid for the complete current Project type graph. No hidden historical expression counterexample was found. Output emission and readmission require additional lifecycle seams; a verified plan alone must not be called accepted output.

Whole-target gate and representation

The proposed whole-target exclusions cover the actual Project fields (model.rs:1654–1672), all global alias-bearing records, calculation parameter names, and nested diagram-node aliases (:1518–1530,1582–1591; analysis/types.rs:29–41,78–89). Require exhaustive borrowed struct destructuring without .. at these exclusions. Compare collection lengths before zipped iteration, every ordered map key, each record's own ID, and each parameter's ID at the same Vec index. This rejects records moved between diagrams, substituted map keys with unchanged record IDs, parameter reorder/reidentity, body-variant changes and every non-name payload mutation. Do not identify target records by their proposed spellings.

Opaque payload equality is appropriate after these checks. No generic model diff, normalized clone, serde round trip or temporary target mutation is needed. Large target-only collections should fail lengths/variants before walking; diagnostics should describe the rejected field/category rather than debug-format an arbitrary target tree. Target memory is externally owned and is not silently admitted or counted as a new owned Project.

Consuming AuthoredMixedProject and borrowing immutable &Project avoids a duplicate model and guarantees target stability through safe Rust. A sparse BTreeMap<Target,&str> points only into that external target, never into the plan's own source field; a physical edit map owns only keys and qualified Targets. There is no self-reference. Original source/target references must remain read-only, with no loose-map constructor, legacy adapter, replacement Budget or partial-batch installation path.

Compare final names across all qualified catalog members, including unchanged names. The key is (None,name) for globals and (Some(calculation_id),name) for parameters; this matches existing update.rs:41–74. Equal parameter IDs in different calculations remain distinct. Validate changed byte lengths with checked arithmetic before scanning names, then existing valid_alias. Simultaneous swaps are legal; changed-to-unchanged collisions reject. Check sparse target membership against the authenticated catalog rather than relying on names alone.

Generate edits by streaming both authenticated occurrence families into one sorted physical map. Replacement authority is the whole alias Word only, never the reference's @/trivia extent. Exact-set verification can rescan the unique original families and compare required key/Target pairs plus the total count, without a second seen set. Check spans/source spelling and nonoverlap before export; count equality alone is insufficient.

Expr equality scratch: complete reachability proof

At this source, Project reaches analysis::Expr through exactly four root families:

Root	Definition / production
Calculation body	`analysis/types.rs:85–94`; `lower.rs:512–542` calls `self.binding`.
EngineeringProperty base	`model.rs:667–671`; `lower.rs:568–577`.
AnalysisCase override values	`model.rs:673–677`; `lower.rs:619–662`.
AnalysisCheck predicate	`model.rs:679–682`; `lower.rs:591–614`.

binding (lower.rs:686–699) either returns Unbound or invokes expression lowering. Each Work::Visit emits exactly one ExpressionSite before constructing that node; all Expr variants' children are scheduled (:768–858). Resolver retains the resulting arena verbatim when merging typed definitions/elements (mixed/project.rs:507–561). Let E be this retained arena length, not the number of roots or occurrences.

The historical path is closed: Trace → TraceBaseline → SemanticBaseline → FacetReview (model.rs:1634–1650; semantic/types.rs:84–134,164–174,258–260). Its facet payload catalog contains no analysis Binding or Expr. Historical Binding is LifelineBinding, two IDs; annotation/message expression is String (model.rs:526–536). Defaults/real bounds/operation/contextual payloads likewise contain no Expr. Ordinary constraint/event/state expressions are strings. No arbitrary serde_json::Value or boxed Project payload introduces another analysis-expression root.

Expr::eq (analysis/expression_lifecycle.rs:170–270) starts with one pair and iteratively replaces a popped pair by corresponding child pairs. Pending pairs form a frontier of distinct nodes in the original tree. List lengths and call lengths/keys are checked before paired child insertion; fixed-arity variants push only their matching children; variant mismatch stops. It never recursively invokes Expr equality while that Vec is live. Sequential parent/container equality calls therefore cannot overlap these work vectors. Each work-vector cardinality is at most its original tree's node count, hence ≤E, even for a huge/deep mismatching target.

Reserve E logical scratch entries before any such comparison and release after all comparisons have returned and their vectors dropped. With E=0, no original Expression variant exists: compare parent variants first and reject target-only Expression without entering Expr equality. Exact equality is bounded normalized structural equality, not arithmetic (analysis_numeric/exact.rs:24–36); no numeric meter charge is needed for it. The E proof concerns logical live pairs, not Vec capacity, heap bytes or RSS. Future additions to the payload type graph require renewing the reachability proof.

Controls should cover a wide matching list/call frontier, deep equal/mismatching trees, target-only deep expression against Unbound, E=0 historical traces and late refusal/drop. Conservative E reservation may reject an otherwise unchanged target when inherited auxiliary headroom is insufficient. Do not promise unconditional no-op success.

Scalar equality decision

The source map's geometry example was incomplete: signed zero also occurs in DataValue::Real and DataTypeDefinition real bounds (model.rs:310–344), ordinary defaults (:615–627), and historical Data/Field reviews (semantic/types.rs:94–100). A geometry-only bitwise matcher would not enforce a universal bitwise rule.

Parent selected existing whole-model PartialEq semantics everywhere, including +0/-0 equivalence, while preserving every original non-name source byte. This is coherent and avoids a bespoke matcher. A target differing only in zero sign can therefore be semantically equal and produce no scalar edit; do not claim bitwise target representation identity. NaN targets do not compare equal to admitted finite originals. Include signed-zero controls in both geometry and an ordinary/historical real carrier so the policy remains explicit.

Simpler candidates and accounting decision

Candidate	Judgment
Borrow target; sparse names + physical map	Preferred: exact retained increment R+M entries, no name copies or typed-path clones; explicit lifetime.
Own target and copy changed names	Sound if independent ownership becomes necessary, but copies name bytes and owns a second full model. No current need justifies it.
Resolve every target name directly per edit	Saves R slots but repeated parameter Vec searches can become quadratic. Not the simpler bounded choice.
Pairwise final-name collision scans	Saves Q scratch slots at quadratic work cost. Prefer one borrowed qualified-name set.
Reuse both legacy producer batches or normalize/clone whole Project	Adds containers, provenance and accounting, or obscures unsupported mutations. Reject for this bounded task.
New metered Expr equality helper	Could reduce conservative scratch refusals, but duplicates/refactors proven matching logic. Defer unless E overreservation demonstrably blocks useful cases.

For separated phases, the proposed incremental peak max(E,R+Q,R+M) is sound: equality reservation dies before changed-name collection; namespace set dies before edit collection. A combined implementation must account actual overlap. No-op planning remains subject to its actual phase reservations. Planning must preserve parse Usage, numeric units and frame metric; local storage checks do not recharge those counters (budget.rs:139–165).

Concrete obligations before emission/readmission

These are contract requirements for the subsequent seam, not a proposed implementation now:

Exact policy: preserve all inherited Limits and ProjectLimits. mixed.rs:223–268 currently reconstructs defaults or creates a new Budget; plain parse() would lose tightened policy. Deliberate output parsing should use the exact inherited per-snapshot profile, with fresh output usage explicitly reported separately. Never reset the original live witness or describe post-reparse numeric/parse counters as unchanged. This is not a cumulative old+new parsing budget unless separately specified.
Complete preflight: checked final file lengths and total include unchanged files. Compute per-file removed/replacement totals before final addition so transient growth does not defeat a valid combined shrink/grow result. Keep all edits in original coordinates and emit each original file once. Reserve F output-map entries, source bytes, copied canonical path keys and entry path before their allocations; optional length maps need additional slots. Output-size validation alone is not a simultaneous live-storage bound.
Documented consumed transition: decide exactly when old index/syntax/maps are dropped. Output buffers coexist with old B+R+M during emission and remain owned through verification. Dropping old auxiliary state can reduce the verification baseline, but does not make retained output strings/maps free. A carried ledger/baseline must include every surviving auxiliary allocation before parsing and resolving output. Current resolve starts Ledger::default() (mixed/project.rs:438–442), and parser metadata checks are separate (mixed.rs:278–335); merely wrapping those current entry points in two independently passing limits does not establish aggregate live storage.
Actual verification: parse/resolve/admit emitted bytes and compare the whole resulting Project with the immutable intended target. Retain E as a scalar if the old witness has been dropped and reserve equality scratch against the verification baseline too; the validated target's unchanged Expr shapes supply the same bound. A structurally invalid output and a valid-but-wrong output must reach their respective distinct refusals, with no partial usable result.
Return lifetime: a ResolvedMixedProject borrows its bundle; ordinary owning fields cannot safely return both bundle and borrowing witness. A later narrow consumed-parts result may own output/model/warnings after temporary syntax drops, or a caller-owned output can support a borrowing witness. Specify which guarantee is returned; do not imply a reusable index survives when it was discarded. No-op execution may return an explicit unchanged owner rather than pretend owned copies are free.
Staleness and installation: if execution accepts a current SourceBundle, compare entry, entire canonical file set, paths and exact bytes with the retained snapshot, including untouched files. This detects stale supplied snapshots; atomic disk/history/GUI adoption is separate and is not granted by a pure plan.

Proceed with a clearly named planning dependency once these planning rules are incorporated. Keep emission/readmission unimplemented until its policy, aggregate baseline and returned ownership are specified. This preserves the required whole-output gate without inventing a new framework or weakening full-edit parity requirements.

12 KiB Raw Blame History Unescape Escape