CodeFixProvider that instantiates collections

Question

Another piece of Roslyn-based code, this time a CodeFixProvider.

I'm looking for feedback on Roslyn-specific code:

• Am I using the SemanticModel correctly?
• Are there any codeflows I have overlooked that might cause issues?
• Am I violating any best practices I'm unaware of?
• etc

Analyzer

namespace DiagnosticTools.Collections.NotInitializedException
{
    [DiagnosticAnalyzer]
    [ExportDiagnosticAnalyzer(DiagnosticId, LanguageNames.CSharp)]
    internal class NotInstantiatedCollectionAnalyzer : ISyntaxNodeAnalyzer<SyntaxKind>
    {
        internal const string DiagnosticId = "CollectionNotInstantiated";
        private static string Description = Resources.NotInstantiatedCollection_Description;
        private static string MessageFormat = Resources.NotInstantiatedCollection_MessageFormat;
        private static string Category = Resources.Category_Collections;
        private static readonly DiagnosticDescriptor Rule = new DiagnosticDescriptor(DiagnosticId, Description, MessageFormat, Category, DiagnosticSeverity.Warning);

        public ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics
        {
            get
            {
                return ImmutableArray.Create(Rule);
            }
        }

        public ImmutableArray<SyntaxKind> SyntaxKindsOfInterest
        {
            get
            {
                return ImmutableArray.Create(SyntaxKind.VariableDeclaration);
            }
        }

        public void AnalyzeNode(SyntaxNode node, SemanticModel semanticModel, Action<Diagnostic> addDiagnostic, CancellationToken cancellationToken)
        {
            var declaration = node as VariableDeclarationSyntax;
            var typeOfVariable = semanticModel.GetTypeInfo(declaration.Type).Type;
            var isCollection = typeOfVariable.IsCollection();
            var isInitialized = declaration.Variables[0].Initializer != null;

            if (isCollection && !isInitialized)
            {
                addDiagnostic(Diagnostic.Create(Rule, declaration.GetLocation(), declaration.Variables[0].Identifier.Value));
            }
        }
    }
}

CodeFixProvider

namespace DiagnosticTools.Collections.NotInitializedException
{
    [ExportCodeFixProvider(NotInstantiatedCollectionAnalyzer.DiagnosticId, LanguageNames.CSharp)]
    class NotInstantiatedCollectionFixProvider : ICodeFixProvider
    {
        public IEnumerable<string> GetFixableDiagnosticIds()
        {
            return new[] { NotInstantiatedCollectionAnalyzer.DiagnosticId };
        }

        public async Task<IEnumerable<CodeAction>> GetFixesAsync(Document document, TextSpan span, IEnumerable<Diagnostic> diagnostics, CancellationToken cancellationToken)
        {
            var root = await document.GetSyntaxRootAsync(cancellationToken);
            var token = root.FindToken(span.Start);
            document.TryGetSemanticModel(out SemanticModel semanticModel);
            var statement = token.Parent.Parent as VariableDeclarationSyntax;

            // Construct variable declaration
            var declarator = new SeparatedSyntaxList<VariableDeclaratorSyntax>();
            var identifier = statement.Variables[0].Identifier;
            var newType = semanticModel.GetTypeInfo(statement.Type);

            // Leave the method when we encounter an interface like IEnumerable<T>.
            // The warning remains, but there will be no solution proposal.
            if (newType.Type.IsAbstract)
            {
                return new CodeAction[] { };
            }

            var genericArguments = statement.Type
                                            .DescendantNodes()
                                            .OfType<TypeArgumentListSyntax>()
                                            .FirstOrDefault();

            var newObject = RoslynUtils.GetInstantiationExpressionFromType(newType.Type.Name, genericArguments);
            var equalsClause = SyntaxFactory.EqualsValueClause(
                                    SyntaxFactory.Token(SyntaxKind.EqualsToken)
                                        .WithLeadingTrivia(
                                            new SyntaxTriviaList() {
                                                SyntaxFactory.Space
                                            }), 
                                    newObject);

            declarator = declarator.Add(SyntaxFactory.VariableDeclarator(identifier, null, equalsClause));

            var newStatement = SyntaxFactory.VariableDeclaration(statement.Type, declarator);
            var newRoot = root.ReplaceNode(statement, newStatement);

            return new[]
            {
                CodeAction.Create(Resources.NotInstantiatedCollection_ActionMessage, document.WithSyntaxRoot(newRoot))
            };
        }
    }
}

