Add nullable annotations to NuGet.LibraryModel - #5417
#5417Add nullable annotations to NuGet.LibraryModel#5417zivkan merged 4 commits intodevNuGet/NuGet.Client:devfrom dev-zivkan-nullable-NuGet.LibraryModelNuGet/NuGet.Client:dev-zivkan-nullable-NuGet.LibraryModelCopy head branch name to clipboard
Conversation
also fix bug in null annotations in product code that the tests found
| // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
|
||
| #if NETFRAMEWORK || NETSTANDARD | ||
| #if !NET5_0_OR_GREATER |
There was a problem hiding this comment.
One of the test projects targeting netcoreapp3.1 started failing once I added the RequiredModiferAttributes file, hence I needed to fix this, as this IsExernalInit attribute was added in .NET 5.
| public int CompareTo(DownloadDependency other) | ||
| public int CompareTo(DownloadDependency? other) | ||
| { | ||
| if (other == null) return 1; |
There was a problem hiding this comment.
this code would previously throw if other was null. But, the interface for IComparable<T> shows an expectation of handling nulls, and the docs explain that nulls should be sorted after non null values. We can change this to throw an ArgumentNullException if we want, but to avoid a compiler warning/error, we have to keep the ? in the method signature, since IComparable<T> does.
| if (!string.IsNullOrEmpty(flags)) | ||
| { | ||
| var splitFlags = flags.Split(new char[] { ',' }, StringSplitOptions.RemoveEmptyEntries) | ||
| var splitFlags = flags!.Split(new char[] { ',' }, StringSplitOptions.RemoveEmptyEntries) |
There was a problem hiding this comment.
Two lines up, !string.ISNullOrEmpty prevents flags being null here. In case you didn't review my other PRs enabling nullable in other projects, only the .NET 5 BCL has annotated string.IsNullOrEmpty, hence on .NET Standard and .NET Framework (and theoretically .NET Core 3.1 and earlier, for our tests), the compiler doesn't know, hence we have to tell it with the !
| public bool Equals(Library? x, Library? y) | ||
| { | ||
| if (x is null && y is null) return true; | ||
| if (x is null || y is null) return false; |
There was a problem hiding this comment.
This is also a change in behaviour. Previously it would throw a null reference exception. But I think this is better.
| public int CompareTo(LibraryIdentity other) | ||
| public int CompareTo(LibraryIdentity? other) | ||
| { | ||
| if (other is null) return 1; |
There was a problem hiding this comment.
another example of code that previously threw NullReferenceException, violating the IComparable<T> contract.
| } | ||
|
|
||
| [SetsRequiredMembers] | ||
| public LibraryDependency(LibraryRange libraryRange) : this() |
There was a problem hiding this comment.
Should we add XML doc comments indicating that specifying a null libraryRange will throw an ArgumentNullException? Or is that overkill here?
There was a problem hiding this comment.
I'm not seeing the text within any exception docs, including in other XMLDoc that already exists, in VS 2022 Int Preview, public preview, or GA versions 😕
There was a problem hiding this comment.
Interesting, I expected the tooltip of the Exception to show the text but yeah I can't figure out how the text is displayed in the IDE. It must only be for docs? 🤷
| } | ||
|
|
||
| [SetsRequiredMembers] | ||
| public LibraryDependency(LibraryRange libraryRange) : this() |
There was a problem hiding this comment.
Interesting, I expected the tooltip of the Exception to show the text but yeah I can't figure out how the text is displayed in the IDE. It must only be for docs? 🤷
Note: this will not be merged until the dev branch targets V6.9
Bug
Fixes: NuGet/Home#12889
Regression? No
Description
Enable nullable in NuGet.LibraryModel, and its test project.
This PR uses the
requiredkeyword for the first time in our code. This is a C#11 feature, and the compiler needs attributes that are only available in .NET 7's BCL. Therefore, I had to copy these attributes from the .NET runtime repo into this repo. For more info on the impact on customers, please see the linked issue. I wrote a long description in there. Note that this does create an API breaking change, but there's little reason for other repos to create instances of these classes, except maybe for tests.Since I needed to add a new file in
build\Shared, I noticed that a bunch of other files in that directory were not listed in the solution's virtual directory, so I added all of them, not just the one new file I added.Otherwise, this was the easiest project to enable nullable on so far, but the project is mostly models, not business logic, and has a lot less code than other projects. From memory, the nullable checks didn't find any bugs, and other than the
requiredkeyword, I don't remember any other breaking changes.PR Checklist
PR has a meaningful title
PR has a linked issue.
Described changes
Tests
Documentation