TTS-2142 Fix method grouping

.github/workflows/build-nuget-package.yml

@@ -48,7 +48,7 @@ jobs:
         - name: Run unit tests
           shell: pwsh
           run: |
-            $testFiles = Get-ChildItem -Path . -Filter "*test.dll" -Recurse | ForEach-Object { $_.FullName }
+            $testFiles = Get-ChildItem -Path . -Filter "ObfuscarTest*.dll" -Recurse | ForEach-Object { $_.FullName }
             foreach ($testFile in $testFiles) {
               dotnet test $testFile --logger trx
             }

Obfuscar/GraphNode.cs

@@ -24,19 +24,19 @@
 
 #endregion
 
+using Mono.Cecil;
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.Linq;
-using Mono.Cecil;
 
 namespace Obfuscar
 {
     [DebuggerDisplay("{type}")]
-    class GraphNode
+    internal class GraphNode
     {
-        private TypeKey type;
-        private bool scaned;
-        private List<GraphNode> baseNodes = new List<GraphNode>();
+        private readonly TypeKey type;
+        private bool scanned;
+        private readonly List<GraphNode> baseNodes = new List<GraphNode>();
 
         public GraphNode(TypeKey type)
         {
@@ -45,7 +45,7 @@ public GraphNode(TypeKey type)
 
         internal void Scan(Dictionary<TypeKey, GraphNode> nodes, HashSet<TypeKey> toRemove, Project project)
         {
-            if (scaned)
+            if (scanned)
             {
                 return;
             }
@@ -66,7 +66,7 @@ internal void Scan(Dictionary<TypeKey, GraphNode> nodes, HashSet<TypeKey> toRemo
                 }
             }
 
-            scaned = true;
+            scanned = true;
         }
 
         public static List<TypeKey> GetBaseTypes(Project project, TypeDefinition type)
@@ -102,7 +102,7 @@ public static List<TypeKey> GetBaseTypes(Project project, TypeDefinition type)
             return result;
         }
 
-        public void FillMethodGroup(IList<MethodGroup> groups, Project project)
+        public void FillMethodGroup(IDictionary<MethodKey, MethodGroup> groups, Project project)
         {
             if (baseNodes.Count == 0)
             {
@@ -132,32 +132,38 @@ public void FillMethodGroup(IList<MethodGroup> groups, Project project)
                     continue;
                 }
 
-                var newGroup = new MethodGroup();
-                newGroup.Methods.Add(new MethodKey(method));
+                var key = new MethodKey(method);
+                if (!groups.TryGetValue(key, out var newGroup))
+                {
+                    newGroup = new MethodGroup();
+                    newGroup.Methods.Add(key);
+                    groups[key] = newGroup;
+                }
+
                 MethodDefinition rootMethod = null;
                 if (method.Parameters.Any(p => p.ParameterType is GenericParameter))
                 {
                     // If the method has generic arguments we need to group it with overloads in the same class so they are renamed the same
                     // way, otherwise the call site updating may fail to choose the right overload
-                    MatchMethodGroup(method, newGroup, project, ref rootMethod);
+                    MatchMethodGroup(method, ref newGroup, groups, project, ref rootMethod);
                 }
                 else
                 {
                     foreach (var baseType in baseNodes)
                     {
-                        baseType.MatchMethodGroup(method, newGroup, project, ref rootMethod);
+                        baseType.MatchMethodGroup(method, ref newGroup, groups, project, ref rootMethod);
                     }
                 }
 
                 // Mark external methods declared in a base class outside the scanned class hierarchy
-                if (rootMethod == null && !newGroup.External && method.IsVirtual && method.IsReuseSlot && !method.IsSpecialName)
+                if (!newGroup.External && rootMethod == null && method.IsVirtual && method.IsReuseSlot && !method.IsSpecialName)
                 {
                     newGroup.External = true;
                 }
 
-                if (newGroup.Methods.Count > 1 || newGroup.External)
+                if (newGroup.Methods.Count < 2 && !newGroup.External)
                 {
-                    groups.Add(newGroup);
+                    groups.Remove(key);
                 }
             }
         }
@@ -179,22 +185,47 @@ private void FillMethods(IList<MethodDefinition> methods)
             }
         }
 
