[API Compat] Add attribute diffing for generic and regular parameters (…

…#7058)

* Add attribute diffing for generic and regular parameters

* PR Feedback (avoid duplication)

src/Microsoft.DotNet.ApiCompat/src/Rules/Compat/AttributeDifference.cs

@@ -34,7 +34,7 @@ public override DifferenceType Diff(IDifferences differences, IAssembly impl, IA
     [ExportDifferenceRule]
     internal class AttributeDifference : CompatDifferenceRule
     {
-        private MappingSettings _settings = new MappingSettings()
+        private readonly MappingSettings _settings = new MappingSettings()
         {
             Filter = new AttributesFilter(includeAttributes: true)
         };
@@ -64,25 +64,58 @@ public override DifferenceType Diff(IDifferences differences, ITypeDefinition im
             if (impl == null || contract == null)
                 return DifferenceType.Unknown;
 
-            return CheckAttributeDifferences(differences, impl, impl.Attributes, contract.Attributes);
+            bool changed = CheckAttributeDifferences(differences, impl, impl.Attributes, contract.Attributes);
+            if (impl.IsGeneric)
+            {
+                IGenericParameter[] method1GenParams = impl.GenericParameters.ToArray();
+                IGenericParameter[] method2GenParam = contract.GenericParameters.ToArray();
+                for (int i = 0; i < impl.GenericParameterCount; i++)
+                    changed |= CheckAttributeDifferences(differences, method1GenParams[i], method1GenParams[i].Attributes, method2GenParam[i].Attributes, member: contract);
+            }
+
+            return changed ? DifferenceType.Changed : DifferenceType.Unchanged; ;
         }
 
         public override DifferenceType Diff(IDifferences differences, ITypeDefinitionMember impl, ITypeDefinitionMember contract)
         {
-            if (impl == null || contract == null)
+            var implMethod = impl as IMethodDefinition;
+            var contractMethod = contract as IMethodDefinition;
+            if (implMethod == null || contractMethod == null)
                 return DifferenceType.Unknown;
 
-            return CheckAttributeDifferences(differences, impl, impl.Attributes, contract.Attributes);
+            bool changed = CheckAttributeDifferences(differences, implMethod, implMethod.Attributes, implMethod.Attributes);
+
+            IParameterDefinition[] method1Params = implMethod.Parameters.ToArray();
+            IParameterDefinition[] method2Params = contractMethod.Parameters.ToArray();
+            for (int i = 0; i < implMethod.ParameterCount; i++)
+                changed |= CheckAttributeDifferences(differences, method1Params[i], method1Params[i].Attributes, method2Params[i].Attributes, member: implMethod);
+
+            if (implMethod.IsGeneric)
+            {
+                IGenericParameter[] method1GenParams = implMethod.GenericParameters.ToArray();
+                IGenericParameter[] method2GenParam = contractMethod.GenericParameters.ToArray();
+                for (int i = 0; i < implMethod.GenericParameterCount; i++)
+                    changed |= CheckAttributeDifferences(differences, method1GenParams[i], method1GenParams[i].Attributes, method2GenParam[i].Attributes, member: implMethod);
+            }
+
+            return changed ? DifferenceType.Changed : DifferenceType.Unchanged;
         }
 
-        private DifferenceType CheckAttributeDifferences(IDifferences differences, IReference target, IEnumerable<ICustomAttribute> implAttributes, IEnumerable<ICustomAttribute> contractAttributes)
+        private bool CheckAttributeDifferences(IDifferences differences, IReference target, IEnumerable<ICustomAttribute> implAttributes, IEnumerable<ICustomAttribute> contractAttributes, IReference member = null)
         {
-            DifferenceType difference = DifferenceType.Unchanged;
             AttributesMapping<IEnumerable<ICustomAttribute>> attributeMapping = new AttributesMapping<IEnumerable<ICustomAttribute>>(_settings);
             AttributeComparer attributeComparer = new AttributeComparer();
             attributeMapping.AddMapping(0, contractAttributes.OrderBy(a => a, attributeComparer));
             attributeMapping.AddMapping(1, implAttributes.OrderBy(a => a, attributeComparer));
 
+            string errString = $"'{target.FullName()}'";
+            if (target is IParameterDefinition || target is IGenericParameter)
+            {
+                errString = target is IGenericParameter ? "generic param" : "parameter";
+                errString += $" '{target.FullName()}' on member '{member?.FullName()}'";
+            }
+
+            bool changed = false;
             foreach (var group in attributeMapping.Attributes)
             {
                 switch (group.Difference)
@@ -96,8 +129,9 @@ private DifferenceType CheckAttributeDifferences(IDifferences differences, IRefe
 
                             // Allow for additions
                             differences.Add(new Difference("AddedAttribute",
-                                $"Attribute '{type.FullName()}' exists on '{target.FullName()}' in the {Implementation} but not the {Contract}."));
+                                $"Attribute '{type.FullName()}' exists on {errString} in the {Implementation} but not the {Contract}."));
 
+                            changed = true;
                             break;
                         }
                     case DifferenceType.Changed:
@@ -111,9 +145,9 @@ private DifferenceType CheckAttributeDifferences(IDifferences differences, IRefe
                             string implementationKey = attributeComparer.GetKey(group[1].Attributes.First());
 
                             differences.AddIncompatibleDifference("CannotChangeAttribute",
-                                $"Attribute '{type.FullName()}' on '{target.FullName()}' changed from '{contractKey}' in the {Contract} to '{implementationKey}' in the {Implementation}.");
+                                $"Attribute '{type.FullName()}' on {errString} changed from '{contractKey}' in the {Contract} to '{implementationKey}' in the {Implementation}.");
 
-                            difference = DifferenceType.Changed;
+                            changed = true;
                             break;
                         }
 
@@ -125,16 +159,16 @@ private DifferenceType CheckAttributeDifferences(IDifferences differences, IRefe
                                 break;
 
                             differences.AddIncompatibleDifference("CannotRemoveAttribute",
-                                $"Attribute '{type.FullName()}' exists on '{target.FullName()}' in the {Contract} but not the {Implementation}.");
+                                $"Attribute '{type.FullName()}' exists on {errString} in the {Contract} but not the {Implementation}.");
 
 
                             // removals of an attribute are considered a "change" of the type
-                            difference = DifferenceType.Changed;
+                            changed = true;
                             break;
                         }
                 }
             }
