feat: suppress nullability warnings after an IsNotNull expectation - #1021
Merged
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The implementation is well-scoped and backed by extensive suppressor-focused tests, with only a minor test naming clarity nit outstanding.
Pull request overview
Adds a new Roslyn DiagnosticSuppressor (IsNotNullSuppressor) to suppress specific C# nullability warnings (CS8600/CS8602/CS8604/CS8629) when a preceding Expect.That(subject).IsNotNull()-style expectation guarantees the same local/parameter subject was evaluated and not written to in between. This extends the aweXpect.Analyzers package to better align compiler nullability diagnostics with runtime-verified expectations, and introduces dedicated suppressor test infrastructure.
Changes:
- Add
IsNotNullSuppressorand suppression descriptors/justification resources for nullability warnings after not-null expectations. - Add a new C# suppressor verifier and a comprehensive suppressor test suite covering control-flow, scoping, and mutation cases.
- Bump
Microsoft.CodeAnalysis.*.Testingpackages to1.1.4to support suppressed diagnostic assertions and avoid Roslyn 4.11 suppression issues.
File summaries
| File | Description |
|---|---|
| Tests/aweXpect.Analyzers.Tests/Verifiers/CSharpSuppressorVerifier.cs | New verifier tailored to validating compiler warnings and suppressor behavior under nullable-enabled compilation. |
| Tests/aweXpect.Analyzers.Tests/IsNotNullSuppressorTests.cs | New test suite validating when nullability warnings should/shouldn’t be suppressed. |
| Source/aweXpect.Analyzers/Rules.cs | Adds suppression descriptors for the supported nullability warning IDs. |
| Source/aweXpect.Analyzers/Resources.resx | Adds localized justification text for suppressions. |
| Source/aweXpect.Analyzers/Resources.Designer.cs | Regenerated designer to expose the new resource string. |
| Source/aweXpect.Analyzers/IsNotNullSuppressor.cs | Implements the suppressor logic and matching rules for safe suppression. |
| Directory.Packages.props | Updates Roslyn analyzer testing package versions to 1.1.4. |
Review details
Files not reviewed (1)
- Source/aweXpect.Analyzers/Resources.Designer.cs: Generated file
- Files reviewed: 6/7 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Test Results 23 files - 27 23 suites - 27 9m 42s ⏱️ + 1m 46s Results for commit 739b154. ± Comparison against base commit f37799d. This pull request removes 3245 and adds 3236 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
🚀 Benchmark ResultsDetails
|
Contributor
👽 Mutation ResultsaweXpectDetails
The final mutation score is NaN%Coverage Thresholds: high:80 low:60 break:0aweXpect.CoreDetails
The final mutation score is NaN%Coverage Thresholds: high:80 low:60 break:0 |
vbreuss
force-pushed
the
feat/is-not-null-suppressor
branch
from
September 5, 2026 11:47
4940798 to
e3ff5f6
Compare
Add `IsNotNullSuppressor`, a `DiagnosticSuppressor` that silences CS8600, CS8602, CS8604 and CS8629 for a subject that a preceding `Expect.That(subject).IsNotNull()` verified to be not null. The suppression only applies when the expectation is guaranteed to have been evaluated for the same subject: it must be a sequential statement preceding the warning in an enclosing block, no branching may separate the two and the subject must not be written to in between. The analyzer testing packages are bumped to 1.1.4, because 1.1.2 throws on any suppressed diagnostic against Roslyn 4.11 and lacks `WithIsSuppressed`.
The suppressor silenced nullability warnings in cases where the subject could still be null: - a loop reaches the usage again, so a write at the end of an iteration invalidates an expectation from outside the loop - `Expect.ThatAny` only requires any of its expectations to be met, just like the already excluded `Or` - the warning for `subject.Member` was matched against an expectation for `subject` - a write via `this._subject` or a deconstruction was not detected - a property could return a different value on each access Subjects are now restricted to local variables and parameters, which also covers writes through a lambda that captures them. Additionally, `IsNotNullOrEmpty` and `IsNotNullOrWhiteSpace` are accepted as expectations that guarantee a not-null subject. The result type is not used to detect them, because it also replaces the nullable subject type for expectations that a null subject does fulfil, e.g. `IsNotEmpty` on a string or `IsNotEqualTo` on a collection.
vbreuss
force-pushed
the
feat/is-not-null-suppressor
branch
from
September 5, 2026 11:47
e3ff5f6 to
c6dc424
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The suppressor’s control-flow invalidation is stricter than described (treats non-branching statements as “branching”), and write-detection incorrectly treats in arguments as writes, leading to incorrect/overly-restrictive behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- Source/aweXpect.Analyzers/Resources.Designer.cs: Generated file
- Files reviewed: 6/7 changed files
- Comments generated: 2
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
vbreuss
enabled auto-merge (squash)
September 5, 2026 11:53
vbreuss
disabled auto-merge
September 5, 2026 11:54
There was a problem hiding this comment.
🔵 Needs a closer look
The write-tracking logic misses ++/-- mutations, which can incorrectly allow suppression after the subject was modified between the expectation and the warned usage.
Review details
Files not reviewed (1)
- Source/aweXpect.Analyzers/Resources.Designer.cs: Generated file
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
Source/aweXpect.Analyzers/IsNotNullSuppressor.cs:274
- WritesTo(...) currently treats assignments and ref/out arguments as writes, but it misses ++/-- operations (prefix/postfix unary), which also mutate the subject and should invalidate a preceding IsNotNull expectation per the stated rules ("subject must not be written to in between"). This can lead to suppressing a nullability warning even though the subject was modified between expectation and usage.
- Files reviewed: 6/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
github-actions Bot
added a commit
that referenced
this pull request
Sep 5, 2026
…er an `IsNotNull` expectation (#1021) by Valentin Breuß
github-actions Bot
added a commit
that referenced
this pull request
Sep 5, 2026
…er an `IsNotNull` expectation (#1021) by Valentin Breuß
Contributor
|
This is addressed in release v2.37.0. |
This was referenced Sep 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add
IsNotNullSuppressor, aDiagnosticSuppressorthat silences CS8600, CS8602, CS8604 and CS8629 for a subject that a precedingExpect.That(subject).IsNotNull()verified to be not null.The suppression only applies when the expectation is guaranteed to have been evaluated for the same subject: it must be a sequential statement preceding the warning in an enclosing block, no branching may separate the two and the subject must not be written to in between.
The analyzer testing packages are bumped to 1.1.4, because 1.1.2 throws on any suppressed diagnostic against Roslyn 4.11 and lacks
WithIsSuppressed.