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 S2699 FP: Support overridden methods annotated with AssertionMethodAttribute #6652

Merged
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
992760e
WIP: Add support for InheritedAttributes for methods
martin-strecker-sonarsource Jan 17, 2023
5edbd1f
Remove unsed using
martin-strecker-sonarsource Jan 17, 2023
c55ae73
Add test for TypeSymbol and clean up
martin-strecker-sonarsource Jan 17, 2023
f00585f
Fix FP
martin-strecker-sonarsource Jan 17, 2023
12f4797
Move AttributeUsageInherited and add more tests.
martin-strecker-sonarsource Jan 23, 2023
adf4e69
Improve doc comment.
martin-strecker-sonarsource Jan 23, 2023
7bd37c9
More code comments for clarity
martin-strecker-sonarsource Jan 23, 2023
e6819a8
Apply suggestions from code review
martin-strecker-sonarsource Jan 23, 2023
f85288b
Fix after merge
martin-strecker-sonarsource Jan 23, 2023
31a72be
Make compilation private field
martin-strecker-sonarsource Jan 23, 2023
339746a
Add comment
martin-strecker-sonarsource Jan 23, 2023
a594007
Fix comment
martin-strecker-sonarsource Jan 23, 2023
2ebc6ac
Add doc comment
martin-strecker-sonarsource Feb 13, 2023
1fdc7ba
Remove doc comment.
martin-strecker-sonarsource Feb 13, 2023
8810dff
Add test for duplicate AttributeUsage attribute
martin-strecker-sonarsource Feb 16, 2023
afff954
Add inherit doc
martin-strecker-sonarsource Feb 16, 2023
835c6f0
Remove GetDiagnostics from SnippetCompiler and make Compilation publi…
martin-strecker-sonarsource Mar 2, 2023
e2224f8
Code review
martin-strecker-sonarsource Mar 2, 2023
3db33a0
Better align test cases
martin-strecker-sonarsource Mar 2, 2023
23501da
Simplify string
martin-strecker-sonarsource Mar 2, 2023
6e39239
inline assembly
martin-strecker-sonarsource Mar 2, 2023
825fedf
Simplify test class
martin-strecker-sonarsource Mar 2, 2023
297a031
Remove "My" prefix from attributes
martin-strecker-sonarsource Mar 2, 2023
42e1eb4
Rearrange test cases
martin-strecker-sonarsource Mar 2, 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
Expand Up @@ -152,6 +152,6 @@ private static bool IsTestIgnored(IMethodSymbol method)
|| methodSymbol.ContainingType.DerivesFromAny(KnownAssertionTypes);

private static bool IsCustomAssertion(ISymbol methodSymbol) =>
methodSymbol.GetAttributes().Any(x => x.AttributeClass.Name == CustomAssertionAttributeName);
methodSymbol.GetAttributesWithInherited().Any(x => x.AttributeClass.Name == CustomAssertionAttributeName);
}
}
Expand Up @@ -46,6 +46,25 @@ public static bool TryGetAttributeValue<T>(this AttributeData attribute, string
}
}

/// <summary>
/// Returns the <see cref="AttributeUsageAttribute.Inherited"/> setting for the attribute associated with <paramref name="attribute"/>.
/// The returned value is in line with the runtime behavior of <see cref="System.Reflection.MemberInfo.GetCustomAttributes(bool)"/>.
/// </summary>
pavel-mikula-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
/// <param name="attribute">The <see cref="AttributeData"/> of an <see cref="Attribute"/>.</param>
/// <returns>
/// The <see cref="AttributeUsageAttribute.Inherited"/> value of the <see cref="AttributeUsageAttribute"/> applied to the <paramref name="attribute"/>.
/// Returns <see langword="true"/> if no <see cref="AttributeUsageAttribute"/> is applied to the <see cref="Attribute"/> associated with <paramref name="attribute"/>
/// as this is the default behavior of <see cref="System.Reflection.MemberInfo.GetCustomAttributes(bool)"/>.
/// </returns>
martin-strecker-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
public static bool AttributeUsageInherited(this AttributeData attribute) =>
martin-strecker-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
attribute.AttributeClass.GetAttributes()
.Where(x => x.AttributeClass is { Name: nameof(AttributeUsageAttribute), ContainingNamespace.Name: "System" })
martin-strecker-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
.SelectMany(x => x.NamedArguments.Where(x => x.Key == nameof(AttributeUsageAttribute.Inherited)))
pavel-mikula-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
.Select(x => x.Value)
.Where(x => x is { Kind: TypedConstantKind.Primitive, Type.SpecialType: SpecialType.System_Boolean })
martin-strecker-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
.Select(x => (bool?)x.Value)
pavel-mikula-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
.FirstOrDefault() ?? true; // Default value of Inherited is true
pavel-mikula-sonarsource marked this conversation as resolved.
Show resolved Hide resolved

