[XABT] Enable NRT for some tasks. #10099

jpobst · 2025-05-01T23:17:37Z

Enable NRT for tasks in Xamarin.Android.Build.Tasks that only needed trivial changes.

The following conventions were used:

When the task property is guarded by [Register], MSBuild will ensure the property is not null, thus we set these values to a non-allocating default:

[Required]
public ITaskItem Manifest { get; set; } = null!;  // NRT - guarded by [Required]

[Required]
public ITaskItem[] AssetDirectories { get; set; } = [];

[Required]
public string PackageName { get; set; } = "";

For non-[Required] properties and fields, set the type to nullable.

public ITaskItem? Manifest { get; set; }

public ITaskItem[]? AssetDirectories { get; set; }

public string? PackageName { get; set; }

Additionally, string.IsNullOrEmpty (string) and friends are not properly annotated for NRT in netstandard2.0. Create string extension methods like foo.IsNullOrEmpty () that are properly annotated and use them where needed.

A few of our unit tests were not filling in [Required] task properties and thus failing due to getting different results. Update the tests to pass the [Required] properties.

jonathanpeppers · 2025-05-03T14:22:58Z

src/Xamarin.Android.Build.Tasks/Tasks/Aapt2Compile.cs

-			if (!string.IsNullOrEmpty (ExtraArgs))
+			if (!ExtraArgs.IsNullOrEmpty ())


So, you can also use pattern matching instead:

android/.github/copilot-instructions.md

Lines 61 to 75 in dc07800

If you see a `string.IsNullOrEmpty()` check:

```csharp

if (!string.IsNullOrEmpty (NonRequiredProperty)) {

// Code here

}

```

Convert this to:

```csharp

if (NonRequiredProperty is { Length: > 0 }) {

// Code here

}

```

Should we do this instead of an extension method?

GH Copilot was also able to make the change as mentioned here. I'm not sure it will be able to know about an extension method we made unless the file was somehow always added in its context.

Maybe we need a poll, but I find string.IsNullOrEmpty() or value.IsNullOrEmpty() to be easier to read than value is { Length: > 0 }. I want proper semantic names, not expressions!

How does everyone else feel?

I find IsNullOrEmpty() to be easier to read as well, it's documenting the intent and the effect very clearly.

jonpryor · 2025-05-03T14:52:40Z

src/Xamarin.Android.Build.Tasks/MamJsonToXml.cs


 		[Required]
-		public  ITaskItem[] MappingFiles        { get; set; }
+		public  ITaskItem[] MappingFiles        { get; set; } = [];


Should this also be = null!? It would prevent a GC allocation, and it'll be set to a different value by MSBuild anyway…

They are the same as Array.Empty<ITaskItem>(), it shouldn't allocate.

jonpryor · 2025-05-03T14:53:37Z

src/Xamarin.Android.Build.Tasks/Tasks/Aapt2LinkAssetPack.cs


 		[Required]
-		public ITaskItem[] AssetDirectories { get; set; }
+		public ITaskItem[] AssetDirectories { get; set; } = [];


Ditto here.

jonpryor · 2025-05-03T14:58:55Z

Array initializers:

public ITaskItem[] AssetDirectories { get; set; } = [];

result in use of Array.Empty<T>(). This is a caching "non-allocating" method, i.e. there is a single GC allocation for each type T. If you wouldn't otherwise use Array.Empty<ITaskItem>(), then this is a (single) extra GC allocation.

Given MSBuild semantics here, I think we should use = null! for arrays, not = [].

jpobst · 2025-05-03T16:33:45Z

Maybe we need a poll, but I find string.IsNullOrEmpty() or value.IsNullOrEmpty() to be easier to read than value is { Length: > 0 }. I want proper semantic names, not expressions!

I find IsNullOrEmpty to be easier to read as well.

there is a single GC allocation for each type T

As the only 2 types of T here are ITaskItem and string, we're talking about allocating 2 empty arrays for the whole process, which seems fine.

I think we should use = null! for arrays

In general my preference is to use the null forgiveness operator as sparingly as possible if there's a cheap/easy alternative.

jonathanpeppers

If we are good with the other syntax in here, I think I am.

Maybe the last thing is to update this example to use IsNullOrEmpty() extension method:

android/.github/copilot-instructions.md

Lines 61 to 75 in dc07800

    
           If you see a `string.IsNullOrEmpty()` check: 
        
           ```csharp 
        
           if (!string.IsNullOrEmpty (NonRequiredProperty)) { 
        
               // Code here 
        
           } 
        
           ``` 
        
           Convert this to: 
        
           ```csharp 
        
           if (NonRequiredProperty is { Length: > 0 }) { 
        
               // Code here 
        
           } 
        
           ```

If someone ever asked copilot to opt into NRT, it could have a chance at doing the right thing.

I guess we'd also put a path to where the new extension method lives?

jpobst · 2025-05-05T17:50:24Z

