Rework global property management in MSBuildWorkspace's ProjectBuildManager

Essentially, this change stops passing global properties around and makes the ProjectBuildManager own
a set of global properties. This improves general sanity.

src/Workspaces/Core/MSBuild/MSBuild/Build/ProjectBuildManager.cs

@@ -4,6 +4,7 @@
 using System.Collections.Generic;
 using System.Collections.Immutable;
 using System.Diagnostics;
+using System.Linq;
 using System.Threading;
 using System.Threading.Tasks;
 using System.Xml;
@@ -41,6 +42,8 @@ internal class ProjectBuildManager
             { PropertyNames.ContinueOnError, PropertyValues.ErrorAndContinue }
         }.ToImmutableDictionary();
 
+        private readonly ImmutableDictionary<string, string> _additionalGlobalProperties;
+
         private MSB.Evaluation.ProjectCollection _projectCollection;
         private MSBuildDiagnosticLogger _logger;
         private bool _started;
@@ -53,92 +56,43 @@ internal class ProjectBuildManager
             }
         }
 
-        private static MSB.Evaluation.Project FindProject(
-            string path,
-            IDictionary<string, string> globalProperties,
-            MSB.Evaluation.ProjectCollection projectCollection,
-            CancellationToken cancellationToken)
+        public ProjectBuildManager(ImmutableDictionary<string, string> additionalGlobalProperties)
         {
-            var loadedProjects = projectCollection.GetLoadedProjects(path);
-            if (loadedProjects == null || loadedProjects.Count == 0)
-            {
-                return null;
-            }
-
-            // We need to walk through all of the projects that have been previously loaded from this path and
-            // find the one that has the given set of global properties, plus the default global properties that
-            // we load every project with.
-
-            globalProperties = globalProperties ?? ImmutableDictionary<string, string>.Empty;
-            var totalGlobalProperties = projectCollection.GlobalProperties.Count + globalProperties.Count;
-
-            foreach (var loadedProject in loadedProjects)
-            {
-                cancellationToken.ThrowIfCancellationRequested();
-
-                // If this project has a different number of global properties than we expect, it's not the
-                // one we're looking for.
-                if (loadedProject.GlobalProperties.Count != totalGlobalProperties)
-                {
-                    continue;
-                }
-
-                // Since we loaded all of them, the projects in this collection should all have the default
-                // global properties (i.e. the ones in _projectCollection.GlobalProperties). So, we just need to
-                // check the extra global properties.
-
-                var found = true;
-                foreach (var (key, value) in globalProperties)
-                {
-                    // MSBuild escapes the values of a project's global properties, so we must too.
-                    var escapedValue = MSB.Evaluation.ProjectCollection.Escape(value);
-
-                    if (!loadedProject.GlobalProperties.TryGetValue(key, out var actualValue) ||
-                        !string.Equals(actualValue, escapedValue, StringComparison.Ordinal))
-                    {
-                        found = false;
-                        break;
-                    }
-                }
-
-                if (found)
-                {
-                    return loadedProject;
-                }
-            }
-
-            // We couldn't find a project with this path and the set of global properties we expect.
-            return null;
+            _additionalGlobalProperties = additionalGlobalProperties ?? ImmutableDictionary<string, string>.Empty;
         }
 
