Add CS8629 suppression and member access expression matching to IsNotNullAssertionSuppressor

[Copilot]

The IsNotNullAssertionSuppressor was missing CS8629 ("Nullable value type may be null") and couldn't match member access expressions passed to Assert.That().

var value = new { Id = id }; // id is int?
await Assert.That(value.Id).IsNotNull();
int x = value.Id.Value; // CS8629 not suppressed

Two root causes:

• CS8629 not in supported diagnostics — only CS8600/CS8602/CS8604/CS8618 were handled
• Expression matching limited to simple identifiers — Assert.That(value.Id) couldn't match against value.Id in subsequent usage because GetIdentifierFromNode reduced everything to the root IdentifierNameSyntax

Changes

• Add CS8629 to IsNullabilityWarning() and SupportedSuppressions
• Replace GetIdentifierFromNode → GetTargetExpression returning ExpressionSyntax to preserve full member access chains
• Add ExpressionsMatch with recursive semantic symbol comparison for member access chains, direct symbol comparison for identifiers
• Add 4 tests: simple int?, negative case, anonymous type member access, named type member access

Original prompt

---

This section details on the original issue you should resolve

<issue_title>[Feature]: Suppress nullable warning</issue_title>
<issue_description>### Problem Statement

Hi,

Assert.That().IsNotNull() don't suppress nullable warnings for some code after the Assert.
The following code gives a warning CS8629 "Nullable value type may be null." when the Id is guaranteed to be not null after the Assert.

Proposed Solution

[Test]
[Arguments(1)]
public async Task CheckNull(int? id)
{
    var value = new
    {
        Id = id
    };

    await Assert.That(value.Id)
                .IsNotNull();

    await Assert.That(value.Id.Value)
                .EqualTo(0);
}

Alternatives Considered

For example with an extra if ... is not null, the compiler can suppress the warning :

[Test]
[Arguments(1)]
public async Task CheckNull(int? id)
{
    var value = new
    {
        Id = id
    };

    await Assert.That(value.Id)
                .IsNotNull();

    if (value.Id is not null)
    {
        await Assert.That(value.Id.Value)
                    .EqualTo(0);
    }
}

Feature Category

Other

How important is this feature to you?

Nice to have - would improve my experience

Additional Context

No response

Contribution

• I'm willing to submit a pull request for this feature</issue_description>

<agent_instructions>Check the nullable code suppressor for not null assertions - any reason why it wouldn't always work?</agent_instructions>

Comments on the Issue (you are @copilot in this section)

@thomhurst There is meant to be an analyzer to suppress the nullable warnings - maybe it's not always firing for some reason

• Fixes [Feature]: Suppress nullable warning #5200

---

📱 Kick off Copilot coding agent tasks wherever you are with GitHub Mobile, available on iOS and Android.

[claude]

Code Review: Add CS8629 suppression and member access expression matching

Overall, this is a well-scoped fix that correctly addresses both root causes identified in the issue. The semantic symbol comparison approach in ExpressionsMatch is the right way to handle this.

What's Good

• Semantic symbol comparison in ExpressionsMatch is correct and handles aliasing/renaming properly. Comparing by syntax text alone (e.g., string matching on value.Id) would be fragile; using SymbolEqualityComparer is the right approach.
• Recursive member access chain comparison correctly distinguishes obj1.Id from obj2.Id by recursively comparing receivers — a simple member-symbol comparison would produce false positives here.
• Test coverage is solid: simple nullable, negative case, anonymous type, named type — the four cases cover the meaningful scenarios for CS8629.

Concerns / Issues

1. GetTargetExpression fallback may return a sub-expression prematurely

_ => node.DescendantNodesAndSelf()
    .OfType<ExpressionSyntax>()
    .FirstOrDefault(e => e is IdentifierNameSyntax or MemberAccessExpressionSyntax)