Extensions

namespace DiagnosticTools.Utilities
{
    public static class Extensions
    {
        public static bool IsSubclassOf(this ITypeSymbol symbol, Type type)
        {
            var @base = symbol.BaseType;

            while(@base != null)
            {
                if (@base.Name == type.Name) { return true; }
                @base = @base.BaseType;
            }

            return false;  
        }

        public static bool ImplementsInterface(this ITypeSymbol symbol, Type type)
        {
            return symbol.Interfaces.Any(x => x.Name == type.Name);
        }

        public static bool IsCollection(this ITypeSymbol symbol)
        {
            return symbol.Interfaces.Any(x => x.Name == typeof(IEnumerable).Name
                                        ||    x.Name == typeof(IEnumerable<>).Name);
        }
    }
}

RoslynUtils

namespace DiagnosticTools.Utilities
{
    public static class RoslynUtils
    {
        public static ExpressionSyntax GetInstantiationExpressionFromType(string typeName, TypeArgumentListSyntax typeArgumentsSyntax)
        {
            // 0: Name of type
            // 1: (Optional) generic arguments
            // 2: (Optional) arguments
            var result = " new {0}{1}({2})";
            var typeArguments = GetParsedTypeArguments(typeArgumentsSyntax);
            var arguments = ""; // Not implemented until I find out a place where to use them

            return SyntaxFactory.ParseExpression(string.Format(result, typeName, typeArguments, arguments));
        }

        private static string GetParsedTypeArguments(TypeArgumentListSyntax arguments)
        {
            if (arguments.Arguments.Count == 0) { return ""; }

            var sb = new StringBuilder("<");
            for (var i = 0; i < arguments.Arguments.Count; i++)
            {
                sb.Append(arguments.Arguments[i].ToString());
                if (i != arguments.Arguments.Count - 1)
                {
                    sb.Append(", ");
                }
            }

            sb.Append(">");
            return sb.ToString();
        }
    }
}

Answer 1

I know nothing about rosyln, so there would be someone better to check that, however...

Analyzer

• You create Description, MessageFormat & Category as static strings but only use them in the constructor of Rule. I don't think that the extra fields provide anything other than a minor increase in readability.
• Inconsistent use of readonly. If you keep the fields I suggested removing above, make them all readonly. This will give clear intention that they should not be used (I guess that you want them as const's ideally)
• The result of SyntaxKindsOfInterest is deterministic, therefore, it can be made a readonly static (A Micro-Optimisation I know). I can't remember the language semantics off hand, but the same could probably happen for SupportedDiagnostics
• There is no parameter checking inside of AnalyzeNode
• Inside AnalyzeNode, you don't check for a null reference after the type cast of declaration before attempting to use it
• You assume that the Variables collection is going to always have 1 or more elements in the collection

CodeFixProvider

• The result of GetFixableDiagnosticIds is deterministic and can be kept as a static readonly field
• No parameter checking inside of GetFixesAsync
• No checking for a null reference after the type case of statement
• You assume that the Variables collection is going to always have 1 or more elements in the collection
• You could have something similar to EventArgs.Empty for the result of if (newType.Type.IsAbstract)

Misc

• RosylnUtils performs no parameter checking
• GetTypedArguments should use return String.Empty not ""

I relise that this is probably just test code, hope this helps in either case. These are just the things that I would do.