-            return difference;
+            return changed;
         }
     }
 

src/Microsoft.DotNet.ApiCompat/tests/AttributeDifferenceTests.cs

@@ -3,9 +3,7 @@
 
 using System;
 using System.IO;
-using System.Reflection;
 using Xunit;
-using Xunit.Abstractions;
 
 namespace Microsoft.DotNet.ApiCompat.Tests
 {
@@ -22,7 +20,11 @@ public void AttributeDifferenceIsFound()
 
             Assert.Contains("CannotRemoveAttribute : Attribute 'System.ComponentModel.DesignerAttribute' exists on 'AttributeDifference.AttributeDifferenceClass1' in the implementation but not the contract.", runOutput);
             Assert.Contains("CannotRemoveAttribute : Attribute 'AttributeDifference.FooAttribute' exists on 'AttributeDifference.AttributeDifferenceClass1' in the implementation but not the contract.", runOutput);
-            Assert.Contains("Total Issues: 2", runOutput);
+            Assert.Contains("CannotRemoveAttribute : Attribute 'System.ComponentModel.DefaultValueAttribute' exists on generic param 'T' on member 'AttributeDifference.AttributeDifferenceClass1.GenericMethodWithAttribute<T>()' in the implementation but not the contract.", runOutput);
+            Assert.Contains("CannotRemoveAttribute : Attribute 'System.ComponentModel.DefaultValueAttribute' exists on generic param 'T' on member 'AttributeDifference.AttributeDifferenceClass1.GenericMethodWithAttribute<T>()' in the implementation but not the contract.", runOutput);
+            Assert.Contains("CannotRemoveAttribute : Attribute 'AttributeDifference.FooAttribute' exists on parameter 'myParameter' on member 'AttributeDifference.AttributeDifferenceClass1.MethodWithAttribute(System.String, System.Object)' in the implementation but not the contract.", runOutput);
+            Assert.Contains("CannotRemoveAttribute : Attribute 'System.ComponentModel.DefaultValueAttribute' exists on generic param 'TOne' on member 'AttributeDifference.AttributeDifferenceGenericCLass<TOne, TTwo>' in the implementation but not the contract.", runOutput);
+            Assert.Contains("Total Issues: 5", runOutput);
         }
 
         [Fact]
