Skip to content

LT-22607 TonePars use XAmpleParser code for result processing #1002

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
AndyBlack wants to merge 2 commits into
base: main
Choose a base branch
from
Open

LT-22607 TonePars use XAmpleParser code for result processing #1002

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eec842e
LT-22607 TonePars use XAmpleParser code for result processing
AndyBlack Jul 9, 2026
7251cf8
Add ToneParsInvoker test fix
AndyBlack Jul 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions Src/LexText/ParserCore/XAmpleParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,13 @@ public ParseResult ParseWord(string word)
CheckDisposed();

var results = new StringBuilder(m_xample.ParseWord(word));
ParseResult result = ProcessParseResults(ref results);

return result;
}

public ParseResult ProcessParseResults(ref StringBuilder results)

@johnml1135 johnml1135 Jul 9, 2026

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.

Claude - Sonnet 5: The ref on StringBuilder results is functionally unnecessary — StringBuilder.Replace mutates in place and returns the same instance, and neither call site (ParseWord here, or ToneParsInvoker.SaveResultsInDatabase) reads results after the call. A plain string results parameter would be simpler and would avoid the caller in ToneParsInvoker.cs (SaveResultsInDatabase) having to allocate a throwaway StringBuilder purely to satisfy this signature.

Also: this method doesn't call CheckDisposed(), unlike Update()/Reset()/ParseWord()/ParseWordXml() on this same DisposableBase-derived class. Lower severity — this mirrors a pre-existing gap in the code this PR deletes (which had the same CheckDisposed() omitted, with a //TODO: fix! comment), and there's no demonstrated live path where a disposed instance is called through today — but now that this method is called from another assembly (ToneParsInvoker), it's worth closing for consistency with the rest of the public API.

