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

Add new analyzer/fixer for async void [RelayCommand] methods #714

Merged
merged 5 commits into from
Jun 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Collections.Immutable;
using System.Composition;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CodeActions;
using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Editing;
using Microsoft.CodeAnalysis.Simplification;
using Microsoft.CodeAnalysis.Text;
using static CommunityToolkit.Mvvm.SourceGenerators.Diagnostics.DiagnosticDescriptors;

namespace CommunityToolkit.Mvvm.CodeFixers;

/// <summary>
/// A code fixer that automatically updates the return type of <see langword="async"/> <see cref="void"/> methods using <c>[RelayCommand]</c> to return a <see cref="Task"/> instead.
/// </summary>
[ExportCodeFixProvider(LanguageNames.CSharp)]
[Shared]
public sealed class AsyncVoidReturningRelayCommandMethodCodeFixer : CodeFixProvider
{
/// <inheritdoc/>
public override ImmutableArray<string> FixableDiagnosticIds { get; } = ImmutableArray.Create(AsyncVoidReturningRelayCommandMethodId);

/// <inheritdoc/>
public override FixAllProvider? GetFixAllProvider()
{
return WellKnownFixAllProviders.BatchFixer;
}

/// <inheritdoc/>
public override async Task RegisterCodeFixesAsync(CodeFixContext context)
{
Diagnostic diagnostic = context.Diagnostics[0];
TextSpan diagnosticSpan = context.Span;

SyntaxNode? root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false);

// Get the method declaration from the target diagnostic
if (root!.FindNode(diagnosticSpan) is MethodDeclarationSyntax methodDeclaration)
{
// Register the code fix to update the return type to be Task instead
context.RegisterCodeFix(
CodeAction.Create(
title: "Change return type to Task",
createChangedDocument: token => ChangeReturnType(context.Document, root, methodDeclaration, token),
equivalenceKey: "Change return type to Task"),
diagnostic);
}
}

/// <summary>
/// Applies the code fix to a target method declaration and returns an updated document.
/// </summary>
/// <param name="document">The original document being fixed.</param>
/// <param name="root">The original tree root belonging to the current document.</param>
/// <param name="methodDeclaration">The <see cref="MethodDeclarationSyntax"/> to update.</param>
/// <param name="cancellationToken">The cancellation token for the operation.</param>
/// <returns>An updated document with the applied code fix, and the return type of the method being <see cref="Task"/>.</returns>
private static async Task<Document> ChangeReturnType(Document document, SyntaxNode root, MethodDeclarationSyntax methodDeclaration, CancellationToken cancellationToken)
{
// Get the semantic model (bail if it's not available)
if (await document.GetSemanticModelAsync(cancellationToken) is not SemanticModel semanticModel)
{
return document;
}

// Also bail if we can't resolve the Task symbol (this should really never happen)
if (semanticModel.Compilation.GetTypeByMetadataName("System.Threading.Tasks.Task") is not INamedTypeSymbol taskSymbol)
{
return document;
}

// Create the new syntax node for the return, and configure it to automatically add "using System.Threading.Tasks;" if needed
SyntaxNode typeSyntax = SyntaxGenerator.GetGenerator(document).TypeExpression(taskSymbol).WithAdditionalAnnotations(Simplifier.AddImportsAnnotation);

// Replace the void return type with the newly created Task type expression
return document.WithSyntaxRoot(root.ReplaceNode(methodDeclaration.ReturnType, typeSyntax));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,9 @@ public sealed class ClassUsingAttributeInsteadOfInheritanceCodeFixer : CodeFixPr
public override async Task RegisterCodeFixesAsync(CodeFixContext context)
{
Diagnostic diagnostic = context.Diagnostics[0];
TextSpan diagnosticSpan = diagnostic.Location.SourceSpan;
TextSpan diagnosticSpan = context.Span;

// Retrieve the property passed by the analyzer
// Retrieve the properties passed by the analyzer
if (diagnostic.Properties[ClassUsingAttributeInsteadOfInheritanceAnalyzer.TypeNameKey] is not string typeName ||
diagnostic.Properties[ClassUsingAttributeInsteadOfInheritanceAnalyzer.AttributeTypeNameKey] is not string attributeTypeName)
{
Expand All @@ -59,11 +59,9 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context)
context.RegisterCodeFix(
CodeAction.Create(
title: "Inherit from ObservableObject",
createChangedDocument: token => UpdateReference(context.Document, root, classDeclaration, attributeTypeName),
createChangedDocument: token => RemoveAttribute(context.Document, root, classDeclaration, attributeTypeName),
equivalenceKey: "Inherit from ObservableObject"),
diagnostic);

return;
}
}

