S2259 FP: Lifted operator results in null value in value type comparison

[aureole82]

Repro steps

public class TokenService
{
    private AccessTokenModel _lastTokenModel;
    public DateTime? LastIssue { get; private set; }

    public string GetAccessToken()
    {
        if (LastIssue + _lastTokenModel?.ExpiresIn > DateTime.Now)
            return _lastTokenModel.Token; // S2259: '_lastTokenModel' is null on at least one execution path.

        ...
    }
}

public class AccessTokenModel
{
    public TimeSpan ExpiresIn { get; set; }
    public string Token { get; set; }
}

Expected behavior

No complaints because: https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/nullable-value-types#lifted-operators

Actual behavior

S2259: '_lastTokenModel' is null on at least one execution path.

Related information

Microsoft Visual Studio Professional 2019 Version 16.9.4
Installed Version: Professional
SonarLint for Visual Studio 4.33.1.28913
.

[aureole82]

This is originally created by me here: https://community.sonarsource.com/t/s2259-false-positive-xy-is-null-on-at-least-one-execution-path/41172

[pavel-mikula-sonarsource]

Hi @aureole82,

Can you please provide more explanation why do you think that it's a False Positive?

// This is usage of lifted operator that you've send a link to. 
// That makes this + operator and IF condition save and so issue is raised on this line.
// Usage of _lastTokenModel? explicitly states, that you expect _lastTokenModel to be null at this line sometimes.
if (LastIssue + _lastTokenModel?.ExpiresIn > DateTime.Now)   
    // At this line _lastTokenModel could be null. You explicitly expect it to be null sometimes. 
    // So _lastTokenModel.Token can lead to null reference exception as I see it.
    return _lastTokenModel.Token; 

This looks like a True Positive to me.

[aureole82]

Hi @aureole82,

Can you please provide more explanation why do you think that it's a False Positive?

// This is usage of lifted operator that you've send a link to. 
// That makes this + operator and IF condition save and so issue is raised on this line.
// Usage of _lastTokenModel? explicitly states, that you expect _lastTokenModel to be null at this line sometimes.
if (LastIssue + _lastTokenModel?.ExpiresIn > DateTime.Now)   
    // At this line _lastTokenModel could be null. You explicitly expect it to be null sometimes. 
    // So _lastTokenModel.Token can lead to null reference exception as I see it.
    return _lastTokenModel.Token; 

This looks like a True Positive to me.

Please state a setting where the if clause will be evaluated to true AND _lastTokenModel is null. I don't see any.

[pavel-mikula-sonarsource]

Ah, ok. Now I see it. So it's based on fact that (DateTime)null is translated into DateTime.MinValue and that is less than DateTime.Now.

That's a tricky chain of constraints. But I can confirm it as a False Positive now.

[aureole82]

I don't know if (DateTime?)null is translated into DateTime.MinValue (and actually I don't believe it) but it doesn't matter! Please read the link I posted before:

For the comparison operators <, >, <=, and >=, if one or both operands are null, the result is false; otherwise, the contained values of operands are compared.

Do you actually know what a Nullable<T> is and how it behaves? Even Visual Studio indicates it:

[martin-strecker-sonarsource]

Related C# spec documents for lifting and null handling by operators:

https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/nullable-value-types#lifted-operators

https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/operators/boolean-logical-operators#nullable-boolean-logical-operators

See also #5918 (comment) for where to add this logic.

[pavel-mikula-sonarsource]

Related to nullable => MMF-2401 => demilestoning for now

[martin-strecker-sonarsource]

We investigated the control flow graph with the following results:

