Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SA1413 now also checks enum members. #2245

Merged
merged 2 commits into from
Dec 24, 2016
Merged

SA1413 now also checks enum members. #2245

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b0e5f48
SA1413 now also checks enum members.
Dec 6, 2016
763906e
Changes due to review comments.
Dec 8, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 5 additions & 65 deletions ...eCop.Analyzers/StyleCop.Analyzers.CodeFixes/MaintainabilityRules/SA1413CodeFixProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,15 @@

namespace StyleCop.Analyzers.MaintainabilityRules
{
using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Composition;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Helpers;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CodeActions;
using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Text;

/// <summary>
/// Implements a code fix for <see cref="SA1413UseTrailingCommasInMultiLineInitializers"/>.
Expand Down Expand Up @@ -53,67 +49,11 @@ public override Task RegisterCodeFixesAsync(CodeFixContext context)
private static async Task<Document> GetTransformedDocumentAsync(Document document, Diagnostic diagnostic, CancellationToken cancellationToken)
{
var syntaxRoot = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
var parent = syntaxRoot.FindNode(diagnostic.Location.SourceSpan).Parent;
var text = await document.GetTextAsync(cancellationToken).ConfigureAwait(false);
var syntaxNode = syntaxRoot.FindNode(diagnostic.Location.SourceSpan);

SyntaxNode newParent = null;
switch (parent.Kind())
{
case SyntaxKind.ObjectInitializerExpression:
case SyntaxKind.ArrayInitializerExpression:
case SyntaxKind.CollectionInitializerExpression:
newParent = RewriteInitializer((InitializerExpressionSyntax)parent);
break;

case SyntaxKind.AnonymousObjectCreationExpression:
newParent = RewriteAnonymousObjectInitializer((AnonymousObjectCreationExpressionSyntax)parent);
break;

default:
throw new InvalidOperationException("Unknown initializer type: " + parent.Kind());
}

var newSyntaxRoot = syntaxRoot.ReplaceNode(parent, newParent);

var newDocument = document.WithSyntaxRoot(newSyntaxRoot.WithoutFormatting());
return newDocument;
}

private static SyntaxNode RewriteInitializer(InitializerExpressionSyntax initializer)
{
var existingItems = new List<ExpressionSyntax>(initializer.Expressions);
var last = existingItems.Last();
existingItems.Remove(last);
existingItems.Add(last.WithoutTrailingTrivia());

var existingSeparators = initializer.Expressions.GetSeparators();
var newSeparators = new List<SyntaxToken>(existingSeparators);
newSeparators.Add(SyntaxFactory.Token(SyntaxKind.CommaToken).WithTrailingTrivia(last.GetTrailingTrivia()));

var newInitializerExpressions = SyntaxFactory.SeparatedList(
existingItems,
newSeparators);

var fixedInitializer = initializer.WithExpressions(newInitializerExpressions);
return fixedInitializer;
}

private static SyntaxNode RewriteAnonymousObjectInitializer(AnonymousObjectCreationExpressionSyntax initializer)
{
var existingItems = new List<AnonymousObjectMemberDeclaratorSyntax>(initializer.Initializers);
var last = existingItems.Last();
existingItems.Remove(last);
existingItems.Add(last.WithoutTrailingTrivia());

var existingSeparators = initializer.Initializers.GetSeparators();
var newSeparators = new List<SyntaxToken>(existingSeparators);
newSeparators.Add(SyntaxFactory.Token(SyntaxKind.CommaToken).WithTrailingTrivia(last.GetTrailingTrivia()));

var newInitializerExpressions = SyntaxFactory.SeparatedList(
existingItems,
newSeparators);

var fixedInitializer = initializer.WithInitializers(newInitializerExpressions);
return fixedInitializer;
TextChange textChange = new TextChange(diagnostic.Location.SourceSpan, syntaxNode.ToString() + ",");
return document.WithText(text.WithChanges(textChange));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 The code above should be the same pattern as the code for RewriteInitializer and RewriteAnonymousObjectInitializer.
If this code performs better than the existing code, then please also change the other methods to use the same pattern, for consistency.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the code to only use my method and it still passes all the unit tests. Not sure how to test if it performs better but it does a lot less work: f983b5f#diff-9ac99be9ffc83f0bfb6aa1415e965bccR49.

}
}
92 changes: 92 additions & 0 deletions StyleCop.Analyzers/StyleCop.Analyzers.Test/MaintainabilityRules/SA1413UnitTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,98 @@ void Foo()
await this.VerifyCSharpFixAsync(testCode, fixedTestCode).ConfigureAwait(false);
}

