Skip to content

Commit

Permalink
Ensure extracted variable names are safe, and rely on Roslyn to do mo…
Browse files Browse the repository at this point in the history 
…st of the formatting
  • Loading branch information
@bradwilson
bradwilson committed Nov 18, 2023
1 parent 755591a commit 03d1006
Show file tree
Hide file tree
Showing 2 changed files with 127 additions and 57 deletions.
121 changes: 70 additions & 51 deletions src/xunit.analyzers.fixes/X2000/AssertSingleShouldBeUsedForSingleParameterFixer.cs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
using System.Collections.Immutable;
using System.Composition;
using System.Globalization;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
Expand All @@ -8,6 +10,8 @@
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Editing;
using Microsoft.CodeAnalysis.Formatting;
using Microsoft.CodeAnalysis.Simplification;
using static Microsoft.CodeAnalysis.CSharp.SyntaxFactory;

namespace Xunit.Analyzers.Fixes;
Expand All @@ -22,6 +26,19 @@ public AssertSingleShouldBeUsedForSingleParameterFixer() :
base(Descriptors.X2023_AssertSingleShouldBeUsedForSingleParameter.Id)
{ }

static string GetSafeVariableName(
string targetParameterName,
ImmutableHashSet<string> localSymbols)
{
var idx = 2;
var result = targetParameterName;

while (localSymbols.Contains(result))
result = string.Format(CultureInfo.InvariantCulture, "{0}_{1}", targetParameterName, idx++);

return result;
}

public override async Task RegisterCodeFixesAsync(CodeFixContext context)
{
var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false);
Expand Down Expand Up @@ -61,36 +78,53 @@ static async Task<Document> UseSingleMethod(
if (invocation.Expression is MemberAccessExpressionSyntax memberAccess &&
invocation.ArgumentList.Arguments[0].Expression is IdentifierNameSyntax collectionVariable)
{
var replacementNode =
invocation
.WithArgumentList(ArgumentList(SeparatedList(new[] { Argument(collectionVariable) })))
.WithExpression(memberAccess.WithName(IdentifierName(replacementMethod)));

if (invocation.Parent != null)
var semanticModel = await document.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false);
if (semanticModel != null && invocation.Parent != null)
{
var leadingTrivia = invocation.Parent.GetLeadingTrivia();
var trailingTrivia = invocation.Parent.GetTrailingTrivia();
var startLocation = invocation.GetLocation().SourceSpan.Start;
var localSymbols = semanticModel.LookupSymbols(startLocation).OfType<ILocalSymbol>().Select(s => s.Name).ToImmutableHashSet();
var replacementNode =
invocation
.WithArgumentList(ArgumentList(SeparatedList(new[] { Argument(collectionVariable) })))
.WithExpression(memberAccess.WithName(IdentifierName(replacementMethod)));

if (invocation.ArgumentList.Arguments[1].Expression is SimpleLambdaExpressionSyntax lambdaExpression)
{
var originalParameterName = lambdaExpression.Parameter.Identifier.Text;
var parameterName = GetSafeVariableName(originalParameterName, localSymbols);

if (parameterName != originalParameterName)
{
var body = lambdaExpression.Body;
var tokens =
body
.DescendantTokens()
.Where(t => t.Kind() == SyntaxKind.IdentifierToken && t.Text == originalParameterName)
.ToArray();
body = body.ReplaceTokens(tokens, (t1, t2) => Identifier(t2.LeadingTrivia, parameterName, t2.TrailingTrivia));
lambdaExpression = lambdaExpression.WithBody(body);
}

var oneItemVariableStatement =
OneItemVariableStatement(lambdaExpression, replacementNode)
.WithLeadingTrivia(leadingTrivia);
OneItemVariableStatement(parameterName, replacementNode)
.WithLeadingTrivia(invocation.GetLeadingTrivia());

ReplaceCollectionWithSingle(editor, oneItemVariableStatement, invocation.Parent);
AppendLambdaStatements(editor, oneItemVariableStatement, lambdaExpression, leadingTrivia, trailingTrivia);
AppendLambdaStatements(editor, oneItemVariableStatement, lambdaExpression);
}
else if (invocation.ArgumentList.Arguments[1].Expression is IdentifierNameSyntax identifierExpression)
{
var semanticModel = await document.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false);
var isMethod = semanticModel.GetSymbolInfo(identifierExpression).Symbol?.Kind == SymbolKind.Method;
if (isMethod)
{
var oneItemVariableStatement = OneItemVariableStatement(DefaultParameterName, replacementNode)
.WithLeadingTrivia(leadingTrivia);
var parameterName = GetSafeVariableName(DefaultParameterName, localSymbols);

var oneItemVariableStatement =
OneItemVariableStatement(parameterName, replacementNode)
.WithLeadingTrivia(invocation.GetLeadingTrivia());

ReplaceCollectionWithSingle(editor, oneItemVariableStatement, invocation.Parent);
AppendMethodInvocation(editor, oneItemVariableStatement, identifierExpression, leadingTrivia, trailingTrivia);
AppendMethodInvocation(editor, oneItemVariableStatement, identifierExpression, parameterName);
}
}
}
Expand All @@ -99,15 +133,6 @@ static async Task<Document> UseSingleMethod(
return editor.GetChangedDocument();
}

