8
\$\begingroup\$

A new diagnostic has seen the light of day! This one will alert you when you have a string.Format() call where the formatting placeholders are in the wrong order. The right order is defined as:

For each placeholder with index N, its value should be higher or equal to the placeholder at index N-1

If you want a more elaborate overview of what scenarios should be handled how you can take a look at the accompanying unit tests. I think I have covered all special scenarios in which placeholders can be used but since I get surprised by exotic scenarios all the time, please let me know if I forgot anything.

Unit tests Github

I omitted the tests because it would put my post way over the limited amount of characters.

Analyzer Github

using System.Collections.Immutable;
using System.Globalization;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Diagnostics;

namespace VSDiagnostics.Diagnostics.Strings.StringPlaceholdersInWrongOrder
{
    [DiagnosticAnalyzer(LanguageNames.CSharp)]
    public class StringPlaceholdersInWrongOrderAnalyzer : DiagnosticAnalyzer
    {
        private static readonly string Category = VSDiagnosticsResources.StringsCategory;
        private const string DiagnosticId = nameof(StringPlaceholdersInWrongOrderAnalyzer);
        private static readonly string Message = VSDiagnosticsResources.StringPlaceholdersInWrongOrderMessage;
        private const DiagnosticSeverity Severity = DiagnosticSeverity.Warning;
        private static readonly string Title = VSDiagnosticsResources.StringPlaceholdersInWrongOrderTitle;

        internal static DiagnosticDescriptor Rule => new DiagnosticDescriptor(DiagnosticId, Title, Message, Category, Severity, true);

        public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Rule);

        public override void Initialize(AnalysisContext context)
        {
            context.RegisterSyntaxNodeAction(AnalyzeNode, SyntaxKind.InvocationExpression);
        }

        private void AnalyzeNode(SyntaxNodeAnalysisContext context)
        {
            var invocation = context.Node as InvocationExpressionSyntax;
            if (invocation == null)
            {
                return;
            }

            // Verify we're dealing with a string.Format() call
            var memberAccessExpression = invocation.Expression as MemberAccessExpressionSyntax;
            if (memberAccessExpression != null)
            {
                var invokedType = context.SemanticModel.GetSymbolInfo(memberAccessExpression.Expression);
                var invokedMethod = context.SemanticModel.GetSymbolInfo(memberAccessExpression.Name);
                if (invokedType.Symbol == null || invokedMethod.Symbol == null)
                {
                    return;
                }

                if (invokedType.Symbol.MetadataName != typeof(string).Name || invokedMethod.Symbol.MetadataName != nameof(string.Format))
                {
                    return;
                }
            }

            // Verify the format is a literal expression and not a method invocation or an identifier
            // The overloads are in the form string.Format(string, object[]) or string.Format(CultureInfo, string, object[])
            if (invocation.ArgumentList == null || invocation.ArgumentList.Arguments.Count < 2)
            {
                return;
            }

            var firstArgument = invocation.ArgumentList.Arguments[0];
            var secondArgument = invocation.ArgumentList.Arguments[1];

            var firstArgumentSymbol = context.SemanticModel.GetSymbolInfo(firstArgument.Expression);
            if (!(firstArgument.Expression is LiteralExpressionSyntax) &&
                (firstArgumentSymbol.Symbol?.MetadataName == typeof(CultureInfo).Name && !(secondArgument?.Expression is LiteralExpressionSyntax)))
            {
                return;
            }

            // Get the formatted string from the correct position
            var firstArgumentIsLiteral = firstArgument.Expression is LiteralExpressionSyntax;
            var formatString = firstArgumentIsLiteral
                ? ((LiteralExpressionSyntax) firstArgument.Expression).GetText().ToString()
                : ((LiteralExpressionSyntax) secondArgument.Expression).GetText().ToString();

            // Verify that all placeholders are counting from low to high.
            // Not all placeholders have to be used necessarily, we only re-order the ones that are actually used in the format string.
            //
            // Display a warning when the integers in question are not in ascending or equal order. 
            var placeholders = StringPlaceholdersInWrongOrderHelper.GetPlaceholders(formatString);

            // If there's no placeholder used or there's only one, there's nothing to re-order
            if (placeholders.Count <= 1)
            {
                return;
            }

            for (var index = 1; index < placeholders.Count; index++)
            {
                int firstValue, secondValue;
                if (!int.TryParse(StringPlaceholdersInWrongOrderHelper.Normalize(placeholders[index - 1].Value), out firstValue) ||
                    !int.TryParse(StringPlaceholdersInWrongOrderHelper.Normalize(placeholders[index].Value), out secondValue))
                {
                    // Parsing failed
                    return;
                }

                // Given a scenario like this:
                //     string.Format("{0} {1} {4} {3}", a, b, c, d)
                // it would otherwise crash because it's trying to access index 4, which we obviously don't have.
                var argumentsToSkip = firstArgumentIsLiteral ? 1 : 2;
                if (firstValue >= invocation.ArgumentList.Arguments.Count - argumentsToSkip || secondValue >= invocation.ArgumentList.Arguments.Count - argumentsToSkip)
                {
                    return;
                }

                // They should be in ascending or equal order
                if (firstValue > secondValue)
                {
                    context.ReportDiagnostic(Diagnostic.Create(Rule, invocation.GetLocation()));
                    return;
                }
            }
        }
    }
}

