Remove nullable disable warnings #48846

jaredpar · 2025-05-07T19:46:14Z

This change removes all uses of #nullable disable warnings in the code base. This means the files now get full nullable validation where previously they were using annotations with no compiler enforcement. For the most part this change just follows the warnings that result from removing the directive but there are a few small checks that got added, like Debug.Assert, that are worth looking at.

I will also call out in the review a few patterns in the sdk / System.CommandLine that are hard to work with in nullable.

Note: to make this easier to review I have added review comments any place I made a change that was not purely mechanical in nature.

Copilot

Pull Request Overview

This pull request removes bulk usages of "#nullable disable warnings" to enforce full nullable validation across the codebase. In doing so, it updates type annotations and parameters in multiple files to adapt to stricter nullability rules.

Removed "#nullable disable warnings" directives from various files.
Adjusted method and record parameter types to non-nullable where appropriate.
Added a null-coalescing fallback to ensure string values are not unexpectedly null.

Reviewed Changes

Copilot reviewed 44 out of 44 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/Cli/dotnet/Commands/Sdk/Check/SdkOutputWriter.cs	Removed "#nullable disable warnings".
src/Cli/dotnet/Commands/Sdk/Check/RuntimeOutputWriter.cs	Removed "#nullable disable warnings".
src/Cli/dotnet/Commands/Run/RunProperties.cs	Updated RunProperties record to enforce a non-null RunCommand.
src/Cli/dotnet/Commands/Run/RunCommand.cs	Changed restoreArgs parameter to be non-nullable.
src/Cli/dotnet/Commands/Run/LaunchSettings/*	Removed "#nullable disable warnings".
src/Cli/dotnet/Commands/Run/CommonRunHelpers.cs	Adjusted parameters from nullable to non-nullable for consistency.
src/Cli/dotnet/Commands/New/PostActions/*	Removed "#nullable disable warnings".
src/Cli/dotnet/Commands/New/NewCommandParser.cs	Added a fallback for NetStandardImplicitPackageVersion.
src/Cli/dotnet/Commands/New/MSBuildEvaluation/*	Removed "#nullable disable warnings" and updated dictionary types.
src/Cli/dotnet/Commands/New/DotnetCommandCallbacks.cs	Removed "#nullable disable warnings".
src/Cli/dotnet/CommandFactory/CommandSpec.cs	Updated parameter types and default values for clarity.

Comments suppressed due to low confidence (5)

src/Cli/dotnet/CommandFactory/CommandSpec.cs:8

Updating the path parameter to be non-nullable assumes that all initializations of CommandSpec provide a valid path. Verify that this contract is maintained across the codebase to prevent unexpected null values.

public class CommandSpec( string path, string? args, Dictionary<string, string>? environmentVariables = null)

src/Cli/dotnet/Commands/Run/RunCommand.cs:324

Converting the restoreArgs parameter from nullable to non-nullable requires confirming that all call sites pass a valid array. Verify that this change aligns with caller expectations and handle potential null values upstream if needed.

static ProjectInstance EvaluateProject(string? projectFilePath, Func<ProjectCollection, ProjectInstance>? projectFactory, string[] restoreArgs, ILogger? binaryLogger)

src/Cli/dotnet/Commands/Run/CommonRunHelpers.cs:37

Updating the args parameter to be non-nullable enforces a stricter contract. Ensure that all callers are updated to provide a non-null string array.

public static Dictionary<string, string> GetGlobalPropertiesFromArgs(string[] args)

src/Cli/dotnet/Commands/New/NewCommandParser.cs:171

[nitpick] Using an empty string as a fallback for NetStandardImplicitPackageVersion is acceptable if it conforms with downstream expectations. Confirm that this fallback value appropriately handles cases where the version string might be null.

{ "NetStandardImplicitPackageVersion", new FrameworkDependencyFile().GetNetStandardLibraryVersion() ?? "" },

src/Cli/dotnet/Commands/New/MSBuildEvaluation/MSBuildEvaluator.cs:198

Allowing null values in the properties dictionary may lead to downstream issues if consumers assume non-null strings. Review the consumers of this dictionary to ensure proper null handling or consider defaulting to empty strings where appropriate.

Dictionary<string, string?> properties = new()

src/Cli/dotnet/Commands/Run/RunProperties.cs

jaredpar · 2025-05-07T19:47:15Z

src/Cli/dotnet/CommandFactory/CommandSpec.cs

 {
    public string Path { get; } = path;

-    public string Args { get; } = args;
+    public string? Args { get; } = args;


Having Args be potentially null seems suspicious, I considered changing this to the following but don't know enough about the code base to say if that is okay

public string Args { get; } = args ?? "";

jaredpar · 2025-05-07T19:48:10Z

src/Cli/dotnet/Commands/New/NewCommandParser.cs

@@ -170,7 +168,7 @@ private static CliTemplateEngineHost CreateHost(
            { "prefs:language", preferredLang },
            { "dotnet-cli-version", Product.Version },
            { "RuntimeFrameworkVersion", new Muxer().SharedFxVersion },
-            { "NetStandardImplicitPackageVersion", new FrameworkDependencyFile().GetNetStandardLibraryVersion() },
+            { "NetStandardImplicitPackageVersion", new FrameworkDependencyFile().GetNetStandardLibraryVersion() ?? "" },


@baronfel should verify this change as being accurate. It is also possible the templating engine is wrong and it should accept a Dictionary<string, string?> but I don't have the depth to say which is better.

The GetNetStandardLibraryVersion method shouldn't return a nullable - it's one of that hard-coded, required things and we should blow up if it's not present.

Happy to change that to throw on failure if you think it's the best course.

jaredpar · 2025-05-07T19:48:52Z

src/Cli/dotnet/Commands/Run/RunProperties.cs

 using Microsoft.Build.Execution;
 using Microsoft.DotNet.Cli.Utils;

 namespace Microsoft.DotNet.Cli.Commands.Run;

-internal record RunProperties(string? RunCommand, string? RunArguments, string? RunWorkingDirectory)
+internal record RunProperties(string RunCommand, string? RunArguments, string? RunWorkingDirectory)


This is another place where having arguments, specifically RunArguments, be potentially null feels potentially off and "" may work.

jaredpar · 2025-05-07T19:50:06Z

src/Cli/dotnet/Commands/Solution/Add/SolutionAddCommand.cs

@@ -39,7 +38,7 @@ private static bool IsSolutionFolderPathInDirectoryScope(string relativePath)

    public SolutionAddCommand(ParseResult parseResult) : base(parseResult)
    {
-        _fileOrDirectory = parseResult.GetValue(SolutionCommandParser.SlnArgument);
+        _fileOrDirectory = parseResult.GetValue(SolutionCommandParser.SlnArgument)!;


The ParseResult type in general does not work well with nullable analysis because GetValue is hard coded to return T?. One possible solution is add a GetValueOrThrow extension method. Happy to make that change if it's desired.

We chatted about this briefly but the main gap here is that the S.CL library enforces the arity of an option/argument but there's no way for that Arity to influence the return type of this method.

jaredpar · 2025-05-07T19:52:22Z

src/Cli/dotnet/Commands/Solution/Add/SolutionAddCommand.cs

            var projectPlatform = projectInstancePlatforms.FirstOrDefault(
-                platform => platform.Replace(" ", string.Empty) == solutionPlatform.Replace(" ", string.Empty), projectInstancePlatforms.FirstOrDefault());
+                platform => platform.Replace(" ", string.Empty) == solutionPlatform.Replace(" ", string.Empty), projectInstancePlatforms.FirstOrDefault()!);


Lacking the ! on the default value (last argument) the compiler will end up inferring FirstOrDefault<string?> not FirstOrDefault<string>. That is because the default value directly infers the value for T and FirstOrDefault is null returning. Don't know enough about the semantics here to suggest a more null friendly pattern.

jaredpar · 2025-05-07T19:52:55Z

src/Cli/dotnet/Commands/Test/IPC/Models/FileArtifactMessages.cs

@@ -5,4 +5,4 @@ namespace Microsoft.DotNet.Cli.Commands.Test.IPC.Models;

 internal sealed record FileArtifactMessage(string? FullPath, string? DisplayName, string? Description, string? TestUid, string? TestDisplayName, string? SessionUid);

-internal sealed record FileArtifactMessages(string? ExecutionId, string InstanceId, FileArtifactMessage[] FileArtifacts) : IRequest;
+internal sealed record FileArtifactMessages(string? ExecutionId, string? InstanceId, FileArtifactMessage[] FileArtifacts) : IRequest;


It seems like most record types in the sdk default to putting ? on all parameters. I suspect that many could, and should, be unannotated.

jaredpar · 2025-05-07T19:53:21Z

src/Cli/dotnet/Commands/Test/Models.cs

@@ -114,7 +115,7 @@ internal sealed record TestModule(RunProperties RunProperties, string? ProjectFu

 internal sealed record Handshake(Dictionary<byte, string>? Properties);

-internal sealed record CommandLineOption(string? Name, string? Description, bool? IsHidden, bool? IsBuiltIn);
+internal sealed record CommandLineOption(string Name, string Description, bool? IsHidden, bool? IsBuiltIn);


Same comment about record declarations

jaredpar · 2025-05-07T19:53:48Z

src/Cli/dotnet/Commands/Test/Terminal/ExceptionFlattener.cs

@@ -27,7 +25,7 @@ internal static FlatException[] Flatten(string? errorMessage, Exception? excepti
        IEnumerable<Exception?> aggregateExceptions = exception switch
        {
            AggregateException aggregate => aggregate.Flatten().InnerExceptions,
-            _ => [exception?.InnerException],
+            _ => [exception?.InnerException!],


This is known issue in collection expression null inference that I needed to work around. @cston

jaredpar · 2025-05-07T19:54:16Z

src/Cli/dotnet/Commands/Test/TestCommandParser.cs

@@ -29,13 +28,13 @@ internal static class TestCommandParser
    {
        Description = CliCommandStrings.CmdTestCaseFilterDescription,
        HelpName = CliCommandStrings.CmdTestCaseFilterExpression
-    }.ForwardAsSingle(o => $"-property:VSTestTestCaseFilter={SurroundWithDoubleQuotes(o)}");
+    }.ForwardAsSingle(o => $"-property:VSTestTestCaseFilter={SurroundWithDoubleQuotes(o!)}");


Most of the ! is related to the ParseResult issue mentioned above.

jaredpar · 2025-05-09T17:59:07Z

src/Cli/dotnet/Commands/Solution/Add/SolutionAddCommand.cs

            project.AddProjectConfigurationRule(new ConfigurationRule(BuildDimension.Platform, "*", solutionPlatform, projectPlatform));
        }

        foreach (var solutionBuildType in solution.BuildTypes)
        {
            var projectBuildType = projectInstanceBuildTypes.FirstOrDefault(
-                buildType => buildType.Replace(" ", string.Empty) == solutionBuildType.Replace(" ", string.Empty), projectInstanceBuildTypes.FirstOrDefault());
+                buildType => buildType.Replace(" ", string.Empty) == solutionBuildType.Replace(" ", string.Empty), projectInstanceBuildTypes.FirstOrDefault()!);


Same FirstOrDefault pattern as above.

jaredpar · 2025-05-09T18:00:03Z

src/Cli/dotnet/Commands/Workload/WorkloadCommandBase.cs

+        if (nugetPackageDownloader is not null)
+        {
+            IsPackageDownloaderProvided = true;
+            PackageDownloader = nugetPackageDownloader;
+        }
+        else
+                      {

            
+            IsPackageDownloaderProvided = false;
+            PackageDownloader = new NuGetPackageDownloader.NuGetPackageDownloader(
+                TempPackagesDirectory,
+                filePermissionSetter: null,
+                new FirstPartyNuGetPackageSigningVerifier(),
+                nugetLogger,
+                Reporter,
+                restoreActionConfig: RestoreActionConfiguration,
+                verifySignatures: VerifySignatures,
+                shouldUsePackageSourceMapping: true);
+        }


This is just a simple expansion of the original code that is more NRT friendly. Could also change this to be a tuple return but this seemed more readable.

jaredpar · 2025-05-09T18:01:47Z

src/Cli/dotnet/CommonOptions.cs

            key.Equals("DOTNET_MSBUILD_SDK_RESOLVER_CLI_DIR", StringComparison.InvariantCultureIgnoreCase)
                ? null
                : Environment.GetEnvironmentVariable(key));
        var ridFileName = "NETCoreSdkRuntimeIdentifierChain.txt";
+        var sdkPath = dotnetRootPath is not null ? Path.Combine(dotnetRootPath, "sdk") : "sdk";


This is a case where the NRT analysis seems to have spotted a legitimate bug in the code. In the case that dotnetRootpath is null then the Path.Combine calls below will throw. I changed this to just use "sdk" as a relative path but possible that a throws here is a better idea.

jaredpar · 2025-05-09T18:02:50Z

src/Tasks/Microsoft.NET.Build.Tasks/GetPackagesToPrune.cs

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

-#nullable disable warnings
+#nullable disable


For most of the build tasks there was a single ? annotation. Instead of fully annotating the build tasks, which is hundreds of changes, I just removed the errant annotation and disable NRT in the file. Seems much better to approach those as a separate change where a good deal of thought is put into how NRT should be structured in tasks.

jaredpar · 2025-05-09T18:05:07Z

@marcpopMSFT, @MiYanni, @baronfel

This is a follow up change where I removed all uses of #nullable disable warnings from the sdk. This puts the SDK into a pretty clean state with respect to nullable annotations. To make this easier to review I put review comments in GitHub anywhere that I made a non-mechanical change to the code.

baronfel · 2025-05-09T18:32:38Z

src/Cli/dotnet/CommonOptions.cs

        .AddCompletions(CliCompletion.RunTimesFromProjectFile);

    public static Option<string> LongFormRuntimeOption =
        new DynamicForwardedOption<string>("--runtime")
        {
            HelpName = RuntimeArgName
-        }.ForwardAsMany(RuntimeArgFunc)
+        }.ForwardAsMany(RuntimeArgFunc!)


why did you need the ! for these? The static function is known, isn't it?

public static Option<T> ForwardAsMany<T>(this ForwardedOption<T> option, Func<T?, IEnumerable<string>> format);

This requires that the delegate have a string? parameter, presumably more fallout of ParseResult design. The RuntimeArgFunc though depends on having a non-null parameter. The parameter is immediately passed to GetArchFromRid and that throws on null.

Forgind · 2025-05-13T19:50:25Z

I believe that if you do not have a dotnet.config in your current directory (or a parent/grandparent/etc. directory), this change means that you cannot use this version of the SDK with debug bits because of a new Debug.Assert.

Forgind · 2025-05-13T19:51:01Z

Process terminated.
Assertion failed.
dotnetConfigPath is not null
at Microsoft.DotNet.Cli.Commands.Test.TestCommandParser.GetTestRunnerName() in C:\GitHub\sdkMain\src\Cli\dotnet\Commands\Test\TestCommandParser.cs:line 167
at Microsoft.DotNet.Cli.Commands.Test.TestCommandParser.ConstructCommand() in C:\GitHub\sdkMain\src\Cli\dotnet\Commands\Test\TestCommandParser.cs:line 212
at Microsoft.DotNet.Cli.Commands.Test.TestCommandParser..cctor() in C:\GitHub\sdkMain\src\Cli\dotnet\Commands\Test\TestCommandParser.cs:line 155
at Microsoft.DotNet.Cli.Commands.Test.TestCommandParser.GetCommand() in C:\GitHub\sdkMain\src\Cli\dotnet\Commands\Test\TestCommandParser.cs:line 159
at Microsoft.DotNet.Cli.Parser..cctor() in C:\GitHub\sdkMain\src\Cli\dotnet\Parser.cs:line 62
at Microsoft.DotNet.Cli.Parser.get_Instance() in C:\GitHub\sdkMain\src\Cli\dotnet\Parser.cs:line 214
at Microsoft.DotNet.Cli.Program.ProcessArgs(String[] args, TimeSpan startupTime, ITelemetry telemetryClient) in C:\GitHub\sdkMain\src\Cli\dotnet\Program.cs:line 128
at Microsoft.DotNet.Cli.Program.Main(String[] args) in C:\GitHub\sdkMain\src\Cli\dotnet\Program.cs:line 77

jaredpar added 3 commits May 7, 2025 11:56

progress

594dd03

progress

3d58d3f

progress

28a929e

Copilot AI review requested due to automatic review settings May 7, 2025 19:46

Copilot AI reviewed May 7, 2025

View reviewed changes

src/Cli/dotnet/Commands/Run/RunProperties.cs Show resolved Hide resolved

jaredpar commented May 7, 2025

View reviewed changes

jaredpar added 2 commits May 9, 2025 10:37

Merge remote-tracking branch 'origin/main' into nrt2

d7a7b0f

more

1cfd469

jaredpar changed the title ~~Remove nullable disable warnings part 1~~ Remove nullable disable warnings May 9, 2025

jaredpar commented May 9, 2025

View reviewed changes

baronfel reviewed May 9, 2025

View reviewed changes

baronfel approved these changes May 12, 2025

View reviewed changes

MiYanni approved these changes May 12, 2025

View reviewed changes

jaredpar merged commit 4a7b8ec into main May 12, 2025
30 checks passed

jaredpar deleted the dev/jaredpar/nrt1 branch May 12, 2025 21:23

Forgind mentioned this pull request May 13, 2025

Remove bad assert #48922

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove nullable disable warnings #48846

Remove nullable disable warnings #48846

Remove nullable disable warnings #48846

Remove nullable disable warnings #48846

Conversation

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

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

Choose a reason for hiding this comment