Bump external/xamarin-android-tools and fix NRT warnings

[jonathanpeppers]

Bumps external/xamarin-android-tools from 2fd1240 to ed6aab1, which enables nullable reference types in BaseTasks.

Fixes all resulting CS8600/CS8602/CS8604 NRT warnings across 18 files without using ! (null-forgiving operator).

Key changes in xamarin-android-tools

• ed6aab1 [BaseTasks] Enable nullable reference types ([AndroidClientHandler] Prevent orphaned thread in DoProcessRequest #344)
• 4fd9ae0 [Tools.AndroidSdk] Remove unused key_name variables (use ant version 1.9.8 as 1.9.7 is no longer available at ibiblio #345)
• 3bfeba6 [copilot] Add /review GitHub agentic workflow (Add support for Round Icon #346)

NRT fix patterns used

• Null-coalescing for params args: value ?? "" for nullable strings passed to params object[]
• Null checks + throw: for GetRegisteredTaskObjectAssemblyLocal/UnregisterTaskObjectAssemblyLocal results that must exist
• Null-conditional calls: Log?.LogDebugMessage(...) for nullable TaskLoggingHelper
• Remove unnecessary ?.: e.g. min_sdk?.Value -> min_sdk.Value inside if (min_sdk != null) blocks
• Nullable variable declarations: e.g. string? mono = ... for nullable return types

Supersedes #11243

[Copilot]

Pull request overview

This PR updates the external/xamarin-android-tools dependency to a commit that enables nullable reference types (NRT) in BaseTasks, and then adjusts the dotnet/android codebase to address the resulting NRT warnings in build tasks/utilities.

Changes:

• Bump external/xamarin-android-tools (enables NRT in BaseTasks) and update consumers accordingly.
• Add null-handling patterns (null checks/throws, null-coalescing for logging args, null-conditional debug logging) across MSBuild tasks and utilities.
• Tighten some internal build-pipeline assumptions by failing fast when required registered task objects are missing.

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/Xamarin.Android.Build.Tasks/Utilities/RtxtParser.cs	Make debug logging tolerant of nullable logger under NRT.
src/Xamarin.Android.Build.Tasks/Utilities/FileResourceParser.cs	Adjust debug logging calls for nullable Log under NRT.
src/Xamarin.Android.Build.Tasks/Tasks/RewriteMarshalMethods.cs	Add null check for required registered NativeCodeGenState.
src/Xamarin.Android.Build.Tasks/Tasks/ResolveAndroidTooling.cs	Coalesce nullable strings when used as message args for coded errors.
src/Xamarin.Android.Build.Tasks/Tasks/Legacy/ValidateJavaVersion.cs	Coalesce nullable BuildTools version when used as message arg.
src/Xamarin.Android.Build.Tasks/Tasks/Legacy/ResolveAndroidTooling.cs	Coalesce nullable version strings when used as warning args.
src/Xamarin.Android.Build.Tasks/Tasks/GetJavaPlatformJar.cs	NRT-driven adjustments around manifest logging / message args.
src/Xamarin.Android.Build.Tasks/Tasks/GenerateTypeMappings.cs	Add null check for required registered NativeCodeGenState.
src/Xamarin.Android.Build.Tasks/Tasks/GenerateMainAndroidManifest.cs	Add null checks for required registered objects; avoid null output path on cleanup.
src/Xamarin.Android.Build.Tasks/Tasks/GenerateLayoutBindings.cs	Make registered widget collection nullable and guard its use.
src/Xamarin.Android.Build.Tasks/Tasks/GenerateAdditionalProviderSources.cs	Add null check for required registered NativeCodeGenStateCollection.
src/Xamarin.Android.Build.Tasks/Tasks/ConvertCustomView.cs	Coalesce nullable resource directory metadata when logging coded diagnostics.
src/Xamarin.Android.Build.Tasks/Tasks/CalculateLayoutCodeBehind.cs	Remove unnecessary null-conditional on non-null widgets.
src/Xamarin.Android.Build.Tasks/Tasks/BuildAppBundle.cs	Change debug log formatting while addressing NRT warnings (but see review comment).
src/Xamarin.Android.Build.Tasks/Tasks/AndroidWarning.cs	Coalesce nullable resource string / format args under NRT.
src/Xamarin.Android.Build.Tasks/Tasks/AndroidError.cs	Coalesce nullable resource string / format args under NRT.
src/Xamarin.Android.Build.Tasks/Tasks/AndroidDotnetToolTask.cs	Treat cached mono path as nullable under NRT.
src/Xamarin.Android.Build.Tasks/Tasks/Aapt2Compile.cs	Ensure non-null enumerable passed to async processing under NRT.

Comments suppressed due to low confidence (2)

src/Xamarin.Android.Build.Tasks/Tasks/GenerateLayoutBindings.cs:213

• After the widgets is null || widgets.Count == 0 guard, widgets is known to be non-null, but later the code still uses the null-forgiving operator (widgets!) when calling GenerateSource. Since this PR aims to fix NRT warnings without !, please refactor to avoid the null-forgiving operator here (e.g., by passing the non-null widgets variable directly after the guard).

			string collectionKey;
			if (!GetRequiredMetadata (item, CalculateLayoutCodeBehind.WidgetCollectionKeyMetadata, out collectionKey))
				return;

			ICollection<LayoutWidget>? widgets = BuildEngine4.GetRegisteredTaskObjectAssemblyLocal<ICollection<LayoutWidget>> (ProjectSpecificTaskObjectKey (collectionKey), RegisteredTaskObjectLifetime.Build);
			if (widgets is null || widgets.Count == 0) {
				string inputPaths = String.Join ("; ", resourceItems.Select (i => i.ItemSpec));
				LogCodedWarning ("XA4222", Properties.Resources.XA4222, inputPaths);
				return;
			}

src/Xamarin.Android.Build.Tasks/Utilities/FileResourceParser.cs:76

• ResourceParser.Log is nullable, but this method now mixes Log?.LogDebugMessage(...) with later unconditional Log.LogDebugMessage(...) calls in the same method body. If Log is required for this parser, consider validating Log is non-null once at the start of Parse() (and then using Log. consistently) instead of sprinkling null-conditional calls (which can also leave CS8602 warnings on the remaining Log. usages).

		public IList<R> Parse (string resourceDirectory, IEnumerable<string> additionalResourceDirectories, IEnumerable<string> aarLibraries, Dictionary<string, string> resourceMap)
		{
			Log?.LogDebugMessage ($"Parsing Directory {resourceDirectory}");
			publicXml = LoadPublicXml ();
			var result = new List<R> ();
			Dictionary<string, ICollection<R>> resources = new Dictionary<string, ICollection<R>> ();
			foreach (var knownType in RtxtParser.knownTypes) {
				if (knownType == "styleable") {
					resources.Add (knownType, new List<R> ());
					continue;
				}
				resources.Add (knownType, new SortedSet<R> (new RComparer ()));
			}
			foreach (var dir in Directory.EnumerateDirectories (resourceDirectory, "*", SearchOption.TopDirectoryOnly)) {
				foreach (var file in Directory.EnumerateFiles (dir, "*.*", SearchOption.AllDirectories)) {
					ProcessResourceFile (file, resources);
				}
			}
			foreach (var dir in additionalResourceDirectories ?? []) {
				Log.LogDebugMessage ($"Processing Directory {dir}");
				if (Directory.Exists (dir)) {
					foreach (var file in Directory.EnumerateFiles (dir, "*.*", SearchOption.AllDirectories)) {
						ProcessResourceFile (file, resources);
					}
				} else {
					Log.LogDebugMessage ($"Skipping non-existent directory: {dir}");
				}

[jonathanpeppers]

/review

[github-actions]

✅ Android PR Reviewer completed successfully!

[github-actions]

🤖 Review Summary

Verdict: ✅ LGTM (with minor items)

Overall this is a clean, well-structured NRT fix PR. The ResourceParser refactoring (property → constructor injection for Log) is a good improvement, the removal of ! null-forgiving operators in JavaResourceParser is correct, and the RtxtParser cleanup eliminating instance state in favor of method parameters is cleaner.

Issue counts:

• ⚠️ 1 warning — behavioral change in ConvertCustomView.cs where ?? "" interacts with FixupResourceFilename's StartsWith logic
• 💡 2 suggestions — fallback message for missing resources, minor formatting

Positive callouts:

• The nativeCodeGenStates is null → throw InvalidOperationException pattern across multiple tasks is the right approach — fail fast on internal errors rather than letting a NullReferenceException crash later
• Good catch removing the unnecessary ?. in CalculateLayoutCodeBehind (widgets?.Count → widgets.Count) and GetJavaPlatformJar (min_sdk?.Value → min_sdk.Value) inside null-guarded blocks
• The ResourceParser constructor injection pattern properly enforces the Log dependency at construction time, which is much better than the previous mutable property

CI: Public checks passing ✅

Generated by Android PR Reviewer for issue #11247 · ● 12.1M

[github-actions]

🤖 ⚠️ Correctness — resdir?.ItemSpec ?? "" changes behavior vs. the pre-NRT code.

Previously, when resdir was null, null was passed to FixupResourceFilename, which has a if (resourceDir != null && ...) guard that skipped the fixup and kept the raw file path. Now, passing "" causes StartsWith("") to always be true, so the fixup path executes unconditionally — stripping leading path separators and prepending "Resources/", mangling the filename in the error message.

Consider guarding the call instead:

case TraceLevel.Error:
	if (resdir != null)
		Log.FixupResourceFilenameAndLogCodedError ("XA1002", message, file, resdir.ItemSpec, resource_name_case_map);
	else
		Log.LogCodedError ("XA1002", file: file, lineNumber: 0, message: message);
	break;

Same issue on line 62 for the TraceLevel.Warning case.

Rule: NRT fix must preserve runtime behavior

[jonathanpeppers]

Good catch — fixed. Now guarding on resdir != null and falling back to Log.LogCodedError/Log.LogCodedWarning directly when there is no resource directory, preserving the original behavior.

[github-actions]

🤖 💡 Formatting — Missing blank line between the new constructor and the CompareTuple nested class declaration.

Rule: Minimal diffs / consistent formatting

[jonathanpeppers]

Fixed, added the blank line.

[github-actions]

🤖 💡 Error handling — GetString() ?? "" silently turns a missing/mistyped ResourceName into an empty error message, which would be very hard to diagnose.

Consider a more informative fallback, e.g.:

var message = Properties.Resources.ResourceManager.GetString (ResourceName, Properties.Resources.Culture)
	?? $"(Missing resource: {ResourceName})";

Same applies to AndroidWarning.cs.

Rule: Log messages must have context (Postmortem #6)

[jonathanpeppers]

Good idea — changed both AndroidError and AndroidWarning to use ?? $"(Missing resource: {ResourceName})" so a misconfigured resource name is diagnosable.