Improve formatter edge case behavior (#1628)

eclipse-langium · Aug 14, 2024 · 6690076 · 6690076
1 parent 245336b
commit 6690076
Show file tree

Hide file tree

Showing 2 changed files with 43 additions and 13 deletions.
diff --git a/packages/langium/src/lsp/formatter.ts b/packages/langium/src/lsp/formatter.ts
@@ -150,17 +150,21 @@ export abstract class AbstractFormatter implements Formatter {
     protected avoidOverlappingEdits(textDocument: TextDocument, textEdits: TextEdit[]): TextEdit[] {
         const edits: TextEdit[] = [];
         for (const edit of textEdits) {
-            const last = edits[edits.length - 1];
-            if (last) {
+            let last = edits[edits.length - 1];
+            while (last) {
                 const currentStart = textDocument.offsetAt(edit.range.start);
                 const lastEnd = textDocument.offsetAt(last.range.end);
                 if (currentStart < lastEnd) {
                     edits.pop();
+                    last = edits[edits.length - 1];
+                }
+                else {
+                    break;
                 }
             }
             edits.push(edit);
         }
-        return edits;
+        return edits.filter(edit => this.isNecessary(edit, textDocument));
     }
 
     protected iterateAstFormatting(document: LangiumDocument, range?: Range): void {
@@ -200,11 +204,8 @@ export abstract class AbstractFormatter implements Formatter {
         return false;
     }
 
-    /**
-     * @deprecated This method has been deprecated with 3.1. It now always returns `true` and is no longer used by the default formatter implementation.
-     */
-    protected isNecessary(_edit: TextEdit, _document: TextDocument): boolean {
-        return true;
+    protected isNecessary(edit: TextEdit, document: TextDocument): boolean {
+        return edit.newText !== document.getText(edit.range).replace(/\r/g, '');
     }
 
     protected iterateCstFormatting(document: LangiumDocument, formattings: Map<string, FormattingAction>, options: FormattingOptions, range?: Range): TextEdit[] {
@@ -382,7 +383,10 @@ export abstract class AbstractFormatter implements Formatter {
         context.indentation += (tabs ?? 0);
         const edits: TextEdit[] = [];
         if (chars !== undefined) {
-            edits.push(this.createSpaceTextEdit(betweenRange, chars, formatting.options));
+            // Do not apply formatting on the same line if preceding node is hidden
+            if (!a?.hidden) {
+                edits.push(this.createSpaceTextEdit(betweenRange, chars, formatting.options));
+            }
         } else if (lines !== undefined) {
             edits.push(this.createLineTextEdit(betweenRange, lines, context, formatting.options));
         } else if (tabs !== undefined) {

diff --git a/packages/langium/test/grammar/lsp/grammar-formatter.test.ts b/packages/langium/test/grammar/lsp/grammar-formatter.test.ts
@@ -7,8 +7,8 @@
 import { EmptyFileSystem } from 'langium';
 import { expandToString } from 'langium/generate';
 import { createLangiumGrammarServices } from 'langium/grammar';
-import { expectFormatting } from 'langium/test';
-import { describe, test } from 'vitest';
+import { expectFormatting, parseDocument } from 'langium/test';
+import { describe, expect, test } from 'vitest';
 
 const services = createLangiumGrammarServices(EmptyFileSystem);
 const formatting = expectFormatting(services.grammar);
@@ -41,10 +41,12 @@ describe('Grammar Formatter', () => {
     test('Formats interface extends references', async () => {
         await formatting({
             before: expandToString`
-                interface A extends   B,C,    D,E{}
+                interface A // This is a comment
+                extends   B,C,    D,E{}
             `,
             after: expandToString`
-                interface A extends B, C, D, E {
+                interface A // This is a comment
+                extends B, C, D, E {
                 }
             `
         });
@@ -62,6 +64,30 @@ describe('Grammar Formatter', () => {
         });
     });
 
+    test('No edits if document is already formatted', async () => {
+
+        const formatter = services.grammar.lsp.Formatter;
+        if (!formatter) {
+            throw new Error(`No formatter registered for language ${services.grammar.LanguageMetaData.languageId}`);
+        }
+        const document = await parseDocument(services.grammar, expandToString`
+            interface Test {
+                // This is a comment
+                a: string
+                b: number
+                // This is another comment
+                c: boolean
+            }
+            `);
+        const identifier = { uri: document.uri.toString() };
+        const options = {
+            insertSpaces: true,
+            tabSize: 4
+        };
+        const edits = await formatter.formatDocument(document, { options, textDocument: identifier });
+        expect(edits.length).toBe(0);
+    });
+
     test('Formats parser rule definitions with alternatives', async () => {
         await formatting({
             before: expandToString`