Merge pull request #181 from CommunityToolkit/dev/icommand-overloads-diagnostics

Fix [ICommand] generator crashing with overloads, add diagnostics

Author: Sergio0694

CommunityToolkit.Mvvm.SourceGenerators/AnalyzerReleases.Unshipped.md

@@ -27,3 +27,4 @@ MVVMTK0019 | CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator
 MVVMTK0020 | CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator | Error | See https://aka.ms/mvvmtoolkit/error
 MVVMTK0021 | CommunityToolkit.Mvvm.SourceGenerators.ObservableRecipientGenerator | Error | See https://aka.ms/mvvmtoolkit/error
 MVVMTK0022 | CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator | Error | See https://aka.ms/mvvmtoolkit/error
+MVVMTK0023 | CommunityToolkit.Mvvm.SourceGenerators.ICommandGenerator | Error | See https://aka.ms/mvvmtoolkit/error

CommunityToolkit.Mvvm.SourceGenerators/Diagnostics/DiagnosticDescriptors.cs

@@ -146,7 +146,7 @@ internal static class DiagnosticDescriptors
     /// Format: <c>"The CanExecute name must refer to a valid member, but "{0}" has no matches in type {1}"</c>.
     /// </para>
     /// </summary>
-    public static readonly DiagnosticDescriptor InvalidCanExecuteMemberName = new DiagnosticDescriptor(
+    public static readonly DiagnosticDescriptor InvalidCanExecuteMemberNameError = new DiagnosticDescriptor(
         id: "MVVMTK0009",
         title: "Invalid ICommand.CanExecute member name",
         messageFormat: "The CanExecute name must refer to a valid member, but \"{0}\" has no matches in type {1}",
@@ -162,7 +162,7 @@ internal static class DiagnosticDescriptors
     /// Format: <c>"The CanExecute name must refer to a single member, but "{0}" has multiple matches in type {1}"</c>.
     /// </para>
     /// </summary>
-    public static readonly DiagnosticDescriptor MultipleCanExecuteMemberNameMatches = new DiagnosticDescriptor(
+    public static readonly DiagnosticDescriptor MultipleCanExecuteMemberNameMatchesError = new DiagnosticDescriptor(
         id: "MVVMTK0010",
         title: "Multiple ICommand.CanExecute member name matches",
         messageFormat: "The CanExecute name must refer to a single member, but \"{0}\" has multiple matches in type {1}",
@@ -178,7 +178,7 @@ internal static class DiagnosticDescriptors
     /// Format: <c>"The CanExecute name must refer to a compatible member, but no valid members were found for "{0}" in type {1}"</c>.
     /// </para>
     /// </summary>
-    public static readonly DiagnosticDescriptor InvalidCanExecuteMember = new DiagnosticDescriptor(
+    public static readonly DiagnosticDescriptor InvalidCanExecuteMemberError = new DiagnosticDescriptor(
         id: "MVVMTK0011",
         title: "No valid ICommand.CanExecute member match",
         messageFormat: "The CanExecute name must refer to a compatible member, but no valid members were found for \"{0}\" in type {1}",
@@ -363,4 +363,20 @@ internal static class DiagnosticDescriptors
         isEnabledByDefault: true,
         description: "Fields annotated with [AlsoBroadcastChange] must be contained in a type that inherits from ObservableRecipient or that is annotated with [ObservableRecipient] (including base types).",
         helpLinkUri: "https://aka.ms/mvvmtoolkit");
+
+    /// <summary>
+    /// Gets a <see cref="DiagnosticDescriptor"/> indicating when a specified <c>[ICommand]</c> method has any overloads.
+    /// <para>
+    /// Format: <c>"The CanExecute name must refer to a single member, but "{0}" has multiple matches in type {1}"</c>.
+    /// </para>
+    /// </summary>
+    public static readonly DiagnosticDescriptor MultipleICommandMethodOverloadsError = new DiagnosticDescriptor(
+        id: "MVVMTK0023",
+        title: "Multiple overloads for method annotated with ICommand",
+        messageFormat: "The method {0}.{1} cannot be annotated with [ICommand], has it has multiple overloads (command methods must be unique within their containing type)",
+        category: typeof(ICommandGenerator).FullName,
+        defaultSeverity: DiagnosticSeverity.Error,
+        isEnabledByDefault: true,
+        description: "Methods with multiple overloads cannot be annotated with [ICommand], as command methods must be unique within their containing type.",
+        helpLinkUri: "https://aka.ms/mvvmtoolkit");
 }

CommunityToolkit.Mvvm.SourceGenerators/Input/ICommandGenerator.Execute.cs

@@ -36,6 +36,12 @@ internal static class Execute
         {
             ImmutableArray<Diagnostic>.Builder builder = ImmutableArray.CreateBuilder<Diagnostic>();
 
+            // Validate the method definition is unique
+            if (!IsCommandDefinitionUnique(methodSymbol, builder))
+            {
+                goto Failure;
+            }
+
             // Get the command field and property names
             (string fieldName, string propertyName) = GetGeneratedFieldAndPropertyNames(methodSymbol);
 
@@ -300,6 +306,40 @@ public static ImmutableArray<MemberDeclarationSyntax> GetSyntax(CommandInfo comm
             return ImmutableArray.Create<MemberDeclarationSyntax>(fieldDeclaration, propertyDeclaration);
         }
 
+        /// <summary>
+        /// Validates that a target method used as source for a command is unique within its containing type.
+        /// </summary>
+        /// <param name="methodSymbol">The input <see cref="IMethodSymbol"/> instance to process.</param>
+        /// <param name="diagnostics">The current collection of gathered diagnostics.</param>
+        /// <returns>Whether or not <paramref name="methodSymbol"/> was unique within its containing type.</returns>
+        private static bool IsCommandDefinitionUnique(IMethodSymbol methodSymbol, ImmutableArray<Diagnostic>.Builder diagnostics)
+        {
+            foreach (ISymbol symbol in methodSymbol.ContainingType.GetMembers(methodSymbol.Name))
+            {
+                if (symbol is IMethodSymbol otherSymbol &&
+                    otherSymbol.HasAttributeWithFullyQualifiedName("global::CommunityToolkit.Mvvm.Input.ICommandAttribute"))
+                {
+                    // If the first [ICommand] overload is the current symbol, return immediately. This makes it so
+                    // that if multiple overloads are present, only the ones after the first declared one will have
+                    // diagnostics generated for them, while the first one will remain valid and will keep working.
+                    if (SymbolEqualityComparer.Default.Equals(methodSymbol, otherSymbol))
+                    {
+                        return true;
+                    }
+
+                    diagnostics.Add(
+                        MultipleICommandMethodOverloadsError,
+                        methodSymbol,
+                        methodSymbol.ContainingType,
+                        methodSymbol);
+
+                    return false;
+                }
+            }
+
+            return true;
+        }
+
         /// <summary>
         /// Get the generated field and property names for the input method.
         /// </summary>
@@ -554,7 +594,7 @@ private static bool TryGetCanExecuteExpressionType(
 
             if (memberName is null)
             {
-                diagnostics.Add(InvalidCanExecuteMemberName, methodSymbol, memberName ?? string.Empty, methodSymbol.ContainingType);
+                diagnostics.Add(InvalidCanExecuteMemberNameError, methodSymbol, memberName ?? string.Empty, methodSymbol.ContainingType);
 
                 goto Failure;
             }
@@ -571,11 +611,11 @@ private static bool TryGetCanExecuteExpressionType(
                     return true;
                 }
 
-                diagnostics.Add(InvalidCanExecuteMemberName, methodSymbol, memberName, methodSymbol.ContainingType);
+                diagnostics.Add(InvalidCanExecuteMemberNameError, methodSymbol, memberName, methodSymbol.ContainingType);
             }
             else if (canExecuteSymbols.Length > 1)
             {
-                diagnostics.Add(MultipleCanExecuteMemberNameMatches, methodSymbol, memberName, methodSymbol.ContainingType);
+                diagnostics.Add(MultipleCanExecuteMemberNameMatchesError, methodSymbol, memberName, methodSymbol.ContainingType);
             }
             else if (TryGetCanExecuteExpressionFromSymbol(canExecuteSymbols[0], commandTypeArguments, out canExecuteExpressionType))
             {
@@ -585,7 +625,7 @@ private static bool TryGetCanExecuteExpressionType(
             }
             else
             {
-                diagnostics.Add(InvalidCanExecuteMember, methodSymbol, memberName, methodSymbol.ContainingType);
+                diagnostics.Add(InvalidCanExecuteMemberError, methodSymbol, memberName, methodSymbol.ContainingType);
             }
 
             Failure:

tests/CommunityToolkit.Mvvm.SourceGenerators.UnitTests/Test_SourceGeneratorsDiagnostics.cs

@@ -1080,6 +1080,31 @@ public partial class MyViewModel : ObservableObject
         VerifyGeneratedDiagnostics<ObservablePropertyGenerator>(source, "MVVMTK0022");
     }
 
+    [TestMethod]
+    public void MultipleICommandMethodOverloads_WithOverloads()
+    {
+        string source = @"
+            using CommunityToolkit.Mvvm.Input;
+
+            namespace MyApp
+            {
+                public partial class SampleViewModel
+                {
+                    [ICommand]
+                    private void GreetUser()
+                    {
+                    }
+
+                    [ICommand]
+                    private void GreetUser(object value)
+                    {
+                    }
+                }
+            }";
+
+        VerifyGeneratedDiagnostics<ICommandGenerator>(source, "MVVMTK0023");
+    }
+
     /// <summary>
     /// Verifies the output of a source generator.
     /// </summary>