Fix bug preventing attributes with arguments from getting either suppressed or all kept (without a docId collision).

Author: carlossanlop

src/Compatibility/ApiDiff/Microsoft.DotNet.ApiDiff/MemoryOutputDiffGenerator.cs

@@ -2,11 +2,8 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System.Collections.Concurrent;
-using System.Collections.Generic;
-using System.Collections.Specialized;
 using System.Diagnostics;
 using System.Diagnostics.CodeAnalysis;
-using System.Reflection;
 using DiffPlex.DiffBuilder;
 using DiffPlex.DiffBuilder.Model;
 using Microsoft.CodeAnalysis;
@@ -305,7 +302,7 @@ private static string GetFinalAssemblyDiff(string assemblyName, string diffText)
         return sb.ToString();
     }
 
-    private static ConcurrentDictionary<string, MemberDeclarationSyntax> CollectChildrenNodes(SyntaxNode? parentNode, SemanticModel? model)
+    private ConcurrentDictionary<string, MemberDeclarationSyntax> CollectChildrenNodes(SyntaxNode? parentNode, SemanticModel? model)
     {
         if (parentNode == null)
         {
@@ -521,7 +518,7 @@ private Dictionary<string, AttributeSyntax> CollectAttributeNodes(MemberDeclarat
                     continue;
                 }
                 var realAttributeNode = attributeNode.WithArgumentList(attributeNode.ArgumentList);
-                if (!dictionary.TryAdd(realAttributeNode.ToFullString(), realAttributeNode))
+                if (!dictionary.TryAdd(GetAttributeDocId(realAttributeNode, model), realAttributeNode))
                 {
                     _log.LogWarning(string.Format(Resources.AttributeAlreadyExists, realAttributeNode.ToFullString(), GetDocId(memberNode, model)));
                 }
@@ -650,7 +647,33 @@ private static IEnumerable<T> GetMembersOfType<T>(SyntaxNode node) where T : Mem
         .Where(n => n is T m && IsEnumMemberOrHasPublicOrProtectedModifierOrIsDestructor(m))
         .Cast<T>();
 
-    private static string GetDocId(SyntaxNode node, SemanticModel model)
+    private string GetAttributeDocId(AttributeSyntax attribute, SemanticModel model)
+    {
+        string? attributeDocId = null;
+        if (attribute.ArgumentList is AttributeArgumentListSyntax list && list.Arguments.Any())
+        {
+            // Workaround to ensure that repeated attributes with different arguments are all included
+            attributeDocId = attribute.ToFullString();
+        }
+        else if (model.GetSymbolInfo(attribute).Symbol is IMethodSymbol constructor)
+        {
+            try
+            {
+                attributeDocId = constructor.ContainingType.GetDocumentationCommentId();
+            }
+            catch (Exception e)
+            {
+                _log.LogWarning(e.Message);
+            }
+        }
+        // Workaround because some rare cases of attributes in WinForms like System.Windows.Markup.ContentPropertyAttribute
+        // cannot be found in the current model.
+        attributeDocId ??= attribute.ToFullString();
+
+        return attributeDocId;
+    }
+
+    private string GetDocId(SyntaxNode node, SemanticModel model)
     {
         ISymbol? symbol = node switch
         {
@@ -660,7 +683,7 @@ private static string GetDocId(SyntaxNode node, SemanticModel model)
             EventFieldDeclarationSyntax eventFieldDeclaration => model.GetDeclaredSymbol(eventFieldDeclaration.Declaration.Variables.First()),
             PropertyDeclarationSyntax propertyDeclaration     => model.GetDeclaredSymbol(propertyDeclaration),
             _ => model.GetDeclaredSymbol(node)
-        };
+        }; 
 
         if (symbol?.GetDocumentationCommentId() is string docId)
         {

test/Microsoft.DotNet.ApiDiff.Tests/Diff.Attribute.Tests.cs

@@ -879,7 +879,74 @@ public class MyClass
                 attributesToExclude: ["T:MyNamespace.MyAttributeAttribute"]); // Overrides the default list
 
     [Fact]
-    public Task TestSuppressTypeAndAttributeUsage() => RunTestAsync(
+    public Task SuppressChangedAttribute() => RunTestAsync(
+                // Includes dummy property addition so that something else shows up in the diff, but not the attribute change.
+                beforeCode: """
+                namespace MyNamespace
+                {
+                    [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCode("Original text")]
+                    public class MyClass
+                    {
+                    }
+                }
+                """,
+                afterCode: """
+                using System;
+                namespace MyNamespace
+                {
+                    [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCode("Changed text")]
+                    public class MyClass
+                    {
+                        public int X { get; }
+                    }
+                }
+                """,
+                expectedCode: """
+                  namespace MyNamespace
+                  {
+                      public class MyClass
+                      {
+                +         public int X { get; }
+                      }
+                  }
+                """,
+                attributesToExclude: ["T:System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute"]); // Overrides the default list
+
+    [Fact]
+    public Task AttributesRepeatedWithDifferentArguments() => RunTestAsync(
+                beforeCode: """
+                namespace MyNamespace
+                {
+                    public class MyClass
+                    {
+                    }
+                }
+                """,
+                afterCode: """
+                using System;
+                namespace MyNamespace
+                {
+                    [System.Runtime.Versioning.UnsupportedOSPlatformAttribute("tvos")]
+                    [System.Runtime.Versioning.UnsupportedOSPlatformAttribute("linux")]
+                    public class MyClass
+                    {
+                    }
+                }
+                """,
+                expectedCode: """
+                  namespace MyNamespace
+                  {
+                +     [System.Runtime.Versioning.UnsupportedOSPlatformAttribute("tvos")]
+                +     [System.Runtime.Versioning.UnsupportedOSPlatformAttribute("linux")]
+                      public class MyClass
+                      {
+                      }
+                  }
+                """,
+                attributesToExclude: null); // null forces using the default list
+
+    [Fact]
+    public Task SuppressTypeAndAttributeUsage() => RunTestAsync(
                 beforeCode: """
                 namespace MyNamespace
                 {