Add nullable annotations to NuGet.Common by zivkan · Pull Request #5333 · NuGet/NuGet.Client

zivkan · Jul 25, 2023

Bug

Fixes: NuGet/Home#12775

Regression? Last working version:

Description

Add <Nullable>enable</Nullable> to NuGet.Common and test project.
Annotate all public APIs
Fix all remaining compiler errors

PR Checklist

PR has a meaningful title
PR has a linked issue.
Described changes
Tests
- Automated tests added
- OR
- Test exception: no functionality changes, existing tests are sufficient
- OR
- N/A
Documentation
- Documentation PR or issue filled
- OR
- N/A

zivkan · Jul 25, 2023

 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

+// BCL annotations on Environment.GetEnvironmentVariable makes this file difficult to annotate in .NET 5+
+#nullable disable


This file nearly made me give up. .NET Core 1.0 didn't have Environment.GetFolderPath, so this file has #if to compile different code for .NET Framework and everything else. On .NET Framework it uses GetFolderPath, which is annotated in .NET 5+ to not return nulls. But in our .NET Standard (and .NET (Core) if we add those TFMs) it will use Environment.GetEnvironmentVariable, which can return null.

However, .NET Core 2.0 added GetFolderPath, so I think that porting the .NET Standard code to use that. However, it's significantly higher risk than the rest of this PR, so I want to do it in a separate PR in case it needs to be reverted.

zivkan · Jul 25, 2023

+                {
+                    throw new ArgumentException(paramName: nameof(telemetryEvent), message: "Property 'Name' must not be null");
+                }
                _telemetryActivity = NuGetTelemetryService?.StartActivity(telemetryEvent.Name);


This exposes an API design problem. We have code creating instances of TelemetryEvent with a null name, but this code will throw a NullReferenceException if such an instance is used here.

zivkan · Jul 25, 2023

            }
