Fix bug in MergeEquivalentAnalyses

[jtmaxwell3]

Comparing MergeEquivalentAnalyses to John Lambert's memoization code, I noticed that it was not computing equivalence correctly. So I borrowed AnalysisStateKey from the memoization code to fix the problem.

---

This change is 

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.51%. Comparing base (d734722) to head (4562502).

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #493   +/-   ##
=======================================
  Coverage   73.51%   73.51%           
=======================================
  Files         451      451           
  Lines       37692    37692           
  Branches     5183     5183           
=======================================
  Hits        27708    27708           
  Misses       8845     8845           
  Partials     1139     1139           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ddaspit]

@ddaspit reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on jtmaxwell3).

---

src/SIL.Machine.Morphology.HermitCrab/AnalysisStratumRule.cs line 155 at r1 (raw file):

                if (mergeEquivalentAnalyses)
                {
                    var key = AnalysisStateKey.PinAndKey(mruleOutWord);

wordCache and output don't agree on identity. Word.ValueEquals ignores SyntacticFeatureStruct, which the key includes. So analyses differing only in SyntacticFeatureStruct now get distinct keys, skip the merge, and are then dropped by output.Add.

Also, wordCache[key] = mruleOutWord runs before output.Add accepts it, so a rejected word stays canonical and swallows every later word with that key.

[ddaspit]

@ddaspit made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on jtmaxwell3).

---

src/SIL.Machine.Morphology.HermitCrab/AnalysisStratumRule.cs line 155 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

wordCache and output don't agree on identity. Word.ValueEquals ignores SyntacticFeatureStruct, which the key includes. So analyses differing only in SyntacticFeatureStruct now get distinct keys, skip the merge, and are then dropped by output.Add.

Also, wordCache[key] = mruleOutWord runs before output.Add accepts it, so a rejected word stays canonical and swallows every later word with that key.

You could do something like this:

var output = new Dictionary<Word, Word>(FreezableEqualityComparer<Word>.Default) { [input] = input };
...
if (mergeEquivalentAnalyses)
{
    AnalysisStateKey key = AnalysisStateKey.PinAndKey(mruleOutWord);
    if (wordCache.TryGetValue(key, out Word canonicalWord))
    {
        canonicalWord.Alternatives.Add(mruleOutWord);
        continue;
    }
    // Distinct key, but Word.ValueEquals ignores SyntacticFeatureStruct, so the output
    // may already hold an equal word. Keep this analysis as that word's alternative
    // rather than letting the output silently drop it.
    if (output.TryGetValue(mruleOutWord, out Word equivalentWord))
    {
        equivalentWord.Alternatives.Add(mruleOutWord);
        continue;
    }
    // Registered only once the word is certain to reach the output: a canonical that
    // never gets returned would swallow every later word with the same key.
    wordCache[key] = mruleOutWord;
}
if (!output.ContainsKey(mruleOutWord))
    output.Add(mruleOutWord, mruleOutWord);
...
return output.Keys;

[jtmaxwell3]

