Idea for copy on write #5031
Idea for copy on write #5031
Conversation
|
Perf data for this is very promising. Old way of clone for the microsoft/pylance-release#4263:
New way with copy on write:
I believe this is functionally equivalent. In the fix all command, instead of cloning the service, we enter edit mode. During edit mode, all changes to a source file create a new SourceFileInfo and SourceFile. From then on, all changes are tracked. Once done with the changes, we leave edit mode and the changes made are returned in a list. |
packages/pyright-internal/src/analyzer/backgroundAnalysisProgram.ts
Outdated
Show resolved
Hide resolved
| @@ -46,6 +48,99 @@ test('test applyWorkspaceEdits changes', async () => { | |||
| assert.strictEqual(cloned.test_program.getSourceFile(range.fileName)?.getFileContent(), 'Text Changed'); | |||
| }); | |||
|
|
|||
| test('test edit mode for workspace', async () => { | |||
There was a problem hiding this comment.
I think we need to have tests that change the same file multiple times and has nested changes such as
First add new import statement from test import a, c and then add new symbol to the new import you added in from test import a[, b], c in different edit and then remove import statement or reorder import statement in the file (assuming it had existing import statement) in another edit
or you can think about other multiple edits, nested edit case
let's say we provide refactoring to merge import statements and remove unused imports. so user doesn't have duplicated import statements or same symbol with different name such as import os as o and import os as os and etc in the same file.
so user would do something like
python.analysis.fixAll = [ "removeDuplicateImports", "remouveUnusedImport" ]
There was a problem hiding this comment.
I changed the test to do a nested change. I didn't think it would matter if there was more than one, so I just added one extra nested change and verified we ended up with a single change.
…right into rchiodo/copy_on_write
| exitEditMode(): FileEditAction | undefined { | ||
| this._isEditMode = false; | ||
|
|
||
| // If we had an edit, return it |
There was a problem hiding this comment.
Nit: include period at end of sentence.
| this._isEditMode = true; | ||
| } | ||
|
|
||
| exitEditMode(): FileEditAction | undefined { |
There was a problem hiding this comment.
I was hoping to remove knowledge of VSCode's FileEditAction and TextDocument from this file. Could we simply return the new contents as a string and let the caller construct the FileEditAction from it?
There was a problem hiding this comment.
Sure that works. You mean at the program level?
There was a problem hiding this comment.
I was thinking at the service level. The service is the part that has knowledge of LSP concepts like text documents, etc.
There was a problem hiding this comment.
Actually, I should have said "languageServerBase", not "service".
There was a problem hiding this comment.
Something needs the file path though? You mean create another interface that just stores the content and the file path?
There was a problem hiding this comment.
Ah, I see what you mean. Yeah, then keeping it in the service (in the useEditMode method) makes sense.
There was a problem hiding this comment.
we get can filepath from sourceFile? and I guess that means, rename is simply delete and add new file?
…right into rchiodo/copy_on_write
Wrote a simple test, but also used this in pyrx to change how fix-all works.
I put back the change events for the program/sourceFile. It was the easiest way to capture the events (otherwise I believe all the changes would be a full document replace).