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

Self-versioned documents #10747

Merged
merged 18 commits into from
Aug 27, 2024
Merged

Self-versioned documents #10747

merged 18 commits into from
Aug 27, 2024

Conversation

davidwengier
Copy link
Contributor

@davidwengier davidwengier commented Aug 15, 2024 •
edited
Loading

Fixes #9197

A few integration tests to investigate, but the guts are here.

I'm slightly worried this might cause us to recompile files more often, but there is also the chance this fixes a bunch of bugs by recompiling files more often :)

Commit-at-a-time review is highly recommended, as there are lots of flow on effects of API changes

@davidwengier davidwengier requested a review from a team as a code owner August 15, 2024 20:15
@davidwengier davidwengier marked this pull request as draft August 16, 2024 01:11
@davidwengier davidwengier force-pushed the dev/dawengie/SelfVersionedDocumentSnapshots branch 4 times, most recently from d8a4db3 to 92743cc Compare August 16, 2024 05:07
@davidwengier davidwengier force-pushed the dev/dawengie/SelfVersionedDocumentSnapshots branch from 92743cc to 374e85c Compare August 16, 2024 05:11
@DustinCampbell
Copy link
Member

DustinCampbell commented Aug 19, 2024 •
edited
Loading

Commit-at-a-time review is highly recommended, as there are lots of flow on effects of API changes.

Note that I won't give a checkmark (nor should anyone else, honestly 😄) for a draft PR. Happy to leave early feedback though.

DustinCampbell
DustinCampbell approved these changes Aug 19, 2024
View reviewed changes
Copy link
Member

@DustinCampbell DustinCampbell left a comment

Choose a reason for hiding this comment

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

Did a pass through your commits.

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/DocumentState.cs Outdated Show resolved Hide resolved
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/IDocumentSnapshot.cs Show resolved Hide resolved
...oft.VisualStudio.LanguageServices.Razor/LanguageClient/Endpoints/RazorCustomMessageTarget.cs Outdated Show resolved Hide resolved
...nguageServices.Razor/LanguageClient/Endpoints/RazorCustomMessageTarget_UpdateCSharpBuffer.cs Outdated Show resolved Hide resolved
...zor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/GeneratedDocumentSynchronizerTest.cs Show resolved Hide resolved
src/Razor/test/Microsoft.CodeAnalysis.Razor.Workspaces.Test/ProjectSystem/ProjectStateTest.cs Outdated Show resolved Hide resolved
...or/test/Microsoft.AspNetCore.Razor.Test.Common.Tooling/ProjectSystem/TestDocumentSnapshot.cs Outdated Show resolved Hide resolved
...or/test/Microsoft.AspNetCore.Razor.Test.Common.Tooling/ProjectSystem/TestDocumentSnapshot.cs Outdated Show resolved Hide resolved
...o.LanguageServices.Razor/LanguageClient/Endpoints/RazorCustomMessageTarget_SemanticTokens.cs Outdated Show resolved Hide resolved
...oft.VisualStudio.LanguageServices.Razor/LanguageClient/Endpoints/RazorCustomMessageTarget.cs Outdated Show resolved Hide resolved
@davidwengier davidwengier marked this pull request as ready for review August 19, 2024 22:11
@davidwengier
Copy link
Contributor Author

Should have taken this out of draft a few days ago, sorry. I do still want to get an integration test run with for it though

maryamariyan
maryamariyan reviewed Aug 21, 2024
View reviewed changes
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Folding/FoldingRangeEndpoint.cs
@@ -55,7 +55,7 @@ public TextDocumentIdentifier GetTextDocumentIdentifier(FoldingRangeParams reque

var requestParams = new RazorFoldingRangeRequestParam
{
HostDocumentVersion = documentContext.Version,
HostDocumentVersion = documentContext.Snapshot.Version,
Copy link
Member

Choose a reason for hiding this comment

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

I can't tell from diff why we need to get version from Snapshot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simply because that's where it is now. It used to be on DocumentContext by way of DocumentVersionCache, but the version cache has been deleted and each document snapshot is responsible for its own version number now.

maryamariyan
maryamariyan reviewed Aug 21, 2024
View reviewed changes
Copy link
Member

@maryamariyan maryamariyan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, but seems like some merge conflicts have occurred

@davidwengier
Merge remote-tracking branch 'upstream/main' into dev/dawengie/SelfVe…
b477b4a 
…rsionedDocumentSnapshots

# Conflicts:
#	src/Razor/src/Microsoft.CodeAnalysis.Remote.Razor/ProjectSystem/DocumentContextExtensions.cs
@davidwengier
Copy link
Contributor Author

Integration tests work! Found a bug in code actions from the integration tests, which was also present in the presentation endpoints. And a slight whoopsy in our code action telemetry, which I didn't introduce, but I fixed because I'm nice like that 😛

Running another round of integration tests, and then will have to consider merging, versus holding off until after the snap.

@davidwengier davidwengier mentioned this pull request Aug 26, 2024
Merged
davidwengier
davidwengier commented Aug 27, 2024
View reviewed changes
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/GeneratedDocumentPublisher.cs Outdated
if (previouslyPublishedData.HostDocumentVersion > hostDocumentVersion)
{
// We've already published a newer version of this document. No-op.
Debug.Fail("Html document being published that is older than one we've previously published!");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ryzngard reports that removing this line (and the equiv for C#) fixes issues in VS Code, so this bears removal, and subsequent investigation

@davidwengier
Copy link
Contributor Author

Alright, I'm merging. Let the chips fall where they may.

@davidwengier davidwengier merged commit 1ea1552 into main Aug 27, 2024
12 checks passed
@davidwengier davidwengier deleted the dev/dawengie/SelfVersionedDocumentSnapshots branch August 27, 2024 05:32
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Aug 27, 2024
This was referenced Sep 4, 2024
Closed
Closed
@dotnet-bot dotnet-bot mentioned this pull request Sep 14, 2024
Closed
@davidwengier davidwengier mentioned this pull request Oct 7, 2024
Closed
@phil-allen-msft phil-allen-msft modified the milestones: Next, 17.12 P3 Oct 31, 2024
@davidwengier davidwengier mentioned this pull request Nov 14, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maryamariyan maryamariyan maryamariyan left review comments

@ryzngard ryzngard ryzngard approved these changes

@DustinCampbell DustinCampbell DustinCampbell approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
17.12 P3
Development

Successfully merging this pull request may close these issues.

Stop using VS buffer version numbers in the language server
5 participants
@davidwengier @DustinCampbell @ryzngard @maryamariyan @phil-allen-msft