That fixes the AI’s complaint, but it doesn’t fix the bug in Word.ValueEquals. If there are two words that are identical except for SyntacticFeatureStruct, and one of them would succeed but the other would fail, then storing the one that would succeed as an alternative to the one that would fail will cause both of them to fail and so we would lose a valid analysis. On Sep 9, 2026, at 6:24 AM, Damien Daspit ***@***.***> wrote: @ddaspit commented on this pull request. @ddaspit <https://github.com/ddaspit> made 1 comment. Reviewable <https://reviewable.io/reviews/sillsdev/machine/493> status: all files reviewed, 1 unresolved discussion (waiting on jtmaxwell3). src/SIL.Machine.Morphology.HermitCrab/AnalysisStratumRule.cs line 155 at r1 <https://reviewable.io/reviews/sillsdev/machine/493#-P0i-GfNEhaUy1NPaMqq:-P15JwgK0RecAyvJQI4I:b3b7lgv> (raw file <https://github.com/sillsdev/machine/blob/4562502b8b18d147c1d7610ce3fd4352cfcf51ca/src/SIL.Machine.Morphology.HermitCrab/AnalysisStratumRule.cs#L155>): Previously, ddaspit (Damien Daspit) wrote… You could do something like this: var output = new Dictionary<Word, Word>(FreezableEqualityComparer<Word>.Default) { [input] = input }; ... if (mergeEquivalentAnalyses) { AnalysisStateKey key = AnalysisStateKey.PinAndKey(mruleOutWord); if (wordCache.TryGetValue(key, out Word canonicalWord)) { canonicalWord.Alternatives.Add(mruleOutWord); continue; } // Distinct key, but Word.ValueEquals ignores SyntacticFeatureStruct, so the output // may already hold an equal word. Keep this analysis as that word's alternative // rather than letting the output silently drop it. if (output.TryGetValue(mruleOutWord, out Word equivalentWord)) { equivalentWord.Alternatives.Add(mruleOutWord); continue; } // Registered only once the word is certain to reach the output: a canonical that // never gets returned would swallow every later word with the same key. wordCache[key] = mruleOutWord; } if (!output.ContainsKey(mruleOutWord)) output.Add(mruleOutWord, mruleOutWord); ... return output.Keys; — Reply to this email directly, view it on GitHub <#493?email_source=notifications&email_token=ABVKIQMID77K4RGMLTQFWU35OFKYNA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMJVGQ4TGMZQGA3KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#pullrequestreview-5154933006>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABVKIQIWYQ4PKDN7ABBYUTL5OFKYNAVCNFSNUABDKJSXA33TNF2G64TZHM3TONBVG44DCO2JONZXKZJ3GUZTGNZZHAYTIMJYUF3AE>. Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS <https://github.com/notifications/mobile/ios/ABVKIQID5Z3VNYTAYVSCZ6D5OFKYNA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMJVGQ4TGMZQGA3KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KUZTPN52GK4S7NFXXG> and Android <https://github.com/notifications/mobile/android/ABVKIQIAZQ6GO6YTJ5XGX6L5OFKYNA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMJVGQ4TGMZQGA3KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>. Download it today! You are receiving this because you authored the thread.

[johnml1135]

Heads-up from #494. Your suggested fix parks an analysis with different features behind one that stays active, and the parser keeps working on the active one only. If the active one has stricter features, the parked one's valid parses get filtered out and lost. That is exactly what happens under PriorityUnion in #494. The fix there is one line: when merging, loosen the active analysis's features to cover both. Whichever PR lands first should carry it.

(Claude helped).

[ddaspit]

It is less than ideal that Word.ValueEquals and AnalysisStateKey disagree, but I would prefer to leave Word.ValueEquals alone. Changing it will have larger downstream effects that I would rather not deal with. We should just make the local change to deal with the discrepancy in this case.

@ddaspit made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on jtmaxwell3).

[jtmaxwell3]

@ddaspit: which local change do you have in mind? My change, which can drop solutions in output.Add, your change, which can lose valid solutions if the equivalent word fails but the alternative would succeed, or the change that John proposed in 3be1eb9?

[jtmaxwell3]

BTW, I still think that there is a bug in output.Add that is independent of MergeEquivalentAnalyses: if there are two words that are equivalent except for SyntacticFeatureStruct then one of them will be dropped. I think that this hasn't been a problem so far because SyntacticFeatureStruct is mostly determined by the morphemes (including the morphemes that are in templates). But if there were an inflection morpheme that could be in two different templates with different feature structures, then you could get two different words that are equivalent except for SyntaticFeatureStruct.

[ddaspit]

@jtmaxwell3 This bug potentially changes things a bit, since it impacts the question of whether SyntacticFeatureStruct should be compared in Word.ValueEquals. This is what I would suggest:

1. Fix the bug in output.Add by including SyntacticFeatureStruct in Word.ValueEquals. This is a larger change, but it should fix incorrectly dropped analyses and make this PR simpler.
2. Merge this PR.
3. Merge PR #494.

@ddaspit made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on jtmaxwell3).