Rule S3909: Collections should implement the generic interface #389
Conversation
| } | ||
| } | ||
|
|
||
| issues.ForEach(i => c.ReportDiagnostic(i)); |
There was a problem hiding this comment.
You could use issues.ForEach(c.ReportDiagnostic); instead :)
| var typeSymbol = c.SemanticModel.GetSymbolInfo(typeSyntax.Type).Symbol?.GetSymbolType(); | ||
|
|
||
| var originalDefinition = typeSymbol?.OriginalDefinition; | ||
| if (originalDefinition.IsAny(genericTypes)) |
There was a problem hiding this comment.
It took me some time to understand that if we have at least one generic interface the rule does not raise issues (I thought there is a bug and I had to check the tests). Perhaps it will be more obvious if you extract the two lines above into a method IsGenericCollection(typeSymbol), WDYT?
| SyntaxKind.ClassDeclaration); | ||
| } | ||
|
|
||
| private static KnownType ToKnownType(ITypeSymbol typeSymbol, IEnumerable<KnownType> knownTypes) |
There was a problem hiding this comment.
You could remove the knownTypes argument and perhaps rename the method to something like SuggestGenericCollectionType or better and refactor it to directly return a generic type counterpart or null... WDYT?
| public sealed class CollectionsShouldImplementGenericInterface : SonarDiagnosticAnalyzer | ||
| { | ||
| internal const string DiagnosticId = "S3909"; | ||
| private const string MessageFormat = "Refactor this collection to implement '{0}'."; |
There was a problem hiding this comment.
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 string SuggestGenericCollectionType(ITypeSymbol typeSymbol) | ||
| { | ||
| KnownType suggestedGenericType; | ||
| if (nongenericToGenericMapping.TryGetValue(typeSymbol.ToDisplayString(), out suggestedGenericType)) |
There was a problem hiding this comment.
If you use ImmutableDictionary you will have the GetValueOrDefault extension method and this could be written as:
return nongenericToGenericMapping.GetValueOrDefault(typeSymbol.ToDisplayString())?.TypeName;
perhaps we should some day add such method for normal IDictionary<K,V>
Fix #361