NET-1872 S3900 FP: with nullable context enabled

Problem stated on the community: Validation of not nullable reference type demanded (S3900)

When nullable context is enabled (feature which was introduced in C# 8 / .NET Framework 4.8), or when [NotNull] attributes are used, rule S3900 should assume that reference type parameters are non nullable unless explicitly marked as nullable (e.g. string?).

Reproducer project with both .NET Framework 4.8 and .NET 6: https://github.com/andrei-epure-sonarsource/s3900-nullable-FP/tree/main

SonarCloud S3900 issues: https://sonarcloud.io/project/issues?id=2021-12-23-S3900&open=AX3oLi7fKxs11Dyvb5nV&resolved=false&rules=csharpsquid%3AS3900&types=CODE_SMELL

Having in mind that the rule will be rewritten in MMF-2401 , which will be coming in the next few months, we should still aim at reducing the noise with the current analyzer when nullable context is enabled or when netstandard System.Diagnostics.CodeAnalysis.NotNullAttribute is used.

On the public API boundary, the nullable annotation specifies what the API expects in terms of nullability. If a parameter is specified as being expected to be non-nullable the "NotNull" check is especially important. In the dotnet-runtime repo, it is mandatory to do such checks. See e.g. the Path.Combine API's implementation:

https://github.com/dotnet/runtime/blob/e579ccb60acb044453f2cf1907482b165658488e/src/libraries/System.Private.CoreLib/src/System/IO/Path.cs#L344-L373

APIs that accept a nullable reference type are handling the null case gracefully instead. see e.g.

https://github.com/dotnet/runtime/blob/e579ccb60acb044453f2cf1907482b165658488e/src/libraries/System.Private.CoreLib/src/System/IO/Path.cs#L320-L327

This is true for real public APIs where the clients of the API are unknown. If the public API is not published but is part of a closed solution, any clients consuming the API are known. In that case, raising S3900 on public APIs creates a lot of FPs. The analyzer can not distinguish between these two scenarios, but the user can. Our conclusion is to create a new rule complementing S3900. The new rule is for the real public API case only (Working title: "Non-nullable parameters of public methods should be validated against null"). Once we do have the rule specified and implemented, we will change S3900 to behave like in the known clients scenario like so:

#nullable enable

M1(SampleClass sample) // Expecting a non-nullable reference type
{
   // S3900: FP. We will remove any S3900 issues in the future
   // New rule: Raise an issue here and only here. This rule will be opt-in and not enabled by default.
}

M2(SampleClass? sample)  // Expecting a nullable reference type
{
   // S3900: Triggers as before
  // New rule: Doesn't trigger
}

The new rule issue is #7035.

This distinction between the two rules seems suitable if nullable reference types are enabled (as default in new projects since C# 10).

I haven't tested the Roslyn behaviour of the unreasonable M3([NotNull] SampleClass? sample), but for sure you should also inspect the nullable attribute family and define which rules should trigger,

And what are you plans, if nullable reference types are diabled? Always S3900 as before?

Internal ticket NET-1872

@pavel-mikula-sonarsource

@pavel-mikula-sonarsource Thanks for updating this recently. I'm encouraged that some progress is being made as it's been quite a while since this was opened.

In the interim, is there a recommendation to remove S3900 altogether?

I have a ruleset that includes it because it's relevant for legacy projects, so I still want the suggestion.. but not when adding != null checks then result in another warning that I'm checking for null on a non-nullable reference type.

@DSmith-VERX

@DSmith-VERX I'd recommend to activate the rule on projects where it makes sense. Generally, not all rules are suitable for all projects.

The previous message is just technical migration. The ticket is not scheduled yet. So it basically only says that it's still alive. S3900 will generally require more attention and we're considering splitting the rule into two.

@pavel-mikula-sonarsource

@pavel-mikula-sonarsource I think null value handling makes sense on every project. Is there another rule that will tell you to validate the nullable ref types when they actually are nullable?
Creating a whole separate quality profile for this one difference seems like overkill, but I can do it. Unfortunately, I work on many large legacy solutions where not every project in the solution uses the same framework version or nullable setting though.

The other rule is the proposal in #7035 that Martin mentioned above in this thread.

Navigation Menu

Repository navigation

NET-1872 S3900 FP: with nullable context enabled #5217

1 remaining item

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

NET-1872 S3900 FP: with nullable context enabled #5217

Description

Activity

martin-strecker-sonarsource commented on Apr 4, 2023

milbrandt commented on Apr 4, 2023

1 remaining item

pavel-mikula-sonarsource commented on Jun 6, 2025

DSmith-VERX commented on Jun 6, 2025

pavel-mikula-sonarsource commented on Jun 10, 2025

DSmith-VERX commented on Jun 10, 2025

pavel-mikula-sonarsource commented on Jun 10, 2025

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions