Update S1200: Fixed generic type counting, excluded more types from counting #1472

valhristov · 2018-06-13T11:36:34Z

valhristov · 2018-06-13T11:37:18Z

sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Rules/AvoidExcessiveClassCoupling.cs

                    }
                },
-                SyntaxKind.ClassDeclaration);
+                SyntaxKind.ClassDeclaration,
+                SyntaxKind.StructDeclaration);


We are also analyzing structs and interfaces now.

valhristov · 2018-06-13T12:52:05Z

sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Rules/AvoidExcessiveClassCoupling.cs

-                SyntaxKind.ClassDeclaration);
+                SyntaxKind.ClassDeclaration,
+                SyntaxKind.StructDeclaration,
+                SyntaxKind.InterfaceDeclaration);


Now handling struct and interface too.

valhristov · 2018-06-13T12:52:34Z

sonaranalyzer-dotnet/src/SonarAnalyzer.Common/Helpers/KnownType.cs

@@ -170,6 +179,7 @@ internal sealed class KnownType
        internal static readonly KnownType System_IO_StreamReader = new KnownType("System.IO.StreamReader");
        internal static readonly KnownType System_IO_StreamWriter = new KnownType("System.IO.StreamWriter");
        internal static readonly KnownType System_IO_TextWriter = new KnownType("System.IO.TextWriter");
+        internal static readonly KnownType System_Lazy = new KnownType("System.Lazy<T>");


Decided that Lazy is also worth excluding

duncanp-sonar · 2018-06-14T12:05:53Z

...-dotnet/its/expected/akka.net/Akka.Cluster-{6AB00F61-269A-4501-B06A-026707F000A7}-S1200.json

-{
-"id":  "S1200",
-"message":  "Split this class into smaller and more specialized ones to reduce its dependencies on other classes from 30 to the maximum authorized 20 or less.",
+"message":  "Split this class into smaller and more specialized ones to reduce its dependencies on other classes from 31 to the maximum authorized 30 or less.",


The number of classes where the detected dependencies count went up. Is this expected?

sources\akka.net\src\core\Akka.Remote\Endpoint.cs 33->35
sources\akka.net\src\core\Akka.Remote\Transport\ThrottleTransportAdapter.cs 33->35

sources\akka.net\src\core\Akka.Cluster\ClusterMetricsCollector.cs 30->31
sources\akka.net\src\contrib\cluster\Akka.Cluster.Tools\Singleton\ClusterSingletonManager.cs 40->41
sources\akka.net\src\core\Akka.Remote\Endpoint.cs 43->44
sources\akka.net\src\core\Akka.Remote\RemoteWatcher.cs 38->39
sources\akka.net\src\core\Akka.Remote\Transport\AkkaPduCodec.cs 33->34
sources\akka.net\src\core\Akka.Remote\Transport\Helios\HeliosTransport.cs 34->35

It is normal, we started to count some things (method return types for example) and stopped counting other - bounded generic types (e.g. IDictionary<int> and IDictionary<string> are counted as +1, not +2 as before).

duncanp-sonar · 2018-06-14T12:18:06Z

sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Rules/AvoidExcessiveClassCoupling.cs


-            var indexerProperty = property as IndexerDeclarationSyntax;
-            if (indexerProperty?.ExpressionBody != null)
+            public IList<ITypeSymbol> DependentTypes { get; } = new List<ITypeSymbol>();


Could this be a set rather than a list?

it could probably save some iterations, I guess

duncanp-sonar · 2018-06-14T12:26:03Z

sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Rules/AvoidExcessiveClassCoupling.cs

-        }
+            public override void VisitClassDeclaration(ClassDeclarationSyntax node)
+            {
+                // don't drill down in child classes, but walk the original


What's the reason for this - because child classes will be analysed separately?
If so, it would be worth adding as a comment.

if you call the base method the walker will continue through all descendant nodes, including inner classes, thus collecting dependent types from inner classes, which I think is not expected.

As discussed this morning, this might be worth adding as a comment in the C# RSPEC sub task

duncanp-sonar · 2018-06-14T12:29:47Z

sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Rules/AvoidExcessiveClassCoupling.cs

+            namedType.TypeKind != TypeKind.Enum &&
+            !namedType.IsAny(ignoredTypes);
+
+        private static IEnumerable<INamedTypeSymbol> ExpandGenericTypes(INamedTypeSymbol namedType)


Some documentation of the intention would be useful e.g. what is expected for Dictionary<string, MyClass>?

duncanp-sonar · 2018-06-14T12:31:12Z

sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Rules/AvoidExcessiveClassCoupling.cs

-                type.TypeKind != TypeKind.Enum &&
-                type.TypeKind != TypeKind.Pointer &&
-                !type.IsAny(typesExcludedFromCoupling);
+            public override void VisitObjectCreationExpression(ObjectCreationExpressionSyntax node)


Are calls to static classes captured?

duncanp-sonar

How is inheritance handled?

tests for calls to methods/properties/constants on static classes
tests for catch statements e.g. catch (MyException e)
attributes?
if a class references itself, will that be counted as a reference?

valhristov · 2018-06-15T13:14:58Z

Added the requested tests. The current class is not counted as dependency.

Attributes are metadata, imo they should not be counted (and are not)

…ounting

antoinebj · 2018-06-15T14:07:40Z

Hi,

I was the author of #1180 for which this PR represents the fix. I must have missed some updates.

Anyway, if it's not too late, I noticed the following comment in the code:
void Foo<T>(Dictionary<string,List<T>>) where T : IEnumerable<int> should return Dictionary, string, List, IEnumerable and int

Have you considered not counting IEnumerable in such cases?
That interface is implemented by Dictionary (and also List), meaning that if the dictionary instance is put in a variable of type IEnumerable, or passed to a private method that only needs to process it as an enumerable, it will count as an additional dependency, which hardly seems fair.
More generically, if a coupling to type A is identified, and A implements or derives from B, should B really also be counted when it is used elsewhere in the class?

valhristov · 2018-06-15T14:29:30Z

Hi @antoinebj it is never late to create a new ticket :)

I played a bit with Visual Studio's Code Metrics (not sure if it is available in all editions, though) and for the following code it seems to count all classes and interfaces:

public class Class1 // Class coupling is 3
{
    public void Foo(Dictionary<string, int> dd, IDictionary<string, int> d, IEnumerable<int> e) { }
}

I decided to try to be relatively close to it's way of counting. VS however, counts Task, Action, etc. types, but we are a bit more permissive and do not count some "core" classes.

antoinebj · 2018-06-15T14:44:58Z

@valhristov
Whether Microsoft's method is the most correct or not, I understand the need for some consistency at the expense of whatever might be perceived as more accurate.
On the other hand, it could be argued that the counting "fairness" matters more in Sonar because it's used for quality gates (in a broad sense) that may frustrate developers, whereas VS Code Metrics seems to serve more as a reporting tool that does not have much impact.
Anyway, I'll try the updated rule when it is made available and open a new issue if I have any further questions or suggestions.
Thanks 👍

ghost assigned valhristov Jun 13, 2018

ghost added the Status: Needs Review label Jun 13, 2018

valhristov requested a review from duncanp-sonar June 13, 2018 11:36

valhristov commented Jun 13, 2018

View reviewed changes

valhristov force-pushed the val/S1200 branch 2 times, most recently from a873c33 to 4396023 Compare June 13, 2018 12:50

valhristov commented Jun 13, 2018

View reviewed changes

valhristov force-pushed the val/S1200 branch 2 times, most recently from 35f861e to a2c9f07 Compare June 13, 2018 14:27

duncanp-sonar reviewed Jun 14, 2018

View reviewed changes

duncanp-sonar suggested changes Jun 14, 2018

View reviewed changes

valhristov force-pushed the val/S1200 branch 2 times, most recently from 076d756 to 5a12bc0 Compare June 15, 2018 11:52

Update S1200: Fixed generic type counting, excluded more types from c…

39d4677

…ounting

valhristov force-pushed the val/S1200 branch from 5d486b5 to 39d4677 Compare June 15, 2018 13:22

valhristov changed the title ~~Exclude Action, Func and Task from counting; included method return t…~~ Update S1200: Fixed generic type counting, excluded more types from counting Jun 15, 2018

duncanp-sonar approved these changes Jun 15, 2018

View reviewed changes

duncanp-sonar merged commit e3303ff into master Jun 15, 2018

ghost removed the Status: Needs Review label Jun 15, 2018

valhristov deleted the val/S1200 branch June 15, 2018 14:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update S1200: Fixed generic type counting, excluded more types from counting #1472

Update S1200: Fixed generic type counting, excluded more types from counting #1472

valhristov commented Jun 13, 2018 •

edited

Loading

valhristov Jun 13, 2018 •

edited

Loading

valhristov Jun 13, 2018

valhristov Jun 13, 2018

duncanp-sonar Jun 14, 2018 •

edited

Loading

valhristov Jun 15, 2018 •

edited

Loading

duncanp-sonar Jun 14, 2018

valhristov Jun 14, 2018

duncanp-sonar Jun 14, 2018

valhristov Jun 14, 2018

duncanp-sonar Jun 15, 2018

duncanp-sonar Jun 14, 2018

duncanp-sonar Jun 14, 2018

duncanp-sonar left a comment •

edited

Loading

valhristov commented Jun 15, 2018

antoinebj commented Jun 15, 2018

valhristov commented Jun 15, 2018

antoinebj commented Jun 15, 2018

Update S1200: Fixed generic type counting, excluded more types from counting #1472

Update S1200: Fixed generic type counting, excluded more types from counting #1472

Conversation

valhristov commented Jun 13, 2018 • edited Loading

valhristov Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duncanp-sonar Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

valhristov Jun 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duncanp-sonar left a comment • edited Loading

Choose a reason for hiding this comment

valhristov commented Jun 15, 2018

antoinebj commented Jun 15, 2018

valhristov commented Jun 15, 2018

antoinebj commented Jun 15, 2018

valhristov commented Jun 13, 2018 •

edited

Loading

valhristov Jun 13, 2018 •

edited

Loading

duncanp-sonar Jun 14, 2018 •

edited

Loading

valhristov Jun 15, 2018 •

edited

Loading

duncanp-sonar left a comment •

edited

Loading