/// <summary>
/// Verifies that the last value of an empty enum does not produce a diagnostic.
/// </summary>
/// <returns>A <see cref="Task"/> representing the asynchronous unit test.</returns>
[Fact]
public async Task VerifyEmptyEnumAsync()
{
var testCode = @"enum EmptyEnum
{
}
";

await this.VerifyCSharpDiagnosticAsync(testCode, EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false);
}

/// <summary>
/// Verifies that the last value of an enum with a trailing comma does not produce a diagnostic.
/// </summary>
/// <returns>A <see cref="Task"/> representing the asynchronous unit test.</returns>
[Fact]
public async Task VerifyEnumWithTrailingCommaAsync()
{
var testCode = @"enum TestEnum
{
One,
Two,
}
";

await this.VerifyCSharpDiagnosticAsync(testCode, EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false);
}

/// <summary>
/// Verifies that the last value of an enum without a trailing comma produces a diagnostic.
/// </summary>
/// <returns>A <see cref="Task"/> representing the asynchronous unit test.</returns>
[Fact]
public async Task VerifyEnumWithoutTrailingCommaAsync()
{
var testCode = @"enum TestEnum
{
One,
Two
}
";

var fixedTestCode = @"enum TestEnum
{
One,
Two,
}
";

DiagnosticResult[] expected =
{
this.CSharpDiagnostic().WithLocation(4, 5),
};

await this.VerifyCSharpDiagnosticAsync(testCode, expected, CancellationToken.None).ConfigureAwait(false);
await this.VerifyCSharpDiagnosticAsync(fixedTestCode, EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false);
await this.VerifyCSharpFixAsync(testCode, fixedTestCode).ConfigureAwait(false);
}

/// <summary>
/// Verifies that the last value of an enum without a trailing comma produces a diagnostic.
/// </summary>
/// <returns>A <see cref="Task"/> representing the asynchronous unit test.</returns>
[Fact]
public async Task VerifyEnumWithValueWithoutTrailingCommaAsync()
{
var testCode = @"enum TestEnum
{
One = 2 /* test comment */
}
";

var fixedTestCode = @"enum TestEnum
{
One = 2, /* test comment */
}
";

DiagnosticResult[] expected =
{
this.CSharpDiagnostic().WithLocation(3, 5),
};

await this.VerifyCSharpDiagnosticAsync(testCode, expected, CancellationToken.None).ConfigureAwait(false);
await this.VerifyCSharpDiagnosticAsync(fixedTestCode, EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false);
await this.VerifyCSharpFixAsync(testCode, fixedTestCode).ConfigureAwait(false);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add test cases where the element without comma is directly followed by an end of line comment. Like:

    var testCode = @"enum TestEnum
{
    One = 2 /* test comment */
}
";

and verify that it is properly changed to

    var testCode = @"enum TestEnum
{
    One = 2, /* test comment */
}
";

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed my unit test and it still passes

/// <inheritdoc/>
protected override CodeFixProvider GetCSharpCodeFixProvider()
{
Expand Down
17 changes: 17 additions & 0 deletions ...StyleCop.Analyzers/MaintainabilityRules/SA1413UseTrailingCommasInMultiLineInitializers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ namespace StyleCop.Analyzers.MaintainabilityRules
{
using System;
using System.Collections.Immutable;
using System.Linq;
using Helpers;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
Expand Down Expand Up @@ -73,6 +74,22 @@ public override void Initialize(AnalysisContext context)

context.RegisterSyntaxNodeAction(HandleObjectInitializerAction, ObjectInitializerKinds);
context.RegisterSyntaxNodeAction(HandleAnonymousObjectInitializerAction, SyntaxKind.AnonymousObjectCreationExpression);
context.RegisterSyntaxNodeAction(HandleEnumMemberDeclarationAction, SyntaxKind.EnumDeclaration);
}

private static void HandleEnumMemberDeclarationAction(SyntaxNodeAnalysisContext context)
{
var initializer = (EnumDeclarationSyntax)context.Node;
var lastMember = initializer.Members.LastOrDefault();
if (lastMember == null)
{
return;
}

if (initializer.Members.Count() != initializer.Members.SeparatorCount)
{
context.ReportDiagnostic(Diagnostic.Create(Descriptor, lastMember.GetLocation()));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code above should follow the same pattern as the methods below. Specifically you can check EnumDeclarationSyntax.Members.Count against EnumDeclarationSyntax.Members.SeparatorCount. That will work just as well and it will perform quite a bit better.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing that out. I was looking for something like this 👍

}

private static void HandleObjectInitializer(SyntaxNodeAnalysisContext context)
Expand Down