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 16 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,14 @@ public static bool TryGetAttributeValue<T>(this AttributeData attribute, string
}
}

public static bool HasAttributeUsageInherited(this AttributeData attribute) =>
attribute.AttributeClass.GetAttributes()
.Where(x => x.AttributeClass.Is(KnownType.System_AttributeUsageAttribute))
.SelectMany(x => x.NamedArguments.Where(x => x.Key == nameof(AttributeUsageAttribute.Inherited)))
pavel-mikula-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
.Where(x => x.Value is { Kind: TypedConstantKind.Primitive, Type.SpecialType: SpecialType.System_Boolean })
.Select(x => (bool?)x.Value.Value)
.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
35 changes: 35 additions & 0 deletions analyzers/src/SonarAnalyzer.Common/Helpers/SymbolHelper.cs
Expand Up @@ -19,6 +19,7 @@
*/

using System.Reflection;
using static System.Net.WebRequestMethods;

namespace SonarAnalyzer.Helpers
{
Expand Down Expand Up @@ -209,6 +210,40 @@ public static bool IsPubliclyAccessible(this ISymbol symbol)
symbol?.GetAttributes().Where(a => a.AttributeClass.IsAny(attributeTypes))
?? Enumerable.Empty<AttributeData>();

/// <summary>
/// Returns attributes for the symbol by also respecting <see cref="AttributeUsageAttribute.Inherited"/>.
/// The returned <see cref="AttributeData"/> is consistent with the results from <see cref="MemberInfo.GetCustomAttributes(bool)"/>.
/// </summary>
public static IEnumerable<AttributeData> GetAttributesWithInherited(this ISymbol symbol)
{
foreach (var attribute in symbol.GetAttributes())
{
yield return attribute;
}

var baseSymbol = BaseSymbol(symbol);
while (baseSymbol is not null)
{
foreach (var attribute in baseSymbol.GetAttributes().Where(x => x.HasAttributeUsageInherited()))
{
yield return attribute;
}

baseSymbol = BaseSymbol(baseSymbol);
}

static ISymbol BaseSymbol(ISymbol symbol) =>
symbol switch
{
INamedTypeSymbol namedType => namedType.BaseType,
IMethodSymbol { OriginalDefinition: { } originalDefinition } method when !method.Equals(originalDefinition) => BaseSymbol(originalDefinition),
IMethodSymbol { OverriddenMethod: { } overridenMethod } => overridenMethod,
// Support for other kinds of symbols needs to be implemented/tested as needed. A full list can be found here:
// https://learn.microsoft.com/dotnet/api/system.attributetargets
_ => 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 @@ -21,6 +21,7 @@
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Moq;
using Moq.Protected;
using SonarAnalyzer.Common;
using SonarAnalyzer.Extensions;

namespace SonarAnalyzer.UnitTest.Extensions
Expand Down Expand Up @@ -141,6 +142,126 @@ public void TryGetAttributeValue_ObjectConversion(object value, Type expectedTyp
actualValue.Should().Be(value);
}

[DataTestMethod]
[DataRow(true)]
[DataRow(false)]
public void HasAttributeUsageInherited_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.HasAttributeUsageInherited();
actual.Should().Be(inherited);
}

[TestMethod]
public void HasAttributeUsageInherited_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.HasAttributeUsageInherited();
actual.Should().Be(true); // The default for Inherited = true
}

[TestMethod]
public void HasAttributeUsageInherited_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.HasAttributeUsageInherited();
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 HasAttributeUsageInherited_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.HasAttributeUsageInherited();
actual.Should().Be(expected);
}

[TestMethod]
public void HasAttributeUsageInherited_DuplicateAttributeUsage()
{
var code = $$"""
pavel-mikula-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
pavel-mikula-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
using System;

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

[My]
public class Program
{
}
""";
var snippet = new SnippetCompiler(code, ignoreErrors: true, AnalyzerLanguage.CSharp);
var attribute = snippet.GetTypeSymbol("Program").GetAttributes().Single(x => x.HasName("MyAttribute"));
var actual = attribute.HasAttributeUsageInherited();
snippet.GetDiagnostics().Should().ContainEquivalentOf(new { Id = "CS0579" }); // "Duplicate 'AttributeUsage' attribute"
actual.Should().BeTrue();
pavel-mikula-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
}

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.EmitAssembly();
var type = assembly.GetType(className, throwOnError: true);
pavel-mikula-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
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.EmitAssembly();
var type = assembly.GetType(className, throwOnError: true);
pavel-mikula-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
type.GetCustomAttributes(inherit: true).Select(x => x.GetType().Name).Should().BeEquivalentTo(expectedAttributes);
}
}
}