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

LSP: Support document symbols #3275

Merged
merged 35 commits into from
Jul 23, 2024
Merged

LSP: Support document symbols #3275

merged 35 commits into from
Jul 23, 2024

Conversation

PgBiel
Copy link
Contributor

@PgBiel PgBiel commented Jun 13, 2024

Fixes #3252

The main implementation is mostly done. All that is left is:

  1. DONE: For selection_range, I imagine that the correct range would be one pointing to the identifier for each symbol (e.g. a range around Name in pub type Name); right now I just duplicated the full symbol's range. I will look into this soon (draft in the meantime).
  2. Code style nitpicks might be in order. Feel free to ask for any changes in this regard.

It may also be desirable to write the full function signature alongside function symbols instead of just their types (at least, that's how rust-analyzer does it), but, in principle, the type has been working fine for an MVP. We could change this later if needed.

By the way, the #[allow(deprecated)] are because DocumentSymbol contains a deprecated field, but we have to specify it anyway to construct it.

Showcase

The source code below:

import gleam/option.{type Option}

pub type A

pub type B {
    C

    D(Option(Int))

    E(something: Option(Int))

    F(Int, something: Option(Int))
}

pub const my_const = 5

pub fn super_func(a: Int) -> Int {
    a + 5
}

produces the following outline on VSCode:

image

- Add label location to relevant types
- Modify parser to attach label locations
- Use label locations for selection_range
- Ensure 'range' spans the whole symbol, including type constructors and function bodies
@PgBiel
Copy link
Contributor Author

PgBiel commented Jun 16, 2024

I've fixed symbol locations such that:

  1. They now properly span the whole symbol, including a custom type's constructors or a function's body, so VSCode can now properly show the full path of symbols to your current cursor location.
    image

  2. The symbol's selection_range now only spans its name (whenever available, which should be always for top-level definitions). This is consistent with what the LSP standards expect, and also with what rust-analyzer does. This ensures clicking the symbol on the outline takes you to its name and not to pub fn or similar.

    • To do this, I had to add name_location / label_location / alias_location fields to a few symbols and properly populate them in the parser. Perhaps it could be more consistent to have a selection_location field common to those types, like rust-analyzer does, which is always present and is used to (later) instruct the LSP on what to do. But I think the current, less generalized impl will do for now.

P.S.: The clippy failure is unrelated to this PR.

@PgBiel PgBiel marked this pull request as ready for review June 16, 2024 00:25
@PgBiel
Copy link
Contributor Author

PgBiel commented Jun 16, 2024

Included doc comments as part of the symbol ranges as well now. Seems to match the LSP spec's recommendation, as well as what rust-analyzer does (for comparison).

image

@PgBiel
Copy link
Contributor Author

PgBiel commented Jun 16, 2024

By the way, let me know if I should have used ast::visit for any part of the implementation. In particular, I wanted to avoid introducing those more fine-grained location fields to the AST structures to be filled during parsing, but I couldn't think of a better way. I'm open to suggestions regarding any possible improvements here.

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Wonderful! Thank you! I've left some notes inline

compiler-core/src/ast.rs Outdated Show resolved Hide resolved
compiler-core/src/ast.rs Outdated Show resolved Hide resolved
compiler-core/src/ast.rs Outdated Show resolved Hide resolved
compiler-core/src/ast.rs Outdated Show resolved Hide resolved
compiler-core/src/ast.rs Outdated Show resolved Hide resolved
compiler-core/src/ast.rs Outdated Show resolved Hide resolved
compiler-core/src/ast.rs Outdated Show resolved Hide resolved
compiler-core/src/ast.rs Outdated Show resolved Hide resolved
compiler-core/src/language_server/engine.rs Outdated Show resolved Hide resolved
- Had to add a small workaround for 'put_doc' at ast.rs; hope there is a better solution
don't compare the source spans, only the labels
Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Lovely work! I've left a few notes inline, and we'll need tests for the document symbols part.

compiler-core/src/analyse.rs Outdated Show resolved Hide resolved
compiler-core/src/analyse.rs Outdated Show resolved Hide resolved
compiler-core/src/analyse.rs Outdated Show resolved Hide resolved
compiler-core/src/ast.rs Outdated Show resolved Hide resolved
compiler-core/src/package_interface.rs Outdated Show resolved Hide resolved
compiler-core/src/package_interface.rs Outdated Show resolved Hide resolved
compiler-core/src/package_interface.rs Outdated Show resolved Hide resolved
compiler-core/src/package_interface.rs Outdated Show resolved Hide resolved
compiler-core/src/package_interface.rs Outdated Show resolved Hide resolved
@PgBiel
Copy link
Contributor Author

PgBiel commented Jun 26, 2024

I'll look into adding some tests soon :)

compiler-core/src/ast.rs Outdated Show resolved Hide resolved
Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Lovely work!! Could you add some tests and also add that comment to each of the #[allow attributes please 🙏

compiler-core/src/ast.rs Show resolved Hide resolved
@lpil lpil marked this pull request as draft July 13, 2024 12:33
@PgBiel
Copy link
Contributor Author

PgBiel commented Jul 13, 2024

Sure, I'll do it soon!
Sorry, had run out of time 😅
But I definitely want to move this PR across the finish line :)

