Skip to content

Fix S3220 FP: Rule does not take into account generics #9389

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.

Sign up for GitHub

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

Merged
mary-georgiou-sonarsource merged 3 commits into from
Jun 5, 2024
Merged

Fix S3220 FP: Rule does not take into account generics #9389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b3eadec
fix FP
mary-georgiou-sonarsource Jun 4, 2024
0590535
small cleanup
mary-georgiou-sonarsource Jun 4, 2024
2bb8c77
apply comments
mary-georgiou-sonarsource Jun 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 26 additions & 32 deletions analyzers/src/SonarAnalyzer.CSharp/Rules/InvocationResolvesToOverrideWithParams.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,28 +25,21 @@ public sealed class InvocationResolvesToOverrideWithParams : SonarDiagnosticAnal
{
private const string DiagnosticId = "S3220";
private const string MessageFormat = "Review this call, which partially matches an overload without 'params'. The partial match is '{0}'.";

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

protected override void Initialize(SonarAnalysisContext context)
{
context.RegisterNodeAction(
c =>
{
var invocation = (InvocationExpressionSyntax)c.Node;
CheckCall(c, invocation, invocation.ArgumentList);
},
SyntaxKind.InvocationExpression);
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(Rule);

protected override void Initialize(SonarAnalysisContext context) =>
context.RegisterNodeAction(
c =>
{
var objectCreation = (ObjectCreationExpressionSyntax)c.Node;
CheckCall(c, objectCreation, objectCreation.ArgumentList);
var node = c.Node;
var argumentList = (node as InvocationExpressionSyntax)?.ArgumentList
?? ((ObjectCreationExpressionSyntax)node).ArgumentList;
CheckCall(c, node, argumentList);
},
SyntaxKind.InvocationExpression,
SyntaxKind.ObjectCreationExpression);
}

private static void CheckCall(SonarSyntaxNodeReportingContext context, SyntaxNode node, ArgumentListSyntax argumentList)
{
Expand All @@ -55,14 +48,16 @@ private static void CheckCall(SonarSyntaxNodeReportingContext context, SyntaxNod
&& method.Parameters.LastOrDefault() is { IsParams: true }
&& !IsInvocationWithExplicitArray(argumentList, method, context.SemanticModel)
&& ArgumentTypes(context, argumentList) is var argumentTypes
&& argumentTypes.All(x => x is not IErrorTypeSymbol)
&& OtherOverloadsOf(method).FirstOrDefault(IsPossibleMatch) is { } otherMethod)
&& Array.TrueForAll(argumentTypes, x => x is not IErrorTypeSymbol)
&& OtherOverloadsOf(method).FirstOrDefault(IsPossibleMatch) is { } otherMethod
&& method.IsGenericMethod == otherMethod.IsGenericMethod)
{
context.ReportIssue(Rule, node, otherMethod.ToMinimalDisplayString(context.SemanticModel, node.SpanStart));
}

bool IsPossibleMatch(IMethodSymbol method) =>
ArgumentsMatchParameters(argumentList, argumentTypes, method, context.SemanticModel) && MethodAccessibleWithinType(method, context.ContainingSymbol.ContainingType);
ArgumentsMatchParameters(argumentList, argumentTypes, method, context.SemanticModel)
&& MethodAccessibleWithinType(method, context.ContainingSymbol.ContainingType);
}

private static ITypeSymbol[] ArgumentTypes(SonarSyntaxNodeReportingContext context, ArgumentListSyntax argumentList) =>
Expand All @@ -81,7 +76,7 @@ private static bool IsInvocationWithExplicitArray(ArgumentListSyntax argumentLis
{
var lookup = new CSharpMethodParameterLookup(argumentList, invokedMethodSymbol);
var parameters = argumentList.Arguments.Select(Valid).ToArray();
return parameters.All(x => x is not null) && parameters.Count(x => x.IsParams) == 1;
return Array.TrueForAll(parameters, x => x is not null) && parameters.Count(x => x.IsParams) == 1;

IParameterSymbol Valid(ArgumentSyntax argument) =>
lookup.TryGetSymbol(argument, out var parameter)
Expand All @@ -94,7 +89,7 @@ private static bool ArgumentsMatchParameters(ArgumentListSyntax argumentList, IT
{
var lookup = new CSharpMethodParameterLookup(argumentList, possibleOtherMethod);
var parameters = argumentList.Arguments.Select((argument, index) => Valid(argument, argumentTypes[index])).ToArray();
return parameters.All(x => x is not null) && possibleOtherMethod.Parameters.Except(parameters).All(x => x.HasExplicitDefaultValue);
return Array.TrueForAll(parameters, x => x is not null) && possibleOtherMethod.Parameters.Except(parameters).All(x => x.HasExplicitDefaultValue);

IParameterSymbol Valid(ArgumentSyntax argument, ITypeSymbol type) =>
lookup.TryGetSymbol(argument, out var parameter)
Expand All @@ -105,19 +100,18 @@ IParameterSymbol Valid(ArgumentSyntax argument, ITypeSymbol type) =>
}

private static bool MethodAccessibleWithinType(IMethodSymbol method, ITypeSymbol type) =>
IsInTypeOrNested(method, type)
|| method.DeclaredAccessibility switch
{
Accessibility.Private => false,
// ProtectedAndInternal corresponds to `private protected`.
Accessibility.ProtectedAndInternal => type.DerivesFrom(method.ContainingType) && method.IsInSameAssembly(type),
// ProtectedOrInternal corresponds to `protected internal`.
Accessibility.ProtectedOrInternal => type.DerivesFrom(method.ContainingType) || method.IsInSameAssembly(type),
Accessibility.Protected => type.DerivesFrom(method.ContainingType),
Accessibility.Internal => method.IsInSameAssembly(type),
Accessibility.Public => true,
_ => false,
};
IsInTypeOrNested(method, type) || method.DeclaredAccessibility switch
{
Accessibility.Private => false,
// ProtectedAndInternal corresponds to `private protected`.
Accessibility.ProtectedAndInternal => type.DerivesFrom(method.ContainingType) && method.IsInSameAssembly(type),
// ProtectedOrInternal corresponds to `protected internal`.
Accessibility.ProtectedOrInternal => type.DerivesFrom(method.ContainingType) || method.IsInSameAssembly(type),
Accessibility.Protected => type.DerivesFrom(method.ContainingType),
Accessibility.Internal => method.IsInSameAssembly(type),
Accessibility.Public => true,
_ => false,
};

private static bool IsInTypeOrNested(IMethodSymbol method, ITypeSymbol type) =>
type is not null && (method.IsInType(type) || IsInTypeOrNested(method, type.ContainingType));
Expand Down
11 changes: 9 additions & 2 deletions analyzers/tests/SonarAnalyzer.Test/TestCases/InvocationResolvesToOverrideWithParams.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,11 +414,18 @@ public void AccessibilityAcrossAssemblies()
class Repro_8522
{
T Get<T>(params string[] key) => default;

string Get(string key) => default;

T GetBothHaveGenerics<T>(params int[] ints) => default;
T GetBothHaveGenerics<T>(int anInt) => default;

T GenericsWhereOneHasObjectParam<T>(params int[] ints) => default;
T GenericsWhereOneHasObjectParam<T>(object anInt) => default;

void Test()
{
Get<string>("text"); // Noncompliant FP
Get<string>("text"); // Compliant
GetBothHaveGenerics<string>(1); // Compliant, when both methods are generic it seems to resolve correctly to the T GetBothHaveGenerics<T>(int anInt).
GenericsWhereOneHasObjectParam<string>(1); // Noncompliant
}
}