Match lift-ranges range-element on guid rather than id

[imnasnainaec]

Quick Summary

• Chorus matches range-element across revisions by its @id, comparing ordinally and
  normalizing nothing.
• But that id is the possibility's own name. It moves whenever the name is renamed,
  recapitalized, or respelled. Chorus then sees not "an attribute was edited" but "one
  element deleted, a different element added."
• What that costs, worst first:
  1. If the other user edited the same range-element in the same sync, DoDeletions
     raises a RemovedVsEditedElementConflict — a conflict note the users did nothing to
     earn.
  2. The merge output then carries two <range-element> with the same @guid, ids
     differing only in the respelling, because our element is an unmatched addition while
     theirs was kept by the conflict.
  3. Whichever side loses, the losing element's <label>, <abbrev>, <description> and
     traits travel with it rather than merging.
• This PR matches on the guid FLEx writes on every range-element, and on the id only
  among the elements that name no guid, since LIFT makes the guid optional.

What prompted it

FieldWorks sillsdev/FieldWorks#1063
(LT-22697) fixes a LIFT export defect: several .lift-ranges writes emitted FLEx's
in-memory NFD strings while the rest of the export, and the companion .lift, are NFC.
The fix normalizes the range-element ids, so every affected id is respelled once — and,
because the id is derived from the name at export time rather than stored, respelled on
every export by any client that has not yet upgraded. The exporter fix therefore lands on
exactly the weakness above, repeatedly, for as long as a project has a mixed set of FLEx
versions.

Nothing here is specific to normalization. An ordinary rename of a part of speech has always
had the same shape and is fixed by the same change.

The change

FindByPreferredKeyAttribute

A new finder beside the others in FindNodeToMerge.cs, constructed with a preferred key and a
fallback key.

• Matching rule. Elements naming a guid and elements lacking one are matched separately and
  never against each other. Two elements naming a guid are partners when the guid agrees,
  whatever their ids say; two naming none are partners when the id agrees, which is what this
  handler did before. Element names must agree too, matching the (name, key) tuple
  FindByKeyAttribute already uses. Unlike that single-key finder, a duplicate id is kept
  rather than treated as a programming error, since two elements may legitimately share an id
  and still be told apart by their guid; the first is indexed, matching how the merger resolves
  siblings it cannot distinguish.
• Why the two sets stay apart. If a guid-bearing element could match a bare one on the id,
  a parent holding both could give a single element two partners — Chorus would merge one
  incoming edit into two siblings, raising a conflict nobody caused and, where our side had not
  also changed, quietly applying someone's edit to a different object. Keeping them apart makes
  matching an equivalence relation, so nothing can be paired twice.
• What the stricter rule costs. Where one revision names a guid for an element and another
  does not:
  • The element reads as a deletion plus an addition, and as a RemovedVsEditedElementConflict
    if the other revision also edited it — the shape this PR removes for respelling, reached
    instead by dropping a guid.
  • A writer that starts naming guids pays that once, as projects will as their clients upgrade.
  • Writers that persistently disagree over whether to write it pay it on every merge between
    them. FLEx writes a guid on every range-element, so the fallback is for files whose writers
    omit it consistently, not for a mixture.
• Identity is decided by index, not by position. GetNodeToMerge resolves through the guid
  index, so a sibling that merely still carries the old id cannot win by appearing earlier in
  the file.
• Indexed per parent. Each parent's children are indexed once — by guid, and by id among the
  children that name no guid — rather than rescanned for every child, the same reason
  FindByKeyAttribute carries an index: a semantic-domain range runs to a couple of thousand
  elements and is walked for ours, theirs, ancestor and the ambiguity pass. An index is built
  when its parent is first searched and never revisited, which the callers here satisfy and
  which the code now says out loud.
• Implements IFindMatchingNodesToMerge, not just IFindNodeToMerge, so
  XmlMergeService.RemoveAmbiguousChildren still applies to range-elements. That also means a
  ranges file already carrying the duplicated pair, from a merge done before this change,
  has the two collapsed back into one — with a merge warning — the next time it is merged.
• One quiet improvement on the ambiguity path. FindByKeyAttribute.GetMatchingNodes
  compares only the key attribute, so differently-named siblings sharing an id could be judged
  ambiguous and one of them dropped. This finder requires the element names to agree, which also
  aligns GetMatchingNodes with the tuple GetNodeToMerge resolves on.
