Skip to content

Remove the null-forgiving operators from src/ #408

Closed

Description

@rdeago

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 the Version and Reason of an override decision. Most of them need a type that carries the non-null value.
  • SelfVersionService, three sites. Two of them read a NuGetVersion that a separate bool says is not null, out of sight of flow analysis. The third is a JsonNode cast.
  • Path.GetDirectoryName(...)!, six sites, in Buildvana.Tool, Buildvana.Sdk.Tasks, and Buildvana.Core.Testing.
  • Buildvana.Core.Json and Buildvana.Core.Configuration, about 19 sites: Utf8JsonReader.GetString(), JsonNode indexers, and reflection results.
  • Others: null! in the out parameters and a tuple of GitService, two sites in CommandRegistry, and one each in GitignorePattern and ConstantValueParser.

[\w\)\]]![\.;,\)\]\s] finds them with ripgrep. The pattern may miss a few forms.

Proposal

  1. 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 a bool local.
    • State the exceptions, if any survive the work below.
  2. Remove every operator under src/, in commits grouped by area.

Acceptance criteria

  • csharp-style-guide.md has 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

  1. added theissue type on Oct 21, 2026
  2. added
    refactor[issue/PR] requests / implements a refactor of existing code without affecting functionality.
    on Oct 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    refactor[issue/PR] requests / implements a refactor of existing code without affecting functionality.

    Type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions