Fix S1200: Should not count generic type parameters of extension methods

[antoinebj]

Description

Violation of rule S1200 is raised excessively for some static classes containing extension methods that extend generic types.

Repro steps

1. Define a class containing a few extension methods for a generic type, such as:

public static class DictionaryExtensions
{
    public static TValue GetOrAdd<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key, Func<TValue> createValue)
    {
        if (!dictionary.TryGetValue(key, out var value))
        {
            dictionary[key] = value = createValue();
        }

        return value;
    }

    public static TValue GetValueOrDefault<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key)
    {
        dictionary.TryGetValue(key, out var obj);
        return obj;
    }

    public static TValue GetValueOrDefault<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key, TValue defaultValue)
    {
        return dictionary.TryGetValue(key, out var value) ? value : defaultValue;
    }

    public static TValue GetValueOrDefault<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key, Func<TValue> getDefaultValue)
    {
        return dictionary.TryGetValue(key, out var value) ? value : getDefaultValue();
    }

    public static IEnumerable<TValue> SelectValueByKey<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key)
    {
        if (dictionary.TryGetValue(key, out var value))
        {
            yield return value;
        }
    }

    public static ReadOnlyDictionary<TKey, TValue> AsReadOnly<TKey, TValue>(this IDictionary<TKey, TValue> dictionary)
    {
        return new ReadOnlyDictionary<TKey, TValue>(dictionary);
    }
}

2. Run analysis with rule S1200 activated with its default tolerance of 20

Expected behavior

The analyzer should not request that this class be split, i.e. rule S1200 should not have been violated by that code.

Actual behavior

Each generic type parameter of a method is considered a different dependency for the class. In turn, other generic types using the method's generic types as type parameters are considered as dependencies.

In the above example, TKey, TValue, IDictionary<TKey, TValue> and Func<TValue> are counted as separate dependencies every time they are redefined in a method.

This adds up very quickly to 21 in the above example, above the tolerance of 20, and therefore raises a rule violation.

Related information

• SonarC# Version 6.8.1.4648
• Visual Studio Version 15.5.4

[antoinebj]

To broaden the subject of the issue, primitive types such as int or string are not counted, but Task<int> and Task<string> are counted both separately. This does not seem quite fair.

I read #548 where Task, Func & Action are mentioned. It was suggested to exclude them from the count, but it was not implemented. I'd be grateful for an explanation if you have time. Perhaps the default tolerance of 20 is supposed to compensate?

If you're inclined to go over this rule again, my current feeling is that all basic generic types should be excluded:

• Func
• Action
• Task
• Array
• Almost anything from System.Collections.* namespaces
• Possibly others

Or, since this list might be too hard to get just right, maybe as an alternative you could only count generic type definitions instead of generic types (obtained through Type.GetGenericTypeDefinition()). If you choose do that, don't forget nested generic types, IEnumerable<KeyValuePair<int, string>> + IEnumerable<KeyValuePair<short, string>> would only count for 1 IEnumerable<> and 1 KeyValuePair<,>.

[antoinebj]

As yet another alternative, I have found that the default of 30 that FxCop rule CA1506 applies seems more suitable, nearly all of our classes which have a reasonable level of responsability are under 30.
Is there a particular reason you set it to 20 by default in Sonar?

[Evangelink]

Hi @antoinebj,

Thanks again (for your other issue) for this feedback!

