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 S3415 FP/FN: Support named arguments #6759

Merged
merged 28 commits into from Mar 8, 2023
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
c3341e2
Change implementation to use parameter names instead of position
Tim-Pohlmann Feb 15, 2023
9e34b10
Improve finding of parameters
Tim-Pohlmann Feb 15, 2023
656b372
Add FP test for dynamic argument
Tim-Pohlmann Feb 17, 2023
ead5938
Extended MethodParameterLookupBase.TryGetSyntax to work with MethodSy…
Tim-Pohlmann Feb 20, 2023
59e4453
Add additional Any() check
Tim-Pohlmann Feb 20, 2023
b7e075e
Add more tests
Tim-Pohlmann Feb 20, 2023
59f99ff
Refactor rule logic
Tim-Pohlmann Feb 21, 2023
de05f24
Refactor code
Tim-Pohlmann Feb 22, 2023
1d07958
Minor formatting
Tim-Pohlmann Feb 22, 2023
e33e208
Add more tests
Tim-Pohlmann Feb 22, 2023
af1f519
Minor formatting
Tim-Pohlmann Feb 22, 2023
163fed3
Refactoring
Tim-Pohlmann Feb 22, 2023
809bcbb
More Refactoring
Tim-Pohlmann Feb 23, 2023
873d748
Change issue location
Tim-Pohlmann Feb 24, 2023
41c415d
Add const support
Tim-Pohlmann Feb 24, 2023
31a4eee
Make MethodParameterLookupBase constructors not accept null for argum…
Tim-Pohlmann Feb 24, 2023
2c35ea1
Rename test methdos for clarity
Tim-Pohlmann Feb 24, 2023
6c87be7
Remove code smells and improve tests
Tim-Pohlmann Feb 24, 2023
d86671f
Add more tests
Tim-Pohlmann Feb 24, 2023
0729173
Minor refactorings
Tim-Pohlmann Feb 24, 2023
18da7c0
Simplify object creation
Tim-Pohlmann Feb 24, 2023
cd626a1
Add missing NotEqual support for XUnit
Tim-Pohlmann Feb 28, 2023
f9ea124
Improve issue location to include argument name
Tim-Pohlmann Feb 28, 2023
ce5108b
Fix bad calls of MethodParameterLookup constructor
Tim-Pohlmann Feb 28, 2023
9dc1928
Change switch to ternary expression
Tim-Pohlmann Mar 1, 2023
6ebf655
Fix error
Tim-Pohlmann Mar 1, 2023
fffa219
Fix formatting
Tim-Pohlmann Mar 6, 2023
bfeb315
Fix typo
Tim-Pohlmann Mar 6, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 11 additions & 4 deletions analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpFacade.cs
Expand Up @@ -18,6 +18,7 @@
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

using Microsoft.CodeAnalysis;
using SonarAnalyzer.Helpers.Facade;

namespace SonarAnalyzer.Helpers;
Expand Down Expand Up @@ -51,13 +52,19 @@ internal sealed class CSharpFacade : ILanguageFacade<SyntaxKind>
node.FindConstantValue(model);

public IMethodParameterLookup MethodParameterLookup(SyntaxNode invocation, IMethodSymbol methodSymbol) =>
invocation != null ? new CSharpMethodParameterLookup(GetArgumentList(invocation), methodSymbol) : null;

public IMethodParameterLookup MethodParameterLookup(SyntaxNode invocation, SemanticModel semanticModel) =>
invocation != null ? new CSharpMethodParameterLookup(GetArgumentList(invocation), semanticModel) : null;

private static ArgumentListSyntax GetArgumentList(SyntaxNode invocation) =>
invocation switch
{
null => null,
ObjectCreationExpressionSyntax x => new CSharpMethodParameterLookup(x.ArgumentList, methodSymbol),
InvocationExpressionSyntax x => new CSharpMethodParameterLookup(x, methodSymbol),
ArgumentListSyntax x => x,
ObjectCreationExpressionSyntax x => x.ArgumentList,
InvocationExpressionSyntax x => x.ArgumentList,
_ when ImplicitObjectCreationExpressionSyntaxWrapper.IsInstance(invocation) =>
new CSharpMethodParameterLookup(((ImplicitObjectCreationExpressionSyntaxWrapper)invocation).ArgumentList, methodSymbol),
((ImplicitObjectCreationExpressionSyntaxWrapper)invocation).ArgumentList,
_ => throw new ArgumentException($"{invocation.GetType()} does not contain an ArgumentList.", nameof(invocation)),
};

