Deep clone TaskRegistry and Toolset

src/Build.UnitTests/Definition/Toolset_Tests.cs

@@ -6,13 +6,15 @@
 using System.Linq;
 using Microsoft.Build.BackEnd;
 using Microsoft.Build.Collections;
+using Microsoft.Build.Engine.UnitTests.TestComparers;
 using Microsoft.Build.Evaluation;
 using Microsoft.Build.Execution;
 using Microsoft.Build.Internal;
 using Microsoft.Build.Shared;
 using Microsoft.Build.UnitTests.BackEnd;
 using Xunit;
 using Xunit.NetCore.Extensions;
+using static Microsoft.Build.Engine.UnitTests.TestComparers.TaskRegistryComparers;
 
 #nullable disable
 
@@ -123,45 +125,7 @@ public void ValidateToolsetTranslation()
             ((ITranslatable)t).Translate(TranslationHelpers.GetWriteTranslator());
             Toolset t2 = Toolset.FactoryForDeserialization(TranslationHelpers.GetReadTranslator());
 
-            Assert.Equal(t.ToolsVersion, t2.ToolsVersion);
-            Assert.Equal(t.ToolsPath, t2.ToolsPath);
-            Assert.Equal(t.OverrideTasksPath, t2.OverrideTasksPath);
-            Assert.Equal(t.Properties.Count, t2.Properties.Count);
-
-            foreach (string key in t.Properties.Values.Select(p => p.Name))
-            {
-                Assert.Equal(t.Properties[key].Name, t2.Properties[key].Name);
-                Assert.Equal(t.Properties[key].EvaluatedValue, t2.Properties[key].EvaluatedValue);
-            }
-
-            Assert.Equal(t.SubToolsets.Count, t2.SubToolsets.Count);
-
-            foreach (string key in t.SubToolsets.Keys)
-            {
-                SubToolset subToolset1 = t.SubToolsets[key];
-                SubToolset subToolset2 = null;
-
-                if (t2.SubToolsets.TryGetValue(key, out subToolset2))
-                {
-                    Assert.Equal(subToolset1.SubToolsetVersion, subToolset2.SubToolsetVersion);
-                    Assert.Equal(subToolset1.Properties.Count, subToolset2.Properties.Count);
-
-                    foreach (string subToolsetPropertyKey in subToolset1.Properties.Values.Select(p => p.Name))
-                    {
-                        Assert.Equal(subToolset1.Properties[subToolsetPropertyKey].Name, subToolset2.Properties[subToolsetPropertyKey].Name);
-                        Assert.Equal(subToolset1.Properties[subToolsetPropertyKey].EvaluatedValue, subToolset2.Properties[subToolsetPropertyKey].EvaluatedValue);
-                    }
-                }
-                else
-                {
-                    Assert.True(false, $"Sub-toolset {key} was lost in translation.");
-                }
-            }
-
-            Assert.Equal(t.DefaultOverrideToolsVersion, t2.DefaultOverrideToolsVersion);
-
-            Assert.NotNull(t2.ImportPropertySearchPathsTable);
-            Assert.Single(t2.ImportPropertySearchPathsTable);
+            Assert.Equal(t, t2, new ToolsetComparer());
             Assert.Equal(@"c:\foo", t2.ImportPropertySearchPathsTable["MSBuildExtensionsPath"].SearchPaths[0]);
         }
 

src/Build.UnitTests/Instance/ProjectInstance_Internal_Tests.cs

@@ -9,6 +9,7 @@
 using Microsoft.Build.BackEnd;
 using Microsoft.Build.Construction;
 using Microsoft.Build.Definition;
+using Microsoft.Build.Engine.UnitTests.TestComparers;
 using Microsoft.Build.Evaluation;
 using Microsoft.Build.Execution;
 using Microsoft.Build.Framework;
@@ -480,8 +481,8 @@ public void CloneTaskRegistry()
             ProjectInstance first = GetSampleProjectInstance();
             ProjectInstance second = first.DeepCopy();
 
-            // Task registry object should be immutable
-            first.TaskRegistry.ShouldBeSameAs(second.TaskRegistry);
+            // Task registry object should be cloned
+            Assert.Equal(first.TaskRegistry, second.TaskRegistry, new TaskRegistryComparers.TaskRegistryComparer());
         }
 
         /// <summary>
@@ -530,7 +531,7 @@ public void CloneToolsVersion()
             ProjectInstance first = GetSampleProjectInstance();
             ProjectInstance second = first.DeepCopy();
 
-            second.Toolset.ShouldBe(first.Toolset);
+            Assert.Equal(first.Toolset, second.Toolset, new TaskRegistryComparers.ToolsetComparer());
         }
 
         /// <summary>

src/Build.UnitTests/TestComparers/TaskRegistryComparers.cs

@@ -3,6 +3,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Linq;
 using Microsoft.Build.Evaluation;
 using Microsoft.Build.Execution;
 using Microsoft.Build.Framework;
@@ -137,6 +138,26 @@ public bool Equals(Toolset x, Toolset y)
                         Assert.Equal(xp.Value.EvaluatedValue, yp.Value.EvaluatedValue);
                     });
 