{
results = results.Replace("DB_REF_HERE", "'0'");
results = results.Replace("<...>", "[...]");
var wordformElem = XElement.Parse(results.ToString());
Expand Down
6 changes: 4 additions & 2 deletions ...ilities/pcpatrflex/DisambiguateInFLExDB/DisambiguateInFLExDBTests/ToneParsInvokerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
// Copyright (c) 2019 SIL International
// Copyright (c) 2019 SIL International
// This software is licensed under the LGPL, version 2.1 or later
// (http://www.gnu.org/licenses/lgpl-2.1.html)

using NUnit.Framework;
using PtxUtils;
using SIL.DisambiguateInFLExDB;
using SIL.FieldWorks.Common.FwUtils;
using SIL.FieldWorks.WordWorks.Parser;
using SIL.LCModel;
using SIL.ToneParsFLEx;
using System;
Expand Down Expand Up @@ -117,7 +118,8 @@ public void ToneParsInvokerTest()
string toneParsRuleFile = Path.Combine(TestDataDir, "KvgTP.ctl");
string intxCtlFile = Path.Combine(TestDataDir, "KVGintx.ctl");
string inputFile = Path.Combine(TestDataDir, "KVGinput.txt");
invoker = new ToneParsInvoker(toneParsRuleFile, intxCtlFile, inputFile, '+', MyCache);
XAmpleParser xampleParser = new XAmpleParser(MyCache, null);
invoker = new ToneParsInvoker(toneParsRuleFile, intxCtlFile, inputFile, '+', MyCache, xampleParser);
CreateExpectedFileStrings();
File.Copy(Path.Combine(TestDataDir, "ToneParsInvoker.ana"), Path.Combine(Path.GetTempPath(), "ToneParsInvoker.ana"), true);
ToneParsInvokerOptions.Instance.VerifyInformation = true;
Expand Down
6 changes: 4 additions & 2 deletions Src/Utilities/pcpatrflex/ToneParsFLExDll/ToneParsFLExForm.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,7 +481,8 @@ private void InvokeToneParser(string inputFile)
tbIntxCtlFile.Text,
inputFile,
GetDecompSeparationCharacter(),
Cache
Cache,
m_XAmpleParser
);
invoker.Extractor = Extractor;
invoker.ParsingStatus = lblParsingStatus;
Expand Down Expand Up @@ -786,7 +787,8 @@ private void ShowLog_Click(object sender, EventArgs e)
tbIntxCtlFile.Text,
"",
' ',
Cache
Cache,
m_XAmpleParser
);
if (File.Exists(invoker.ToneParsLogFile))
{
Expand Down
180 changes: 10 additions & 170 deletions Src/Utilities/pcpatrflex/ToneParsFLExDll/ToneParsInvoker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,15 @@ public class ToneParsInvoker

public const string kTPLexicon = "TPlex.txt";
public FLExDBExtractor Extractor { get; set; }
protected XAmpleParser XAmpleParser { get; set; }

public ToneParsInvoker(
string toneParsRuleFile,
string intxCtlFile,
string inputFile,
char decomp,
LcmCache cache
LcmCache cache,
XAmpleParser xAmpleParser
)
{
ToneParsRuleFile = toneParsRuleFile;
Expand All @@ -73,6 +75,11 @@ LcmCache cache
DatabaseName = ConvertNameToUseAnsiCharacters(cache.ProjectId.Name);
InitFileNames();
Queue = new IdleQueue { IsPaused = true };
XAmpleParser = xAmpleParser;
if ( XAmpleParser == null )
{
XAmpleParser = new XAmpleParser(Cache, null);

@johnml1135 johnml1135 Jul 9, 2026

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.

Claude - Sonnet 5: This fallback constructs a full XAmpleParser (native XAmple engine init + a ParserModelChangeListener registered on Cache.DomainDataByFlid), but ToneParsInvoker has no IDisposable/Dispose and never disposes it — a genuine resource leak. Worse, it's not an edge case: in ToneParsFLExForm.cs, m_XAmpleParser is only populated on the HermitCrab code path, so whenever XAmple is the active parser, every "Parse Text"/"Parse Segment"/"Show Log" click passes null here and leaks a new native handle + listener each time.

Separately: this fallback passes dataDir = null, and XAmpleParser.Update() does Path.Combine(m_dataDir, "Configuration", "Grammar") with no null guard. Nothing in ToneParsInvoker calls Update()/Reset() today so this is latent, but every other production caller of new XAmpleParser(...) passes a real dataDir — this is a new, unguarded landmine if that ever changes.

}
}

private void InitFileNames()
Expand Down Expand Up @@ -684,9 +691,8 @@ public void SaveResultsInDatabase()
IWfiWordform thiswf = GetWordformFromString(wordform);
if (thiswf != null)
{
var parseResult = ConvertParserFilerResultXmlToParseResult(
ParserFilerXMLString
);
var sb = new StringBuilder(ParserFilerXMLString);
var parseResult = XAmpleParser.ProcessParseResults(ref sb);
m_parseFiler.ProcessParse(thiswf, ParserPriority.Low, parseResult);
}
i++;
Expand Down Expand Up @@ -724,171 +730,5 @@ protected void ExecuteIdleQueue(IdleQueue idleQueue)
task.Delegate(task.Parameter);
idleQueue.Clear();
}

//-----------------------
// ConvertParserFilerResultXmlToParseResult(), TryCreateParseMorph(), and ProcessMsaHvo() are from XAmpleParser.cs
// (I renamed ParseWord() to ConvertParserFilerResultXmlToParseResult() to avoid confusion.)
// Ideally, we'd expose them from XAmpleParser.cs
public ParseResult ConvertParserFilerResultXmlToParseResult(string results)
{
//TODO: fix! CheckDisposed();
results = results.Replace("DB_REF_HERE", "'0'");
results = results.Replace("<...>", "[...]");
var wordformElem = XElement.Parse(results.ToString());
string errorMessage = null;
var exceptionElem = wordformElem.Element("Exception");
if (exceptionElem != null)
{
var totalAnalysesValue = (string)exceptionElem.Attribute("totalAnalyses");
switch ((string)exceptionElem.Attribute("code"))
{
case "ReachedMaxAnalyses":
errorMessage = String.Format(
"Maximum permitted analyses ({0}) reached." /*ParserCoreStrings.ksReachedMaxAnalysesAllowed*/
,
totalAnalysesValue
);
break;
case "ReachedMaxBufferSize":
errorMessage = String.Format(
"Maximum internal buffer size ({0}) reached." /*ParserCoreStrings.ksReachedMaxInternalBufferSize*/
,
totalAnalysesValue
);
break;
}
}
else
{
errorMessage = (string)wordformElem.Element("Error");
}

ParseResult result;
using (
new WorkerThreadReadHelper(
Cache.ServiceLocator.GetInstance<IWorkerThreadReadHandler>()
)
)
{
var analyses = new List<ParseAnalysis>();
foreach (XElement analysisElem in wordformElem.Descendants("WfiAnalysis"))
{
var morphs = new List<ParseMorph>();
bool skip = false;
foreach (XElement morphElem in analysisElem.Descendants("Morph"))
{
ParseMorph morph;
if (!TryCreateParseMorph(Cache, morphElem, out morph))
{
skip = true;
break;
}
if (morph != null)
morphs.Add(morph);
}

if (!skip && morphs.Count > 0)
analyses.Add(new ParseAnalysis(morphs));
}
result = new ParseResult(analyses, errorMessage);
}

return result;
}

