Summary: Issues should be raised at the code causing the problem only.
We are using SonarQube 10.6 and SonarLint in several current versions
Lets take the following example:
namespace TestS3342AndS4017;
using System;
using System.Collections.Generic;
using System.Threading.Tasks;
public class External
{
private Func<DerivedArgumentClass, IDictionary<string, IList<string>>, Task> action;
public External(Func<DerivedArgumentClass, IDictionary<string, IList<string>>, Task> action)
{
this.action = action;
}
public Task InvokeAsync()
{
return this.action.Invoke(new DerivedArgumentClass(), new Dictionary<string, IList<string>>()) ?? Task.CompletedTask;
}
}
public class DerivedArgumentClass : BaseArgumentClass
{
public string? DerivedProperty { get; set; }
}
public class BaseArgumentClass
{
public string? BaseProperty { get; set; }
}
public class UserCode
{
static UserCode()
{
var test = new External(ActionAsync);
}
private static Task ActionAsync(
DerivedArgumentClass argument, // S3242
IDictionary<string, IList<string>> someData) // S4017
{
argument.BaseProperty = "abc";
return Task.CompletedTask;
}
}
In the class UserCode you can find the method ActionAsync currently getting the issues S3242 and S4017.
Even the rules are different, both should not complain here for the same reason: The user code is following the external interface. There is no option to solve the problem here. (Only, we can suppress it.)
If a method is used in an assignment to Action<T> or Func<T> these issues should not be raised. Instead the definition of such (generic) delegates should be checked for these rules.
Where is your snippet implementing an external interface? I think I am missing something
I cannot find S3342. Did you make a typo?
As a fix about S4017, I would suggest that you remove the rule from your quality profile, as it is a non-SonarWay rule and we work on it less often than SonarWay ones.
The “external interface” is implemented as public class External.
The first argument of UserCode.ActionAsync gets S3342.
I like the idea of S4017, so I do not like to disable it at all. I just ask whether the definition of the rule is complete. I recommend this rule (these rules) should not complain if the root cause is the “interface” (concrete here delegate or Action).
Hey again!
I added a ticket on our internal backlog to track your suggested change about S4017, as I agree with it.
About S3342, I am still confused. We do not have a rule with that ID.
Could you send me a screenshot of SonarQube, SonarCloud or even your IDE raising the issue?
It might be one of the external rules we are implicitly importing.
Thanks for clarifying.
About S3242, I do not think we can track the usecase you suggested, as it is far more sophisticated than what the rule currently does.
Keep in mind that we are already not raising in cases with abstract/birtual methods and interfaces, e.g.:
public abstract class A
{
public abstract void Method(Thingo foo);
}
public class B : A
{
public override void Method(Thingo foo) // We correctly do not raise here
{
foo.Foo();
}
}
public class Thingo : Thing { }
public class Thing
{
public void Foo() { }
}
I don’t see a difference between S3242 and S4017 regarding the question.
Both complain at the user code, while the “problem” is raised by the “interface definition”.
The problem is that we cannot know that ActionAsync from your snippet will be used specifically for implementing the External’s callback.
If you were implementing External via inheritance, we would know this and should not raise.
In fact, S3242 is not raising in this case, but S4017 does while it should not. Which is why I created a ticket to fix this.
I think you can: ActionAsync is used to initialize an Action<...>.
For me, this is enough to not blame on the method.
You should blame on the defintion of the Action<...> or Func<...> instead.
I misunderstood your usecase for S3242.
Now I understand.
Unfortunately, I do not think it is possible to track the new External(ActionAsync), as it would be very computationally expensive. Even with symbolic execution, it would be very tricky to get it right.
My suggestion is to either in-line disable the rule for the specific method, or disable the rule completely from your Quality Profile, if the rule is very noisy for you.
Sorry that it couldn’t be done, but thanks for raising the point regardless!
Which ticket? Could you give a link?
Our backlog on JIRA is internal, so links will not work outside the organization.