+                Helpers.AssertDictionariesEqual(
+                    x.SubToolsets,
+                    y.SubToolsets,
+                    (xp, yp) =>
+                    {
+                        Assert.Equal(xp.Key, yp.Key);
+                        SubToolset subToolset1 = xp.Value;
+                        SubToolset subToolset2 = yp.Value;
+                        Assert.Equal(subToolset1.SubToolsetVersion, subToolset2.SubToolsetVersion);
+                        Assert.Equal(subToolset1.Properties.Count, subToolset2.Properties.Count);
+
+                        foreach (string subToolsetPropertyKey in subToolset1.Properties.Values.Select(p => p.Name))
+                        {
+                            Assert.Equal(subToolset1.Properties[subToolsetPropertyKey].Name, subToolset2.Properties[subToolsetPropertyKey].Name);
+                            Assert.Equal(subToolset1.Properties[subToolsetPropertyKey].EvaluatedValue, subToolset2.Properties[subToolsetPropertyKey].EvaluatedValue);
+                        }
+                    });
+
+                Assert.Equal(x.DefaultOverrideToolsVersion, y.DefaultOverrideToolsVersion);
+
                 return true;
             }
 

src/Build/BackEnd/Components/Communications/CloningExtensions.cs

@@ -0,0 +1,47 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using Microsoft.Build.Collections;
+using Microsoft.Build.Execution;
+
+namespace Microsoft.Build.BackEnd;
+
+internal static class CloningExtensions
+{
+    public static PropertyDictionary<ProjectPropertyInstance>? DeepClone(
+        this PropertyDictionary<ProjectPropertyInstance>? properties)
+        => properties == null ? null : new(properties.Select<ProjectPropertyInstance, ProjectPropertyInstance>(p => p.DeepClone()));
+
+    public static Dictionary<TKey, TValue>? DeepClone<TKey, TValue>(
+        this IDictionary<TKey, TValue>? dictionary,
+        IEqualityComparer<TKey> comparer) where TKey : notnull
+        => dictionary.DeepClone(null, null, comparer);
+
+    public static Dictionary<TKey, TValue>? DeepClone<TKey, TValue>(
+        this IDictionary<TKey, TValue>? dictionary,
+        Func<TValue, TValue> valueClone,
+        IEqualityComparer<TKey> comparer) where TKey : notnull
+        => dictionary.DeepClone(null, valueClone, comparer);
+
+    public static Dictionary<TKey, TValue>? DeepClone<TKey, TValue>(
+        this IDictionary<TKey, TValue>? dictionary,
+        Func<TKey, TKey> keyClone,
+        IEqualityComparer<TKey> comparer) where TKey : notnull
+        => dictionary.DeepClone(keyClone, null, comparer);
+
+    public static Dictionary<TKey, TValue>? DeepClone<TKey, TValue>(
+        this IDictionary<TKey, TValue>? dictionary,
+        Func<TKey, TKey>? keyClone,
+        Func<TValue, TValue>? valueClone,
+        IEqualityComparer<TKey> comparer) where TKey : notnull
+        => dictionary?.ToDictionary(
+        p => (keyClone ?? Identity)(p.Key),
+        p => (valueClone ?? Identity)(p.Value),
+        comparer);
+
+    private static T Identity<T>(T value) => value;
+}
+

src/Build/Definition/ProjectImportPathMatch.cs

@@ -11,9 +11,17 @@ namespace Microsoft.Build.Evaluation
 {
     /// <summary>
     /// Class representing a reference to a project import path with property fall-back
+    /// This class is immutable.
+    /// If mutability would be needed in the future, it should be implemented via copy-on-write or
+    ///  a DeepClone would need to be added (and called from DeepClone methods of owning types)
     /// </summary>
     internal class ProjectImportPathMatch : ITranslatable
     {
+        // Those are effectively readonly and should stay so. Cannot be marked readonly due to ITranslatable
+        private string _propertyName;
+        private string _msBuildPropertyFormat;
+        private List<string> _searchPaths;
+
         /// <summary>
         /// ProjectImportPathMatch instance representing no fall-back
         /// </summary>
@@ -24,9 +32,9 @@ internal ProjectImportPathMatch(string propertyName, List<string> searchPaths)
             ErrorUtilities.VerifyThrowArgumentNull(propertyName, nameof(propertyName));
             ErrorUtilities.VerifyThrowArgumentNull(searchPaths, nameof(searchPaths));
 
-            PropertyName = propertyName;
-            SearchPaths = searchPaths;
-            MsBuildPropertyFormat = $"$({PropertyName})";
+            _propertyName = propertyName;
+            _searchPaths = searchPaths;
+            _msBuildPropertyFormat = $"$({PropertyName})";
         }
 
         public ProjectImportPathMatch(ITranslator translator)
@@ -37,23 +45,23 @@ public ProjectImportPathMatch(ITranslator translator)
         /// <summary>
         /// String representation of the property reference - eg. "MSBuildExtensionsPath32"
         /// </summary>
-        public string PropertyName;
+        public string PropertyName => _propertyName;
 
         /// <summary>
         /// Returns the corresponding property name - eg. "$(MSBuildExtensionsPath32)"
         /// </summary>
-        public string MsBuildPropertyFormat;
+        public string MsBuildPropertyFormat => _msBuildPropertyFormat;
 
         /// <summary>
         /// Enumeration of the search paths for the property.
         /// </summary>
-        public List<string> SearchPaths;
+        public IReadOnlyList<string> SearchPaths => _searchPaths;
 
         public void Translate(ITranslator translator)
         {
-            translator.Translate(ref PropertyName);
-            translator.Translate(ref MsBuildPropertyFormat);
-            translator.Translate(ref SearchPaths);
+            translator.Translate(ref _propertyName);
+            translator.Translate(ref _msBuildPropertyFormat);
+            translator.Translate(ref _searchPaths);
         }
 
         /// <summary>

src/Build/Definition/SubToolset.cs

@@ -50,6 +50,9 @@ private SubToolset(ITranslator translator)
             ((ITranslatable)this).Translate(translator);
         }
 
+        internal SubToolset DeepClone()
+            => new SubToolset(_subToolsetVersion, _properties?.DeepClone());
+
         /// <summary>
         /// VisualStudioVersion that corresponds to this subtoolset
         /// </summary>