From e1e45211e3b2b022d81d894f0b55c24eca3ea902 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Thu, 22 Aug 2024 09:36:06 -0700 Subject: [PATCH 1/4] Make values more strongly typed --- .../Workspace/ProjectSystem/ProjectSystemProject.cs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs b/src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs index 411e8ed26fd8f..b2982fd5fbe2e 100644 --- a/src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs +++ b/src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs @@ -38,9 +38,10 @@ internal sealed partial class ProjectSystemProject /// /// A semaphore taken for all mutation of any mutable field in this type. /// - /// This is, for now, intentionally pessimistic. There are no doubt ways that we could allow more to run in parallel, - /// but the current tradeoff is for simplicity of code and "obvious correctness" than something that is subtle, fast, and wrong. - private readonly SemaphoreSlim _gate = new SemaphoreSlim(initialCount: 1); + /// This is, for now, intentionally pessimistic. There are no doubt ways that we could allow more to run in + /// parallel, but the current tradeoff is for simplicity of code and "obvious correctness" than something that is + /// subtle, fast, and wrong. + private readonly SemaphoreSlim _gate = new(initialCount: 1); /// /// The number of active batch scopes. If this is zero, we are not batching, non-zero means we are batching. @@ -52,13 +53,13 @@ internal sealed partial class ProjectSystemProject private readonly List _projectReferencesAddedInBatch = []; private readonly List _projectReferencesRemovedInBatch = []; - private readonly Dictionary _analyzerPathsToAnalyzers = []; - private readonly List _analyzersAddedInBatch = []; + private readonly Dictionary _analyzerPathsToAnalyzers = []; + private readonly List _analyzersAddedInBatch = []; /// /// The list of s that will be removed in this batch. /// - private readonly List _analyzersRemovedInBatch = []; + private readonly List _analyzersRemovedInBatch = []; private readonly List> _projectPropertyModificationsInBatch = []; From 9a038f70cdd1d2a85b099ddc5631383e84be3239 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Thu, 22 Aug 2024 09:41:59 -0700 Subject: [PATCH 2/4] Subscribe --- .../ProjectSystem/ProjectSystemProject.cs | 17 +++++++++++++---- ...ctSystemProjectFactory.ProjectUpdateState.cs | 7 +++++++ 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs b/src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs index b2982fd5fbe2e..ca7333b2c96ee 100644 --- a/src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs +++ b/src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs @@ -654,12 +654,21 @@ await _projectSystemProjectFactory.ApplyBatchChangeToWorkspaceMaybeAsync(useAsyn newSolution: solutionChanges.Solution.RemoveProjectReference(Id, projectReference)); } + // Analyzer reference removing... + { + projectUpdateState = projectUpdateState.WithIncrementalAnalyzerReferencesRemoved(_analyzersRemovedInBatch); + + foreach (var analyzerReference in _analyzersRemovedInBatch) + solutionChanges.UpdateSolutionForProjectAction(Id, solutionChanges.Solution.RemoveAnalyzerReference(Id, analyzerReference)); + } + // Analyzer reference adding... - solutionChanges.UpdateSolutionForProjectAction(Id, solutionChanges.Solution.AddAnalyzerReferences(Id, _analyzersAddedInBatch)); + { + projectUpdateState = projectUpdateState.WithIncrementalAnalyzerReferencesAdded(_analyzersAddedInBatch); - // Analyzer reference removing... - foreach (var analyzerReference in _analyzersRemovedInBatch) - solutionChanges.UpdateSolutionForProjectAction(Id, solutionChanges.Solution.RemoveAnalyzerReference(Id, analyzerReference)); + solutionChanges.UpdateSolutionForProjectAction( + Id, solutionChanges.Solution.AddAnalyzerReferences(Id, projectUpdateState.AddedAnalyzerReferences)); + } // Other property modifications... foreach (var propertyModification in _projectPropertyModificationsInBatch) diff --git a/src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProjectFactory.ProjectUpdateState.cs b/src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProjectFactory.ProjectUpdateState.cs index f5f72dad55794..f39c09b89c689 100644 --- a/src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProjectFactory.ProjectUpdateState.cs +++ b/src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProjectFactory.ProjectUpdateState.cs @@ -3,6 +3,7 @@ // See the LICENSE file in the project root for more information. using System; +using System.Collections.Generic; using System.Collections.Immutable; using Microsoft.CodeAnalysis.Diagnostics; @@ -123,9 +124,15 @@ public ProjectUpdateState WithIncrementalMetadataReferenceAdded(PortableExecutab public ProjectUpdateState WithIncrementalAnalyzerReferenceRemoved(AnalyzerFileReference reference) => this with { RemovedAnalyzerReferences = RemovedAnalyzerReferences.Add(reference) }; + public ProjectUpdateState WithIncrementalAnalyzerReferencesRemoved(List references) + => this with { RemovedAnalyzerReferences = RemovedAnalyzerReferences.AddRange(references) }; + public ProjectUpdateState WithIncrementalAnalyzerReferenceAdded(AnalyzerFileReference reference) => this with { AddedAnalyzerReferences = AddedAnalyzerReferences.Add(reference) }; + public ProjectUpdateState WithIncrementalAnalyzerReferencesAdded(List references) + => this with { AddedAnalyzerReferences = AddedAnalyzerReferences.AddRange(references) }; + /// /// Returns a new instance with any incremental state that should not be saved between updates cleared. /// From 7f9c67551e299d1bf7049c52a78fdc618235feb8 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Thu, 22 Aug 2024 10:00:32 -0700 Subject: [PATCH 3/4] Add length checks --- .../Portable/Workspace/ProjectSystem/ProjectSystemProject.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs b/src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs index ca7333b2c96ee..f9f8337dd7535 100644 --- a/src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs +++ b/src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs @@ -655,6 +655,7 @@ await _projectSystemProjectFactory.ApplyBatchChangeToWorkspaceMaybeAsync(useAsyn } // Analyzer reference removing... + if (_analyzersRemovedInBatch.Count > 0) { projectUpdateState = projectUpdateState.WithIncrementalAnalyzerReferencesRemoved(_analyzersRemovedInBatch); @@ -663,11 +664,12 @@ await _projectSystemProjectFactory.ApplyBatchChangeToWorkspaceMaybeAsync(useAsyn } // Analyzer reference adding... + if (_analyzersAddedInBatch.Count > 0) { projectUpdateState = projectUpdateState.WithIncrementalAnalyzerReferencesAdded(_analyzersAddedInBatch); solutionChanges.UpdateSolutionForProjectAction( - Id, solutionChanges.Solution.AddAnalyzerReferences(Id, projectUpdateState.AddedAnalyzerReferences)); + Id, solutionChanges.Solution.AddAnalyzerReferences(Id, _analyzersAddedInBatch)); } // Other property modifications... From ad31fade78d20777c464993d4b3fa4e6cf9e4bcb Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Thu, 22 Aug 2024 12:31:45 -0700 Subject: [PATCH 4/4] Fix --- .../ProjectSystem/ProjectSystemProject.cs | 81 +++++++++++-------- 1 file changed, 49 insertions(+), 32 deletions(-) diff --git a/src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs b/src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs index f9f8337dd7535..54171becad401 100644 --- a/src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs +++ b/src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs @@ -930,47 +930,54 @@ public void AddAnalyzerReference(string fullPath) foreach (var mappedFullPath in mappedPaths) { if (_analyzerPathsToAnalyzers.ContainsKey(mappedFullPath)) - { throw new ArgumentException($"'{fullPath}' has already been added to this project.", nameof(fullPath)); - } } - foreach (var mappedFullPath in mappedPaths) + if (_activeBatchScopes > 0) { - // Are we adding one we just recently removed? If so, we can just keep using that one, and avoid removing - // it once we apply the batch - var analyzerPendingRemoval = _analyzersRemovedInBatch.FirstOrDefault(a => a.FullPath == mappedFullPath); - if (analyzerPendingRemoval != null) - { - _analyzersRemovedInBatch.Remove(analyzerPendingRemoval); - _analyzerPathsToAnalyzers.Add(mappedFullPath, analyzerPendingRemoval); - } - else + foreach (var mappedFullPath in mappedPaths) { - // Nope, we actually need to make a new one. - var analyzerReference = new AnalyzerFileReference(mappedFullPath, _analyzerAssemblyLoader); - - _analyzerPathsToAnalyzers.Add(mappedFullPath, analyzerReference); - - if (_activeBatchScopes > 0) + // Are we adding one we just recently removed? If so, we can just keep using that one, and avoid removing + // it once we apply the batch + var analyzerPendingRemoval = _analyzersRemovedInBatch.FirstOrDefault(a => a.FullPath == mappedFullPath); + if (analyzerPendingRemoval != null) { - _analyzersAddedInBatch.Add(analyzerReference); + _analyzersRemovedInBatch.Remove(analyzerPendingRemoval); + _analyzerPathsToAnalyzers.Add(mappedFullPath, analyzerPendingRemoval); } else { - _projectSystemProjectFactory.ApplyChangeToWorkspace(w => w.OnAnalyzerReferenceAdded(Id, analyzerReference)); + // Nope, we actually need to make a new one. + var analyzerReference = new AnalyzerFileReference(mappedFullPath, _analyzerAssemblyLoader); + + _analyzersAddedInBatch.Add(analyzerReference); + _analyzerPathsToAnalyzers.Add(mappedFullPath, analyzerReference); } } } + else + { + _projectSystemProjectFactory.ApplyChangeToWorkspaceWithProjectUpdateState((w, projectUpdateState) => + { + foreach (var mappedFullPath in mappedPaths) + { + var analyzerReference = new AnalyzerFileReference(mappedFullPath, _analyzerAssemblyLoader); + _analyzerPathsToAnalyzers.Add(mappedFullPath, analyzerReference); + w.OnAnalyzerReferenceAdded(Id, analyzerReference); + + projectUpdateState = projectUpdateState.WithIncrementalAnalyzerReferenceAdded(analyzerReference); + } + + return projectUpdateState; + }); + } } } public void RemoveAnalyzerReference(string fullPath) { if (string.IsNullOrEmpty(fullPath)) - { throw new ArgumentException("message", nameof(fullPath)); - } var mappedPaths = GetMappedAnalyzerPaths(fullPath); @@ -980,28 +987,38 @@ public void RemoveAnalyzerReference(string fullPath) foreach (var mappedFullPath in mappedPaths) { if (!_analyzerPathsToAnalyzers.ContainsKey(mappedFullPath)) - { throw new ArgumentException($"'{fullPath}' is not an analyzer of this project.", nameof(fullPath)); - } } - foreach (var mappedFullPath in mappedPaths) + if (_activeBatchScopes > 0) { - var analyzerReference = _analyzerPathsToAnalyzers[mappedFullPath]; + foreach (var mappedFullPath in mappedPaths) + { + var analyzerReference = _analyzerPathsToAnalyzers[mappedFullPath]; - _analyzerPathsToAnalyzers.Remove(mappedFullPath); + _analyzerPathsToAnalyzers.Remove(mappedFullPath); - if (_activeBatchScopes > 0) - { // This analyzer may be one we've just added in the same batch; in that case, just don't add it in // the first place. if (!_analyzersAddedInBatch.Remove(analyzerReference)) _analyzersRemovedInBatch.Add(analyzerReference); } - else + } + else + { + _projectSystemProjectFactory.ApplyChangeToWorkspaceWithProjectUpdateState((w, projectUpdateState) => { - _projectSystemProjectFactory.ApplyChangeToWorkspace(w => w.OnAnalyzerReferenceRemoved(Id, analyzerReference)); - } + foreach (var mappedFullPath in mappedPaths) + { + var analyzerReference = _analyzerPathsToAnalyzers[mappedFullPath]; + _analyzerPathsToAnalyzers.Remove(mappedFullPath); + + w.OnAnalyzerReferenceRemoved(Id, analyzerReference); + projectUpdateState = projectUpdateState.WithIncrementalAnalyzerReferenceRemoved(analyzerReference); + } + + return projectUpdateState; + }); } } }