Address NRT review feedback and clear remaining warnings

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()
 		{

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;

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,15 +125,16 @@ public ListWrapper TransUnitsForXml
 		/// Adds the specified translation unit.
 		/// </summary>
 		/// <param name="tu">The translation unit.</param>
-		/// <returns>true if the translation unit was successfully added. Otherwise, false.</returns>
+		/// <returns>The id under which the translation unit was added, or null if it was not
+		/// added.</returns>
 		/// ------------------------------------------------------------------------------------
-		internal bool AddTransUnitRaw(XLiffTransUnit? tu)
+		internal string? AddTransUnitRaw(XLiffTransUnit? tu)
 		{
 			if (tu == null || tu.IsEmpty)
-				return false;
+				return null;
 
-			bool lockTaken = false;
 			string key;
+			bool lockTaken = false;
 			try
 			{
 				_transUnitIdLock.Enter(ref lockTaken);
@@ -141,38 +143,40 @@ 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))
+				key = tu.Id ?? "";
+				if (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.
 				// 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
 			{
 				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;
 
 			// 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;
 		}
 
@@ -187,7 +191,7 @@ 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);

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<XLiffNote> Notes
 		/// </summary>
 		/// ------------------------------------------------------------------------------------
 		[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

src/L10NSharp/XLiffUtils/XliffLocalizationManager.cs

@@ -22,6 +22,7 @@ internal class XliffLocalizationManager : ILocalizationManagerInternal<XLiffDocu
 		private readonly string? _customXliffFileFolder;
 		private readonly string? _origExeExtension;
 		private readonly Version _appVersion;
+		private readonly ILocalizedStringCache<XLiffDocument>? _stringCache;
 
 		public Dictionary<IComponent, string> ComponentCache { get; } = new Dictionary<IComponent, string>();
 
@@ -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);
 		}
 
 		/// <summary>
@@ -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()
 		/// <summary>
 		/// Full file name and path to the default string file (i.e. English strings).
 		/// </summary>
+		/// <remarks>Empty for instances created by the minimal constructor.</remarks>
 		/// ------------------------------------------------------------------------------------
-		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<XLiffDocument> StringCache { get; } = null!;
+		public ILocalizedStringCache<XLiffDocument> 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})");
 						}