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

Rule S3909: Collections should implement the generic interface #389

Merged
merged 4 commits into from
Jun 6, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions sonaranalyzer-dotnet/rspec/cs/S3909_c#.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
<p>The NET Framework 2.0 introduced the generic interface <code>System.Collections.Generic.IEnumerable&lt;T&gt;</code> and it should be preferred over
the older, non generic, interfaces.</p>
<p>This rule raises an issue when a public type implements <code>System.Collections.IEnumerable</code>.</p>
<h2>Noncompliant Code Example</h2>
<pre>
using System;
using System.Collections;

public class MyData
{
public MyData()
{
}
}

public class MyList : CollectionBase // Noncompliant
{
public void Add(MyData data)
{
InnerList.Add(data);
}

// ...
}
</pre>
<h2>Compliant Solution</h2>
<pre>
using System;
using System.Collections.ObjectModel;

public class MyData
{
public MyData()
{
}
}

public class MyList : Collection&lt;MyData&gt;
{
// Implementation...
}
</pre>

13 changes: 13 additions & 0 deletions sonaranalyzer-dotnet/rspec/cs/S3909_c#.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{
"title": "Collections should implement the generic interface",
"type": "CODE_SMELL",
"status": "ready",
"remediation": {
"func": "Constant\/Issue",
"constantCost": "15min"
},
"tags": [

],
"defaultSeverity": "Major"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/*
* SonarAnalyzer for .NET
* Copyright (C) 2015-2017 SonarSource SA
* mailto: contact AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

using System.Collections.Generic;

namespace SonarAnalyzer.Helpers
{
public static class CollectionUtils
{
public static HashSet<T> ToHashSet<T>(this IEnumerable<T> enumerable)
{
if (enumerable == null)
{
return new HashSet<T>();
}

return new HashSet<T>(enumerable);
}

public static TValue GetValueOrDefault<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key)
{
return dictionary.GetValueOrDefault(key, default(TValue));
}

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

return defaultValue;
}
}
}
27 changes: 27 additions & 0 deletions sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/RspecStrings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -5937,6 +5937,33 @@
<data name="S3904_Type" xml:space="preserve">
<value>BUG</value>
</data>
<data name="S3909_Category" xml:space="preserve">
<value>Sonar Code Smell</value>
</data>
<data name="S3909_Description" xml:space="preserve">
<value>The NET Framework 2.0 introduced the generic interface System.Collections.Generic.IEnumerable&lt;T&gt; and it should be preferred over the older, non generic, interfaces.</value>
</data>
<data name="S3909_IsActivatedByDefault" xml:space="preserve">
<value>False</value>
</data>
<data name="S3909_Remediation" xml:space="preserve">
<value>Constant/Issue</value>
</data>
<data name="S3909_RemediationCost" xml:space="preserve">
<value>15min</value>
</data>
<data name="S3909_Severity" xml:space="preserve">
<value>Major</value>
</data>
<data name="S3909_Tags" xml:space="preserve">
<value />
</data>
<data name="S3909_Title" xml:space="preserve">
<value>Collections should implement the generic interface</value>
</data>
<data name="S3909_Type" xml:space="preserve">
<value>CODE_SMELL</value>
</data>
<data name="S3925_Category" xml:space="preserve">
<value>Sonar Bug</value>
</data>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
/*
* SonarAnalyzer for .NET
* Copyright (C) 2015-2017 SonarSource SA
* mailto: contact AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

using System.Collections.Generic;
using System.Collections.Immutable;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Diagnostics;
using SonarAnalyzer.Common;
using SonarAnalyzer.Helpers;

namespace SonarAnalyzer.Rules.CSharp
{
[DiagnosticAnalyzer(LanguageNames.CSharp)]
[Rule(DiagnosticId)]
public sealed class CollectionsShouldImplementGenericInterface : SonarDiagnosticAnalyzer
{
internal const string DiagnosticId = "S3909";
private const string MessageFormat = "Refactor this collection to implement '{0}'.";
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking about making this rule to report only once per class and put the additional base classes and interfaces as secondary locations. But then I realized that you should probably report only once anyway, because if you implement at least one generic interface/class the rule will not report anymore. What will happen with FxCop if you have a class like this? How many reports are we going to get? What interface do they suggest in this case?

public class Foo : IList, IEnumerable {}


private static readonly DiagnosticDescriptor rule =
DiagnosticDescriptorBuilder.GetDescriptor(DiagnosticId, MessageFormat, RspecStrings.ResourceManager);
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(rule);

private static readonly Dictionary<string, KnownType> nongenericToGenericMapping =
new Dictionary<string, KnownType>
{
[KnownType.System_Collections_ICollection.TypeName] = KnownType.System_Collections_Generic_ICollection_T,
[KnownType.System_Collections_IList.TypeName] = KnownType.System_Collections_Generic_IList_T,
[KnownType.System_Collections_IEnumerable.TypeName] = KnownType.System_Collections_Generic_IEnumerable_T,
[KnownType.System_Collections_CollectionBase.TypeName] = KnownType.System_Collections_ObjectModel_Collection_T,
};

private static readonly ISet<KnownType> genericTypes = nongenericToGenericMapping.Values.ToHashSet();

protected override void Initialize(SonarAnalysisContext context)
{
context.RegisterSyntaxNodeActionInNonGenerated(c =>
{
var classDeclaration = c.Node as ClassDeclarationSyntax;

var implementedTypes = classDeclaration?.BaseList?.Types;
if (implementedTypes == null)
{
return;
}

var issues = new List<Diagnostic>();
foreach (var typeSyntax in implementedTypes)
{
var typeSymbol = c.SemanticModel.GetSymbolInfo(typeSyntax.Type).Symbol?.GetSymbolType();
if (typeSymbol == null)
{
continue;
}

if (typeSymbol.OriginalDefinition.IsAny(genericTypes))
{
return;
}

var suggestedGenericType = SuggestGenericCollectionType(typeSymbol);
if (suggestedGenericType != null)
{
issues.Add(Diagnostic.Create(rule,
classDeclaration.Identifier.GetLocation(),
suggestedGenericType));
}
}

issues.ForEach(c.ReportDiagnostic);
},
SyntaxKind.ClassDeclaration);
}

private static string SuggestGenericCollectionType(ITypeSymbol typeSymbol)
{
return nongenericToGenericMapping.GetValueOrDefault(typeSymbol.ToDisplayString())?.TypeName;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@
<Link>Properties\AssemblyInfo.Shared.cs</Link>
</Compile>
<Compile Include="Helpers\CognitiveComplexityWalker.cs" />
<Compile Include="Helpers\CollectionUtils.cs" />
<Compile Include="Helpers\ControlFlowGraph\Blocks\ForeachCollectionProducerBlock.cs" />
<Compile Include="Helpers\ControlFlowGraph\Blocks\ForInitializerBlock.cs" />
<Compile Include="Helpers\ControlFlowGraph\Blocks\LockBlock.cs" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ internal sealed class KnownType
public static readonly KnownType System_Collections_Generic_IList_T = new KnownType(SpecialType.System_Collections_Generic_IList_T, "System.Collections.Generic.IList<T>");
public static readonly KnownType System_Collections_Generic_ICollection_T = new KnownType(SpecialType.System_Collections_Generic_ICollection_T, "System.Collections.Generic.ICollection<T>");
public static readonly KnownType System_Collections_ICollection = new KnownType("System.Collections.ICollection");
public static readonly KnownType System_Collections_CollectionBase = new KnownType("System.Collections.CollectionBase");

public static readonly KnownType System_Object = new KnownType(SpecialType.System_Object, "object");
public static readonly KnownType System_String = new KnownType(SpecialType.System_String, "string");
Expand Down Expand Up @@ -181,6 +182,7 @@ internal sealed class KnownType
public static readonly KnownType System_Collections_Immutable_ImmutableStack_T = new KnownType("System.Collections.Immutable.ImmutableStack<T>");
public static readonly KnownType System_Collections_Immutable_IImmutableStack_T = new KnownType("System.Collections.Immutable.IImmutableStack<T>");

public static readonly KnownType System_Collections_ObjectModel_Collection_T = new KnownType("System.Collections.ObjectModel.Collection<T>");
public static readonly KnownType System_Collections_ObjectModel_ReadOnlyCollection_T = new KnownType("System.Collections.ObjectModel.ReadOnlyCollection<T>");
public static readonly KnownType System_Collections_ObjectModel_ReadOnlyDictionary_TKey_TValue = new KnownType("System.Collections.ObjectModel.ReadOnlyDictionary<TKey, TValue>");

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
<p>The NET Framework 2.0 introduced the generic interface <code>System.Collections.Generic.IEnumerable&lt;T&gt;</code> and it should be preferred over
the older, non generic, interfaces.</p>
<p>This rule raises an issue when a public type implements <code>System.Collections.IEnumerable</code>.</p>
<h2>Noncompliant Code Example</h2>
<pre>
using System;
using System.Collections;

public class MyData
{
public MyData()
{
}
}

public class MyList : CollectionBase // Noncompliant
{
public void Add(MyData data)
{
InnerList.Add(data);
}

// ...
}
</pre>
<h2>Compliant Solution</h2>
<pre>
using System;
using System.Collections.ObjectModel;

public class MyData
{
public MyData()
{
}
}

public class MyList : Collection&lt;MyData&gt;
{
// Implementation...
}
</pre>

Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,7 @@ public class RuleTypeTests
["3902"] = "CODE_SMELL",
["3903"] = "BUG",
["3904"] = "BUG",
["3909"] = "CODE_SMELL",
["3925"] = "BUG",
["3926"] = "BUG",
["3928"] = "CODE_SMELL",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*
* SonarAnalyzer for .NET
* Copyright (C) 2015-2017 SonarSource SA
* mailto: contact AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

using Microsoft.VisualStudio.TestTools.UnitTesting;
using SonarAnalyzer.Rules.CSharp;

namespace SonarAnalyzer.UnitTest.Rules
{
[TestClass]
public class CollectionsShouldImplementGenericInterfaceTest
{
[TestMethod]
[TestCategory("Rule")]
public void CollectionsShouldImplementGenericInterface()
{
Verifier.VerifyAnalyzer(@"TestCases\CollectionsShouldImplementGenericInterface.cs",
new CollectionsShouldImplementGenericInterface());
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
using System;
using System.Collections;
using System.Collections.Generic;
using System.Collections.ObjectModel;

namespace Tests.Diagnostics
{
class TestClass_01 : CollectionBase { } // Noncompliant {{Refactor this collection to implement 'System.Collections.ObjectModel.Collection<T>'.}}
// ^^^^^^^^^^^^
class TestClass_02 : IList { } // Noncompliant {{Refactor this collection to implement 'System.Collections.Generic.IList<T>'.}}

class TestClass_03 : IEnumerable { } // Noncompliant {{Refactor this collection to implement 'System.Collections.Generic.IEnumerable<T>'.}}

class TestClass_04 : ICollection { } // Noncompliant {{Refactor this collection to implement 'System.Collections.Generic.ICollection<T>'.}}

class TestClass_05 : IEnumerable, ICollection { } // Noncompliant {{Refactor this collection to implement 'System.Collections.Generic.IEnumerable<T>'.}}
// Noncompliant@-1 {{Refactor this collection to implement 'System.Collections.Generic.ICollection<T>'.}}

class TestClass_06 : IEnumerable, ICollection<string> { }
class TestClass_07 : IEnumerable, IList<string> { }
class TestClass_08 : IEnumerable, IEnumerable<string> { }
class TestClass_09 : IEnumerable, Collection<string> { }
class TestClass_10<T> : IEnumerable, IList<T> { }

class TestClass_11<T> : IEnumerable, ICollection, ICollection<T> { }

class TestClass_12 : IEnumerable, IList, IEnumerable<string> { }

class TestClass_13 { }

class TestClass_14 : Exception { }

class TestClass_15 : IEqualityComparer { }

class TestClass_16 : IList, InvalidType { } // Noncompliant
}