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 S4144 FP: when type constraints are used #9398

Merged
merged 33 commits into from
Jun 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
e4141f1
Fix via semantic model type check
martin-strecker-sonarsource Jun 4, 2024
3fc4b3e
Refactor
martin-strecker-sonarsource Jun 4, 2024
baf9a5f
Fix
martin-strecker-sonarsource Jun 4, 2024
3b3d68f
Unsued using
martin-strecker-sonarsource Jun 4, 2024
0170497
Add proper support for type parameters and add more tests
martin-strecker-sonarsource Jun 5, 2024
798bb24
Better comments
martin-strecker-sonarsource Jun 5, 2024
f7cc661
Test improvements
martin-strecker-sonarsource Jun 5, 2024
08d5da4
fix test
martin-strecker-sonarsource Jun 5, 2024
fccc74d
Make CombinatorialDataAttribute derive from TestMethodAttribute
martin-strecker-sonarsource Jun 5, 2024
c378171
wip: Fix..
martin-strecker-sonarsource Jun 6, 2024
de61440
Rename to CombinatorialDataTestMethod
martin-strecker-sonarsource Jun 7, 2024
8605bd4
Remove unsed parameter
martin-strecker-sonarsource Jun 7, 2024
a45e584
Add support for VB
martin-strecker-sonarsource Jun 7, 2024
8f70d1a
Add VB tests
martin-strecker-sonarsource Jun 7, 2024
16bcfde
More VB tests
martin-strecker-sonarsource Jun 7, 2024
093915b
Correct tests
martin-strecker-sonarsource Jun 7, 2024
586d42b
Net only test
martin-strecker-sonarsource Jun 10, 2024
a26c3ce
Fix code smells and formatting
martin-strecker-sonarsource Jun 10, 2024
3c35dac
Add all combinations for constraints
martin-strecker-sonarsource Jun 10, 2024
7a4d933
Review
martin-strecker-sonarsource Jun 12, 2024
01944a9
Move test cases and add a further nested example
martin-strecker-sonarsource Jun 12, 2024
7fc57b4
Codecoverage: More tests
martin-strecker-sonarsource Jun 12, 2024
50c9dde
Simplify
martin-strecker-sonarsource Jun 12, 2024
1ed56ed
Formatting
martin-strecker-sonarsource Jun 12, 2024
8f9ffc3
Grammar
martin-strecker-sonarsource Jun 12, 2024
6a3504d
Better is ITypeParameterSymbol check
martin-strecker-sonarsource Jun 12, 2024
90d582b
Better names
martin-strecker-sonarsource Jun 12, 2024
b9e7640
Simplify
martin-strecker-sonarsource Jun 13, 2024
f17f3b3
Add OriginalDefinition check and test
martin-strecker-sonarsource Jun 13, 2024
18aab61
Better comment
martin-strecker-sonarsource Jun 13, 2024
0288dd8
Correct comment
martin-strecker-sonarsource Jun 13, 2024
2cdf1b3
Fix name
martin-strecker-sonarsource Jun 19, 2024
9b25eb7
Comment
martin-strecker-sonarsource Jun 20, 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
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,11 @@ protected override IEnumerable<IMethodDeclaration> GetMethodDeclarations(SyntaxN
? ((CompilationUnitSyntax)node).GetMethodDeclarations()
: ((TypeDeclarationSyntax)node).GetMethodDeclarations();

protected override bool AreDuplicates(IMethodDeclaration firstMethod, IMethodDeclaration secondMethod) =>
protected override bool AreDuplicates(SemanticModel model, IMethodDeclaration firstMethod, IMethodDeclaration secondMethod) =>
firstMethod is { Body: { Statements: { Count: > 1 } } }
&& firstMethod.Identifier.ValueText != secondMethod.Identifier.ValueText
&& HaveSameParameters<ParameterSyntax>(firstMethod.ParameterList.Parameters, secondMethod.ParameterList.Parameters)
&& HaveSameParameters(firstMethod.ParameterList?.Parameters, secondMethod.ParameterList?.Parameters)
&& HaveSameTypeParameters(model, firstMethod.TypeParameterList?.Parameters, secondMethod.TypeParameterList?.Parameters)
&& firstMethod.Body.IsEquivalentTo(secondMethod.Body, false);

protected override SyntaxToken GetMethodIdentifier(IMethodDeclaration method) =>
Expand Down
Tim-Pohlmann marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -18,58 +18,92 @@
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

namespace SonarAnalyzer.Rules
namespace SonarAnalyzer.Rules;

public abstract class MethodsShouldNotHaveIdenticalImplementationsBase<TSyntaxKind, TMethodDeclarationSyntax> : SonarDiagnosticAnalyzer<TSyntaxKind>
where TSyntaxKind : struct
{
public abstract class MethodsShouldNotHaveIdenticalImplementationsBase<TSyntaxKind, TMethodDeclarationSyntax> : SonarDiagnosticAnalyzer<TSyntaxKind>
where TSyntaxKind : struct
{
private const string DiagnosticId = "S4144";
private const string DiagnosticId = "S4144";

protected abstract TSyntaxKind[] SyntaxKinds { get; }
protected abstract TSyntaxKind[] SyntaxKinds { get; }

protected abstract IEnumerable<TMethodDeclarationSyntax> GetMethodDeclarations(SyntaxNode node);
protected abstract SyntaxToken GetMethodIdentifier(TMethodDeclarationSyntax method);
protected abstract bool AreDuplicates(TMethodDeclarationSyntax firstMethod, TMethodDeclarationSyntax secondMethod);
protected abstract IEnumerable<TMethodDeclarationSyntax> GetMethodDeclarations(SyntaxNode node);
protected abstract SyntaxToken GetMethodIdentifier(TMethodDeclarationSyntax method);
protected abstract bool AreDuplicates(SemanticModel model, TMethodDeclarationSyntax firstMethod, TMethodDeclarationSyntax secondMethod);

protected override string MessageFormat => "Update this method so that its implementation is not identical to '{0}'.";
protected override string MessageFormat => "Update this method so that its implementation is not identical to '{0}'.";

protected MethodsShouldNotHaveIdenticalImplementationsBase() : base(DiagnosticId) { }
protected MethodsShouldNotHaveIdenticalImplementationsBase() : base(DiagnosticId) { }

protected override void Initialize(SonarAnalysisContext context) =>
context.RegisterNodeAction(Language.GeneratedCodeRecognizer,
c =>
protected override void Initialize(SonarAnalysisContext context) =>
context.RegisterNodeAction(Language.GeneratedCodeRecognizer,
c =>
{
if (IsExcludedFromBeingExamined(c))
{
if (IsExcludedFromBeingExamined(c))
{
return;
}
return;
}

var methodsToHandle = new LinkedList<TMethodDeclarationSyntax>(GetMethodDeclarations(c.Node));
var methodsToHandle = new LinkedList<TMethodDeclarationSyntax>(GetMethodDeclarations(c.Node));

while (methodsToHandle.Any())
while (methodsToHandle.Any())
{
var method = methodsToHandle.First.Value;
methodsToHandle.RemoveFirst();
var duplicates = methodsToHandle.Where(x => AreDuplicates(c.SemanticModel, method, x)).ToList();

foreach (var duplicate in duplicates)
{
var method = methodsToHandle.First.Value;
methodsToHandle.RemoveFirst();
var duplicates = methodsToHandle.Where(x => AreDuplicates(method, x)).ToList();

foreach (var duplicate in duplicates)
{
methodsToHandle.Remove(duplicate);
c.ReportIssue(SupportedDiagnostics[0], GetMethodIdentifier(duplicate), [GetMethodIdentifier(method).ToSecondaryLocation()], GetMethodIdentifier(method).ValueText);
}
methodsToHandle.Remove(duplicate);
c.ReportIssue(SupportedDiagnostics[0], GetMethodIdentifier(duplicate), [GetMethodIdentifier(method).ToSecondaryLocation()], GetMethodIdentifier(method).ValueText);
}
},
SyntaxKinds);

protected virtual bool IsExcludedFromBeingExamined(SonarSyntaxNodeReportingContext context) =>
context.ContainingSymbol.Kind != SymbolKind.NamedType;

protected static bool HaveSameParameters<TSyntax>(SeparatedSyntaxList<TSyntax>? leftParameters, SeparatedSyntaxList<TSyntax>? rightParameters)
where TSyntax : SyntaxNode =>
(leftParameters == null && rightParameters == null)
|| (leftParameters != null
&& rightParameters != null
&& leftParameters.Value.Count == rightParameters.Value.Count
&& leftParameters.Value.Select((left, index) => left.IsEquivalentTo(rightParameters.Value[index])).All(x => x));
}
},
SyntaxKinds);

protected virtual bool IsExcludedFromBeingExamined(SonarSyntaxNodeReportingContext context) =>
context.ContainingSymbol.Kind != SymbolKind.NamedType;

protected static bool HaveSameParameters<TSyntax>(SeparatedSyntaxList<TSyntax>? leftParameters, SeparatedSyntaxList<TSyntax>? rightParameters)
where TSyntax : SyntaxNode =>
(leftParameters is null && rightParameters is null) // In VB.Net the parameter list can be omitted
|| (leftParameters?.Count == rightParameters?.Count && HaveSameParameterLists(leftParameters.Value, rightParameters.Value));

protected static bool HaveSameTypeParameters<TSyntax>(SemanticModel model, SeparatedSyntaxList<TSyntax>? firstTypeParameterList, SeparatedSyntaxList<TSyntax>? secondTypeParameterList)
where TSyntax : SyntaxNode
{
var firstSymbols = firstTypeParameterList?.Select(x => model.GetDeclaredSymbol(x)).OfType<ITypeParameterSymbol>() ?? [];
var secondSymbols = secondTypeParameterList?.Select(x => model.GetDeclaredSymbol(x)).OfType<ITypeParameterSymbol>().ToArray() ?? [];
return firstSymbols.All(x => Array.Exists(secondSymbols, secondSymbol => TypeParametersHaveSameNameAndConstraints(x, secondSymbol)));
}

private static bool HaveSameParameterLists<TSyntax>(SeparatedSyntaxList<TSyntax> firstParameters,
SeparatedSyntaxList<TSyntax> secondParameters) where TSyntax : SyntaxNode =>
firstParameters.Equals(secondParameters, (first, second) => first.IsEquivalentTo(second)); // Perf: Syntactic equivalence for all parameters

private static bool TypeParametersHaveSameNameAndConstraints(ITypeParameterSymbol first, ITypeParameterSymbol second) =>
first.Name == second.Name
&& first.HasConstructorConstraint == second.HasConstructorConstraint
&& first.HasReferenceTypeConstraint == second.HasReferenceTypeConstraint
&& first.HasValueTypeConstraint == second.HasValueTypeConstraint
&& first.HasUnmanagedTypeConstraint() == second.HasUnmanagedTypeConstraint()
&& first.ConstraintTypes.Length == second.ConstraintTypes.Length
&& first.ConstraintTypes.All(x => second.ConstraintTypes.Any(y => TypeConstraintsAreSame(x, y)));

private static bool TypeConstraintsAreSame(ITypeSymbol first, ITypeSymbol second) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

This name confuses me a bit. It makes sense when called from TypeParametersHaveSameNameAndConstraints but does not seem accurate when called from TypesAreSameGenericType.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. That is right, but I couldn't come up with a better name.

first.Equals(second) // M1<T>(T x) where T: IComparable <-> M2<T>(T x) where T: IComparable
|| AreSameNamedTypeParameters(first, second) // M1<T1, T2>() where T1: T2 <-> M2<T1, T2>() where T1: T2
// T2 of M1 is a different symbol than T2 of M2, but if they have the same name they can be interchanged.
// T2 equivalency is checked as well by the TypeConstraintsAreSame call in TypeParametersHaveSameNameAndConstraints.
|| TypesAreSameGenericType(first, second); // M1<T>(T x) where T: IEquatable<T> <-> M2<T>(T x) where T: IEquatable<T>

private static bool TypesAreSameGenericType(ITypeSymbol firstParameterType, ITypeSymbol secondParameterType) =>
firstParameterType is INamedTypeSymbol { IsGenericType: true } namedTypeFirst
&& secondParameterType is INamedTypeSymbol { IsGenericType: true } namedTypeSecond
&& namedTypeFirst.OriginalDefinition.Equals(namedTypeSecond.OriginalDefinition)
&& namedTypeFirst.TypeArguments.Length == namedTypeSecond.TypeArguments.Length
&& namedTypeFirst.TypeArguments.Equals(namedTypeSecond.TypeArguments, TypeConstraintsAreSame);

private static bool AreSameNamedTypeParameters(ITypeSymbol first, ITypeSymbol second) =>
first is ITypeParameterSymbol x && second is ITypeParameterSymbol y && x.Name == y.Name;
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,11 @@ public sealed class MethodsShouldNotHaveIdenticalImplementations : MethodsShould
protected override IEnumerable<MethodBlockSyntax> GetMethodDeclarations(SyntaxNode node) =>
((TypeBlockSyntax)node).Members.OfType<MethodBlockSyntax>();

protected override bool AreDuplicates(MethodBlockSyntax firstMethod, MethodBlockSyntax secondMethod) =>
protected override bool AreDuplicates(SemanticModel model, MethodBlockSyntax firstMethod, MethodBlockSyntax secondMethod) =>
firstMethod.Statements.Count > 1
&& firstMethod.GetIdentifierText() != secondMethod.GetIdentifierText()
&& HaveSameParameters(firstMethod.GetParameters(), secondMethod.GetParameters())
&& HaveSameTypeParameters(model, firstMethod.SubOrFunctionStatement?.TypeParameterList?.Parameters, secondMethod.SubOrFunctionStatement?.TypeParameterList?.Parameters)
&& VisualBasicEquivalenceChecker.AreEquivalent(firstMethod.Statements, secondMethod.Statements);

protected override SyntaxToken GetMethodIdentifier(MethodBlockSyntax method) =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,7 @@ static void StaticLocalFunction(HttpRequest request)
}
"""").Verify();

[TestMethod]
[CombinatorialData]
[CombinatorialDataTestMethod]
public void UseAspNetModelBinding_CompliantAccess(
[DataValues(
"_ = {0}.Keys",
Expand Down Expand Up @@ -293,8 +292,7 @@ public void HandleRequest(HttpRequest request)
}
"""").VerifyNoIssues();

[TestMethod]
[CombinatorialData]
[CombinatorialDataTestMethod]
public void UseAspNetModelBinding_InheritanceAccess(
[DataValues(
": Controller",
Expand Down
Loading