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

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

Merged
merged 39 commits into from
Oct 31, 2024

Conversation

jurgenvinju
Copy link
Member

@jurgenvinju jurgenvinju commented Oct 16, 2024

  • lang::rascal::lsp::Actions module contains:
    • Command definitions
    • rascalCodeActions functions to detect code actions from a focus
    • rascalExecutor for executing commands
  • factor common code for code actions from ParameterizedLSP in common class
  • introduce command execution to Rascal document service
  • introduce codeAction LSP message
    • merge in quickfixes from Diagnostics
    • call Actions module for more actions
  • testing manually
  • adding test code
  • think of a way to expose Command and CodeAction definitions to rascal-core, such that the checker can annotate messages with quickfixes
  • try to remove more clones by factoring actions streaming code with function parameters.
RascalActions.mov

@jurgenvinju 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 jurgenvinju self-assigned this Oct 16, 2024
@jurgenvinju
Copy link
Member Author

@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
Copy link
Member Author

  • 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 :-)

Copy link
Contributor

@toinehartman toinehartman left a comment

Choose a reason for hiding this comment

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

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!

@jurgenvinju
Copy link
Member Author

jurgenvinju commented Oct 23, 2024

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?

Copy link
Member

@DavyLandman DavyLandman left a comment

Choose a reason for hiding this comment

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

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.

@jurgenvinju jurgenvinju marked this pull request as ready for review October 24, 2024 15:20
@jurgenvinju
Copy link
Member Author

jurgenvinju commented Oct 24, 2024

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.

@jurgenvinju
Copy link
Member Author

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?

Copy link
Member

@DavyLandman DavyLandman left a comment

Choose a reason for hiding this comment

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

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.

@jurgenvinju
Copy link
Member Author

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

@jurgenvinju
Copy link
Member Author

@toinehartman @DavyLandman we now have single-point-of-change for translations from LSP startLine/column to Rascal's lines and columns with Locations::toRascalPosition applied everywhere. I've search for + 1.

Copy link

sonarcloud bot commented Oct 30, 2024

@jurgenvinju jurgenvinju merged commit b555025 into main Oct 31, 2024
13 checks passed
@jurgenvinju jurgenvinju deleted the code-actions-for-rascal branch October 31, 2024 08:23
@toinehartman
Copy link
Contributor

Thanks @jurgenvinju! Nice work. Looking forward to trying out the initial refactorings and use of this framework to add others.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants