API and implementation of codefixes and code actions for Rascal itself #478

jurgenvinju · 2024-10-16T08:51:15Z

RascalActions.mov

…or Rascal itself

jurgenvinju · 2024-10-16T08:57:34Z

@DavyLandman @toinehartman this is still a draft but if you are interested and perhaps have feedback... that would be nice.

I'm trying to prepare us for the inevitable removed of Tree@\loc, and some quickfixes and code actions in the editor to help our users and ourselves with this would be really useful. The DSL part of this follow the exact same design on purpose.

jurgenvinju · 2024-10-16T09:00:11Z

Typically the type-checker would attach CodeActions to error messages, for quick-fix purposes
And for more general tools/refactorings/visualizations:
- visualize import/extend graph
- visualize call graph
- convert curly body function to expression function
- add parameter to all overloads
- convert switch to overloaded function
- ... etc ... the possibilities are endless :-)

toinehartman

Nice improvement! I like how this allows to write transformations and other code actions relatively easily. I am curious how you'll tackle contribution code actions from the type checker!

rascal-lsp/src/main/rascal/lang/rascal/lsp/Actions.rsc

jurgenvinju · 2024-10-23T08:39:27Z

Nice improvement! I like how this allows to write transformations and other code actions relatively easily. I am curious how you'll tackle contribution code actions from the type checker!

I was planning on learning that from you @toinehartman

But for the actions that can be attached to an error or warning message, it will be simply a action keyword field of the Message constructors. So all the code that produces Messages can attach a CodeAction with edits or a command at will, using the local information at hand.

The only small problem is that the type definitions for CodeAction and Command are now in rascal-lsp and I don't want a circular dependency. So I guess we will:

clone these definitions temporarily to rascal-core and at the same time,
move them to the standard library. I was thinking in util::IDEServices, or in Message?

…Becomes functionn from dsl tests

…hird option in the menu

…e third

… that adds a license

DavyLandman

I like how most of it looks 👍🏼 great reuse of the existing code-actions added the previous PR. My only concern is with the extra evaluator.

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

rascal-lsp/src/main/rascal/lang/rascal/lsp/Actions.rsc

…o the next test

…with respect to code actions

jurgenvinju · 2024-10-24T15:21:56Z

I think this is ready if you do too @toinehartman @DavyLandman. Note that this does not test any quickfix CodeActions attached to error or warning messages from the checker, yet. That's because the checker does not produce them yet. I plan to add a test here anway, as soon as one of those quickfixes starts popping up by the checker. But that's after merging IMHO.

sonarcloud · 2024-10-24T15:23:24Z

Quality Gate passed

Issues
6 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 SonarCloud

jurgenvinju · 2024-10-24T16:07:25Z

I like how most of it looks 👍🏼 great reuse of the existing code-actions added the previous PR. My only concern is with the extra evaluator.

I fixed the evaluator but the PR still thinks this is open? Help, where do I click?

DavyLandman

Some small remarks after a second review. I've changed my vote to approve, such that if you've went through them, and dealt with them to your discretion, you can merge.

DavyLandman · 2024-10-25T08:55:10Z

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