@@ -34,15 +36,15 @@ public void AttributeDifferenceIsFoundWithExcludeAttributesFile()
 
             string runOutput = Helpers.RunApiCompat(_implementationPath, new string[] { _contractPath }, new string[] { excludeAttributesFile.Path }, "implementation", "contract");
             Assert.Contains("CannotRemoveAttribute : Attribute 'System.ComponentModel.DesignerAttribute' exists on 'AttributeDifference.AttributeDifferenceClass1' in the implementation but not the contract.", runOutput);
-            Assert.Contains("Total Issues: 1", runOutput);
+            Assert.Contains("Total Issues: 3", runOutput);
         }
 
         [Fact]
         public void NoIssuesWithExcludeAttributesFile()
         {
             using TempFile excludeAttributesFile = TempFile.Create();
 
-            File.WriteAllLines(excludeAttributesFile.Path, new string[] { "T:System.ComponentModel.DesignerAttribute", "T:AttributeDifference.FooAttribute" });
+            File.WriteAllLines(excludeAttributesFile.Path, new string[] { "T:System.ComponentModel.DesignerAttribute", "T:AttributeDifference.FooAttribute", "T:System.ComponentModel.DefaultValueAttribute" });
 
             string runOutput = Helpers.RunApiCompat(_implementationPath, new string[] { _contractPath }, new string[] { excludeAttributesFile.Path }, null, null);
 

src/Microsoft.DotNet.ApiCompat/tests/Helpers.cs

@@ -16,7 +16,7 @@ public static string RunApiCompat(string left, IEnumerable<string> rightDirs, IE
         {
             using var writer = new StringWriter();
 
-            var args = Helpers.GetApiCompatArgs(left, rightDirs, excludeAttributesFile, leftName, rightName);
+            var args = GetApiCompatArgs(left, rightDirs, excludeAttributesFile, leftName, rightName);
             new ApiCompatRunner(writer).Run(args);
 
             return writer.ToString();

src/Microsoft.DotNet.ApiCompat/tests/TestProjects/AttributeDifference/Contract/AttributeDifference.cs

@@ -7,6 +7,11 @@ namespace AttributeDifference
 {
     [DisplayName("Attribute difference class1")]
     public class AttributeDifferenceClass1
+    {
+        public string MethodWithAttribute(string myParameter, [DefaultValue("myObject")] object myObject) => throw null;
+        public T GenericMethodWithAttribute<T>() => throw null;
+    }
+    public class AttributeDifferenceGenericCLass<TOne, [DefaultValue("TTwo")] TTwo>
     {
     }
 }

src/Microsoft.DotNet.ApiCompat/tests/TestProjects/AttributeDifference/Implementation/AttributeDifferenceClass1.cs

@@ -10,6 +10,12 @@ namespace AttributeDifference
     [DisplayName("Attribute difference class1")]
     [Foo]
     public class AttributeDifferenceClass1
+    {
+        public string MethodWithAttribute([Foo] string myParameter, [DefaultValue("myObject")] object myObject) => myParameter;
+        public T GenericMethodWithAttribute<[DefaultValue("T")] T>() => default(T);
+    }
+
+    public class AttributeDifferenceGenericCLass<[DefaultValue("TOne")] TOne, [DefaultValue("TTwo")] TTwo>
     {
     }