Expand Down
Expand Up @@ -18,26 +18,25 @@
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

namespace SonarAnalyzer.Helpers
namespace SonarAnalyzer.Helpers;

internal class CSharpMethodParameterLookup : MethodParameterLookupBase<ArgumentSyntax>
{
internal class CSharpMethodParameterLookup : MethodParameterLookupBase<ArgumentSyntax>
{
public CSharpMethodParameterLookup(InvocationExpressionSyntax invocation, SemanticModel semanticModel)
: this(invocation.ArgumentList, semanticModel) { }
public CSharpMethodParameterLookup(InvocationExpressionSyntax invocation, SemanticModel semanticModel)
: this(invocation.ArgumentList, semanticModel) { }

public CSharpMethodParameterLookup(InvocationExpressionSyntax invocation, IMethodSymbol methodSymbol)
: this(invocation.ArgumentList, methodSymbol) { }
public CSharpMethodParameterLookup(InvocationExpressionSyntax invocation, IMethodSymbol methodSymbol)
: this(invocation.ArgumentList, methodSymbol) { }

public CSharpMethodParameterLookup(ArgumentListSyntax argumentList, SemanticModel semanticModel)
: base(argumentList?.Arguments, argumentList == null ? null : semanticModel.GetSymbolInfo(argumentList.Parent).Symbol as IMethodSymbol) { }
public CSharpMethodParameterLookup(ArgumentListSyntax argumentList, SemanticModel semanticModel)
: base(argumentList.Arguments, semanticModel.GetSymbolInfo(argumentList.Parent)) { }

public CSharpMethodParameterLookup(ArgumentListSyntax argumentList, IMethodSymbol methodSymbol)
: base(argumentList?.Arguments, methodSymbol) { }
public CSharpMethodParameterLookup(ArgumentListSyntax argumentList, IMethodSymbol methodSymbol)
: base(argumentList.Arguments, methodSymbol) { }

protected override SyntaxNode Expression(ArgumentSyntax argument) =>
argument.Expression;
protected override SyntaxNode Expression(ArgumentSyntax argument) =>
argument.Expression;

protected override SyntaxToken? GetNameColonArgumentIdentifier(ArgumentSyntax argument) =>
argument.NameColon?.Name.Identifier;
}
protected override SyntaxToken? GetNameColonArgumentIdentifier(ArgumentSyntax argument) =>
argument.NameColon?.Name.Identifier;
}
12 changes: 5 additions & 7 deletions analyzers/src/SonarAnalyzer.CSharp/Helpers/CSharpSyntaxHelper.cs
Expand Up @@ -338,12 +338,10 @@ public static bool IsComment(this SyntaxTrivia trivia)
///
/// There can be zero, one or more results based on parameter type (Optional or ParamArray/params).
/// </summary>
public static ImmutableArray<SyntaxNode> ArgumentValuesForParameter(SemanticModel semanticModel, ArgumentListSyntax argumentList, string parameterName)
{
var methodParameterLookup = new CSharpMethodParameterLookup(argumentList, semanticModel);
return methodParameterLookup.TryGetSyntax(parameterName, out var expressions)
? expressions
: ImmutableArray<SyntaxNode>.Empty;
}
public static ImmutableArray<SyntaxNode> ArgumentValuesForParameter(SemanticModel semanticModel, ArgumentListSyntax argumentList, string parameterName) =>
argumentList != null
&& new CSharpMethodParameterLookup(argumentList, semanticModel).TryGetSyntax(parameterName, out var expressions)
? expressions
: ImmutableArray<SyntaxNode>.Empty;
}
}
Expand Up @@ -18,63 +18,85 @@
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

namespace SonarAnalyzer.Rules.CSharp
namespace SonarAnalyzer.Rules.CSharp;