Maybe the last thing is to update this example to use IsNullOrEmpty() extension method:

Is there a way to scope that example solely to the Xamarin.Android.Build.Tasks project? For example, we wouldn't want it to make that change in anything targeting .NET 10 because string.IsNullOrEmpty is properly annotated.

I also noticed those instructions specify using "" instead of string.Empty. Do we want to change this PR to use ""? I used string.Empty but I don't really have a preference here.

jonathanpeppers · 2025-05-05T17:51:27Z

It says to only do it for "For MSBuild task properties like:", but we could put a stronger suggestion in there. Could mention netstandard2.0?

jpobst · 2025-05-05T23:48:10Z

Updated copilot-instructions.md and switched from string.Empty to "".

Context: #10099 Context: #9918 #10099 enabled NRT checks for a number of XABT Tasks and was merged because all the tests passed. However, #10099 was not rebased on the tip of `main` which, in the meantime, contained code from the recently merged #9918 PR. #9918 introduced a couple of `string` properties to the `AndroidComputeResPaths` and `CreateAar` tasks, which were not nullable annotated. This resulted in the following build errors once #10099 was merged: src/Xamarin.Android.Build.Tasks/Tasks/AndroidComputeResPaths.cs(52,17): error CS8618: Non-nullable property 'PrefixProperty' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable. src/Xamarin.Android.Build.Tasks/Tasks/CreateAar.cs(37,17): error CS8618: Non-nullable property 'PrefixProperty' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable. This PR makes the `PrefixProperty` properties `[Required]` (because of the way they are used in the tasks) and assigns default values to them, as per convention introduced in #10099

Enable NRT for some "utility" classes in `Xamarin.Android.Build.Tasks` that only needed trivial changes. The conventions established in #10099 were used.

jpobst force-pushed the dev/jpobst/xabt-nrt-1 branch from 28d1aee to e2761df Compare May 2, 2025 17:57

jpobst marked this pull request as ready for review May 2, 2025 19:03

jpobst requested review from grendello, jonathanpeppers and jonpryor as code owners May 2, 2025 19:03

jonathanpeppers reviewed May 3, 2025

View reviewed changes

jonpryor reviewed May 3, 2025

View reviewed changes

jonathanpeppers approved these changes May 5, 2025

View reviewed changes

jpobst force-pushed the dev/jpobst/xabt-nrt-1 branch from e2761df to 105d65b Compare May 5, 2025 19:40

[XABT] Enable NRT for some tasks.

4652b88

jpobst force-pushed the dev/jpobst/xabt-nrt-1 branch from 105d65b to 4652b88 Compare May 5, 2025 19:42

jonathanpeppers approved these changes May 6, 2025

View reviewed changes

jpobst merged commit 65430b1 into main May 6, 2025
59 checks passed

jpobst deleted the dev/jpobst/xabt-nrt-1 branch May 6, 2025 02:50

grendello mentioned this pull request May 6, 2025

[XABT] Fix build errors caused by enabling nullable checks in the tasks #10110

Merged

jpobst mentioned this pull request May 9, 2025

[XABT] Enable NRT for some utility classes. #10129

Merged

jpobst added a commit that referenced this pull request May 9, 2025

[XABT] Enable NRT for some utility classes. (#10129)

3e28a41

Enable NRT for some "utility" classes in `Xamarin.Android.Build.Tasks` that only needed trivial changes. The conventions established in #10099 were used.

github-actions bot locked and limited conversation to collaborators Jun 5, 2025

		if (!string.IsNullOrEmpty (ExtraArgs))
		if (!ExtraArgs.IsNullOrEmpty ())

	If you see a `string.IsNullOrEmpty()` check:

	```csharp
	if (!string.IsNullOrEmpty (NonRequiredProperty)) {
	// Code here
	}
	```

	Convert this to:

	```csharp
	if (NonRequiredProperty is { Length: > 0 }) {
	// Code here
	}
	```

[XABT] Enable NRT for some tasks. #10099

[XABT] Enable NRT for some tasks. #10099

Uh oh!

Conversation

jpobst commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonathanpeppers May 3, 2025

Choose a reason for hiding this comment

Uh oh!

jonpryor May 3, 2025

Choose a reason for hiding this comment

Uh oh!

grendello May 5, 2025

Choose a reason for hiding this comment

Uh oh!

jonpryor May 3, 2025

Choose a reason for hiding this comment

Uh oh!

jonathanpeppers May 3, 2025

Choose a reason for hiding this comment

Uh oh!

jonpryor May 3, 2025

Choose a reason for hiding this comment

Uh oh!

jonpryor commented May 3, 2025

Uh oh!

jpobst commented May 3, 2025

Uh oh!

jonathanpeppers left a comment

Choose a reason for hiding this comment

Uh oh!

jpobst commented May 5, 2025

Uh oh!

jonathanpeppers commented May 5, 2025

Uh oh!

jpobst commented May 5, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jpobst commented May 1, 2025 •

edited

Loading