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

chore: introduce the Visitor pattern #5727

Closed
wants to merge 6 commits into from

Conversation

asterite
Copy link
Collaborator

Description

Problem*

Resolves

Summary*

Additional Context

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@asterite asterite changed the base branch from master to ab/lsp-signature-help August 15, 2024 14:07
@asterite asterite mentioned this pull request Aug 15, 2024
5 tasks
@asterite
Copy link
Collaborator Author

To be honest, I'm a bit hesitant about this. It helps with not repeating how to traverse the AST, but on the other hand if a new AST node is added we can add it to the visitor but there's no requirement for all visitors to handle it, so we might miss handling new nodes 🤔

@asterite
Copy link
Collaborator Author

I continued working on this a bit more, and... I'm less and less sure about this. It makes the code a bit harder to follow, and it's easy to forget to handle some cases. The resulting code is a bit shorter, but traversing the whole AST is about 300 lines of code, and it's simple code, so it's also not that bad to do it manually.

I'll close this, but the code is here in case we want to use it in the future.

@asterite asterite closed this Aug 15, 2024
@asterite asterite deleted the ab/visitor branch August 15, 2024 15:29
github-merge-queue bot pushed a commit that referenced this pull request Aug 15, 2024
# Description

## Problem

Resolves #1585

## Summary

While working on autocompletion I was always checking what Rust Analyzer
did, or how it worked, and I noticed that in Rust Analyzer when it
autocompletes a function a little popup with the parameter names and
types would show up, which is really useful! But it turned out that's a
separate feature: signature help.

This PR implements that. I think it makes autocompletion much better. 

Before:


![lsp-signature-help-before](https://github.com/user-attachments/assets/2480e9bf-ae17-47a6-be0a-bcc58f2b2b92)

After:


![lsp-signature-help-after](https://github.com/user-attachments/assets/e50573d0-17cb-4c3a-b50b-a4fed462acdf)

Note: in the latest version the popup text starts with "fn ", but I
didn't want to re-record all the gifs 😅 .

You basically don't have to remember what were the parameter types.

It also works with methods:


![lsp-signature-help-method](https://github.com/user-attachments/assets/72fe65ee-2c68-4463-aa75-fd304fcbe50c)

And if you have an `fn`:


![lsp-signature-help-fn](https://github.com/user-attachments/assets/d29001e1-5de4-47f1-aede-da01bc1a3c53)

And here it's working in an aztec project (I always check that it works
outside of small toy programs 😊):


![lsp-signature-help-aztec](https://github.com/user-attachments/assets/3aa1d395-09bc-4578-b56d-c0e90630c4da)

## Additional Context

I promise this will be the last big LSP PR I send 🤞 . Now that we have
most of the LSP features registered, next PRs should be much smaller,
just adding on top of what there is.

These PRs are also relatively big because they traverse the AST and that
code is kind of boilerplatey. For that reason here I decided to put the
boilerplatey code in a `traversal.rs` file, so that it's clear there's
no extra logic there other than traversing the AST. I might send a
follow-up PR doing the same for autocompletion.

One more thing: you can trigger this functionality as a VSCode command
(Trigger Parameter Hints). But then when offering autocompletion you can
also tell VSCode to execute a command, and in this PR we tell it to
execute exactly that command (Rust Analyzer does the same, though they
have a custom command for it, not sure why).

UPDATE: I managed to implement the visitor pattern to reduce the
boilerplate. [Here's a PR](#5727)
with that. I think it will greatly simplify things, and make it easier
to keep the code updated as we add more nodes. It takes inspiration in
[how it's done in the Crystal
compiler](https://github.com/crystal-lang/crystal/blob/master/src/compiler/crystal/syntax/visitor.cr),
which in turn is inspired by how it was done in the Java editor support
for Eclipse 😄

## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.

---------

Co-authored-by: jfecher <jake@aztecprotocol.com>
Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

it's easy to forget to handle some cases.

I think something that would help this & the code size a bit would be to collapse some cases. E.g. Rust has pattern matching so we don't need to have a method for every expression or statement type. We can have one visit_expression function where the function can then just match on the expression cases it cares about since the trait should still handle the recursion, e.g:

fn visit_expression(&mut self, expr: &Expression) {
    match &expr.kind {
        ExpressionKind::Variable(..) => do_something(),
        ExpressionKind::Literal(..) => do_something_else(),
        _ => (), // don't care! Let the trait helpers recur for us behind the scenes.
    }
}

Similarly, another strategy would be that we don't have to go all out with this. The trait could just be for recurring over expressions. We also may not need a trait at all. It is more common in functional languages to have a mapping or folding function than a full visitor pattern. E.g. we can have a helper that recurs over expressions and calls a helper function on each node so that we no longer have to handle the recursion ourselves.

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.

2 participants