Code Fix Github

using System.Collections.Generic;
using System.Collections.Immutable;
using System.Composition;
using System.Linq;
using System.Text;
using System.Text.RegularExpressions;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CodeActions;
using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;

namespace VSDiagnostics.Diagnostics.Strings.StringPlaceholdersInWrongOrder
{
    [ExportCodeFixProvider("StringPlaceHoldersInWrongOrder", LanguageNames.CSharp), Shared]
    public class StringPlaceHoldersInWrongOrderCodeFix : CodeFixProvider
    {
        public override ImmutableArray<string> FixableDiagnosticIds => ImmutableArray.Create(StringPlaceholdersInWrongOrderAnalyzer.Rule.Id);

        public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer;

        public override async Task RegisterCodeFixesAsync(CodeFixContext context)
        {
            var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false);
            var diagnostic = context.Diagnostics.First();
            var diagnosticSpan = diagnostic.Location.SourceSpan;

            var stringFormatInvocation = root.FindToken(diagnosticSpan.Start).Parent.AncestorsAndSelf().OfType<InvocationExpressionSyntax>().First();
            context.RegisterCodeFix(
                CodeAction.Create(VSDiagnosticsResources.StringPlaceholdersInWrongOrderCodeFixTitle, x => ReOrderPlaceholdersAsync(context.Document, root, stringFormatInvocation),
                    nameof(StringPlaceholdersInWrongOrderAnalyzer)),
                diagnostic);
        }

