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.

Sign up for GitHub

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

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

Changes from 1 commit
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
130 changes: 126 additions & 4 deletions src/main/java/com/redhat/devtools/lsp4ij/features/rename/LSPRenameRefactoringDialog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,25 +13,43 @@
*******************************************************************************/
package com.redhat.devtools.lsp4ij.features.rename;

import com.intellij.openapi.application.ApplicationManager;
import com.intellij.openapi.command.WriteCommandAction;
import com.intellij.openapi.editor.Document;
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.openapi.util.TextRange;
import com.intellij.openapi.util.text.StringUtil;
import com.intellij.openapi.vfs.VirtualFile;
import com.intellij.psi.PsiElement;
import com.intellij.psi.PsiFile;
import com.intellij.psi.PsiReference;
import com.intellij.psi.impl.source.resolve.reference.PsiReferenceUtil;
import com.intellij.psi.search.PsiSearchHelper;
import com.intellij.psi.search.UsageSearchContext;
import com.intellij.refactoring.RefactoringBundle;
import com.intellij.refactoring.ui.NameSuggestionsField;
import com.intellij.refactoring.ui.RefactoringDialog;
import com.intellij.util.containers.ContainerUtil;
import com.redhat.devtools.lsp4ij.LSPFileSupport;
import com.redhat.devtools.lsp4ij.LSPIJUtils;
import com.redhat.devtools.lsp4ij.LanguageServerBundle;
import com.redhat.devtools.lsp4ij.features.LSPPsiElement;
import com.redhat.devtools.lsp4ij.features.refactoring.WorkspaceEditData;
import com.redhat.devtools.lsp4ij.internal.CancellationUtil;
import com.redhat.devtools.lsp4ij.internal.StringUtils;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import javax.swing.*;
import java.util.List;
import java.util.*;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.CompletionException;
import java.util.concurrent.ExecutionException;
Expand All @@ -47,6 +65,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 +163,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 +176,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 +199,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 +228,90 @@ protected void dispose() {
super.dispose();
}

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

Document document = LSPIJUtils.getDocument(file);
TextRange wordTextRange = document != null ? LSPIJUtils.getWordRangeAt(document, file, offset) : null;
if (wordTextRange != null) {
LSPPsiElement wordElement = new LSPPsiElement(file, wordTextRange);
String wordText = wordElement.getText();
if (StringUtil.isNotEmpty(wordText)) {
// When testing, just collect references on the current thread
if (ApplicationManager.getApplication().isUnitTestMode()) {
Copy link
Contributor Author

@SCWells72 SCWells72 Dec 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't love this, but it's important that the textual search occur under a modal progress indicator, and that doesn't work well with the rename refactoring unit tests. The same logic occurs either way, but in the real app it occurs under a modal progress indicator, and during tests it occurs on the current thread. It is consistent with quite a few other places in LSP4IJ that do the same thing, though, including some in rename refactoring support.

ContainerUtil.addAllNotNull(externalReferences, collectExternalReferences(file, wordText, wordTextRange, null));
} 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 References to '" + wordText + "'", true) {
@Override
public void run(@NotNull ProgressIndicator progressIndicator) {
progressIndicator.setIndeterminate(true);
ContainerUtil.addAllNotNull(externalReferences, collectExternalReferences(file, wordText, wordTextRange, progressIndicator));
}
});
}
}
}

return externalReferences;
}

@NotNull
private static Collection<PsiReference> collectExternalReferences(@NotNull PsiFile file,
@NotNull String wordText,
@NotNull TextRange wordTextRange,
@Nullable ProgressIndicator progressIndicator) {
Map<String, PsiReference> externalReferencesByKey = new LinkedHashMap<>();

PsiSearchHelper.getInstance(file.getProject()).processElementsWithWord(
(element, offsetInElement) -> {
PsiReference originalReference = element.findReferenceAt(offsetInElement);
List<PsiReference> references = originalReference != null ?
PsiReferenceUtil.unwrapMultiReference(originalReference) :
Collections.emptyList();
for (PsiReference reference : references) {
// Deduplicate using a unique key with reference type, file, and text range
String referenceKey = getReferenceKey(reference);
if (referenceKey != null) {
// Only add references we haven't added previously
if (!externalReferencesByKey.containsKey(referenceKey)) {
PsiElement targetElement = reference.resolve();
PsiFile targetFile = targetElement != null ? targetElement.getContainingFile() : null;
TextRange targetTextRange = targetFile != null ? targetElement.getTextRange() : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Move declaration of targetTextRange and targetText in the ifblock (L297))

if ((targetFile != null) && Objects.equals(file, targetFile) &&
(targetTextRange != null) && Objects.equals(wordTextRange, targetTextRange)) {
externalReferencesByKey.put(referenceKey, reference);
}
}
}
}
if (progressIndicator != null) {
progressIndicator.checkCanceled();
}
return true;
},
file.getUseScope(),
wordText,
UsageSearchContext.ANY,
// TODO: This should use the client-config caseSensitive setting for the file; what's
// the best way to get that from here?
true
SCWells72 marked this conversation as resolved.
Show resolved Hide resolved
);

return externalReferencesByKey.values();
}

@Nullable
private static String getReferenceKey(@NotNull PsiReference reference) {
PsiElement sourceElement = reference.getElement();
PsiFile sourceFile = sourceElement.getContainingFile();
VirtualFile sourceVirtualFile = sourceFile != null ? sourceFile.getVirtualFile() : null;
if (sourceVirtualFile != null) {
return reference.getClass().getName() + "::" + sourceVirtualFile.getPath() + "::" + reference.getAbsoluteRange();
}
return null;
}
}
Loading