-        private void MatchMethodGroup(MethodDefinition method, MethodGroup newGroup, Project project, ref MethodDefinition rootMethod)
+        private void MatchMethodGroup(
+            MethodDefinition method, ref MethodGroup newGroup, IDictionary<MethodKey, MethodGroup> groups,
+            Project project, ref MethodDefinition rootMethod)
         {
             foreach (var baseMethod in type.TypeDefinition.Methods)
             {
                 if (MethodKey.MethodMatch(baseMethod, method)
                     || MethodKey.MethodMatch(method, baseMethod))
                 {
-                    newGroup.Methods.Add(new MethodKey(baseMethod));
                     newGroup.External |= !project.Contains(type);
                     if (baseMethod.IsNewSlot)
                         rootMethod = baseMethod;
+
+                    var baseKey = new MethodKey(baseMethod);
+                    if (!groups.TryGetValue(baseKey, out var baseGroup))
+                    {
+                        // add into the current group
+                        newGroup.Methods.Add(baseKey);
+                        groups[baseKey] = newGroup;
+                        continue;
+                    }
+
+                    if (baseGroup == newGroup)
+                    {
+                        // already in the current group
+                        continue;
+                    }
+
+                    // save a little time by updating the smaller group
+                    if (baseGroup.Methods.Count > newGroup.Methods.Count)
+                        (newGroup, baseGroup) = (baseGroup, newGroup);
+
+                    // merge into an existing group
+                    foreach (var key in baseGroup.Methods)
+                        groups[key] = newGroup;
+                    newGroup.Merge(baseGroup);
                 }
             }
             // Add base type methods recursively as the method might override something further up the hierarchy
             foreach (var baseType in baseNodes)
-                baseType.MatchMethodGroup(method, newGroup, project, ref rootMethod);
+                baseType.MatchMethodGroup(method, ref newGroup, groups, project, ref rootMethod);
         }
 
         internal TypeKey[] GetBaseTypes()

Obfuscar/InheritMap.cs

@@ -64,34 +64,13 @@ public InheritMap(Project project)
             //    nodes.Remove(item);
             //}
 
-            var methods = new List<MethodGroup>();
             var properties = new List<PropertyGroup>();
             foreach (var node in nodes)
             {
-                node.Value.FillMethodGroup(methods, Project);
+                node.Value.FillMethodGroup(methodGroups, Project);
                 node.Value.FillPropertyGroup(properties, Project);
             }
 
-            // Merge overlapping method groups
-            foreach (var group in methods)
-            {
-                MethodGroup mergeGroup = null;
-                foreach (var item in group.Methods)
-                {
-                    if (methodGroups.TryGetValue(item, out mergeGroup))
-                    {
-                        mergeGroup.Merge(group);
-                        break;
-                    }
-                }
-                if (mergeGroup == null)
-                    mergeGroup = group;
-                foreach (var item in group.Methods)
-                {
-                    methodGroups[item] = mergeGroup;
-                }
-            }
-
             // Merge overlapping property groups
             foreach (var group in properties)
             {

ObfuscarTestNet/CollectionExpressionTest.cs

@@ -12,7 +12,7 @@ public class CollectionExpressionTest
         [TestMethod]
         public void CheckCollectionExpression()
         {
-            string outputPath = TestHelper.OutputPath;
+            string outputPath = TestHelper.GenerateOutputPath();
             string xml = string.Format(
                 @"<?xml version='1.0'?>" +
                 @"<Obfuscator>" +

ObfuscarTestNet/Input/AssemblyWithGenericsHierarchy.cs

@@ -0,0 +1,56 @@
+using System.Reflection;
+
+namespace ObfuscarTestNet.Input
+{
+    /// <summary> Interface for testing proper generic method grouping. </summary>
+    public interface IBaseInterface<T>
+    {
+        /// <summary> This method would normally go into an int param method grouping. </summary>
+        void Method(int index, T value);
+
+        /// <summary> This method would normally go into a string param method grouping. </summary>
+        void Method(string key, T value);
+    }
+
+    /// <summary> Class obfuscated by default. </summary>
+    public class BaseClass1<T> : IBaseInterface<T>
+    {
+        /// <summary> This method should be named same as base method after obfuscation. </summary>
+        public virtual void Method(int index, T value)
+        {
+        }
+
+        /// <summary> This method should be named same as base method after obfuscation. </summary>
+        public virtual void Method(string key, T value)
+        {
+        }
+    }
+
+    /// <summary> Class excluded from obfuscation. </summary>
+    [Obfuscation(Exclude = true, ApplyToMembers = true)]
+    public class BaseClass2<T> : IBaseInterface<T>
+    {
+        /// <summary> This method would normally cause all int param methods to be skipped. </summary>
+        public virtual void Method(int index, T value)
+        {
+        }
+
+        /// <summary> This method would normally cause all string param methods to be skipped. </summary>
+        public virtual void Method(string key, T value)
+        {
+        }
+    }
+
+    /// <summary> Derived class excluded from obfuscation. </summary>
+    [Obfuscation(Exclude = true, ApplyToMembers = true)]
+    public class Class2<T, V> : BaseClass2<T>
+    {
+        /// <summary> Due to this method having two generic arguments it should be renamed same as both <see cref="IBaseInterface{T}"/> methods
+        /// for proper overload resolution, effectively merging the int and string parameter groups. But due to a bug in ThreeShape.Obfuscar v4.5.0,
+        /// the group merging fails and as a result one of the overloads in <see cref="BaseClass1{T}"/> gets renamed anyway
+        /// which leads to the method implementation being impossible to resolve at runtime. </summary>
+        public void Method(V key, T value)
+        {
+        }
+    }
+}

ObfuscarTestNet/MethodGroupingTest.cs

@@ -0,0 +1,40 @@
+using Obfuscar;
+using System.IO;
+using System.Linq;
+using System.Reflection;
+
+namespace ObfuscarTestNet
+{
+    [TestClass]
+    public class MethodGroupingTest
+    {
+        [TestMethod]
+        public void CheckGenericMethodGrouping()
+        {
+            // Arrange
+            const string assemblyName = "AssemblyWithGenericsHierarchy";
+            const string assemblyDll = $"{assemblyName}.dll";
+            var outputPath = TestHelper.GenerateOutputPath();
+            var xml = $"""
+                       <?xml version='1.0'?>
+                       <Obfuscator>
+                           <Var name='InPath' value='{TestHelper.InputPath}' />
+                           <Var name='OutPath' value='{outputPath}' />
+                           <Var name="AbortOnInconsistentState" value="false" />
+                           <Var name='KeepPublicApi' value='false' />
+                           <Module file='$(InPath){Path.DirectorySeparatorChar}{assemblyDll}'/>
+                       </Obfuscator>
+                       """;
+
+            // Act
+            var output = TestHelper.BuildAndObfuscate(assemblyName, xml);
+            var assembly = Assembly.LoadFrom(Path.GetFullPath(Path.Combine(outputPath, assemblyDll)));
+
+            // Assert
+
+            // all methods should have been skipped
+            Assert.IsTrue(output.Mapping.ClassMap.SelectMany(t => t.Value.Methods.Values).All(t => t.Status == ObfuscationStatus.Skipped));
+            Assert.IsTrue(assembly.DefinedTypes.SelectMany(t => t.DeclaredMethods).All(t => t.Name == "Method"));
+        }
+    }
+}

ObfuscarTestNet/SourceBuilder.cs

@@ -40,9 +40,10 @@ public string Build(string fileName, params string[] references)
             return outPath;
         }
 
-        public bool AddAssembly(string assemblyDll)
+        public void AddAssembly(string assemblyDll)
         {
-            if (string.IsNullOrEmpty(assemblyDll)) return false;
+            if (string.IsNullOrEmpty(assemblyDll))
+                throw new ArgumentException("Assembly DLL path cannot be null or empty.", nameof(assemblyDll));
 
             var file = Path.GetFullPath(assemblyDll);
 
@@ -52,22 +53,20 @@ public bool AddAssembly(string assemblyDll)
                 var path = Path.GetDirectoryName(typeof(object).Assembly.Location)!;
                 file = Path.Combine(path, assemblyDll);
                 if (!File.Exists(file))
-                    return false;
+                    Assert.Fail($"Could not find assembly file: {assemblyDll}");
             }
 
-            if (_references.Any(r => r.FilePath == file)) return true;
+            if (_references.Any(r => r.FilePath == file)) return;
 
             try
             {
                 var reference = MetadataReference.CreateFromFile(file);
                 _references.Add(reference);
             }
-            catch
+            catch (Exception e)
             {
-                return false;
+                Assert.Fail($"Could not add assembly reference \"{file}\": {e.Message}");
             }
-
-            return true;
         }
 
         private void AddAssemblies(params string[] assemblies)
@@ -76,30 +75,28 @@ private void AddAssemblies(params string[] assemblies)
                 AddAssembly(assembly);
         }
 