+            else
+            {
+                Debug.Fail("Looking at all references to the static Create methods, I don't think this code path is possible.");


I didn't feel comfortable deleting the if != null statement, but looking at usage it should never be null. I thought maybe this Debug.Fail will give us confidence in a few weeks if none of us hit this debug point, so we can make the change later.

Sounds good to me.

martinrrm

I didn't review the tests but I trust they are good since CI is passing.

aortiz-msft · Jul 28, 2023

Reviewed until src/NuGet.Core/NuGet.Common/Migrations/Migration1.cs. Might need an extra day to go through the rest.

aortiz-msft · Aug 1, 2023

+            string? destFileDirectory = Path.GetDirectoryName(destFilePath);
+            if (destFileDirectory is null)
+            {
+                throw new ArgumentException(paramName: destFilePath, message: "Path.GetDirectoryName(destFilePath) returned null");


Path.GetDirectoryName(destFilePath) returned null

Does this need to be localized? At the minimum, we should have a string defined somewhere so that the exception text is guaranteed to be the same for folks who are parsing the text. Same for all other instances.

I don't expect any customers to ever see this message, so no, I don't think it needs to be localized. It's only feasible if someone using NuGet.Common (part of the Client SDK) as part of their app calls the API with bad input (or if someone modifying NuGet starts calling this in a way that is buggy). Frankly, I think it's a bad API, NuGet shouldn't be in the business of proving and supporting trivial file utility APIs.

I also think it's a bad idea to parse exception messages. .NET Core changed some exception messages from what they were on the .NET Framework (I think ArgumentNullException's message is an example), and exception messages that are localized will have unexpected results when running on machines not using the same language as the developer who wrote code trying to parse the exception message. Even for tests this is a bad idea, since I don't want to exclude people running their devbox in something other than en-US from being able to run tests locally.

I'm okay with the answer. Suggestion for message: "Not a valid file path".

aortiz-msft

🕐

nkolev92

Looks pretty good. Nicely done.
TelemetryEvent is really the only type where I wish we can do better.

nkolev92 · Aug 1, 2023

-        private static ILogger _instance;
+        private static ILogger? _instance;

        public static ILogger Instance


tbh this optimization seems pointless

What optimization? I was annotating exactly what was implemented

the lazy creation of the null logger. It's not really heavy so no need to delay it.
My bad, I should've pointed out that it was the existing code.

Up to you whether you change it or not.

ah, since I had to annotate hundreds of APIs, I didn't pay attention to most of what I was annotating. Trying to resolve all the compiler errors, and minimize risk of regression by avoiding unnecessary code changes.

aortiz-msft · Aug 2, 2023

+                    if (firstParentDirectory is not null)
                    {
-                        isCaseInsensitive &= result;
+                        isCaseInsensitive &= CheckIfFileSystemIsCaseInsensitive(firstParentDirectory);


isCaseInsensitive &= CheckIfFileSystemIsCaseInsensitive(firstParentDirectory);

Just for my own learning, why wouldn't it be sufficient to check any one of these paths? I.e. why not return immediately?

I don't know.

Technically, the whole thing is flawed, because individual filesystems can be case sensitive or not. On Windows I can configure a specific partition to be case sensitive, even though Windows' default is case insensitive filesystems. I expect there are filesystems on Linux where you can enable insensitivity. But I imagine that the number of people who change the defaults is very close to zero, and NuGet wouldn't be the only software that breaks when those defaults are changed.

aortiz-msft · Aug 2, 2023


-            var tempPath = GetTempFilePath(Path.GetDirectoryName(destFilePath));
+            string? destFileDirectory = Path.GetDirectoryName(destFilePath);
+            if (destFileDirectory is null)


Should we also raise an exception if the destFileDirectory is an Empty string? https://learn.microsoft.com/en-us/dotnet/api/system.io.path.getdirectoryname?view=net-7.0

It looks like an exception will be thrown either way (either by us, or by the BCL). The difference is that the ArgumentException's ParamName value will be a different name, a name that doesn't exist in our API. But given our code is open source and this API only has a single path parameter, it's trivial to discover which parameter had an input error.

So, it's still the caller's responsibility to pass in sensible input. It doesn't cause the API to produce incorrect results. Since pushing changes resets approvals, unfortunately I don't feel motivated to add such a minor change (same goes for reverting my "see if this fixes E2E tests" commit)

you could do a follow up PR :)

aortiz-msft

zivkan requested a review from a team as a code owner July 25, 2023 16:31

zivkan commented Jul 25, 2023

View reviewed changes

martinrrm previously approved these changes Jul 26, 2023

View reviewed changes

jeffkl previously approved these changes Jul 31, 2023

View reviewed changes

aortiz-msft reviewed Aug 1, 2023

View reviewed changes

Comment thread src/NuGet.Core/NuGet.Common/PathUtil/PathUtility.cs Outdated

aortiz-msft reviewed Aug 1, 2023

View reviewed changes

Comment thread test/NuGet.Core.Tests/NuGet.Common.Test/SynchronizationTest.cs

aortiz-msft requested changes Aug 1, 2023

View reviewed changes

nkolev92 reviewed Aug 1, 2023

View reviewed changes

zivkan added 3 commits August 2, 2023 10:44

Annotate NuGet.Common

2ad6141

Annotate NuGet.Common.Test

58602e9

PR feedback

3582fc9

zivkan dismissed stale reviews from jeffkl and martinrrm via 3582fc9 August 2, 2023 08:44

zivkan force-pushed the dev-zivkan-nullable-NuGet.Common branch from d246c7b to 3582fc9 Compare August 2, 2023 08:44

zivkan requested review from aortiz-msft and nkolev92 August 2, 2023 08:45

See if this fixes E2E tests

bbff759

nkolev92 approved these changes Aug 2, 2023

View reviewed changes

aortiz-msft reviewed Aug 2, 2023

View reviewed changes

aortiz-msft approved these changes Aug 2, 2023

View reviewed changes

zivkan merged commit 267b938 into dev Aug 3, 2023

zivkan deleted the dev-zivkan-nullable-NuGet.Common branch August 3, 2023 08:55

zivkan mentioned this pull request Aug 26, 2025

enable nullability in PackageItemViewModelTests #6596

Merged

3 tasks

Search code, repositories, users, issues, pull requests...

Uh oh!

Conversation

zivkan commented Jul 25, 2023

Bug

Description

PR Checklist

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!

Choose a reason for hiding this comment

Uh oh!

martinrrm left a comment

Choose a reason for hiding this comment

Uh oh!

aortiz-msft commented Jul 28, 2023

Uh oh!

aortiz-msft Aug 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

aortiz-msft left a comment

Choose a reason for hiding this comment

Uh oh!

nkolev92 left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aortiz-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aortiz-msft Aug 1, 2023 •

edited

Loading