Fix TaskRegistry mutability

JanKrivanek · JanKrivanek · commit 50eebaea85c2 · 2023-07-13T18:23:19.000+02:00
diff --git a/src/Build.UnitTests/BackEnd/TaskRegistry_Tests.cs b/src/Build.UnitTests/BackEnd/TaskRegistry_Tests.cs
@@ -2182,18 +2182,15 @@ internal TaskRegistry CreateTaskRegistryAndRegisterTasks(List<ProjectUsingTaskEl
                 ? new TaskRegistry(toolset, ProjectCollection.GlobalProjectCollection.ProjectRootElementCache)
                 : new TaskRegistry(ProjectCollection.GlobalProjectCollection.ProjectRootElementCache);
 
-            foreach (ProjectUsingTaskElement projectUsingTaskElement in usingTaskElements)
-            {
-                TaskRegistry.RegisterTasksFromUsingTaskElement(
-                        _loggingService,
-                        _loggerContext,
-                        Directory.GetCurrentDirectory(),
-                        projectUsingTaskElement,
-                        registry,
-                        RegistryExpander,
-                        ExpanderOptions.ExpandPropertiesAndItems,
-                        FileSystems.Default);
-            }
+            string currentDir = Directory.GetCurrentDirectory();
+            TaskRegistry.InitializeTaskRegistryFromUsingTaskElements(
+                _loggingService,
+                _loggerContext,
+                usingTaskElements.Select(el => (el, currentDir)),
+                registry,
+                RegistryExpander,
+                ExpanderOptions.ExpandPropertiesAndItems,
+                FileSystems.Default);
 
             return registry;
         }
