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

Document symbols could include local definitions #3628

Open
michaelpj opened this issue Jun 8, 2023 · 8 comments
Open

Document symbols could include local definitions #3628

michaelpj opened this issue Jun 8, 2023 · 8 comments
Labels
Hackathon This issue is suitable for hackathon sessions level: easy The issue is suited for beginners type: enhancement New feature or request

Comments

@michaelpj
Copy link
Collaborator

At the moment we include all the "top-level" stuff. But we could include local definitions as well. The thing that would be awkward is reflecting nested scopes, but often they are also associated with a symbol.

It might also be rather noisy, but arguably that's a client problem? e.g. things should be shown collapsed by default, if you open up a function, sure you get lots of stuff.

Examples to think about:

f x = g x
  where 
    g = ...

f x = 
  let g = ..
  in g x

f x =
   -- awkward nesting and shadowing
   let g = let g = ... in g
   in g x

f x = do
    -- nest g inside res? what if we discarded the result?
   res <- let g = ... in g x
   pure res
@michaelpj michaelpj added type: enhancement New feature or request status: needs triage level: easy The issue is suited for beginners Hackathon This issue is suitable for hackathon sessions and removed status: needs triage labels Jun 8, 2023
@wz1000
Copy link
Collaborator

wz1000 commented Jun 8, 2023

We have an interval map in Development.IDE.Spans.LocalBindings that could be used to implement this

@sgillespie
Copy link
Contributor

Hi, I've been hacking at this, and here's an example of how it looks in emacs with eglot:

ss-20240522-213934

The markup itself looks like this:

* * *
Local bindings:

```haskell
hf :: HieASTs a
rm :: RefMap a
<-- Snip -->
prettyNames :: [Text]

```
* * *

How is this?

@michaelpj
Copy link
Collaborator Author

I'm a little confused about where you're adding this? I thought the idea was to put it into the document symbols, so it would show up in the outline, or similar?

@sgillespie
Copy link
Contributor

Thanks for responding, I completely misunderstood the description. Will attempt to add it to textDocument/documentSymbol

@sgillespie
Copy link
Contributor

After some discussion on Matrix, it sounds like what we really want is a something like
this:

  • f
    • g
      • h

(assuming f, g, and h are nested local functions). The IntervalMap in
Development.IDE.Spans.LocalBindings does not currently seem to give us this. Instead we
end up with a flat list of names [f, g, h].

This does not seem to be so straightforward. What am I missing?

@michaelpj
Copy link
Collaborator Author

This does not seem to be so straightforward. What am I missing?

We might just need to change it! There's no guarantee the existing code does what we want. I guess one question is whether we would be okay with the non-nested version or whether that's going to be more confusing than not having local bindings...

Instead we end up with a flat list of names [f, g, h].

This is a little surprising to me, since AFAICT completions seem to work properly, i.e. in this program

f = goto 
  where
     goto = hello
      where
        hello = 1

I get "goto" in the completions at the top level but not "hello".

@sgillespie
Copy link
Contributor

I get "goto" in the completions at the top level but not "hello".

This is because when we use the cursor position, we search upwards (getLocalScope), which gives us all names in scope at that point. To get all the local names in a top-level function, I believe we need to search the opposite direction (getFuzzyScope). However, AFAICT we're not getting this in a convenient form.

We might just need to change it! There's no guarantee the existing code does what we want. I guess one question is whether we would be okay with the non-nested version or whether that's going to be more confusing than not having local bindings...

This is certainly a possibility. We are currently using the IntervalMap from FingerTree which (to me) looks difficult to extend. We could roll our own that makes it easier to search subintervals.

@sgillespie
Copy link
Contributor

We might just need to change it! There's no guarantee the existing code does what we want. I guess one question is whether we would be okay with the non-nested version or whether that's going to be more confusing than not having local bindings...

I asked about this on the fingertree issue tracker, but so far I haven't received a response.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hackathon This issue is suitable for hackathon sessions level: easy The issue is suited for beginners type: enhancement New feature or request
Projects
Development

No branches or pull requests

3 participants