[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class AssertionArgsShouldBePassedInCorrectOrder : SonarDiagnosticAnalyzer
{
[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class AssertionArgsShouldBePassedInCorrectOrder : SonarDiagnosticAnalyzer
{
internal const string DiagnosticId = "S3415";
private const string MessageFormat = "Make sure these 2 arguments are in the correct order: expected value, actual value.";
internal const string DiagnosticId = "S3415";
private const string MessageFormat = "Make sure these 2 arguments are in the correct order: expected value, actual value.";

private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, MessageFormat);
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(Rule);
private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, MessageFormat);
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(Rule);

private static readonly IDictionary<string, ImmutableArray<KnownType>> MethodsWithType = new Dictionary<string, ImmutableArray<KnownType>>
protected override void Initialize(SonarAnalysisContext context) =>
context.RegisterNodeAction(c =>
{
["AreEqual"] = ImmutableArray.Create(KnownType.Microsoft_VisualStudio_TestTools_UnitTesting_Assert, KnownType.NUnit_Framework_Assert),
["AreNotEqual"] = ImmutableArray.Create(KnownType.Microsoft_VisualStudio_TestTools_UnitTesting_Assert, KnownType.NUnit_Framework_Assert),
["AreSame"] = ImmutableArray.Create(KnownType.Microsoft_VisualStudio_TestTools_UnitTesting_Assert, KnownType.NUnit_Framework_Assert),
["AreNotSame"] = ImmutableArray.Create(KnownType.Microsoft_VisualStudio_TestTools_UnitTesting_Assert, KnownType.NUnit_Framework_Assert),
["Equal"] = ImmutableArray.Create(KnownType.Xunit_Assert),
["Same"] = ImmutableArray.Create(KnownType.Xunit_Assert),
["NotSame"] = ImmutableArray.Create(KnownType.Xunit_Assert)
};

protected override void Initialize(SonarAnalysisContext context) =>
context.RegisterNodeAction(c =>
if (c.Node is InvocationExpressionSyntax { ArgumentList: { Arguments.Count: >= 2 } argumentList } invocation
&& GetParameters(invocation.GetName()) is { } knownAssertParameters
&& c.SemanticModel.GetSymbolInfo(invocation).AllSymbols()
.SelectMany(symbol =>
symbol is IMethodSymbol { IsStatic: true, ContainingSymbol: INamedTypeSymbol container } methodSymbol
? knownAssertParameters.Select(knownParameters => FindWrongArguments(c.SemanticModel, container, methodSymbol, argumentList, knownParameters))
: Enumerable.Empty<WrongArguments?>())
.FirstOrDefault(x => x is not null) is (Expected: var expected, Actual: var actual))
{
var methodCall = (InvocationExpressionSyntax)c.Node;
if (!methodCall.Expression.IsKind(SyntaxKind.SimpleMemberAccessExpression)
|| methodCall.ArgumentList.Arguments.Count < 2)
{
return;
}
c.ReportIssue(Diagnostic.Create(Rule, CreateLocation(expected, actual)));
}
},
SyntaxKind.InvocationExpression);

var firstArgument = methodCall.ArgumentList.Arguments[0];
var secondArgument = methodCall.ArgumentList.Arguments[1];
if (firstArgument.Expression is LiteralExpressionSyntax
|| secondArgument.Expression is not LiteralExpressionSyntax)
private static KnownAssertParameters[] GetParameters(string name) =>
name switch
{
"AreEqual" => new KnownAssertParameters[]
{
return;
}

var methodCallExpression = (MemberAccessExpressionSyntax)methodCall.Expression;

var methodKnownTypes = MethodsWithType.GetValueOrDefault(methodCallExpression.Name.Identifier.ValueText);
if (methodKnownTypes == null)
new(KnownType.Microsoft_VisualStudio_TestTools_UnitTesting_Assert, "expected", "actual"),
new(KnownType.NUnit_Framework_Assert, "expected", "actual")
},
"AreNotEqual" => new KnownAssertParameters[]
{
return;
}

var symbolInfo = c.SemanticModel.GetSymbolInfo(methodCallExpression.Expression).Symbol;
var isAnyTrackedAssertType = (symbolInfo as INamedTypeSymbol).IsAny(methodKnownTypes);
if (!isAnyTrackedAssertType)
new(KnownType.Microsoft_VisualStudio_TestTools_UnitTesting_Assert, "notExpected", "actual"),
new(KnownType.NUnit_Framework_Assert, "expected", "actual")
},
"AreSame" => new KnownAssertParameters[]
{
new(KnownType.Microsoft_VisualStudio_TestTools_UnitTesting_Assert, "expected", "actual"),
new(KnownType.NUnit_Framework_Assert, "expected", "actual")
},
"AreNotSame" => new KnownAssertParameters[]
{
new(KnownType.Microsoft_VisualStudio_TestTools_UnitTesting_Assert, "notExpected", "actual"),
new(KnownType.NUnit_Framework_Assert, "expected", "actual")
},
"Equal" or "NotEqual" or "Same" or "NotSame" => new KnownAssertParameters[]
{
return;
}
new(KnownType.Xunit_Assert, "expected", "actual")
},
_ => null
};