if (arg?.Length == 0)
{
    Tag(""If"", arg);
}
else
{
    Tag(""Else"", arg);
}

        digraph "RoslynCfg" {
subgraph "cluster_1" {
label = "LocalLifetime region, Locals: isNull, isObject"
subgraph "cluster_2" {
label = "LocalLifetime region, Captures: #0"
subgraph "cluster_3" {
label = "LocalLifetime region, Captures: #1"
cfg0_block2 [shape=record label="{BLOCK #2|0# FlowCaptureOperation: #1 / IdentifierNameSyntax: arg|1# ParameterReferenceOperation / IdentifierNameSyntax: arg|##########|## BranchValue ##|0# IsNullOperation / IdentifierNameSyntax: arg|1# FlowCaptureReferenceOperation: #1 / IdentifierNameSyntax: arg|##########}"]
cfg0_block3 [shape=record label="{BLOCK #3|0# FlowCaptureOperation: #0 / MemberBindingExpressionSyntax: .Length|1# ConversionOperation / MemberBindingExpressionSyntax: .Length|2# PropertyReferenceOperation / MemberBindingExpressionSyntax: .Length|3# FlowCaptureReferenceOperation: #1 / IdentifierNameSyntax: arg|##########}"]
}
cfg0_block4 [shape=record label="{BLOCK #4|0# FlowCaptureOperation: #0 / IdentifierNameSyntax: arg|1# DefaultValueOperation / IdentifierNameSyntax: arg|##########}"]
cfg0_block5 [shape=record label="{BLOCK #5|## BranchValue ##|0# BinaryOperation / BinaryExpressionSyntax: arg?.Length == 0|1# FlowCaptureReferenceOperation: #0 / ConditionalAccessExpressionSyntax: arg?.Length|1# ConversionOperation / LiteralExpressionSyntax: 0|2# LiteralOperation / LiteralExpressionSyntax: 0|##########}"]
}
cfg0_block1 [shape=record label="{BLOCK #1|0# SimpleAssignmentOperation / VariableDeclaratorSyntax: isNull = null|1# LocalReferenceOperation / VariableDeclaratorSyntax: isNull = null|1# ConversionOperation / LiteralExpressionSyntax: null|2# LiteralOperation / LiteralExpressionSyntax: null|##########|0# SimpleAssignmentOperation / VariableDeclaratorSyntax: isObject = new object()|1# LocalReferenceOperation / VariableDeclaratorSyntax: isObject = new object()|1# ObjectCreationOperation / ObjectCreationExpressionSyntax: new object()|##########}"]
cfg0_block6 [shape=record label="{BLOCK #6|0# ExpressionStatementOperation / ExpressionStatementSyntax: Tag(\"If\", arg);|1# InvocationOperation: Tag / InvocationExpressionSyntax: Tag(\"If\", arg)|2# ArgumentOperation / ArgumentSyntax: \"If\"|3# LiteralOperation / LiteralExpressionSyntax: \"If\"|2# ArgumentOperation / ArgumentSyntax: arg|3# ConversionOperation / IdentifierNameSyntax: arg|4# ParameterReferenceOperation / IdentifierNameSyntax: arg|##########}"]
cfg0_block7 [shape=record label="{BLOCK #7|0# ExpressionStatementOperation / ExpressionStatementSyntax: Tag(\"Else\", arg);|1# InvocationOperation: Tag / InvocationExpressionSyntax: Tag(\"Else\", arg)|2# ArgumentOperation / ArgumentSyntax: \"Else\"|3# LiteralOperation / LiteralExpressionSyntax: \"Else\"|2# ArgumentOperation / ArgumentSyntax: arg|3# ConversionOperation / IdentifierNameSyntax: arg|4# ParameterReferenceOperation / IdentifierNameSyntax: arg|##########}"]
cfg0_block8 [shape=record label="{BLOCK #8|0# ExpressionStatementOperation / ExpressionStatementSyntax: Tag(\"End\", arg);|1# InvocationOperation: Tag / InvocationExpressionSyntax: Tag(\"End\", arg)|2# ArgumentOperation / ArgumentSyntax: \"End\"|3# LiteralOperation / LiteralExpressionSyntax: \"End\"|2# ArgumentOperation / ArgumentSyntax: arg|3# ConversionOperation / IdentifierNameSyntax: arg|4# ParameterReferenceOperation / IdentifierNameSyntax: arg|##########}"]
}
cfg0_block0 [shape=record label="{ENTRY #0}"]
cfg0_block9 [shape=record label="{EXIT #9}"]
cfg0_block1 -> cfg0_block2
cfg0_block2 -> cfg0_block3 [label="Else"]
cfg0_block2 -> cfg0_block4 [label="WhenTrue"]
cfg0_block3 -> cfg0_block5
cfg0_block4 -> cfg0_block5
cfg0_block0 -> cfg0_block1
cfg0_block5 -> cfg0_block6 [label="Else"]
cfg0_block5 -> cfg0_block7 [label="WhenFalse"]
cfg0_block6 -> cfg0_block8
cfg0_block7 -> cfg0_block8
cfg0_block8 -> cfg0_block9
}

The work on (the upcoming) MMF-2401 will be based on this.

[martin-strecker-sonarsource]

Collection of community posts about S2259 and nullable value types and null coalescing ?.:

• The assumption about the incorrect operation of the code verification system - Clean Code / Report False-positive / False-negative... - Sonar Community
• Using c# null conditional operator breaks the rule analysis - SonarLint / Visual Studio - Sonar Community
• False positive null pointer C# - Clean Code / Report False-positive / False-negative... - Sonar Community
• C# S2259 (is null on at least one execution path) - false negatives - Clean Code / Report False-positive / False-negative... - Sonar Community
• C# S2259 (Null pointers should not be dereferenced) - inconsistent reporting / false negatives - SonarLint - Sonar Community

[pavel-mikula-sonarsource]

ToDo:

• Remove workaround from FP S2259: Suppress warnings for lifted operator results in null value in value type comparison #6117
• Implement properly