static LocalDeclarationStatementSyntax OneItemVariableStatement(
SimpleLambdaExpressionSyntax lambdaExpression,
InvocationExpressionSyntax replacementNode)
{
var lambdaParameterName = lambdaExpression.Parameter.Identifier.ValueText;

return OneItemVariableStatement(lambdaParameterName, replacementNode);
}

static LocalDeclarationStatementSyntax OneItemVariableStatement(
string parameterName,
InvocationExpressionSyntax replacementNode)
Expand Down Expand Up @@ -139,48 +164,42 @@ static void ReplaceCollectionWithSingle(
static void AppendLambdaStatements(
DocumentEditor editor,
LocalDeclarationStatementSyntax oneItemVariableStatement,
SimpleLambdaExpressionSyntax lambdaExpression,
SyntaxTriviaList leadingTrivia,
SyntaxTriviaList trailingTrivia)
SimpleLambdaExpressionSyntax lambdaExpression)
{
if (lambdaExpression.ExpressionBody is InvocationExpressionSyntax lambdaBody)
{
var assertStatement =
editor.InsertAfter(
oneItemVariableStatement,
ExpressionStatement(lambdaBody)
.WithLeadingTrivia(leadingTrivia)
.WithTrailingTrivia(trailingTrivia);

editor.InsertAfter(oneItemVariableStatement, assertStatement);
.WithAdditionalAnnotations(Formatter.Annotation, Simplifier.Annotation)
);
}
else if (lambdaExpression.Block != null && lambdaExpression.Block.Statements.Count != 0)
{
var allLambdaBlockStatements = lambdaExpression.Block.Statements.Select((s, i) =>
{
s = s.WithoutTrivia().WithLeadingTrivia(leadingTrivia);
if (i == lambdaExpression.Block.Statements.Count - 1)
s = s.WithTrailingTrivia(trailingTrivia);
return s;
});

editor.InsertAfter(oneItemVariableStatement, allLambdaBlockStatements);
editor.InsertAfter(
oneItemVariableStatement,
lambdaExpression.Block.Statements.Select(
(s, i) => s.WithAdditionalAnnotations(Formatter.Annotation, Simplifier.Annotation)
)
);
}
}