private static WrongArguments? FindWrongArguments(SemanticModel semanticModel,
INamedTypeSymbol container,
IMethodSymbol symbol,
ArgumentListSyntax argumentList,
KnownAssertParameters knownParameters) =>
container.Is(knownParameters.AssertClass)
&& CSharpFacade.Instance.MethodParameterLookup(argumentList, symbol) is var parameterLookup
&& parameterLookup.TryGetSyntax(knownParameters.ExpectedParamterName, out var expectedArguments)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo

Suggested change
&& parameterLookup.TryGetSyntax(knownParameters.ExpectedParamterName, out var expectedArguments)
&& parameterLookup.TryGetSyntax(knownParameters.ExpectedParameterName, out var expectedArguments)

&& expectedArguments.FirstOrDefault() is { } expected
&& semanticModel.GetConstantValue(expected).HasValue is false
&& parameterLookup.TryGetSyntax(knownParameters.ActualParameterName, out var actualArguments)
&& actualArguments.FirstOrDefault() is { } actual
&& semanticModel.GetConstantValue(actual).HasValue is true

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
&& semanticModel.GetConstantValue(actual).HasValue is true
&& semanticModel.GetConstantValue(actual).HasValue

? new(expected, actual)
: null;

private static Location CreateLocation(SyntaxNode argument1, SyntaxNode argument2) =>
argument1.Span.CompareTo(argument2.Span) < 0
? argument1.Parent.CreateLocation(argument2.Parent)
: argument2.Parent.CreateLocation(argument1.Parent);

c.ReportIssue(Diagnostic.Create(Rule, firstArgument.CreateLocation(secondArgument)));
},
SyntaxKind.InvocationExpression);
}
private readonly record struct KnownAssertParameters(KnownType AssertClass, string ExpectedParamterName, string ActualParameterName);
private readonly record struct WrongArguments(SyntaxNode Expected, SyntaxNode Actual);
}
Expand Up @@ -29,13 +29,11 @@ public class CSharpObjectCreationTracker : ObjectCreationTracker<SyntaxKind>
&& argumentList.Arguments.Count > index
&& argumentList.Arguments[index].Expression.HasConstantValue(context.SemanticModel);

internal override object ConstArgumentForParameter(ObjectCreationContext context, string parameterName)
{
var argumentList = ObjectCreationFactory.Create(context.Node).ArgumentList;
var values = CSharpSyntaxHelper.ArgumentValuesForParameter(context.SemanticModel, argumentList, parameterName);
return values.Length == 1 && values[0] is ExpressionSyntax valueSyntax
internal override object ConstArgumentForParameter(ObjectCreationContext context, string parameterName) =>
ObjectCreationFactory.TryCreate(context.Node, out var objectCreation)
&& CSharpSyntaxHelper.ArgumentValuesForParameter(context.SemanticModel, objectCreation.ArgumentList, parameterName) is { Length: 1 } values
&& values[0] is ExpressionSyntax valueSyntax
? valueSyntax.FindConstantValue(context.SemanticModel)
: null;
}
}
}
Expand Up @@ -49,6 +49,12 @@ node switch
? Create(node)
: null;

public static bool TryCreate(SyntaxNode node, out IObjectCreation objectCreation)
{
objectCreation = TryCreate(node);
return objectCreation is not null;
}

private class ObjectCreation : IObjectCreation
{
private readonly ObjectCreationExpressionSyntax objectCreation;
Expand Down
Expand Up @@ -33,6 +33,7 @@ public interface ILanguageFacade
DiagnosticDescriptor CreateDescriptor(string id, string messageFormat, bool? isEnabledByDefault = null, bool fadeOutCode = false);
object FindConstantValue(SemanticModel model, SyntaxNode node);
IMethodParameterLookup MethodParameterLookup(SyntaxNode invocation, IMethodSymbol methodSymbol);
IMethodParameterLookup MethodParameterLookup(SyntaxNode invocation, SemanticModel semanticModel);
string GetName(SyntaxNode expression);
}

Expand Down