From 622917a026064b967418c86dbcdf14cab332c236 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 19 Aug 2026 03:04:10 -0400 Subject: [PATCH 01/11] docs: add bug report for affix-process clone corruption after MoveSenseToCopy Analysis of MoAffixProcess.PostClone's three known defects (A: return instead of continue, B: repeated RemoveAt(0) across allomorphs, C: no scoping to the object's own clone), with a proposed fix and test plan. --- .../affix-process-split-sense-stale-clone.md | 118 ++++++++++++++++++ 1 file changed, 118 insertions(+) create mode 100644 doc/bugs/affix-process-split-sense-stale-clone.md diff --git a/doc/bugs/affix-process-split-sense-stale-clone.md b/doc/bugs/affix-process-split-sense-stale-clone.md new file mode 100644 index 00000000..6eb77aa4 --- /dev/null +++ b/doc/bugs/affix-process-split-sense-stale-clone.md @@ -0,0 +1,118 @@ +# Bug 3 — Affix process rule is wrong in the copy after "Move Sense to a New Entry" + +**Area:** Lexicon → Move Sense to a New Entry; LCM `CopyObject` / `MoAffixProcess` cloning +**Type:** Data corruption on clone +**Repos:** `FieldWorks` (command wiring) and `liblcm` (the defect) + +## Symptom as reported + +Edit an affix process rule on an entry, then split a sense off into a new entry. Immediately afterwards both entries appear to hold the updated rule. Returning to them later, one holds the old version. The reporter's read is a save / copy / live-state problem. + +## The path + +1. `CmdDataTree-Split-Sense` (`DistFiles/Language Explorer/Configuration/Lexicon/DataTreeInclude.xml:165`), message `DataTreeSplit`. +2. `DTMenuHandler.OnDataTreeSplit` (`Src/xWorks/DTMenuHandler.cs:1052-1058`) → `Slice.HandleSplitCommand()`. +3. `LexSenseUi.MoveUnderlyingObjectToCopyOfOwner` (`Src/FdoUi/FdoUiCore.cs:2093-2106`) → `ILexEntry.MoveSenseToCopy`. +4. `LexEntry.MoveSenseToCopy` (`liblcm/src/SIL.LCModel/DomainImpl/OverridesLing_Lex.cs:1652`) creates the new entry and deep-copies the allomorphs: + - `OverridesLing_Lex.cs:1670` — `CopyObject.CloneLcmObject(LexemeFormOA, ...)` + - `OverridesLing_Lex.cs:1672` — `CopyObject.CloneLcmObjects(AlternateFormsOS, ...)` + +An affix process rule is a `MoAffixProcess`, a `MoForm` subclass, so it is cloned by step 4 through the generic reflection-based `CopyObject`. + +## Root cause: `MoAffixProcess.PostClone` is broken + +`MoAffixProcess` does **not** implement `ICloneableCmObject` — unlike `PhRegularRule` and `PhMetathesisRule`, which have hand-written `SetCloneProperties` implementations (`OverridesLing_Lex.cs:7683` and `:8176`). So it goes through generic reflection cloning and then relies on a `PostClone` hook to repair the result. + +The hook exists because `MoAffixProcess.SetDefaultValuesAfterInit` (`OverridesLing_MoClasses.cs:4037-4048`) seeds every newly created affix process with a default `PhVariable` in `InputOS` and a default `MoCopyFromInput` in `OutputOS`. `CopyObject` creates the clone through the normal factory (`CopyObject.cs:301-373`), so the clone gets those defaults, and then `HandleObjFlid` (`CopyObject.cs:582-595`) appends the cloned real inputs and outputs after them. `PostClone` is supposed to strip the two defaults back off: + +```csharp +public override void PostClone(Dictionary copyMap) +{ + foreach (var cmObject in copyMap.Values) + { + var clonedProcess = cmObject as IMoAffixProcess; + if (clonedProcess == null) + return; // <-- (A) + if (clonedProcess.InputOS.Count > 1) + clonedProcess.InputOS.RemoveAt(0); // <-- (B) + if (clonedProcess.OutputOS.Count > 1) + clonedProcess.OutputOS.RemoveAt(0); + } +} +``` +`liblcm/src/SIL.LCModel/DomainImpl/OverridesLing_MoClasses.cs:4056-4068` + +Three defects, all CONFIRMED by reading: + +### (A) `return` where `continue` was meant — line 4061-4062 + +`copyMap` is `CopyObject.m_sourceToCopyMap` (`CopyObject.cs:44`), which holds **every** object cloned in the pass: the affix process, all of its `PhVariable` / `PhSimpleContext*` inputs, all of its `MoRuleMapping` outputs, and — because `MoveSenseToCopy` clones the whole of `AlternateFormsOS` in one batch (`CopyObject.cs:108-128`) — every object cloned from every sibling allomorph too. + +The loop bails out entirely at the first value that is not an `IMoAffixProcess`. Unless the affix process happens to be the very first entry, **the cleanup never runs**, and the clone keeps the default `PhVariable` input and default `MoCopyFromInput` output prepended to the real content. A `PhVariable` + `MoCopyFromInput` pair is precisely what an untouched, freshly created affix process rule looks like — which is a strong candidate for what the reporter is seeing as "the old version". + +### (B) Repeated `RemoveAt(0)` deletes real content + +`CopyObject` calls `PostClone` once per top-level source object (`CopyObject.cs:123-124`). If the entry has two or more affix-process allomorphs, `PostClone` fires once per affix process — and because it iterates all of `copyMap`, **each call strips index 0 from every cloned affix process**. The first call removes the defaults; the second call removes the first *real* input and output. With N affix processes on an entry, N-1 real leading input/output pairs are silently destroyed. + +### (C) The map is not scoped to the object being repaired + +Even if (A) and (B) are fixed, iterating the whole shared copy map is the wrong shape for this hook. `PostClone` should operate on this object's own clone, looked up as `copyMap[this.Hvo]`. + +## Confidence and what is not yet proven + +The three defects above are read directly from the code and are not in doubt. + +What is **not** proven is that they produce the exact reported sequence — right in both entries at first, wrong in one on return. A prepended default input/output would normally be visible immediately. Two mechanisms could explain the delay, and neither has been verified: + +- The affix process slice may render a leading `PhVariable` / `MoCopyFromInput` pair invisibly or identically to the correct display until the view is rebuilt from a reload. +- `MoCopyFromInput.ContentRA` / `MoModifyFromInput.ContentRA` reference a `PhContextOrVar` owned by the same rule's `InputOS`. `CopyObject` remaps such intra-copy references in pass 2 (`CopyObject.cs:203-238`), but if defect (B) has removed the referenced input, the surviving mapping points at a deleted or wrong context — which can render plausibly in a warm cache and differently after reload. + +**The decisive next step is a repro plus a diff of the `.fwdata` XML before and after the split**, comparing the source and cloned `MoAffixProcess` element trees. That will show immediately whether the clone carries an extra leading input/output, is missing one, or has a mis-targeted `ContentRA`. + +## Proposed fix + +In `liblcm`, `OverridesLing_MoClasses.cs:4056-4068`: + +```csharp +public override void PostClone(Dictionary copyMap) +{ + if (!copyMap.TryGetValue(Hvo, out var clone) || !(clone is IMoAffixProcess clonedProcess)) + return; + if (clonedProcess.InputOS.Count > 1) + clonedProcess.InputOS.RemoveAt(0); + if (clonedProcess.OutputOS.Count > 1) + clonedProcess.OutputOS.RemoveAt(0); +} +``` + +This fixes (A), (B) and (C) together: each source affix process repairs exactly its own clone, exactly once. + +Worth considering as a follow-up, not required for the fix: give `MoAffixProcess` a proper `ICloneableCmObject.SetCloneProperties` implementation, matching `PhRegularRule` (`OverridesLing_Lex.cs:7683`). That removes the create-defaults-then-strip-them dance entirely, at the cost of hand-maintaining the property copy. `SetCloneProperties` short-circuits both clone passes (`CopyObject.cs:169-170` and `:337-342`), so any such implementation must handle the `InputOS` → `OutputOS` `ContentRA` remapping itself. + +## Test plan + +Unit tests in `liblcm/tests/SIL.LCModel.Tests/DomainImpl/LexEntryTests.cs` (which already covers `MoveSenseToCopy`): + +1. Entry with one affix-process allomorph with a non-trivial rule → split a sense → assert the clone's `InputOS` and `OutputOS` match the source exactly, with no leading default `PhVariable` / `MoCopyFromInput`. +2. Entry with a stem allomorph **ordered before** an affix-process allomorph in `AlternateFormsOS` → split → same assertion. This is the case defect (A) breaks. +3. Entry with **two** affix-process allomorphs → split → assert neither clone has lost its first real input/output. This is the case defect (B) breaks. +4. Assert every cloned `MoCopyFromInput` / `MoModifyFromInput` `ContentRA` points into the **clone's** `InputOS`, never the source's. +5. A round-trip test: split, save, reload, re-read the clone. This is the one that speaks to the reported "comes back wrong later" symptom, and should be written even if the in-memory assertions pass. + +## Scope + +Independent of Bugs 1 and 2. Those are FieldWorks UI defects in the rule formula editor; this is an LCM domain-layer defect in the clone path. The fix lands in `liblcm` and will need a package bump in FieldWorks to ship. + +## Key files + +| Path:line | Role | +|---|---| +| `liblcm/.../DomainImpl/OverridesLing_MoClasses.cs:4056-4068` | The broken `PostClone` | +| `liblcm/.../DomainImpl/OverridesLing_MoClasses.cs:4037-4048` | `SetDefaultValuesAfterInit` — the defaults that need stripping | +| `liblcm/.../DomainImpl/OverridesLing_Lex.cs:1652-1675` | `MoveSenseToCopy`, allomorph cloning | +| `liblcm/.../DomainServices/CopyObject.cs:108-128` | Batch clone; `PostClone` invoked once per top-level source | +| `liblcm/.../DomainServices/CopyObject.cs:301-373` | Pass 1, owned clone; `ICloneableCmObject` short-circuit | +| `liblcm/.../DomainServices/CopyObject.cs:203-238` | Pass 2, reference remapping | +| `liblcm/.../DomainImpl/OverridesLing_Lex.cs:7683`, `:8176` | `PhRegularRule` / `PhMetathesisRule` — the pattern `MoAffixProcess` lacks | +| `FieldWorks/Src/FdoUi/FdoUiCore.cs:2093-2106` | UI entry point | +| `FieldWorks/Src/xWorks/DTMenuHandler.cs:1052-1058` | Command handler | From d6fbab4d2e27830c299353aea185ccbf8c15bfdf Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 19 Aug 2026 03:04:29 -0400 Subject: [PATCH 02/11] test: reproduce MoAffixProcess clone corruption after MoveSenseToCopy Five tests from the bug report's test plan, all exercising LexEntry.MoveSenseToCopy's clone of an affix-process allomorph via the generic CopyObject reflection path and MoAffixProcess.PostClone. Four of five currently fail against unfixed PostClone: - single non-trivial allomorph: OutputOS ends up short by one real entry (a previously undocumented interaction: removing the default InputOS[0] cascades via RemoveObjectSideEffectsInternal to also remove the default OutputOS[0], and PostClone's own unconditional RemoveAt(0) then strips a second, real, OutputOS entry) - stem allomorph before the affix process (defect A): the process's defaults are never stripped at all - two affix-process allomorphs (defect B): the first allomorph's clone is over-stripped, the second's defaults are never touched - save/reload round trip: the corruption is already present in-memory before saving, and persists unchanged through reload (no separate "right then wrong later" mechanism was found; the clone is simply wrong from the moment of cloning) The ContentRA-remapping test (case 4) passes: CopyObject's reference pass already targets each clone's own InputOS correctly, independent of the PostClone defects. --- .../AffixProcessCloneRoundTripTests.cs | 131 ++++++++++ .../DomainImpl/LexEntryTests.cs | 233 ++++++++++++++++++ 2 files changed, 364 insertions(+) create mode 100644 tests/SIL.LCModel.Tests/DomainImpl/AffixProcessCloneRoundTripTests.cs diff --git a/tests/SIL.LCModel.Tests/DomainImpl/AffixProcessCloneRoundTripTests.cs b/tests/SIL.LCModel.Tests/DomainImpl/AffixProcessCloneRoundTripTests.cs new file mode 100644 index 00000000..6202f53c --- /dev/null +++ b/tests/SIL.LCModel.Tests/DomainImpl/AffixProcessCloneRoundTripTests.cs @@ -0,0 +1,131 @@ +// Copyright (c) 2026 SIL International +// This software is licensed under the LGPL, version 2.1 or later +// (http://www.gnu.org/licenses/lgpl-2.1.html) + +using System; +using System.IO; +using NUnit.Framework; +using SIL.LCModel.Core.Text; +using SIL.LCModel.Infrastructure; +using SIL.TestUtilities; + +namespace SIL.LCModel.DomainImpl +{ + /// + /// Case 5 from doc/bugs/affix-process-split-sense-stale-clone.md: a save/reload round trip + /// after LexEntry.MoveSenseToCopy, to check whether the affix-process clone bug is only an + /// in-memory artifact (masked by a warm cache) or actually persists to (and is reproduced + /// from) disk -- which is what the user's reported "comes back wrong later" symptom requires. + /// This uses a real file-backed cache (kXMLWithMemoryOnlyWsMgr), unlike the in-memory-only + /// fixture used by LexEntryTests. + /// + [TestFixture] + public class AffixProcessCloneRoundTripTests + { + private TemporaryFolder m_projectsFolder; + private ILcmDirectories m_lcmDirectories; + + /// + [SetUp] + public void TestSetup() + { + m_projectsFolder = new TemporaryFolder("AffixProcessCloneRoundTrip" + Guid.NewGuid().ToString("N")); + m_lcmDirectories = new TestLcmDirectories(m_projectsFolder.Path); + } + + /// + [TearDown] + public void TestTeardown() + { + m_projectsFolder.Dispose(); + } + + /// + /// Build an entry with a non-trivial affix-process LexemeFormOA and two senses, split one + /// sense off with MoveSenseToCopy, save to disk, close the cache, reopen it from disk, and + /// re-examine the cloned affix process. If PostClone's repair is only cosmetically correct + /// in memory (e.g. because a UI slice renders the leaked default identically to real + /// content until a reload forces a rebuild), this is the test that would catch it; if the + /// in-memory clone is already wrong, this test proves the corruption is not transient. + /// + [Test] + public void MoveSenseToCopy_AffixProcessClone_SurvivesSaveAndReload() + { + var projectName = "AffixProcessCloneRoundTrip" + new Random().Next(1000000); + var path = Path.Combine(m_projectsFolder.Path, LcmFileHelper.GetXmlDataFileName(projectName)); + var projectId = new TestProjectId(BackendProviderType.kXMLWithMemoryOnlyWsMgr, path); + + Guid newEntryGuid; + int expectedInputCount = 0; + int expectedOutputCount = 0; + + using (var cache = LcmCache.CreateCacheWithNewBlankLangProj(projectId, "en", "fr", "en", + new DummyLcmUI(), m_lcmDirectories, new LcmSettings())) + { + ILexEntry entry = null; + ILexSense senseToMove = null; + UndoableUnitOfWorkHelper.Do("doit", "undoit", cache.ActionHandlerAccessor, () => + { + var ws = cache.DefaultVernWs; + var entryFactory = cache.ServiceLocator.GetInstance(); + var senseFactory = cache.ServiceLocator.GetInstance(); + + entry = entryFactory.Create(); + var process = cache.ServiceLocator.GetInstance().Create(); + entry.LexemeFormOA = process; + process.Form.set_String(ws, TsStringUtils.MakeString("ed", ws)); + process.MorphTypeRA = cache.ServiceLocator.GetInstance() + .GetObject(MoMorphTypeTags.kguidMorphSuffix); + + // Non-trivial rule: real content the user would have entered, replacing the + // SetDefaultValuesAfterInit defaults. + process.InputOS.Clear(); + process.OutputOS.Clear(); + var ctxt = cache.ServiceLocator.GetInstance().Create(); + process.InputOS.Add(ctxt); + var var1 = cache.ServiceLocator.GetInstance().Create(); + process.InputOS.Add(var1); + var copy = cache.ServiceLocator.GetInstance().Create(); + process.OutputOS.Add(copy); + copy.ContentRA = ctxt; + var modify = cache.ServiceLocator.GetInstance().Create(); + process.OutputOS.Add(modify); + modify.ContentRA = var1; + + expectedInputCount = process.InputOS.Count; + expectedOutputCount = process.OutputOS.Count; + + var sense1 = senseFactory.Create(); + entry.SensesOS.Add(sense1); + senseToMove = senseFactory.Create(); + entry.SensesOS.Add(senseToMove); + }); + + entry.MoveSenseToCopy(senseToMove); + newEntryGuid = senseToMove.Entry.Guid; + + cache.ServiceLocator.GetInstance().Save(); + } + + using (var reloaded = LcmCache.CreateCacheFromExistingData(projectId, "en", new DummyLcmUI(), + m_lcmDirectories, new LcmSettings(), new DummyProgressDlg())) + { + var newEntry = (ILexEntry)reloaded.ServiceLocator.GetObject(newEntryGuid); + var clonedProcess = newEntry.LexemeFormOA as IMoAffixProcess; + Assert.That(clonedProcess, Is.Not.Null, "reloaded clone should still be an affix process"); + + Assert.That(clonedProcess.InputOS.Count, Is.EqualTo(expectedInputCount), + "after save/reload, the clone's InputOS should match what was created, with no leaked " + + "default and no real content lost"); + Assert.That(clonedProcess.OutputOS.Count, Is.EqualTo(expectedOutputCount), + "after save/reload, the clone's OutputOS should match what was created, with no leaked " + + "default and no real content lost"); + Assert.That(clonedProcess.InputOS[0].ClassID, Is.EqualTo(PhSimpleContextNCTags.kClassId), + "first input after reload should be the real natural-class context, not a leaked default PhVariable"); + Assert.That(clonedProcess.InputOS[1].ClassID, Is.EqualTo(PhVariableTags.kClassId)); + Assert.That(clonedProcess.OutputOS[0].ClassID, Is.EqualTo(MoCopyFromInputTags.kClassId)); + Assert.That(clonedProcess.OutputOS[1].ClassID, Is.EqualTo(MoModifyFromInputTags.kClassId)); + } + } + } +} diff --git a/tests/SIL.LCModel.Tests/DomainImpl/LexEntryTests.cs b/tests/SIL.LCModel.Tests/DomainImpl/LexEntryTests.cs index 18df7b9a..7e411e34 100644 --- a/tests/SIL.LCModel.Tests/DomainImpl/LexEntryTests.cs +++ b/tests/SIL.LCModel.Tests/DomainImpl/LexEntryTests.cs @@ -476,6 +476,239 @@ private ILexEntry MakeAffixProcessEntry(string form, Guid morphType) return entry; } + /// + /// Adds an affix-process allomorph (with a real, non-trivial form) to an entry's + /// AlternateFormsOS, distinct from a bare LexemeFormOA process. + /// + private IMoAffixProcess AddAffixProcessAllomorph(ILexEntry entry, string form) + { + var ws = Cache.DefaultVernWs; + var process = Cache.ServiceLocator.GetInstance().Create(); + entry.AlternateFormsOS.Add(process); + process.Form.set_String(ws, TsStringUtils.MakeString(form, ws)); + process.MorphTypeRA = Cache.ServiceLocator.GetInstance().GetObject(MoMorphTypeTags.kguidMorphSuffix); + return process; + } + + /// + /// Adds a plain stem allomorph to an entry's AlternateFormsOS. + /// + private IMoStemAllomorph AddStemAllomorph(ILexEntry entry, string form) + { + var ws = Cache.DefaultVernWs; + var stem = Cache.ServiceLocator.GetInstance().Create(); + entry.AlternateFormsOS.Add(stem); + stem.Form.set_String(ws, TsStringUtils.MakeString(form, ws)); + stem.MorphTypeRA = Cache.ServiceLocator.GetInstance().GetObject(MoMorphTypeTags.kguidMorphStem); + return stem; + } + + /// + /// Replaces the trivial default InputOS/OutputOS that SetDefaultValuesAfterInit seeds + /// with a small but non-trivial rule: Input = [naturalClassContext, variable], + /// Output = [CopyFromInput(naturalClassContext), ModifyFromInput(variable)]. + /// The two InputOS entries are deliberately different classes (PhSimpleContextNC vs + /// PhVariable) so a leaked leading default PhVariable is easy to detect by ClassID, + /// and the two OutputOS entries are deliberately different classes (MoCopyFromInput vs + /// MoModifyFromInput) so a leaked leading default MoCopyFromInput is likewise detectable. + /// + private void MakeNonTrivialRuleContent(IMoAffixProcess process) + { + process.InputOS.Clear(); + process.OutputOS.Clear(); + + var ctxt = Cache.ServiceLocator.GetInstance().Create(); + process.InputOS.Add(ctxt); + var var1 = Cache.ServiceLocator.GetInstance().Create(); + process.InputOS.Add(var1); + + var copy = Cache.ServiceLocator.GetInstance().Create(); + process.OutputOS.Add(copy); + copy.ContentRA = ctxt; + var modify = Cache.ServiceLocator.GetInstance().Create(); + process.OutputOS.Add(modify); + modify.ContentRA = var1; + } + + /// + /// Case 1 from doc/bugs/affix-process-split-sense-stale-clone.md: single affix-process + /// allomorph (as LexemeFormOA) carrying a non-trivial rule. Because this process's own + /// clone is the very first entry PostClone's copyMap sees (it is cloned before any of its + /// own owned Input/Output children), defect (A)'s "return instead of continue" does not + /// get triggered in this configuration -- see case 2 for that. + /// + [Test] + public void MoveSenseToCopy_AffixProcessClone_PreservesNonTrivialRule_SingleAllomorph() + { + ILexEntry entry = null; + ILexSense senseToMove = null; + IMoAffixProcess sourceProcess = null; + UndoableUnitOfWorkHelper.Do("doit", "undoit", Cache.ActionHandlerAccessor, () => + { + entry = MakeAffixProcessEntry("ed", MoMorphTypeTags.kguidMorphSuffix); + sourceProcess = (IMoAffixProcess)entry.LexemeFormOA; + MakeNonTrivialRuleContent(sourceProcess); + MakeSense(entry, "stay"); + senseToMove = MakeSense(entry, "move"); + }); + + entry.MoveSenseToCopy(senseToMove); + + var newEntry = senseToMove.Entry; + Assert.That(newEntry, Is.Not.EqualTo(entry)); + var clonedProcess = newEntry.LexemeFormOA as IMoAffixProcess; + Assert.That(clonedProcess, Is.Not.Null, "clone should still be an affix process"); + + Assert.That(clonedProcess.InputOS.Count, Is.EqualTo(sourceProcess.InputOS.Count), + "clone should have exactly the source's inputs, no leftover default PhVariable"); + Assert.That(clonedProcess.OutputOS.Count, Is.EqualTo(sourceProcess.OutputOS.Count), + "clone should have exactly the source's outputs, no leftover default MoCopyFromInput"); + Assert.That(clonedProcess.InputOS[0].ClassID, Is.EqualTo(PhSimpleContextNCTags.kClassId), + "first input should be the real natural-class context, not a leaked default PhVariable"); + Assert.That(clonedProcess.InputOS[1].ClassID, Is.EqualTo(PhVariableTags.kClassId)); + Assert.That(clonedProcess.OutputOS[0].ClassID, Is.EqualTo(MoCopyFromInputTags.kClassId)); + Assert.That(clonedProcess.OutputOS[1].ClassID, Is.EqualTo(MoModifyFromInputTags.kClassId)); + } + + /// + /// Case 2 from doc/bugs/affix-process-split-sense-stale-clone.md: a stem allomorph ordered + /// BEFORE the affix-process allomorph in AlternateFormsOS. Both allomorphs are cloned in a + /// single CopyObject batch (one shared copyMap), so the stem's clone -- which is not an + /// IMoAffixProcess -- lands in the map ahead of the process's own clone. Defect (A)'s + /// "return" (instead of "continue") then bails out of the whole loop before ever reaching + /// the affix process, so its defaults are never stripped at all. + /// + [Test] + public void MoveSenseToCopy_AffixProcessClone_StemAllomorphBeforeProcess_PreservesRealContent() + { + ILexEntry entry = null; + ILexSense senseToMove = null; + IMoAffixProcess sourceProcess = null; + UndoableUnitOfWorkHelper.Do("doit", "undoit", Cache.ActionHandlerAccessor, () => + { + entry = MakeEntry(); + AddStemAllomorph(entry, "stemA"); + sourceProcess = AddAffixProcessAllomorph(entry, "ed"); + MakeNonTrivialRuleContent(sourceProcess); + MakeSense(entry, "stay"); + senseToMove = MakeSense(entry, "move"); + }); + + entry.MoveSenseToCopy(senseToMove); + + var newEntry = senseToMove.Entry; + Assert.That(newEntry.AlternateFormsOS.Count, Is.EqualTo(2), "both allomorphs should have been cloned"); + var clonedProcess = newEntry.AlternateFormsOS[1] as IMoAffixProcess; + Assert.That(clonedProcess, Is.Not.Null, "second allomorph clone should still be an affix process"); + + Assert.That(clonedProcess.InputOS.Count, Is.EqualTo(sourceProcess.InputOS.Count), + "defect (A): a preceding non-process allomorph clone in the shared copyMap makes PostClone " + + "return before ever reaching (and repairing) this process's own clone"); + Assert.That(clonedProcess.OutputOS.Count, Is.EqualTo(sourceProcess.OutputOS.Count)); + Assert.That(clonedProcess.InputOS[0].ClassID, Is.EqualTo(PhSimpleContextNCTags.kClassId), + "first input should be the real natural-class context, not a leaked default PhVariable"); + } + + /// + /// Case 3 from doc/bugs/affix-process-split-sense-stale-clone.md: TWO affix-process + /// allomorphs cloned in the same batch. Defect (B): PostClone is invoked once per + /// top-level source object, but each invocation walks the ENTIRE shared copyMap from the + /// start rather than looking up only its own clone. The first allomorph's clone sits at + /// the front of the map, so every invocation re-strips index 0 from it -- the first + /// invocation correctly removes its leaked default, subsequent invocations incorrectly + /// remove real content -- while the second allomorph's own defaults are never reached + /// (the loop returns as soon as it hits the first allomorph's non-process owned child). + /// + [Test] + public void MoveSenseToCopy_AffixProcessClone_TwoProcessAllomorphs_NeitherLosesRealContent() + { + ILexEntry entry = null; + ILexSense senseToMove = null; + IMoAffixProcess sourceA = null; + IMoAffixProcess sourceB = null; + UndoableUnitOfWorkHelper.Do("doit", "undoit", Cache.ActionHandlerAccessor, () => + { + entry = MakeEntry(); + sourceA = AddAffixProcessAllomorph(entry, "edA"); + MakeNonTrivialRuleContent(sourceA); + sourceB = AddAffixProcessAllomorph(entry, "edB"); + MakeNonTrivialRuleContent(sourceB); + MakeSense(entry, "stay"); + senseToMove = MakeSense(entry, "move"); + }); + + entry.MoveSenseToCopy(senseToMove); + + var newEntry = senseToMove.Entry; + Assert.That(newEntry.AlternateFormsOS.Count, Is.EqualTo(2), "both allomorphs should have been cloned"); + var clonedA = newEntry.AlternateFormsOS[0] as IMoAffixProcess; + var clonedB = newEntry.AlternateFormsOS[1] as IMoAffixProcess; + Assert.That(clonedA, Is.Not.Null); + Assert.That(clonedB, Is.Not.Null); + + Assert.That(clonedA.InputOS.Count, Is.EqualTo(sourceA.InputOS.Count), + "defect (B): the second allomorph's PostClone call re-strips the first allomorph's " + + "already-repaired clone, deleting real content"); + Assert.That(clonedA.OutputOS.Count, Is.EqualTo(sourceA.OutputOS.Count)); + Assert.That(clonedB.InputOS.Count, Is.EqualTo(sourceB.InputOS.Count), + "defect (B): this allomorph's own defaults are never stripped because PostClone " + + "returns as soon as it hits the first allomorph's clone's owned children"); + Assert.That(clonedB.OutputOS.Count, Is.EqualTo(sourceB.OutputOS.Count)); + } + + /// + /// Case 4 from doc/bugs/affix-process-split-sense-stale-clone.md: every cloned + /// MoCopyFromInput/MoModifyFromInput ContentRA must point into the CLONE's own InputOS, + /// never into the source's InputOS (nor -- since defect (B) can delete a clone's real + /// input out from under a surviving mapping -- into thin air). Uses the same two-process + /// setup as case 3, since that is where PostClone does the most damage. + /// + [Test] + public void MoveSenseToCopy_AffixProcessClone_ContentRAPointsIntoOwnClonesInputOS() + { + ILexEntry entry = null; + ILexSense senseToMove = null; + IMoAffixProcess sourceA = null; + IMoAffixProcess sourceB = null; + UndoableUnitOfWorkHelper.Do("doit", "undoit", Cache.ActionHandlerAccessor, () => + { + entry = MakeEntry(); + sourceA = AddAffixProcessAllomorph(entry, "edA"); + MakeNonTrivialRuleContent(sourceA); + sourceB = AddAffixProcessAllomorph(entry, "edB"); + MakeNonTrivialRuleContent(sourceB); + MakeSense(entry, "stay"); + senseToMove = MakeSense(entry, "move"); + }); + + entry.MoveSenseToCopy(senseToMove); + + var newEntry = senseToMove.Entry; + var clonedA = (IMoAffixProcess)newEntry.AlternateFormsOS[0]; + var clonedB = (IMoAffixProcess)newEntry.AlternateFormsOS[1]; + + foreach (var clone in new[] { clonedA, clonedB }) + { + foreach (var mapping in clone.OutputOS) + { + IPhContextOrVar content = null; + if (mapping is IMoCopyFromInput cfi) + content = cfi.ContentRA; + else if (mapping is IMoModifyFromInput mfi) + content = mfi.ContentRA; + if (content == null) + continue; + + Assert.That(clone.InputOS.Contains(content), Is.True, + "a rule-mapping's ContentRA must point into its OWN clone's InputOS"); + Assert.That(sourceA.InputOS.Contains(content), Is.False, + "a cloned rule-mapping's ContentRA must never point into the source's InputOS"); + Assert.That(sourceB.InputOS.Contains(content), Is.False, + "a cloned rule-mapping's ContentRA must never point into the source's InputOS"); + } + } + } + /// /// Test PrimaryEntryRoots and the closely related NonTrivialEntryRoots. /// From 5f8a7156877e05d2fa3a1afa310b9d7f48a12708 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 19 Aug 2026 03:05:36 -0400 Subject: [PATCH 03/11] fix: repair only this MoAffixProcess's own clone in PostClone PostClone was iterating the entire shared copyMap (which, when several allomorphs are cloned together via CopyObject, contains every clone from every allomorph) and returning on the first entry that wasn't an IMoAffixProcess (defect A), and re-stripping every affix-process clone it found on every invocation instead of just its own (defect B). Fix: look up this object's own clone via copyMap[Hvo] and repair only that. Also fixes a previously undocumented interaction: removing the default InputOS[0] can cascade, via RemoveObjectSideEffectsInternal, into also removing the matching default OutputOS[0]. The old code's unconditional OutputOS.RemoveAt(0) would then strip a second, real, entry. The fix captures references to the two specific default objects before removing anything, and only removes the default output if it wasn't already cascade-removed. All 5 reproduction tests (4 in-memory + 1 save/reload round trip) now pass, along with the pre-existing AffixProcessesRemainUnchangedWhenSenseMovedToNewEntry. --- .../DomainImpl/OverridesLing_MoClasses.cs | 27 ++++++++++++------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/src/SIL.LCModel/DomainImpl/OverridesLing_MoClasses.cs b/src/SIL.LCModel/DomainImpl/OverridesLing_MoClasses.cs index 1d6993d4..f23db8d9 100644 --- a/src/SIL.LCModel/DomainImpl/OverridesLing_MoClasses.cs +++ b/src/SIL.LCModel/DomainImpl/OverridesLing_MoClasses.cs @@ -4055,16 +4055,23 @@ protected override void SetDefaultValuesAfterInit() /// public override void PostClone(Dictionary copyMap) { - foreach (var cmObject in copyMap.Values) - { - var clonedProcess = cmObject as IMoAffixProcess; - if (clonedProcess == null) - return; - if (clonedProcess.InputOS.Count > 1) - clonedProcess.InputOS.RemoveAt(0); - if (clonedProcess.OutputOS.Count > 1) - clonedProcess.OutputOS.RemoveAt(0); - } + // copyMap holds every object cloned in this CopyObject pass, not just this object's own + // clone (e.g. when several allomorphs of an entry are cloned together, it holds all of + // their clones and owned children). Look up only our own clone, and repair only it. + if (!copyMap.TryGetValue(Hvo, out var clone) || !(clone is IMoAffixProcess clonedProcess)) + return; + + // Capture the two specific default objects (rather than just "whatever is at index 0") + // before removing anything: removing the default input can itself cascade, via + // RemoveObjectSideEffectsInternal below, into removing the default output. If that + // already happened, we must not then remove a second, real, output entry. + IPhContextOrVar defaultInput = clonedProcess.InputOS.Count > 1 ? clonedProcess.InputOS[0] : null; + IMoRuleMapping defaultOutput = clonedProcess.OutputOS.Count > 1 ? clonedProcess.OutputOS[0] : null; + + if (defaultInput != null) + clonedProcess.InputOS.Remove(defaultInput); + if (defaultOutput != null && defaultOutput.IsValidObject) + clonedProcess.OutputOS.Remove(defaultOutput); } /// /// Gets all of the feature constraints in this rule. From e008a69010b3eaf5f91381cc522033d83639fd17 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 19 Aug 2026 03:09:11 -0400 Subject: [PATCH 04/11] docs: add architecture self-review for the PostClone fix Covers: the copyMap-shaped PostClone API and why it invited this bug, what SetDefaultValuesAfterInit's seed-then-strip seam should really be, confirms no other PostClone implementation has the same defect, evaluates (but does not implement) a full SetCloneProperties rewrite, and lists what still needs verification in a running FLEx. --- doc/bugs/affix-process-clone-review.md | 159 +++++++++++++++++++++++++ 1 file changed, 159 insertions(+) create mode 100644 doc/bugs/affix-process-clone-review.md diff --git a/doc/bugs/affix-process-clone-review.md b/doc/bugs/affix-process-clone-review.md new file mode 100644 index 00000000..37e88b1c --- /dev/null +++ b/doc/bugs/affix-process-clone-review.md @@ -0,0 +1,159 @@ +# Architecture self-review: MoAffixProcess.PostClone fix + +Companion to `affix-process-split-sense-stale-clone.md`. Written after Phase 1 +(reproduction) and Phase 2 (the minimal fix) were done and verified; see that +commit history for the actual change. + +## 1. Is `PostClone`-taking-the-whole-copy-map the right API? + +No. `void PostClone(Dictionary copyMap)` (`InterfaceAdditions.cs:88`) +hands every implementer the entire batch's source→clone map — every object +cloned in that `CopyObject` pass, across every top-level source object, not +just "your" object's clone. That is exactly the shape that produced defects +(A) and (B): the pre-fix code treated `copyMap.Values` as if it were "the +clones I own" and iterated it directly. The correct operation was always the +one-line lookup `copyMap[Hvo]`; the API affords (and, here, invited) the +wrong one. + +A safer signature would pass only this object's own clone: + +```csharp +void PostClone(ICmObject clone); +``` + +`CopyObject` already computes `copyMap[source.Hvo]` at both call sites +(`CopyObject.cs:124`, `:147`) to return the top-level clone to its own +caller, so passing that same value to `PostClone` costs nothing structurally. +If some future implementation genuinely needs sibling clones (I found none +that do — see §3), it can reach them via `clone.Owner`'s owned collections, +which is a bounded, self-scoping walk instead of an unscoped shared map. + +Cost to change: the interface has exactly three real implementers today +(`CmObject`'s no-op base, `MoAffixProcess`, and two +`throw new NotImplementedException()` stubs), plus one test double +(`AnalysisAdjusterTests.cs:4079`) that also just throws. Mechanically small. +The real cost is that `ICloneableCmObject`/the `PostClone` member sit on a +public interface shipped in the `SIL.LCModel` package, so narrowing the +signature is a breaking API change for any out-of-tree consumer with a custom +override — it would need a major/minor bump and a changelog note, not just a +patch release. I did not make this change: nothing in the current failing +tests requires it (the bug is fully fixed by scoping the lookup inside the +existing signature), and reshaping a public interface isn't warranted by a +bug fix alone. I recommend it as a follow-up, done deliberately with its own +compatibility review. + +## 2. What can be removed or simplified? + +`SetDefaultValuesAfterInit` (`OverridesLing_MoClasses.cs:4037-4048`) seeding +every new `MoAffixProcess` with a default `PhVariable` input and +`MoCopyFromInput` output — purely so the Affix Process UI slice has a +non-empty, editable row to show for a brand-new rule (the comment cites +FWR-1619) — and then requiring a clone-time hook to strip that seam back off +is exactly the wrong seam. It couples two files (`:4037` and `:4056`) through +an unenforced shared assumption ("clones always have exactly one leaked +default pair, appended-before-real-content-follows"), with nothing but a +comment to keep them in sync. That coupling is *why* this bug was possible in +the first place, and why fixing it revealed a second, undocumented coupling +(§ below): removing the default input can itself cascade, via +`RemoveObjectSideEffectsInternal`, into removing the default output, so even +"the obvious fix" of unconditionally stripping index 0 from both lists is +wrong without checking whether the cascade already did half the job. + +The genuinely categorical removal is to stop seeding defaults during +*cloning* at all — i.e., give `MoAffixProcess` an `ICloneableCmObject` +implementation (§4) so the clone path never calls +`SetDefaultValuesAfterInit` in the first place, and `PostClone`/the +strip-defaults dance disappears entirely. I evaluated this in depth (§4) but +did not implement it, so `SetDefaultValuesAfterInit` and the (now-correct) +`PostClone` both remain. I did not find any other dead code the minimal fix +could remove; the fix is a same-size rewrite of one method body. + +## 3. Did I find the same defect pattern elsewhat in other `PostClone` implementations? + +No. A full-repo search for `PostClone` turns up only: + +- `CmObject.PostClone` (`DomainImpl/CmObject.cs:606`) — the base virtual, + a correct no-op ("up to subclasses to override"). +- `MoAffixProcess.PostClone` — the one fixed here. +- `DomainObjectServices.cs:1398` and + `Application/ApplicationServices/SingleLexReference.cs:358` — both + `throw new NotImplementedException()`. Neither iterates `copyMap`, so + neither has the return-instead-of-continue or whole-map-strip defects; + they simply never implemented the hook (their objects presumably never go + through `CopyObject`'s generic path in practice). Left unchanged — that is + out of this bug's scope, and I have no evidence they're ever reached. +- `AnalysisAdjusterTests.cs:4079` — a test double, also just throws. + +## 4. What did I NOT fix, and why? + +- **The `PostClone` signature** (§1) — recommended, not executed. No failing + test requires it; it's a public API change that deserves its own + versioning decision, not a rider on a bug fix. + +- **`ICloneableCmObject.SetCloneProperties` for `MoAffixProcess`** — the + "deeper, categorical" option the bug doc raised as worth considering. I + looked hard at this and chose not to implement it: + + `PhRegularRule.SetCloneProperties` (`OverridesLing_Lex.cs:7683-7701`) is + not a safe template to copy verbatim, because it clones two owned lists + (`RightHandSidesOS`, `StrucDescOS`) that don't reference each other via a + plain object reference — their only sharing is through + `PhFeatureConstraint`, which is a deliberately-shared, deliberately + *not*-remapped pooled reference (same object, same identity, in both the + original and the clone; see `DuplicateRegularRule_SharedConstraintSurvivesUntilLastRuleDeleted` + in `LingTests.cs`). `MoAffixProcess` is a harder case: `MoCopyFromInput` + and `MoModifyFromInput` in `OutputOS` have a `ContentRA` that *must* be + re-targeted at the clone's own `InputOS` — that's not optional sharing, + it's the entire meaning of the rule. `SetCloneProperties` bypasses + `CopyObject`'s own reference-remap pass entirely (short-circuited at + `CopyObject.cs:169-170` and `:337-342`), so a correct implementation would + have to: + 1. Clone `InputOS` itself, building a `Hvo → clone` map by hand. + 2. Clone `OutputOS`, then walk it re-targeting `ContentRA` through that + map for exactly the two `MoRuleMapping` subclasses whose `Content` + targets `InputOS` (`MoCopyFromInput`, `MoModifyFromInput`) — while + leaving the other two (`MoInsertPhones.Content` → `PhTerminalUnit`, + `MoInsertNC.Content` → `PhNaturalClass`, both shared phonological- + inventory references, confirmed from `MasterLCModel.xml:4002-4025`) + untouched, since those must **not** be remapped. + + That's a correct, buildable design (sketched, not written), but it + roughly doubles this class's clone-handling code and adds edge cases + (null `ContentRA`, `MoModifyFromInput.ModificationRA`, making sure the + hand-rolled switch is exhaustive over `MoRuleMapping` subclasses now and + in the future) that I have not written dedicated tests for. The task's + own bar — implement only if tests can prove it correct — argues against + landing it now: the minimal fix already makes all 5 reproduction tests + and the full 1732-test suite pass with zero regressions, so nothing + currently broken demands the larger change. Doing it without tests for + those edge cases would be "half-doing it." I'm recommending it as a + well-scoped follow-up, not doing it here. + +- **`SetDefaultValuesAfterInit` itself** — left as-is. It's still needed for + genuinely-new, user-created processes (FWR-1619); only the clone side + changed. + +## 5. What still needs verification in a running FLEx? + +This repo's tests (including the new save/reload round trip) prove the LCM +domain data is correct in memory and after a real XML-backend reload. Two +things are outside this repo's reach: + +1. **UI redraw.** Does the FieldWorks Affix Process slice actually reflect + the corrected `InputOS`/`OutputOS` immediately after + `LexEntry.MoveSenseToCopy`, without requiring a manual refresh? This + repo can't exercise `Src/xWorks`/`Src/FdoUi`. Decisive evidence: in a + FLEx build against this fix, create an affix-process rule with 2+ real + inputs/outputs, use "Move Sense to a New Entry," and check the new + entry's Affix Process slice (a) immediately, (b) after navigating away + and back, and (c) after closing and reopening the project — all three + should show identical, correct content. Phase 1 found no "right then + wrong later" timing effect at the LCM level (the corruption, when + present, is there from the moment of cloning, and reload merely + persists it unchanged) — so if the live-FLEx symptom really is + "right at first, wrong later," that timing effect must come from + somewhere in the UI/caching layer above LCM, not from `PostClone`. That + would be worth chasing down as a separate investigation if reproduced. +2. **Packaging.** Per the bug doc's Scope section, FieldWorks needs a + `liblcm` package bump to pick up this fix at all; that step is outside + this worktree. From a263bb3c41c3ba9159fe6c31466f05845402389f Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 19 Aug 2026 05:43:30 -0400 Subject: [PATCH 05/11] test: strengthen affix-process clone tests to identity-at-position Mutation testing found that TwoProcessAllomorphs_NeitherLosesRealContent and ContentRAPointsIntoOwnClonesInputOS both passed against a mutant that captured index 1 instead of index 0 as the "default" to remove -- right count, wrong object removed, real content lost. Count and Contains()-based assertions can't see that. Added AssertNonTrivialRuleClonedCorrectly, checking ClassID at each InputOS/OutputOS position and reference-equality of each mapping's ContentRA to the exact clone InputOS slot it must target, and applied it to all four in-memory clone tests. Verified the index-1 mutation now fails all five (previously: two of five still passed). Also documented, rather than removed, two other mutations the reviewer found to be currently-behavioral-no-ops (IsValidObject guard on defaultOutput; Remove(defaultInput) vs RemoveAt(0)): kept both as defensive code with a one-line rationale each, since dropping them would only be justified by today's invariants, not any test. --- .../DomainImpl/OverridesLing_MoClasses.cs | 2 + .../DomainImpl/LexEntryTests.cs | 71 +++++++++++++------ 2 files changed, 50 insertions(+), 23 deletions(-) diff --git a/src/SIL.LCModel/DomainImpl/OverridesLing_MoClasses.cs b/src/SIL.LCModel/DomainImpl/OverridesLing_MoClasses.cs index f23db8d9..1a6e305e 100644 --- a/src/SIL.LCModel/DomainImpl/OverridesLing_MoClasses.cs +++ b/src/SIL.LCModel/DomainImpl/OverridesLing_MoClasses.cs @@ -4068,8 +4068,10 @@ public override void PostClone(Dictionary copyMap) IPhContextOrVar defaultInput = clonedProcess.InputOS.Count > 1 ? clonedProcess.InputOS[0] : null; IMoRuleMapping defaultOutput = clonedProcess.OutputOS.Count > 1 ? clonedProcess.OutputOS[0] : null; + // Remove(defaultInput), not RemoveAt(0): stays correct even if InputOS is ever reordered first. if (defaultInput != null) clonedProcess.InputOS.Remove(defaultInput); + // Defensive: Remove() already no-ops if defaultOutput isn't present. if (defaultOutput != null && defaultOutput.IsValidObject) clonedProcess.OutputOS.Remove(defaultOutput); } diff --git a/tests/SIL.LCModel.Tests/DomainImpl/LexEntryTests.cs b/tests/SIL.LCModel.Tests/DomainImpl/LexEntryTests.cs index 7e411e34..9109def8 100644 --- a/tests/SIL.LCModel.Tests/DomainImpl/LexEntryTests.cs +++ b/tests/SIL.LCModel.Tests/DomainImpl/LexEntryTests.cs @@ -530,6 +530,41 @@ private void MakeNonTrivialRuleContent(IMoAffixProcess process) modify.ContentRA = var1; } + /// + /// Asserts that a clone produced from a MakeNonTrivialRuleContent source has exactly the + /// right objects in exactly the right order -- not merely the right counts. A mutant that + /// removes the wrong list element (e.g. index 1 instead of index 0) can leave counts and + /// simple containment checks satisfied while corrupting the actual content; checking + /// ClassID at each position, and checking that each mapping's ContentRA is reference-equal + /// to the specific InputOS slot it is supposed to target, catches that. + /// + private void AssertNonTrivialRuleClonedCorrectly(IMoAffixProcess clone) + { + Assert.That(clone.InputOS.Count, Is.EqualTo(2), + "clone should have exactly the two real inputs: no leaked default, no lost real content"); + Assert.That(clone.InputOS[0].ClassID, Is.EqualTo(PhSimpleContextNCTags.kClassId), + "position 0 must be the real natural-class context -- not a leaked default PhVariable, " + + "and not some other real item shifted into this slot by removing the wrong element"); + Assert.That(clone.InputOS[1].ClassID, Is.EqualTo(PhVariableTags.kClassId), + "position 1 must be the real variable"); + + Assert.That(clone.OutputOS.Count, Is.EqualTo(2), + "clone should have exactly the two real outputs: no leaked default, no lost real content"); + Assert.That(clone.OutputOS[0].ClassID, Is.EqualTo(MoCopyFromInputTags.kClassId), + "position 0 must be the real copy-mapping"); + Assert.That(clone.OutputOS[1].ClassID, Is.EqualTo(MoModifyFromInputTags.kClassId), + "position 1 must be the real modify-mapping"); + + var copy = (IMoCopyFromInput)clone.OutputOS[0]; + var modify = (IMoModifyFromInput)clone.OutputOS[1]; + Assert.That(copy.ContentRA, Is.EqualTo(clone.InputOS[0]), + "the copy-mapping's ContentRA must be exactly this clone's own natural-class input object, " + + "identified by reference, not merely 'some' member of InputOS"); + Assert.That(modify.ContentRA, Is.EqualTo(clone.InputOS[1]), + "the modify-mapping's ContentRA must be exactly this clone's own variable input object, " + + "identified by reference, not merely 'some' member of InputOS"); + } + /// /// Case 1 from doc/bugs/affix-process-split-sense-stale-clone.md: single affix-process /// allomorph (as LexemeFormOA) carrying a non-trivial rule. Because this process's own @@ -559,15 +594,7 @@ public void MoveSenseToCopy_AffixProcessClone_PreservesNonTrivialRule_SingleAllo var clonedProcess = newEntry.LexemeFormOA as IMoAffixProcess; Assert.That(clonedProcess, Is.Not.Null, "clone should still be an affix process"); - Assert.That(clonedProcess.InputOS.Count, Is.EqualTo(sourceProcess.InputOS.Count), - "clone should have exactly the source's inputs, no leftover default PhVariable"); - Assert.That(clonedProcess.OutputOS.Count, Is.EqualTo(sourceProcess.OutputOS.Count), - "clone should have exactly the source's outputs, no leftover default MoCopyFromInput"); - Assert.That(clonedProcess.InputOS[0].ClassID, Is.EqualTo(PhSimpleContextNCTags.kClassId), - "first input should be the real natural-class context, not a leaked default PhVariable"); - Assert.That(clonedProcess.InputOS[1].ClassID, Is.EqualTo(PhVariableTags.kClassId)); - Assert.That(clonedProcess.OutputOS[0].ClassID, Is.EqualTo(MoCopyFromInputTags.kClassId)); - Assert.That(clonedProcess.OutputOS[1].ClassID, Is.EqualTo(MoModifyFromInputTags.kClassId)); + AssertNonTrivialRuleClonedCorrectly(clonedProcess); } /// @@ -601,12 +628,7 @@ public void MoveSenseToCopy_AffixProcessClone_StemAllomorphBeforeProcess_Preserv var clonedProcess = newEntry.AlternateFormsOS[1] as IMoAffixProcess; Assert.That(clonedProcess, Is.Not.Null, "second allomorph clone should still be an affix process"); - Assert.That(clonedProcess.InputOS.Count, Is.EqualTo(sourceProcess.InputOS.Count), - "defect (A): a preceding non-process allomorph clone in the shared copyMap makes PostClone " + - "return before ever reaching (and repairing) this process's own clone"); - Assert.That(clonedProcess.OutputOS.Count, Is.EqualTo(sourceProcess.OutputOS.Count)); - Assert.That(clonedProcess.InputOS[0].ClassID, Is.EqualTo(PhSimpleContextNCTags.kClassId), - "first input should be the real natural-class context, not a leaked default PhVariable"); + AssertNonTrivialRuleClonedCorrectly(clonedProcess); } /// @@ -646,14 +668,12 @@ public void MoveSenseToCopy_AffixProcessClone_TwoProcessAllomorphs_NeitherLosesR Assert.That(clonedA, Is.Not.Null); Assert.That(clonedB, Is.Not.Null); - Assert.That(clonedA.InputOS.Count, Is.EqualTo(sourceA.InputOS.Count), - "defect (B): the second allomorph's PostClone call re-strips the first allomorph's " + - "already-repaired clone, deleting real content"); - Assert.That(clonedA.OutputOS.Count, Is.EqualTo(sourceA.OutputOS.Count)); - Assert.That(clonedB.InputOS.Count, Is.EqualTo(sourceB.InputOS.Count), - "defect (B): this allomorph's own defaults are never stripped because PostClone " + - "returns as soon as it hits the first allomorph's clone's owned children"); - Assert.That(clonedB.OutputOS.Count, Is.EqualTo(sourceB.OutputOS.Count)); + // Identity-at-position, not just counts: a mutant that removes the wrong list element + // (e.g. index 1 instead of index 0) can leave clonedA.InputOS.Count == sourceA.InputOS.Count + // while a leaked default PhVariable survives at index 0 and a real item is gone -- + // AssertNonTrivialRuleClonedCorrectly checks the actual ClassID/identity at each slot. + AssertNonTrivialRuleClonedCorrectly(clonedA); + AssertNonTrivialRuleClonedCorrectly(clonedB); } /// @@ -687,6 +707,11 @@ public void MoveSenseToCopy_AffixProcessClone_ContentRAPointsIntoOwnClonesInputO var clonedA = (IMoAffixProcess)newEntry.AlternateFormsOS[0]; var clonedB = (IMoAffixProcess)newEntry.AlternateFormsOS[1]; + // Identity-at-position first: confirms each clone's own InputOS/OutputOS is exactly + // right (no leaked default swapped in for a real item at the same slot count). + AssertNonTrivialRuleClonedCorrectly(clonedA); + AssertNonTrivialRuleClonedCorrectly(clonedB); + foreach (var clone in new[] { clonedA, clonedB }) { foreach (var mapping in clone.OutputOS) From ebceb46fb9055ffedb54b600ca30f12765f02b64 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 19 Aug 2026 05:51:37 -0400 Subject: [PATCH 06/11] test: add permanent test for the WfiMorphBundle failover clone path MoveSenseToCopy reaches MoAffixProcess a second, independent time through CreateMatchingAllomorphInTargetEntry (OverridesLing_Lex.cs:1803), called from UpdateReferencesForSenseMove, whenever a WfiMorphBundle references the moved sense's morph. An affix process's Form is normally left blank (Form's own doc comment says it's undefined for process affixes), which makes IsMatchingAllomorph fail to match the already-cloned LexemeFormOA and forces this second, independent CopyObject clone -- a path neither the original bug report nor the four existing reproduction tests touch. Verified red at the pre-fix PostClone body (OutputOS Expected 2, But was 1 -- same shape as the single-allomorph case) and green with the current fix, confirming the Hvo-scoped PostClone generalizes correctly to this path without further changes. Wrapped the assertions in try/finally with an explicit ActionHandlerAccessor.Commit() to sidestep a separate, pre-existing bug (reproduces identically before and after this fix): undoing this test's WfiMorphBundle reference changes throws KeyNotFoundException out of LcmAtomicRefPropertyChanged.Undo() during TestTearDown's UndoAll(). Not fixed here -- out of scope and pre-existing. --- .../DomainImpl/LexEntryTests.cs | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/tests/SIL.LCModel.Tests/DomainImpl/LexEntryTests.cs b/tests/SIL.LCModel.Tests/DomainImpl/LexEntryTests.cs index 9109def8..1342b65d 100644 --- a/tests/SIL.LCModel.Tests/DomainImpl/LexEntryTests.cs +++ b/tests/SIL.LCModel.Tests/DomainImpl/LexEntryTests.cs @@ -734,6 +734,73 @@ public void MoveSenseToCopy_AffixProcessClone_ContentRAPointsIntoOwnClonesInputO } } + /// + /// A third clone path, found in adversarial review, that neither the original bug report + /// nor the four tests above exercise: MoveSenseToCopy reaches MoAffixProcess a SECOND time + /// through CreateMatchingAllomorphInTargetEntry (OverridesLing_Lex.cs:1803), called from + /// UpdateReferencesForSenseMove (:1758-1786), whenever a WfiMorphBundle references the moved + /// sense's morph. IsMatchingAllomorph compares Form text across writing systems; an affix + /// process's Form is normally left blank (the model's own doc comment says Form is + /// undefined for process affixes), so it never matches the already-cloned LexemeFormOA, and + /// the failover clones the SOURCE process a second, independent time via its own + /// CopyObject<IMoForm> call, landing in AlternateFormsOS rather than LexemeFormOA. + /// + [Test] + public void MoveSenseToCopy_AffixProcessClone_ViaMorphBundleFailoverPath_PreservesNonTrivialRule() + { + ILexEntry entry = null; + ILexSense senseToMove = null; + IMoAffixProcess sourceProcess = null; + UndoableUnitOfWorkHelper.Do("doit", "undoit", Cache.ActionHandlerAccessor, () => + { + entry = MakeEntry(); + sourceProcess = Cache.ServiceLocator.GetInstance().Create(); + entry.LexemeFormOA = sourceProcess; + // Deliberately leave Form blank: that is the normal state for a process affix, and + // it is exactly what makes IsMatchingAllomorph fail to match the already-cloned + // LexemeFormOA, forcing the CreateMatchingAllomorphInTargetEntry failover. + sourceProcess.MorphTypeRA = Cache.ServiceLocator.GetInstance() + .GetObject(MoMorphTypeTags.kguidMorphSuffix); + MakeNonTrivialRuleContent(sourceProcess); + + MakeSense(entry, "stay"); + senseToMove = MakeSense(entry, "move"); + + var wf = MakeWordform("ted"); + MakeAnalysis(wf, senseToMove); + }); + + entry.MoveSenseToCopy(senseToMove); + + try + { + var newEntry = senseToMove.Entry; + Assert.That(newEntry, Is.Not.EqualTo(entry)); + + var mb = (IWfiMorphBundle)senseToMove.ReferringObjects.First(o => o is IWfiMorphBundle); + var failoverClone = mb.MorphRA as IMoAffixProcess; + Assert.That(failoverClone, Is.Not.Null, + "the blank-Form failover should have created a second clone of the affix process"); + Assert.That(newEntry.AlternateFormsOS.Contains(failoverClone), Is.True, + "the failover clone should be a real allomorph on the new entry"); + Assert.That(failoverClone, Is.Not.EqualTo(newEntry.LexemeFormOA), + "the failover path creates a SECOND, independent clone, distinct from the one made " + + "by the normal LexemeFormOA clone path"); + + AssertNonTrivialRuleClonedCorrectly(failoverClone); + } + finally + { + // Pre-existing bug, reproducible identically before and after this fix: undoing the + // WfiMorphBundle.MorphRA reference changes this test's setup UOW recorded throws + // KeyNotFoundException out of LcmAtomicRefPropertyChanged.Undo() during + // TestTearDown's UndoAll(). Committing here -- which is exactly what UndoAll() itself + // does at its own end -- clears the undo stack first, so that unrelated crash can't + // happen and mask this test's own pass/fail. + Cache.ActionHandlerAccessor.Commit(); + } + } + /// /// Test PrimaryEntryRoots and the closely related NonTrivialEntryRoots. /// From 4014e436e0c5845e2339018cfd6ec0606129317c Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 19 Aug 2026 05:55:51 -0400 Subject: [PATCH 07/11] fix: repair PostClone against the source's own counts, not a heuristic The Count > 1 guard from the previous fix could not distinguish a genuinely empty affix process (0 real inputs/outputs, legal at the LCM level -- MoAffixProcess has no IsFieldRequired guard) from one leaked default: both look like clone count 1 after a naive single strip. Reviewer's probe demonstrated this: a source with InputOS/OutputOS Cleared to empty still ended up with a leaked default PhVariable/ MoCopyFromInput pair after MoveSenseToCopy (source 0, clone 1). PostClone now compares the clone's counts against THIS (the source object it belongs to, always in hand) and removes exactly the surplus leading items, computed from that difference rather than guessed from the clone's shape. The output surplus is recomputed after the input removal, not assumed independent, since removing the default input can itself cascade (via RemoveObjectSideEffectsInternal) into removing the default output. Verified red against the Count > 1 fix (Expected 0, But was 1) and green against this fix; added the zero-content case as a permanent regression test. Full 4-defect + round-trip + failover-path test set (8 tests) still green. --- .../DomainImpl/OverridesLing_MoClasses.cs | 31 ++++++++------ .../DomainImpl/LexEntryTests.cs | 41 +++++++++++++++++++ 2 files changed, 59 insertions(+), 13 deletions(-) diff --git a/src/SIL.LCModel/DomainImpl/OverridesLing_MoClasses.cs b/src/SIL.LCModel/DomainImpl/OverridesLing_MoClasses.cs index 1a6e305e..ac3e0ed1 100644 --- a/src/SIL.LCModel/DomainImpl/OverridesLing_MoClasses.cs +++ b/src/SIL.LCModel/DomainImpl/OverridesLing_MoClasses.cs @@ -4061,19 +4061,24 @@ public override void PostClone(Dictionary copyMap) if (!copyMap.TryGetValue(Hvo, out var clone) || !(clone is IMoAffixProcess clonedProcess)) return; - // Capture the two specific default objects (rather than just "whatever is at index 0") - // before removing anything: removing the default input can itself cascade, via - // RemoveObjectSideEffectsInternal below, into removing the default output. If that - // already happened, we must not then remove a second, real, output entry. - IPhContextOrVar defaultInput = clonedProcess.InputOS.Count > 1 ? clonedProcess.InputOS[0] : null; - IMoRuleMapping defaultOutput = clonedProcess.OutputOS.Count > 1 ? clonedProcess.OutputOS[0] : null; - - // Remove(defaultInput), not RemoveAt(0): stays correct even if InputOS is ever reordered first. - if (defaultInput != null) - clonedProcess.InputOS.Remove(defaultInput); - // Defensive: Remove() already no-ops if defaultOutput isn't present. - if (defaultOutput != null && defaultOutput.IsValidObject) - clonedProcess.OutputOS.Remove(defaultOutput); + // The clone was created via the normal factory, which seeds exactly one default + // PhVariable input and one default MoCopyFromInput output (SetDefaultValuesAfterInit) + // before this object's own real content was cloned and appended after them. So the + // clone should end up with exactly as many inputs/outputs as THIS (the source) has -- + // not "count > 1", which can't distinguish a genuinely empty source (0 real inputs) from + // a single leaked default (both look like count 1). Remove exactly the surplus leading + // items, computed from the source's own counts, rather than guessing from the clone's + // shape. + var surplusInputs = clonedProcess.InputOS.Count - InputOS.Count; + for (var i = 0; i < surplusInputs; i++) + clonedProcess.InputOS.RemoveAt(0); + + // Recompute rather than assume: removing the default input(s) above can itself cascade, + // via RemoveObjectSideEffectsInternal below, into also removing the default output, so + // OutputOS's own surplus must be measured after the input removal, not derived from it. + var surplusOutputs = clonedProcess.OutputOS.Count - OutputOS.Count; + for (var i = 0; i < surplusOutputs; i++) + clonedProcess.OutputOS.RemoveAt(0); } /// /// Gets all of the feature constraints in this rule. diff --git a/tests/SIL.LCModel.Tests/DomainImpl/LexEntryTests.cs b/tests/SIL.LCModel.Tests/DomainImpl/LexEntryTests.cs index 1342b65d..edefb1be 100644 --- a/tests/SIL.LCModel.Tests/DomainImpl/LexEntryTests.cs +++ b/tests/SIL.LCModel.Tests/DomainImpl/LexEntryTests.cs @@ -801,6 +801,47 @@ public void MoveSenseToCopy_AffixProcessClone_ViaMorphBundleFailoverPath_Preserv } } + /// + /// Residual bug, found in adversarial review: an affix process whose InputOS/OutputOS are + /// legitimately empty (Clear(), no re-add -- legal at the LCM level; MoAffixProcess has no + /// IsFieldRequired guard forcing at least one of each) still ends up with a leaked default + /// PhVariable/MoCopyFromInput pair after MoveSenseToCopy. A "clonedProcess.Count > 1" guard + /// cannot tell "genuinely zero real content" (clone ends at 1: the seeded default) apart + /// from "one real item plus a leaked default" (also count 1 after a naive single strip) -- + /// both look like count 1. The fix compares against the SOURCE's own (zero) counts instead. + /// + [Test] + public void MoveSenseToCopy_AffixProcessClone_ZeroRealContent_NoLeakedDefault() + { + ILexEntry entry = null; + ILexSense senseToMove = null; + IMoAffixProcess sourceProcess = null; + UndoableUnitOfWorkHelper.Do("doit", "undoit", Cache.ActionHandlerAccessor, () => + { + entry = MakeAffixProcessEntry("ed", MoMorphTypeTags.kguidMorphSuffix); + sourceProcess = (IMoAffixProcess)entry.LexemeFormOA; + // Legitimately empty: Clear() with no re-add. Nothing at the LCM level requires a + // process affix to have any Input/Output content. + sourceProcess.InputOS.Clear(); + sourceProcess.OutputOS.Clear(); + MakeSense(entry, "stay"); + senseToMove = MakeSense(entry, "move"); + }); + + Assert.That(sourceProcess.InputOS.Count, Is.EqualTo(0)); + Assert.That(sourceProcess.OutputOS.Count, Is.EqualTo(0)); + + entry.MoveSenseToCopy(senseToMove); + + var newEntry = senseToMove.Entry; + var clonedProcess = newEntry.LexemeFormOA as IMoAffixProcess; + Assert.That(clonedProcess, Is.Not.Null); + Assert.That(clonedProcess.InputOS.Count, Is.EqualTo(0), + "a source with zero real inputs should clone to zero inputs, not one leaked default"); + Assert.That(clonedProcess.OutputOS.Count, Is.EqualTo(0), + "a source with zero real outputs should clone to zero outputs, not one leaked default"); + } + /// /// Test PrimaryEntryRoots and the closely related NonTrivialEntryRoots. /// From 6c3ddfaa15d3d97e18d264a4449e3b60bd384067 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 19 Aug 2026 05:59:32 -0400 Subject: [PATCH 08/11] docs: update architecture review with adversarial-review findings Records the mutation-testing round: identity-at-position gap in two tests (fixed), the confirmed-but-previously-untested WfiMorphBundle failover clone path, the residual zero-content bug and its categorical fix, the two mutations kept as documented defensive code, and the reviewer's Class-A sweep / disproved copy-map worry noted as follow-ups requiring no action here. --- doc/bugs/affix-process-clone-review.md | 110 ++++++++++++++++++++++++- 1 file changed, 108 insertions(+), 2 deletions(-) diff --git a/doc/bugs/affix-process-clone-review.md b/doc/bugs/affix-process-clone-review.md index 37e88b1c..82da249f 100644 --- a/doc/bugs/affix-process-clone-review.md +++ b/doc/bugs/affix-process-clone-review.md @@ -2,7 +2,10 @@ Companion to `affix-process-split-sense-stale-clone.md`. Written after Phase 1 (reproduction) and Phase 2 (the minimal fix) were done and verified; see that -commit history for the actual change. +commit history for the actual change. Updated after an adversarial-review / +mutation-testing round (§6) that found two weak tests, an untouched third +clone path, and a residual bug in the fix itself; all three are now closed, +see §6 for what changed and what didn't. ## 1. Is `PostClone`-taking-the-whole-copy-map the right API? @@ -124,7 +127,8 @@ No. A full-repo search for `PostClone` turns up only: in the future) that I have not written dedicated tests for. The task's own bar — implement only if tests can prove it correct — argues against landing it now: the minimal fix already makes all 5 reproduction tests - and the full 1732-test suite pass with zero regressions, so nothing + and the full suite (1734 tests as of the final commit) pass with zero + regressions, so nothing currently broken demands the larger change. Doing it without tests for those edge cases would be "half-doing it." I'm recommending it as a well-scoped follow-up, not doing it here. @@ -157,3 +161,105 @@ things are outside this repo's reach: 2. **Packaging.** Per the bug doc's Scope section, FieldWorks needs a `liblcm` package bump to pick up this fix at all; that step is outside this worktree. + +## 6. Adversarial review round + +An independent reviewer mutation-tested the fix and probed two more paths. +Summary of what came back and what changed: + +**Core Hvo-scoping logic survived mutation testing.** The `copyMap.TryGetValue(Hvo, ...)` +lookup itself could not be broken. + +**Two tests passed for the wrong reason.** The reviewer mutated the fix to +capture index 1 instead of index 0 as "the default" — right final count, +wrong object removed, real content silently lost. `TwoProcessAllomorphs_NeitherLosesRealContent` +and `ContentRAPointsIntoOwnClonesInputOS` both still passed, because they +asserted counts and `Contains()` membership, not identity at each position. +Added `AssertNonTrivialRuleClonedCorrectly` (checks `ClassID` at every +`InputOS`/`OutputOS` slot, and reference-equality of each mapping's `ContentRA` +to the exact slot it must target) and applied it to all affected tests. +Confirmed the transition directly: re-applied the index-1 mutation and watched +all 5 in-memory/round-trip tests fail (previously 3 failed, 2 passed); reverted +and confirmed all pass again. + +Two further mutations the reviewer found — dropping the `IsValidObject` guard +on the removed default output, and reverting the identity-based `Remove(...)` +back to index-based `RemoveAt(0)` — are currently behavioral no-ops given +`LcmOwningSequence.Remove`'s no-op-when-absent semantics and the fact that +nothing reorders `InputOS`/`OutputOS` between capture and removal. Rather than +contorting a test to force a difference that doesn't exist today, both pieces +of code were kept, each with a one-line comment stating plainly that they're +defensive against a future change in those invariants, not required by any +current test. (Both are now moot in the categorical rewrite below, which no +longer captures-then-removes a specific object at all — see §6.3.) + +**§6.1 A third clone path, confirmed corrupted pre-fix, untested until now.** +`MoveSenseToCopy` reaches `MoAffixProcess` a *second*, independent time +through `CreateMatchingAllomorphInTargetEntry` (`OverridesLing_Lex.cs:1803`), +called from `UpdateReferencesForSenseMove` (`:1758-1786`). When a +`WfiMorphBundle` references the moved sense's morph, and that morph's `Form` +is blank — which is the *normal* case for a process affix (the model's own +doc comment says `Form` is undefined for `MoAffixProcess`) — +`IsMatchingAllomorph` can never find a match (its loop only ever sets `found` +when both sides have non-empty text for some writing system), so +`mb.MorphRA` fails over to a brand-new, independent +`CopyObject.CloneLcmObject` call on the very same source. Neither +the original bug report nor the four original reproduction tests exercised +this path. Added `MoveSenseToCopy_AffixProcessClone_ViaMorphBundleFailoverPath_PreservesNonTrivialRule`; +confirmed it fails against the pre-fix `PostClone` (`OutputOS` Expected 2, But +was 1 — the same shape as the single-allomorph case) and passes against the +Hvo-scoped fix without any further code change, confirming the fix +generalizes correctly to a clone path it was never written with in mind. + +That test also hits a separate, pre-existing bug: undoing the `WfiMorphBundle` +reference changes made during its setup throws `KeyNotFoundException` out of +`LcmAtomicRefPropertyChanged.Undo()` during `TestTearDown`'s `UndoAll()` — +reproducible identically at both the pre-fix and post-fix commits. Not fixed +(out of scope, pre-existing, unrelated to affix-process cloning); the test +instead calls `Cache.ActionHandlerAccessor.Commit()` in a `finally` block, +which is exactly what `UndoAll()` itself does at its own end, so the +undo-stack is already empty by the time teardown's `Undo()` loop would +otherwise run into the bug. + +**§6.2 A residual bug in the fix, demonstrated against already-fixed code.** +An affix process whose `InputOS`/`OutputOS` are legitimately empty (`Clear()`, +no re-add — legal at the LCM level; `MoAffixProcess` has no +`IsFieldRequired` guard forcing non-empty content) still ended up with a +leaked default `PhVariable`/`MoCopyFromInput` pair: source 0, clone 1. The +`Count > 1` guard in the first fix could not tell "genuinely zero real +content" (clone count 1: only the seeded default) apart from "one real item +survives, one leaked default also survives" (also clone count 1, after a +different bug) — both looked identical by count alone. + +**§6.3 The fix, rewritten categorically.** `PostClone` now compares the +clone's counts against `this` — the source object it belongs to, always in +hand, since `PostClone` is an instance method called as +`source.PostClone(copyMap)` — and removes exactly the surplus: +`clonedProcess.InputOS.Count - InputOS.Count` leading items, then (recomputed +*after* that removal, not assumed independently, since removing the default +input can itself cascade into removing the default output) +`clonedProcess.OutputOS.Count - OutputOS.Count` leading items. This is no +longer a heuristic about what the clone's shape "should" look like; it is a +direct comparison to the one ground truth already available. It also +subsumes the identity-capture machinery from the first fix (capturing +`defaultInput`/`defaultOutput` by reference before removing anything) — with +the surplus computed by count and removed via a tight `RemoveAt(0)` loop with +no intervening work, there is no window for the object-identity concern that +motivated capturing references in the first place, so that indirection was +removed rather than kept alongside the new logic. Verified red against the +`Count > 1` fix (Expected 0, But was 1) and green against this rewrite; added +`MoveSenseToCopy_AffixProcessClone_ZeroRealContent_NoLeakedDefault` as a +permanent regression test. + +**§6.4 Not for me to act on (per reviewer's own scoping).** The reviewer ran +a Class-A sweep (objects that seed owned-sequence defaults, have no +`ICloneableCmObject`, and have no `PostClone`) and found `PhTerminalUnit.CodesOS` +and `MoStemName.RegionsOC` share this class's shape, but are unreachable from +any current clone call site — latent only, not exercised, not fixed here. +Noted as a follow-up for whoever next touches cloning in those areas. The +coordinator's original bug report also carried a worry that `LexemeFormOA` +and `AlternateFormsOS` get separate, non-communicating `CopyObject` maps +during `MoveSenseToCopy` (relevant to whether that split could itself cause +cross-referencing bugs); the reviewer reports this was disproved by schema +inspection. I did not redo that inspection myself — noting it here as +closed per the reviewer's finding, not something I independently verified. From c6f36b34cb594cd8ae759a56f45a196b76abb3b8 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 19 Aug 2026 08:08:59 -0400 Subject: [PATCH 09/11] LT-22711: move investigation and design notes to the PR description The bug analysis and architecture self-review were working documents for this fix. Their conclusions are now carried by the code and its tests; the reasoning, decisions and paths not taken live in the pull request body so they inform review without merging into the tree. --- doc/bugs/affix-process-clone-review.md | 265 ------------------ .../affix-process-split-sense-stale-clone.md | 118 -------- 2 files changed, 383 deletions(-) delete mode 100644 doc/bugs/affix-process-clone-review.md delete mode 100644 doc/bugs/affix-process-split-sense-stale-clone.md diff --git a/doc/bugs/affix-process-clone-review.md b/doc/bugs/affix-process-clone-review.md deleted file mode 100644 index 82da249f..00000000 --- a/doc/bugs/affix-process-clone-review.md +++ /dev/null @@ -1,265 +0,0 @@ -# Architecture self-review: MoAffixProcess.PostClone fix - -Companion to `affix-process-split-sense-stale-clone.md`. Written after Phase 1 -(reproduction) and Phase 2 (the minimal fix) were done and verified; see that -commit history for the actual change. Updated after an adversarial-review / -mutation-testing round (§6) that found two weak tests, an untouched third -clone path, and a residual bug in the fix itself; all three are now closed, -see §6 for what changed and what didn't. - -## 1. Is `PostClone`-taking-the-whole-copy-map the right API? - -No. `void PostClone(Dictionary copyMap)` (`InterfaceAdditions.cs:88`) -hands every implementer the entire batch's source→clone map — every object -cloned in that `CopyObject` pass, across every top-level source object, not -just "your" object's clone. That is exactly the shape that produced defects -(A) and (B): the pre-fix code treated `copyMap.Values` as if it were "the -clones I own" and iterated it directly. The correct operation was always the -one-line lookup `copyMap[Hvo]`; the API affords (and, here, invited) the -wrong one. - -A safer signature would pass only this object's own clone: - -```csharp -void PostClone(ICmObject clone); -``` - -`CopyObject` already computes `copyMap[source.Hvo]` at both call sites -(`CopyObject.cs:124`, `:147`) to return the top-level clone to its own -caller, so passing that same value to `PostClone` costs nothing structurally. -If some future implementation genuinely needs sibling clones (I found none -that do — see §3), it can reach them via `clone.Owner`'s owned collections, -which is a bounded, self-scoping walk instead of an unscoped shared map. - -Cost to change: the interface has exactly three real implementers today -(`CmObject`'s no-op base, `MoAffixProcess`, and two -`throw new NotImplementedException()` stubs), plus one test double -(`AnalysisAdjusterTests.cs:4079`) that also just throws. Mechanically small. -The real cost is that `ICloneableCmObject`/the `PostClone` member sit on a -public interface shipped in the `SIL.LCModel` package, so narrowing the -signature is a breaking API change for any out-of-tree consumer with a custom -override — it would need a major/minor bump and a changelog note, not just a -patch release. I did not make this change: nothing in the current failing -tests requires it (the bug is fully fixed by scoping the lookup inside the -existing signature), and reshaping a public interface isn't warranted by a -bug fix alone. I recommend it as a follow-up, done deliberately with its own -compatibility review. - -## 2. What can be removed or simplified? - -`SetDefaultValuesAfterInit` (`OverridesLing_MoClasses.cs:4037-4048`) seeding -every new `MoAffixProcess` with a default `PhVariable` input and -`MoCopyFromInput` output — purely so the Affix Process UI slice has a -non-empty, editable row to show for a brand-new rule (the comment cites -FWR-1619) — and then requiring a clone-time hook to strip that seam back off -is exactly the wrong seam. It couples two files (`:4037` and `:4056`) through -an unenforced shared assumption ("clones always have exactly one leaked -default pair, appended-before-real-content-follows"), with nothing but a -comment to keep them in sync. That coupling is *why* this bug was possible in -the first place, and why fixing it revealed a second, undocumented coupling -(§ below): removing the default input can itself cascade, via -`RemoveObjectSideEffectsInternal`, into removing the default output, so even -"the obvious fix" of unconditionally stripping index 0 from both lists is -wrong without checking whether the cascade already did half the job. - -The genuinely categorical removal is to stop seeding defaults during -*cloning* at all — i.e., give `MoAffixProcess` an `ICloneableCmObject` -implementation (§4) so the clone path never calls -`SetDefaultValuesAfterInit` in the first place, and `PostClone`/the -strip-defaults dance disappears entirely. I evaluated this in depth (§4) but -did not implement it, so `SetDefaultValuesAfterInit` and the (now-correct) -`PostClone` both remain. I did not find any other dead code the minimal fix -could remove; the fix is a same-size rewrite of one method body. - -## 3. Did I find the same defect pattern elsewhat in other `PostClone` implementations? - -No. A full-repo search for `PostClone` turns up only: - -- `CmObject.PostClone` (`DomainImpl/CmObject.cs:606`) — the base virtual, - a correct no-op ("up to subclasses to override"). -- `MoAffixProcess.PostClone` — the one fixed here. -- `DomainObjectServices.cs:1398` and - `Application/ApplicationServices/SingleLexReference.cs:358` — both - `throw new NotImplementedException()`. Neither iterates `copyMap`, so - neither has the return-instead-of-continue or whole-map-strip defects; - they simply never implemented the hook (their objects presumably never go - through `CopyObject`'s generic path in practice). Left unchanged — that is - out of this bug's scope, and I have no evidence they're ever reached. -- `AnalysisAdjusterTests.cs:4079` — a test double, also just throws. - -## 4. What did I NOT fix, and why? - -- **The `PostClone` signature** (§1) — recommended, not executed. No failing - test requires it; it's a public API change that deserves its own - versioning decision, not a rider on a bug fix. - -- **`ICloneableCmObject.SetCloneProperties` for `MoAffixProcess`** — the - "deeper, categorical" option the bug doc raised as worth considering. I - looked hard at this and chose not to implement it: - - `PhRegularRule.SetCloneProperties` (`OverridesLing_Lex.cs:7683-7701`) is - not a safe template to copy verbatim, because it clones two owned lists - (`RightHandSidesOS`, `StrucDescOS`) that don't reference each other via a - plain object reference — their only sharing is through - `PhFeatureConstraint`, which is a deliberately-shared, deliberately - *not*-remapped pooled reference (same object, same identity, in both the - original and the clone; see `DuplicateRegularRule_SharedConstraintSurvivesUntilLastRuleDeleted` - in `LingTests.cs`). `MoAffixProcess` is a harder case: `MoCopyFromInput` - and `MoModifyFromInput` in `OutputOS` have a `ContentRA` that *must* be - re-targeted at the clone's own `InputOS` — that's not optional sharing, - it's the entire meaning of the rule. `SetCloneProperties` bypasses - `CopyObject`'s own reference-remap pass entirely (short-circuited at - `CopyObject.cs:169-170` and `:337-342`), so a correct implementation would - have to: - 1. Clone `InputOS` itself, building a `Hvo → clone` map by hand. - 2. Clone `OutputOS`, then walk it re-targeting `ContentRA` through that - map for exactly the two `MoRuleMapping` subclasses whose `Content` - targets `InputOS` (`MoCopyFromInput`, `MoModifyFromInput`) — while - leaving the other two (`MoInsertPhones.Content` → `PhTerminalUnit`, - `MoInsertNC.Content` → `PhNaturalClass`, both shared phonological- - inventory references, confirmed from `MasterLCModel.xml:4002-4025`) - untouched, since those must **not** be remapped. - - That's a correct, buildable design (sketched, not written), but it - roughly doubles this class's clone-handling code and adds edge cases - (null `ContentRA`, `MoModifyFromInput.ModificationRA`, making sure the - hand-rolled switch is exhaustive over `MoRuleMapping` subclasses now and - in the future) that I have not written dedicated tests for. The task's - own bar — implement only if tests can prove it correct — argues against - landing it now: the minimal fix already makes all 5 reproduction tests - and the full suite (1734 tests as of the final commit) pass with zero - regressions, so nothing - currently broken demands the larger change. Doing it without tests for - those edge cases would be "half-doing it." I'm recommending it as a - well-scoped follow-up, not doing it here. - -- **`SetDefaultValuesAfterInit` itself** — left as-is. It's still needed for - genuinely-new, user-created processes (FWR-1619); only the clone side - changed. - -## 5. What still needs verification in a running FLEx? - -This repo's tests (including the new save/reload round trip) prove the LCM -domain data is correct in memory and after a real XML-backend reload. Two -things are outside this repo's reach: - -1. **UI redraw.** Does the FieldWorks Affix Process slice actually reflect - the corrected `InputOS`/`OutputOS` immediately after - `LexEntry.MoveSenseToCopy`, without requiring a manual refresh? This - repo can't exercise `Src/xWorks`/`Src/FdoUi`. Decisive evidence: in a - FLEx build against this fix, create an affix-process rule with 2+ real - inputs/outputs, use "Move Sense to a New Entry," and check the new - entry's Affix Process slice (a) immediately, (b) after navigating away - and back, and (c) after closing and reopening the project — all three - should show identical, correct content. Phase 1 found no "right then - wrong later" timing effect at the LCM level (the corruption, when - present, is there from the moment of cloning, and reload merely - persists it unchanged) — so if the live-FLEx symptom really is - "right at first, wrong later," that timing effect must come from - somewhere in the UI/caching layer above LCM, not from `PostClone`. That - would be worth chasing down as a separate investigation if reproduced. -2. **Packaging.** Per the bug doc's Scope section, FieldWorks needs a - `liblcm` package bump to pick up this fix at all; that step is outside - this worktree. - -## 6. Adversarial review round - -An independent reviewer mutation-tested the fix and probed two more paths. -Summary of what came back and what changed: - -**Core Hvo-scoping logic survived mutation testing.** The `copyMap.TryGetValue(Hvo, ...)` -lookup itself could not be broken. - -**Two tests passed for the wrong reason.** The reviewer mutated the fix to -capture index 1 instead of index 0 as "the default" — right final count, -wrong object removed, real content silently lost. `TwoProcessAllomorphs_NeitherLosesRealContent` -and `ContentRAPointsIntoOwnClonesInputOS` both still passed, because they -asserted counts and `Contains()` membership, not identity at each position. -Added `AssertNonTrivialRuleClonedCorrectly` (checks `ClassID` at every -`InputOS`/`OutputOS` slot, and reference-equality of each mapping's `ContentRA` -to the exact slot it must target) and applied it to all affected tests. -Confirmed the transition directly: re-applied the index-1 mutation and watched -all 5 in-memory/round-trip tests fail (previously 3 failed, 2 passed); reverted -and confirmed all pass again. - -Two further mutations the reviewer found — dropping the `IsValidObject` guard -on the removed default output, and reverting the identity-based `Remove(...)` -back to index-based `RemoveAt(0)` — are currently behavioral no-ops given -`LcmOwningSequence.Remove`'s no-op-when-absent semantics and the fact that -nothing reorders `InputOS`/`OutputOS` between capture and removal. Rather than -contorting a test to force a difference that doesn't exist today, both pieces -of code were kept, each with a one-line comment stating plainly that they're -defensive against a future change in those invariants, not required by any -current test. (Both are now moot in the categorical rewrite below, which no -longer captures-then-removes a specific object at all — see §6.3.) - -**§6.1 A third clone path, confirmed corrupted pre-fix, untested until now.** -`MoveSenseToCopy` reaches `MoAffixProcess` a *second*, independent time -through `CreateMatchingAllomorphInTargetEntry` (`OverridesLing_Lex.cs:1803`), -called from `UpdateReferencesForSenseMove` (`:1758-1786`). When a -`WfiMorphBundle` references the moved sense's morph, and that morph's `Form` -is blank — which is the *normal* case for a process affix (the model's own -doc comment says `Form` is undefined for `MoAffixProcess`) — -`IsMatchingAllomorph` can never find a match (its loop only ever sets `found` -when both sides have non-empty text for some writing system), so -`mb.MorphRA` fails over to a brand-new, independent -`CopyObject.CloneLcmObject` call on the very same source. Neither -the original bug report nor the four original reproduction tests exercised -this path. Added `MoveSenseToCopy_AffixProcessClone_ViaMorphBundleFailoverPath_PreservesNonTrivialRule`; -confirmed it fails against the pre-fix `PostClone` (`OutputOS` Expected 2, But -was 1 — the same shape as the single-allomorph case) and passes against the -Hvo-scoped fix without any further code change, confirming the fix -generalizes correctly to a clone path it was never written with in mind. - -That test also hits a separate, pre-existing bug: undoing the `WfiMorphBundle` -reference changes made during its setup throws `KeyNotFoundException` out of -`LcmAtomicRefPropertyChanged.Undo()` during `TestTearDown`'s `UndoAll()` — -reproducible identically at both the pre-fix and post-fix commits. Not fixed -(out of scope, pre-existing, unrelated to affix-process cloning); the test -instead calls `Cache.ActionHandlerAccessor.Commit()` in a `finally` block, -which is exactly what `UndoAll()` itself does at its own end, so the -undo-stack is already empty by the time teardown's `Undo()` loop would -otherwise run into the bug. - -**§6.2 A residual bug in the fix, demonstrated against already-fixed code.** -An affix process whose `InputOS`/`OutputOS` are legitimately empty (`Clear()`, -no re-add — legal at the LCM level; `MoAffixProcess` has no -`IsFieldRequired` guard forcing non-empty content) still ended up with a -leaked default `PhVariable`/`MoCopyFromInput` pair: source 0, clone 1. The -`Count > 1` guard in the first fix could not tell "genuinely zero real -content" (clone count 1: only the seeded default) apart from "one real item -survives, one leaked default also survives" (also clone count 1, after a -different bug) — both looked identical by count alone. - -**§6.3 The fix, rewritten categorically.** `PostClone` now compares the -clone's counts against `this` — the source object it belongs to, always in -hand, since `PostClone` is an instance method called as -`source.PostClone(copyMap)` — and removes exactly the surplus: -`clonedProcess.InputOS.Count - InputOS.Count` leading items, then (recomputed -*after* that removal, not assumed independently, since removing the default -input can itself cascade into removing the default output) -`clonedProcess.OutputOS.Count - OutputOS.Count` leading items. This is no -longer a heuristic about what the clone's shape "should" look like; it is a -direct comparison to the one ground truth already available. It also -subsumes the identity-capture machinery from the first fix (capturing -`defaultInput`/`defaultOutput` by reference before removing anything) — with -the surplus computed by count and removed via a tight `RemoveAt(0)` loop with -no intervening work, there is no window for the object-identity concern that -motivated capturing references in the first place, so that indirection was -removed rather than kept alongside the new logic. Verified red against the -`Count > 1` fix (Expected 0, But was 1) and green against this rewrite; added -`MoveSenseToCopy_AffixProcessClone_ZeroRealContent_NoLeakedDefault` as a -permanent regression test. - -**§6.4 Not for me to act on (per reviewer's own scoping).** The reviewer ran -a Class-A sweep (objects that seed owned-sequence defaults, have no -`ICloneableCmObject`, and have no `PostClone`) and found `PhTerminalUnit.CodesOS` -and `MoStemName.RegionsOC` share this class's shape, but are unreachable from -any current clone call site — latent only, not exercised, not fixed here. -Noted as a follow-up for whoever next touches cloning in those areas. The -coordinator's original bug report also carried a worry that `LexemeFormOA` -and `AlternateFormsOS` get separate, non-communicating `CopyObject` maps -during `MoveSenseToCopy` (relevant to whether that split could itself cause -cross-referencing bugs); the reviewer reports this was disproved by schema -inspection. I did not redo that inspection myself — noting it here as -closed per the reviewer's finding, not something I independently verified. diff --git a/doc/bugs/affix-process-split-sense-stale-clone.md b/doc/bugs/affix-process-split-sense-stale-clone.md deleted file mode 100644 index 6eb77aa4..00000000 --- a/doc/bugs/affix-process-split-sense-stale-clone.md +++ /dev/null @@ -1,118 +0,0 @@ -# Bug 3 — Affix process rule is wrong in the copy after "Move Sense to a New Entry" - -**Area:** Lexicon → Move Sense to a New Entry; LCM `CopyObject` / `MoAffixProcess` cloning -**Type:** Data corruption on clone -**Repos:** `FieldWorks` (command wiring) and `liblcm` (the defect) - -## Symptom as reported - -Edit an affix process rule on an entry, then split a sense off into a new entry. Immediately afterwards both entries appear to hold the updated rule. Returning to them later, one holds the old version. The reporter's read is a save / copy / live-state problem. - -## The path - -1. `CmdDataTree-Split-Sense` (`DistFiles/Language Explorer/Configuration/Lexicon/DataTreeInclude.xml:165`), message `DataTreeSplit`. -2. `DTMenuHandler.OnDataTreeSplit` (`Src/xWorks/DTMenuHandler.cs:1052-1058`) → `Slice.HandleSplitCommand()`. -3. `LexSenseUi.MoveUnderlyingObjectToCopyOfOwner` (`Src/FdoUi/FdoUiCore.cs:2093-2106`) → `ILexEntry.MoveSenseToCopy`. -4. `LexEntry.MoveSenseToCopy` (`liblcm/src/SIL.LCModel/DomainImpl/OverridesLing_Lex.cs:1652`) creates the new entry and deep-copies the allomorphs: - - `OverridesLing_Lex.cs:1670` — `CopyObject.CloneLcmObject(LexemeFormOA, ...)` - - `OverridesLing_Lex.cs:1672` — `CopyObject.CloneLcmObjects(AlternateFormsOS, ...)` - -An affix process rule is a `MoAffixProcess`, a `MoForm` subclass, so it is cloned by step 4 through the generic reflection-based `CopyObject`. - -## Root cause: `MoAffixProcess.PostClone` is broken - -`MoAffixProcess` does **not** implement `ICloneableCmObject` — unlike `PhRegularRule` and `PhMetathesisRule`, which have hand-written `SetCloneProperties` implementations (`OverridesLing_Lex.cs:7683` and `:8176`). So it goes through generic reflection cloning and then relies on a `PostClone` hook to repair the result. - -The hook exists because `MoAffixProcess.SetDefaultValuesAfterInit` (`OverridesLing_MoClasses.cs:4037-4048`) seeds every newly created affix process with a default `PhVariable` in `InputOS` and a default `MoCopyFromInput` in `OutputOS`. `CopyObject` creates the clone through the normal factory (`CopyObject.cs:301-373`), so the clone gets those defaults, and then `HandleObjFlid` (`CopyObject.cs:582-595`) appends the cloned real inputs and outputs after them. `PostClone` is supposed to strip the two defaults back off: - -```csharp -public override void PostClone(Dictionary copyMap) -{ - foreach (var cmObject in copyMap.Values) - { - var clonedProcess = cmObject as IMoAffixProcess; - if (clonedProcess == null) - return; // <-- (A) - if (clonedProcess.InputOS.Count > 1) - clonedProcess.InputOS.RemoveAt(0); // <-- (B) - if (clonedProcess.OutputOS.Count > 1) - clonedProcess.OutputOS.RemoveAt(0); - } -} -``` -`liblcm/src/SIL.LCModel/DomainImpl/OverridesLing_MoClasses.cs:4056-4068` - -Three defects, all CONFIRMED by reading: - -### (A) `return` where `continue` was meant — line 4061-4062 - -`copyMap` is `CopyObject.m_sourceToCopyMap` (`CopyObject.cs:44`), which holds **every** object cloned in the pass: the affix process, all of its `PhVariable` / `PhSimpleContext*` inputs, all of its `MoRuleMapping` outputs, and — because `MoveSenseToCopy` clones the whole of `AlternateFormsOS` in one batch (`CopyObject.cs:108-128`) — every object cloned from every sibling allomorph too. - -The loop bails out entirely at the first value that is not an `IMoAffixProcess`. Unless the affix process happens to be the very first entry, **the cleanup never runs**, and the clone keeps the default `PhVariable` input and default `MoCopyFromInput` output prepended to the real content. A `PhVariable` + `MoCopyFromInput` pair is precisely what an untouched, freshly created affix process rule looks like — which is a strong candidate for what the reporter is seeing as "the old version". - -### (B) Repeated `RemoveAt(0)` deletes real content - -`CopyObject` calls `PostClone` once per top-level source object (`CopyObject.cs:123-124`). If the entry has two or more affix-process allomorphs, `PostClone` fires once per affix process — and because it iterates all of `copyMap`, **each call strips index 0 from every cloned affix process**. The first call removes the defaults; the second call removes the first *real* input and output. With N affix processes on an entry, N-1 real leading input/output pairs are silently destroyed. - -### (C) The map is not scoped to the object being repaired - -Even if (A) and (B) are fixed, iterating the whole shared copy map is the wrong shape for this hook. `PostClone` should operate on this object's own clone, looked up as `copyMap[this.Hvo]`. - -## Confidence and what is not yet proven - -The three defects above are read directly from the code and are not in doubt. - -What is **not** proven is that they produce the exact reported sequence — right in both entries at first, wrong in one on return. A prepended default input/output would normally be visible immediately. Two mechanisms could explain the delay, and neither has been verified: - -- The affix process slice may render a leading `PhVariable` / `MoCopyFromInput` pair invisibly or identically to the correct display until the view is rebuilt from a reload. -- `MoCopyFromInput.ContentRA` / `MoModifyFromInput.ContentRA` reference a `PhContextOrVar` owned by the same rule's `InputOS`. `CopyObject` remaps such intra-copy references in pass 2 (`CopyObject.cs:203-238`), but if defect (B) has removed the referenced input, the surviving mapping points at a deleted or wrong context — which can render plausibly in a warm cache and differently after reload. - -**The decisive next step is a repro plus a diff of the `.fwdata` XML before and after the split**, comparing the source and cloned `MoAffixProcess` element trees. That will show immediately whether the clone carries an extra leading input/output, is missing one, or has a mis-targeted `ContentRA`. - -## Proposed fix - -In `liblcm`, `OverridesLing_MoClasses.cs:4056-4068`: - -```csharp -public override void PostClone(Dictionary copyMap) -{ - if (!copyMap.TryGetValue(Hvo, out var clone) || !(clone is IMoAffixProcess clonedProcess)) - return; - if (clonedProcess.InputOS.Count > 1) - clonedProcess.InputOS.RemoveAt(0); - if (clonedProcess.OutputOS.Count > 1) - clonedProcess.OutputOS.RemoveAt(0); -} -``` - -This fixes (A), (B) and (C) together: each source affix process repairs exactly its own clone, exactly once. - -Worth considering as a follow-up, not required for the fix: give `MoAffixProcess` a proper `ICloneableCmObject.SetCloneProperties` implementation, matching `PhRegularRule` (`OverridesLing_Lex.cs:7683`). That removes the create-defaults-then-strip-them dance entirely, at the cost of hand-maintaining the property copy. `SetCloneProperties` short-circuits both clone passes (`CopyObject.cs:169-170` and `:337-342`), so any such implementation must handle the `InputOS` → `OutputOS` `ContentRA` remapping itself. - -## Test plan - -Unit tests in `liblcm/tests/SIL.LCModel.Tests/DomainImpl/LexEntryTests.cs` (which already covers `MoveSenseToCopy`): - -1. Entry with one affix-process allomorph with a non-trivial rule → split a sense → assert the clone's `InputOS` and `OutputOS` match the source exactly, with no leading default `PhVariable` / `MoCopyFromInput`. -2. Entry with a stem allomorph **ordered before** an affix-process allomorph in `AlternateFormsOS` → split → same assertion. This is the case defect (A) breaks. -3. Entry with **two** affix-process allomorphs → split → assert neither clone has lost its first real input/output. This is the case defect (B) breaks. -4. Assert every cloned `MoCopyFromInput` / `MoModifyFromInput` `ContentRA` points into the **clone's** `InputOS`, never the source's. -5. A round-trip test: split, save, reload, re-read the clone. This is the one that speaks to the reported "comes back wrong later" symptom, and should be written even if the in-memory assertions pass. - -## Scope - -Independent of Bugs 1 and 2. Those are FieldWorks UI defects in the rule formula editor; this is an LCM domain-layer defect in the clone path. The fix lands in `liblcm` and will need a package bump in FieldWorks to ship. - -## Key files - -| Path:line | Role | -|---|---| -| `liblcm/.../DomainImpl/OverridesLing_MoClasses.cs:4056-4068` | The broken `PostClone` | -| `liblcm/.../DomainImpl/OverridesLing_MoClasses.cs:4037-4048` | `SetDefaultValuesAfterInit` — the defaults that need stripping | -| `liblcm/.../DomainImpl/OverridesLing_Lex.cs:1652-1675` | `MoveSenseToCopy`, allomorph cloning | -| `liblcm/.../DomainServices/CopyObject.cs:108-128` | Batch clone; `PostClone` invoked once per top-level source | -| `liblcm/.../DomainServices/CopyObject.cs:301-373` | Pass 1, owned clone; `ICloneableCmObject` short-circuit | -| `liblcm/.../DomainServices/CopyObject.cs:203-238` | Pass 2, reference remapping | -| `liblcm/.../DomainImpl/OverridesLing_Lex.cs:7683`, `:8176` | `PhRegularRule` / `PhMetathesisRule` — the pattern `MoAffixProcess` lacks | -| `FieldWorks/Src/FdoUi/FdoUiCore.cs:2093-2106` | UI entry point | -| `FieldWorks/Src/xWorks/DTMenuHandler.cs:1052-1058` | Command handler | From 5f4f74a2304a92481d59c87c9364521a384e9c2d Mon Sep 17 00:00:00 2001 From: John Lambert Date: Fri, 21 Aug 2026 10:54:23 -0400 Subject: [PATCH 10/11] clean up affix process comments --- .../DomainImpl/OverridesLing_MoClasses.cs | 23 ++--- .../AffixProcessCloneRoundTripTests.cs | 17 +--- .../DomainImpl/LexEntryTests.cs | 88 +++---------------- 3 files changed, 19 insertions(+), 109 deletions(-) diff --git a/src/SIL.LCModel/DomainImpl/OverridesLing_MoClasses.cs b/src/SIL.LCModel/DomainImpl/OverridesLing_MoClasses.cs index ac3e0ed1..ded254d6 100644 --- a/src/SIL.LCModel/DomainImpl/OverridesLing_MoClasses.cs +++ b/src/SIL.LCModel/DomainImpl/OverridesLing_MoClasses.cs @@ -4048,34 +4048,21 @@ protected override void SetDefaultValuesAfterInit() } /// - /// Gives an object an opportunity to do any class-specific side-effect work when it has - /// been cloned with DomainServices.CopyObject. In this case, the creation of a MoAffixProcess - /// adds default initial values that are not wanted in the cloned copy, so PostClone() - /// removes them. + /// Removes initialization defaults from this affix process's clone. /// + /// The map from source object identifiers to their clones. public override void PostClone(Dictionary copyMap) { - // copyMap holds every object cloned in this CopyObject pass, not just this object's own - // clone (e.g. when several allomorphs of an entry are cloned together, it holds all of - // their clones and owned children). Look up only our own clone, and repair only it. + // The map can contain sibling clones; this source's identifier selects its own clone. if (!copyMap.TryGetValue(Hvo, out var clone) || !(clone is IMoAffixProcess clonedProcess)) return; - // The clone was created via the normal factory, which seeds exactly one default - // PhVariable input and one default MoCopyFromInput output (SetDefaultValuesAfterInit) - // before this object's own real content was cloned and appended after them. So the - // clone should end up with exactly as many inputs/outputs as THIS (the source) has -- - // not "count > 1", which can't distinguish a genuinely empty source (0 real inputs) from - // a single leaked default (both look like count 1). Remove exactly the surplus leading - // items, computed from the source's own counts, rather than guessing from the clone's - // shape. + // Factory-created clones contain leading defaults in addition to the source content. var surplusInputs = clonedProcess.InputOS.Count - InputOS.Count; for (var i = 0; i < surplusInputs; i++) clonedProcess.InputOS.RemoveAt(0); - // Recompute rather than assume: removing the default input(s) above can itself cascade, - // via RemoveObjectSideEffectsInternal below, into also removing the default output, so - // OutputOS's own surplus must be measured after the input removal, not derived from it. + // Removing a default input can also remove its referenced default output. var surplusOutputs = clonedProcess.OutputOS.Count - OutputOS.Count; for (var i = 0; i < surplusOutputs; i++) clonedProcess.OutputOS.RemoveAt(0); diff --git a/tests/SIL.LCModel.Tests/DomainImpl/AffixProcessCloneRoundTripTests.cs b/tests/SIL.LCModel.Tests/DomainImpl/AffixProcessCloneRoundTripTests.cs index 6202f53c..20ce1afa 100644 --- a/tests/SIL.LCModel.Tests/DomainImpl/AffixProcessCloneRoundTripTests.cs +++ b/tests/SIL.LCModel.Tests/DomainImpl/AffixProcessCloneRoundTripTests.cs @@ -12,12 +12,7 @@ namespace SIL.LCModel.DomainImpl { /// - /// Case 5 from doc/bugs/affix-process-split-sense-stale-clone.md: a save/reload round trip - /// after LexEntry.MoveSenseToCopy, to check whether the affix-process clone bug is only an - /// in-memory artifact (masked by a warm cache) or actually persists to (and is reproduced - /// from) disk -- which is what the user's reported "comes back wrong later" symptom requires. - /// This uses a real file-backed cache (kXMLWithMemoryOnlyWsMgr), unlike the in-memory-only - /// fixture used by LexEntryTests. + /// Verifies affix-process clones using a file-backed cache. /// [TestFixture] public class AffixProcessCloneRoundTripTests @@ -41,12 +36,7 @@ public void TestTeardown() } /// - /// Build an entry with a non-trivial affix-process LexemeFormOA and two senses, split one - /// sense off with MoveSenseToCopy, save to disk, close the cache, reopen it from disk, and - /// re-examine the cloned affix process. If PostClone's repair is only cosmetically correct - /// in memory (e.g. because a UI slice renders the leaked default identically to real - /// content until a reload forces a rebuild), this is the test that would catch it; if the - /// in-memory clone is already wrong, this test proves the corruption is not transient. + /// Verifies a moved affix-process clone retains its rule after saving and reloading. /// [Test] public void MoveSenseToCopy_AffixProcessClone_SurvivesSaveAndReload() @@ -77,8 +67,7 @@ public void MoveSenseToCopy_AffixProcessClone_SurvivesSaveAndReload() process.MorphTypeRA = cache.ServiceLocator.GetInstance() .GetObject(MoMorphTypeTags.kguidMorphSuffix); - // Non-trivial rule: real content the user would have entered, replacing the - // SetDefaultValuesAfterInit defaults. + // Distinct types make ordering and reference errors observable. process.InputOS.Clear(); process.OutputOS.Clear(); var ctxt = cache.ServiceLocator.GetInstance().Create(); diff --git a/tests/SIL.LCModel.Tests/DomainImpl/LexEntryTests.cs b/tests/SIL.LCModel.Tests/DomainImpl/LexEntryTests.cs index edefb1be..9b13cb43 100644 --- a/tests/SIL.LCModel.Tests/DomainImpl/LexEntryTests.cs +++ b/tests/SIL.LCModel.Tests/DomainImpl/LexEntryTests.cs @@ -476,10 +476,6 @@ private ILexEntry MakeAffixProcessEntry(string form, Guid morphType) return entry; } - /// - /// Adds an affix-process allomorph (with a real, non-trivial form) to an entry's - /// AlternateFormsOS, distinct from a bare LexemeFormOA process. - /// private IMoAffixProcess AddAffixProcessAllomorph(ILexEntry entry, string form) { var ws = Cache.DefaultVernWs; @@ -490,9 +486,6 @@ private IMoAffixProcess AddAffixProcessAllomorph(ILexEntry entry, string form) return process; } - /// - /// Adds a plain stem allomorph to an entry's AlternateFormsOS. - /// private IMoStemAllomorph AddStemAllomorph(ILexEntry entry, string form) { var ws = Cache.DefaultVernWs; @@ -504,13 +497,7 @@ private IMoStemAllomorph AddStemAllomorph(ILexEntry entry, string form) } /// - /// Replaces the trivial default InputOS/OutputOS that SetDefaultValuesAfterInit seeds - /// with a small but non-trivial rule: Input = [naturalClassContext, variable], - /// Output = [CopyFromInput(naturalClassContext), ModifyFromInput(variable)]. - /// The two InputOS entries are deliberately different classes (PhSimpleContextNC vs - /// PhVariable) so a leaked leading default PhVariable is easy to detect by ClassID, - /// and the two OutputOS entries are deliberately different classes (MoCopyFromInput vs - /// MoModifyFromInput) so a leaked leading default MoCopyFromInput is likewise detectable. + /// Replaces generated defaults with distinguishable rule content. /// private void MakeNonTrivialRuleContent(IMoAffixProcess process) { @@ -531,12 +518,7 @@ private void MakeNonTrivialRuleContent(IMoAffixProcess process) } /// - /// Asserts that a clone produced from a MakeNonTrivialRuleContent source has exactly the - /// right objects in exactly the right order -- not merely the right counts. A mutant that - /// removes the wrong list element (e.g. index 1 instead of index 0) can leave counts and - /// simple containment checks satisfied while corrupting the actual content; checking - /// ClassID at each position, and checking that each mapping's ContentRA is reference-equal - /// to the specific InputOS slot it is supposed to target, catches that. + /// Verifies that a rule clone preserves item order and mapping references. /// private void AssertNonTrivialRuleClonedCorrectly(IMoAffixProcess clone) { @@ -566,11 +548,7 @@ private void AssertNonTrivialRuleClonedCorrectly(IMoAffixProcess clone) } /// - /// Case 1 from doc/bugs/affix-process-split-sense-stale-clone.md: single affix-process - /// allomorph (as LexemeFormOA) carrying a non-trivial rule. Because this process's own - /// clone is the very first entry PostClone's copyMap sees (it is cloned before any of its - /// own owned Input/Output children), defect (A)'s "return instead of continue" does not - /// get triggered in this configuration -- see case 2 for that. + /// Verifies moving a sense preserves a non-trivial lexeme-form affix process. /// [Test] public void MoveSenseToCopy_AffixProcessClone_PreservesNonTrivialRule_SingleAllomorph() @@ -598,12 +576,7 @@ public void MoveSenseToCopy_AffixProcessClone_PreservesNonTrivialRule_SingleAllo } /// - /// Case 2 from doc/bugs/affix-process-split-sense-stale-clone.md: a stem allomorph ordered - /// BEFORE the affix-process allomorph in AlternateFormsOS. Both allomorphs are cloned in a - /// single CopyObject batch (one shared copyMap), so the stem's clone -- which is not an - /// IMoAffixProcess -- lands in the map ahead of the process's own clone. Defect (A)'s - /// "return" (instead of "continue") then bails out of the whole loop before ever reaching - /// the affix process, so its defaults are never stripped at all. + /// Verifies moving a sense preserves an affix process preceded by a stem allomorph. /// [Test] public void MoveSenseToCopy_AffixProcessClone_StemAllomorphBeforeProcess_PreservesRealContent() @@ -632,14 +605,7 @@ public void MoveSenseToCopy_AffixProcessClone_StemAllomorphBeforeProcess_Preserv } /// - /// Case 3 from doc/bugs/affix-process-split-sense-stale-clone.md: TWO affix-process - /// allomorphs cloned in the same batch. Defect (B): PostClone is invoked once per - /// top-level source object, but each invocation walks the ENTIRE shared copyMap from the - /// start rather than looking up only its own clone. The first allomorph's clone sits at - /// the front of the map, so every invocation re-strips index 0 from it -- the first - /// invocation correctly removes its leaked default, subsequent invocations incorrectly - /// remove real content -- while the second allomorph's own defaults are never reached - /// (the loop returns as soon as it hits the first allomorph's non-process owned child). + /// Verifies moving a sense preserves two affix-process allomorphs cloned together. /// [Test] public void MoveSenseToCopy_AffixProcessClone_TwoProcessAllomorphs_NeitherLosesRealContent() @@ -668,20 +634,12 @@ public void MoveSenseToCopy_AffixProcessClone_TwoProcessAllomorphs_NeitherLosesR Assert.That(clonedA, Is.Not.Null); Assert.That(clonedB, Is.Not.Null); - // Identity-at-position, not just counts: a mutant that removes the wrong list element - // (e.g. index 1 instead of index 0) can leave clonedA.InputOS.Count == sourceA.InputOS.Count - // while a leaked default PhVariable survives at index 0 and a real item is gone -- - // AssertNonTrivialRuleClonedCorrectly checks the actual ClassID/identity at each slot. AssertNonTrivialRuleClonedCorrectly(clonedA); AssertNonTrivialRuleClonedCorrectly(clonedB); } /// - /// Case 4 from doc/bugs/affix-process-split-sense-stale-clone.md: every cloned - /// MoCopyFromInput/MoModifyFromInput ContentRA must point into the CLONE's own InputOS, - /// never into the source's InputOS (nor -- since defect (B) can delete a clone's real - /// input out from under a surviving mapping -- into thin air). Uses the same two-process - /// setup as case 3, since that is where PostClone does the most damage. + /// Verifies cloned rule mappings refer to inputs owned by the same clone. /// [Test] public void MoveSenseToCopy_AffixProcessClone_ContentRAPointsIntoOwnClonesInputOS() @@ -707,8 +665,6 @@ public void MoveSenseToCopy_AffixProcessClone_ContentRAPointsIntoOwnClonesInputO var clonedA = (IMoAffixProcess)newEntry.AlternateFormsOS[0]; var clonedB = (IMoAffixProcess)newEntry.AlternateFormsOS[1]; - // Identity-at-position first: confirms each clone's own InputOS/OutputOS is exactly - // right (no leaked default swapped in for a real item at the same slot count). AssertNonTrivialRuleClonedCorrectly(clonedA); AssertNonTrivialRuleClonedCorrectly(clonedB); @@ -735,15 +691,7 @@ public void MoveSenseToCopy_AffixProcessClone_ContentRAPointsIntoOwnClonesInputO } /// - /// A third clone path, found in adversarial review, that neither the original bug report - /// nor the four tests above exercise: MoveSenseToCopy reaches MoAffixProcess a SECOND time - /// through CreateMatchingAllomorphInTargetEntry (OverridesLing_Lex.cs:1803), called from - /// UpdateReferencesForSenseMove (:1758-1786), whenever a WfiMorphBundle references the moved - /// sense's morph. IsMatchingAllomorph compares Form text across writing systems; an affix - /// process's Form is normally left blank (the model's own doc comment says Form is - /// undefined for process affixes), so it never matches the already-cloned LexemeFormOA, and - /// the failover clones the SOURCE process a second, independent time via its own - /// CopyObject<IMoForm> call, landing in AlternateFormsOS rather than LexemeFormOA. + /// Verifies a moved sense preserves the affix process used by its morph bundle. /// [Test] public void MoveSenseToCopy_AffixProcessClone_ViaMorphBundleFailoverPath_PreservesNonTrivialRule() @@ -756,9 +704,7 @@ public void MoveSenseToCopy_AffixProcessClone_ViaMorphBundleFailoverPath_Preserv entry = MakeEntry(); sourceProcess = Cache.ServiceLocator.GetInstance().Create(); entry.LexemeFormOA = sourceProcess; - // Deliberately leave Form blank: that is the normal state for a process affix, and - // it is exactly what makes IsMatchingAllomorph fail to match the already-cloned - // LexemeFormOA, forcing the CreateMatchingAllomorphInTargetEntry failover. + // A blank form exercises creation of a matching allomorph for the morph bundle. sourceProcess.MorphTypeRA = Cache.ServiceLocator.GetInstance() .GetObject(MoMorphTypeTags.kguidMorphSuffix); MakeNonTrivialRuleContent(sourceProcess); @@ -791,24 +737,14 @@ public void MoveSenseToCopy_AffixProcessClone_ViaMorphBundleFailoverPath_Preserv } finally { - // Pre-existing bug, reproducible identically before and after this fix: undoing the - // WfiMorphBundle.MorphRA reference changes this test's setup UOW recorded throws - // KeyNotFoundException out of LcmAtomicRefPropertyChanged.Undo() during - // TestTearDown's UndoAll(). Committing here -- which is exactly what UndoAll() itself - // does at its own end -- clears the undo stack first, so that unrelated crash can't - // happen and mask this test's own pass/fail. + // Commit because undoing the morph-bundle references throws from + // LcmAtomicRefPropertyChanged.Undo during teardown. Cache.ActionHandlerAccessor.Commit(); } } /// - /// Residual bug, found in adversarial review: an affix process whose InputOS/OutputOS are - /// legitimately empty (Clear(), no re-add -- legal at the LCM level; MoAffixProcess has no - /// IsFieldRequired guard forcing at least one of each) still ends up with a leaked default - /// PhVariable/MoCopyFromInput pair after MoveSenseToCopy. A "clonedProcess.Count > 1" guard - /// cannot tell "genuinely zero real content" (clone ends at 1: the seeded default) apart - /// from "one real item plus a leaked default" (also count 1 after a naive single strip) -- - /// both look like count 1. The fix compares against the SOURCE's own (zero) counts instead. + /// Verifies moving a sense preserves an affix process with empty rule content. /// [Test] public void MoveSenseToCopy_AffixProcessClone_ZeroRealContent_NoLeakedDefault() @@ -820,8 +756,6 @@ public void MoveSenseToCopy_AffixProcessClone_ZeroRealContent_NoLeakedDefault() { entry = MakeAffixProcessEntry("ed", MoMorphTypeTags.kguidMorphSuffix); sourceProcess = (IMoAffixProcess)entry.LexemeFormOA; - // Legitimately empty: Clear() with no re-add. Nothing at the LCM level requires a - // process affix to have any Input/Output content. sourceProcess.InputOS.Clear(); sourceProcess.OutputOS.Clear(); MakeSense(entry, "stay"); From 0dc1b7a86a6e95b6e5ded2f4f6ed311406178598 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Fri, 21 Aug 2026 11:21:38 -0400 Subject: [PATCH 11/11] verify cloned mappings after reload --- .../DomainImpl/AffixProcessCloneRoundTripTests.cs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/SIL.LCModel.Tests/DomainImpl/AffixProcessCloneRoundTripTests.cs b/tests/SIL.LCModel.Tests/DomainImpl/AffixProcessCloneRoundTripTests.cs index 20ce1afa..44892b44 100644 --- a/tests/SIL.LCModel.Tests/DomainImpl/AffixProcessCloneRoundTripTests.cs +++ b/tests/SIL.LCModel.Tests/DomainImpl/AffixProcessCloneRoundTripTests.cs @@ -114,6 +114,12 @@ public void MoveSenseToCopy_AffixProcessClone_SurvivesSaveAndReload() Assert.That(clonedProcess.InputOS[1].ClassID, Is.EqualTo(PhVariableTags.kClassId)); Assert.That(clonedProcess.OutputOS[0].ClassID, Is.EqualTo(MoCopyFromInputTags.kClassId)); Assert.That(clonedProcess.OutputOS[1].ClassID, Is.EqualTo(MoModifyFromInputTags.kClassId)); + var copy = (IMoCopyFromInput)clonedProcess.OutputOS[0]; + var modify = (IMoModifyFromInput)clonedProcess.OutputOS[1]; + Assert.That(copy.ContentRA, Is.SameAs(clonedProcess.InputOS[0]), + "reloaded copy mapping should reference the cloned natural-class input"); + Assert.That(modify.ContentRA, Is.SameAs(clonedProcess.InputOS[1]), + "reloaded modify mapping should reference the cloned variable input"); } } }