@@ -108,11 +110,12 @@ public void didChangeWorkspaceFolders(DidChangeWorkspaceFoldersParams params) {

    @Override
    public CompletableFuture<Object> executeCommand(ExecuteCommandParams params) {
-        if (params.getCommand().startsWith(RASCAL_META_COMMAND)) {
+        if (params.getCommand().startsWith(RASCAL_META_COMMAND) || params.getCommand().startsWith(RASCAL_COMMAND)) {


I'm not sure if for RASCAL_COMMAND we also want to do this? as this is about delegating to the sub-language, but that doesn't apply for the rascal commands.

Shouldn't that be a separate branch? that only runs documentService.execute command directly? or are we always passing in the rascal-langauge name everytime?

DavyLandman · 2024-10-25T08:56:12Z

rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/parametric/ParametricTextDocumentService.java

-                    .map(cmd  -> Either.<Command,CodeAction>forRight(cmd))
-                    .collect(Collectors.toList())
-            );
+        return CodeActions.mergeAndConvertCodeActions(this, dedicatedLanguageName, contribs.getName(), quickfixes, codeActions);


good that these got extracted out into a helper class.

DavyLandman · 2024-10-25T09:00:12Z

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

+        // convert to Rascal 1-based line
+        final var startLine = start.getLine() + 1;
+        // convert to Rascal UTF-32 column width
+        final var startColumn = columns.get(loc).translateInverseColumn(start.getLine(), start.getCharacter(), false);


I'm positive I've seen this code before, can we reduce the duplication? especially as it's easy to mess this up.

There is some overlap with this code, introduced for the rename refactoring. I agree with the proposal to factor this out.

DavyLandman · 2024-10-25T09:02:21Z

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

+    }
+
+    private CompletableFuture<IList> computeCodeActions(final int startLine, final int startColumn, ITree tree, PathConfig pcfg) {
+        IList focus = TreeSearch.computeFocusList(tree, startLine, startColumn);


please move this calculation of the focus list into the completeable future, to move as much of the calculation to async task.

DavyLandman · 2024-10-25T09:05:55Z

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

+
+            // finds an open menu with the right item in it (Change to a) and then select
+            // the parent that handles UI events like click() and sendKeys()
+            const menuContainer = await ide.hasElement(editor, By.xpath("//div[contains(@class, 'focused') and contains(@class, 'action')]/span[contains(text(), 'Add missing license header')]//ancestor::*[contains(@class, 'monaco-list')]"), Delays.normal, "Add-license action should be available and focused");


we should refactor this into a function into the IDE class, with the text as an argument.

toinehartman · 2024-10-28T08:35:44Z

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

+        // convert to Rascal 1-based line
+        final var startLine = start.getLine() + 1;
+        // convert to Rascal UTF-32 column width
+        final var startColumn = columns.get(loc).translateInverseColumn(start.getLine(), start.getCharacter(), false);


There is some overlap with this code, introduced for the rename refactoring. I agree with the proposal to factor this out.

jurgenvinju · 2024-10-28T10:59:27Z

@toinehartman right. Maybe we could make a Position to Position method like: Position fromLSPpositionToRascalPosition(Position p) . It is fairly critical code with weird constants and everything.

scaffolded an initial implementation of codefixes and code actioons f…

fac1420

…or Rascal itself

jurgenvinju changed the title ~~scaffolded an initial implementation of codefixes and code actioons for Rascal itself~~ API and implementation of codefixes and code actions for Rascal itself Oct 16, 2024

jurgenvinju self-assigned this Oct 16, 2024

jurgenvinju requested review from toinehartman and DavyLandman October 16, 2024 08:57

jurgenvinju added 6 commits October 16, 2024 13:56

added first example code action for Rascal

05c9f9a

added default

f60cff5

final fixes

62f627f

added sort imports action

ea0991d

fixed broken indentation

46f76ea

improved asyncronous command handling

55f6845

toinehartman reviewed Oct 17, 2024

View reviewed changes

rascal-lsp/src/main/rascal/lang/rascal/lsp/Actions.rsc Show resolved Hide resolved

jurgenvinju added 5 commits October 17, 2024 13:53

removed superfluous newlines in sorted imports and extends

f08530e

better grouping for imports and extends

0d0dcac

syntax better spaced

6be8c16

added \'add missing license\' action for Rascal

0ab364f

grammar rules have 1 line distance

4a83365

jurgenvinju added 10 commits October 23, 2024 10:52

added UI test for Rascal code actions; factored out common assertLine…

0eea6fc

…Becomes functionn from dsl tests

changed order of imports for testing purposes

d799dc2

use arrow keys to skip the other actions

1e074bc

fixed typo

17953b9

Merge branch 'main' into code-actions-for-rascal

4eecdaa

reintroduced method lost in merge

214dd88

changed test because the action is not currently focused, it is the t…

ed75af4

…hird option in the menu

bringing mo to the mountain, just test the first action instead of th…

31f88d8

…e third

added LICENSE to test project for testing purposes of the code action…

e853737

… that adds a license

added LICENSE to original files

eec1297

DavyLandman requested changes Oct 24, 2024

View reviewed changes

jurgenvinju added 8 commits October 24, 2024 13:58

make sure to revert changes made by code actions before we continue t…

e021fa5

…o the next test

factoring common functionality between Rascal LSP and Parametric DSL …

78250cd

…with respect to code actions

acted on suggestions in review by @DavyLandman

f8b0702

fixed bug with command going to the wrong LSP server

c07ba2e

adding license action only appears on module name now

58997ec

factoring constants

0644f00

fixed commands for Rascal itself

bbba2cb

fixed indentation

d5362ac

jurgenvinju marked this pull request as ready for review October 24, 2024 15:20

DavyLandman approved these changes Oct 25, 2024

View reviewed changes

toinehartman approved these changes Oct 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API and implementation of codefixes and code actions for Rascal itself #478

API and implementation of codefixes and code actions for Rascal itself #478

jurgenvinju commented Oct 16, 2024 •

edited

Loading

jurgenvinju commented Oct 16, 2024

jurgenvinju commented Oct 16, 2024

toinehartman left a comment

jurgenvinju commented Oct 23, 2024 •

edited

Loading

DavyLandman left a comment

jurgenvinju commented Oct 24, 2024 •

edited

Loading

sonarcloud bot commented Oct 24, 2024

jurgenvinju commented Oct 24, 2024

DavyLandman left a comment

DavyLandman Oct 25, 2024

DavyLandman Oct 25, 2024

DavyLandman Oct 25, 2024

toinehartman Oct 28, 2024

DavyLandman Oct 25, 2024

DavyLandman Oct 25, 2024

jurgenvinju Oct 28, 2024

toinehartman Oct 28, 2024

jurgenvinju commented Oct 28, 2024

API and implementation of codefixes and code actions for Rascal itself #478

Are you sure you want to change the base?

API and implementation of codefixes and code actions for Rascal itself #478

Conversation

jurgenvinju commented Oct 16, 2024 • edited Loading

jurgenvinju commented Oct 16, 2024

jurgenvinju commented Oct 16, 2024

toinehartman left a comment

Choose a reason for hiding this comment

jurgenvinju commented Oct 23, 2024 • edited Loading

DavyLandman left a comment

Choose a reason for hiding this comment

jurgenvinju commented Oct 24, 2024 • edited Loading

sonarcloud bot commented Oct 24, 2024

Quality Gate passed

jurgenvinju commented Oct 24, 2024

DavyLandman left a 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

jurgenvinju commented Oct 28, 2024

jurgenvinju commented Oct 16, 2024 •

edited

Loading

jurgenvinju commented Oct 23, 2024 •

edited

Loading

jurgenvinju commented Oct 24, 2024 •

edited

Loading