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 S2699 FP: Support overridden methods annotated with AssertionMethodAttribute #6652
Fix S2699 FP: Support overridden methods annotated with AssertionMethodAttribute #6652
Conversation
fadd232
to
f00585f
Compare
[DataTestMethod] | ||
[DataRow(true, true)] | ||
[DataRow(false, true)] // The "Inherited" flag is not inherited for the AttributeUsage attribute itself. See also the SymbolHelperTest.GetAttributesWithInherited... tests, | ||
// where the reflection behavior of MemberInfo.GetCustomAttributes is also tested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit surprsing because AttributeUsageAttribute has Inherited = true
but it seems it is not respected by GetCustomAttributes
.
|
||
[DataTestMethod] | ||
[DataRow("BaseClass`1", "MyInheritedAttribute", "MyDerivedInheritedAttribute", "MyNotInheritedAttribute", "MyDerivedNotInheritedAttribute", "MyUnannotatedAttribute")] | ||
[DataRow("DerivedOpenGeneric`1", "MyInheritedAttribute", "MyDerivedInheritedAttribute", "MyDerivedNotInheritedAttribute", "MyUnannotatedAttribute")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here MyNotInherited
is not returned by GetCustomAttributes
as expected, but MyDerivedNotInherited
is returned. GetCustomAttributes is the source of truth and so we need to follow that behavior here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Round 1 - implementation. I didn't check UTs yet
analyzers/tests/SonarAnalyzer.UnitTest/TestFramework/SnippetCompiler.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestFramework/SnippetCompiler.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestFramework/SnippetCompiler.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestFramework/SnippetCompiler.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Pavel Mikula <57188685+pavel-mikula-sonarsource@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Round 2 on implementation. Few items were skipped
analyzers/src/SonarAnalyzer.Common/Extensions/AttributeDataExtensions.cs
Outdated
Show resolved
Hide resolved
@martin-strecker-sonarsource what is needed to close this one? |
@andrei-epure-sonarsource there were only some doc comment changes left which I just fixed. Can you approve or should I ask some else from the team? |
Can you please rebase, just to avoid surprises. As this has been around for a while and it bit me recently |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor polishing in the UTs
analyzers/tests/SonarAnalyzer.UnitTest/TestFramework/SnippetCompiler.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/Extensions/AttributeDataExtensionsTest.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/Extensions/AttributeDataExtensionsTest.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/Extensions/AttributeDataExtensionsTest.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/Extensions/AttributeDataExtensionsTest.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/Helpers/SymbolHelperTest.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/Helpers/SymbolHelperTest.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/Helpers/SymbolHelperTest.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/Helpers/SymbolHelperTest.cs
Outdated
Show resolved
Hide resolved
Kudos, SonarCloud Quality Gate passed! |
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I've converted it to draft to prevent merging before 8.54 release
ah, master is locked, we don't need drafts anymore |
Fixes #6525