migrate core library to use nullable reference types

[hahn-kev]

After seeing some fixes from @imnasnainaec, I figured I'd try my hand at enabling NRT and fixing some issues that came up.

There's currently 4 warnings still in place, I wasn't sure what the right call would be, so we can leave them in or come up with a fix together. They are:

    src\L10NSharp\XLiffUtils\XLiffBody.cs(173,22): warning CS8604: Possible null reference argument for parameter 'key' in 'string ConcurrentDictionary<string, string>.this[string key]'.
    src\L10NSharp\XLiffUtils\XLiffBody.cs(175,22): warning CS8604: Possible null reference argument for parameter 'key' in 'string ConcurrentDictionary<string, string>.this[string key]'.
    src\L10NSharp\XLiffUtils\XLiffBody.cs(201,21): warning CS8604: Possible null reference argument for parameter 'key' in 'string ConcurrentDictionary<string, string>.this[string key]'.
    src\L10NSharp\XLiffUtils\XliffLocalizationManager.cs(126,12): warning CS8618: Non-nullable property 'DefaultStringFilePath' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable.

There's a lot of code that used XLiffTransUnit.Id with the assumption that it's not null, however it can be, I've fixed many of those.

Some places I've added checks like if x is null throw new exception this is only in cases where we would have thrown anyway when accessing a property on x later down, so while this may not fix a bug, it does give much better feedback as to what was null.

There's a few places where code may have been returning null, which are now returning an empty string, these were changed due to the method already returning an empty string elsewhere, with the assumption that the method was written to never return null, and these cases were missed.

One change I did make is to constrain ILocalizationManagerInternal<T> T to only be a class, this may effect consumers, but I don't think that's possible as the only implementation of this internal interface is a T of XLiffDocument, and I think if anything else were to be used there would be a crash at runtime, so I don't think it's a problem.

---

This change is 

[github-actions]

Test Results

    7 files  ±0  124 suites  ±0   31s ⏱️ +4s
187 tests ±0  182 ✔️ ±0    5 💤 ±0  0 ❌ ±0 
714 runs  ±0  699 ✔️ ±0  15 💤 ±0  0 ❌ ±0 

Results for commit f14ab15. ± Comparison against base commit ae81ffb.

♻️ This comment has been updated with latest results.

[imnasnainaec]

@imnasnainaec reviewed 8 files and all commit messages, and made 6 comments.
Reviewable status: 8 of 35 files reviewed, 6 unresolved discussions (waiting on hahn-kev).

---