static void AppendMethodInvocation(
DocumentEditor editor,
LocalDeclarationStatementSyntax oneItemVariableStatement,
IdentifierNameSyntax methodExpression,
SyntaxTriviaList leadingTrivia,
SyntaxTriviaList trailingTrivia)
string parameterName)
{
var methodStatement =
InvocationExpression(
methodExpression,
ArgumentList(SingletonSeparatedList(Argument(IdentifierName(DefaultParameterName))))
editor.InsertAfter(
oneItemVariableStatement,
ExpressionStatement(
InvocationExpression(
methodExpression,
ArgumentList(SingletonSeparatedList(Argument(IdentifierName(parameterName))))
)
)
.WithLeadingTrivia(leadingTrivia)
.WithTrailingTrivia(trailingTrivia);

editor.InsertAfter(oneItemVariableStatement, ExpressionStatement(methodStatement));
.WithAdditionalAnnotations(Formatter.Annotation, Simplifier.Annotation)
);
}
}
63 changes: 57 additions & 6 deletions ...xunit.analyzers.tests/Fixes/X2000/AssertSingleShouldBeUsedForSingleParameterFixerTests.cs
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using Microsoft.CodeAnalysis.CSharp;
using Xunit;
using Xunit.Analyzers.Fixes;
using Verify = CSharpVerifier<Xunit.Analyzers.AssertSingleShouldBeUsedForSingleParameter>;
Expand All @@ -10,32 +11,82 @@ public class AssertSingleShouldBeUsedForSingleParameterFixerTests
"[|Assert.Collection(collection, item => Assert.NotNull(item))|]",
@"var item = Assert.Single(collection);
Assert.NotNull(item);"
},
{
@"var item = 42;
[|Assert.Collection(collection, item => Assert.NotNull(item))|]",
@"var item = 42;
var item_2 = Assert.Single(collection);
Assert.NotNull(item_2);"
},
{
"[|Assert.Collection(collection, item => { Assert.NotNull(item); })|]",
@"var item = Assert.Single(collection);
Assert.NotNull(item);"
},
{
@"var item = 42;
[|Assert.Collection(collection, item => { Assert.NotNull(item); })|]",
@"var item = 42;
var item_2 = Assert.Single(collection);
Assert.NotNull(item_2);"
},
{
"[|Assert.Collection(collection, item => { Assert.NotNull(item); Assert.NotNull(item); })|]",
@"var item = Assert.Single(collection);
Assert.NotNull(item);
Assert.NotNull(item);"
Assert.NotNull(item); Assert.NotNull(item);"
},
{
@"var item = 42;
[|Assert.Collection(collection, item => { Assert.NotNull(item); Assert.NotNull(item); })|]",
@"var item = 42;
var item_2 = Assert.Single(collection);
Assert.NotNull(item_2); Assert.NotNull(item_2);"
},
{
@"[|Assert.Collection(collection, item => {
if (item != null) {
Assert.NotNull(item);
Assert.NotNull(item);
}
})|]",
@"var item = Assert.Single(collection);
if (item != null)
{
Assert.NotNull(item);
Assert.NotNull(item);
}"
},
{
@"var item = 42;
[|Assert.Collection(collection, item => {
if (item != null) {
Assert.NotNull(item);
Assert.NotNull(item);
}
})|]",
@"var item = Assert.Single(collection);
Assert.NotNull(item);
Assert.NotNull(item);"
@"var item = 42;
var item_2 = Assert.Single(collection);
if (item_2 != null)
{
Assert.NotNull(item_2);
Assert.NotNull(item_2);
}"
},
{
"[|Assert.Collection(collection, ElementInspector)|]",
@"var item = Assert.Single(collection);
ElementInspector(item);"
},
{
@"var item = 42;
var item_2 = 21.12;
[|Assert.Collection(collection, ElementInspector)|]",
@"var item = 42;
var item_2 = 21.12;
var item_3 = Assert.Single(collection);
ElementInspector(item_3);"
},
};

const string beforeTemplate = @"
Expand Down Expand Up @@ -79,6 +130,6 @@ public async void ReplacesCollectionMethod(
var before = string.Format(beforeTemplate, statementBefore);
var after = string.Format(afterTemplate, statementAfter);

await Verify.VerifyCodeFix(before, after, AssertSingleShouldBeUsedForSingleParameterFixer.Key_UseSingleMethod);
await Verify.VerifyCodeFix(LanguageVersion.CSharp8, before, after, AssertSingleShouldBeUsedForSingleParameterFixer.Key_UseSingleMethod);
}
}

0 comments on commit 03d1006

Please sign in to comment.