CA1710: Suffix Requirement For Abstract Types?

[TheXenocide]

Analyzer package

Microsoft.CodeAnalysis.FxCopAnalyzers

Package Version

v2.9.8

Diagnostic ID

CA1710: Identifiers should have correct suffix

Description

The following class reports CS1710 because it believes that types that inherit from Attribute should be suffixed with Attribute, which is generally well and good, especially in the case of attributes since common usage dictates that the suffix can be left off when applying attributes to code elsewhere.

public abstract class HostTestMethodAttributeBase : TestMethodAttribute
{
    // ...
}

In this particular example, however, the offending type could not actually be applied to any method which negates that part of the value of the suffix validation. Additionally, since this abstract class is intended to serve as a reusable foundation for inheriting types, not to be used directly, it seems reasonable that we would prefer the (also fairly common) *Base suffix.

---

I'm not sure what approach I think would be most appropriate here. I suppose it could be configurable, it could ignore abstract types or be more discerning in the case of abstract types it could perhaps check to see if it's using *<Expected Suffix>Base (e.g. ApiExceptionBase, CustomAspectAttributeBase, StreamingCollectionBase, etc.) and allow that as an acceptable variation of the suffix. I actually almost like the option of suggesting all abstract types use the suffix Base specifically, though I'm not sure if that is quite as ubiquitous of a standard as this particular analyzer intends.

[Evangelink]

Hi @TheXenocide,

With v3+ of the analyzer you won't have the issue anymore because indirect type detection was deactivated by default (see https://github.com/dotnet/roslyn-analyzers/blob/master/docs/Analyzer%20Configuration.md#exclude-indirect-base-types). Note that you will also be able to specify custom required suffixes (see https://github.com/dotnet/roslyn-analyzers/blob/master/docs/Analyzer%20Configuration.md#additional-required-suffixes).

Regarding the possibility to also tweak the rule to exclude the Base suffix, I'll let @mavasani decides what he wants to do. I don't have any strong opinion either way (implementing or not) but I don't see the need to have a user-option allowing custom suffixes to be ignored.

[TheXenocide]

The issue I'm having isn't from the implementing class (which indirectly inherits from Attribute, but is also appropriately suffixed with Attribute since it can actually be used as an attribute), but rather the abstract class itself (which is abstract and suffixed "AttributeBase", but inherits from Attribute directly and reports the warning). With that, I don't think the indirect default change impacts this issue for us. It also doesn't look like there's any way we could use the configurable additional required suffixes since it's purely inheritance based, so we can't tell it "all abstract classes should end in Base" - I'm not really sure how it would behave if multiple expected suffixes were required, even if that were possible. If two possible expected suffixes applied (e.g. inherit from Attribute AND implement some collection interface with a required naming convention), but that behavior would likely apply here as well (if we could specify the Base suffix)

[TheXenocide]

I'm actually not so sure I like disabling the indirect detection by default. Wouldn't that mean that an exception class that inherits from ApplicationException or some other existing Exception will no longer warn that it should end with the "Exception" suffix?

[Evangelink]

Thank you for your comments!

Regarding the Base suffix for abstract type, I agree that there is currently no way to achieve such behavior. I am wondering if it wouldn't be better to have a separate rule for abstract type naming, as I often see the Abstract prefix naming convention too (for example, that's what is used in roslyn and in this repo). This rule would allow to give a prefix OR a suffix that should be matched by any abstract type. @mavasani WDYT?

Regarding the ApplicationException, you are right, and I think that's a mistake we made... We had a lot of issues with false-positives on collection types and it seems not enough thoughts and tests for the other cases so we forgot to handle them correctly. @mavasani let me know if you agree that Exception, Attribute and maybe EventArgs should have a special behavior ignoring the exclude_indirect_base_types option.

Here are some tests to show the behavior:

[Fact]
public async Task TypeInheritsIndirectlyFromException_Diagnostic()
{
    await VerifyCS.VerifyAnalyzerAsync(@"
using System;

public class C : ApplicationException {} // No issue on C but it would make sense to have one
",
        GetCA1710CSharpResultAt(4, 14, "C", "Exception"));

    await VerifyVB.VerifyAnalyzerAsync(@"
Imports System

Public Class C ' No issue on C but it would make sense to have one
Inherits ApplicationException
End Class
",
        GetCA1710BasicResultAt(4, 14, "C", "Exception"));
}

[Fact]
public async Task TypeInheritsIndirectlyFromAttribute_Diagnostic()
{
    await VerifyCS.VerifyAnalyzerAsync(@"
using System;

public class MyAttribute : Attribute {}

public class C : MyAttribute {} // No issue on C but it would make sense to have one
",
        GetCA1710CSharpResultAt(6, 14, "C", "Attribute"));

    await VerifyVB.VerifyAnalyzerAsync(@"
Imports System

Public Class MyAttribute
Inherits Attribute
End Class

Public Class C ' No issue on C but it would make sense to have one
Inherits MyAttribute
End Class
",
        GetCA1710CSharpResultAt(8, 14, "C", "Attribute"));
}

[TheXenocide]

@Evangelink A separate rule seems like it might make sense for that, though that still leaves me with gray area around how the scenario that an abstract class that inherits from a type with an expected suffix plays out. If the abstract suffix were added but didn't coordinate with this particular analyzer then one of them will always raise a flag (if your type ends in Attribute it will complain that it doesn't end in Base, but if your type ends in Base then it will complain that it doesn't end in Attribute). In my ideal world I would want it to enforce both combined (so an abstract attribute class would have to be named *AttributeBase or else get a warning). At a minimum I think it would be very helpful to be able to turn off CA1710 just for abstract types. A more complete solution might be to make individual suffixes configurable, though I think in my case I want all suffixes from CA1710 disabled for abstract types.

[Evangelink]

@mavasani I think it'd be nice to take a decision on this ticket which is now quite old.