From 06c2e4bf04e09035defe06498ac03e6aca8bd950 Mon Sep 17 00:00:00 2001 From: Peter Retzlaff Date: Fri, 15 Dec 2023 18:16:08 +0100 Subject: [PATCH 1/2] Fix AST getting out of sync with the document, when the client sends multiple content changes at once. On a textDocument/didChange notification, if the contentChanges contain multiple entries, each of them needs to be handled as if all previous changes were already applied. When we don't, the calculation of the change's byte offsets will be wrong, which can lead to a syntax tree that produces parsing errors where the actual file does not have any issues. To solve this, we now apply the text of each change record to the full file text and use the result when applying the next change of the contentChanges array. --- src/common/providers/astProvider.ts | 51 +++++++++++++++++++---------- 1 file changed, 33 insertions(+), 18 deletions(-) diff --git a/src/common/providers/astProvider.ts b/src/common/providers/astProvider.ts index 65db827c..d93271ee 100644 --- a/src/common/providers/astProvider.ts +++ b/src/common/providers/astProvider.ts @@ -107,25 +107,28 @@ export class ASTProvider { let hasContentChanges = false; if ("contentChanges" in params && params.contentChanges) { hasContentChanges = true; + let currentText = tree?.rootNode.text; for (const change of params.contentChanges) { - if ("range" in change) { - tree?.edit(this.getEditFromChange(change, tree.rootNode.text)); - } else { - const currentText = tree?.rootNode.text; - if (currentText) { - const regex = new RegExp(/\r\n|\r|\n/); - const lines = currentText.split(regex); - const range = { - start: { line: 0, character: 0 }, - end: { line: lines.length, character: 0 }, - }; - tree?.edit( - this.getEditFromChange( - { text: change.text, range: range }, - currentText, - ), - ); - } + if (currentText && "range" in change) { + tree?.edit(this.getEditFromChange(change, currentText)); + currentText = this.applyChangeToDocument(change, currentText); + } else if (currentText){ + const regex = new RegExp(/\r\n|\r|\n/); + const lines = currentText.split(regex); + const range = { + start: { line: 0, character: 0 }, + end: { line: lines.length, character: 0 }, + }; + tree?.edit( + this.getEditFromChange( + { text: change.text, range: range }, + currentText, + ), + ); + currentText = this.applyChangeToDocument( + { text: change.text, range: range }, + currentText + ); } } } @@ -227,6 +230,18 @@ export class ASTProvider { } }; + private applyChangeToDocument( + change: { text: string; range: Range }, + text: string, + ): string { + const [startIndex, endIndex] = Utils.getIndicesFromRange( + change.range, + text, + ); + + return text.substring(0, startIndex) + change.text + text.substring(endIndex); + }; + private getEditFromChange( change: { text: string; range: Range }, text: string, From e9da3a2ad4041fc08a5d9bc780eb56f05c05ddc5 Mon Sep 17 00:00:00 2001 From: Razze Date: Fri, 22 Dec 2023 18:33:26 +0100 Subject: [PATCH 2/2] Refactor the loop to apply contentChanges to the tree. Also, only update the document text with each change when it's required, i.e. when there's more than one contentChange. --- src/common/providers/astProvider.ts | 85 +++++++++++++++++------------ 1 file changed, 51 insertions(+), 34 deletions(-) diff --git a/src/common/providers/astProvider.ts b/src/common/providers/astProvider.ts index d93271ee..8571b5f9 100644 --- a/src/common/providers/astProvider.ts +++ b/src/common/providers/astProvider.ts @@ -10,7 +10,11 @@ import { import { URI } from "vscode-uri"; import Parser, { Edit, Point, SyntaxNode } from "web-tree-sitter"; import { ElmWorkspaceMatcher } from "../util/elmWorkspaceMatcher"; -import { Position, Range } from "vscode-languageserver-textdocument"; +import { + Position, + Range, + TextDocumentContentChangeEvent, +} from "vscode-languageserver-textdocument"; import { TextDocumentEvents } from "../util/textDocumentEvents"; import { TreeUtils } from "../util/treeUtils"; import { ISourceFile } from "../../compiler/forest"; @@ -107,29 +111,9 @@ export class ASTProvider { let hasContentChanges = false; if ("contentChanges" in params && params.contentChanges) { hasContentChanges = true; - let currentText = tree?.rootNode.text; - for (const change of params.contentChanges) { - if (currentText && "range" in change) { - tree?.edit(this.getEditFromChange(change, currentText)); - currentText = this.applyChangeToDocument(change, currentText); - } else if (currentText){ - const regex = new RegExp(/\r\n|\r|\n/); - const lines = currentText.split(regex); - const range = { - start: { line: 0, character: 0 }, - end: { line: lines.length, character: 0 }, - }; - tree?.edit( - this.getEditFromChange( - { text: change.text, range: range }, - currentText, - ), - ); - currentText = this.applyChangeToDocument( - { text: change.text, range: range }, - currentText - ); - } + + if (tree) { + this.applyChangesToTree(tree, params.contentChanges); } } @@ -230,17 +214,50 @@ export class ASTProvider { } }; - private applyChangeToDocument( - change: { text: string; range: Range }, - text: string, - ): string { - const [startIndex, endIndex] = Utils.getIndicesFromRange( - change.range, - text, - ); + private applyChangesToTree( + tree: Parser.Tree, + changes: TextDocumentContentChangeEvent[], + ): void { + let text = tree.rootNode.text; - return text.substring(0, startIndex) + change.text + text.substring(endIndex); - }; + if (!text) { + return; + } + + const multipleChanges = changes.length > 1; + for (const change of changes) { + const changeRecord = this.getChangeWithRange(change, text); + const edit = this.getEditFromChange(changeRecord, text); + + tree?.edit(edit); + + // If there are multiple changes, we also need to take care to apply each change to the + // rootNode text. Otherwise, the startIndex and endIndex for later changes will be off. + if (multipleChanges) { + text = + text.substring(0, edit.startIndex) + + change.text + + text.substring(edit.oldEndIndex); + } + } + } + + private getChangeWithRange( + change: TextDocumentContentChangeEvent, + text: string, + ): { text: string; range: Range } { + if ("range" in change) { + return change; + } else { + const regex = new RegExp(/\r\n|\r|\n/); + const lines = text.split(regex); + const range = { + start: { line: 0, character: 0 }, + end: { line: lines.length, character: 0 }, + }; + return { text: change.text, range: range }; + } + } private getEditFromChange( change: { text: string; range: Range },