• The ambiguity warning names one key, the one that matched. Whether an element names a guid
  decides which key it was matched on, so that key alone is what its ambiguous siblings share.
  Naming both would accuse a key the match never consulted, and which for two elements sharing a
  guid may well differ between them. The text follows the phrasing FindByKeyAttribute already
  uses: The key attribute 'guid' has values that are the same '…'.
• Its documentation says preferred key and fallback key, not guid. Per review feedback. The
  finder takes any two attribute names, so naming the guid in its summary put the LIFT case into
  merge/xml/generic; calling that key permanent claimed something the class neither enforces nor
  checks; and both keys are read with GetOptionalAttributeString, so calling only one of them
  optional invented an asymmetry that is not there. The wiring comment and the tests keep the
  guid/id language, which is where the concrete keys belong. The summary also no longer says
  "falling back", which had outlived the separate-sets rule above: an element that names a guid and
  finds no partner is not retried against the id.

IFindNodeToMerge

The summary promised that a non-null result is a value in acceptableTargets — a contract no
indexed implementation has ever honored, since FindByKeyAttribute and the new finder both
answer from their index without consulting the set. It now states what implementations do
guarantee, a child of parentToSearchIn or null, and puts the filtering obligation on callers
that pass a strict subset, which is what MergeChildrenMethod.FindMatchingNode has always done.

Wiring

range-element is switched to FindByPreferredKeyAttribute("guid", "id"). Note that
AddLiftRangeElementStrategies is registered by both the ranges handler and the main LIFT
handler, so this also governs any range-element appearing inside a .lift header's
<ranges>. That is intended — the identity argument is the same either way — but it is a wider
surface than the .lift-ranges file alone, and only the .lift-ranges path has test coverage
here.

Deliberately not in this PR

• <range> itself. Its id is a stable identifier (grammatical-info, morph-type)
  rather than user data, and XmlMergeService.Do3WayMerge splits the file into records by
  range/@id outside the ElementStrategy system. Changing the element strategy alone would put
  the two out of step.
• <trait>. Keyed on name+value, so a respelled value is a delete plus an add there
  too, including the feature-set trait whose value is a composed feature-structure name.
  • Nothing to prefer instead — @id is optional on a trait and FLEx does not write it.
  • Smaller damage anyway — a trait is a leaf that both sides regenerate.
• The other OptionalKeyAttrFinder call sites. Same prefer-then-fall-back shape, and the
  obvious thing to reuse, but it implements only IFindNodeToMerge:
  • Adopting it here would have silently switched ambiguity detection off for range-elements.
  • Making it implement IFindMatchingNodesToMerge would change behavior for note, field,
    relation, etymology and example in the LIFT handler — a wider change than this defect
    justifies, and worth doing on its own.
• Filtering acceptableTargets inside the finders. The documentation is brought in line
  with the implementations rather than the reverse; making every finder honor the set would
  change matching for callers that pass a strict subset, which none of the current ones do.
• Warning when a merged <range> ends up with a repeated range-element/@id. Defensible,
  but it is a new diagnostic needing a post-merge pass over the file rather than anything the
  finder can see, and the trade-off below covers what such a file costs its consumers.

One behavior trade-off, for reviewers to weigh

Two range-elements that share an id but hold different guids are now kept apart, where before
they were merged into one and a guid was lost; RemoveAmbiguousChildren no longer treats such a
pair as ambiguous either. They are two objects, so separate is right — but a merged ranges file
can now carry a duplicate range-element/@id, which is what <grammatical-info value="…"/> and
traits are resolved against. What its two consumers do with such a file:

• The post-merge sort. LiftSorter.SortRange keys its sorted collection through
  GetUniqueKey, which appends a numeric suffix rather than throwing on a repeated id. Run
  against the SIL.Lift 18.0.0-beta0032 this repo resolves, a ranges file holding two same-id
  elements sorts cleanly and keeps both.
• FLEx's LIFT import (read from sillsdev/FieldWorks@main):
  • Cannot fail on a duplicate id. Every id-keyed map is written with the indexer
    (m_dictPos[id] = pos in ProcessPartOfSpeech) or guarded by ContainsKey
    (AddToPossibilityMap); nothing calls Add on a bare range-element id.
  • Resolves each element on its own — guid (GetPossibilityForGuidIfExisting), then name and
    abbreviation (FindMatchingPossibility), then created. Two same-id elements whose guids the
    project does not know collapse into one possibility if their labels and abbrevs agree, and
    stand as two if they differ: the same dedupe-by-label the merge used to do, moved inside the
    project where a user can see and fix it.
  • Resolves trait and grammatical-info values through the id-keyed m_dictPos
    (FindOrCreatePartOfSpeech), which for a duplicated id holds the element processed last, so
    such a reference attaches to one of the two deterministically.

