Initial attempt at removing explicit #nullable enable in source #44936

MiYanni · 2024-11-18T22:37:46Z

Related: #25920

Summary

I started working on this in March and aborted the idea because I hit a point where thousands of changes needed to occur. There has been a resurgence in trying to get the repo to be Nullable enable everywhere.

I started by removing #nullable enable in any source files. Then, I attempted to make those projects use <Nullable>enable</Nullable>. There were many issues with this, but in the end, to make the build work, any shared source files I had to add:

#pragma warning disable IDE0240
#nullable enable
#pragma warning restore IDE0240

This is because those files are now shared between both nullable and non-nullable projects. So, this is a stopgap to getting to a point where nullable is enabled everywhere and no source files contain #nullable enable.

Problems

When making projects <Nullable>enable</Nullable>, there are several chain reactions that can occur.

Classes referenced from packages using nullability can cause your callsites to those classes to become invalid
Interfaces that require changes in nullability cause all implementors to also need to change
Base classes that require changes can potentially affect derived classes
Shared source files is incredibly painful since there's no way to resolve problems other than either:
- Using #nullable enable in the file, or
- Changing the project to <Nullable>enable</Nullable>
Logic can be intentionally (or unintentionally) handling null or you need to add custom logic to handle the null situation
- Sometimes, you need to manually unwind the callstack to determine where exactly null should be handled
- Sometimes, just adding ? to everything is really detrimental as you can cause more work for yourself than just handling null in a strategic location
- ! should be used sparingly and intentionally. For example, string.IsNullOrEmpty is not recognized as a null check, so you might need to help the analyzers by putting ! on uses of the variable after that point.
string used as a property where you don't know if string.Empty is an acceptable value
- If you leave it as a non-nullable property, it needs to be set at class instantiation, where previously, it did not. Therefore, you tend to add ? but this can cause a chain reaction of null checking necessary throughout the class and callers of that class and its properties.

Projects now using `<Nullable>enable</Nullable>`

Microsoft.DotNet.Cli.Utils.csproj
Microsoft.DotNet.Configurer.csproj
Microsoft.DotNet.NativeWrapper.csproj
Microsoft.DotNet.SdkResolver.csproj
Microsoft.NET.Build.Extensions.Tasks.csproj
Microsoft.NET.Sdk.Publish.Tasks.csproj

Other changes

If I noticed typos/spelling errors, I'd usually fix them
Some old files needed to be touched, so I tried to clean them up as I was fixing their nullability
I added at least one record to replace passing around a tuple

…xing projects previously using these statements to now use <Nullable>enable</Nullable>. Got the Microsoft.DotNet.Cli.Utils.csproj building. More to come.

… enable.

… nullable enable. Added WorkloadRootPath.cs since the tuples were problematic with nullability logic.

…e enable.

…m/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md#nullable-reference-type-support Converted Microsoft.DotNet.Configurer.csproj to nullable. Realized the other projects that need nullable are massive, so putting this on the backburner.

# Conflicts: # src/Cli/Microsoft.DotNet.Cli.Utils/Microsoft.DotNet.Cli.Utils.csproj # src/Cli/Microsoft.DotNet.Cli.Utils/UILanguageOverride.cs # src/Cli/dotnet/dotnet.csproj # src/Common/WorkloadFileBasedInstall.cs # src/Containers/Microsoft.NET.Build.Containers/LocalDaemons/DockerCli.cs # src/Resolvers/Microsoft.DotNet.NativeWrapper/Microsoft.DotNet.NativeWrapper.csproj # src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/SdkDirectoryWorkloadManifestProvider.cs # src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/WorkloadInstallType.cs # src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/WorkloadResolver.cs # src/Tasks/Common/NuGetUtils.cs # src/WebSdk/Publish/Tasks/Microsoft.NET.Sdk.Publish.Tasks.csproj # src/WebSdk/Publish/Tasks/MsDeploy/CommonUtility.cs # src/WebSdk/Publish/Tasks/Tasks/MsDeploy/MSDeploy.cs # src/WebSdk/Publish/Tasks/Tasks/MsDeploy/VsMsdeploy.cs # src/WebSdk/Publish/Tasks/Tasks/WebJobs/GenerateRunCommandFile.cs # src/WebSdk/Publish/Tasks/Tasks/ZipDeploy/HttpClientHelpers.cs # src/WebSdk/Publish/Tasks/Tasks/ZipDeploy/HttpResponseMessageWrapper.cs # src/WebSdk/Publish/Tasks/WebJobsCommandGenerator.cs

…ccidental removal of nullable on the WorkloadManifestReader.

dotnet-issue-labeler · 2024-11-18T22:37:54Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