I cannot remember why I have used a default value of 20 (probably because that's the value used by Java).

I am not really sure which direction we should go for so I will have a chat with the rest of the team to gather opinions and will get back to you right after so we can discuss about it.

[Evangelink]

Alright I did some tests and I have noticed that the implementation is quite buggy (especially around generics). We are working on another topic at the moment but I'll update your ticket (mark it as Confirmed and FP).

I have read all your suggestions and here is what I think:

• primitive types are excluded
• every generic type is split (e.g. Dictionary<Car, Person> will be treated as Dictionary<>, Car and Person so if you use another Dictionary it doesn't count twice).

WDYT?

[antoinebj]

Thanks for your replies

I wanted to get back to you on that earlier, but had little time lately.
I suspected that you had chosen 20 because it was the value for Java. However, the algorithms are probably not identical, the one for Java could be smarter and not require a high threshold to compensate for double or triple-counting.

For future reference, I'd like to illustrate the reason why I think the over-simplistic algorithm of CA1506 should not be used as a basis.

Imagine someone coded in C# 1 / .NET 1.0 style, without generics:

void DoSomething(IList listOfAs, IList listOfBs, IList listOfStrings)
{
    A itemOfTypeA = (A)listOfAs[0];
    B itemOfTypeB = (B)listOfBs[0];
    string itemOfTypeString = (string)listOfStrings[0];
}

CA1506 (and S1200 which mimicks it) would count 3 dependencies: IList, A, and B

Now let's write the exact same thing C# 2 / .NET 2.0 style:

void DoSomething(IList<A> listOfAs, IList<B> listOfBs, IList<string> listOfStrings)
{
    A itemOfTypeA = listOfAs[0];
    B itemOfTypeB = listOfBs[0];
    string itemOfTypeString = listOfStrings[0];
}

Boom, now we have 5 dependencies (+66%): IList<A>, IList<B>, IList<string>, A, and B

It gets worse if we have another method in the class:

static A GetFirstItemOfTypeA(IEnumerable<A> collectionOfAs) => collectionOfAs.First();

and make the previous example call it:

void DoSomething(IList<A> listOfAs, IList<B> listOfBs, IList<string> listOfStrings)
{
    A itemOfTypeA = GetFirstItemOfTypeA(listOfAs);
    B itemOfTypeB = listOfBs[0];
    string itemOfTypeString = listOfStrings[0];
}

Now for the same conceptual object, you have a 6th dependency: IEnumerable<A>

Imagine other methods taking IReadOnlyList<A>, or ICollection<A> depending on what they plan to do with the original list. Every time, it adds a dependency.

So I agree with the 2 points you suggest:

• primitive types are excluded
• every generic type is split

But I'll add a 3rd, which I believe requires consideration:

• if the class depends on a type, any occurrence of one of the ancestor types of that type, or any occurrence of the interfaces implemented by that type should not be counted
  In other words, if SelectedItemCollection (in WPF) is counted as a dependency of a class, then none of the following should count as additional dependencies if they are also used in that class:
  ObservableCollection<object> (1st ancestor), Collection<object> (2nd and last ancestor), IList<object>, ICollection<object>, IReadOnlyList<object>, IReadOnlyCollection<object>, IEnumerable<object>, IList, ICollection, IEnumerable, INotifyCollectionChanged, INotifyPropertyChanged

I may not have thought it entirely through, so I wouldn't be surprised if you find something wrong with that 3rd point. Please do let me know what you think.

[antoinebj]

I believe you had a reply about generics, which I received as e-mail notification, but you probably deleted it afterwards. Just in case, I'll address it anyway.

Let's take this method (it's inspired from your example from the deleted reply):

public static TValue GetValueOrDefault<TKey, TValue>(this Dictionary<TKey, TValue> dictionary, TKey key) {}

For that method, I would only count Dictionary<,>, and not even TKey that is passed as parameter.
If a class contained only this method, it would not depend on TKey, nor TValue, you could place it in any project as long as Dictionary<,> were available.
Unless the method body casts the generic type parameters, it can only treat them as object.
If it did cast them, those casts would be counted on their own merit.

To futher illustrate why I would not count TKey nor TValue, let's see how the above would look without generics:

public static object GetValueOrDefault(this HashTable dictionary, object key) {}

(HashTable is what preceded Dictionary<,> before .NET 2.0)
That method only uses HashTable and Object (twice). Object is a primitive and doesn't count, so only HashTable is left : it's 1 dependency.
In the count of dependencies, there should be as little discrepancy between generics-based code and non-generics-based code as possible, otherwise:

1. It's not conceptually fair.
2. Unscrupulous developers will try to pass quality gates or avoid warnings by regressing to non-generics. And these can be the kind of developers which triggered the desire to use this tool in the first place.

Now if the generic had type constraints:

public static TValue GetValueOrDefault<TKey, TValue>(this Dictionary<TKey, TValue> dictionary, TKey key)
    where TKey : Foo
    where TValue : Bar
{}

Then the non-generic would be:

public static Bar GetValueOrDefault(this HashTable dictionary, Foo key) {}

So type constraints should count (except class, new(), etc).

In general, I'd say that for generics we can look at how the non-generic version would look like to decide what should really count as a dependency.

So, for:

public class Class1
{
   public void Foo<T>() { }
   public void Bar<T>() { }
}

I count 2: Foo<> and Bar<>

For:

public class Class1
{
   public void Foo<T>() where T: class { }
   public void Bar<T>() where T: System.Exception { }
}

I count 3: Foo<>, Bar<> and System.Exception.