Add nullable annotations to NuGet.Frameworks - #5155
#5155Add nullable annotations to NuGet.Frameworks#5155zivkan merged 4 commits intodevNuGet/NuGet.Client:devfrom dev-zivkan-NuGet.Frameworks-nullableNuGet/NuGet.Client:dev-zivkan-NuGet.Frameworks-nullableCopy head branch name to clipboard
Conversation
| public int CompareFrameworks(NuGetFramework? x, NuGetFramework? y) | ||
| { | ||
| if (x is null && y is null) return 0; | ||
| if (x is null) return -1; | ||
| if (y is null) return 1; |
There was a problem hiding this comment.
This method, and CompareUsingPrecedence below, would have previously thrown NullReferenceExceptions if either of the inputs were null. However, in versions of the BCL where IComparable is annotated, Compare methods allow nulls. So, in the spirit of "least surprise", I thought we should also support nulls. But, it is a behaviour change.
| { | ||
| // parse the version as a normal dot delimited version | ||
| _ = Version.TryParse(versionString, out version); | ||
| version = Version.Parse(versionString); |
There was a problem hiding this comment.
Previously, TryParse would end up setting version to a default value (null), but it doesn't make sense to me that our method named GetVersion should return null. Especially when versionString is allowed to be null, in which time we return a default, non-null value. There, this is a change in behaviour since now it can throw.
Same thing for the else branch's TryParse to Parse change, 13 lines below.
There was a problem hiding this comment.
This does seem a little dangerous to start throwing now but the method is not a Try method... My only worry is that if this one breaking change causes problems, we'd need to revert this whole thing. But I'm not sure if its worth keeping the existing behavior, then sending another PR to adjust the behavior. It all comes down to if anyone is broken by it and how many people are affected...
There was a problem hiding this comment.
My only worry is that if this one breaking change causes problems, we'd need to revert this whole thing.
When we fix a 3 year old bug, we don't revert the commit that introduced the bug, we just fix the bug. Commits don't even need to be that old. Specific example, when fixing a CPM bug, we don't revert CPM. So, I don't see why we'd have to revert this PR, we could just change this one method. no?
There was a problem hiding this comment.
Specific example, when fixing a CPM bug, we don't revert CPM. So, I don't see why we'd have to revert this PR, we could just change this one method. no?
I just recently tried to fix a bug in static graph-based restore and ended up having to revert the fix, bringing back the older, less impactful bug. I also had to revert some refactorings that I had to add back later and the whole thing was a mess. I'm wondering if there's a bug late in a ship cycle and its too risky to fix forward, we'd be forced to revert. We're early in the ship cycle so I suppose that reduces risk. This pull request is just adding nullability and reverting it would not be the same as completely backing out functionality like CPM.
My point is that if the overwhelming majority of this change is non-risky and non-breaky, we can consider a different pull request to follow-up on this one that makes a breaking change in the event that later on we'd need to make a revert due to circumstances out of our control. If it doesn't break anyone then the whole thing is moot, if it breaks and we can fix it, then its also moot. If it breaks and we have to revert, I'd hate to lose the rest of this change. I'll leave it up to you in this case, just trying to help everyone learn from my recent mistake.
There was a problem hiding this comment.
I forgot to mention that Intellisense says this method has zero references within our own repo. So, this method is only part of the NuGet Client SDK, so we're unlikely to get any feedback until after we ship the new version and someone using the API upgrades to our new package version. But considering this method returns System.Version, not a Version type from NuGet.Versioning, it's a really pointless public API to have. Customers should just use System.Version.Parse() directly. This really is an excellent candidate for a public API we should delete at some point.
But for this reason, I'm really not concerned about the breaking change. Such low risk with such an easy mitigation.
| { | ||
| // parse the version as a normal dot delimited version | ||
| _ = Version.TryParse(versionString, out version); | ||
| version = Version.Parse(versionString); |
There was a problem hiding this comment.
This does seem a little dangerous to start throwing now but the method is not a Try method... My only worry is that if this one breaking change causes problems, we'd need to revert this whole thing. But I'm not sure if its worth keeping the existing behavior, then sending another PR to adjust the behavior. It all comes down to if anyone is broken by it and how many people are affected...
| } | ||
|
|
||
| public static Version GetVersion(string versionString) | ||
| public static Version GetVersion(string? versionString) |
There was a problem hiding this comment.
nit: The nesting of the elses can be removed now as well which makes this method faster.
| { | ||
| // parse the version as a normal dot delimited version | ||
| _ = Version.TryParse(versionString, out version); | ||
| version = Version.Parse(versionString); |
There was a problem hiding this comment.
Specific example, when fixing a CPM bug, we don't revert CPM. So, I don't see why we'd have to revert this PR, we could just change this one method. no?
I just recently tried to fix a bug in static graph-based restore and ended up having to revert the fix, bringing back the older, less impactful bug. I also had to revert some refactorings that I had to add back later and the whole thing was a mess. I'm wondering if there's a bug late in a ship cycle and its too risky to fix forward, we'd be forced to revert. We're early in the ship cycle so I suppose that reduces risk. This pull request is just adding nullability and reverting it would not be the same as completely backing out functionality like CPM.
My point is that if the overwhelming majority of this change is non-risky and non-breaky, we can consider a different pull request to follow-up on this one that makes a breaking change in the event that later on we'd need to make a revert due to circumstances out of our control. If it doesn't break anyone then the whole thing is moot, if it breaks and we can fix it, then its also moot. If it breaks and we have to revert, I'd hate to lose the rest of this change. I'll leave it up to you in this case, just trying to help everyone learn from my recent mistake.
Bug
Fixes: NuGet/Home#12570
Regression? No
Description
#if NET40_CLIENTwas used. Delete it, since we stopped building for .NET 4.0 a while agoArgumentNullExceptionon some public APIs where nulls are not expected/allowed, but wasn't previously being enforced (would have thrownNullReferenceException, so not has a better error experience).PR Checklist
PR has a meaningful title
PR has a linked issue.
Described changes
Tests
Documentation