-        public async Task<(MSB.Evaluation.Project project, DiagnosticLog log)> LoadProjectAsync(
-            string path, IDictionary<string, string> globalProperties, CancellationToken cancellationToken)
+        private ImmutableDictionary<string, string> AllGlobalProperties()
+            => s_defaultGlobalProperties.AddRange(_additionalGlobalProperties);
+
+        private static async Task<(MSB.Evaluation.Project project, DiagnosticLog log)> LoadProjectAsync(
+            string path, MSB.Evaluation.ProjectCollection projectCollection, CancellationToken cancellationToken)
         {
             var log = new DiagnosticLog();
 
             try
             {
-                var projectCollection = _projectCollection ?? new MSB.Evaluation.ProjectCollection(s_defaultGlobalProperties);
+                var loadedProjects = projectCollection.GetLoadedProjects(path);
+                if (loadedProjects != null && loadedProjects.Count > 0)
+                {
+                    Debug.Assert(loadedProjects.Count == 1);
 
-                var project = FindProject(path, globalProperties, projectCollection, cancellationToken);
+                    return (loadedProjects.First(), log);
+                }
 
-                if (project == null)
+                using (var stream = FileUtilities.OpenAsyncRead(path))
+                using (var readStream = await SerializableBytes.CreateReadableStreamAsync(stream, cancellationToken).ConfigureAwait(false))
+                using (var xmlReader = XmlReader.Create(readStream, s_xmlReaderSettings))
                 {
-                    using (var stream = FileUtilities.OpenAsyncRead(path))
-                    using (var readStream = await SerializableBytes.CreateReadableStreamAsync(stream, cancellationToken).ConfigureAwait(false))
-                    using (var xmlReader = XmlReader.Create(readStream, s_xmlReaderSettings))
-                    {
-                        var xml = MSB.Construction.ProjectRootElement.Create(xmlReader, projectCollection);
+                    var xml = MSB.Construction.ProjectRootElement.Create(xmlReader, projectCollection);
 
-                        // When constructing a project from an XmlReader, MSBuild cannot determine the project file path.  Setting the
-                        // path explicitly is necessary so that the reserved properties like $(MSBuildProjectDirectory) will work.
-                        xml.FullPath = path;
+                    // When constructing a project from an XmlReader, MSBuild cannot determine the project file path.  Setting the
+                    // path explicitly is necessary so that the reserved properties like $(MSBuildProjectDirectory) will work.
+                    xml.FullPath = path;
 
-                        project = new MSB.Evaluation.Project(xml, globalProperties, toolsVersion: null, projectCollection);
-                    }
-                }
+                    var project = new MSB.Evaluation.Project(xml, globalProperties: null, toolsVersion: null, projectCollection);
 
-                return (project, log);
+                    return (project, log);
+                }
             }
             catch (Exception e)
             {
@@ -147,23 +101,49 @@ internal class ProjectBuildManager
             }
         }
 
+        public Task<(MSB.Evaluation.Project project, DiagnosticLog log)> LoadProjectAsync(
+            string path, CancellationToken cancellationToken)
+        {
+            if (_started)
+            {
+                return LoadProjectAsync(path, _projectCollection, cancellationToken);
+            }
+            else
+            {
+                var projectCollection = new MSB.Evaluation.ProjectCollection(AllGlobalProperties());
+                try
+                {
+                    return LoadProjectAsync(path, projectCollection, cancellationToken);
+                }
+                finally
+                {
+                    // unload project so collection will release global strings
+                    projectCollection.UnloadAllProjects();
+                }
+            }
+        }
+
         public async Task<string> TryGetOutputFilePathAsync(
-            string path, IDictionary<string, string> globalProperties, CancellationToken cancellationToken)
+            string path, CancellationToken cancellationToken)
         {
-            // This tries to get the project output path and retrieving the $(TargetPath) property.
+            Debug.Assert(_started);
 
-            var (project, _) = await LoadProjectAsync(path, globalProperties, cancellationToken).ConfigureAwait(false);
+            // This tries to get the project output path and retrieving the evaluated $(TargetPath) property.
+
+            var (project, _) = await LoadProjectAsync(path, cancellationToken).ConfigureAwait(false);
             return project?.GetPropertyValue(PropertyNames.TargetPath);
         }
 