DescendantNodesAndSelf() returns nodes in pre-order (root before children), so this should correctly return an outer MemberAccessExpressionSyntax before any inner IdentifierNameSyntax. However, if the node type is something unexpected (e.g., ConditionalAccessExpressionSyntax for value?.Id), this fallback will return the inner value identifier rather than the full value?.Id. This isn't a regression — the old code had the same limitation — but it's worth noting that ConditionalAccessExpressionSyntax is not handled at all. Consider adding it to both the switch and ExpressionsMatch as a future improvement.

2. WasAssertedNotNull only searches MethodDeclarationSyntax

var containingMethod = targetExpression.FirstAncestorOrSelf<MethodDeclarationSyntax>();

This means the suppressor won't work inside lambda expressions or local functions — both common in async test code. For example:

Func<Task> act = async () => {
    await Assert.That(value.Id).IsNotNull();
    int x = value.Id.Value; // NOT suppressed — lambda scope not found
};

A more robust approach would walk ancestors matching MethodDeclarationSyntax | LocalFunctionStatementSyntax | AnonymousFunctionExpressionSyntax and use the innermost one. This is pre-existing but worth addressing since test code often uses lambdas for ThrowsException wrappers.

3. Mixed-type ExpressionsMatch silently returns false

// For simple identifiers...
if (assertArgument is IdentifierNameSyntax && targetExpression is IdentifierNameSyntax) { ... }

// For member access chains...
if (assertArgument is MemberAccessExpressionSyntax && targetExpression is MemberAccessExpressionSyntax) { ... }

return false; // mixed types

This is semantically correct (e.g., asserting id should not suppress a warning on wrapper.Id). However, the silent return false for all other combinations makes the code harder to reason about. A brief comment explaining that mismatched types are intentionally not matched would improve readability.

4. ElementAccessExpressionSyntax not handled

Array/indexer patterns like Assert.That(arr[0]).IsNotNull() aren't handled, but this is out of scope for this PR.

Minor

SupportedSuppressions.FirstOrDefault(s => s.SuppressedDiagnosticId == diagnostic.Id) in Suppress() is an O(n) LINQ search on every suppression event. A static readonly Dictionary<string, SuppressionDescriptor> would be cleaner and O(1), though the list is only 5 entries so this is negligible in practice.

Summary

The implementation is correct and solves the reported issue. The concerns above are either pre-existing limitations or minor suggestions. Recommend merging after considering the lambda/local-function scoping issue (#2), which could be a meaningful gap in real test scenarios.

[claude]

Follow-up Review

This is a follow-up to the previous claude[bot] review. All actionable concerns have been addressed:

Previous Concerns — Status

✅ Concern #2 (Lambda/local function scoping) — Fixed. The WasAssertedNotNull method now walks ancestors matching MethodDeclarationSyntax | LocalFunctionStatementSyntax | AnonymousFunctionExpressionSyntax, so assertions inside lambdas and local functions are correctly found. Two new tests (Suppresses_Inside_Lambda, Suppresses_Inside_Local_Function) validate this.

✅ Concern #3 (Silent return false in ExpressionsMatch) — Fixed. A comment now explains the intentional behaviour: "Mismatched expression types are intentionally not matched — asserting id should not suppress warnings on wrapper.Id or vice versa."

✅ Minor (O(n) FirstOrDefault in Suppress) — Fixed. SuppressionsByDiagnosticId dictionary provides O(1) lookup and Descriptors array keeps SupportedSuppressions construction clean.

⚠️ Concern #1 (ConditionalAccessExpressionSyntax value?.Id) — Acknowledged as pre-existing; not addressed here, which is acceptable scope-wise.

⚠️ Concern #4 (ElementAccessExpressionSyntax for indexers) — Out of scope; still not handled, which is fine.

New Code Quality

The new tests are well-structured and cover all the meaningful scenarios. The recursive ExpressionsMatch + SymbolsMatch design is clean and semantically correct. No new issues introduced.

Verdict

All previously raised actionable issues have been resolved. The implementation is correct, well-tested, and ready to merge.