Improve handling of qualified names when renaming #531

toinehartman · 2024-11-26T16:20:15Z

Support renaming fully qualified modules names (instead of last name segment only)
Reject renaming of modules from qualified declaration names.
Explicitly do not support external imports
Support module renames from explorer view.
Escape name segments (\syntax) and v.v.
Resolve module names via dependencies
Use 'did rename' instead of 'will rename' callback, to prevent problems in case the evaluator is locked
Add UI test for module move

…g/module-names

DavyLandman

Looks good, with some exceptions that I marked down.

rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/BaseWorkspaceService.java

rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/IBaseTextDocumentService.java

rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/rascal/RascalLanguageServer.java

rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/rascal/RascalLanguageServices.java

rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/rascal/RascalWorkspaceService.java

rascal-lsp/src/main/rascal/lang/rascal/lsp/refactor/Util.rsc

rascal-lsp/src/main/rascal/lang/rascal/lsp/refactor/rename/Modules.rsc

rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/rascal/RascalWorkspaceService.java

This works around a more generic solution of sharing evaluators beteen the document and workspace services, whic is being tracked in #541.

rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/BaseLanguageServer.java

DavyLandman

Some small changes, looks mostly fine 👍🏼

rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/rascal/RascalLanguageServer.java

rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/rascal/RascalLanguageServices.java

rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/rascal/RascalTextDocumentService.java

rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/rascal/RascalLanguageServices.java

rascal-vscode-extension/src/test/vscode-suite/ide.test.ts

sonarcloud · 2024-12-12T15:33:17Z

Quality Gate passed

Issues
13 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

DavyLandman

Looks good, 2 points that we should still pay attention to:

the work inside the eval closure should really only call the evaluator, nothing more (just to avoid deadlocks), I've left a comment where it's happening.
the test currently fails.

DavyLandman · 2024-12-12T19:25:51Z

rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/rascal/RascalLanguageServices.java

+                ISourceLocation newLoc = sourceLocationFromUri(rename.getNewUri());
+
+                ISourceLocation currentWsFolder = findContainingWorkspaceFolder(currentLoc, sortedWorkspaceFolders)
+                    .orElseThrow(() -> new RuntimeException(String.format("Cannot automatically change uses of %s, since it is outside the current workspace.", currentLoc)));


nit: if we move this orElseThrow into the findContainingWorkspaceFolder we don't have to repeat it at the call site. (since it happens twice)

DavyLandman · 2024-12-12T19:26:18Z

rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/rascal/RascalLanguageServices.java

