From b43609a57c74ce487fc3380f8a2de49876b633e6 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Mon, 31 Aug 2026 14:57:25 -0400 Subject: [PATCH 1/2] Address NRT review feedback and clear remaining warnings - MapToExistingLanguageIfPossible now returns the empty string (not null) for empty input, honoring its [return: NotNullIfNotNull] contract. - Removed the incorrect [MemberNotNullWhen(false, Id, Source, Target)] from XLiffTransUnit.IsEmpty: IsEmpty is a conjunction, so !IsEmpty does not imply all three members are non-null. Adjusted ToString and XLiffBody.AddTransUnitRaw, which relied on that bogus guarantee. - DefaultInstalledStringFilePath now throws a clear InvalidOperationException instead of passing a possibly-null folder to Path.Combine. - StringCache is backed by a nullable field and throws a clear InvalidOperationException instead of being `null!` and NREing when the minimal constructor was used. - MergeXliffDocuments hoists the trans-unit id once per iteration and skips units without one, replacing ten null-forgiving `tu.Id!` uses. - Fixed the four remaining warnings: CS8604 in XLiffBody (hoisted documented locals) and CS8618 for DefaultStringFilePath (defaults to string.Empty). Co-Authored-By: Claude Opus 5 (1M context) --- src/L10NSharp/LocalizationManagerInternal.cs | 4 +- src/L10NSharp/XLiffUtils/XLiffBody.cs | 21 +++++--- src/L10NSharp/XLiffUtils/XLiffTransUnit.cs | 4 +- .../XLiffUtils/XliffLocalizationManager.cs | 50 ++++++++++++------- 4 files changed, 49 insertions(+), 30 deletions(-) diff --git a/src/L10NSharp/LocalizationManagerInternal.cs b/src/L10NSharp/LocalizationManagerInternal.cs index 0f7a721..e53c5bc 100644 --- a/src/L10NSharp/LocalizationManagerInternal.cs +++ b/src/L10NSharp/LocalizationManagerInternal.cs @@ -566,8 +566,8 @@ public static string GetDynamicStringOrEnglish(string appId, string id, string? [return: NotNullIfNotNull("langId")] internal static string? MapToExistingLanguageIfPossible(string? langId) { - if (langId is null || string.IsNullOrEmpty(langId)) - return null; + if (langId is null || langId.Length == 0) + return langId; // It's a concurrent dictionary, so we can (for performance) try this without a lock. if (MapToExistingLanguage.TryGetValue(langId, out var realId)) return realId; diff --git a/src/L10NSharp/XLiffUtils/XLiffBody.cs b/src/L10NSharp/XLiffUtils/XLiffBody.cs index caf7483..f476b8b 100644 --- a/src/L10NSharp/XLiffUtils/XLiffBody.cs +++ b/src/L10NSharp/XLiffUtils/XLiffBody.cs @@ -132,7 +132,6 @@ internal bool AddTransUnitRaw(XLiffTransUnit? tu) return false; bool lockTaken = false; - string key; try { _transUnitIdLock.Enter(ref lockTaken); @@ -141,11 +140,11 @@ internal bool AddTransUnitRaw(XLiffTransUnit? tu) // it into the dictionary. This assumes nothing else modifies IDs once they // are in this system: once our locked code has given the TU an ID, any other // thread will see that it is non-empty. - key = tu.Id; - if (string.IsNullOrEmpty(key)) + var key = tu.Id; + if (key is null || key.Length == 0) { - tu.Id = (++_transUnitId).ToString(); - key = tu.Id; + key = (++_transUnitId).ToString(); + tu.Id = key; } // If a translation unit with the specified id already exists, then quit here. @@ -167,12 +166,15 @@ public bool AddTransUnit(XLiffTransUnit tu) if (!AddTransUnitRaw(tu)) return false; + // AddTransUnitRaw generates an Id for null/empty Ids, so it is non-null on success. + var id = tu.Id!; + // If the target exists, store its value in the dictionary lookup. Otherwise, store // the source value there. if (tu.Target?.Value != null) - TranslationsById[tu.Id] = tu.Target.Value; + TranslationsById[id] = tu.Target.Value; else if (tu.Source?.Value != null) - TranslationsById[tu.Id] = tu.Source.Value; + TranslationsById[id] = tu.Source.Value; return true; } @@ -194,11 +196,14 @@ internal void AddTransUnitOrVariantFromExisting(XLiffTransUnit tu, string langId if (existingTu == null) return; + // GetTransUnitForId returns null for a null id, so existingTu proves tu.Id isn't. + var id = tu.Id!; + //notice, we don't care if there is already a string in there for this language //(that was the cause of a previous bug), because the XLiff of language X should //surely take precedence, as the translation for that language. existingTu.AddOrReplaceVariant(variantToAdd); - TranslationsById[tu.Id] = variantToAdd.Value; + TranslationsById[id] = variantToAdd.Value; } /// ------------------------------------------------------------------------------------ diff --git a/src/L10NSharp/XLiffUtils/XLiffTransUnit.cs b/src/L10NSharp/XLiffUtils/XLiffTransUnit.cs index 9d58a02..57b8087 100644 --- a/src/L10NSharp/XLiffUtils/XLiffTransUnit.cs +++ b/src/L10NSharp/XLiffUtils/XLiffTransUnit.cs @@ -1,5 +1,4 @@ using System.Collections.Generic; -using System.Diagnostics.CodeAnalysis; using System.Xml.Serialization; using static System.String; @@ -114,7 +113,6 @@ public List Notes /// /// ------------------------------------------------------------------------------------ [XmlIgnore] - [MemberNotNullWhen(false, nameof(Id), nameof(Source), nameof(Target))] public bool IsEmpty => IsNullOrEmpty(Id) && Notes.Count == 0 && Source == null && Target == null; @@ -206,7 +204,7 @@ public void RemoveVariant(string? langId) /// ------------------------------------------------------------------------------------ public override string ToString() { - return IsEmpty ? "Empty" : Id; + return IsNullOrEmpty(Id) ? "Empty" : Id!; } #endregion diff --git a/src/L10NSharp/XLiffUtils/XliffLocalizationManager.cs b/src/L10NSharp/XLiffUtils/XliffLocalizationManager.cs index 81f068b..fa0ebf2 100644 --- a/src/L10NSharp/XLiffUtils/XliffLocalizationManager.cs +++ b/src/L10NSharp/XLiffUtils/XliffLocalizationManager.cs @@ -22,6 +22,7 @@ internal class XliffLocalizationManager : ILocalizationManagerInternal? _stringCache; public Dictionary ComponentCache { get; } = new Dictionary(); @@ -104,7 +105,7 @@ internal XliffLocalizationManager(string appId, string origExtension, string app if (string.IsNullOrEmpty(_customXliffFileFolder)) _customXliffFileFolder = null; - StringCache = new XliffLocalizedStringCache(this); + _stringCache = new XliffLocalizedStringCache(this); } /// @@ -181,8 +182,8 @@ private void CreateOrUpdateDefaultXliffFileIfNecessary( Console.WriteLine("WARNING - L10NSharp Update deleted corrupted {0}", DefaultStringFilePath); } if (verAttribute != null && - Version.TryParse(verAttribute.Value, out var existingVer) && - existingVer >= _appVersion) + Version.TryParse(verAttribute.Value, out var existingVer) && + existingVer >= _appVersion) { return; } @@ -204,10 +205,10 @@ private void CreateOrUpdateDefaultXliffFileIfNecessary( else { stringCache.UpdateLocalizedInfo(new LocalizingInfo("_dummyEntryToGetValidFile") - { - LangId = "en", - Text = "No strings were collected. This entry prevents an invalid, zero-length file. Delete this file to try regenerating it." - } + { + LangId = "en", + Text = "No strings were collected. This entry prevents an invalid, zero-length file. Delete this file to try regenerating it." + } ); } } @@ -298,16 +299,21 @@ public void Dispose() /// /// Full file name and path to the default string file (i.e. English strings). /// + /// Empty for instances created by the minimal constructor. /// ------------------------------------------------------------------------------------ - internal string DefaultStringFilePath { get; } + internal string DefaultStringFilePath { get; } = string.Empty; internal string DefaultInstalledStringFilePath => - Path.Combine(_installedXliffFileFolder, + Path.Combine( + _installedXliffFileFolder ?? throw new InvalidOperationException( + $"{nameof(DefaultInstalledStringFilePath)} is not available on a localization manager created without a folder of installed XLIFF files."), LocalizationManager.GetTranslationFileNameForLanguage(Id, LocalizationManager.kDefaultLang)); /// ------------------------------------------------------------------------------------ - public ILocalizedStringCache StringCache { get; } = null!; + public ILocalizedStringCache StringCache => + _stringCache ?? throw new InvalidOperationException( + "There is no string cache on a localization manager created by the minimal constructor."); /// ------------------------------------------------------------------------------------ @@ -600,15 +606,20 @@ internal static XLiffDocument MergeXliffDocuments(XLiffDocument xliffNew, XLiffD foreach (var tu in xliffNew.File.Body.TransUnitsUnordered) { xliffOutput.File.Body.AddTransUnit(tu); + var id = tu.Id; + // AddTransUnit assigns an Id unless the unit is entirely empty. + if (id == null) + continue; + if (tu.Dynamic) ++newDynamicCount; if (xliffOld != null) { - var tuOld = xliffOld.File.Body.GetTransUnitForId(tu.Id!); + var tuOld = xliffOld.File.Body.GetTransUnitForId(id); if (tuOld == null) { ++newStringCount; - newStringIds.Add(tu.Id!); + newStringIds.Add(id); } else { @@ -637,14 +648,14 @@ internal static XLiffDocument MergeXliffDocuments(XLiffDocument xliffNew, XLiffD if (tu.Source?.Value != tuOld.Source?.Value) { ++changedStringCount; - changedStringIds.Add(tu.Id!); + changedStringIds.Add(id); if (!string.IsNullOrWhiteSpace(tuOld.Source?.Value)) tu.AddNote("en", $"OLD TEXT (before {xliffNew.File.ProductVersion}): {tuOld.Source!.Value}"); } if (tuOld.Dynamic && !tu.Dynamic) { ++wrongDynamicFlagCount; - wrongDynamicStringIds.Add(tu.Id!); + wrongDynamicStringIds.Add(id); tu.AddNote("en", $"Not dynamic: found in static scan of compiled code (version {xliffNew.File.ProductVersion})"); } } @@ -657,14 +668,19 @@ internal static XLiffDocument MergeXliffDocuments(XLiffDocument xliffNew, XLiffD { foreach (var tu in xliffOld.File.Body.TransUnitsUnordered) { - var tuNew = xliffNew.File.Body.GetTransUnitForId(tu.Id!); + var tuNew = xliffNew.File.Body.GetTransUnitForId(tu.Id); if (tuNew == null) { xliffOutput.File.Body.AddTransUnit(tu); + var id = tu.Id; + // AddTransUnit assigns an Id unless the unit is entirely empty. + if (id == null) + continue; + if (tu.Dynamic) { ++missingDynamicStringCount; - missingDynamicStringIds.Add(tu.Id!); + missingDynamicStringIds.Add(id); if (newDynamicCount > 0) // note only if attempt made to collect dynamic strings { tu.Notes.RemoveAll(n => n.Text != null && n.Text.StartsWith("Not found")); @@ -674,7 +690,7 @@ internal static XLiffDocument MergeXliffDocuments(XLiffDocument xliffNew, XLiffD else { ++missingStringCount; - missingStringIds.Add(tu.Id!); + missingStringIds.Add(id); tu.Notes.RemoveAll(n => n.Text != null && n.Text.StartsWith("Not found")); tu.AddNote("en", $"Not found in static scan of compiled code (version {xliffNew.File.ProductVersion})"); } From 9128cec4bd4102fc82ec829bcc551ae444b390a6 Mon Sep 17 00:00:00 2001 From: Danny Rorabaugh Date: Wed, 2 Sep 2026 13:45:34 -0400 Subject: [PATCH 2/2] Update null id handling --- .../XLiffLocalizationManagerTests.cs | 15 ++++++++ src/L10NSharp/XLiffUtils/XLiffBody.cs | 35 +++++++++---------- 2 files changed, 32 insertions(+), 18 deletions(-) diff --git a/src/L10NSharp.Tests/XLiffLocalizationManagerTests.cs b/src/L10NSharp.Tests/XLiffLocalizationManagerTests.cs index f439045..038f275 100644 --- a/src/L10NSharp.Tests/XLiffLocalizationManagerTests.cs +++ b/src/L10NSharp.Tests/XLiffLocalizationManagerTests.cs @@ -280,6 +280,21 @@ public void AddTransUnit_NullSourceNullTarget_DoesNotThrow() Assert.DoesNotThrow(() => body.AddTransUnit(tu)); } + [Test] + public void AddTransUnit_NoId_GeneratesIdAndIndexesTranslation() + { + var body = new XLiffBody(); + var tu = new XLiffTransUnit { + Source = new XLiffTransUnitVariant { Lang = "en", Value = "Text." } + }; + + Assert.That(body.AddTransUnit(tu), Is.True); + + // AddTransUnit relies on the Id generated for an Id-less unit. + Assert.That(tu.Id, Is.Not.Null.And.Not.Empty); + Assert.That(body.TranslationsById[tu.Id], Is.EqualTo("Text.")); + } + [Test] public void MergeXliffDocuments_BaselineUnitHasNullSource_DoesNotThrow() { diff --git a/src/L10NSharp/XLiffUtils/XLiffBody.cs b/src/L10NSharp/XLiffUtils/XLiffBody.cs index f476b8b..99e031b 100644 --- a/src/L10NSharp/XLiffUtils/XLiffBody.cs +++ b/src/L10NSharp/XLiffUtils/XLiffBody.cs @@ -93,8 +93,9 @@ public ListWrapper TransUnitsForXml /// ------------------------------------------------------------------------------------ internal XLiffTransUnit? GetTransUnitForId(string? id) { - if (id == null) - return null; + if (id == null) + return null; + _transUnitDict.TryGetValue(id, out XLiffTransUnit? result); return result; } @@ -124,13 +125,15 @@ public ListWrapper TransUnitsForXml /// Adds the specified translation unit. /// /// The translation unit. - /// true if the translation unit was successfully added. Otherwise, false. + /// The id under which the translation unit was added, or null if it was not + /// added. /// ------------------------------------------------------------------------------------ - internal bool AddTransUnitRaw(XLiffTransUnit? tu) + internal string? AddTransUnitRaw(XLiffTransUnit? tu) { if (tu == null || tu.IsEmpty) - return false; + return null; + string key; bool lockTaken = false; try { @@ -140,8 +143,8 @@ internal bool AddTransUnitRaw(XLiffTransUnit? tu) // it into the dictionary. This assumes nothing else modifies IDs once they // are in this system: once our locked code has given the TU an ID, any other // thread will see that it is non-empty. - var key = tu.Id; - if (key is null || key.Length == 0) + key = tu.Id ?? ""; + if (key.Length == 0) { key = (++_transUnitId).ToString(); tu.Id = key; @@ -151,7 +154,7 @@ internal bool AddTransUnitRaw(XLiffTransUnit? tu) // This check and the dictionary write must both happen inside the lock to avoid // a TOCTOU race where two threads with the same ID both pass the check. if (GetTransUnitForId(key) != null) - return false; + return null; _transUnitDict[key] = tu; } finally @@ -159,16 +162,15 @@ internal bool AddTransUnitRaw(XLiffTransUnit? tu) if (lockTaken) _transUnitIdLock.Exit(false); } - return true; + return key; } public bool AddTransUnit(XLiffTransUnit tu) { - if (!AddTransUnitRaw(tu)) + // Use inserted key (not tu.Id), so TranslationsById and _transUnitDict can't drift. + var id = AddTransUnitRaw(tu); + if (id == null) return false; - // AddTransUnitRaw generates an Id for null/empty Ids, so it is non-null on success. - var id = tu.Id!; - // If the target exists, store its value in the dictionary lookup. Otherwise, store // the source value there. if (tu.Target?.Value != null) @@ -189,21 +191,18 @@ internal void AddTransUnitOrVariantFromExisting(XLiffTransUnit tu, string langId { var variantToAdd = tu.GetVariantForLang(langId); - if (variantToAdd == null || AddTransUnit(tu)) + if (variantToAdd == null || AddTransUnit(tu) || tu.Id == null) return; var existingTu = GetTransUnitForId(tu.Id); if (existingTu == null) return; - // GetTransUnitForId returns null for a null id, so existingTu proves tu.Id isn't. - var id = tu.Id!; - //notice, we don't care if there is already a string in there for this language //(that was the cause of a previous bug), because the XLiff of language X should //surely take precedence, as the translation for that language. existingTu.AddOrReplaceVariant(variantToAdd); - TranslationsById[id] = variantToAdd.Value; + TranslationsById[tu.Id] = variantToAdd.Value; } /// ------------------------------------------------------------------------------------