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.

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

Implement analyzer and codefix to switch to Assert.ThrowsExactly[Async] #4459

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -168,4 +168,7 @@
<data name="UseProperAssertMethodsFix" xml:space="preserve">
<value>Use '{0}'</value>
</data>
</root>
<data name="UseNewerAssertThrows" xml:space="preserve">
<value>Use '{0}'</value>
</data>
</root>
146 changes: 146 additions & 0 deletions src/Analyzers/MSTest.Analyzers.CodeFixes/UseNewerAssertThrowsFixer.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System.Collections.Immutable;
using System.Composition;

using Analyzer.Utilities;

using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CodeActions;
using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Editing;

using MSTest.Analyzers.Helpers;

namespace MSTest.Analyzers;

[ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(UseNewerAssertThrowsFixer))]
[Shared]
public sealed class UseNewerAssertThrowsFixer : CodeFixProvider
{
public sealed override ImmutableArray<string> FixableDiagnosticIds { get; }
= ImmutableArray.Create(DiagnosticIds.UseNewerAssertThrowsRuleId);

public override FixAllProvider GetFixAllProvider()
// See https://github.com/dotnet/roslyn/blob/main/docs/analyzers/FixAllProvider.md for more information on Fix All Providers
=> WellKnownFixAllProviders.BatchFixer;

public override async Task RegisterCodeFixesAsync(CodeFixContext context)
{
SyntaxNode root = await context.Document.GetRequiredSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false);
Diagnostic diagnostic = context.Diagnostics[0];

SyntaxNode diagnosticNode = root.FindNode(diagnostic.Location.SourceSpan, getInnermostNodeForTie: true);
if (diagnosticNode is not InvocationExpressionSyntax invocation)
{
Debug.Fail($"Is this an interesting scenario where IInvocationOperation for Assert call isn't associated with InvocationExpressionSyntax? SyntaxNode type: '{diagnosticNode.GetType()}', Text: '{diagnosticNode.GetText()}'");
return;
}

SyntaxNode methodNameIdentifier = invocation.Expression;
if (methodNameIdentifier is MemberAccessExpressionSyntax memberAccess)
{
methodNameIdentifier = memberAccess.Name;
}

if (methodNameIdentifier is not GenericNameSyntax genericNameSyntax)
{
Debug.Fail($"Is this an interesting scenario where we are unable to retrieve GenericNameSyntax corresponding to the assert method? SyntaxNode type: '{methodNameIdentifier}', Text: '{methodNameIdentifier.GetText()}'.");
return;
}

string updatedMethodName = genericNameSyntax.Identifier.Text switch
{
"ThrowsException" => "ThrowsExactly",
"ThrowsExceptionAsync" => "ThrowsExactlyAsync",
// The analyzer should report a diagnostic only for ThrowsException and ThrowsExceptionAsync
_ => throw ApplicationStateGuard.Unreachable(),
};

context.RegisterCodeFix(
CodeAction.Create(
title: string.Format(CultureInfo.InvariantCulture, CodeFixResources.UseNewerAssertThrows, updatedMethodName),
ct => Task.FromResult(context.Document.WithSyntaxRoot(UpdateMethodName(new SyntaxEditor(root, context.Document.Project.Solution.Workspace), invocation, genericNameSyntax, updatedMethodName, diagnostic.AdditionalLocations))),
equivalenceKey: nameof(UseProperAssertMethodsFixer)),
diagnostic);
}

