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.
  - After some discussion, we opted to avoid the use of ! entirely. In this PR, I've converted several uses of string.IsNullOrEmpty to a simple null and length check. The only reason this is required is because we build for both .NET (core) and .NET Framework. The Framework version of this method does not have the attribute that makes it aware of being a null-check. This also applies to string.IsNullOrWhitespace. However, my resolution for this was to just add an additional null check prior to the method call, which isn't costly.
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
Some shared source files were able to have the #nullable enable removed as all projects they were shared with became nullable-enabled projects

Going forward

After some discussion, it seems like the forward strategy will be:

Have #nullable enable at the top of every shared source file (follow-up PR)
Progressively convert projects by adding <Nullable>enable</Nullable> to them
When all projects are converted, add <Nullable>enable</Nullable> to the main Directory.Build.props, remove it from every .csproj, and remove #nullable enable from the shared source files

…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.

I think this makes it a little harder to review, but it otherwise seems positive

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

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

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

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

src/Common/CliFolderPathCalculatorCore.cs

src/Common/EnvironmentVariableNames.cs

src/Tasks/Common/ConflictResolution/PlatformManifestReader.cs

src/Tasks/Common/FileUtilities.MetadataReader.cs

src/Cli/Microsoft.DotNet.Cli.Utils/BuiltInCommand.cs

src/Cli/Microsoft.DotNet.Cli.Utils/DotDefaultPathCorrector.cs

MiYanni · 2024-11-22T21:44:32Z

src/Common/PathUtilities.cs

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


This is a shared source file. However, the 2 projects that use it are already nullable enabled projects.

RikkiGibson · 2024-11-22T23:30:49Z

Roslyn uses some msbuild config to suppress nullable warnings on non-netcore targets.

This allows running nullable analysis against only the netcore versions of core library methods like string.IsNullOrEmpty, where they are annotated for nullable analysis for work properly with them. This would eliminate the need to use ! in some cases in this PR.

See property DisableNullableWarnings and its usage: https://github.com/dotnet/roslyn/blob/fe4c98aafc5bd0fe0455b9de381930fe89646ab4/eng/targets/Imports.targets#L43

Possibly, dotnet/sdk may wish to do something similar.

src/Tasks/Common/ConflictResolution/ConflictResolver.cs

MiYanni · 2024-11-23T00:01:05Z

@RikkiGibson I'm going through and manually resolving all ! uses. It'll be in my next commit for this PR. I decided just to use manual null and Length checks or combine a null check with string.IsNullOrWhitespace since double checking null is minimal impact. If the uses of those checks is really problematic, we could consider that option later down the line.

…Other minor adjustments based on PR feedback.

jaredpar · 2024-11-27T14:42:59Z

Have #nullable enable at the top of every shared source file (follow-up PR)

Another approach to consider is:

Put #nullable disable at the top of every source file that is not currently part of project with <Nullable>enable</Nullable>.
Put <Nullable>enable</Nullable> in Directory.Build.props and remove it in every project.

This approach has a couple of advantages:

Makes it clear at a glance whether a source file has nullability enabled or not. Unless it's suppressed at the top of the file then it's enabled
This is the only mass refactoring you need to make on the project to enable nullability. No more large PRs like this because after this change you opt in at the file level simply by deleting the #nullable disable at the top of the file. That is a very incremental change that is easier to review
Makes it easier to deal with files shared amongst multiple projects because every file is always compiled in the same nullability mode

Forgind · 2024-12-04T18:46:44Z

src/Cli/Microsoft.DotNet.Cli.Utils/BuiltInCommand.cs

@@ -55,7 +55,7 @@ public CommandResult Execute()
                    // Reset the Reporters to the new Console Out and Error.
                    Reporter.Reset();

-                    if (!string.IsNullOrEmpty(_workingDirectory))
+                    if (_workingDirectory is not null && _workingDirectory.Length != 0)


Why is this necessary?

string.IsNullOrEmpty does not have the proper attributes in .NET Framework to be recognized as a null-check. Therefore, these cause errors since we build for both TFMs. There are several other options to avoid this problem, but are all generally workarounds. On the day I removed all ! uses (called the null-forgiving operator), I also converted all the string.IsNullOrEmpty to have the functional equivalent check like this (for the projects being converted in this PR).

Have you considered only running nullable checks when targeting core? Highly recommend in multi targeted projects.

I have but I'm not doing this as a project. This PR was something I started as a side-thing in March and only revived the PR as there was renewed interest in making the repo nullability compatible. If someone on my team wants to make a decision on the direction of this, then that's fine. All I've done it make an old PR buildable and express the problems around nullability to @marcpopMSFT.

You've suggested some completely reasonable ways to handle this stuff but I don't even know if this is a project we want to get done yet or what the timeframe would be. Only "decision" we made was we didn't want to use the null-forgiving operator (!) when it can be avoided. This repo doesn't have strong, identifiable standards for coding as it is an amalgamation of many different repos.

Forgind · 2024-12-04T18:47:58Z

src/Cli/Microsoft.DotNet.Cli.Utils/DangerousFileDetector.cs

-                        Type iismType = Type.GetTypeFromCLSID(new Guid(CLSID_InternetSecurityManager));
-                        internetSecurityManager = (IInternetSecurityManager)Activator.CreateInstance(iismType);
+                        Type? iismType = Type.GetTypeFromCLSID(new Guid(CLSID_InternetSecurityManager));
+                        if (iismType is not null)


Should there be an error if it is null?

If it is null right now, it'll throw a null-reference exception. When discussing doing this work with @marcpopMSFT, he accepted that we would have functional changes when adding null-checks like this. For this situation, if it is null, I'm assuming that zone check will fail so this method will return false.