-        public void Start()
+        public void Start(IDictionary<string, string> globalProperties = null)
         {
             if (_started)
             {
                 throw new InvalidOperationException();
             }
 
-            _projectCollection = new MSB.Evaluation.ProjectCollection(s_defaultGlobalProperties);
+            globalProperties = globalProperties ?? ImmutableDictionary<string, string>.Empty;
+            var allProperties = s_defaultGlobalProperties.AddRange(globalProperties);
+            _projectCollection = new MSB.Evaluation.ProjectCollection(allProperties);
 
             _logger = new MSBuildDiagnosticLogger()
             {

src/Workspaces/Core/MSBuild/MSBuild/MSBuildProjectLoader.Worker.cs

@@ -130,7 +130,7 @@ public async Task<ImmutableArray<ProjectInfo>> LoadAsync(CancellationToken cance
                 var results = ImmutableArray.CreateBuilder<ProjectInfo>();
                 var processedPaths = new HashSet<string>(PathUtilities.Comparer);
 
-                _buildManager.Start();
+                _buildManager.Start(_globalProperties);
                 try
                 {
                     foreach (var projectPath in _requestedProjectPaths)
@@ -186,7 +186,7 @@ private async Task<ImmutableArray<ProjectFileInfo>> LoadProjectFileInfosAsync(st
                     ProjectLoadOperation.Evaluate,
                     projectPath,
                     targetFramework: null,
-                    () => loader.LoadProjectFileAsync(projectPath, _globalProperties, _buildManager, cancellationToken)
+                    () => loader.LoadProjectFileAsync(projectPath, _buildManager, cancellationToken)
                 ).ConfigureAwait(false);
 
                 // If there were any failures during load, we won't be able to build the project. So, bail early with an empty project.

src/Workspaces/Core/MSBuild/MSBuild/MSBuildProjectLoader.Worker_ResolveReferences.cs

@@ -314,7 +314,7 @@ private bool IsProjectLoadable(string projectPath)
 
             private async Task<bool> VerifyUnloadableProjectOutputExistsAsync(string projectPath, ResolvedReferencesBuilder builder, CancellationToken cancellationToken)
             {
-                var outputFilePath = await _buildManager.TryGetOutputFilePathAsync(projectPath, _globalProperties, cancellationToken).ConfigureAwait(false);
+                var outputFilePath = await _buildManager.TryGetOutputFilePathAsync(projectPath, cancellationToken).ConfigureAwait(false);
                 return builder.Contains(outputFilePath)
                     && File.Exists(outputFilePath);
             }

src/Workspaces/Core/MSBuild/MSBuild/MSBuildProjectLoader.cs

@@ -27,8 +27,6 @@ public partial class MSBuildProjectLoader
         private readonly NonReentrantLock _dataGuard = new NonReentrantLock();
         private ImmutableDictionary<string, string> _properties;
 
-        internal readonly ProjectBuildManager BuildManager;
-
         internal MSBuildProjectLoader(
             Workspace workspace,
             DiagnosticReporter diagnosticReporter,
@@ -46,8 +44,6 @@ public partial class MSBuildProjectLoader
             {
                 _properties = _properties.AddRange(properties);
             }
-
-            BuildManager = new ProjectBuildManager();
         }
 
         /// <summary>
@@ -183,12 +179,14 @@ private DiagnosticReportingMode GetReportingModeForUnrecognizedProjects()
                 }
             }
 
+            var buildManager = new ProjectBuildManager(_properties);
+
             var worker = new Worker(
                 _workspace,
                 _diagnosticReporter,
                 _pathResolver,
                 _projectFileLoaderRegistry,
-                BuildManager,
+                buildManager,
                 projectPaths.ToImmutable(),
                 baseDirectory: Path.GetDirectoryName(absoluteSolutionPath),
                 _properties,
@@ -237,12 +235,14 @@ private DiagnosticReportingMode GetReportingModeForUnrecognizedProjects()
                 onPathFailure: reportingMode,
                 onLoaderFailure: reportingMode);
 
+            var buildManager = new ProjectBuildManager(_properties);
+
             var worker = new Worker(
                 _workspace,
                 _diagnosticReporter,
                 _pathResolver,
                 _projectFileLoaderRegistry,
-                BuildManager,
+                buildManager,
                 requestedProjectPaths: ImmutableArray.Create(projectFilePath),
                 baseDirectory: Directory.GetCurrentDirectory(),
                 globalProperties: _properties,

src/Workspaces/Core/MSBuild/MSBuild/MSBuildWorkspace.cs

@@ -13,6 +13,7 @@
 using Microsoft.CodeAnalysis.Diagnostics;
 using Microsoft.CodeAnalysis.Host;
 using Microsoft.CodeAnalysis.Host.Mef;
+using Microsoft.CodeAnalysis.MSBuild.Build;
 using Microsoft.CodeAnalysis.Shared.Utilities;
 using Microsoft.CodeAnalysis.Text;
 using Roslyn.Utilities;
@@ -281,11 +282,12 @@ protected override void ApplyProjectChanges(ProjectChanges projectChanges)
                 if (this.HasProjectFileChanges(projectChanges))
                 {
                     var projectPath = project.FilePath;
-                    if (_projectFileLoaderRegistry.TryGetLoaderFromProjectPath(projectPath, out var loader))
+                    if (_projectFileLoaderRegistry.TryGetLoaderFromProjectPath(projectPath, out var fileLoader))
                     {
                         try
                         {
-                            _applyChangesProjectFile = loader.LoadProjectFileAsync(projectPath, _loader.Properties, _loader.BuildManager, CancellationToken.None).Result;
+                            var buildManager = new ProjectBuildManager(_loader.Properties);
+                            _applyChangesProjectFile = fileLoader.LoadProjectFileAsync(projectPath, buildManager, CancellationToken.None).Result;
                         }
                         catch (IOException exception)
                         {

src/Workspaces/Core/MSBuild/MSBuild/ProjectFile/IProjectFileLoader.cs

 // Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.
 
-using System.Collections.Generic;
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.CodeAnalysis.Host;
@@ -13,7 +12,6 @@ internal interface IProjectFileLoader : ILanguageService
         string Language { get; }
         Task<IProjectFile> LoadProjectFileAsync(
             string path,
-            IDictionary<string, string> globalProperties,
             ProjectBuildManager buildManager,
             CancellationToken cancellationToken);
     }

src/Workspaces/Core/MSBuild/MSBuild/ProjectFile/ProjectFileLoader.cs

 // Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.
 
 using System;
-using System.Collections.Generic;
 using System.Linq;
 using System.Threading;
 using System.Threading.Tasks;
@@ -19,7 +18,7 @@ internal abstract class ProjectFileLoader : IProjectFileLoader
 
         protected abstract ProjectFile CreateProjectFile(MSB.Evaluation.Project project, ProjectBuildManager buildManager, DiagnosticLog log);
 
-        public async Task<IProjectFile> LoadProjectFileAsync(string path, IDictionary<string, string> globalProperties, ProjectBuildManager buildManager, CancellationToken cancellationToken)
+        public async Task<IProjectFile> LoadProjectFileAsync(string path, ProjectBuildManager buildManager, CancellationToken cancellationToken)
         {
             if (path == null)
             {
@@ -27,7 +26,7 @@ public async Task<IProjectFile> LoadProjectFileAsync(string path, IDictionary<st
             }
 
             // load project file async
-            var (project, log) = await buildManager.LoadProjectAsync(path, globalProperties, cancellationToken).ConfigureAwait(false);
+            var (project, log) = await buildManager.LoadProjectAsync(path, cancellationToken).ConfigureAwait(false);
 
             return this.CreateProjectFile(project, buildManager, log);
         }

src/Workspaces/MSBuildTest/MSBuildWorkspaceTests.cs

@@ -3517,7 +3517,7 @@ public async Task TestOpenProject_CommandLineArgsHaveNoErrors()
 
                 var projectFilePath = GetSolutionFileName(@"CSharpProject\CSharpProject.csproj");
 
-                var buildManager = new ProjectBuildManager();
+                var buildManager = new ProjectBuildManager(ImmutableDictionary<string, string>.Empty);
                 buildManager.Start();
 
                 var projectFile = await loader.LoadProjectFileAsync(projectFilePath, buildManager, CancellationToken.None);