private static SyntaxNode UpdateMethodName(SyntaxEditor editor, InvocationExpressionSyntax invocation, GenericNameSyntax genericNameSyntax, string updatedMethodName, IReadOnlyList<Location> additionalLocations)
{
editor.ReplaceNode(genericNameSyntax, genericNameSyntax.WithIdentifier(SyntaxFactory.Identifier(updatedMethodName).WithTriviaFrom(genericNameSyntax.Identifier)));

// The object[] parameter to format the message is named parameters in the old ThrowsException[Async] methods, but is named messageArgs in the new ThrowsExactly[Async] methods.
if (invocation.ArgumentList.Arguments.FirstOrDefault(arg => arg.NameColon is { Name.Identifier.Text: "parameters" }) is { } arg)
{
editor.ReplaceNode(arg.NameColon!.Name, arg.NameColon!.Name.WithIdentifier(SyntaxFactory.Identifier("messageArgs").WithTriviaFrom(arg.NameColon.Name.Identifier)));
}

if (additionalLocations.Count == 1)
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
{
// The existing ThrowsException call is using the Func<object> overload. The new ThrowsExactly method does not have this overload, so we need to adjust.
// This is a best effort handling.
SyntaxNode actionArgument = editor.OriginalRoot.FindNode(additionalLocations[0].SourceSpan, getInnermostNodeForTie: true);

// If it's not ParenthesizedLambdaExpressionSyntax (e.g, a variable of type Func<object>), we can switch it to '() => _ = variableName()'
// But for now, we will only handle ParenthesizedLambdaExpressionSyntax.
if (actionArgument is ParenthesizedLambdaExpressionSyntax lambdaSyntax)
{
if (lambdaSyntax.ExpressionBody is not null)
{
editor.ReplaceNode(
lambdaSyntax.ExpressionBody,
AssignToDiscard(lambdaSyntax.ExpressionBody));
}
else if (lambdaSyntax.Block is not null)
{
// This is more complex. We need to iterate through all descendants of type ReturnStatementSyntax, and split it into two statements.
// The first statement will be an assignment expression to a discard, and the second statement will be 'return;'.
// We may even need to add extra braces in case the return statement (for example) is originally inside an if statement without braces.
// For example:
// if (condition)
// return Whatever;
// should be converted to:
// if (condition)
// {
// _ = Whatever;
// return;
// }
// Keep in mind: When descending into descendant nodes, we shouldn't descend into potential other lambda expressions or local functions.
IEnumerable<ReturnStatementSyntax> returnStatements = lambdaSyntax.Block.DescendantNodes(descendIntoChildren: node => node is not (LocalFunctionStatementSyntax or AnonymousFunctionExpressionSyntax)).OfType<ReturnStatementSyntax>();
foreach (ReturnStatementSyntax returnStatement in returnStatements)
{
if (returnStatement.Expression is not { } returnExpression)
{
// This should be an error in user code.
continue;
}

ExpressionStatementSyntax returnReplacement = SyntaxFactory.ExpressionStatement(AssignToDiscard(returnStatement.Expression));

if (returnStatement.Parent is BlockSyntax blockSyntax)
{
editor.InsertAfter(returnStatement, SyntaxFactory.ReturnStatement());
editor.ReplaceNode(returnStatement, returnReplacement);
}
else
{
editor.ReplaceNode(
returnStatement,
SyntaxFactory.Block(
returnReplacement,
SyntaxFactory.ReturnStatement()));
}
}
}
}
}

return editor.GetChangedRoot();
}

private static AssignmentExpressionSyntax AssignToDiscard(ExpressionSyntax expression)
=> SyntaxFactory.AssignmentExpression(SyntaxKind.SimpleAssignmentExpression, SyntaxFactory.IdentifierName("_"), expression);
}
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,11 @@
<target state="translated">Přidat [TestMethod]</target>
<note />
</trans-unit>
<trans-unit id="UseNewerAssertThrows">
<source>Use '{0}'</source>
<target state="new">Use '{0}'</target>
<note />
</trans-unit>
<trans-unit id="UseProperAssertMethodsFix">
<source>Use '{0}'</source>
<target state="translated">Použít {0}</target>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,11 @@
<target state="translated">„[TestMethod]“ hinzufügen</target>
<note />
</trans-unit>
<trans-unit id="UseNewerAssertThrows">
<source>Use '{0}'</source>
<target state="new">Use '{0}'</target>
<note />
</trans-unit>
<trans-unit id="UseProperAssertMethodsFix">
<source>Use '{0}'</source>
<target state="translated">"{0}" verwenden</target>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,11 @@
<target state="translated">Agregar '[TestMethod]'</target>
<note />
</trans-unit>
<trans-unit id="UseNewerAssertThrows">
<source>Use '{0}'</source>
<target state="new">Use '{0}'</target>
<note />
</trans-unit>
<trans-unit id="UseProperAssertMethodsFix">
<source>Use '{0}'</source>
<target state="translated">Usar "{0}"</target>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,11 @@
<target state="translated">Ajouter « [TestMethod] »</target>
<note />
</trans-unit>
<trans-unit id="UseNewerAssertThrows">
<source>Use '{0}'</source>
<target state="new">Use '{0}'</target>
<note />
</trans-unit>
<trans-unit id="UseProperAssertMethodsFix">
<source>Use '{0}'</source>
<target state="translated">Utiliser « {0} »</target>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,11 @@
<target state="translated">Aggiungi '[TestMethod]'</target>
<note />
</trans-unit>
<trans-unit id="UseNewerAssertThrows">
<source>Use '{0}'</source>
<target state="new">Use '{0}'</target>
<note />
</trans-unit>
<trans-unit id="UseProperAssertMethodsFix">
<source>Use '{0}'</source>
<target state="translated">Usa '{0}'</target>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,11 @@
<target state="translated">'[TestMethod]' の追加</target>
<note />
</trans-unit>
<trans-unit id="UseNewerAssertThrows">
<source>Use '{0}'</source>
<target state="new">Use '{0}'</target>
<note />
</trans-unit>
<trans-unit id="UseProperAssertMethodsFix">
<source>Use '{0}'</source>
<target state="translated">'{0}' を使用します</target>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,11 @@
<target state="translated">'[TestMethod]' 추가</target>
<note />
</trans-unit>
<trans-unit id="UseNewerAssertThrows">
<source>Use '{0}'</source>
<target state="new">Use '{0}'</target>
<note />
</trans-unit>
<trans-unit id="UseProperAssertMethodsFix">
<source>Use '{0}'</source>
<target state="translated">'{0}' 사용</target>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,11 @@
<target state="translated">Dodaj „[TestMethod]”</target>
<note />
</trans-unit>
<trans-unit id="UseNewerAssertThrows">
<source>Use '{0}'</source>
<target state="new">Use '{0}'</target>
<note />
</trans-unit>
<trans-unit id="UseProperAssertMethodsFix">
<source>Use '{0}'</source>
<target state="translated">Użyj „{0}”</target>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,11 @@
<target state="translated">Adicionar ''[TestMethod]"</target>
<note />
</trans-unit>
<trans-unit id="UseNewerAssertThrows">
<source>Use '{0}'</source>
<target state="new">Use '{0}'</target>
<note />
</trans-unit>
<trans-unit id="UseProperAssertMethodsFix">
<source>Use '{0}'</source>
<target state="translated">Usar '{0}'</target>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,11 @@
<target state="translated">Добавить "[TestMethod]"</target>
<note />
</trans-unit>
<trans-unit id="UseNewerAssertThrows">
<source>Use '{0}'</source>
<target state="new">Use '{0}'</target>
<note />
</trans-unit>
<trans-unit id="UseProperAssertMethodsFix">
<source>Use '{0}'</source>
<target state="translated">Использовать "{0}"</target>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,11 @@
<target state="translated">'[TestMethod]' ekle</target>
<note />
</trans-unit>
<trans-unit id="UseNewerAssertThrows">
<source>Use '{0}'</source>
<target state="new">Use '{0}'</target>
<note />
</trans-unit>
<trans-unit id="UseProperAssertMethodsFix">
<source>Use '{0}'</source>
<target state="translated">'{0}' kullan</target>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,11 @@
<target state="translated">添加“[TestMethod]”</target>
<note />
</trans-unit>
<trans-unit id="UseNewerAssertThrows">
<source>Use '{0}'</source>
<target state="new">Use '{0}'</target>
<note />
</trans-unit>
<trans-unit id="UseProperAssertMethodsFix">
<source>Use '{0}'</source>
<target state="translated">使用“{0}”</target>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,11 @@
<target state="translated">新增 '[TestMethod]'</target>
<note />
</trans-unit>
<trans-unit id="UseNewerAssertThrows">
<source>Use '{0}'</source>
<target state="new">Use '{0}'</target>
<note />
</trans-unit>
<trans-unit id="UseProperAssertMethodsFix">
<source>Use '{0}'</source>
<target state="translated">使用 '{0}'</target>
Expand Down
5 changes: 5 additions & 0 deletions src/Analyzers/MSTest.Analyzers/AnalyzerReleases.Unshipped.md
Original file line number Diff line number Diff line change
@@ -1,2 +1,7 @@
; Unshipped analyzer release
; https://github.com/dotnet/roslyn-analyzers/blob/main/src/Microsoft.CodeAnalysis.Analyzers/ReleaseTrackingAnalyzers.Help.md
### New Rules