Expand All @@ -75,7 +73,7 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context)
/// <param name="classDeclaration">The <see cref="ClassDeclarationSyntax"/> to update.</param>
/// <param name="attributeTypeName">The name of the attribute that should be removed.</param>
/// <returns>An updated document with the applied code fix, and <paramref name="classDeclaration"/> inheriting from <c>ObservableObject</c>.</returns>
private static Task<Document> UpdateReference(Document document, SyntaxNode root, ClassDeclarationSyntax classDeclaration, string attributeTypeName)
private static Task<Document> RemoveAttribute(Document document, SyntaxNode root, ClassDeclarationSyntax classDeclaration, string attributeTypeName)
{
// Insert ObservableObject always in first position in the base list. The type might have
// some interfaces in the base list, so we just copy them back after ObservableObject.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public sealed class FieldReferenceForObservablePropertyFieldCodeFixer : CodeFixP
public override async Task RegisterCodeFixesAsync(CodeFixContext context)
{
Diagnostic diagnostic = context.Diagnostics[0];
TextSpan diagnosticSpan = diagnostic.Location.SourceSpan;
TextSpan diagnosticSpan = context.Span;

// Retrieve the properties passed by the analyzer
if (diagnostic.Properties[FieldReferenceForObservablePropertyFieldAnalyzer.FieldNameKey] is not string fieldName ||
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,3 +66,4 @@ Rule ID | Category | Severity | Notes
--------|----------|----------|-------
MVVMTK0037 | CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator | Error | See https://aka.ms/mvvmtoolkit/errors/mvvmtk0037
MVVMTK0038 | CommunityToolkit.Mvvm.SourceGenerators.RelayCommandGenerator | Error | See https://aka.ms/mvvmtoolkit/errors/mvvmtk0038
MVVMTK0039 | CommunityToolkit.Mvvm.SourceGenerators.RelayCommandGenerator | Warning | See https://aka.ms/mvvmtoolkit/errors/mvvmtk0039
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
<Compile Include="$(MSBuildThisFileDirectory)ComponentModel\ObservableValidatorValidateAllPropertiesGenerator.Execute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)ComponentModel\TransitiveMembersGenerator.cs" />
<Compile Include="$(MSBuildThisFileDirectory)ComponentModel\TransitiveMembersGenerator.Execute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Diagnostics\Analyzers\AsyncVoidReturningRelayCommandMethodAnalyzer.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Diagnostics\Analyzers\InvalidClassLevelNotifyDataErrorInfoAttributeAnalyzer.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Diagnostics\Analyzers\InvalidClassLevelNotifyPropertyChangedRecipientsAttributeAnalyzer.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Diagnostics\Analyzers\ClassUsingAttributeInsteadOfInheritanceAnalyzer.cs" />
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Collections.Immutable;
using System.Linq;
using CommunityToolkit.Mvvm.SourceGenerators.Extensions;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Diagnostics;
using static CommunityToolkit.Mvvm.SourceGenerators.Diagnostics.DiagnosticDescriptors;

namespace CommunityToolkit.Mvvm.SourceGenerators;

/// <summary>
/// A diagnostic analyzer that generates a warning when using <c>[RelayCommand]</c> over an <see langword="async"/> <see cref="void"/> method.
/// </summary>
[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class AsyncVoidReturningRelayCommandMethodAnalyzer : DiagnosticAnalyzer
{
/// <inheritdoc/>
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(AsyncVoidReturningRelayCommandMethod);

/// <inheritdoc/>
public override void Initialize(AnalysisContext context)
{
context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.Analyze | GeneratedCodeAnalysisFlags.ReportDiagnostics);
context.EnableConcurrentExecution();

context.RegisterCompilationStartAction(static context =>
{
// Get the symbol for [RelayCommand]
if (context.Compilation.GetTypeByMetadataName("CommunityToolkit.Mvvm.Input.RelayCommandAttribute") is not INamedTypeSymbol relayCommandSymbol)
{
return;
}

context.RegisterSymbolAction(context =>
{
// We're only looking for async void methods
if (context.Symbol is not IMethodSymbol { IsAsync: true, ReturnsVoid: true } methodSymbol)
{
return;
}

// We only care about methods annotated with [RelayCommand]
if (!methodSymbol.HasAttributeWithType(relayCommandSymbol))
{
return;
}

// Warn on async void methods using [RelayCommand] (they should return a Task instead)
context.ReportDiagnostic(Diagnostic.Create(
AsyncVoidReturningRelayCommandMethod,
context.Symbol.Locations.FirstOrDefault(),
context.Symbol));
}, SymbolKind.Method);
});
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@ internal static class DiagnosticDescriptors
/// </summary>
public const string FieldReferenceForObservablePropertyFieldId = "MVVMTK0034";

/// <summary>
/// The diagnostic id for <see cref="AsyncVoidReturningRelayCommandMethod"/>.
/// </summary>
public const string AsyncVoidReturningRelayCommandMethodId = "MVVMTK0039";

/// <summary>
/// Gets a <see cref="DiagnosticDescriptor"/> indicating when a duplicate declaration of <see cref="INotifyPropertyChanged"/> would happen.
/// <para>
Expand Down Expand Up @@ -637,4 +642,20 @@ internal static class DiagnosticDescriptors
isEnabledByDefault: true,
description: "All attributes targeting the generated field or property for a method annotated with [RelayCommand] must be using valid expressions.",
helpLinkUri: "https://aka.ms/mvvmtoolkit/errors/mvvmtk0038");

/// <summary>
/// Gets a <see cref="DiagnosticDescriptor"/> indicating when a method with <c>[RelayCommand]</c> is async void.
/// <para>
/// Format: <c>"The method {0} annotated with [RelayCommand] is async void (make sure to return a Task type instead)"</c>.
/// </para>
/// </summary>
public static readonly DiagnosticDescriptor AsyncVoidReturningRelayCommandMethod = new DiagnosticDescriptor(
id: AsyncVoidReturningRelayCommandMethodId,
title: "Async void returning method annotated with RelayCommand",
messageFormat: "The method {0} annotated with [RelayCommand] is async void (make sure to return a Task type instead)",
category: typeof(RelayCommandGenerator).FullName,
Copy link
Contributor

Choose a reason for hiding this comment

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

I see this is already existing for all other descriptors, but I never seen categories to be very long names (CommunityToolkit.Mvvm.SourceGenerators.RelayCommandGenerator)

Copy link
Member Author

Choose a reason for hiding this comment

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

Mmh good point. We might want to address this in a separate PR for all descriptors 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note: this will be a breaking change if someone is currently changing severity via dotnet_analyzer_diagnostic.category-TheLongCategoryName.severity = ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Gotcha. I suppose we could do that in 9.0 then 🙂

defaultSeverity: DiagnosticSeverity.Warning,
isEnabledByDefault: true,
description: "All asynchronous methods annotated with [RelayCommand] should return a Task type, to benefit from the additional support provided by AsyncRelayCommand and AsyncRelayCommand<T>.",
helpLinkUri: "https://aka.ms/mvvmtoolkit/errors/mvvmtk0039");
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Threading.Tasks;
using CommunityToolkit.Mvvm.Input;
using Microsoft.CodeAnalysis.Testing;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using CSharpCodeFixTest = Microsoft.CodeAnalysis.CSharp.Testing.CSharpCodeFixTest<
CommunityToolkit.Mvvm.SourceGenerators.AsyncVoidReturningRelayCommandMethodAnalyzer,
CommunityToolkit.Mvvm.CodeFixers.AsyncVoidReturningRelayCommandMethodCodeFixer,
Microsoft.CodeAnalysis.Testing.Verifiers.MSTestVerifier>;
using CSharpCodeFixVerifier = Microsoft.CodeAnalysis.CSharp.Testing.CSharpCodeFixVerifier<
CommunityToolkit.Mvvm.SourceGenerators.AsyncVoidReturningRelayCommandMethodAnalyzer,
CommunityToolkit.Mvvm.CodeFixers.AsyncVoidReturningRelayCommandMethodCodeFixer,
Microsoft.CodeAnalysis.Testing.Verifiers.MSTestVerifier>;

namespace CommunityToolkit.Mvvm.SourceGenerators.Roslyn401.UnitTests;

[TestClass]
public class Test_AsyncVoidReturningRelayCommandMethodCodeFixer
{
[TestMethod]
public async Task AsyncVoidMethod_FileContainsSystemThreadingTasksUsingDirective()
{
string original = """
using System.Threading.Tasks;
using CommunityToolkit.Mvvm.Input;

partial class C
{
[RelayCommand]
private async void Foo()
{
}
}
""";

string @fixed = """
using System.Threading.Tasks;
using CommunityToolkit.Mvvm.Input;

partial class C
{
[RelayCommand]
private async Task Foo()
{
}
}
""";

CSharpCodeFixTest test = new()
{
TestCode = original,
FixedCode = @fixed,
ReferenceAssemblies = ReferenceAssemblies.Net.Net60
};

test.TestState.AdditionalReferences.Add(typeof(RelayCommand).Assembly);
test.ExpectedDiagnostics.AddRange(new[]
{
// /0/Test0.cs(7,24): error MVVMTK0039: The method C.Foo() annotated with [RelayCommand] is async void (make sure to return a Task type instead)
CSharpCodeFixVerifier.Diagnostic().WithSpan(7, 24, 7, 27).WithArguments("C.Foo()")
Sergio0694 marked this conversation as resolved.
Show resolved Hide resolved
});

await test.RunAsync();
}

[TestMethod]
public async Task AsyncVoidMethod_FileDoesNotContainSystemThreadingTasksUsingDirective()
{
string original = """
using CommunityToolkit.Mvvm.Input;

partial class C
{
[RelayCommand]
private async void Foo()
{
}
}
""";

string @fixed = """
using System.Threading.Tasks;
Sergio0694 marked this conversation as resolved.
Show resolved Hide resolved
using CommunityToolkit.Mvvm.Input;

partial class C
{
[RelayCommand]
private async Task Foo()
{
}
}
""";

CSharpCodeFixTest test = new()
{
TestCode = original,
FixedCode = @fixed,
ReferenceAssemblies = ReferenceAssemblies.Net.Net60
};

test.TestState.AdditionalReferences.Add(typeof(RelayCommand).Assembly);
test.ExpectedDiagnostics.AddRange(new[]
{
// /0/Test0.cs(7,24): error MVVMTK0039: The method C.Foo() annotated with [RelayCommand] is async void (make sure to return a Task type instead)
CSharpCodeFixVerifier.Diagnostic().WithSpan(6, 24, 6, 27).WithArguments("C.Foo()")
});

await test.RunAsync();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1801,6 +1801,25 @@ public class TestAttribute : Attribute
VerifyGeneratedDiagnostics<RelayCommandGenerator>(source, "MVVMTK0038");
}

[TestMethod]
public async Task AsyncVoidReturningRelayCommandMethodAnalyzer()
{
string source = """
using System;
using CommunityToolkit.Mvvm.Input;

public partial class MyViewModel
{
[RelayCommand]
private async void {|MVVMTK0039:TestAsync|}()
{
}
}
""";

await VerifyAnalyzerDiagnosticsAndSuccessfulGeneration<AsyncVoidReturningRelayCommandMethodAnalyzer>(source, LanguageVersion.CSharp8);
}
Sergio0694 marked this conversation as resolved.
Show resolved Hide resolved

/// <summary>
/// Verifies the diagnostic errors for a given analyzer, and that all available source generators can run successfully with the input source (including subsequent compilation).
/// </summary>
Expand Down