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

Hover info for local bindings #4969

Draft
wants to merge 11 commits into
base: trunk
Choose a base branch
from
Draft

Hover info for local bindings #4969

wants to merge 11 commits into from

Conversation

ChrisPenner
Copy link
Contributor

@ChrisPenner ChrisPenner commented May 17, 2024

Overview

  • Emits Type InfoNotes from the typecheecker containing information about the types of local bindings.
  • Wires up Hover in LSP to provide this type info when hovering on references to local-bindings
image

Implementation notes

  • Rather than only emitting notes for top-level bindings in the type-checker, emit notes for ALL bindings under different tags accordingly.
  • Wire up hover so if the hover is a var reference, check the local binding types and find a match.

Interesting/controversial decisions

  • Local var names don't need to be unique within a file (and you can shadow them anyways) so there may be many types emitted for the same var name, to account for this I also emit the scope of each binding as an annotation. When looking up the type of a binding you just find the smallest enclosing scope associated to that var name. This handles shadowing and multiple vars of the same name in the file.

Test coverage

  • Add tests for hover info.

Loose ends

TODO:

  • Support hovering the name of a binding:
    • The LHS of bindings are converted into Abs nodes in the resulting tree, and then are moved around all over the place in the tree as part of normalization, This makes it tricky to determine which variable we're referencing when hovering over the var at the binding site; ironically we can find the type of a var, just not at it's definition point 😆 . I can probably fix this either by keeping more info in the tree, or adding some special cases to how the LSP crawls the ABT, but it'll take a little more time to sort out.
  • Support hovering variables bound as function arguments
    • I should be able to emit notes about these during typechecking too. Just need to track down the right spot for it.
  • Ensure we handle letrec's correctly, I suspect it doesn't work in some cases.

Unlocks new features:

  • Will allow things like inline type-hints on local vars in your editor

@aryairani aryairani requested a review from dolio May 21, 2024 19:27
@ceedubs
Copy link
Contributor

ceedubs commented May 22, 2024

Hmm I feel like I'm doing something wrong here, but when I try this out I get a totally different type than in your screenshot.

image

Also I just get "No information found" for local variables in the more involved definition that I tried. Any idea why our results might differ? I'm running a local build of 00d32cc .

@ChrisPenner
Copy link
Contributor Author

@aryairani P.s. I don't think this is ready for review from anyone yet btw; I still have to fix let-recs, figure out what's up with Cody's issue, and fix some annotations 😄

@ChrisPenner ChrisPenner removed the request for review from dolio May 22, 2024 23:00
@ChrisPenner
Copy link
Contributor Author

@ceedubs Hrmm, I get your result if I remove the Nat. from the a Nat.+ 2;
Looks like I might be grabbing types from before TNDR has done its thing, probably fixable 😄

@ceedubs
Copy link
Contributor

ceedubs commented May 23, 2024

Oh I didn't realize that I failed to copy verbatim. Something something jpegs are lossy when copy/pasting code. But I guess that it was good because I might have caught an issue :)

@ChrisPenner
Copy link
Contributor Author

No matter how much I work on something I can always trust @ceedubs to find something I missed, which I actually really appreciated 😄 ;
Something to be said for having someone really unlucky on the team 😅 😆

@ceedubs
Copy link
Contributor

ceedubs commented Jun 6, 2024

@ChrisPenner I don't have a sense of how much work it is to get this to a state in which you think that it is ready for review. It's certainly not a high priority, but I just wanted to let you know that this would be a very welcome feature if you think that it's not much work to push it through :)

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