src/Cli/Microsoft.DotNet.Cli.Utils/DotDefaultPathCorrector.cs

src/Cli/Microsoft.DotNet.Cli.Utils/EnvironmentProvider.cs

Forgind · 2024-12-04T18:51:05Z

src/Cli/Microsoft.DotNet.Cli.Utils/FrameworkDependencyFile.cs

@@ -111,7 +109,7 @@ internal static bool TryGetMostFitRuntimeIdentifier(

        private DependencyContext CreateDependencyContext()
        {
-            using (Stream depsFileStream = File.OpenRead(_depsFilePath))
+            using (Stream depsFileStream = File.OpenRead(_depsFilePath ?? string.Empty))


This looks like an error would be better

I didn't opt to adding any additional logic, especially throwing exceptions, as I originally didn't intend on changing ANY logic. However, we decided that we didn't want to use the null-forgiving operator (!), so the only changes I've made in logic is checking for null and letting the program "continue" if it was null, as you saw above in your one comment.

Before this, it would've failed (with a NRE) if _depsFilePath were somehow null, so this actually does change the logic to make it fail in a different way. I think "_depsFilePath is unexpectedly null" is more clear and accurate than "could not find file ''", which might have multiple causes.

I commented on multiple like this where I think the error you end up getting from ?? string.Empty (or equivalent) is less clear and actionable than what the error otherwise would've been. I think there's a lot of potential benefit from nullable (and hence from a PR like this), but I also think it's important to not miss the woods for the trees: bringing an error forward so we can act on it is ideal. Hiding it to make an analyzer happy is not. Though I agree I'm proposing adding more than you did, I think it would make it clearer rather than less clear. Does that make sense?

Yes, @marcpopMSFT is aware the logic will change in unintended ways, but we have not opted on redesigning the code around nullability, as that is a major project. We're only making the code handle nullability support. If that support results in bugs, that is an understood risk and not in the scope of this PR.

Your concerns are exactly the concerns I brought up with Marc, so I'm on your side for this. But I'm not supposed to invest the effort in making things better, only making them compatible. It would take way too long to write this code properly to handle nullability in the design of it. There are thousands of situations just like this and we don't have a way to make them less error prone without knowing the entire codebase. As it stands, I'm doing the minimum viable changes to make the code compatible with nullability, which would still take a while. It is not intended to make the code better. Changing a potential null-ref error into a different error is completely reasonable for this PR.

The guidance I got from other teams was to make the switch over first and then start investing in changing hte logic. They did indicate that changing the logic to provide better error handling and messaging was a desired outcome but this switch is already going to be quite challenging. I wonder if it makes sense to create an issue with a list of all the spots we want to revisit later?

I don't know if it is possible to create that list as it would be a majority of the changes. Most uses of string in the codebase fall into this category, and there are likely thousands of strings.

Forgind · 2024-12-04T19:03:20Z

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

        {
            var installType = GetWorkloadInstallType(sdkFeatureBand, dotnetDir);
            var architecture = RuntimeInformation.ProcessArchitecture.ToString();

-            if (installType == InstallType.FileBased)
+            if (dotnetDir is not null && installType == InstallType.FileBased)


I think dotnetDir is explicitly non-null here?

According to the call stack, the SdkDirectoryWorkloadManifestProvider has the _sdkOrUserLocalPath field which is nullable. So that gets passed into this as dotnetDir. Unless you change the design of all of these classes to be string vs string?, then you can't really undo this stuff.

Forgind · 2024-12-04T19:04:46Z

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

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


I think this makes it a little harder to review, but it otherwise seems positive

Forgind · 2024-12-04T19:06:04Z

src/Tasks/Common/ConflictResolution/PackageRank.cs


-                if (preferredPackageId.Length != 0)
+                if (preferredPackageId is not null && preferredPackageId.Length != 0)


I think you can do this?

Suggested change

if (preferredPackageId is not null && preferredPackageId.Length != 0)

if (preferredPackageId?.Length != 0)

If you change it to that, that doesn't guarantee that preferredPackageId is non-null. If it is null, it won't equal 0, and therefore, you couldn't use it in the if-statement, since it is used as an index. You need to check it is non-null explicitly before it is used.

Good point. How about preferredPackageId?.Length > 0? I don't think null > 0

src/Tasks/Common/ConflictResolution/PlatformManifestReader.cs

src/WebSdk/Publish/Tasks/Tasks/Kudu/KuduDeploy.cs

# Conflicts: # src/WebSdk/Publish/Tasks/Tasks/MsDeploy/CreateMSDeployScript.cs

Forgind

Given the scope of this change, I think this looks good to me. The only change I'd still like to see is the WorkloadSdkResolver change to make it never return null. Thanks for making all these changes! I can see them helping us avoid nullability errors going forward 🙂

MiYanni · 2024-12-10T22:05:12Z

@jaredpar We discussed it. What we're going to do is:

Merge this PR as is
Create another PR that reverts the conversions of string.IsNullOrEmpty and additional check for string.IsNullOrWhitespace that I did in this commit and adds the workaround for disabling the null-check warning on .NET Framework
Lastly, we can have another PR that changes the way we handle the other projects going forward, potentially using your #nullable disable strategy

@Forgind I'll adjust the WorkloadSdkResolver on a different follow-up PR.

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

src/WebSdk/Publish/Tasks/MsDeploy/CommonUtility.cs Outdated Show resolved Hide resolved

333fred reviewed Nov 18, 2024

View reviewed changes

src/RazorSdk/Tool/Microsoft.NET.Sdk.Razor.Tool.csproj Outdated Show resolved Hide resolved