Repository navigation
Description
Problem
The code under src/ holds about 50 null-forgiving operators (!). Each one tells the compiler that a value is not null, on the strength of an invariant that no type states. The invariant then lives in the head of whoever wrote the code. The compiler no longer checks it, and a reader has to rebuild it from code elsewhere.
An example is PinResolution.Target. PinWriting.Moving returns only the pins whose Target is not null. ToolPinUpdater (twice), PinWriting.TargetsOf, and SdkPinWriter then read pin.Target!. The invariant is stated once, as a filter, and assumed four times. A change to Moving would break the four sites at run time, with no warning at compile time.
No rule in .claude/rules/ covers the operator, so new code adds more of them.
PR #407 met the problem in GitHubApiClient.Describe, which read HttpRequestMessage.RequestUri?.AbsoluteUri. The null branch could never run, and Codecov counted the line as partially covered. The fix in that PR keeps the URL that the client built, instead of reading it back through a nullable property.
Where the operators are
As of 04ca2d0, by area:
bv deps, about 20 sites:PinResolution.Target,ItemType,VersionText,ProjectAssetsFile,GroupCaption, and theVersionandReasonof an override decision. Most of them need a type that carries the non-null value.SelfVersionService, three sites. Two of them read aNuGetVersionthat a separateboolsays is not null, out of sight of flow analysis. The third is aJsonNodecast.Path.GetDirectoryName(...)!, six sites, inBuildvana.Tool,Buildvana.Sdk.Tasks, andBuildvana.Core.Testing.Buildvana.Core.JsonandBuildvana.Core.Configuration, about 19 sites:Utf8JsonReader.GetString(),JsonNodeindexers, and reflection results.- Others:
null!in theoutparameters and a tuple ofGitService, two sites inCommandRegistry, and one each inGitignorePatternandConstantValueParser.
[\w\)\]]![\.;,\)\]\s] finds them with ripgrep. The pattern may miss a few forms.
Proposal
- Add a section on the null-forgiving operator to
.claude/rules/common/csharp-style-guide.md. The section bans the operator, and says what to do instead:- Let a type state the invariant: a non-nullable member, or a type that carries the non-null value next to the rest.
- Keep a value the code built, instead of reading it back through a nullable API.
- Write a check that flow analysis follows, such as
if (NuGetVersion.TryParse(text, out var version)), instead of aboollocal. - State the exceptions, if any survive the work below.
- Remove every operator under
src/, in commits grouped by area.
Acceptance criteria
csharp-style-guide.mdhas the section on the null-forgiving operator.- No null-forgiving operator is left under
src/, except the ones the section admits, each with a comment that states the invariant. - The build has zero warnings, and the tests pass.
- No changelog entry: the change is internal.
Open questions
- Should
tests/follow the rule too? A test often reads a value it has just asserted to be not null. - Is there an analyzer that reports the operator, so that the rule needs no review to enforce it?
Activity