@PgBiel
Copy link
Contributor Author

PgBiel commented Jul 19, 2024

Some initial notes from tests:

  1. Initial /// from first line of doc comments isn't being included in symbol spans; (fixed) - now "CommentDoc" tokens have a marker_start field indicating where their /// started, which we use for doc position now
  2. pub const isn't being included in the location (it seems to be generally expected that the constant's location field corresponds to its name) (fixed) - now constructor.location includes everything from pub const to right before the equals;
  3. Constructor over multiple lines is not spanning the end parens, but rather stopping at the start of the line containing it (fixed) - now we just use constructor.location for the constructor's location;
  4. ⚠️ Constructor selection range seems to be across the whole constructor instead of just its name (this causes VSCode errors); (fixed) - after rebasing to main, the meaning of constructor.location changed to mean the whole constructor, having to switch my previous code to name_location for the name
  5. Spans for constructor args aren't including docs (fixed) - for some reason analyse was removing doc info from all constructor args, so fixed that

Other stuff before undrafting:

  • Add some more doc comments
  • Perhaps refactor existing _location fields for consistency Seems unnecessary, at least for this PR
  • Check it all again

@PgBiel PgBiel marked this pull request as ready for review July 20, 2024 03:28
@PgBiel
Copy link
Contributor Author

PgBiel commented Jul 20, 2024

@lpil There we go, had to make several changes when rebasing on main and upon adding tests, but I think it should be fine now. Feel free to take a look :)

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Thank you. I've left some more notes inline.

I think something has gone awry with the rebase, there's conflicts which mean I cannot rebase this branch into main.

compiler-core/src/ast.rs Outdated Show resolved Hide resolved
compiler-core/src/ast.rs Outdated Show resolved Hide resolved
compiler-core/src/parse.rs Outdated Show resolved Hide resolved
compiler-core/src/parse/extra.rs Outdated Show resolved Hide resolved
compiler-core/src/parse/token.rs Outdated Show resolved Hide resolved
@PgBiel
Copy link
Contributor Author

PgBiel commented Jul 20, 2024

I think something has gone awry with the rebase, there's conflicts which mean I cannot rebase this branch into main.

That's probably due to merge commits; you'll have to squash and merge instead. (I tried to manually squash locally, but it's a bit difficult at this point :p)

@lpil lpil marked this pull request as draft July 22, 2024 12:37
@PgBiel PgBiel marked this pull request as ready for review July 23, 2024 03:57
@PgBiel
Copy link
Contributor Author

PgBiel commented Jul 23, 2024

There we go!

The CI is failing as it seems you've added an explicit check against merge commits, but other than that it's fine. I believe I owe a bit of an explanation: The reason I've used merge commits to keep the branch up-to-date with Gleam's main is that it was much simpler (for me, as a contributor): for each update, I only had to resolve the conflicts considering the latest version of the PR; I didn't have to update each and every commit since the PR was created (in a traditional rebase), saving a lot of time (as I had to update the branch multiple times). Then, by squashing and merging at the end (at least up to the latest merge commit, but just pressing "Squash and merge" is of course easier), no merge commits will make it to Gleam's main branch, avoiding any conflicts and applying the patch in the same way. (This is the policy we generally follow at Typst, for instance.)

With that said, it may of course be preferable to be able to rebase the PR to main and keep the commit history depending on your preferred policies, though in principle I don't see problems with either model, so long as the contributor can deliver their changes.

Regarding this PR, you should be able to squash and merge as needed, given the above. Let me know if you face any problems or have any concerns.

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Thank you!!!!

@lpil lpil merged commit 581862a into gleam-lang:main Jul 23, 2024
11 of 12 checks passed
@PgBiel PgBiel deleted the doc-symbols branch July 23, 2024 15:47
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.

LSP: Add Document Symbol support
2 participants