        private static Task<Solution> ReOrderPlaceholdersAsync(Document document, SyntaxNode root, InvocationExpressionSyntax stringFormatInvocation)
        {
            var firstArgumentIsLiteral = stringFormatInvocation.ArgumentList.Arguments[0].Expression is LiteralExpressionSyntax;
            var formatString = ((LiteralExpressionSyntax) stringFormatInvocation.ArgumentList.Arguments[firstArgumentIsLiteral ? 0 : 1].Expression).GetText().ToString();
            var elements = StringPlaceholdersInWrongOrderHelper.GetPlaceholdersSplit(formatString);
            var matches = StringPlaceholdersInWrongOrderHelper.GetPlaceholders(formatString);

            // Here we will store a key-value pair of the old placeholder value and the new value that we associate with it
            var placeholderMapping = new Dictionary<int, int>();

            // This contains the order in which the placeholders appeared in the original format string.
            // For example if it had the string "{1} x {0} y {2} {2}" then this collection would contain the values 1-0-2.
            // You'll notice that we omitted the duplicate: we don't want to add an argument twice.
            // Typically we'd do this by using a HashSet<T> but since we can't easily retrieve an item from the HashSet<T>, 
            // we'll just check for duplicates upon inserting in the list.
            // Based on this we can then reconstruct the argument list by reordering the existing arguments.
            var placeholderIndexOrder = new List<int>();

            var amountOfPlaceholders = 0;
            var newPlaceholderValue = 0;

            var sb = new StringBuilder(elements.Length);
            for (var index = 0; index < elements.Length; index++)
            {
                // If it's a numerical value, it means we're dealing with a placeholder
                // Use Normalize() to account for formatted placeholders
                int placeholderValue;
                if (int.TryParse(StringPlaceholdersInWrongOrderHelper.Normalize(elements[index]), out placeholderValue))
                {
                    // If we already have a new value associated with this placeholder, retrieve it and add it to our result
                    if (placeholderMapping.ContainsKey(placeholderValue))
                    {
                        sb.Append(GetNewElement(matches, amountOfPlaceholders, placeholderMapping[placeholderValue]));
                    }
                    else // Otherwise use the new placeholder value and store the mapping
                    {
                        sb.Append(GetNewElement(matches, amountOfPlaceholders, newPlaceholderValue));
                        placeholderMapping.Add(placeholderValue, newPlaceholderValue);
                        newPlaceholderValue++;
                    }

                    if (!placeholderIndexOrder.Contains(placeholderValue))
                    {
                        placeholderIndexOrder.Add(placeholderValue);
                    }

                    amountOfPlaceholders++;
                }
                else
                {
                    sb.Append(elements[index]);
                }
            }
            var newFormat = sb.ToString();

            // Create a new argument for the formatting string
            var newArgument = stringFormatInvocation.ArgumentList.Arguments[firstArgumentIsLiteral ? 0 : 1].WithExpression(SyntaxFactory.ParseExpression(newFormat));

            // Create a new list for the arguments which are injected in the formatting string
            // In order to do this we iterate over the mapping which is in essence a guideline that tells us which index
            IEnumerable<ArgumentSyntax> args = firstArgumentIsLiteral
                ? new[] { newArgument }
                : new[] { stringFormatInvocation.ArgumentList.Arguments[0], newArgument };

            // Skip the formatting literal and, if applicable, the formatprovider
            var argumentsToSkip = firstArgumentIsLiteral ? 1 : 2;
            for (var index = 0; index < placeholderIndexOrder.Count; index++)
            {
                args = args.Concat(new[] { stringFormatInvocation.ArgumentList.Arguments[placeholderIndexOrder[index] + argumentsToSkip] });
            }

            // If there are less arguments in the new list compared to the old one, it means there was an unused argument
            // In that case we will loop over all the old arguments, see if they're contained in the new list and if not: add them
            // Since the variables weren't used in the first place, we can add them in whatever order we want
            // However because we are traversing from the front, they will be added in the same order as they were anyway
            if (stringFormatInvocation.ArgumentList.Arguments.Count != args.Count())
            {
                foreach (var arg in stringFormatInvocation.ArgumentList.Arguments.Skip(argumentsToSkip))
                {
                    if (!args.Contains(arg))
                    {
                        args = args.Concat(new[] { arg });
                    }
                }
            }

            var newArguments = stringFormatInvocation.ArgumentList.WithArguments(SyntaxFactory.SeparatedList(args));
            var newInvocation = stringFormatInvocation.WithArgumentList(newArguments);
            var newRoot = root.ReplaceNode(stringFormatInvocation, newInvocation);
            var newDocument = document.WithSyntaxRoot(newRoot);
            return Task.FromResult(newDocument.Project.Solution);
        }

        /// <summary>
        ///     Because the Regex.Split does not maintain any brackets or formatting, we have to now reconstruct the previous
        ///     placeholder but with a new index.
        /// </summary>
        private static string GetNewElement(MatchCollection matches, int oldPlaceholderIndex, int newPlaceholderValue)
        {
            var originalValue = matches[oldPlaceholderIndex].Value;
            var newValue = new StringBuilder();

            for (var index = 0; index < originalValue.Length; index++)
            {
                // Formatting detected: append everything remaining
                if (originalValue[index] == ':')
                {
                    newValue.Append(originalValue.Substring(index));
                    return newValue.ToString();
                }

                // Closing brace detected: append the remaining closing brace(s)
                if (originalValue[index] == '}')
                {
                    newValue.Append(originalValue.Substring(index));
                    return newValue.ToString();
                }

                // Opening brace detected: just add it
                if (originalValue[index] == '{')
                {
                    newValue.Append(originalValue[index]);
                }
                else
                // If it's not a formatting delimiter or open- or closing braces, it must be the actual placeholder value.
                // Replace it by appending the new value.
                {
                    newValue.Append(newPlaceholderValue);
                }
            }
            return newValue.ToString();
        }
    }
}

Helper Github

using System.Text.RegularExpressions;

namespace VSDiagnostics.Diagnostics.Strings.StringPlaceholdersInWrongOrder
{
    internal static class StringPlaceholdersInWrongOrderHelper
    {
        private const string Pattern = @"(?<!\{)\{(?:\{\{)*(\d+(?::.*?)?)\}(?:\}\})*(?!\})";