src/L10NSharp/LocalizationManagerInternal.cs line 569 at r1 (raw file):

		internal static string? MapToExistingLanguageIfPossible(string? langId)
		{
			if (langId is null || string.IsNullOrEmpty(langId))

Devin bug:

src/L10NSharp/LocalizationManagerInternal.cs:R569-570

Non-null empty string input can produce null output, violating the declared nullability contract

The language-mapping method returns null for a non-null empty-string input (return null at src/L10NSharp/LocalizationManagerInternal.cs:570 when string.IsNullOrEmpty is true), but its NotNullIfNotNull attribute promises a non-null return whenever the argument is non-null, so callers may use the result without a null check.

Impact: If a caller passes an empty (but non-null) language ID, the returned value is unexpectedly null, which could cause a null-reference crash in downstream lookups.

The guard conflates null and empty but the attribute only covers null

At src/L10NSharp/LocalizationManagerInternal.cs:566-570:

[return: NotNullIfNotNull("langId")]
internal static string? MapToExistingLanguageIfPossible(string? langId)
{
    if (langId is null || string.IsNullOrEmpty(langId))
        return null;

NotNullIfNotNull("langId") tells the compiler: "if langId is not null, the return value is not null." But string.IsNullOrEmpty("") is true, so passing "" (non-null) returns null, violating the contract. Callers like MapToExistingLanguageOrAddMapping at src/L10NSharp/LocalizationManagerInternal.cs:659-662 pass a non-null langId and use the result without null checks, trusting the attribute.

The fix should either return langId (i.e., "") instead of null for the empty-string case, or remove the NotNullIfNotNull attribute.

___ *[`src/L10NSharp/XLiffUtils/XLiffTransUnit.cs` line 117 at r1](https://reviewable.io/reviews/sillsdev/l10nsharp/157#-Ox5tSYEDaCdxzCbmr2g:-Ox5tSYGBFEd1y7JRjOv:b5nmmcv) ([raw file](https://github.com/sillsdev/l10nsharp/blob/f14ab15aabe710f560979cf05f957e891870e3bf/src/L10NSharp/XLiffUtils/XLiffTransUnit.cs#L117)):* > ```Smalltalk > /// ------------------------------------------------------------------------------------ > [XmlIgnore] > [MemberNotNullWhen(false, nameof(Id), nameof(Source), nameof(Target))] > ```

Devin bug:

src/L10NSharp/XLiffUtils/XLiffTransUnit.cs:R117-119

Incorrect nullability guarantee on translation unit emptiness check can hide null-reference crashes

The emptiness check on a translation unit incorrectly promises that three properties are non-null whenever the unit is non-empty ([MemberNotNullWhen(false, ...)] at src/L10NSharp/XLiffUtils/XLiffTransUnit.cs:117), so the compiler silently suppresses null warnings for code that accesses those properties after the check.

Impact: Code that trusts the compiler's null-safety analysis after checking non-emptiness may dereference null Target, Source, or Id at runtime, causing a crash.

The IsEmpty logic uses AND, but the attribute requires all-or-nothing non-nullness

IsEmpty is defined as:

IsNullOrEmpty(Id) && Notes.Count == 0 && Source == null && Target == null

It returns false when ANY single condition is false. For example, a XLiffTransUnit with a valid Id ("foo") but Target == null has IsEmpty == false, yet Target is still null. The MemberNotNullWhen(false, nameof(Id), nameof(Source), nameof(Target)) attribute tells the compiler that ALL three are non-null whenever IsEmpty is false, which is incorrect.

In practice, Target is commonly null for non-empty translation units (it is only set for translated strings — see the constructor at src/L10NSharp/XLiffUtils/XLiffTransUnit.cs:19-20 which sets Target = null). Any future code (or current code in callers) that checks !tu.IsEmpty and then accesses tu.Target.Value without a null check will crash, and the compiler will not warn about it.

___ *[`src/L10NSharp/XLiffUtils/XliffLocalizationManager.cs` line 304 at r1](https://reviewable.io/reviews/sillsdev/l10nsharp/157#-Ox5uJyZ0Odvo1hV0Ny0:-Ox5uJyZ0Odvo1hV0Ny1:bl3p0hu) ([raw file](https://github.com/sillsdev/l10nsharp/blob/f14ab15aabe710f560979cf05f957e891870e3bf/src/L10NSharp/XLiffUtils/XliffLocalizationManager.cs#L304)):* > ```Smalltalk > internal string DefaultStringFilePath { get; } > > internal string DefaultInstalledStringFilePath => > ```

Devin flag:

src/L10NSharp/XLiffUtils/XliffLocalizationManager.cs:R304-307

DefaultInstalledStringFilePath passes nullable field to Path.Combine without null guard

_installedXliffFileFolder was changed from string to string? at line 20, but DefaultInstalledStringFilePath at src/L10NSharp/XLiffUtils/XliffLocalizationManager.cs:304-307 still passes it directly to Path.Combine, which would throw ArgumentNullException if null. This is currently safe because DefaultInstalledStringFilePath is only accessed from XliffLocalizedStringCache.MergeXliffFilesIntoCache (at src/L10NSharp/XLiffUtils/XliffLocalizedStringCache.cs:135), which is only called from the full constructor that always sets _installedXliffFileFolder to a non-null value. However, the type system no longer enforces this invariant, making it fragile.

---

src/L10NSharp/XLiffUtils/XliffLocalizationManager.cs line 310 at r1 (raw file):

		/// ------------------------------------------------------------------------------------
		public ILocalizedStringCache<XLiffDocument> StringCache { get; } = null!;

Devin flag:

src/L10NSharp/XLiffUtils/XliffLocalizationManager.cs:R310

StringCache property uses null-forgiving initializer, relying on constructor ordering

The StringCache property at src/L10NSharp/XLiffUtils/XliffLocalizationManager.cs:310 is initialized as = null!, meaning the compiler trusts it will be set before use. It is indeed set in the full constructor at line 107 (StringCache = new XliffLocalizedStringCache(this)), but the minimal constructor at line 126 does NOT set it. If the minimal constructor is used directly (it's internal), any access to StringCache would throw a NullReferenceException. Currently the minimal constructor is only called via the : this(...) chain from the full constructor, so this is safe, but it's fragile — a future internal caller using the minimal constructor would hit a null deref with no compiler warning.

---

src/L10NSharp/XLiffUtils/XliffLocalizationManager.cs line 607 at r1 (raw file):

				if (xliffOld != null)
				{
					var tuOld = xliffOld.File.Body.GetTransUnitForId(tu.Id!);

Devin flag:

src/L10NSharp/XLiffUtils/XliffLocalizationManager.cs:R603-675

Null-forgiving operators on tu.Id throughout MergeXliffDocuments assume IDs are always present

Throughout MergeXliffDocuments in src/L10NSharp/XLiffUtils/XliffLocalizationManager.cs, tu.Id! is used extensively (lines 603, 607, 637, 640, 644, 656, 663, 667, 673, 675). Since XLiffTransUnit.Id is now string?, these null-forgiving operators suppress warnings. The assumption is safe for translation units read from XLIFF files (the XML deserializer populates the id attribute), and for units that passed through AddTransUnitRaw (which assigns an ID if missing). However, the widespread use of ! means a malformed XLIFF file with a missing id attribute could cause NullReferenceException deep in the merge logic rather than a clear error at parse time.

---

src/L10NSharp/XLiffUtils/XliffLocalizedStringCache.cs line 55 at r1 (raw file):

					Console.WriteLine(e.Message);
					LocalizationManager.SetUILanguage(LocalizationManager.kDefaultLang);
					if (DefaultXliffDocument is null)

Devin flag:

src/L10NSharp/XLiffUtils/XliffLocalizedStringCache.cs:R50-57

XliffLocalizedStringCache constructor may leave DefaultXliffDocument null on error path

In src/L10NSharp/XLiffUtils/XliffLocalizedStringCache.cs:50-57, when loadAvailableXliffFiles is true and MergeXliffFilesIntoCache throws, the catch block calls LocalizationManager.SetUILanguage(kDefaultLang) and then checks if (DefaultXliffDocument is null) throw. This is good defensive coding added in this PR. However, the _tuUpdater initialization at line 62 and the DefaultXliffDocument.File.AmpersandReplacement access at line 64 both assume DefaultXliffDocument is non-null. If the exception path somehow continues (e.g., if the null check is removed in the future), these would crash. The [MemberNotNull] attribute on MergeXliffFilesIntoCache at line 127 correctly enforces this for the normal path.

[imnasnainaec]

Here are a couple of options for the 4 remaining warnings. They collapse into 2 root causes.

Warnings 1–3 — CS8604 in XLiffBody.cs (lines 173, 175, 201)

Cause: XLiffTransUnit.Id is string?, but TranslationsById is a ConcurrentDictionary<string, string> whose indexer requires a non-null key. In all three spots the surrounding code already guarantees Id is non-null:

• Lines 173/175 (AddTransUnit) run only after AddTransUnitRaw(tu) returned true, and that method generates an Id whenever it's null/empty — so tu.Id is non-null on success.
• Line 201 (AddTransUnitOrVariantFromExisting) runs only after GetTransUnitForId(tu.Id) returned non-null, and that method returns null when id is null — so reaching line 201 means tu.Id is non-null.

Option A — null-forgiving operator (minimal, recommended):

// AddTransUnit — after AddTransUnitRaw succeeds, tu.Id is guaranteed non-null.
if (tu.Target?.Value != null)
    TranslationsById[tu.Id!] = tu.Target.Value;
else if (tu.Source?.Value != null)
    TranslationsById[tu.Id!] = tu.Source.Value;

// AddTransUnitOrVariantFromExisting (line 201) — existingTu was found by tu.Id, so it's non-null.
TranslationsById[tu.Id!] = variantToAdd.Value;

Option B — hoist a documented local (clearer intent in AddTransUnit):

if (!AddTransUnitRaw(tu))
    return false;

// AddTransUnitRaw generates an Id for empty/null Ids, so it is non-null on success.
var id = tu.Id!;
if (tu.Target?.Value != null)
    TranslationsById[id] = tu.Target.Value;
else if (tu.Source?.Value != null)
    TranslationsById[id] = tu.Source.Value;

I'd lean toward A plus the one-line comment where the guarantee isn't obvious.

Warning 4 — CS8618 in XliffLocalizationManager.cs:126

Cause: DefaultStringFilePath is a get-only, non-nullable auto-property assigned only in the full constructor. The minimal ctor XliffLocalizationManager(string, string, string) never sets it — and it is used standalone by the test factory CreateLocalizationManager (XLiffLocalizationManagerTests.cs), where the real path can't be computed (the directory fields don't exist yet).

Option A — default the property (recommended): the full ctor still overrides it, and this preserves prior behavior (the field was effectively unset/null in the minimal-ctor case before NRT; empty string is strictly safer than null):

internal string DefaultStringFilePath { get; } = string.Empty;

Option B — make it nullable: internal string? DefaultStringFilePath { get; } — most honest about the minimal ctor, but it ripples: the ~9 usages (File.Delete/Copy/Open, Path.GetDirectoryName, XElement.Load, …) would each need a ! or a guard, likely trading one warning for several new CS8604s.

I'd lean toward A — one line, no downstream churn.

---

_{Drafted by Claude Opus 4.8}