Rule ID | Category | Severity | Notes
--------|----------|----------|-------
MSTEST0038 | Usage | Info | UseNewerAssertThrowsAnalyzer
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions src/Analyzers/MSTest.Analyzers/Helpers/DiagnosticIds.cs
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,5 @@ internal static class DiagnosticIds
public const string UseDeploymentItemWithTestMethodOrTestClassRuleId = "MSTEST0035";
public const string DoNotUseShadowingRuleId = "MSTEST0036";
public const string UseProperAssertMethodsRuleId = "MSTEST0037";
public const string UseNewerAssertThrowsRuleId = "MSTEST0038";
}
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ internal static class WellKnownTypeNames
public const string System = "System";
public const string SystemCollectionsGenericIEnumerable1 = "System.Collections.Generic.IEnumerable`1";
public const string SystemDescriptionAttribute = "System.ComponentModel.DescriptionAttribute";
public const string SystemFunc1 = "System.Func`1";
public const string SystemIAsyncDisposable = "System.IAsyncDisposable";
public const string SystemIDisposable = "System.IDisposable";
public const string SystemReflectionMethodInfo = "System.Reflection.MethodInfo";
Expand Down
18 changes: 18 additions & 0 deletions src/Analyzers/MSTest.Analyzers/Resources.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 7 additions & 1 deletion src/Analyzers/MSTest.Analyzers/Resources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -543,4 +543,10 @@ The type declaring these methods should also respect the following rules:
<data name="DynamicDataShouldBeValidMessageFormat_SourceTypeNotPropertyOrMethod" xml:space="preserve">
<value>'[DynamicData]' member '{0}.{1}' is not a property nor a method. Only properties and methods are supported.</value>
</data>
</root>
<data name="UseNewerAssertThrowsTitle" xml:space="preserve">
<value>Use newer methods to assert exceptions</value>
Copy link
Member

Choose a reason for hiding this comment

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

I don't recall, do we see title on the menu or message?

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 think the message. Not sure of all places that show the title, but Solution Explorer is one that shows the title (when you expand Dependencies -> Analyzers)

</data>
<data name="UseNewerAssertThrowsMessageFormat" xml:space="preserve">
<value>Use 'Assert.ThrowsExactly' instead of 'Assert.ThrowsException'</value>
</data>
</root>
Loading
Loading