Skip to content

Commit

Permalink
fix: Formatting fails when position end is out of range of the document
Browse files Browse the repository at this point in the history
Fixes #183

Signed-off-by: azerr <[email protected]>
  • Loading branch information
angelozerr authored and fbricon committed Mar 27, 2024
1 parent 1578cac commit dbef326
Show file tree
Hide file tree
Showing 9 changed files with 342 additions and 26 deletions.
35 changes: 30 additions & 5 deletions src/main/java/com/redhat/devtools/lsp4ij/LSPIJUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -262,9 +262,34 @@ public static Module getModule(@Nullable VirtualFile file, @NotNull Project proj
return ReadAction.compute(() -> ProjectFileIndex.getInstance(project).getModuleForFile(file, false));
}

public static int toOffset(Position start, Document document) throws IndexOutOfBoundsException {
int lineStartOffset = document.getLineStartOffset(start.getLine());
return lineStartOffset + start.getCharacter();
/**
* Returns a valid offset from the given position in the given document even if position is invalid.
*
* <ul>
* <li>If a line number is negative, it defaults to 0.</li>
* <li>If a line number is greater than the number of lines in a document, it defaults back to the number of lines in the document.</li>
* <li>If the character value is greater than the line length it defaults back to the line length</li>
* </ul>
*
* @param position the LSP position.
* @param document the IJ document.
* @return a valid offset from the given position in the given document.
*/
public static int toOffset(@NotNull Position position, @NotNull Document document) {
// Adjust position line/character according to this comment https://github.com/microsoft/vscode-languageserver-node/blob/ed3cd0f78c1495913bda7318ace2be7f968008af/textDocument/src/main.ts#L26
int line = position.getLine();
if (line < 0) {
// If a line number is negative, it defaults to 0.
line = 0;
} else {
// If a line number is greater than the number of lines in a document, it defaults back to the number of lines in the document.
line = Math.min(line, document.getLineCount() - 1);
}
int lineStartOffset = document.getLineStartOffset(line);
int lineEndOffset = document.getLineEndOffset(line);
int offset = lineStartOffset + position.getCharacter();
// If the character value is greater than the line length it defaults back to the line length
return Math.min(offset, lineEndOffset);
}

public static Position toPosition(int offset, Document document) {
Expand Down Expand Up @@ -653,15 +678,15 @@ public static boolean hasCapability(final Either<Boolean, ? extends Object> eith
/**
* Get the tab size for the given editor.
*/
public static int getTabSize(Editor editor) {
public static int getTabSize(@NotNull Editor editor) {
Project project = editor.getProject();
if (ApplicationManager.getApplication().isReadAccessAllowed()) {
return editor.getSettings().getTabSize(project);
}
return ReadAction.compute(() -> editor.getSettings().getTabSize(project));
}

public static boolean isInsertSpaces(Editor editor) {
public static boolean isInsertSpaces(@NotNull Editor editor) {
Project project = editor.getProject();
if (ApplicationManager.getApplication().isReadAccessAllowed()) {
return !editor.getSettings().isUseTabCharacter(project);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import com.intellij.formatting.service.AsyncDocumentFormattingService;
import com.intellij.formatting.service.AsyncFormattingRequest;
import com.intellij.lang.LanguageFormatting;
import com.intellij.openapi.editor.Document;
import com.intellij.openapi.editor.Editor;
import com.intellij.openapi.project.Project;
import com.intellij.openapi.util.TextRange;
Expand All @@ -33,7 +34,7 @@
/**
* Abstract class for LSP {@link AsyncDocumentFormattingService} implementation.
*/
public abstract class AbstractLSPFormattingService extends AsyncDocumentFormattingService {
public abstract class AbstractLSPFormattingService extends AsyncDocumentFormattingService {

@Nullable
@Override
Expand Down Expand Up @@ -76,7 +77,9 @@ public void run() {
formattingSupport = LSPFileSupport.getSupport(psiFile).getFormattingSupport();
formattingSupport.cancel();
Editor[] editors = LSPIJUtils.editorsForFile(psiFile.getVirtualFile(), psiFile.getProject());
formattingSupport.format(editors.length > 0 ? editors[0] : null, formattingRange, formattingRequest);
Editor editor = editors.length > 0 ? editors[0] : null;
Document document = editor != null ? editor.getDocument() : LSPIJUtils.getDocument(psiFile.getVirtualFile());
formattingSupport.format(document, editor, formattingRange, formattingRequest);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,18 @@
******************************************************************************/
package com.redhat.devtools.lsp4ij.features.formatting;

import com.intellij.openapi.editor.Editor;
import com.intellij.openapi.editor.Document;
import com.intellij.openapi.util.TextRange;
import org.jetbrains.annotations.Nullable;

/**
* LSP formatting parameters.
*
* @param editor the editor
* @param tabSize the tab size and null otherwise.
* @param insertSpaces the insert spaces and null otherwise.
* @param textRange the text range and null otherwise.
* @param document the document.
*/
public record LSPFormattingParams(@Nullable Editor editor, @Nullable TextRange textRange) {
public record LSPFormattingParams(@Nullable Integer tabSize, @Nullable Boolean insertSpaces, @Nullable TextRange textRange, @Nullable Document document) {

}
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,13 @@ public LSPFormattingSupport(@NotNull PsiFile file) {
super(file);
}

public void format(@Nullable Editor editor, @Nullable TextRange textRange, @NotNull AsyncFormattingRequest formattingRequest) {
LSPFormattingParams params = new LSPFormattingParams(editor, textRange);
public void format(@NotNull Document document,
@Nullable Editor editor,
@Nullable TextRange textRange,
@NotNull AsyncFormattingRequest formattingRequest) {
Integer tabSize = editor != null ? LSPIJUtils.getTabSize(editor) : null;
Boolean insertSpaces = editor != null ? LSPIJUtils.isInsertSpaces(editor) : null;
LSPFormattingParams params = new LSPFormattingParams(tabSize, insertSpaces, textRange, document);
CompletableFuture<List<? extends TextEdit>> formatFuture = this.getFeatureData(params);
try {
waitUntilDone(formatFuture, getFile());
Expand Down Expand Up @@ -135,14 +140,14 @@ protected CompletableFuture<List<? extends TextEdit>> doLoad(LSPFormattingParams

if (isRangeFormatting && languageServer.isDocumentRangeFormattingSupported()) {
// Range formatting
DocumentRangeFormattingParams lspParams = createDocumentRangeFormattingParams(params.editor(), params.textRange());
DocumentRangeFormattingParams lspParams = createDocumentRangeFormattingParams(params.tabSize(), params.insertSpaces(), params.textRange(), params.document());
return cancellationSupport.execute(languageServer
.getTextDocumentService()
.rangeFormatting(lspParams), languageServer, LSPRequestConstants.TEXT_DOCUMENT_RANGE_FORMATTING);
}

// Full document formatting
DocumentFormattingParams lspParams = createDocumentFormattingParams(params.editor());
DocumentFormattingParams lspParams = createDocumentFormattingParams(params.tabSize(), params.insertSpaces());
return cancellationSupport.execute(languageServer
.getTextDocumentService()
.formatting(lspParams), languageServer, LSPRequestConstants.TEXT_DOCUMENT_FORMATTING);
Expand All @@ -164,29 +169,33 @@ private static LanguageServerItem getFormattingLanguageServer(List<LanguageServe
return languageServers.get(0);
}

private @NotNull DocumentFormattingParams createDocumentFormattingParams(@Nullable Editor editor) {
private @NotNull DocumentFormattingParams createDocumentFormattingParams(Integer tabSize, Boolean insertSpaces) {
DocumentFormattingParams params = new DocumentFormattingParams();
params.setTextDocument(LSPIJUtils.toTextDocumentIdentifier(getFile().getVirtualFile()));
FormattingOptions options = new FormattingOptions();
if (editor != null) {
options.setTabSize(LSPIJUtils.getTabSize(editor));
options.setInsertSpaces(LSPIJUtils.isInsertSpaces(editor));
if (tabSize != null) {
options.setTabSize(tabSize);
}
if (insertSpaces != null) {
options.setInsertSpaces(insertSpaces);
}
params.setOptions(options);
return params;
}

private @NotNull DocumentRangeFormattingParams createDocumentRangeFormattingParams(@Nullable Editor editor, @NotNull TextRange textRange) {
private @NotNull DocumentRangeFormattingParams createDocumentRangeFormattingParams(Integer tabSize, Boolean insertSpaces, @NotNull TextRange textRange, Document document) {
DocumentRangeFormattingParams params = new DocumentRangeFormattingParams();
params.setTextDocument(LSPIJUtils.toTextDocumentIdentifier(getFile().getVirtualFile()));
FormattingOptions options = new FormattingOptions();
if (editor != null) {
options.setTabSize(LSPIJUtils.getTabSize(editor));
options.setInsertSpaces(LSPIJUtils.isInsertSpaces(editor));
if (tabSize != null) {
options.setTabSize(tabSize);
}
if (insertSpaces != null) {
options.setInsertSpaces(insertSpaces);
}
params.setOptions(options);
if (editor != null) {
Range range = LSPIJUtils.toRange(textRange, editor.getDocument());
if (document != null) {
Range range = LSPIJUtils.toRange(textRange, document);
params.setRange(range);
}
return params;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
/*******************************************************************************
* Copyright (c) 2024 Red Hat, Inc.
* Distributed under license by Red Hat, Inc. All rights reserved.
* This program is made available under the terms of the
* Eclipse Public License v2.0 which accompanies this distribution,
* and is available at http://www.eclipse.org/legal/epl-v20.html
*
* Contributors:
* Red Hat, Inc. - initial API and implementation
******************************************************************************/
package com.redhat.devtools.lsp4ij;

import com.intellij.openapi.editor.Document;
import com.intellij.openapi.editor.impl.DocumentImpl;
import com.intellij.testFramework.fixtures.BasePlatformTestCase;
import org.eclipse.lsp4j.Position;

/**
* Tests for {@link LSPIJUtils#toOffset(Position, Document)}.
*/
public class LSPIJUtils_toOffsetTest extends BasePlatformTestCase {

public void testValidPositionWithOneLine() {
assertOffset("foo", 0, 0, 0, 'f');
assertOffset("foo", 0, 1, 1, 'o');
assertOffset("foo", 0, 2, 2, 'o');
assertOffset("foo", 0, 3, 3, null);
}

public void testValidPositionWithTwoLines() {
assertOffset("foo\nbar", 0, 0, 0, 'f');
assertOffset("foo\nbar", 0, 1, 1, 'o');
assertOffset("foo\nbar", 0, 2, 2, 'o');
assertOffset("foo\nbar", 0, 3, 3, null);

assertOffset("foo\nbar", 1, 0, 4, 'b');
assertOffset("foo\nbar", 1, 1, 5, 'a');
assertOffset("foo\nbar", 1, 2, 6, 'r');
assertOffset("foo\nbar", 1, 3, 7, null);
}

public void testInvalidLineWithOneLine() {
assertOffset("foo", 999999, 0, 0, 'f');
assertOffset("foo", 999999, 1, 1, 'o');
assertOffset("foo", 999999, 2, 2, 'o');
assertOffset("foo", 999999, 3, 3, null);
}

public void testInvalidLineWithTwoLines() {
assertOffset("foo\nbar", 999999, 0, 4, 'b');
assertOffset("foo\nbar", 999999, 1, 5, 'a');
assertOffset("foo\nbar", 999999, 2, 6, 'r');
assertOffset("foo\nbar", 999999, 3, 7, null);
}

public void testInvalidCharacterWithOneLine() {
assertOffset("foo", 0, 999999, 3, null);
}

public void testInvalidCharacterWithTwoLines() {
assertOffset("foo\nbar", 0, 999999, 3, null);
assertOffset("foo\nbar", 1, 999999, 7, null);
}

public void testInvalidPositionWithOneLine() {
assertOffset("foo", 999999, 999999, 3, null);
}

public void testInvalidPositionWithTwoLine() {
assertOffset("foo\nbar", 999999, 999999, 7, null);
}

public void assertOffset(String content, int line, int character, int expectedOffset, Character expectedChar) {
assertOffset(content, new Position(line, character), expectedOffset, expectedChar);
}

public void assertOffset(String content, Position position, int expectedOffset, Character expectedChar) {
Document document = new DocumentImpl(content);
int actualOffset = LSPIJUtils.toOffset(position, document);
assertEquals(expectedOffset, actualOffset);
if (expectedChar == null) {
assertTrue(expectedOffset == content.length() /* offset is at the end of the file */
|| content.charAt(expectedOffset) == '\n' /* or offset is at the end of line */);
} else {
// get the character from the given offset
Character actualChar = document.getCharsSequence().charAt(expectedOffset);
assertEquals(expectedChar, actualChar);
}
}


}
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,12 @@
import com.redhat.devtools.lsp4ij.fixtures.LSPCompletionFixtureTestCase;

/**
* Completion test by emulating LSP textDocument/completion response from the typescript-language-server.
* Completion tests by emulating LSP 'textDocument/completion' responses
* from the typescript-language-server.
*/
public class TypeScriptCompletionTest extends LSPCompletionFixtureTestCase {

public void testCompletionWithNewText() {
public void testCompletionWithTextEdit() {
// 1. Test completion items result
assertCompletion("test.ts",
"''.<caret>", """
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/*******************************************************************************
* Copyright (c) 2024 Red Hat, Inc.
* Distributed under license by Red Hat, Inc. All rights reserved.
* This program is made available under the terms of the
* Eclipse Public License v2.0 which accompanies this distribution,
* and is available at http://www.eclipse.org/legal/epl-v20.html
*
* Contributors:
* Red Hat, Inc. - initial API and implementation
******************************************************************************/
package com.redhat.devtools.lsp4ij.features.formatting;

import com.redhat.devtools.lsp4ij.fixtures.LSPFormattingFixtureTestCase;

/**
* Formatting tests by emulating LSP 'textDocument/formatting', 'textDocument/rangeFormatting'
* responses from the clojure-lsp language server.
*/
public class ClojureFormattingTest extends LSPFormattingFixtureTestCase {

public void testFormatting() {
// Test with TextEdit end which is outside the document length (uses of 999999)
assertFormatting("test.ts",
"""
(let
[binding ""])""",
"""
[
{
"range": {
"start": {
"line": 0,
"character": 0
},
"end": {
"line": 999999,
"character": 999999
}
},
"newText": "(let\\n [binding \\"\\"])"
}
]
""",
"""
(let
[binding ""])""");
}
}
Loading

0 comments on commit dbef326

Please sign in to comment.