diff --git a/src/Build/Definition/ProjectImportPathMatch.cs b/src/Build/Definition/ProjectImportPathMatch.cs
@@ -11,6 +11,9 @@ 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
     {
@@ -19,14 +22,19 @@ internal class ProjectImportPathMatch : ITranslatable
         /// </summary>
         public static readonly ProjectImportPathMatch None = new ProjectImportPathMatch(string.Empty, new List<string>());
 
+        // 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;
+
         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 List<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>
diff --git a/src/Build/Definition/Toolset.cs b/src/Build/Definition/Toolset.cs
@@ -8,7 +8,6 @@
 using System.IO;
 using System.Linq;
 using System.Xml;
-
 using Microsoft.Build.BackEnd;
 using Microsoft.Build.Collections;
 using Microsoft.Build.Construction;
@@ -190,11 +189,6 @@ public class Toolset : ITranslatable
         /// </summary>
         private Expander<ProjectPropertyInstance, ProjectItemInstance> _expander;
 
-        /// <summary>
-        /// Bag of properties for the expander to expand the properties and items in the using tasks files
-        /// </summary>
-        private PropertyDictionary<ProjectPropertyInstance> _propertyBag;
-
         /// <summary>
         /// SubToolsets that map to this toolset.
         /// </summary>
@@ -901,79 +895,79 @@ private void RegisterDefaultTasks(ILoggingService loggingServices, BuildEventCon
         /// </summary>
         private void InitializeProperties(ILoggingService loggingServices, BuildEventContext buildEventContext)
         {
+            if (_expander != null)
+            {
+                return;
+            }
+
             try
             {
-                if (_propertyBag == null)
-                {
-                    List<ProjectPropertyInstance> reservedProperties = new List<ProjectPropertyInstance>();
+                
+                List<ProjectPropertyInstance> reservedProperties = new List<ProjectPropertyInstance>();
 
-                    reservedProperties.Add(ProjectPropertyInstance.Create(ReservedPropertyNames.binPath, EscapingUtilities.Escape(ToolsPath), mayBeReserved: true));
-                    reservedProperties.Add(ProjectPropertyInstance.Create(ReservedPropertyNames.toolsVersion, ToolsVersion, mayBeReserved: true));
+                reservedProperties.Add(ProjectPropertyInstance.Create(ReservedPropertyNames.binPath, EscapingUtilities.Escape(ToolsPath), mayBeReserved: true));
+                reservedProperties.Add(ProjectPropertyInstance.Create(ReservedPropertyNames.toolsVersion, ToolsVersion, mayBeReserved: true));
 
-                    reservedProperties.Add(ProjectPropertyInstance.Create(ReservedPropertyNames.toolsPath, EscapingUtilities.Escape(ToolsPath), mayBeReserved: true));
-                    reservedProperties.Add(ProjectPropertyInstance.Create(ReservedPropertyNames.assemblyVersion, Constants.AssemblyVersion, mayBeReserved: true));
-                    reservedProperties.Add(ProjectPropertyInstance.Create(ReservedPropertyNames.version, MSBuildAssemblyFileVersion.Instance.MajorMinorBuild, mayBeReserved: true));
+                reservedProperties.Add(ProjectPropertyInstance.Create(ReservedPropertyNames.toolsPath, EscapingUtilities.Escape(ToolsPath), mayBeReserved: true));
+                reservedProperties.Add(ProjectPropertyInstance.Create(ReservedPropertyNames.assemblyVersion, Constants.AssemblyVersion, mayBeReserved: true));
+                reservedProperties.Add(ProjectPropertyInstance.Create(ReservedPropertyNames.version, MSBuildAssemblyFileVersion.Instance.MajorMinorBuild, mayBeReserved: true));
 
-                    reservedProperties.Add(ProjectPropertyInstance.Create(ReservedPropertyNames.msbuildRuntimeType,
+                reservedProperties.Add(ProjectPropertyInstance.Create(ReservedPropertyNames.msbuildRuntimeType,
 #if RUNTIME_TYPE_NETCORE
-                        Traits.Instance.ForceEvaluateAsFullFramework ? "Full" : "Core",
+                    Traits.Instance.ForceEvaluateAsFullFramework ? "Full" : "Core",
 #elif MONO
-                        NativeMethodsShared.IsMono ? "Mono" : "Full");
+                    NativeMethodsShared.IsMono ? "Mono" : "Full");
 #else
-                        "Full",
+                    "Full",
 #endif
-                        mayBeReserved: true));
+                    mayBeReserved: true));
 
 
-                    // Add one for the subtoolset version property -- it may or may not be set depending on whether it has already been set by the
-                    // environment or global properties, but it's better to create a dictionary that's one too big than one that's one too small.
-                    int count = _environmentProperties.Count + reservedProperties.Count + Properties.Values.Count + _globalProperties.Count + 1;
+                // Add one for the subtoolset version property -- it may or may not be set depending on whether it has already been set by the
+                // environment or global properties, but it's better to create a dictionary that's one too big than one that's one too small.
+                int count = _environmentProperties.Count + reservedProperties.Count + Properties.Values.Count + _globalProperties.Count + 1;
 
-                    // GenerateSubToolsetVersion checks the environment and global properties, so it's safe to go ahead and gather the
-                    // subtoolset properties here without fearing that we'll have somehow come up with the wrong subtoolset version.
-                    string subToolsetVersion = this.GenerateSubToolsetVersion();
-                    SubToolset subToolset;
-                    ICollection<ProjectPropertyInstance> subToolsetProperties = null;
+                // GenerateSubToolsetVersion checks the environment and global properties, so it's safe to go ahead and gather the
+                // subtoolset properties here without fearing that we'll have somehow come up with the wrong subtoolset version.
+                string subToolsetVersion = this.GenerateSubToolsetVersion();
+                SubToolset subToolset;
+                ICollection<ProjectPropertyInstance> subToolsetProperties = null;
 
-                    if (subToolsetVersion != null)
+                if (subToolsetVersion != null)
+                {
+                    if (SubToolsets.TryGetValue(subToolsetVersion, out subToolset))
                     {
-                        if (SubToolsets.TryGetValue(subToolsetVersion, out subToolset))
-                        {
-                            subToolsetProperties = subToolset.Properties.Values;
-                            count += subToolsetProperties.Count;
-                        }
+                        subToolsetProperties = subToolset.Properties.Values;
+                        count += subToolsetProperties.Count;
                     }
+                }
 
-                    _propertyBag = new PropertyDictionary<ProjectPropertyInstance>(count);
-
-                    // Should be imported in the same order as in the evaluator:
-                    // - Environment
-                    // - Toolset
-                    // - Subtoolset (if any)
-                    // - Global
-                    _propertyBag.ImportProperties(_environmentProperties);
+                PropertyDictionary<ProjectPropertyInstance> propertyBag = new PropertyDictionary<ProjectPropertyInstance>(count);
 
-                    _propertyBag.ImportProperties(reservedProperties);
+                // Should be imported in the same order as in the evaluator:
+                // - Environment
+                // - Toolset
+                // - Subtoolset (if any)
+                // - Global
+                propertyBag.ImportProperties(_environmentProperties);
 
-                    _propertyBag.ImportProperties(Properties.Values);
+                propertyBag.ImportProperties(reservedProperties);
 
-                    if (subToolsetVersion != null)
-                    {
-                        _propertyBag.Set(ProjectPropertyInstance.Create(Constants.SubToolsetVersionPropertyName, subToolsetVersion));
-                    }
-
-                    if (subToolsetProperties != null)
-                    {
-                        _propertyBag.ImportProperties(subToolsetProperties);
-                    }
+                propertyBag.ImportProperties(Properties.Values);
 
-                    _propertyBag.ImportProperties(_globalProperties);
+                if (subToolsetVersion != null)
+                {
+                    propertyBag.Set(ProjectPropertyInstance.Create(Constants.SubToolsetVersionPropertyName, subToolsetVersion));
                 }
 
-                if (_expander == null)
+                if (subToolsetProperties != null)
                 {
-                    _expander = new Expander<ProjectPropertyInstance, ProjectItemInstance>(_propertyBag, FileSystems.Default);
+                    propertyBag.ImportProperties(subToolsetProperties);
                 }
+
+                propertyBag.ImportProperties(_globalProperties);
+
+                _expander = new Expander<ProjectPropertyInstance, ProjectItemInstance>(propertyBag, FileSystems.Default);
             }
             catch (Exception e) when (ExceptionHandling.IsIoRelatedException(e))
             {
@@ -1044,10 +1038,35 @@ private void RegisterOverrideTasks(ILoggingService loggingServices, BuildEventCo
         /// </summary>
         private void LoadAndRegisterFromTasksFile(string[] defaultTaskFiles, ILoggingService loggingServices, BuildEventContext buildEventContext, string taskFileError, ProjectRootElementCacheBase projectRootElementCache, TaskRegistry registry)
         {
-            foreach (string defaultTasksFile in defaultTaskFiles)
+            string currentTasksFile = null;
+            try
             {
-                try
+                TaskRegistry.InitializeTaskRegistryFromUsingTaskElements<ProjectPropertyInstance, ProjectItemInstance>(
+                    loggingServices,
+                    buildEventContext,
+                    EnumerateTasksRegistrations(),
+                    registry,
+                    _expander,
+                    ExpanderOptions.ExpandProperties,
+                    FileSystems.Default);
+            }
+            catch (XmlException e)
+            {
+                // handle XML errors in the default tasks file
+                ProjectFileErrorUtilities.ThrowInvalidProjectFile(new BuildEventFileInfo(currentTasksFile, e),
+                    taskFileError, e.Message);
+            }
+            catch (Exception e) when (ExceptionHandling.IsIoRelatedException(e))
+            {
+                loggingServices.LogError(buildEventContext, new BuildEventFileInfo(currentTasksFile),
+                    taskFileError, e.Message);
+            }
+
+            IEnumerable<(ProjectUsingTaskElement projectUsingTaskXml, string directoryOfImportingFile)> EnumerateTasksRegistrations()
+            {
+                foreach (string defaultTasksFile in defaultTaskFiles)
                 {
+                    currentTasksFile = defaultTasksFile;
                     // Important to keep the following line since unit tests use the delegate.
                     ProjectRootElement projectRootElement;
                     if (_loadXmlFromPath != null)
@@ -1074,27 +1093,9 @@ private void LoadAndRegisterFromTasksFile(string[] defaultTaskFiles, ILoggingSer
                                 elementXml.XmlElement.Name);
                         }
 
-                        TaskRegistry.RegisterTasksFromUsingTaskElement<ProjectPropertyInstance, ProjectItemInstance>(
-                            loggingServices,
-                            buildEventContext,
-                            Path.GetDirectoryName(defaultTasksFile),
-                            usingTask,
-                            registry,
-                            _expander,
-                            ExpanderOptions.ExpandProperties,
-                            FileSystems.Default);
+                        yield return (usingTask, Path.GetDirectoryName(defaultTasksFile));
                     }
                 }
-                catch (XmlException e)
-                {
-                    // handle XML errors in the default tasks file
-                    ProjectFileErrorUtilities.ThrowInvalidProjectFile(new BuildEventFileInfo(defaultTasksFile, e), taskFileError, e.Message);
-                }
-                catch (Exception e) when (ExceptionHandling.IsIoRelatedException(e))
-                {
-                    loggingServices.LogError(buildEventContext, new BuildEventFileInfo(defaultTasksFile), taskFileError, e.Message);
-                    break;
-                }
             }
         }
     }
diff --git a/src/Build/Evaluation/Evaluator.cs b/src/Build/Evaluation/Evaluator.cs
@@ -711,10 +711,15 @@ private void Evaluate()
                 MSBuildEventSource.Log.EvaluatePass4Start(projectFile);
                 using (_evaluationProfiler.TrackPass(EvaluationPass.UsingTasks))
                 {
-                    foreach (var entry in _usingTaskElements)
-                    {
-                        EvaluateUsingTaskElement(entry.Key, entry.Value);
-                    }
+                    // Evaluate the usingtask and add the result into the data passed in
+                    TaskRegistry.InitializeTaskRegistryFromUsingTaskElements<P, I>(
+                        _evaluationLoggingContext.LoggingService,
+                        _evaluationLoggingContext.BuildEventContext,
+                        _usingTaskElements.Select(p => (p.Value, p.Key)),
+                        _data.TaskRegistry,
+                        _expander,
+                        ExpanderOptions.ExpandPropertiesAndItems,
+                        _evaluationContext.FileSystem);
                 }
 
                 // If there was no DefaultTargets attribute found in the depth first pass,
@@ -1015,22 +1020,6 @@ private void EvaluateItemGroupElement(ProjectItemGroupElement itemGroupElement,
             }
         }
 
