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

S2699 Reproducer for AssertionMethodAttribute is ignored when assertion method is inherited #6528

Conversation

martin-strecker-sonarsource
Copy link
Contributor

Test for #6525

@sonarcloud
Copy link

sonarcloud bot commented Dec 13, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarcloud
Copy link

sonarcloud bot commented Dec 13, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Contributor

@cristian-ambrosini-sonarsource cristian-ambrosini-sonarsource left a comment

Choose a reason for hiding this comment

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

Based on the result of the following snippet it seems to me that this is not a FP.
The following will print 'false' both with this implementation and with the base.MyMethod(); call inside Child MyMethod().
It seems to me that once overriden the attributes are lost. For this reason I think the CustomAssertionMethod() within this test case is not decorated by [AssertionMethod] attribute anymore, hence raising the issue. WDYT?

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

public class Parent
{
    [My]
    public virtual void MyMethod() { }
}

public class Child : Parent
{
    public override void MyMethod() { }
}

public class MainClass
{
    public static void Main() =>
        Console.WriteLine(typeof(Child).GetMember("MyMethod")
                                       .SelectMany(m => m.CustomAttributes)
                                       .Any());
}

@github-actions github-actions bot moved this from Review in progress to In progress in Best Kanban Dec 14, 2022
@martin-strecker-sonarsource
Copy link
Contributor Author

The code will print true when you do these variations to the code:

  • AttributeUsage.Inherited = true (Note: Inherited = true is the default)
  • m.GetCustomAttributes(inherit: true)
using System;
using System.Linq;

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

public class Parent
{
    [My]
    public virtual void MyMethod() { }
}

public class Child : Parent
{
    public override void MyMethod() { }
}

public class MainClass
{
    public static void Main() =>
        Console.WriteLine(typeof(Child).GetMember("MyMethod")
                                       .SelectMany(m => m.GetCustomAttributes(inherit: true))
                                       .OfType<MyAttribute>()
                                       .Any());
}

So we should check if there is an AssertionMethodAttribute in the compilation and if it is marked with AttributeUsage.Inherited = true. If so we should look at the original definition (in case of generic parameters as in the OPs request) and the override chain if the attribute was applied somewhere before. There might be some helper method in Roslyn that helps with this, but I couldn't find anything at first glance.

Best Kanban automation moved this from Review in progress to Review approved Dec 15, 2022
@cristian-ambrosini-sonarsource cristian-ambrosini-sonarsource deleted the Martin/S2699_DerivedCustomAssertionMethod branch December 15, 2022 16:06
Best Kanban automation moved this from Review approved to Validate Peach Dec 15, 2022
@martin-strecker-sonarsource martin-strecker-sonarsource moved this from Validate Peach to Done in Best Kanban Dec 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Best Kanban
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants