Fix S1168 FP: Does not respect nullable annotations

[Corniel]

Fixes #6878
Fixes #6825
Add a check on the return type to check nullability.

[Corniel]

I know the current solution is not something that should be shipped. In SonarAnalyzer.CFG the following code exists:

// Copyright (c) Tunnel Vision Laboratories, LLC. All Rights Reserved.
// Licensed under the MIT License. See LICENSE in the project root for license information.

namespace StyleCop.Analyzers.Lightup
{
    public readonly record struct NullabilityInfo
    {
       // ..
    }
}

Obviously that should become available in SonarAnalyzer.Common too. The question is how? Furthermore I noticed that in common there is the Helpers namespace with TypeHelper, and there is the Extensions namespace with NamedTypeInfoExtensions. Should that be updated?

[martin-strecker-sonarsource]

see also #6878

[martin-strecker-sonarsource]

see also #6825

[martin-strecker-sonarsource]

Obviously that should become available in SonarAnalyzer.Common too.

We track the implementation for this in #6825 The idea is to do something

Should that be updated?

Yes. Something like in TypeInfoExtensions needs to be created:

public static class ITypeSymbolExtensions
{
    public NullableAnnotation NullableAnnotation(this ITypeSymbol symbol) { }
}

It is a bit easier as TypeInfoExtensions.Nullability(this TypeInfo typeInfo) but still needs the enum to enum conversion. Maybe one of the LightupHelpers.CreateSyntaxPropertyAccessor already does what we need (there is an overload that takes a type and calls Convert).

[Corniel]

It is a bit easier as TypeInfoExtensions.Nullability(this TypeInfo typeInfo) but still needs the enum to enum conversion. Maybe one of the LightupHelpers.CreateSyntaxPropertyAccessor already does what we need (there is an overload that takes a type and calls Convert).

I could not find anything outside the CFG project. (I searched for NullableAnnotation). Do you know how in the past the LightupHelpers have been extended?

[martin-strecker-sonarsource]

You can have a look at this PR #6099. It shouldn't be as complicated in this case. A simple call to LightupHelpers.CreateSyntaxPropertyAccessor should return a proper Func if I'm not mistaken.

[Corniel]

@martin-strecker-sonarsource I've changed the implementation of the LightupHelpers.CreateSyntaxPropertyAccessor; when the return type of the function and property type don't match, I added a Expression.Convert() to the body. That should not break existing usage of this method, and makes our lives easier.

[martin-strecker-sonarsource]

Nice work, @Corniel! I just have some minor suggestions.

[martin-strecker-sonarsource]

👍That is exactly what we need here!

[martin-strecker-sonarsource]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[martin-strecker-sonarsource]

This line is too long.

[martin-strecker-sonarsource]

LGTM. @Corniel thank you for your contribution!

[martin-strecker-sonarsource]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[sonarcloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

83.3% Coverage
0.0% Duplication

[Corniel]

@martin-strecker-sonarsource Due to the updated indenting, some existing lines popped up that are not covered by existing unit tests. What to do?

1. Remove lines
2. Add tests
3. Ignore coverage

[martin-strecker-sonarsource]

3. Ignore coverage

The coverage is only about the non-whitespace changes and this case: _ => null, We don't care about covering this case as it is just defensive coding.

[Corniel]

3. Ignore coverage

The coverage is only about the non-whitespace changes and this case: _ => null, We don't care about covering this case as it is just defensive coding.

I fully agree, but it is not my call, therefor the question.

[martin-strecker-sonarsource]

Peach validation:

• aspnet-core: ~30 FPs fixed
• dnspy: ~40FPs fixed
• dnspy:extensions: ~120 FPs fixed
• dotnet-runtim: ~140 FPs fixed
• elasticsearch: 5 FPs fixed
• embedio: 3 FPs fixed
• hot-chocolate: 18 FPs fixed
• nodatime: 2 FPs fixed
• nunit: 4 FPs fixed

That's an impressive list @Corniel Thank you for the contribution!