[NET10.0] Annotate converters for nullability 4/n by MartyIX · Pull Request #29549 · dotnet/maui

MartyIX · 2025-05-16T14:04:32Z

Note

Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!

Description of Change

Follow-up to #29314. See #28244 (comment) for details on motivation.

Issues Fixed

Contributes to #28860

dotnet-policy-service · 2025-05-16T14:04:53Z

Hey there @@MartyIX! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

MartyIX · 2025-05-16T20:08:57Z

src/Controls/src/Core/Brush/BrushTypeConverter.cs

+				_parts = css!.Replace("\r\n", "")
 #else
-				_parts = css.Replace("\r\n", "", StringComparison.Ordinal)
+				_parts = css!.Replace("\r\n", "", StringComparison.Ordinal)


For some reason string.IsNullOrWhiteSpace(css) is not enough for the analyzer to realize that css is not null at this point. (if (css is null || css = "") would be an alternative to avoid the ! operator.

MartyIX · 2025-05-19T05:58:58Z

src/Controls/src/Core/Brush/BrushTypeConverter.cs

+				if (strValue.StartsWith(Rgb, StringComparison.InvariantCulture) 
+					|| strValue.StartsWith(Rgba, StringComparison.InvariantCulture) 
+					|| strValue.StartsWith(Hsl, StringComparison.InvariantCulture) 
+					|| strValue.StartsWith(Hsla, StringComparison.InvariantCulture))


StringComparison.InvariantCulture was missing for Hsla, so I added it.

rmarinho · 2025-05-19T11:39:25Z

@MartyIX net10 android needs Android 35 api? we also recommend jdk 21.

MartyIX · 2025-05-19T11:59:11Z

I do have JDK 21 installed on my machine.

net10 android needs Android 35 api?

I don't know. I just ran dotnet build ./microsoft.Maui.BuildTasks.slnf.

bhavanesh2001 · 2025-05-19T14:29:54Z

@MartyIX I ran into the same build issue.

In my case, I had updated my System Environment Variables to set ANDROID_SDK_ROOT and ANDROID_HOME to:

C:\Users\user1\AppData\Local\Android\Sdk

— primarily to get Appium working. After that, I started encountering build errors when trying to build MAUI code.

It turns out that the MAUI/Gradle build process checks for the SDK at:

C:\Program Files (x86)\Android\android-sdk

which is where Visual Studio installs the SDK by default via the Android SDK Manager.

To resolve the issue, I simply removed the manually set environment variables, and the build worked again without errors.

Hope this helps.

MartyIX · 2025-05-19T20:02:05Z

Much appreciated! My setup seems to be a bit different. I'm trying to repair my Visual Studio at the moment, if it is not related to it. It's a wild guess.

Otherwise, my setup looks pretty normal. Not sure what might be wrong. But I'll figure it out... sooner or later.

rmarinho · 2025-05-19T22:27:41Z

/azp run

azure-pipelines · 2025-05-19T22:28:00Z

Azure Pipelines successfully started running 3 pipeline(s).

rmarinho · 2025-05-19T22:28:56Z

I do have JDK 21 installed on my machine.

net10 android needs Android 35 api?

I don't know. I just ran dotnet build ./microsoft.Maui.BuildTasks.slnf.

Sorry it wasn't a question, you need to install api 35 of android

MartyIX · 2025-05-20T07:50:00Z

@bhavanesh2001 I copied C:\Program Files (x86)\Android\android-sdk\platforms\android-Baklava -> C:\Program Files (x86)\Android\android-sdk\platforms\android-36 (effectively renaming it) and it did the trick for me. :)

rmarinho · 2025-05-21T09:21:58Z

src/Controls/src/Core/Shapes/PathFigureCollectionConverter.cs

 								}

