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.

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

Jump to bottom

Fix S4144 FP: when type constraints are used #9398

Merged
merged 33 commits into from
Jun 20, 2024
Merged

Fix S4144 FP: when type constraints are used #9398

merged 33 commits into from
Jun 20, 2024

Conversation

martin-strecker-sonarsource
Copy link
Contributor

Fixes #7068

@martin-strecker-sonarsource martin-strecker-sonarsource marked this pull request as ready for review June 4, 2024 15:25
Tim-Pohlmann
Tim-Pohlmann requested changes Jun 4, 2024
View reviewed changes
Copy link
Contributor

@Tim-Pohlmann Tim-Pohlmann left a comment

Choose a reason for hiding this comment

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

Partial Review

analyzers/src/SonarAnalyzer.Common/Rules/MethodsShouldNotHaveIdenticalImplementationsBase.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/MethodsShouldNotHaveIdenticalImplementationsBase.cs Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/MethodsShouldNotHaveIdenticalImplementationsBase.cs Outdated Show resolved Hide resolved
Tim-Pohlmann
Tim-Pohlmann reviewed Jun 5, 2024
View reviewed changes
Copy link
Contributor

@Tim-Pohlmann Tim-Pohlmann left a comment

Choose a reason for hiding this comment

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

Review complete, only one minor thing in addition to the first pass.

...s/tests/SonarAnalyzer.Test/TestCases/MethodsShouldNotHaveIdenticalImplementations.CSharp9.cs Outdated Show resolved Hide resolved
Tim-Pohlmann
Tim-Pohlmann requested changes Jun 5, 2024
View reviewed changes
Copy link
Contributor

@Tim-Pohlmann Tim-Pohlmann left a comment

Choose a reason for hiding this comment

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

This PR is quite the journey :D

analyzers/tests/SonarAnalyzer.Test/Rules/MethodsShouldNotHaveIdenticalImplementationsTest.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.Test/Rules/MethodsShouldNotHaveIdenticalImplementationsTest.cs Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/MethodsShouldNotHaveIdenticalImplementationsBase.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.Test/Rules/MethodsShouldNotHaveIdenticalImplementationsTest.cs Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.Test/Rules/MethodsShouldNotHaveIdenticalImplementationsTest.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.Test/Rules/MethodsShouldNotHaveIdenticalImplementationsTest.cs Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.Test/Rules/MethodsShouldNotHaveIdenticalImplementationsTest.cs Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.TestFramework/Common/CombinatorialDataAttribute.cs Outdated Show resolved Hide resolved
Tim-Pohlmann
Tim-Pohlmann requested changes Jun 11, 2024
View reviewed changes
Copy link
Contributor

@Tim-Pohlmann Tim-Pohlmann left a comment

Choose a reason for hiding this comment

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

Looks very good but can be simplified more.

analyzers/src/SonarAnalyzer.Common/Rules/MethodsShouldNotHaveIdenticalImplementationsBase.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/MethodsShouldNotHaveIdenticalImplementationsBase.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/MethodsShouldNotHaveIdenticalImplementationsBase.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/MethodsShouldNotHaveIdenticalImplementationsBase.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/MethodsShouldNotHaveIdenticalImplementationsBase.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.Test/Rules/MethodsShouldNotHaveIdenticalImplementationsTest.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.Test/Rules/MethodsShouldNotHaveIdenticalImplementationsTest.cs Show resolved Hide resolved
Tim-Pohlmann
Tim-Pohlmann approved these changes Jun 12, 2024
View reviewed changes
Copy link
Contributor

@Tim-Pohlmann Tim-Pohlmann left a comment

Choose a reason for hiding this comment

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

Looks good. I added a comment why I think the simplification of the previous review is possible. Please take another look before merging.

@martin-strecker-sonarsource
Copy link
Contributor Author

I did quite some rewrite and so I better ask for review again..

Tim-Pohlmann
Tim-Pohlmann approved these changes Jun 14, 2024
View reviewed changes
Copy link
Contributor

@Tim-Pohlmann Tim-Pohlmann left a comment

Choose a reason for hiding this comment

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

Very nice refactor!

analyzers/src/SonarAnalyzer.Common/Rules/MethodsShouldNotHaveIdenticalImplementationsBase.cs Outdated
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 in HaveSameTypeParameters
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is outdated, HaveSameTypeParameters doesn't exist anymore.

analyzers/src/SonarAnalyzer.Common/Rules/MethodsShouldNotHaveIdenticalImplementationsBase.cs
&& first.ConstraintTypes.All(x => second.ConstraintTypes.Any(y => TypesAreSame(x, y)));
&& 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.

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jun 20, 2024

Quality Gate Passed Quality Gate passed for 'Sonar .NET Java Plugin'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jun 20, 2024

Quality Gate Passed Quality Gate passed for 'SonarAnalyzer for .NET'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@martin-strecker-sonarsource martin-strecker-sonarsource merged commit 33a85fe into master Jun 20, 2024
26 checks passed
@martin-strecker-sonarsource martin-strecker-sonarsource deleted the Martin/S4144_Generic branch June 20, 2024 13:53
@martin-strecker-sonarsource
Copy link
Contributor Author

Peach validation: One FN found in https://peach.aws-prd.sonarsource.com/code?id=servuo&selected=servuo:Server/Serialization.cs&line=963

public override void WriteMobileList<T>(List<T> list, bool tidy)

public override void WriteItemList<T>(List<T> list, bool tidy)

Both methods have the same body and were detected before as duplicates. With the new logic, the issue seems to get lost.

@martin-strecker-sonarsource
Copy link
Contributor Author

martin-strecker-sonarsource commented Jun 24, 2024 •
edited
Loading

Correction: These are TNs, as the generic constraints are defined on the virtual/abstract method but not on the override. I will add a reproducer, nevertheless, as this is only detectable by semantic checks.

@martin-strecker-sonarsource martin-strecker-sonarsource mentioned this pull request Jun 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tim-Pohlmann Tim-Pohlmann Tim-Pohlmann approved these changes

@costin-zaharia-sonarsource costin-zaharia-sonarsource Awaiting requested review from costin-zaharia-sonarsource

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix S4144 FP: when type constraints are used
2 participants
@martin-strecker-sonarsource @Tim-Pohlmann