From 60619122ede82283c3436b9e6b13b285c7fbeda6 Mon Sep 17 00:00:00 2001 From: Jason Naylor Date: Tue, 7 Jul 2026 08:18:58 -0700 Subject: [PATCH] Fix LT-22575: Deleting a duplicated phonological rule deletes shared objects Duplicating a rule (LT-21904) gives the copy references to the original's pooled PhFeatureConstraints and environment member contexts; PhPhonData owns both, rules only reference them. The deletion side effects removed those pooled objects unconditionally, assuming a single referring rule, so deleting either copy destroyed constraints and environment members the other still used (and could null the survivor's RHS contexts entirely). Guard the four GC sites with incoming-reference checks: a pooled object is removed from its pool only when the dying rule or context is its last referrer. GC still completes when the last referrer is deleted. Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 1 + .../DomainImpl/OverridesLing_Lex.cs | 21 ++- .../SIL.LCModel.Tests/DomainImpl/LingTests.cs | 143 ++++++++++++++++++ 3 files changed, 159 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f8b4bb61..d0850fb5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Fixed +- [SIL.LCModel] Deleting a duplicated phonological rule no longer deletes pooled feature constraints and environment contexts the original rule still references (LT-22575) - [SIL.LCModel] Fixed crash when bulk deleting entries involved in a lexical relation (LT-21598) - [SIL.LCModel.FixData] Find and Fix removes duplicate Targets from a LexReference (and deletes it if fewer than two distinct Targets remain) (LT-21598) - [SIL.LCModel] Data migration now serializes dates using the culture-neutral `ToLCMTimeFormatWithMillisString` (LT-20698) diff --git a/src/SIL.LCModel/DomainImpl/OverridesLing_Lex.cs b/src/SIL.LCModel/DomainImpl/OverridesLing_Lex.cs index 7ef2899f..1a57a9e7 100644 --- a/src/SIL.LCModel/DomainImpl/OverridesLing_Lex.cs +++ b/src/SIL.LCModel/DomainImpl/OverridesLing_Lex.cs @@ -7635,7 +7635,11 @@ protected override void OnBeforeObjectDeleted() { base.OnBeforeObjectDeleted(); foreach (var constr in FeatureConstraints) - m_cache.LanguageProject.PhonologicalDataOA.FeatConstraintsOS.Remove(constr); + { + // GC the pooled constraint unless a duplicate rule or a process still references it (LT-22575). + if (constr.ReferringObjects.All(o => o is IPhSimpleContextNC ctxt && ctxt.Rule == this)) + m_cache.LanguageProject.PhonologicalDataOA.FeatConstraintsOS.Remove(constr); + } } protected override void RemoveObjectSideEffectsInternal(RemoveObjectEventArgs e) @@ -7646,10 +7650,10 @@ protected override void RemoveObjectSideEffectsInternal(RemoveObjectEventArgs e) var removedCtxt = e.ObjectRemoved as IPhSimpleContextNC; if (removedCtxt != null) { - var featConstrs = GetFeatureConstraintsExcept(removedCtxt); foreach (var constr in removedCtxt.PlusConstrRS.ToList()) { - if (!featConstrs.Contains(constr)) + // GC the pooled constraint only if removedCtxt is its last referrer (LT-22575). + if (constr.ReferringObjects.All(o => o == removedCtxt)) { // Remove constr from removedCtxt first to avoid deleting removedCtxt twice (fixes LT-21729). removedCtxt.PlusConstrRS.Remove(constr); @@ -7658,7 +7662,7 @@ protected override void RemoveObjectSideEffectsInternal(RemoveObjectEventArgs e) } foreach (var constr in removedCtxt.MinusConstrRS.ToList()) { - if (!featConstrs.Contains(constr)) + if (constr.ReferringObjects.All(o => o == removedCtxt)) { // Remove constr from removedCtxt first to avoid deleting removedCtxt twice. removedCtxt.MinusConstrRS.Remove(constr); @@ -8201,7 +8205,8 @@ protected override void SetDefaultValuesAfterInit() protected override void OnBeforeObjectDeleted() { base.OnBeforeObjectDeleted(); - if (MemberRA != null) + // GC the pooled member only if this context is its last referrer (LT-22575). + if (MemberRA != null && MemberRA.ReferringObjects.All(o => o == this)) m_cache.LanguageProject.PhonologicalDataOA.ContextsOS.Remove(MemberRA); } } @@ -8212,7 +8217,11 @@ protected override void OnBeforeObjectDeleted() { base.OnBeforeObjectDeleted(); foreach (var ctxt in MembersRS.ToArray()) - m_cache.LanguageProject.PhonologicalDataOA.ContextsOS.Remove(ctxt); + { + // GC the pooled member only if this sequence is its last referrer (LT-22575). + if (ctxt.ReferringObjects.All(o => o == this)) + m_cache.LanguageProject.PhonologicalDataOA.ContextsOS.Remove(ctxt); + } } protected override void RemoveObjectSideEffectsInternal(RemoveObjectEventArgs e) diff --git a/tests/SIL.LCModel.Tests/DomainImpl/LingTests.cs b/tests/SIL.LCModel.Tests/DomainImpl/LingTests.cs index 94964aaf..c5a08872 100644 --- a/tests/SIL.LCModel.Tests/DomainImpl/LingTests.cs +++ b/tests/SIL.LCModel.Tests/DomainImpl/LingTests.cs @@ -147,6 +147,149 @@ public void MoAffixProcessFeatureConstraints() Assert.That(featureConstrs, Has.Count.EqualTo(3)); } + /// + /// LT-22575: feature constraints are owned by the PhPhonData.FeatConstraintsOS pool and only + /// referenced by rule contexts, so CopyObject hands a duplicate references to the original's + /// constraints, not copies. The rule's delete-time GC must skip constraints with outside referrers. + /// + [Test] + public void DuplicateRegularRule_DeletingCloneDoesNotDeleteOriginalsFeatureConstraints() + { + var phData = Cache.LangProject.PhonologicalDataOA; + + // Build a regular rule whose structural description contains a natural-class context that + // uses an alpha variable (a PhFeatureConstraint). + var rule = Cache.ServiceLocator.GetInstance().Create(); + phData.PhonRulesOS.Add(rule); + var ctxt = Cache.ServiceLocator.GetInstance().Create(); + rule.StrucDescOS.Add(ctxt); + var featConstr = Cache.ServiceLocator.GetInstance().Create(); + phData.FeatConstraintsOS.Add(featConstr); + ctxt.PlusConstrRS.Add(featConstr); + + Assert.That(rule.FeatureConstraints.Count(), Is.EqualTo(1), "original rule should have one feature constraint"); + Assert.That(phData.FeatConstraintsOS.Count, Is.EqualTo(1)); + + // Duplicate the rule the same way the UI does (this drives PhRegularRule.SetCloneProperties). + var clone = (IPhRegularRule)CopyObject.CloneLcmObject(rule, x => phData.PhonRulesOS.Add(x)); + Assert.That(phData.PhonRulesOS.Count, Is.EqualTo(2), "clone should have been created"); + + // Delete the clone. + Cache.DomainDataByFlid.DeleteObj(clone.Hvo); + + // The original rule must be completely unaffected by deleting its clone. + Assert.That(rule.IsValidObject, Is.True, "original rule should still exist"); + Assert.That(rule.StrucDescOS.Count, Is.EqualTo(1), "original rule should still have its context"); + Assert.That(rule.FeatureConstraints.Count(), Is.EqualTo(1), + "deleting the cloned rule must not remove the original rule's feature constraint"); + Assert.That(rule.FeatureConstraints.First().IsValidObject, Is.True, + "the original rule's feature constraint must still be a valid object"); + } + + /// + /// LT-22575: one alpha variable referenced from both a StrucDesc (plus) and an RHS StrucChange + /// (minus) context, shared with the duplicate rather than copied. Deletes the original first to + /// prove survival follows remaining referrers, then checks the pool empties (GC still completes). + /// + [Test] + public void DuplicateRegularRule_SharedConstraintSurvivesUntilLastRuleDeleted() + { + var phData = Cache.LangProject.PhonologicalDataOA; + + var rule = Cache.ServiceLocator.GetInstance().Create(); + phData.PhonRulesOS.Add(rule); + + // One alpha variable used in two owned contexts: StrucDesc (plus) and RHS StrucChange (minus). + var constr = Cache.ServiceLocator.GetInstance().Create(); + phData.FeatConstraintsOS.Add(constr); + var descCtxt = Cache.ServiceLocator.GetInstance().Create(); + rule.StrucDescOS.Add(descCtxt); + descCtxt.PlusConstrRS.Add(constr); + var changeCtxt = Cache.ServiceLocator.GetInstance().Create(); + rule.RightHandSidesOS[0].StrucChangeOS.Add(changeCtxt); + changeCtxt.MinusConstrRS.Add(constr); + + var clone = (IPhRegularRule)CopyObject.CloneLcmObject(rule, x => phData.PhonRulesOS.Add(x)); + + // The clone references the same pooled constraint; no copies are made. + Assert.That(clone.FeatureConstraints.Single(), Is.EqualTo(constr)); + Assert.That(phData.FeatConstraintsOS.Count, Is.EqualTo(1)); + + // Deleting the original must leave the shared constraint for the clone. + Cache.DomainDataByFlid.DeleteObj(rule.Hvo); + Assert.That(constr.IsValidObject, Is.True, "constraint is still referenced by the clone"); + Assert.That(clone.FeatureConstraints.Single(), Is.EqualTo(constr)); + + // Deleting the last rule that uses it removes it from the pool. + Cache.DomainDataByFlid.DeleteObj(clone.Hvo); + Assert.That(phData.FeatConstraintsOS.Count, Is.EqualTo(0), "last referrer gone, constraint should be GCed"); + } + + /// + /// LT-22575: covers the edit path (RemoveObjectSideEffectsInternal on StrucDesc removal), which + /// previously GCed a constraint unused elsewhere in the same rule even when a duplicate used it. + /// The last-referrer check must span all rules, and still GC once no referrer remains. + /// + [Test] + public void DuplicateRegularRule_RemovingContextKeepsConstraintUsedByClone() + { + var phData = Cache.LangProject.PhonologicalDataOA; + + var rule = Cache.ServiceLocator.GetInstance().Create(); + phData.PhonRulesOS.Add(rule); + var ctxt = Cache.ServiceLocator.GetInstance().Create(); + rule.StrucDescOS.Add(ctxt); + var constr = Cache.ServiceLocator.GetInstance().Create(); + phData.FeatConstraintsOS.Add(constr); + ctxt.PlusConstrRS.Add(constr); + + var clone = (IPhRegularRule)CopyObject.CloneLcmObject(rule, x => phData.PhonRulesOS.Add(x)); + + // Editing the original: removing its context must not GC the constraint the clone still uses. + rule.StrucDescOS.Remove(ctxt); + Assert.That(constr.IsValidObject, Is.True, "constraint is still referenced by the clone"); + Assert.That(clone.FeatureConstraints.Single(), Is.EqualTo(constr)); + + // Removing the clone's context too leaves no referrers, so the constraint is GCed. + clone.StrucDescOS.Remove(clone.StrucDescOS[0]); + Assert.That(phData.FeatConstraintsOS.Count, Is.EqualTo(0)); + } + + /// + /// LT-22575: sequence-environment members live in PhPhonData.ContextsOS and are held only by + /// reference (MembersRS), so both rules share them. Unguarded GC deleted them, emptying the + /// original's sequence, which then nulled the RHS LeftContextOA — silently wiping the environment. + /// + [Test] + public void DuplicateRegularRule_DeletingCloneKeepsSharedEnvironmentMembers() + { + var phData = Cache.LangProject.PhonologicalDataOA; + + var rule = Cache.ServiceLocator.GetInstance().Create(); + phData.PhonRulesOS.Add(rule); + var rhs = rule.RightHandSidesOS[0]; + var seqCtxt = Cache.ServiceLocator.GetInstance().Create(); + rhs.LeftContextOA = seqCtxt; + var member1 = Cache.ServiceLocator.GetInstance().Create(); + phData.ContextsOS.Add(member1); + seqCtxt.MembersRS.Add(member1); + var member2 = Cache.ServiceLocator.GetInstance().Create(); + phData.ContextsOS.Add(member2); + seqCtxt.MembersRS.Add(member2); + + var clone = (IPhRegularRule)CopyObject.CloneLcmObject(rule, x => phData.PhonRulesOS.Add(x)); + + // Deleting the duplicate must leave the original's environment intact. + Cache.DomainDataByFlid.DeleteObj(clone.Hvo); + Assert.That(rhs.LeftContextOA, Is.EqualTo(seqCtxt), "original environment should survive"); + Assert.That(seqCtxt.MembersRS.Count, Is.EqualTo(2)); + Assert.That(phData.ContextsOS.Count, Is.EqualTo(2)); + + // Deleting the original GCs the pooled members. + Cache.DomainDataByFlid.DeleteObj(rule.Hvo); + Assert.That(phData.ContextsOS.Count, Is.EqualTo(0)); + } + /// /// Trivial test that a newly created InflAffixTemplate has the Final property set to true. ///