+    }
+
+    private static String readFile(ISourceLocation loc) {
+        URIResolverRegistry reg = URIResolverRegistry.getInstance();


nit: reg can be inlined.

DavyLandman · 2024-12-12T19:29:03Z

rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/rascal/RascalLanguageServices.java

+
+        return runEvaluator("Rascal module rename", semanticEvaluator, eval -> {
+            IFunction rascalGetPathConfig = eval.getFunctionValueFactory().function(getPathConfigType, (t, u) -> addResources(getPathConfig.apply((ISourceLocation) t[0])));
+            ISet qualifiedNameChanges = qualfiedNameChangesFromRenames(fileRenames, workspaceFolders, getPathConfig);


ideally we keep the work inside the eval lock as small as possible. especially since the qualifiedNameChangesFromRenames can end up callin maven (where there is some different locking happening in the latest version of rascal) we would hate to see a deadlock there.

So I would prefer if we can move this code to a completable future "before" this one, and only after that compose that to the runEvaluator closure.

DavyLandman · 2024-12-12T19:30:14Z

rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/rascal/RascalTextDocumentService.java

+                }
+            })
+            .exceptionally(e -> {
+                client.showMessage(new MessageParams(MessageType.Error, e.getCause().getMessage()));


also add a logger.catching call to log the original error in case we want to debug it.

DavyLandman · 2024-12-12T19:32:07Z

rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/rascal/RascalWorkspaceService.java

+            .stream()
+            .map(f -> Locations.toLoc(f.getUri()))
+            .collect(Collectors.toSet()), getExecuter())
+                .thenAccept(folders -> ((RascalTextDocumentService) docService).didRenameFiles(params, folders));


nit: I think the .thenAccept should not be indented as a "sub" of the stream operator, but higher up. It think that the stream chain should be 2 levels of indentation, and then .thenAccept at 1 level of indentation.

toinehartman self-assigned this Nov 26, 2024

toinehartman force-pushed the fix/rename-refactoring/module-names branch from 2e00c0b to 046df83 Compare November 26, 2024 16:25

toinehartman mentioned this pull request Nov 26, 2024

Renaming a module whose name occurs in a qualified function call fails #524

Closed

toinehartman added 7 commits November 27, 2024 15:49

Prepare qualified names for renaming.

78368fc

Perform validity checks outside of changed file iteration.

a5e9641

Implement renaming fully qualified names.

6a27810

Fix bug where qualified prefix would be renamed based on length only.

951d17a

Find imports by import path instead of module name.

3a78abb

Add self-qualified test.

c57048f

Do not support renaming external imports.

7b62cb2

toinehartman force-pushed the fix/rename-refactoring/module-names branch from 046df83 to 7b62cb2 Compare November 27, 2024 15:00

toinehartman added 2 commits December 2, 2024 12:50

Implement Rascal workspace service.

d571b20

Implement Java-based will-rename-files callback.

9f8d64b

toinehartman force-pushed the fix/rename-refactoring/module-names branch from 63a2866 to 9f8d64b Compare December 2, 2024 11:50

toinehartman added 6 commits December 2, 2024 14:05

Pair services in language server.

2cbe317

Move module rename analysis from Java to Rascal.

081c880

Fix lint error.

c2f7645

Merge remote-tracking branch 'origin/main' into fix/rename-refactorin…

be73a8d

…g/module-names

Fix import resolution when renaming module.

d5ea288

Escape qualified names, unescape file names.

2eb84b2

toinehartman force-pushed the fix/rename-refactoring/module-names branch from ab74b9e to 2eb84b2 Compare December 4, 2024 15:17

Only consider imports in projects that can import the renamed module.

9dc8e7d

toinehartman force-pushed the fix/rename-refactoring/module-names branch from 83be677 to 9dc8e7d Compare December 5, 2024 10:50

toinehartman added 3 commits December 5, 2024 19:20

Use didRenameFiles callback.

292acd6

Fix code style.

d39ed23

Add license.

7d23b17

DavyLandman requested changes Dec 6, 2024

View reviewed changes

DavyLandman reviewed Dec 6, 2024

View reviewed changes

rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/rascal/RascalWorkspaceService.java Outdated Show resolved Hide resolved

toinehartman added 2 commits December 6, 2024 16:41

UI test for renaming from explorer callback.

3f80f36

Move executor service to base class.

d7865dc

toinehartman added 3 commits December 9, 2024 13:38

Only set file operation capabilities for Rascal.

b918896

Improve exception handling.

e4e0956

Fix small review suggestions.

dd91729

toinehartman force-pushed the fix/rename-refactoring/module-names branch from 05284c1 to dd91729 Compare December 9, 2024 13:08

toinehartman added 5 commits December 9, 2024 15:59

Move rename files callback to prevent multiple evaluators.

a62056b

This works around a more generic solution of sharing evaluators beteen the document and workspace services, whic is being tracked in #541.

Fixes after PR review.

841ef40

Use for loop instead of streaming.

d931b5d

Reformat for readability.

85b32f3

Remove unused method.

06d9d24

DavyLandman reviewed Dec 10, 2024

View reviewed changes

rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/BaseLanguageServer.java Show resolved Hide resolved

toinehartman added 5 commits December 10, 2024 14:09

Use tree from editor when possible.

b30ecff

Fix LSP restart in loop.

1c8b52f

Fix debug log message.

4ffc1e7

Simplify exceptions.

af85b8a

Handle interrupt in Rascal callback.

fc17d67

toinehartman marked this pull request as ready for review December 10, 2024 14:00

toinehartman requested a review from DavyLandman December 10, 2024 14:10

DavyLandman requested changes Dec 10, 2024

View reviewed changes

toinehartman added 6 commits December 11, 2024 14:36

Improve readFile

496f17a

Do all work inside futures.

15482f2

Parallellize path config computation.

d6b64f3

Inline for readability.

1374f6a

Fix indentation.

c4f8d60

Remove parse tree caching.

2257dac

toinehartman requested a review from DavyLandman December 12, 2024 15:23

DavyLandman approved these changes Dec 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve handling of qualified names when renaming #531

Improve handling of qualified names when renaming #531

toinehartman commented Nov 26, 2024 •

edited

Loading

DavyLandman left a comment

DavyLandman left a comment

sonarcloud bot commented Dec 12, 2024

DavyLandman left a comment

DavyLandman Dec 12, 2024

DavyLandman Dec 12, 2024

DavyLandman Dec 12, 2024

DavyLandman Dec 12, 2024

DavyLandman Dec 12, 2024

Improve handling of qualified names when renaming #531

Are you sure you want to change the base?

Improve handling of qualified names when renaming #531

Conversation

toinehartman commented Nov 26, 2024 • edited Loading

DavyLandman left a comment

Choose a reason for hiding this comment

DavyLandman left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Dec 12, 2024

Quality Gate passed

DavyLandman left a comment

Choose a reason for hiding this comment

DavyLandman Dec 12, 2024

Choose a reason for hiding this comment

DavyLandman Dec 12, 2024

Choose a reason for hiding this comment

DavyLandman Dec 12, 2024

Choose a reason for hiding this comment

DavyLandman Dec 12, 2024

Choose a reason for hiding this comment

DavyLandman Dec 12, 2024

Choose a reason for hiding this comment

toinehartman commented Nov 26, 2024 •

edited

Loading