private static bool TryConvertConstant<T>(TypedConstant constant, out T value)
{
value = default;
Expand Down
28 changes: 28 additions & 0 deletions analyzers/src/SonarAnalyzer.Common/Helpers/SymbolHelper.cs
Expand Up @@ -209,6 +209,34 @@ public static bool IsPubliclyAccessible(this ISymbol symbol)
symbol?.GetAttributes().Where(a => a.AttributeClass.IsAny(attributeTypes))
?? Enumerable.Empty<AttributeData>();

internal static IEnumerable<AttributeData> GetAttributesWithInherited(this ISymbol symbol)
martin-strecker-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
pavel-mikula-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
{
foreach (var attribute in symbol.GetAttributes())
{
yield return attribute;
}

var baseSymbol = GetBaseSymbol(symbol);
while (baseSymbol != null)
martin-strecker-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
{
foreach (var attribute in baseSymbol.GetAttributes().Where(x => x.AttributeUsageInherited()))
{
yield return attribute;
}

baseSymbol = GetBaseSymbol(baseSymbol);
}

static ISymbol GetBaseSymbol(ISymbol symbol) =>
martin-strecker-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
symbol switch
{
INamedTypeSymbol namedType => namedType.BaseType,
IMethodSymbol { OriginalDefinition: { } originalDefinition } method when !method.Equals(originalDefinition) => GetBaseSymbol(originalDefinition),
IMethodSymbol { IsOverride: true, OverriddenMethod: { } overridenMethod } => overridenMethod,
martin-strecker-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
_ => null,
pavel-mikula-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
};
}

internal static bool AnyAttributeDerivesFrom(this ISymbol symbol, KnownType attributeType) =>
symbol?.GetAttributes().Any(a => a.AttributeClass.DerivesFrom(attributeType)) ?? false;

Expand Down
Expand Up @@ -141,6 +141,102 @@ public void TryGetAttributeValue_ObjectConversion(object value, Type expectedTyp
actualValue.Should().Be(value);
}

[DataTestMethod]
[DataRow(true)]
[DataRow(false)]
public void AttributeUsageInherited_InheritedSpecified(bool inherited)
{
var code = $$"""
using System;

[AttributeUsage(AttributeTargets.All, Inherited = {{(inherited ? "true" : "false")}})]
pavel-mikula-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
public class MyAttribute: Attribute
{
}

[My]
public class Program
{
}
""";
var snippet = new SnippetCompiler(code);
var attribute = snippet.GetTypeSymbol("Program").GetAttributes().Single(x => x.HasName("MyAttribute"));
pavel-mikula-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
var actual = attribute.AttributeUsageInherited();
actual.Should().Be(inherited);
}

[TestMethod]
public void AttributeUsageInherited_InheritedUnSpecified()
{
var code = $$"""
using System;

[AttributeUsage(AttributeTargets.All)]
public class MyAttribute: Attribute
{
}

[My]
public class Program
{
}
""";
var snippet = new SnippetCompiler(code);
var attribute = snippet.GetTypeSymbol("Program").GetAttributes().Single(x => x.HasName("MyAttribute"));
var actual = attribute.AttributeUsageInherited();
actual.Should().Be(true); // The default for Inherited = true
}

[TestMethod]
public void AttributeUsageInherited_NoUsageAttribute()
{
var code = $$"""
using System;

public class MyAttribute: Attribute
{
}

[My]
public class Program
{
}
""";
var snippet = new SnippetCompiler(code);
var attribute = snippet.GetTypeSymbol("Program").GetAttributes().Single(x => x.HasName("MyAttribute"));
var actual = attribute.AttributeUsageInherited();
actual.Should().Be(true); // The default for Inherited = true
}

[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.
Copy link
Contributor Author

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.

public void AttributeUsageInherited_UsageInherited(bool inherited, bool expected)
{
var code = $$"""
using System;

[AttributeUsage(AttributeTargets.All, Inherited = {{(inherited ? "true" : "false")}})]
public class BaseAttribute: Attribute
{
}

public class MyAttribute: BaseAttribute
{
}

[My]
public class Program
{
}
""";
var snippet = new SnippetCompiler(code);
var attribute = snippet.GetTypeSymbol("Program").GetAttributes().Single(x => x.HasName("MyAttribute"));
var actual = attribute.AttributeUsageInherited();
actual.Should().Be(expected);
}

private static AttributeData AttributeDataWithName(string attributeClassName)
{
var namedType = new Mock<INamedTypeSymbol>();
Expand Down
180 changes: 180 additions & 0 deletions analyzers/tests/SonarAnalyzer.UnitTest/Helpers/SymbolHelperTest.cs
Expand Up @@ -398,5 +398,185 @@ public void GetClassification_Method(MethodKind methodKind, string expected)

fakeSymbol.Object.GetClassification().Should().Be(expected);
}

[DataTestMethod]
[DataRow("BaseClass`1", "VirtualMethod", "MyInheritedAttribute", "MyDerivedInheritedAttribute", "MyNotInheritedAttribute", "MyDerivedNotInheritedAttribute", "MyUnannotatedAttribute")]
pavel-mikula-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
pavel-mikula-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
pavel-mikula-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
[DataRow("DerivedOpenGeneric`1", "VirtualMethod", "MyInheritedAttribute", "MyDerivedInheritedAttribute", "MyDerivedNotInheritedAttribute", "MyUnannotatedAttribute")]
[DataRow("DerivedOpenGeneric`1", "NonVirtualMethod")]
[DataRow("DerivedOpenGeneric`1", "GenericVirtualMethod`1", "MyInheritedAttribute", "MyDerivedInheritedAttribute", "MyDerivedNotInheritedAttribute", "MyUnannotatedAttribute")]
[DataRow("DerivedOpenGeneric`1", "GenericNonVirtualMethod`1")]
[DataRow("DerivedClosedGeneric", "VirtualMethod", "MyInheritedAttribute", "MyDerivedInheritedAttribute", "MyDerivedNotInheritedAttribute", "MyUnannotatedAttribute")]
[DataRow("DerivedClosedGeneric", "NonVirtualMethod")]
[DataRow("DerivedClosedGeneric", "GenericVirtualMethod`1", "MyInheritedAttribute", "MyDerivedInheritedAttribute", "MyDerivedNotInheritedAttribute", "MyUnannotatedAttribute")]
[DataRow("DerivedClosedGeneric", "GenericNonVirtualMethod`1")]
[DataRow("DerivedNoOverrides`1", "VirtualMethod", "MyInheritedAttribute", "MyDerivedInheritedAttribute", "MyNotInheritedAttribute", "MyDerivedNotInheritedAttribute", "MyUnannotatedAttribute")]
[DataRow("DerivedNoOverrides`1", "NonVirtualMethod", "MyInheritedAttribute", "MyDerivedInheritedAttribute", "MyNotInheritedAttribute", "MyDerivedNotInheritedAttribute", "MyUnannotatedAttribute")]
[DataRow("DerivedNoOverrides`1", "GenericVirtualMethod`1", "MyInheritedAttribute", "MyDerivedInheritedAttribute", "MyNotInheritedAttribute", "MyDerivedNotInheritedAttribute", "MyUnannotatedAttribute")]
[DataRow("DerivedNoOverrides`1", "GenericNonVirtualMethod`1", "MyInheritedAttribute", "MyDerivedInheritedAttribute", "MyNotInheritedAttribute", "MyDerivedNotInheritedAttribute", "MyUnannotatedAttribute")]
public void GetAttributesWithInherited_MethodSymbol(string className, string methodName, params string[] expectedAttributes)
pavel-mikula-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
{
var code = $$"""
using System;

[AttributeUsage(AttributeTargets.All, Inherited = true)]
public class MyInheritedAttribute : Attribute { }

[AttributeUsage(AttributeTargets.All, Inherited = false)]
public class MyNotInheritedAttribute : Attribute { }

public class MyDerivedInheritedAttribute: MyInheritedAttribute { }

public class MyDerivedNotInheritedAttribute: MyNotInheritedAttribute { }

public class MyUnannotatedAttribute : Attribute { }

public class BaseClass<T1>
{
[MyInheritedAttribute]
[MyDerivedInheritedAttribute]
pavel-mikula-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
[MyNotInherited]
[MyDerivedNotInherited]
[MyUnannotatedAttribute]
public virtual void VirtualMethod() { }

[MyInheritedAttribute]
[MyDerivedInheritedAttribute]
[MyNotInherited]
[MyDerivedNotInherited]
[MyUnannotatedAttribute]
public void NonVirtualMethod() { }

[MyInheritedAttribute]
[MyDerivedInheritedAttribute]
[MyNotInherited]
[MyDerivedNotInherited]
[MyUnannotatedAttribute]
public void GenericNonVirtualMethod<T2>() { }

[MyInheritedAttribute]
[MyDerivedInheritedAttribute]
[MyNotInherited]
[MyDerivedNotInherited]
[MyUnannotatedAttribute]
public virtual void GenericVirtualMethod<T2>() { }
}

public class DerivedOpenGeneric<T1>: BaseClass<T1>
{
public override void VirtualMethod() { }
public new void NonVirtualMethod() { }
public new void GenericNonVirtualMethod<T2>() { }
public override void GenericVirtualMethod<T2>() { }
}

public class DerivedClosedGeneric: BaseClass<int>
{
public override void VirtualMethod() { }
public new void NonVirtualMethod() { }
public new void GenericNonVirtualMethod<T2>() { }
public override void GenericVirtualMethod<T2>() { }
}

public class DerivedNoOverrides<T>: BaseClass<T>
{
}

public class Program
{
public static void Main()
{
new {{className.Replace(@"`1", "<int>")}}().{{methodName.Replace(@"`1", "<int>")}}();
}
}
""";
var compiler = new SnippetCompiler(code);
var invocationExpression = compiler.GetNodes<InvocationExpressionSyntax>().Should().ContainSingle().Subject;
var method = compiler.GetSymbol<IMethodSymbol>(invocationExpression);
var actual = method.GetAttributesWithInherited().Select(x => x.AttributeClass.Name).ToList();
actual.Should().BeEquivalentTo(expectedAttributes);

// GetAttributesWithInherited should behave like MemberInfo.GetCustomAttributes from runtime reflection:
var assembly = compiler.GetEmittedAssembly();
var type = assembly.GetType(className, throwOnError: true);
var methodInfo = type.GetMethod(methodName.Replace("`1", string.Empty));
methodInfo.GetCustomAttributes(inherit: true).Select(x => x.GetType().Name).Should().BeEquivalentTo(expectedAttributes);
}

[DataTestMethod]
[DataRow("BaseClass`1", "MyInheritedAttribute", "MyDerivedInheritedAttribute", "MyNotInheritedAttribute", "MyDerivedNotInheritedAttribute", "MyUnannotatedAttribute")]
pavel-mikula-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
[DataRow("DerivedOpenGeneric`1", "MyInheritedAttribute", "MyDerivedInheritedAttribute", "MyDerivedNotInheritedAttribute", "MyUnannotatedAttribute")]
Copy link
Contributor Author

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.

[DataRow("DerivedClosedGeneric", "MyInheritedAttribute", "MyDerivedInheritedAttribute", "MyDerivedNotInheritedAttribute", "MyUnannotatedAttribute")]
[DataRow("Implement")]
public void GetAttributesWithInherited_TypeSymbol(string className, params string[] expectedAttributes)
{
var code = $$"""
using System;

[AttributeUsage(AttributeTargets.All, Inherited = true)]
public class MyInheritedAttribute : Attribute { }

[AttributeUsage(AttributeTargets.All, Inherited = false)]
public class MyNotInheritedAttribute : Attribute { }

public class MyDerivedInheritedAttribute: MyInheritedAttribute { }

public class MyDerivedNotInheritedAttribute: MyNotInheritedAttribute { }

public class MyUnannotatedAttribute : Attribute { }

[MyInheritedAttribute]
[MyDerivedInheritedAttribute]
[MyNotInherited]
[MyDerivedNotInherited]
[MyUnannotatedAttribute]
public class BaseClass<T1>
{
}

[MyInheritedAttribute]
[MyDerivedInheritedAttribute]
[MyNotInherited]
[MyDerivedNotInherited]
[MyUnannotatedAttribute]
public interface IInterface
{
}

public class DerivedOpenGeneric<T1>: BaseClass<T1>
{
}

public class DerivedClosedGeneric: BaseClass<int>
{
}

public class Implement: IInterface
{
}

public class Program
{
public static void Main()
{
new {{className.Replace(@"`1", "<int>")}}();
}
}
""";
var compiler = new SnippetCompiler(code);
var objectCreation = compiler.GetNodes<ObjectCreationExpressionSyntax>().Should().ContainSingle().Subject;
if (compiler.GetSymbol<IMethodSymbol>(objectCreation) is { MethodKind: MethodKind.Constructor, ReceiverType: { } receiver })
{
var actual = receiver.GetAttributesWithInherited().Select(x => x.AttributeClass.Name).ToList();
actual.Should().BeEquivalentTo(expectedAttributes);
}
else
{
Assert.Fail("Constructor could not be found.");
}
// GetAttributesWithInherited should behave like MemberInfo.GetCustomAttributes from runtime reflection:
var assembly = compiler.GetEmittedAssembly();
var type = assembly.GetType(className, throwOnError: true);
type.GetCustomAttributes(inherit: true).Select(x => x.GetType().Name).Should().BeEquivalentTo(expectedAttributes);
}
}
}
Expand Up @@ -64,7 +64,7 @@ public class BaseTest
public class DerivedTest : BaseTest
{
[TestMethod]
public void Derived() // Noncompliant FP: The overridden method needs to be annotated because Roslyn does not respect AttributeUsage.Inherited in ISymbol.GetAttributes
public void Derived() // Compliant
{
CustomAssertionMethod();
}
Expand Down