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

SCWells72 · 2024-12-07T18:23:14Z

Once the user has chosen a different name for the identifier under the caret, external references to that identifier are collected. If any were found, they are updated via PsiReference#handleElementRename() on successful completion of the LSP rename action.

…o that identifier before performing the actual rename, and then on successful rename, any references are updated regarding the new name.

SCWells72 · 2024-12-07T18:24:05Z

src/main/java/com/redhat/devtools/lsp4ij/features/rename/LSPRenameRefactoringDialog.java

+                    // 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 {


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

SCWells72 · 2024-12-07T18:25:16Z

src/main/java/com/redhat/devtools/lsp4ij/features/rename/LSPRenameRefactoringDialog.java

+            String wordText = wordElement.getText();
+            if (StringUtil.isNotEmpty(wordText)) {
+                // When testing, just collect references on the current thread
+                if (ApplicationManager.getApplication().isUnitTestMode()) {


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.

src/main/java/com/redhat/devtools/lsp4ij/features/rename/LSPRenameRefactoringDialog.java

…ll as can be. If language servers involved in the rename refactoring have mixed case-sensitivity, searching and matching is performed in a case-sensitive manner. As it says in the comment, in that (likely unusual) situation, it's much better not to have changed something due to a case-sensitivity mismatch than to change things that should not have been changed.

angelozerr · 2024-12-08T08:29:30Z

src/main/java/com/redhat/devtools/lsp4ij/features/rename/LSPRenameRefactoringDialog.java

+                            if (!externalReferencesByKey.containsKey(referenceKey)) {
+                                PsiElement targetElement = reference.resolve();
+                                PsiFile targetFile = targetElement != null ? targetElement.getContainingFile() : null;
+                                TextRange targetTextRange = targetFile != null ? targetElement.getTextRange() : null;


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

angelozerr · 2024-12-08T08:33:36Z

src/main/java/com/redhat/devtools/lsp4ij/features/rename/LSPRenameRefactoringDialog.java

+                    LSPRenameFeature renameFeature = clientFeatures.getRenameFeature();
+                    if (renameFeature.isRenameSupported(psiFile)) {
+                        LSPCompletionFeature completionFeature = clientFeatures.getCompletionFeature();
+                        if (completionFeature.isCaseSensitive(psiFile)) {


It is strange to use completion feature for rename. We discussed about this caseSenstive property and perhaps it would be better to move caseSensitive in the clientFeatures (we can do that since wehave not released) and the JSON client configuration content will declare just

{ "caseSensitive": true }

If you do that, please update JSON Schema docs and description (this caseSensitive is used n completion and rename both).

Yes, this is what I meant when that flag was introduced and I said that case-sensitivity is a language/grammar-level setting and not specific to any single feature associated with that language/grammar. I agree that the setting should be up-leveled, particularly before it has ever been included in a release. I'm happy to do that as part of this work.

Yes, this is what I meant when that flag was introduced and I said that case-sensitivity is a language/grammar-level setting and not specific to any single feature associated with that language/grammar.

I am sorry it was difficult for me to understand without usecases. Now I underdtand more with this new usecase.

I agree that the setting should be up-leveled, particularly before it has ever been included in a release. I'm happy to do that as part of this work.

Thanks!

Oh, not an issue at all. That wasn't meant as an "I told you so." I think we took the right approach of solving the problem we had until we saw another aspect of it. Here we are, and thankfully before the thing that has to be changed has been put into end user hands. No issues at all! I'll get it moved to a feature-independent location in the next day or two.

I need to discuss with @fbricon but I think we should create asap a new release to rmbed the fix #671

If you want we czn merge your PR and I will do the fix in order to embed the PR in the release.

…c feature. This commit also includes an improvement for how bundled files are refreshed in VFS so that the latest-and-greatest contents are always used. Currently this is used specifically for bundled JSON schema files, but it should work for any bundled file that's needed by an IDE feature.

SCWells72 · 2024-12-09T15:16:45Z

src/main/java/com/redhat/devtools/lsp4ij/LSPIJUtils.java

+     */
+    public static void refreshBundledFile(@NotNull VirtualFile bundledFile) {
+        // Refresh asynchronously then synchronously because otherwise bundled virtual files won't refresh properly
+        bundledFile.refresh(


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.

SCWells72 · 2024-12-09T15:17:32Z

docs/LSPApi.md

@@ -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)


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

…d checks based on PR review feedback.

… logic to find those references is extracted into the common class LSPExternalReferencesFinder.

SCWells72 · 2024-12-09T17:30:21Z

src/main/java/com/redhat/devtools/lsp4ij/LanguageServiceAccessor.java

+     */
+    @ApiStatus.Internal
+    @NotNull
+    public Set<LanguageServerWrapper> getMatchedLanguageServerWrappersSync(@NotNull VirtualFile file) {


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.

SCWells72 · 2024-12-09T17:31:35Z

src/main/java/com/redhat/devtools/lsp4ij/features/rename/LSPRenameRefactoringDialog.java

+        Set<PsiReference> externalReferences = new LinkedHashSet<>();
+
+        // When testing, just collect references on the current thread
+        if (ApplicationManager.getApplication().isUnitTestMode()) {


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.

SCWells72 · 2024-12-09T17:32:19Z

...java/com/redhat/devtools/lsp4ij/server/definition/launching/ClientConfigurationSettings.java

    /**
     * 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.


Fixed a copy-paste comment from earlier.

SCWells72 · 2024-12-09T17:35:15Z

src/main/java/com/redhat/devtools/lsp4ij/usages/LSPExternalReferencesFinder.java

+/**
+ * Utility class that helps to process/find external references to LSP4IJ-based (pseudo-)elements.
+ */
+public final class LSPExternalReferencesFinder {


This is the common utility class for finding/processing external usages of LSP4IJ's pseudo-elements.

SCWells72 · 2024-12-09T17:37:28Z

src/main/java/com/redhat/devtools/lsp4ij/usages/LSPUsageSearcher.java

@@ -92,6 +94,15 @@ public void processElementUsages(@NotNull PsiElement element, @NotNull Processor
        } catch (Exception e) {
            LOGGER.error("Error while collection LSP Usages", e);
        }
+
+        // For completeness' sake, also collect external usages to LSP (pseudo-)elements
+        int offset = ReadAction.compute(() -> {


This is how external usages are also included.

…ages.

…should be propagated.

SCWells72 · 2024-12-09T18:19:41Z

src/main/java/com/redhat/devtools/lsp4ij/usages/LSPUsageSearcher.java

@@ -89,9 +91,14 @@ public void processElementUsages(@NotNull PsiElement element, @NotNull Processor
                    }
                }
            }
+        } catch (ProcessCanceledException pce) {


I changed this behavior in one place based on an observed error. This same change may need to be rolled out anywhere there is try ... catch (Exception) ... where the caught exception could be ProcessCanceledException. That should only be caught/handled in VERY specific situations. Otherwise it should be propagated to the IDE.

It should be done like this with the main classes.

angelozerr · 2024-12-10T08:19:47Z

docs/LSPApi.md

@@ -163,6 +164,14 @@ public class MyLSPCodeLensFeature extends LSPCodeLensFeature {
 }
 ```

+## Client-only Features


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.

angelozerr · 2024-12-10T08:22:35Z

src/main/java/com/redhat/devtools/lsp4ij/usages/LSPExternalReferencesFinder.java

+        // miss changing things that should have been changed than to change things that should not
+        for (LanguageServerWrapper languageServerWrapper : languageServerWrappers) {
+            LSPClientFeatures clientFeatures = languageServerWrapper.getClientFeatures();
+            if ((clientFeatures != null) && clientFeatures.isCaseSensitive(psiFile)) {


clientFeatures != null should never happen

I'll annotate that method as @NotNull in the commit that removes that check so that there's no confusion going forward.

angelozerr · 2024-12-10T08:49:15Z

src/main/java/com/redhat/devtools/lsp4ij/usages/LSPExternalReferencesFinder.java

+        }
+
+        Project project = file.getProject();
+        Set<LanguageServerWrapper> languageServerWrappers = LanguageServiceAccessor.getInstance(project).getMatchedLanguageServerWrappersSync(virtualFile);


Replace with:

boolean caseSensitive = LanguageServiceAccessor.getInstance(file.getProject()) .hasAny(file.getVirtualFile(), ls -> ls.getClientFeatures().isCaseSensitive(file));

and remove your new method getMatchedLanguageServerWrappersSync.

…esence of that method is causing merge conflicts.

angelozerr · 2024-12-11T05:44:24Z

I cannot test the PR but code looks good.

Thanks @SCWells72 !

Issue 670 - LSP rename refactorings now collect external references t…

44c4c18

…o that identifier before performing the actual rename, and then on successful rename, any references are updated regarding the new name.

SCWells72 commented Dec 7, 2024

View reviewed changes

src/main/java/com/redhat/devtools/lsp4ij/features/rename/LSPRenameRefactoringDialog.java Outdated Show resolved Hide resolved

SCWells72 mentioned this pull request Dec 7, 2024

LSP rename should collect non-LSP references before the rename and invoke handleElementRename() on them after successful rename #670

Closed

SCWells72 and others added 2 commits December 7, 2024 12:28

Merge branch 'redhat-developer:main' into issue_670

d694595

angelozerr reviewed Dec 8, 2024

View reviewed changes

SCWells72 commented Dec 9, 2024

View reviewed changes

SCWells72 added 4 commits December 9, 2024 09:24

Spit up the composite check for a valid reference into distinct stage…

8c0f81c

…d checks based on PR review feedback.

Removed unintended change that was being used to test VFS refresh.

9e36498

Issue 670 - Find Usages now also includes external references, so the…

42170d3

… logic to find those references is extracted into the common class LSPExternalReferencesFinder.

This should not have been included in the previous commit.

364e9f1

SCWells72 commented Dec 9, 2024

View reviewed changes

SCWells72 added 2 commits December 9, 2024 11:33

Removed this unused logger.

d7d468c

Made this a proper pure utility class.

b5105e7

SCWells72 commented Dec 9, 2024

View reviewed changes

The @NotNull annotation was lost in the signature change refactoring.

4c06435

SCWells72 commented Dec 9, 2024

View reviewed changes

SCWells72 added 2 commits December 9, 2024 12:11

Changed the way we get the text offset when searching for external us…

c34b31b

…ages.

ProcessCanceledException should not be handled/logged explicitly. It …

6b72fa0

…should be propagated.

SCWells72 commented Dec 9, 2024

View reviewed changes

angelozerr reviewed Dec 10, 2024

View reviewed changes

SCWells72 and others added 5 commits December 10, 2024 08:14

Merge branch 'redhat-developer:main' into issue_670

de4ef90

Removed refreshBundleFile() as Angelo has another approach and the pr…

7e6e64a

…esence of that method is causing merge conflicts.

Implemented PR review feedback suggestions.

1a856ba

Merge branch 'redhat-developer:main' into issue_670

b7d0db3

Merge branch 'redhat-developer:main' into issue_670

4a724eb

angelozerr merged commit acadcf0 into redhat-developer:main Dec 11, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

SCWells72 commented Dec 7, 2024

SCWells72 Dec 7, 2024

SCWells72 Dec 7, 2024 •

edited

Loading

angelozerr Dec 8, 2024

angelozerr Dec 8, 2024

SCWells72 Dec 8, 2024

angelozerr Dec 8, 2024

SCWells72 Dec 8, 2024

angelozerr Dec 8, 2024

SCWells72 Dec 9, 2024

SCWells72 Dec 9, 2024

SCWells72 Dec 9, 2024 •

edited

Loading

SCWells72 Dec 9, 2024 •

edited

Loading

SCWells72 Dec 9, 2024

SCWells72 Dec 9, 2024

SCWells72 Dec 9, 2024

SCWells72 Dec 9, 2024

angelozerr Dec 11, 2024

angelozerr Dec 10, 2024

angelozerr Dec 10, 2024

SCWells72 Dec 10, 2024

angelozerr Dec 10, 2024

angelozerr commented Dec 11, 2024

		@@ -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)

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

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

Conversation

SCWells72 commented Dec 7, 2024

Choose a reason for hiding this comment

SCWells72 Dec 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SCWells72 Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

SCWells72 Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angelozerr commented Dec 11, 2024

SCWells72 Dec 7, 2024 •

edited

Loading

SCWells72 Dec 9, 2024 •

edited

Loading

SCWells72 Dec 9, 2024 •

edited

Loading