Skip to content
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

feat: Update external references on successful rename via LSP (issue 670) #674

Merged
merged 18 commits into from
Dec 11, 2024
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
44c4c18
Issue 670 - LSP rename refactorings now collect external references t…
SCWells72 Dec 7, 2024
d694595
Merge branch 'redhat-developer:main' into issue_670
SCWells72 Dec 7, 2024
7a64bb7
Issue 670 - Derived case-sensitivity for the language server(s) as we…
SCWells72 Dec 7, 2024
c2ceb97
Moved the notion of case-sensitivity to be independent of any specifi…
SCWells72 Dec 9, 2024
8c0f81c
Spit up the composite check for a valid reference into distinct stage…
SCWells72 Dec 9, 2024
9e36498
Removed unintended change that was being used to test VFS refresh.
SCWells72 Dec 9, 2024
42170d3
Issue 670 - Find Usages now also includes external references, so the…
SCWells72 Dec 9, 2024
364e9f1
This should not have been included in the previous commit.
SCWells72 Dec 9, 2024
d7d468c
Removed this unused logger.
SCWells72 Dec 9, 2024
b5105e7
Made this a proper pure utility class.
SCWells72 Dec 9, 2024
4c06435
The @NotNull annotation was lost in the signature change refactoring.
SCWells72 Dec 9, 2024
c34b31b
Changed the way we get the text offset when searching for external us…
SCWells72 Dec 9, 2024
6b72fa0
ProcessCanceledException should not be handled/logged explicitly. It …
SCWells72 Dec 9, 2024
de4ef90
Merge branch 'redhat-developer:main' into issue_670
SCWells72 Dec 10, 2024
7e6e64a
Removed refreshBundleFile() as Angelo has another approach and the pr…
SCWells72 Dec 10, 2024
1a856ba
Implemented PR review feedback suggestions.
SCWells72 Dec 10, 2024
b7d0db3
Merge branch 'redhat-developer:main' into issue_670
SCWells72 Dec 10, 2024
4a724eb
Merge branch 'redhat-developer:main' into issue_670
SCWells72 Dec 10, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion docs/LSPApi.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

