Skip to content

FNs/FPs in S2234: Improvements of the rule #8253

Open
@martin-strecker-sonarsource

Description

In #8238 we reworked the rule S2234 to achieve the status quo of the old implementation while adding support for new language features. There is remaining work to be done, which this ticket summarizes.

Better support for argument to parameter conversion

See #8238 (comment)
This should also fix #3879 (#3880 has test cases, that should be copied for this)

We should also take into account that the assignability of the swapped arguments to their new parameters is not sufficient. If the types change, overload resolution may pick another overload. Consider this:

void Swap(int a, object b);
void Swap(int a, byte b);

int a = 1;
byte b = 1;

Swap(b, a); // calls int, object
Swap(a, b); // calls int, byte

Sharplab

The solution is to add a last check, that rebinds the invocation with the swapped arguments (create a new invocation node via InvocationExpressionSyntax.WithArgumentList) and pass it to SemanticModel.GetSpeculativeSymbolInfo at the position of the invocation. The returned symbol should be the method symbol of the new bound method and needs to be the same as the one invoked before.

Named argument handling

I added ArgumentNameColon() to the Language facade because I thought that a swap of arguments should not be reported, if both arguments already use named arguments, like in Swap(a: b, b: a). Here the mismatch looks highly intentional. I did not implement it, because I wanted to keep the PR focused. So the helper methods are there but are unused at the moment but we should exclude the case described.

MatchingNames

At the moment the language-specific string comparer is used to find matches between an argument expression and a parameter name. This was fine as long as we only considered simple IdentifierNames in the argument expression. But Language.NodeIdentifier() returns "names" in many more cases now for all kinds of expressions. It e.g. returns C for member access A.B.C. Given that, it would make sense to relax the comparison so that for a parameter named parameter the following argument expression would match:

  • A.B.Parameter
  • A.B.PARAMETER()
  • this._parameter
  • para_meter

see also #8238 (comment)

Lambda support

see #8072

Metadata

Assignees

No one assigned

    Labels

    Area: C#C# rules related issues.Area: VB.NETVB.NET rules related issues.Sprint: HardeningFix FPs/FNs/improvementsType: False NegativeRule is NOT triggered when it should be.Type: False PositiveRule IS triggered when it shouldn't be.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions