-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Implement the editor's new 'speculative edit' api to allow copilot suggestions to be classified accurately with Roslyn. #78794
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
Implement the editor's new 'speculative edit' api to allow copilot suggestions to be classified accurately with Roslyn. #78794
Conversation
src/EditorFeatures/Core/SpeculativeEdits/RoslynSpeculativeEditProvider.cs
Outdated
Show resolved
Hide resolved
src/EditorFeatures/Core/SpeculativeEdits/RoslynSpeculativeEditProvider.cs
Outdated
Show resolved
Hide resolved
| var newSolution = document.Project.Solution.WithDocumentText( | ||
| document.Project.Solution.GetRelatedDocumentIds(documentId), | ||
| clonedBuffer.AsTextContainer().CurrentText, | ||
| PreservationMode.PreserveIdentity); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here. this is what AbstractPreviewFactoryService does to fork and get the new roslyn solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite fishy since we're doing the OpenDocument call which will probably do the same thing...? Or I guess this is forking all linked copies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. WithDocumentText just sets the doc text for everything. .OpenDocument ensures all of the following runs:
onAfterUpdate: static (oldSolution, newSolution, data) =>
{
var (@this, documentId, textContainer, isCurrentContext, requireDocumentPresentAndClosed) = data;
@this.AddToOpenDocumentMap(documentId);
@this.SignupForTextChanges(documentId, textContainer, isCurrentContext, (w, id, text, mode) => w.OnDocumentTextChanged(id, text, mode));
var newDoc = newSolution.GetRequiredDocument(documentId);
@this.OnDocumentTextChanged(newDoc);
// Fire and forget that the workspace is changing.
@this.RaiseWorkspaceChangedEventAsync(WorkspaceChangeKind.DocumentChanged, oldSolution, newSolution, documentId: documentId);
// We fire 2 events on source document opened.
@this.RaiseDocumentOpenedEventAsync(newDoc);
@this.RaiseTextDocumentOpenedEventAsync(newDoc);
});Now, how important it is for that to run is unclear to me. However, this is what the lightbulb does for its preview buffers, so i figured i would eliminate chances for strangeness and keep in sync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe file a bug to investigate, but if this is what PreviewWorkspace is doing then we can fix them all up at once.
| <PackageReference Include="Microsoft.VisualStudio.LanguageServer.Client" PrivateAssets="all" /> | ||
| <PackageReference Include="Microsoft.VisualStudio.LanguageServer.Client.Implementation" PrivateAssets="all" /> | ||
| <PackageReference Include="Microsoft.VisualStudio.Text.UI.Wpf" /> | ||
| <PackageReference Include="Microsoft.VisualStudio.Text.Internal" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we have to mark this as private dependency or something. It doesn't appear we upload this package to NuGet (and haven't in quite some time), but check if the editor team has a special requirement here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jasonmalinowski is this a blocking concern on your part? or ca this be merged in and this poitn be resolved later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
text.internal is not part of the vssdk and we don't publish it to nuget
src/EditorFeatures/Core/SpeculativeEdits/RoslynSpeculativeEditProvider.cs
Show resolved
Hide resolved
src/EditorFeatures/Core/SpeculativeEdits/RoslynSpeculativeEditProvider.cs
Outdated
Show resolved
Hide resolved
src/EditorFeatures/Core/SpeculativeEdits/RoslynSpeculativeEditProvider.cs
Outdated
Show resolved
Hide resolved
| // Now take the cloned buffer and apply the edits to it the caller wants to speculate about. | ||
| ApplyEditsToClonedBuffer(options, clonedBuffer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We specifically put a method on the base type to do the cloning so we didn't have to reinvent this bit -- was that not usable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That guy doesn't ensure that we get an ITextDocument with a proper .cs path.
src/EditorFeatures/Core/SpeculativeEdits/RoslynSpeculativeEditProvider.cs
Outdated
Show resolved
Hide resolved
src/EditorFeatures/Core/SpeculativeEdits/RoslynSpeculativeEditProvider.cs
Outdated
Show resolved
Hide resolved
If we don't have LSP hookup, then things like classification through LSP would not work. |
src/VisualStudio/CSharp/Impl/ProjectSystemShim/CSharpEntryPointFinder.cs
Show resolved
Hide resolved
src/VisualStudio/CSharp/Impl/ProjectSystemShim/CSharpEntryPointFinderService.cs
Show resolved
Hide resolved
src/VisualStudio/Core/Def/Microsoft.VisualStudio.LanguageServices.csproj
Outdated
Show resolved
Hide resolved
src/VisualStudio/Core/Def/ProjectSystem/AbstractEntryPointFinderService.cs
Show resolved
Hide resolved
src/VisualStudio/Core/Test/Microsoft.VisualStudio.LanguageServices.UnitTests.vbproj
Outdated
Show resolved
Hide resolved
src/VisualStudio/LiveShare/Impl/Microsoft.VisualStudio.LanguageServices.LiveShare.csproj
Outdated
Show resolved
Hide resolved
src/VisualStudio/VisualBasic/Impl/ProjectSystemShim/VisualBasicEntryPointFinder.vb
Outdated
Show resolved
Hide resolved
src/VisualStudio/VisualBasic/Impl/ProjectSystemShim/VisualBasicEntryPointFinderService.vb
Show resolved
Hide resolved
|
@jasonmalinowski anything remaining that needs to change in this PR for you? |
| // Clone the existing text into a new editor snapshot/buffer that we can fork independently of the original. | ||
| var clonedSnapshotBeforeEdits = this.CloneWithEdits(options); | ||
|
|
||
| // Grab the ITextSnapshot of this cloned buffer before making any changes. The SpeculativeEdit api needs it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment seems backwards from the code -- the comment says we're grabbing the snapshot from the buffer, but the code is grabbing the buffer from the snapshot.
| var documentId = document.Id; | ||
|
|
||
| // Clone the existing text into a new editor snapshot/buffer that we can fork independently of the original. | ||
| var clonedSnapshotBeforeEdits = this.CloneWithEdits(options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why "beforeEdits" in the name -- it's representing after the edits, right?
| var newSolution = document.Project.Solution.WithDocumentText( | ||
| document.Project.Solution.GetRelatedDocumentIds(documentId), | ||
| clonedBuffer.AsTextContainer().CurrentText, | ||
| PreservationMode.PreserveIdentity); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe file a bug to investigate, but if this is what PreviewWorkspace is doing then we can fix them all up at once.
| // is consistent. | ||
| var newSolution = document.Project.Solution.WithDocumentText( | ||
| document.Project.Solution.GetRelatedDocumentIds(documentId), | ||
| clonedBuffer.AsTextContainer().CurrentText, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could just be clonedSnapshotBeforeEdits.AsText() rather than jumping to the container and back.
|
Will fix all feedback in followup. |
No description provided.