-        public bool AddAssembly(Type type)
+        public void AddAssembly(Type type)
         {
             try
             {
                 if (_references.Any(r => r.FilePath == type.Assembly.Location))
-                    return true;
+                    return;
 
                 var systemReference = MetadataReference.CreateFromFile(type.Assembly.Location);
                 _references.Add(systemReference);
             }
-            catch
+            catch (Exception e)
             {
-                return false;
+                Assert.Fail($"Could not add assembly reference for type {type.FullName}: {e.Message}");
             }
-
-            return true;
         }
 
         public void AddNetCoreDefaultReferences()
         {
             var runtimePath = Path.GetDirectoryName(typeof(object).Assembly.Location) + Path.DirectorySeparatorChar;
 
             AddAssemblies(
-                runtimePath + "System.Private.CoreLib.dll",
+                runtimePath + "mscorlib.dll",
                 runtimePath + "System.Runtime.dll",
                 runtimePath + "System.Console.dll",
                 runtimePath + "System.Text.RegularExpressions.dll",
@@ -108,14 +105,19 @@ public void AddNetCoreDefaultReferences()
                 runtimePath + "System.IO.dll",
                 runtimePath + "System.Net.Primitives.dll",
                 runtimePath + "System.Net.Http.dll",
-                runtimePath + "System.Private.Uri.dll",
                 runtimePath + "System.Reflection.dll",
                 runtimePath + "System.ComponentModel.Primitives.dll",
                 runtimePath + "System.Globalization.dll",
                 runtimePath + "System.Collections.Concurrent.dll",
                 runtimePath + "System.Collections.NonGeneric.dll",
                 runtimePath + "Microsoft.CSharp.dll",
-                runtimePath + "netstandard.dll"
+                runtimePath + "netstandard.dll",
+#if NETFRAMEWORK
+                runtimePath + "System.Core.dll"
+#else
+                runtimePath + "System.Private.CoreLib.dll",
+                runtimePath + "System.Private.Uri.dll"
+#endif
             );
         }
     }