Skip to content

HermitCrab: prune copy-agreement matches with an empty copy - #645

Open
johnml1135 wants to merge 2 commits into
masterfrom
fix/634-empty-copy-prune
Open

johnml1135 wants to merge 2 commits into
masterfrom
fix/634-empty-copy-prune

Conversation

@johnml1135

@johnml1135 johnml1135 commented Nov 13, 2026 •

Copy link
Copy Markdown
Collaborator

Start here: the copy-agreement prune now treats an empty capture as a zero-length copy, so a match whose copies are "whole word" and "empty" is refused instead of kept. Closes #634.

A non-empty copy cannot equal an empty one, because synthesis writes every copy from one input. Before this, TryGetSegmentCopies returned false on !capture.Success (an empty group is Range.Null), so FieldWorks' full reduplication X => X X (a zero-or-more variable copied twice) re-ran the whole remaining search under a spurious candidate at every reachable point. Parses are unchanged: synthesis already rejected those candidates.

The fix is in AnalysisMorphologicalTransform.TryGetSegmentCopies. If the word holds any optional node (an unapplied deletion), the match stays undecidable, because a restored deletion might fill the empty copy. Only optional-free words are pruned; that is the conservative reading of the issue's soundness note. PanGloss made the same change in sillsdev/PanGloss#247.

Tests (PruneDisagreeingCopiesTests): EmptyCopyIsNeverPruned encoded the leak and is replaced by EmptyCopyAgainstANonEmptyCopyIsPruned (red before the fix, green after) and EmptyCopyIsKeptWhenTheWordHasAnOptionalNode. ZeroOrMoreCopyRuleAddsNoSynthesisWorkForAWordItDoesNotReduplicate is the work-count regression test #634 asked for: with tracing on, five words parse with the same 15 lexical lookups and 3 word syntheses with and without a zero-or-more copy rule. On master it counts 20 lookups against 15. The whole HermitCrab test project passes: 186 passed, 0 failed. csharpier 1.2.6 reports both changed files clean.

Leak case from #634 (39 non-reduplicated words, tracing on, the issue's synthetic grammar with min="0", rule unapplication trace nodes include the two suffix rules): master 316 unapplications, 211 lexical lookups, 128 word syntheses; this branch 184, 79, 43. The no-rule and min="1" controls give 105/79/43 and 184/79/43 on both builds, so the branch returns the leak case to the min="1" baseline. Parse output for all 43 words is identical.

No lost parses: all 154 conformance grammars (6 languages, 132 edge cases, 16 stress; 2,211 words, 2,755 parses on master) were parsed with the HermitCrab tool on master and on this branch. Parse sets are identical for 153 grammars. The 154th, truncate-morphotactic-identity, orders and glosses equivalent empty-morph parses differently from run to run on master itself (three master runs differ from each other); its per-word parse counts are identical in all six runs. This used the tool, not the hc-conformance runner, which is not in this checkout.

Optional nodes: kept conservative on purpose. PanGloss #247 also aligns copies that hold optional nodes and places an empty copy next to its neighbour, which needs the empty group's position; C# gives Range.Null, so the position is not available here. Its own notes report that a verbatim port of the alignment loses valid analyses. That is a separate, larger change (#635 territory), not safe to fold in here.

Next: review.

🤖 Generated with Claude Code


This change is Reviewable

…other is not

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@codecov-commenter

codecov-commenter commented Nov 13, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.30%. Comparing base (511fecc) to head (b8fe5b4).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #645   +/-   ##
=======================================
  Coverage   75.30%   75.30%           
=======================================
  Files         454      454           
  Lines       38527    38531    +4     
  Branches     5339     5340    +1     
=======================================
+ Hits        29013    29017    +4     
  Misses       8376     8376           
  Partials     1138     1138           

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

// an empty capture is a zero-length copy, which cannot equal a non-empty one; but a restored
// deletion (an optional node) might fill it, so such a word stays undecidable
if (match.Input.Shape.Any(node => node.Annotation.Optional))
return false;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: F1. The HasDisagreeingCopies summary (lines 57-64) still lists only "a part the rule modifies, or a copy with an optional node that is not a segment" as never reported. This branch adds a third case: an empty copy in a word that has any optional node. Add it to that sentence so the documented contract matches the code.

AffixProcessRule rule = CreateCopyRule(part);
Word input = CreateInputWithOptionalSecondSegment(first + second);

Assert.That(AnalysisForms(true, rule, input), Is.EquivalentTo(AnalysisForms(false, rule, input)));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: F2. This test would also pass if the rule produced nothing at all, because it only checks that the pruned and unpruned outputs are equal. Pin the unpruned forms first, as EmptyCopyAgainstANonEmptyCopyIsPruned does, so the guard stays tested if matching changes. Unverified: I could not run a mutation with the guard removed.

@claude

claude Bot commented Nov 13, 2026

Copy link
Copy Markdown
Contributor
  1. Verdict: approve. The change is sound. An empty capture now counts as a zero-length copy, and it is pruned only when the word has no optional node. That is the only way analysis can fill an empty copy: an unapplied deletion, or an epenthesis that EpenthesisAnalysisRewriteRuleSpec marks optional.
  2. What matters most: F1 at src/SIL.Machine.Morphology.HermitCrab/MorphologicalRules/AnalysisMorphologicalTransform.cs:97. The HasDisagreeingCopies summary does not list the new undecidable case.
  3. Counts: 0 Critical, 0 Important, 2 Low.
  4. Ran: dotnet test on the HermitCrab tests, 185 passed and 0 failed; the PruneDisagreeingCopiesTests filter, 36 passed. CI (gh pr checks 645): Build on ubuntu-22.04 passed, Build on windows-latest still pending, Report comment hygiene passed (advisory only).
  5. Not verified: the strict scripts/comment-hygiene.ps1 scan and a mutation run without the optional-node guard (neither could run in this environment), and the conformance suite.

Public API, target framework, package, and sillsdev/machine.py parity: none changed. Only private and internal members changed, and machine.py has no HermitCrab.

Findings: F1 new; F2 new, unverified.

Reviewed at 9f247cd

…t does not reduplicate

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Nov 13, 2026

Copy link
Copy Markdown
Contributor

No new findings. Reviewed at b8fe5b4

@johnml1135
johnml1135 marked this pull request as ready for review November 13, 2026 13:08

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HermitCrab: copy-agreement prune keeps matches with an empty copy, re-running the whole search

2 participants