-        /// <summary>
-        /// Evaluate the usingtask and add the result into the data passed in
-        /// </summary>
-        private void EvaluateUsingTaskElement(string directoryOfImportingFile, ProjectUsingTaskElement projectUsingTaskElement)
-        {
-            TaskRegistry.RegisterTasksFromUsingTaskElement<P, I>(
-                _evaluationLoggingContext.LoggingService,
-                _evaluationLoggingContext.BuildEventContext,
-                directoryOfImportingFile,
-                projectUsingTaskElement,
-                _data.TaskRegistry,
-                _expander,
-                ExpanderOptions.ExpandPropertiesAndItems,
-                _evaluationContext.FileSystem);
-        }
-
         /// <summary>
         /// Retrieve the matching ProjectTargetInstance from the cache and add it to the provided collection.
         /// If it is not cached already, read it and cache it.
diff --git a/src/Build/Instance/ProjectInstance.cs b/src/Build/Instance/ProjectInstance.cs
@@ -561,7 +561,9 @@ internal ProjectInstance(Evaluation.Project.Data data, string directory, string
             this.CreateTargetsSnapshot(data.Targets, data.DefaultTargets, data.InitialTargets, data.BeforeTargets, data.AfterTargets);
             this.CreateImportsSnapshot(data.ImportClosure, data.ImportClosureWithDuplicates);
 
-            this.Toolset = data.Toolset; // UNDONE: This isn't immutable, should be cloned or made immutable; it currently has a pointer to project collection
+            // Toolset and task registry are logically immutable after creation, and shareable by project instances
+            //  with same evaluation (global/local properties) - which is guaranteed here (the passed in data is recreated on evaluation if needed)
+            this.Toolset = data.Toolset;
             this.SubToolsetVersion = data.SubToolsetVersion;
             this.TaskRegistry = data.TaskRegistry;
 
@@ -641,10 +643,8 @@ private ProjectInstance(ProjectInstance that, bool isImmutable, RequestedProject
                     ProjectItemDefinitionInstance>)this).AfterTargets = CreateCloneDictionary(
                     ((IEvaluatorData<ProjectPropertyInstance, ProjectItemInstance, ProjectMetadataInstance,
                         ProjectItemDefinitionInstance>)that).AfterTargets, StringComparer.OrdinalIgnoreCase);
-                this.TaskRegistry =
-                    that.TaskRegistry; // UNDONE: This isn't immutable, should be cloned or made immutable; it currently has a pointer to project collection
-
-                // These are immutable so we don't need to clone them:
+                // These are immutable (or logically immutable after creation) so we don't need to clone them:
+                this.TaskRegistry = that.TaskRegistry;
                 this.Toolset = that.Toolset;
                 this.SubToolsetVersion = that.SubToolsetVersion;
                 _targets = that._targets;
diff --git a/src/Build/Instance/TaskRegistry.cs b/src/Build/Instance/TaskRegistry.cs
