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. ///