The [LSPClientFeatures](https://github.com/redhat-developer/lsp4ij/blob/main/src/main/java/com/redhat/devtools/lsp4ij/client/features/LSPClientFeatures.java) API allows you to customize the behavior of LSP features, including:

- [Client-only features](#client-only-features)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if this break-out of client-only features isn't what you'd prefer.

- [LSP codeAction feature](#lsp-codeAction-feature)
- [LSP codeLens feature](#lsp-codeLens-feature)
- [LSP color feature](#lsp-color-feature)
Expand Down Expand Up @@ -163,6 +164,14 @@ public class MyLSPCodeLensFeature extends LSPCodeLensFeature {
}
```

## Client-only Features
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It already exists a table which list methods from LSPClientFeatures (ex: do a asearch with keepServerAlive). Your new method should be added here. If you think you need a
Client-only features please add it.


Client-only features are used to provide information to LSP4IJ that is not available from language servers but is required for proper integration of language server features into the IDE's corresponding features.

| API | Description | Default Behaviour |
|---------------------------------------|--------------------------------------------------------------------------------------|----------------------------|
| boolean isCaseSensitive(PsiFile file) | Determines whether or not the language grammar for the given file is case-sensitive. | `false` (case-insensitive) |

## LSP CodeAction Feature

| API | Description | Default Behaviour |
Expand Down Expand Up @@ -228,7 +237,7 @@ public class MyLSPCodeLensFeature extends LSPCodeLensFeature {
| boolean isStrikeout(CompletionItem item) | Returns true if the IntelliJ lookup is strike out and false otherwise. | use `item.getDeprecated()` or `item.getTags().contains(CompletionItemTag.Deprecated)` |
| String getTailText(CompletionItem item) | Returns the IntelliJ lookup tail text from the given LSP completion item and null otherwise. | `item.getLabelDetails().getDetail()` |
| boolean isItemTextBold(CompletionItem item) | Returns the IntelliJ lookup item text bold from the given LSP completion item and null otherwise. | `item.getKind() == CompletionItemKind.Keyword` |
| boolean isCaseSensitive(PsiFile file) | Determines whether or not completions should be offered in a case-sensitive manner. | Case-insensitive. |
|

## LSP Declaration Feature

Expand Down
17 changes: 17 additions & 0 deletions src/main/java/com/redhat/devtools/lsp4ij/LSPIJUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -1239,4 +1239,21 @@ public static String getProjectUri(Project project) {
.map(l -> new Location(l.getTargetUri(), l.getTargetSelectionRange() != null ? l.getTargetSelectionRange() : l.getTargetRange()))
.toList();
}

/**
* Forces the provided file that is bundled with LSP4IJ to refresh in VFS so that the latest version is used.
*
* @param bundledFile a file that is contained with the LSP4IJ distribution
*/
public static void refreshBundledFile(@NotNull VirtualFile bundledFile) {
// Refresh asynchronously then synchronously because otherwise bundled virtual files won't refresh properly
bundledFile.refresh(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this may seems strange, but it addresses a problem that I've also had in my plugin with bundled virtual files -- JSON schema, XSDs, etc., -- being stale until actually opened in an editor tab. After chatting with JetBrains, this two-stage forced VFS refresh seems to work reliably.

true,
bundledFile.isDirectory(),
() -> bundledFile.refresh(
false,
bundledFile.isDirectory()
)
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,26 @@ public CompletableFuture<Set<LanguageServerDefinition>> getAsyncMatched() {
}
}

/**
* Returns the language server wrappers for the specified file synchronously.
*
* @param file the file
* @return the language server wrappers that apply to the file
*/
@ApiStatus.Internal
@NotNull
public Set<LanguageServerWrapper> getMatchedLanguageServerWrappersSync(@NotNull VirtualFile file) {
Copy link
Contributor Author

@SCWells72 SCWells72 Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's a better way to do this without adding this method, please let me know. It's needed to encapsulate finding external references by just a PsiFile, an offset, and a processor in a synchronous manner as required by the rename refactoring since updating the external references is just a step in that editor transaction.

UPDATE: I've added a similar method in #669 for checking isSupported(PsiFile, ...). I could refactor that one and extract a method to answer this same question as if that makes sense.

Set<LanguageServerWrapper> languageServerWrappers = new LinkedHashSet<>();
MatchedLanguageServerDefinitions matchedLanguageServerDefinitions = getMatchedLanguageServerDefinitions(file, project, false);
if (matchedLanguageServerDefinitions != null) {
Set<LanguageServerDefinition> languageServerDefinitions = matchedLanguageServerDefinitions.getMatched();
for (LanguageServerDefinition languageServerDefinition : languageServerDefinitions) {
languageServerWrappers.add(new LanguageServerWrapper(project, languageServerDefinition));
}
}
return languageServerWrappers;
}

/**
* Returns the matched language server definitions for the given file.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import com.intellij.openapi.Disposable;
import com.intellij.openapi.project.Project;
import com.intellij.openapi.vfs.VirtualFile;
import com.intellij.psi.PsiFile;
import com.redhat.devtools.lsp4ij.LSPRequestConstants;
import com.redhat.devtools.lsp4ij.LanguageServerWrapper;
import com.redhat.devtools.lsp4ij.ServerStatus;
Expand Down Expand Up @@ -929,6 +930,17 @@ public boolean isEnabled(@NotNull VirtualFile file) {
return true;
}

/**
* Determines whether or not the language grammar for the file is case-sensitive.
*
* @param file the file
* @return true if the file's language grammar is case-sensitive; otherwise false
*/
public boolean isCaseSensitive(@NotNull PsiFile file) {
// Default to case-insensitive
return false;
}

/**
* Set the language server wrapper.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -229,8 +229,8 @@ public void addLookupItem(@NotNull LSPCompletionContext context,
@NotNull LookupElement lookupItem,
int priority,
@NotNull CompletionItem item) {
// Determine whether or not completions should be case-sensitive
boolean caseSensitive = isCaseSensitive(context.getParameters().getOriginalFile());
// Determine whether or not completions in this language should be case-sensitive
boolean caseSensitive = getClientFeatures().isCaseSensitive(context.getParameters().getOriginalFile());

var prioritizedLookupItem = PrioritizedLookupElement.withPriority(lookupItem, priority);

Expand Down Expand Up @@ -293,15 +293,4 @@ public void setServerCapabilities(@Nullable ServerCapabilities serverCapabilitie
completionCapabilityRegistry.setServerCapabilities(serverCapabilities);
}
}

/**
* Determines whether or not completions for the file should be offered in a case-sensitive manner.
*
* @param file the file
* @return true if completions should be offered in a case-sensitive manner; otherwise false
*/
public boolean isCaseSensitive(@NotNull PsiFile file) {
// Default to case-insensitive
return false;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,17 @@
*******************************************************************************/
package com.redhat.devtools.lsp4ij.features.rename;

import com.intellij.openapi.application.ApplicationManager;
import com.intellij.openapi.command.WriteCommandAction;
import com.intellij.openapi.editor.Editor;
import com.intellij.openapi.fileTypes.FileTypes;
import com.intellij.openapi.progress.ProgressIndicator;
import com.intellij.openapi.progress.ProgressManager;
import com.intellij.openapi.progress.Task;
import com.intellij.openapi.project.Project;
import com.intellij.openapi.ui.DialogWrapper;
import com.intellij.psi.PsiFile;
import com.intellij.psi.PsiReference;
import com.intellij.refactoring.RefactoringBundle;
import com.intellij.refactoring.ui.NameSuggestionsField;
import com.intellij.refactoring.ui.RefactoringDialog;
Expand All @@ -27,11 +33,16 @@
import com.redhat.devtools.lsp4ij.features.refactoring.WorkspaceEditData;
import com.redhat.devtools.lsp4ij.internal.CancellationUtil;
import com.redhat.devtools.lsp4ij.internal.StringUtils;
import com.redhat.devtools.lsp4ij.usages.LSPExternalReferencesFinder;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import javax.swing.*;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Set;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.CompletionException;
import java.util.concurrent.ExecutionException;
Expand All @@ -47,6 +58,8 @@
*/
class LSPRenameRefactoringDialog extends RefactoringDialog {

private static final Logger LOGGER = LoggerFactory.getLogger(LSPRenameRefactoringDialog.class);

@NotNull
private final LSPRenameParams renameParams;

Expand Down Expand Up @@ -143,6 +156,10 @@ protected boolean areButtonsValid() {
static void doRename(@NotNull LSPRenameParams renameParams,
@NotNull PsiFile psiFile,
@NotNull Editor editor) {
// Before having the language server perform the rename, see if there are any external references that should
// also be updated upon successful completion by the language server
int offset = editor.getCaretModel().getOffset();
Set<PsiReference> externalReferences = getExternalReferences(psiFile, offset);

CompletableFuture<List<WorkspaceEditData>> future = LSPFileSupport.getSupport(psiFile)
.getRenameSupport()
Expand All @@ -152,7 +169,8 @@ static void doRename(@NotNull LSPRenameParams renameParams,
// The 'rename' is stopped:
// - if user change the editor content
// - if it cancels the Task
String title = LanguageServerBundle.message("lsp.refactor.rename.progress.title", psiFile.getVirtualFile().getName(), renameParams.getNewName());
String newName = renameParams.getNewName();
String title = LanguageServerBundle.message("lsp.refactor.rename.progress.title", psiFile.getVirtualFile().getName(), newName);
waitUntilDoneAsync(future, title, psiFile);

future.handle((workspaceEdits, error) -> {
Expand All @@ -174,8 +192,19 @@ static void doRename(@NotNull LSPRenameParams renameParams,
LSPRenameHandler.showErrorHint(editor, LanguageServerBundle.message("lsp.refactor.rename.cannot.be.renamed.error"));
} else {
// Apply the rename from the LSP WorkspaceEdit list
WriteCommandAction
.runWriteCommandAction(psiFile.getProject(), () -> workspaceEdits.forEach(workspaceEditData -> LSPIJUtils.applyWorkspaceEdit(workspaceEditData.edit())));
WriteCommandAction.runWriteCommandAction(psiFile.getProject(), () -> {
workspaceEdits.forEach(workspaceEditData -> LSPIJUtils.applyWorkspaceEdit(workspaceEditData.edit()));

// Update any found external references with the new name
externalReferences.forEach(externalReference -> {
// Don't let a single failed external reference keep us from updating other references
try {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For relational integrity purposes, it's important that failure of a single rename propagation not short-circuit the rest.

externalReference.handleElementRename(newName);
} catch (Exception e) {
LOGGER.warn("External reference rename failed.", e);
}
});
});
}
return workspaceEdits;
});
Expand All @@ -192,4 +221,32 @@ protected void dispose() {
super.dispose();
}

@NotNull
private static Set<PsiReference> getExternalReferences(@NotNull PsiFile file, int offset) {
Set<PsiReference> externalReferences = new LinkedHashSet<>();

// When testing, just collect references on the current thread
if (ApplicationManager.getApplication().isUnitTestMode()) {
Copy link
Contributor Author

@SCWells72 SCWells72 Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is consistent with the existing precedent of keeping things a bit simpler when tests execute. In this case, no progress indicator is used during test execution.

LSPExternalReferencesFinder.processExternalReferences(file, offset, reference -> {
externalReferences.add(reference);
return true;
});
} else {
// This should happen on a progress indicator since it's performing a textual search of project
// sources, and it must be modal as we need the results synchronously
Project project = file.getProject();
ProgressManager.getInstance().run(new Task.Modal(project, "Finding External References", true) {
@Override
public void run(@NotNull ProgressIndicator progressIndicator) {
progressIndicator.setIndeterminate(true);
LSPExternalReferencesFinder.processExternalReferences(file, offset, reference -> {
externalReferences.add(reference);
return true;
});
}
});
}

return externalReferences;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,30 +19,20 @@
* Client-side settings for a user-defined language server configuration.
*/
public class ClientConfigurationSettings {
/**
* Client-side code completion settings.
*/
public static class ClientConfigurationCompletionSettings {
/**
* Whether or not completions should be offered as case-sensitive. Defaults to false.
*/
public boolean caseSensitive = false;
}

/**
* Client-side code workspace symbol settings.
*/
public static class ClientConfigurationWorkspaceSymbolSettings {
/**
* Whether or not completions should be offered as case-sensitive. Defaults to false.
* Whether or not the language server can support the IDE's Go To Class action efficiently. Defaults to false.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed a copy-paste comment from earlier.

*/
public boolean supportsGotoClass = false;
}

/**
* Client-side code completion settings
* Whether or not the language grammar is case-sensitive. Defaults to false.
*/
public @NotNull ClientConfigurationCompletionSettings completions = new ClientConfigurationCompletionSettings();
public boolean caseSensitive = false;

/**
* Client-side code workspace symbol settings
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@
*******************************************************************************/
package com.redhat.devtools.lsp4ij.server.definition.launching;

import com.intellij.psi.PsiFile;
import com.redhat.devtools.lsp4ij.client.features.LSPClientFeatures;
import org.jetbrains.annotations.NotNull;

/**
* Adds client-side configuration features.
Expand All @@ -24,7 +26,12 @@ public UserDefinedClientFeatures() {
super();

// Use the extended feature implementations
setCompletionFeature(new UserDefinedCompletionFeature());
setWorkspaceSymbolFeature(new UserDefinedWorkspaceSymbolFeature());
}

public boolean isCaseSensitive(@NotNull PsiFile file) {
UserDefinedLanguageServerDefinition serverDefinition = (UserDefinedLanguageServerDefinition) getServerDefinition();
ClientConfigurationSettings clientConfiguration = serverDefinition.getLanguageServerClientConfiguration();
return (clientConfiguration != null) && clientConfiguration.caseSensitive;
}
}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.jetbrains.jsonSchema.extension.JsonSchemaFileProvider;
import com.jetbrains.jsonSchema.extension.SchemaType;
import com.jetbrains.jsonSchema.impl.JsonSchemaVersion;
import com.redhat.devtools.lsp4ij.LSPIJUtils;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;

Expand All @@ -29,6 +30,7 @@
* Abstract base class for JSON schema file providers that are based on JSON schema files bundled in the plugin distribution.
*/
abstract class AbstractLSPJsonSchemaFileProvider implements JsonSchemaFileProvider {

private final String jsonSchemaPath;
private final String jsonFilename;
private VirtualFile jsonSchemaFile = null;
Expand All @@ -47,7 +49,7 @@ public final VirtualFile getSchemaFile() {
jsonSchemaFile = jsonSchemaFileUrl != null ? VirtualFileManager.getInstance().findFileByUrl(jsonSchemaFileUrl) : null;
// Make sure that the IDE is using the absolute latest version of the JSON schema
if (jsonSchemaFile != null) {
jsonSchemaFile.refresh(true, false);
LSPIJUtils.refreshBundledFile(jsonSchemaFile);
}
}
return jsonSchemaFile;
Expand Down
Loading
Loading