        /// <summary>
        ///     Removes all curly braces and formatting definitions from the placeholder
        /// </summary>
        /// <param name="input">The placeholder entry to parse.</param>
        /// <returns>Returns the placeholder index.</returns>
        internal static string Normalize(string input)
        {
            var temp = input.Trim('{', '}');
            var colonIndex = temp.IndexOf(':');
            if (colonIndex > 0)
            {
                return temp.Remove(colonIndex);
            }

            return temp;
        }

        /// <summary>
        ///     Get all elements in a string that are enclosed by an uneven amount of curly brackets (to account for escaped
        ///     brackets).
        ///     The result will be elements that are either plain integers or integers with a format appended to it, delimited by a
        ///     colon.
        /// </summary>
        /// <param name="input">The format string with placeholders.</param>
        /// <returns>Returns a collection of matches according to the regex.</returns>
        internal static MatchCollection GetPlaceholders(string input)
        {
            return Regex.Matches(input, Pattern);
        }

        /// <summary>
        ///     Returns all elements from the input, split on the placeholders.
        ///     This method is useful if you want to make use of the rest of the string as well.
        /// </summary>
        internal static string[] GetPlaceholdersSplit(string input)
        {
            return Regex.Split(input, Pattern);
        }
    }
}
\$\endgroup\$

2 Answers 2

10
\$\begingroup\$
private const string Pattern = @"(?<!\{)\{(?:\{\{)*(\d+(?::.*?)?)\}(?:\}\})*(?!\})";

That pattern is the only reason you need a helper method like this:

/// <returns>Returns the placeholder index.</returns>
internal static string Normalize(string input)
{
    var temp = input.Trim('{', '}');
    var colonIndex = temp.IndexOf(':');
    if (colonIndex > 0)
    {
        return temp.Remove(colonIndex);
    }

    return temp;
}

You could leverage named groups and, on top of a more readable regex, you get named match groups and zero string-manipulation work needed to figure out the placeholder index:

"(?<!\{)\{(?:\{\{)*((?<index>\d+)(?::.*?)?)\}(?:\}\})*(?!\})"

That's it! Your index is given to you as part of the match!

regex validation results

\$\endgroup\$
8
\$\begingroup\$

While reading my code for the bazillionth time, I noticed a logical error which would make it possible for very specific exotic scenarios to pop this up expectedly.

At the very start of the analyzer there is a check to see whether or not we're dealing with a string.Format() call:

// Verify we're dealing with a string.Format() call
var memberAccessExpression = invocation.Expression as MemberAccessExpressionSyntax;
if (memberAccessExpression != null)
{
    var invokedType = context.SemanticModel.GetSymbolInfo(memberAccessExpression.Expression);
    var invokedMethod = context.SemanticModel.GetSymbolInfo(memberAccessExpression.Name);
    if (invokedType.Symbol == null || invokedMethod.Symbol == null)
    {
        return;
    }

    if (invokedType.Symbol.MetadataName != typeof(string).Name || invokedMethod.Symbol.MetadataName != nameof(string.Format))
    {
        return;
    }
}

What's relevant here is the general if statement:

var memberAccessExpression = invocation.Expression as MemberAccessExpressionSyntax;
if (memberAccessExpression != null)
{
}

Which begs the question: what if the invocation's expression is not of type MemberAccessExpressionSyntax?!

After some searching I found an example of an invocation that isn't like this: a simple method call! There the expression is simply an IdentifierNameSyntax.

To keep a long story short -- in order for us to report the diagnostic, we also have to adhere to the following restrictions:

Make sure there are at least 2 arguments

if (invocation.ArgumentList == null || invocation.ArgumentList.Arguments.Count < 2)

Make sure the first argument is a string literal

if (!(firstArgument.Expression is LiteralExpressionSyntax)

Make sure there are placeholders in the string

if (placeholders.Count <= 1)

Make sure the placeholders are in the wrong order

if (firstValue > secondValue)

Knowing all this, I tried my code with the following setup:

Program()
{
    MyMethod("{1}{0}", 2, 3);
}

void MyMethod(string x, int y, int z) { }

and most surely: it provides me with a warning and a fix (which works!) to swap the placeholders and the arguments.


The solution is easy: add an else { return; } to the first if statement.

\$\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.