Older FLEx versions and WeSay's own ILexiconMerger implementation have not been checked. The
cost is bounded either way: a duplicate id is only reachable where two possibilities genuinely
share a name, and it costs a name-based reference landing on one of them, where the old behavior
cost an object its identity outright. RangeElementsWithDifferentGuidsAreNotMatched pins the new
shape so it can be argued about rather than discovered.

Testing

Six tests in LiftRangesFileHandlerTests:

1. RespelledIdMergesAsAnEditWhenTheGuidIsUnchanged — the LT-22697 scenario. We respell the id;
   they edit the abbreviation without upgrading. Asserts one element, our normalized id, and
   their edit surviving, with no conflict.
2. GuidMatchIsPreferredOverAnEarlierSiblingMatchingOnTheOldId — a guid-less sibling carrying
   the old id sits ahead of the respelled element. Asserts their edit lands on the element
   sharing the guid, the guid-less sibling keeps its own content, and no conflict is raised.
3. RangeElementWithoutAGuidStillMergesOnItsId — the fallback, for writers that omit guids.
4. DroppingTheGuidReadsAsADeletionPlusAnAddition — the cost above, where a writer omits the
   guid the ancestor names and the other side edits the element.
5. RangeElementsWithDifferentGuidsAreNotMatched — the trade-off above.
6. RangeElementsDuplicatedByAnEarlierMergeCollapseToOne — the recovery path for repos already
   carrying the duplicate, asserting also that the warning names the shared guid rather than
   blaming the ids, which differ.

The two Unicode spellings are built with Normalize(NormalizationForm.FormD/FormC) rather than
written as literals, since the forms are indistinguishable in source and an editor that
normalizes on save would quietly turn the test into a tautology.

dotnet test src/LibChorusTests/LibChorus.Tests.csproj -f net8.0 \
  --filter "FullyQualifiedName~merge.xml|FullyQualifiedName~FileHandlers"

this branch   541 passed, 13 failed
master        535 passed, 13 failed

The same 13 fail either way — all Mercurial location has not been configured in this
environment, none on a code path this touches. net462 and the full suite have not been run
locally; leaving this as a draft for CI.

---

This change is

[github-actions]

Test Results

       8 files  ±  0     334 suites  ±0   2h 27m 7s ⏱️ -57s
1 008 tests +  7     952 ✔️ +  7    56 💤 ±0  0 ❌ ±0 
3 197 runs  +21  3 074 ✔️ +21  123 💤 ±0  0 ❌ ±0 

Results for commit d3cf429. ± Comparison against base commit 17d3604.

♻️ This comment has been updated with latest results.

[jasonleenaylor]

src/LibChorus/merge/xml/generic/FindNodeToMerge.cs line 216 at r1 (raw file):

	/// <summary>
	/// Search for a matching element using an optional attribute that identifies it permanently (a guid),
	/// falling back to an ordinary key attribute among the elements that name no such attribute.

Revisit this comment with preferred key and fallback key language instead of permanently and ordinary.

[jasonleenaylor]

@jasonleenaylor reviewed 1 file and made 2 comments.
Reviewable status: 1 of 4 files reviewed, all discussions resolved.

---

src/LibChorus/merge/xml/generic/FindNodeToMerge.cs line 309 at r4 (raw file):

		/// Unlike a finder with a single key attribute, two children legitimately hold the same
		/// fallback value here, since a preferred value can still tell them apart. Keep the
		/// first, matching how the merger resolves siblings it cannot tell apart.

This comment could use a bit more work. It mentions fallback, but no arguments or code carries that concept here.

I would replace it all with something like:
Will add the key to node index for the first node matching the key. A second call will skip the node which matches how the merger handles siblings that look like duplicates.

---

src/LibChorus/merge/xml/generic/FindNodeToMerge.cs line 319 at r4 (raw file):

		/// <summary>
		/// A ParentIndex entry is identified by the element name as well as the attribute value,

This ties the method description to a place where the method is being used, it should only describe its own purpose and the 'why' should probably not be specific to any particular calling code.