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.

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

Provide solution methods to allow updating a bulk set of documents at once. #73394

Merged
merged 9 commits into from
May 8, 2024

Conversation

CyrusNajmabadi
Copy link
Member

Useful for fix-all scenarios where we don't want to fork the solution N times per document changed. This now allows there to just be a single fork we apply at once.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels May 8, 2024
@@ -185,28 +185,35 @@ private static async Task<VersionStamp> ComputeLatestDocumentVersionAsync(TextDo
}

private AsyncLazy<VersionStamp> CreateLazyLatestDocumentTopLevelChangeVersion(
Copy link
Member Author

Choose a reason for hiding this comment

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

view with whitespace off.

: contentChanged
? CreateLazyLatestDocumentTopLevelChangeVersion(newDocument, newDocumentStates, newAdditionalDocumentStates)
: _lazyLatestDocumentTopLevelChangeVersion;
if (recalculateDocumentVersion)
Copy link
Member Author

Choose a reason for hiding this comment

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

i broke the nested conditionals into if-statements as it was breaking my brain.

@@ -162,7 +162,7 @@ private async Task<Checksum> ComputeDependentChecksumAsync(SolutionCompilationSt
{
// The generated file still exists in the underlying compilation, but the contents may not match the open file if the open file
// is stale. Replace the syntax tree so we have a tree that matches the text.
newStates = newStates.SetState(id, replacementState);
newStates = newStates.SetState(replacementState);
Copy link
Member Author

Choose a reason for hiding this comment

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

passing 'id' is redundant. it's already retrievable off of hte stat eobject passed in.

return currentSolution;
return args.originalSolution
.WithDocumentSyntaxRoots(syntaxRoots.ToImmutableAndClear())
.WithDocumentTexts(newTexts.ToImmutableAndClear());
Copy link
Contributor

Choose a reason for hiding this comment

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

WithDocumentTexts

I feel like there are a bunch of places that could benefit from this. Have you done an audit for other possible changes. OK with that being separated from this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

i audited the places i just added :) i will also look for other usages of WithDocumentSyntaxRoot and see if any cleanly can move over.

// corresponds to the common case of a single file being edited.
return newStates.Length == 1
? new TouchDocumentAction(oldProjectState, newProjectState, oldProjectState.DocumentStates.GetRequiredState(newStates[0].Id), newStates[0])
: new TouchDocumentsAction(oldProjectState, newProjectState, newStates);
Copy link
Member Author

Choose a reason for hiding this comment

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

i don't love this. but there is special logic in TouchDocumentAction (not plural). It's unclear to me how to maintain that logic here. So i'm solving it in this fashion.

var oldState = this.OldProjectState.DocumentStates.GetRequiredState(newState.Id);
finalCompilation = finalCompilation.ReplaceSyntaxTree(
await oldState.GetSyntaxTreeAsync(cancellationToken).ConfigureAwait(false),
await newState.GetSyntaxTreeAsync(cancellationToken).ConfigureAwait(false));
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: compiler lacks a bulk replace option. And i don't want to removeall+addall, as that would reorder trees.


/// <inheritdoc cref="TouchDocumentAction.CanUpdateCompilationWithStaleGeneratedTreesIfGeneratorsGiveSameOutput"/>
public override bool CanUpdateCompilationWithStaleGeneratedTreesIfGeneratorsGiveSameOutput
=> true;
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 100% sure about this @jasonmalinowski . however, as it is true for TouchDocumentAction, i don't know why it would not be true here just because we're touching multiple docs.

private SolutionCompilationState UpdateDocumentsInMultipleProjects<TDocumentState>(
IEnumerable<(ProjectId projectId, ImmutableArray<TDocumentState> updatedDocumentState)> projectIdAndUpdatedDocuments,
Func<ProjectState, ImmutableArray<TDocumentState>, TranslationAction> updatedDocumentsToProjectState)
where TDocumentState : TextDocumentState
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 is the exact same as the old method, just with teh name tweaked. if we can come up with a good name for both usages, we can remove one forwarding call from the method above.

return UpdateDocumentsInMultipleProjects(projectIdAndNewDocuments, addDocumentsToProjectState);
}

private SolutionCompilationState UpdateDocumentsInMultipleProjects<TDocumentState>(
Copy link
Contributor

Choose a reason for hiding this comment

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

UpdateDocumentsInMultipleProjects

Isn't this essentially a rename of AddDocumentsToMultipleProjects? If so, why keep the old one?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes. removed hte old one.

@CyrusNajmabadi CyrusNajmabadi marked this pull request as ready for review May 8, 2024 23:17
@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner May 8, 2024 23:17
Copy link
Contributor

@ToddGrun ToddGrun left a comment

Choose a reason for hiding this comment

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

:shipit:

@CyrusNajmabadi CyrusNajmabadi enabled auto-merge May 8, 2024 23:33
@CyrusNajmabadi CyrusNajmabadi merged commit 9c9420d into dotnet:main May 8, 2024
25 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone May 8, 2024
@CyrusNajmabadi CyrusNajmabadi deleted the updateDocumentsAtOnce branch May 8, 2024 23:41
@CyrusNajmabadi
Copy link
Member Author

@jasonmalinowski For review when you get back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants