Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Extract local functions to make project updating simpler to understand. #74878

Merged
merged 5 commits into from
Aug 23, 2024
Merged

Extract local functions to make project updating simpler to understand. #74878

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1fa8fcb
Extract local function
CyrusNajmabadi Aug 23, 2024
a2e1b9b
Extract local function
CyrusNajmabadi Aug 23, 2024
9fad9b8
Update src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSy…
CyrusNajmabadi Aug 23, 2024
645f85d
Cleanup
CyrusNajmabadi Aug 23, 2024
fb8abd2
Merge branch 'extractFunctions' of https:/CyrusNajmabadi/…
CyrusNajmabadi Aug 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
204 changes: 119 additions & 85 deletions src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -549,7 +549,8 @@ private async Task OnBatchScopeDisposedMaybeAsync(bool useAsync)

await _projectSystemProjectFactory.ApplyBatchChangeToWorkspaceMaybeAsync(useAsync, (solutionChanges, projectUpdateState) =>
{
// Changes made inside this transformation must be idemopotent in case it is attempted multiple times.
// Changes made inside this transformation must be idempotent in case it is attempted multiple times.
var projectBeforeMutations = solutionChanges.Solution.GetRequiredProject(Id);

var documentFileNamesAddedBuilder = ImmutableArray.CreateBuilder<string>();
documentsToOpen = _sourceFiles.UpdateSolutionForBatch(
Expand Down Expand Up @@ -578,92 +579,14 @@ await _projectSystemProjectFactory.ApplyBatchChangeToWorkspaceMaybeAsync(useAsyn

documentFileNamesAdded = documentFileNamesAddedBuilder.ToImmutable();

// Metadata reference removing. Do this before adding in case this removes a project reference that
// we are also going to add in the same batch. This could happen if case is changing, or we're targeting
// a different output path (say bin vs. obj vs. ref).
foreach (var (path, properties) in _metadataReferencesRemovedInBatch)
{
projectUpdateState = TryRemoveConvertedProjectReference_NoLock(Id, path, properties, projectUpdateState, out var projectReference);

if (projectReference != null)
{
solutionChanges.UpdateSolutionForProjectAction(
Id,
solutionChanges.Solution.RemoveProjectReference(Id, projectReference));
}
else
{
// TODO: find a cleaner way to fetch this
var metadataReference = _projectSystemProjectFactory.Workspace.CurrentSolution.GetRequiredProject(Id).MetadataReferences
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the prior logic seems terrible/broken. We were accessing the mutable current soltuion of the workspace to make this decisino? That just seems never good. I changes to pass in the current 'immutable project state' that we're performing the mutation on as that seems sane and reasonavble.

.Cast<PortableExecutableReference>()
.Single(m => m.FilePath == path && m.Properties == properties);

projectUpdateState = projectUpdateState.WithIncrementalMetadataReferenceRemoved(metadataReference);

solutionChanges.UpdateSolutionForProjectAction(
Id,
newSolution: solutionChanges.Solution.RemoveMetadataReference(Id, metadataReference));
}
}
projectUpdateState = UpdateMetadataReferences(
projectBeforeMutations, solutionChanges, projectUpdateState, _metadataReferencesRemovedInBatch, _metadataReferencesAddedInBatch);

// Metadata reference adding...
if (_metadataReferencesAddedInBatch.Count > 0)
{
var projectReferencesCreated = new List<ProjectReference>();

foreach (var (path, properties) in _metadataReferencesAddedInBatch)
{
projectUpdateState = TryCreateConvertedProjectReference_NoLock(
Id, path, properties, projectUpdateState, solutionChanges.Solution, out var projectReference);

if (projectReference != null)
{
projectReferencesCreated.Add(projectReference);
}
else
{
var metadataReference = CreateMetadataReference_NoLock(path, properties, _projectSystemProjectFactory.SolutionServices);
projectUpdateState = projectUpdateState.WithIncrementalMetadataReferenceAdded(metadataReference);
}
}
UpdateProjectReferences(
Id, solutionChanges, _projectReferencesRemovedInBatch, _projectReferencesAddedInBatch);

solutionChanges.UpdateSolutionForProjectAction(
Id,
solutionChanges.Solution
.AddProjectReferences(Id, projectReferencesCreated)
.AddMetadataReferences(Id, projectUpdateState.AddedMetadataReferences));
}

// Project reference adding...
solutionChanges.UpdateSolutionForProjectAction(
Id,
newSolution: solutionChanges.Solution.AddProjectReferences(Id, _projectReferencesAddedInBatch));

// Project reference removing...
foreach (var projectReference in _projectReferencesRemovedInBatch)
{
solutionChanges.UpdateSolutionForProjectAction(
Id,
newSolution: solutionChanges.Solution.RemoveProjectReference(Id, projectReference));
}

// Analyzer reference removing...
if (_analyzersRemovedInBatch.Count > 0)
{
projectUpdateState = projectUpdateState.WithIncrementalAnalyzerReferencesRemoved(_analyzersRemovedInBatch);

foreach (var analyzerReference in _analyzersRemovedInBatch)
solutionChanges.UpdateSolutionForProjectAction(Id, solutionChanges.Solution.RemoveAnalyzerReference(Id, analyzerReference));
}

// Analyzer reference adding...
if (_analyzersAddedInBatch.Count > 0)
{
projectUpdateState = projectUpdateState.WithIncrementalAnalyzerReferencesAdded(_analyzersAddedInBatch);

solutionChanges.UpdateSolutionForProjectAction(
Id, solutionChanges.Solution.AddAnalyzerReferences(Id, _analyzersAddedInBatch));
}
projectUpdateState = UpdateAnalyzerReferences(
Id, solutionChanges, projectUpdateState, _analyzersRemovedInBatch, _analyzersAddedInBatch);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three simple helpers now: Update metadata references. Update project references. Update analyzer references.


// Other property modifications...
foreach (var propertyModification in _projectPropertyModificationsInBatch)
Expand Down Expand Up @@ -709,6 +632,117 @@ await _projectSystemProjectFactory.ApplyBatchChangeToWorkspaceMaybeAsync(useAsyn
if (hasAnalyzerChanges)
_projectSystemProjectFactory.Workspace.EnqueueUpdateSourceGeneratorVersion(projectId: null, forceRegeneration: true);
}

static ProjectUpdateState UpdateMetadataReferences(
Project projectBeforeMutation,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this guy is interesting in that we pass in the prior project state. that's needed as it is used to find teh MetadataRef instance associated with teh file path we want to remove.

SolutionChangeAccumulator solutionChanges,
ProjectUpdateState projectUpdateState,
List<(string path, MetadataReferenceProperties properties)> metadataReferencesRemovedInBatch,
List<(string path, MetadataReferenceProperties properties)> metadataReferencesAddedInBatch)
{
var projectId = projectBeforeMutation.Id;

// Metadata reference removing. Do this before adding in case this removes a project reference that we are also
// going to add in the same batch. This could happen if case is changing, or we're targeting a different output
// path (say bin vs. obj vs. ref).
foreach (var (path, properties) in metadataReferencesRemovedInBatch)
{
projectUpdateState = TryRemoveConvertedProjectReference_NoLock(projectId, path, properties, projectUpdateState, out var projectReference);

if (projectReference != null)
{
solutionChanges.UpdateSolutionForProjectAction(
projectId, solutionChanges.Solution.RemoveProjectReference(projectId, projectReference));
}
else
{
var metadataReference = projectBeforeMutation.MetadataReferences
.OfType<PortableExecutableReference>()
.Single(m => m.FilePath == path && m.Properties == properties);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: this logic seems odd to me. doing a == properties check here seems unnecessary and probably wrong too. you can't add the same filepath with different properties afaict. but i'm not changing that now. it just seems wonky.


projectUpdateState = projectUpdateState.WithIncrementalMetadataReferenceRemoved(metadataReference);

solutionChanges.UpdateSolutionForProjectAction(
projectId, solutionChanges.Solution.RemoveMetadataReference(projectId, metadataReference));
}
}

// Metadata reference adding...
if (metadataReferencesAddedInBatch.Count > 0)
{
var projectReferencesCreated = new List<ProjectReference>();

foreach (var (path, properties) in metadataReferencesAddedInBatch)
{
projectUpdateState = TryCreateConvertedProjectReference_NoLock(
projectId, path, properties, projectUpdateState, solutionChanges.Solution, out var projectReference);

if (projectReference != null)
{
projectReferencesCreated.Add(projectReference);
}
else
{
var metadataReference = CreateMetadataReference_NoLock(path, properties, solutionChanges.Solution.Services);
projectUpdateState = projectUpdateState.WithIncrementalMetadataReferenceAdded(metadataReference);
}
}

solutionChanges.UpdateSolutionForProjectAction(
projectId,
solutionChanges.Solution
.AddProjectReferences(projectId, projectReferencesCreated)
.AddMetadataReferences(projectId, projectUpdateState.AddedMetadataReferences));
}

return projectUpdateState;
}

static void UpdateProjectReferences(
ProjectId projectId,
SolutionChangeAccumulator solutionChanges,
List<ProjectReference> projectReferencesRemovedInBatch,
List<ProjectReference> projectReferencesAddedInBatch)
{
// Project reference adding...
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: project references swap the normal order. instead of remove/add, it's add/remove. I wasn't certain if it was safe to change that, so i kept it as is.

solutionChanges.UpdateSolutionForProjectAction(
projectId, solutionChanges.Solution.AddProjectReferences(projectId, projectReferencesAddedInBatch));

// Project reference removing...
foreach (var projectReference in projectReferencesRemovedInBatch)
{
solutionChanges.UpdateSolutionForProjectAction(
projectId, solutionChanges.Solution.RemoveProjectReference(projectId, projectReference));
}
}

static ProjectUpdateState UpdateAnalyzerReferences(
ProjectId projectId,
SolutionChangeAccumulator solutionChanges,
ProjectUpdateState projectUpdateState,
List<AnalyzerFileReference> analyzersRemovedInBatch,
List<AnalyzerFileReference> analyzersAddedInBatch)
{
// Analyzer reference removing...
if (analyzersRemovedInBatch.Count > 0)
{
projectUpdateState = projectUpdateState.WithIncrementalAnalyzerReferencesRemoved(analyzersRemovedInBatch);

foreach (var analyzerReference in analyzersRemovedInBatch)
solutionChanges.UpdateSolutionForProjectAction(projectId, solutionChanges.Solution.RemoveAnalyzerReference(projectId, analyzerReference));
}

// Analyzer reference adding...
if (analyzersAddedInBatch.Count > 0)
{
projectUpdateState = projectUpdateState.WithIncrementalAnalyzerReferencesAdded(analyzersAddedInBatch);

solutionChanges.UpdateSolutionForProjectAction(
projectId, solutionChanges.Solution.AddAnalyzerReferences(projectId, analyzersAddedInBatch));
}

return projectUpdateState;
}
}

#endregion
Expand Down
Loading