-								pathFigure.Segments.Add(new LineSegment
+								pathFigure!.Segments.Add(new LineSegment


can we just return if it s null ?

It is my understanding that it would be better to:

if (pathFigure is null) { ThrowBadToken(); } else { pathFigure.Segments.Add(new LineSegment { Point = lastPoint }); }

because one can get here presumably only if the parsed path string is somehow invalid (there is no 'm' / 'M' specified.

Would it make sense to you as an alternative?

I did it this way.

rmarinho

Can we add a test passing null to these converters to make sure it actually works and doesn t break ?

MartyIX · 2025-05-27T13:50:46Z

Can we add a test passing null to these converters to make sure it actually works and doesn t break ?

I added tests which show that converters behave differently but the tests hopefully make sure that they behave the same as before.

MartyIX · 2025-05-27T13:51:49Z

src/Controls/tests/Core.UnitTests/PathFigureCollectionConverterTests.cs

+	public class PathFigureCollectionConverterTests
 	{
-		private readonly PathFigureCollectionConverter _pathFigureCollectionConverter = new();
+		private readonly PathFigureCollectionConverter _converter = new();


Renamed so it's unified with other tests.

MartyIX · 2025-05-27T13:52:38Z

src/Controls/src/Core/Shapes/PathFigureCollectionConverter.cs

 			}

-			void ThrowBadToken()
+			Exception GetBadTokenException()


Modified to return the exception as throw GetBadTokenException() behaves much better with respect to .NET analyzers (detection of dead code, etc.)

MartyIX · 2025-05-27T13:58:22Z

src/Controls/src/Core/ColumnDefinitionCollectionTypeConverter.cs

-		public override object ConvertFrom(ITypeDescriptorContext context, CultureInfo culture, object value)
+		public override object? ConvertFrom(ITypeDescriptorContext? context, CultureInfo? culture, object value)
 		{
 			var strValue = value?.ToString();


In an ideal world, we would go with

Suggested change

var strValue = value?.ToString();

var strValue = value.ToString();

but that would change behavior. This approach is used even in other converters.

rmarinho · 2025-05-27T14:18:11Z

/azp run

azure-pipelines · 2025-05-27T14:18:35Z

Azure Pipelines successfully started running 3 pipeline(s).

rmarinho · 2025-05-27T14:18:55Z

src/Controls/tests/Core.UnitTests/BrushTypeConverterUnitTests.cs

+	public class BrushTypeConverterUnitTests : BaseTestFixture
 	{
-		BrushTypeConverter _converter;
+		private readonly BrushTypeConverter _converter = new();


we don t use private

OK.

IMO there should be an analyzer checking it. So much time of humans and machines is lost on this.

MartyIX requested a review from a team as a code owner May 16, 2025 14:04

MartyIX requested review from jfversluis and rmarinho May 16, 2025 14:04

dotnet-policy-service bot added the community ✨ Community Contribution label May 16, 2025

MartyIX commented May 16, 2025

View reviewed changes

MartyIX commented May 19, 2025

View reviewed changes

MartyIX force-pushed the feature/2025-05-16-Convert-nullability-4-n-NET10 branch from 199468b to e694190 Compare May 19, 2025 21:17

PureWeen added this to MAUI SDK Ongoing May 20, 2025

github-project-automation bot moved this to Todo in MAUI SDK Ongoing May 20, 2025

PureWeen moved this from Todo to Changes Requested in MAUI SDK Ongoing May 20, 2025

PureWeen added this to the .NET 10.0-preview6 milestone May 20, 2025

rmarinho reviewed May 21, 2025

View reviewed changes

rmarinho requested changes May 21, 2025

View reviewed changes

MartyIX added 3 commits May 27, 2025 14:43

Annotate

5498204

review

db9232b

More tests

af9d132

MartyIX force-pushed the feature/2025-05-16-Convert-nullability-4-n-NET10 branch from e694190 to af9d132 Compare May 27, 2025 13:49

MartyIX commented May 27, 2025

View reviewed changes

rmarinho reviewed May 27, 2025

View reviewed changes

rmarinho approved these changes May 28, 2025

View reviewed changes

github-project-automation bot moved this from Changes Requested to Approved in MAUI SDK Ongoing May 28, 2025

rmarinho merged commit 9568b9c into dotnet:net10.0 May 28, 2025
119 of 135 checks passed

github-project-automation bot moved this from Approved to Done in MAUI SDK Ongoing May 28, 2025

MartyIX deleted the feature/2025-05-16-Convert-nullability-4-n-NET10 branch May 28, 2025 12:02

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

	var strValue = value?.ToString();
	var strValue = value.ToString();

Conversation

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

Uh oh!

Description of Change

Issues Fixed

Uh oh!

dotnet-policy-service bot commented May 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmarinho commented May 19, 2025

Uh oh!

MartyIX commented May 19, 2025

Uh oh!

bhavanesh2001 commented May 19, 2025

Uh oh!

MartyIX commented May 19, 2025

Uh oh!

rmarinho commented May 19, 2025

Uh oh!

azure-pipelines bot commented May 19, 2025

Uh oh!

rmarinho commented May 19, 2025

Uh oh!

MartyIX commented May 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmarinho left a comment

Choose a reason for hiding this comment

Uh oh!

MartyIX commented May 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmarinho commented May 27, 2025

Uh oh!

azure-pipelines bot commented May 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MartyIX commented May 16, 2025 •

edited

Loading