private static bool TryCreateParseMorph(
LcmCache cache,
XElement morphElem,
out ParseMorph morph
)
{
XElement formElement = morphElem.Element("MoForm");
Debug.Assert(formElement != null);
var formHvo = (string)formElement.Attribute("DbRef");

XElement msiElement = morphElem.Element("MSI");
Debug.Assert(msiElement != null);
var msaHvo = (string)msiElement.Attribute("DbRef");

// Normally, the hvo for MoForm is a MoForm and the hvo for MSI is an MSA
// There are four exceptions, though, when an irregularly inflected form is involved:
// 1. <MoForm DbRef="x"... and x is an hvo for a LexEntryInflType.
// This is one of the null allomorphs we create when building the
// input for the parser in order to still get the Word Grammar to have something in any
// required slots in affix templates. The parser filer can ignore these.
// 2. <MSI DbRef="y"... and y is an hvo for a LexEntryInflType.
// This is one of the null allomorphs we create when building the
// input for the parser in order to still get the Word Grammar to have something in any
// required slots in affix templates. The parser filer can ignore these.
// 3. <MSI DbRef="y"... and y is an hvo for a LexEntry.
// The LexEntry is a variant form for the first set of LexEntryRefs.
// 4. <MSI DbRef="y"... and y is an hvo for a LexEntry followed by a period and an index digit.
// The LexEntry is a variant form and the (non-zero) index indicates
// which set of LexEntryRefs it is for.
ICmObject objForm;
if (String.IsNullOrEmpty(formHvo) ||
!cache.ServiceLocator
.GetInstance<ICmObjectRepository>()
.TryGetObject(int.Parse(formHvo), out objForm)
)
{
morph = null;
return false;
}
var form = objForm as IMoForm;
if (form == null)
{
morph = null;
return true;
}

// Irregulary inflected forms can have a combination MSA hvo: the LexEntry hvo, a period, and an index to the LexEntryRef
Tuple<int, int> msaTuple = ProcessMsaHvo(msaHvo);
ICmObject objMsa;
if (
!cache.ServiceLocator
.GetInstance<ICmObjectRepository>()
.TryGetObject(msaTuple.Item1, out objMsa)
)
{
morph = null;
return false;
}
var msa = objMsa as IMoMorphSynAnalysis;
if (msa != null)
{
morph = new ParseMorph(form, msa);
return true;
}

var msaAsLexEntry = objMsa as ILexEntry;
if (msaAsLexEntry != null)
{
// is an irregularly inflected form
// get the MoStemMsa of its variant
if (msaAsLexEntry.EntryRefsOS.Count > 0)
{
ILexEntryRef lexEntryRef = msaAsLexEntry.EntryRefsOS[msaTuple.Item2];
ILexSense sense = MorphServices.GetMainOrFirstSenseOfVariant(lexEntryRef);
var inflType = lexEntryRef.VariantEntryTypesRS[0] as ILexEntryInflType;
morph = new ParseMorph(form, sense.MorphoSyntaxAnalysisRA, inflType);
return true;
}
}

// if it is anything else, we ignore it
morph = null;
return true;
}

private static Tuple<int, int> ProcessMsaHvo(string msaHvo)
{
string[] msaHvoParts = msaHvo.Split('.');
return Tuple.Create(
int.Parse(msaHvoParts[0]),
msaHvoParts.Length == 2 ? int.Parse(msaHvoParts[1]) : 0
);
}
// -----------------------
}
}
Loading