Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix S6966 FP: Methods with different signatures should not trigger this rule. #9265

Open
ahusseini opened this issue May 9, 2024 · 2 comments · May be fixed by #9326
Open

Fix S6966 FP: Methods with different signatures should not trigger this rule. #9265

ahusseini opened this issue May 9, 2024 · 2 comments · May be fixed by #9326
Assignees
Labels
Area: C# C# rules related issues. Sprint: Hardening Fix FPs/FNs/improvements Type: False Positive Rule IS triggered when it shouldn't be.
Projects
Milestone

Comments

@ahusseini
Copy link

ahusseini commented May 9, 2024

Description

S6966 is being triggered on methods that do not match in method signature. For example, in the MongoDB driver, Find and FindAsync are not related. Find returns IFindFluent for building chained mongo requests, while FindAsync returns IAsyncCursor. The following should not be triggering S6966.

UnrelatedThing Find(string s, int i = 0) or UnrelatedThing Find(string s)
Task<string> FindAsync(string s)

Only string Find(string s) should trigger this rule.

CleanShot 2024-05-09 at 15 17 41@2x

Repro steps

_ = Find("123"); // INVALID
_ = await FindAsync("123"); // VALID
_ = FindSync("123"); // VALID
private sealed record UnrelatedThing(string Name, int Number);

private UnrelatedThing Find(string s, int i = 0)
{
    return new UnrelatedThing(s, i);
}

private async Task<string> FindAsync(string s)
{
    return await Task.FromResult(s);
}

private string FindSync(string s)
{
    return s;
}

Expected behavior

_ = Find("123"); should not trigger S6966.

Actual behavior

_ = Find("123"); triggers S6966.

Known workarounds

Please provide a description of any known workarounds.

Related information

  • C#/VB.NET Plugins version
  • Visual Studio version
  • MSBuild / dotnet version
  • SonarScanner for .NET version (if used)
  • Operating System
@ahusseini ahusseini changed the title Fix S6966 FP: Methods with different signature should not trigger this rule. Fix S6966 FP: Methods with different signatures should not trigger this rule. May 9, 2024
@MaStr11
Copy link

MaStr11 commented May 10, 2024

Thank you @ahusseini

this is a known problem for Mongo DB's API design and has already been documented for the rule:

var sort = Builders<Person>.Sort.Descending(nameof(Person.Name));
// FP for
// * Find https://mongodb.github.io/mongo-csharp-driver/2.8/apidocs/html/M_MongoDB_Driver_IMongoCollectionExtensions_Find__1_3.htm
// * FindAsync https://mongodb.github.io/mongo-csharp-driver/2.8/apidocs/html/M_MongoDB_Driver_IMongoCollectionExtensions_FindAsync__1_3.htm
// Speculative binding finds "FindAsync" but the return type IAsyncCursor<> of FindAsync is not compatible with return type IFindFluent<,> of "Find"
// Speculative binding does overload resolution according to the C# rules, which ignore return types.
// It seems to ignore the compiler binding error for the following "Sort" which is only defined on IFindFluent, but not in IAsyncCursor.
var snapshot = await personCollection.Find(s => s.Id > 10) // Noncompliant FP
.Sort(sort) // Not defined on IAsyncCursor (return type of FindAsync)
.FirstOrDefaultAsync().ConfigureAwait(false);

In Mongo DB FindSync is the synchronous version of FindAsync while Find is meant for something else. Because FindAsync and Find only differ in their return type, overload resolution is successful.

However, because the return types are not compatible, the overload should be rejected by our logic.

I marked this as a false positive and put it in our backlog.

@martin-strecker-sonarsource martin-strecker-sonarsource added Type: False Positive Rule IS triggered when it shouldn't be. Area: C# C# rules related issues. labels May 10, 2024
@mary-georgiou-sonarsource mary-georgiou-sonarsource added this to the 9.26 milestone May 21, 2024
@mary-georgiou-sonarsource mary-georgiou-sonarsource added the Sprint: Hardening Fix FPs/FNs/improvements label May 21, 2024
@mary-georgiou-sonarsource mary-georgiou-sonarsource moved this from To do to In progress in Best Kanban May 22, 2024
@mary-georgiou-sonarsource mary-georgiou-sonarsource moved this from In progress to To do in Best Kanban May 23, 2024
@mary-georgiou-sonarsource mary-georgiou-sonarsource removed their assignment May 23, 2024
@martin-strecker-sonarsource martin-strecker-sonarsource moved this from To do to In progress in Best Kanban May 23, 2024
@martin-strecker-sonarsource
Copy link
Contributor

We have two options on how to solve this

  1. We need to ensure that the (unwrapped) return type of the async method is covariant-compatible with the return type of the original method. This may produce FNs in a couple of cases like this
    • The return value is ignored on the call side
    • There is an explicit conversion of the return value to the type returned by the async version
    • Nullable<struct> vs struct
    • There might be all kinds of complicated cases with generics (e.g. IAdditionOperators vs int)
  2. Exclude MongoDB's Find method.

Option 2) can be done easily once #9318 is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: C# C# rules related issues. Sprint: Hardening Fix FPs/FNs/improvements Type: False Positive Rule IS triggered when it shouldn't be.
Projects
Best Kanban
  
Review in progress
4 participants