dotnet-issue-labeler · 2024-11-18T22:37:56Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

MiYanni · 2024-11-18T22:46:18Z

src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/WorkloadRootPath.cs

+
+namespace Microsoft.NET.Sdk.WorkloadManifestReader
+{
+    public record WorkloadRootPath(string? Path, bool Installable);


I added this record as there was a tuple being passed around previously. Made more sense to use an actual defined type.

MiYanni · 2024-11-18T23:00:06Z

src/WebSdk/Publish/Tasks/MsDeploy/CommonUtility.cs

                                    //Microsoft.Web.Deployment.DeploymentSyncParameterValidationKind validationKind = Microsoft.Web.Deployment.DeploymentSyncParameterValidationKind.None;
-                                    //Microsoft.Web.Deployment.DeploymentSyncParameterValidationKind currentvalidationKind;
+                                    //Microsoft.Web.Deployment.DeploymentSyncParameterValidationKind currentValidationKind;
                                    //string[] validationKinds = item.Kind.Split(new char[] { ',' });

                                    //foreach (string strValidationKind in validationKinds)
                                    //{
-                                    //    if (System.Enum.TryParse<Microsoft.Web.Deployment.DeploymentSyncParameterValidationKind>(strValidationKind, out currentvalidationKind))
+                                    //    if (System.Enum.TryParse<Microsoft.Web.Deployment.DeploymentSyncParameterValidationKind>(strValidationKind, out currentValidationKind))
                                    //    {
-                                    //        validationKind |= currentvalidationKind;
+                                    //        validationKind |= currentValidationKind;
                                    //    }
                                    //}


I have no idea why this is here. A relic of the past? 🗿

333fred · 2024-11-18T23:21:49Z

src/RazorSdk/Tool/Microsoft.NET.Sdk.Razor.Tool.csproj

@@ -1,4 +1,4 @@
-<Project Sdk="Microsoft.NET.Sdk">
+<Project Sdk="Microsoft.NET.Sdk">


BOM change?

I'd assume so. I had changed more in this file initially but removed those changes.

Consider reverting all changes in this file then.

joeloff · 2024-11-18T23:21:56Z

src/Cli/Microsoft.DotNet.Configurer/Microsoft.DotNet.Configurer.csproj

@@ -9,6 +9,7 @@
    <RepositoryType>git</RepositoryType>
    <DefineConstants Condition="'$(IncludeAspNetCoreRuntime)' == 'false'">$(DefineConstants);EXCLUDE_ASPNETCORE</DefineConstants>
    <IsPackable>true</IsPackable>
+    <Nullable>enable</Nullable>


Should this be in a directory.build.props perhaps?

Not possible without doing the entire repo first or figuring out some way of untangling the mess of shared code files. Or, you manually check nested project directories to see if all the projects in that directory already use nullability and then added a new Directory.Build.props for that sub-directory. Doesn't seem worthwhile, though. This is way too initial to do that kind of change.

joeloff · 2024-11-18T23:23:13Z

src/Cli/dotnet/commands/dotnet-run/RunCommand.cs

@@ -317,7 +317,7 @@ static ICommand CreateCommandFromRunProperties(ProjectInstance project, RunPrope
                CommandSpec commandSpec = new(runProperties.RunCommand, runProperties.RunArguments);

                var command = CommandFactoryUsingResolver.Create(commandSpec)
-                    .WorkingDirectory(runProperties.RunWorkingDirectory);
+                    .WorkingDirectory(runProperties.RunWorkingDirectory ?? string.Empty);


could this previously be null and any concerns about being empty now. Assuming we didn't have explicit null vs. nullorwhitespace checks

This was already defined as string? RunWorkingDirectory at the top of this file. What changed was WorkingDirectory now became part of a nullable-enabled assembly, and its parameter is defined as string projectDirectory. That means, previously, if this was passed null, it would assign it to process.StartInfo.WorkingDirectory. If the decompiled code is to be believed, the default for that property is "", so I just left it as that in this situation. The code for it does handle null, and it'll just return string.Empty anyway.

joeloff · 2024-11-18T23:27:31Z

src/Common/CliFolderPathCalculatorCore.cs

-
-//only Microsoft.DotNet.MSBuildSdkResolver (net7.0) has nullables enabled
-#pragma warning disable IDE0240 // Remove redundant nullable directive
+#pragma warning disable IDE0240
 #nullable enable


shouldn't this just remove the #nullable enable

You cannot. It is a shared file. You'd have to do the entire Microsoft.NET.Build.Tasks and (likely) the dotnet CLI project, which are thousands of changes.

joeloff · 2024-11-18T23:29:27Z

src/Common/EnvironmentVariableNames.cs

@@ -1,10 +1,9 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.

-#pragma warning disable IDE0240 // Nullable directive is redundant (when file is included to a project that already enables nullable
-
+#pragma warning disable IDE0240
 #nullable enable


ditto, should we remove the #nullable enable an

If you see changes like this, it is because I initially tried to remove it, but then added it back again because it is a shared source file and cannot be removed.

joeloff · 2024-11-18T23:37:16Z

src/Tasks/Common/ConflictResolution/PlatformManifestReader.cs


-                    if (line.Length == 0 || line[0] == '#')
+                    if (line is null || line.Length == 0 || line[0] == '#')


should this instead be string.IsNullOrWhitespace(line)?

I think I did that at first but changed it to this because string.IsNullOrWhitespace is not recognized by the analyzer as a null-check. Therefore, the call to the indexer in the rest of the or-statement would need to be line![0] == '#' which is a little jankier. This seemed like a simpler change since it is more explicit to the analyzer.

joeloff · 2024-11-18T23:38:43Z

src/Tasks/Common/FileUtilities.MetadataReader.cs

        {
+            if (sourcePath == null)


are empty paths valid here?

So, the property that eventually flows to here could return null (from ConflictItem.cs):

The value returned from this method ends up here, and you'll see that someone clearly didn't mind if it was null via the comment (also from ConflictItem.cs):

MiYanni added 9 commits March 6, 2024 12:18

Removed the explicit, in-code #nullable enable statements. Started fi…

265f486

…xing projects previously using these statements to now use <Nullable>enable</Nullable>. Got the Microsoft.DotNet.Cli.Utils.csproj building. More to come.

Got the Microsoft.NET.Sdk.Publish.Tasks.csproj building with nullable…

1240664

… enable.

Got Microsoft.NET.Sdk.WorkloadMSBuildSdkResolver.csproj building with…

4ddb7b2

… nullable enable. Added WorkloadRootPath.cs since the tuples were problematic with nullability logic.

Got Microsoft.NET.Build.Extensions.Tasks.csproj building with nullabl…

9ef3407

…e enable.

Added #nullable enable to certain shared files to get the build working.

2a5ca26

Merge branch 'main' into RemoveExplicitNullableSetters2

3d4ea3e

Fixes based on looking over the PR diff. Removed commented code and a…

cd3b4e6

…ccidental removal of nullable on the WorkloadManifestReader.

MiYanni requested review from dsplaisted and a team November 18, 2024 22:37

MiYanni requested review from a team and vijayrkn as code owners November 18, 2024 22:37

dotnet-issue-labeler bot added the untriaged Request triage from a team member label Nov 18, 2024

MiYanni requested a review from v-wuzhai November 18, 2024 22:39

MiYanni commented Nov 18, 2024

View reviewed changes

333fred reviewed Nov 18, 2024

View reviewed changes

joeloff reviewed Nov 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial attempt at removing explicit #nullable enable in source #44936

Initial attempt at removing explicit #nullable enable in source #44936

MiYanni commented Nov 18, 2024 •

edited

Loading

dotnet-issue-labeler bot commented Nov 18, 2024

dotnet-issue-labeler bot commented Nov 18, 2024

MiYanni Nov 18, 2024

MiYanni Nov 18, 2024

333fred Nov 18, 2024

MiYanni Nov 18, 2024

333fred Nov 19, 2024

joeloff Nov 18, 2024

MiYanni Nov 18, 2024

joeloff Nov 18, 2024

MiYanni Nov 18, 2024

joeloff Nov 18, 2024

MiYanni Nov 18, 2024

joeloff Nov 18, 2024

MiYanni Nov 18, 2024

joeloff Nov 18, 2024

MiYanni Nov 18, 2024

joeloff Nov 18, 2024

MiYanni Nov 18, 2024

		@@ -1,4 +1,4 @@
		<Project Sdk="Microsoft.NET.Sdk">
		<Project Sdk="Microsoft.NET.Sdk">


		if (line.Length == 0 \|\| line[0] == '#')
		if (line is null \|\| line.Length == 0 \|\| line[0] == '#')

Initial attempt at removing explicit #nullable enable in source #44936

Are you sure you want to change the base?

Initial attempt at removing explicit #nullable enable in source #44936

Conversation

MiYanni commented Nov 18, 2024 • edited Loading

Summary

Problems

Projects now using <Nullable>enable</Nullable>

Other changes

dotnet-issue-labeler bot commented Nov 18, 2024

dotnet-issue-labeler bot commented Nov 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiYanni commented Nov 18, 2024 •

edited

Loading

Projects now using `<Nullable>enable</Nullable>`