Roslyn-analyzers: CA1501 AvoidExcessiveInheritance reports on all WPF Window/UserControl classes

[Gav-Brown]

Analyzer package

Microsoft.CodeQuality.Analyzers

Package Version

v2.6.2

Diagnostic ID

CA1501

Repro steps

1. Create WPF application with .xaml pages which inherit from Window or UserControl
   2, Compile

Expected behavior

Build succeeds

Actual behavior

Build error reported:

error CA1501: 'SomeTab' has an object hierarchy '9' levels deep within the defining module. If possible, eliminate base classes within the hierarchy to decrease its hierarchy level below '6': 'UserControl, ContentControl, Control, FrameworkElement, UIElement, Visual, DependencyObject, DispatcherObject, Object'

[Gav-Brown]

Using .NET Framework 4.7.2

[mavasani]

Thanks for the report. By the way as a workaround you can configure thresholds for each code metric rule by providing an additional file. See https://github.com/dotnet/roslyn-analyzers/blob/master/src/Microsoft.CodeQuality.Analyzers/UnitTests/Maintainability/CodeMetricsAnalyzerTests.cs

[mennesoft]

Isn't the core issue here that CA1501 is supposed to mark classes that are too deep "within the defining module"? Wouldn't the correct implementation of CA1501 simply check each class in the inheritance tree for it's namespace, and if the ancestor class is outside the namespace of the fully-derived class, just stop counting?

We're getting CA1501 with code like this:

namespace My.Company.Namespace
{
  public class MyTextBox : TextBox
  { 
    // ... snip ...
  }
}

Since MyTextBox is in namespace My.Company.Namespace, and TextBox is in System.Windows.Controls, this class should be a depth of 1 (or maybe 3 if you count "My", "Company" and "Namespace" separately), not 9 like FxCop is currently counting.

[Evangelink]

@sharwell, @mavasani Do you have any preference towards how to fix this issue?

For now, I can see a couple of solutions:
1/ Exclude everything under System.* namespace. That's not particularly nice if you want to use this analyzer while actually working on System types as you do at MS.

2/ Exclude system types in the config file. Or at least exclude some system types (winform, wpf, aspnet...).

3/ Use the namespace as suggested by @mennesoft. Maybe we could be a bit smarter here and compare start of namespace instead of exact match so that MyCompany.MyProduct.MyAssembly.SubNamespace.MyType : MyCompany.MyProduct.OtherAssembly.BaseType would still count BaseType.

---

EDIT:

This what I did for https://github.com/SonarSource/sonar-dotnet/blob/master/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Rules/AvoidExcessiveInheritance.cs

[Evangelink]

Giving some thoughts about this, I'd like to suggest to introduce 2 options for this rule.

Configure counting mechanism

This first option allows to select the counting mechanism of the rule from 3 options:

1. Count everything, this is the current behavior.

