7
\$\begingroup\$

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();
        }
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ Removed a line of code that was a leftover from experimenting. \$\endgroup\$ Apr 25, 2014 at 18:09

1 Answer 1

3
+50
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.