2. Count types from same "root namespace" - this will check whether the types have the same first part of the namespace (e.g. Foo.Bar : TextBox won't count anything but Foo.Bar : Foo.Baz will count 1). Note that if you are using the Company.Product.XXX notation this will consider types from the different products as part of the same "global namespace".

3. Count types from the same "product" - this will check whether the types have the same first two parts of the namespace (i.e. Company.Product).

Configure excluded types

This option will allow user to define fully qualified type names to exclude. The simplest mechanism we could allow would be fully qualified types + namespace.* option to exclude all types from some namespace (including sub-namespaces).

Of course we could put in place some more complex mechanism. For example we could allow the exclusion of all types within some namespace while keeping the types from sub-namespaces. We could allow '*' anywhere in the name and say for example all types ending with Attribute don't count or something like that.

I'd like to get some green light from your side before starting this implementation.

[Evangelink]

ping @mavasani

[mavasani]

@Evangelink I like your suggestion overall. For simplicity, can we start with just the second option, with the default excluded namespace being System.* namespaces? I think we can use the specification format similar to https://github.com/dotnet/roslyn-analyzers/blob/master/docs/Analyzer%20Configuration.md#excluded-symbol-names so users can specify namespaces and/or types to be excluded as appropriate.

[Evangelink]

Suggested solutions

1. excluded-symbol-names syntax

In this solution, we use the https://github.com/dotnet/roslyn-analyzers/blob/master/docs/Analyzer%20Configuration.md#excluded-symbol-names syntax without any additional change or support.

Pros:

• Everything is already in place
• Support specific type or namespace exclusion

Cons:

• No support for wildcards
• The default list will need to contain ALL System.XXX namespaces

2. excluded-symbol-names syntax + tweak

In this solution, we use the https://github.com/dotnet/roslyn-analyzers/blob/master/docs/Analyzer%20Configuration.md#excluded-symbol-names syntax and we will loop through the ContainingNamespace property to enable sub-namespace exclusion.

Pros:

• Easy to implement
• Default value for the option is simple N:System

Cons:

• Loses the ability to exclude higher namespaces without excluding sub-namespaces (e.g. N:System will exclude _ALL_ System sub-namespaces.
• No support for wildcards

3. New "parser"

In this solution, we will start from the SymbolNamesOption syntax but we will tweak the parser to allow the wildcard * symbol. Note that for simplicity and performance we could start with the support of only 1 wildcard which has to be the last character.

Pros:

• Not so complex to implement
• Default value for the option is simple N:System.*
• Support for wildcard (even if we start with only a simple one)
• Ability to exclude specific types or namespaces
• Ability to exclude namespace and sub-namespaces

Cons:

• Need to use ToDisplayString on symbols (the call will happen quite often)

Additional questions

• By adding the System.* exclusion by default we will stop counting the object base level, is this something we want? I could keep the exclusion and always add 1 to the count to artificially put back object.

@mavasani What's your opinion?

[mavasani]

@Evangelink Do you think you can update https://github.com/dotnet/roslyn-analyzers/blob/master/src/Utilities/Compiler/Options/SymbolNamesOption.cs to add wildcard support so all the existing options based on this type also get wildcard support? Currently, this type has 2 fields: https://github.com/dotnet/roslyn-analyzers/blob/96375ed2a27e265bd2a2405164a9e59c8f450909/src/Utilities/Compiler/Options/SymbolNamesOption.cs#L15-L16

1. _names: Set of names that will be used for directly symbol.Name match, not based on documentation comment ID.
2. _symbols: Set of symbols generated from documentation comment ID format specification in option value.

Following method uses these two sets to perform a match: https://github.com/dotnet/roslyn-analyzers/blob/96375ed2a27e265bd2a2405164a9e59c8f450909/src/Utilities/Compiler/Options/SymbolNamesOption.cs#L93-L94

We can add a new field, say _namesWithWildcards, where any name specification with a * gets added to this set. For simplicity, let us just allow a single * in the name specification. We need to then walk the ContainingSymbol chain and comparing symbol names with sub-parts in nameWithWildcard specification to see if get a match. We should not use ToDisplayString as that is allocation heavy.

[Evangelink]

@mavasani I sure can but I still have a few questions:

• Shall I bail out if there is a prefix or shall I handle it? If I handle it, do you confirm that System.* is equal to T:System.* and also N:System.*?

• Shall I consider that cases without any period and using wildcards apply to both types and namespaces or only types? For example, is System* supposed to exclude all types whose names start with System but also all types whose namespace starts with System?

• What about

  By adding the System.* exclusion by default we will stop counting the object base level, is this something we want? I could keep the exclusion and always add 1 to the count to artificially put back object.

[mavasani]

For 1., I think it is reasonable to support both if possible - System.* without a prefix excludes all symbols whose outermost containing symbol (excluding global namespace) has name System, but if there is a T: or N: prefix, that we also require to match the symbol kind.

For 2., IMO System* should match symbols whose ISymbol.Name starts with System.

For 3., I think it sounds fine to exclude object, I don't have strong opinion either way.