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

initial implementation of goto types and separating annotations #2914

Merged
merged 11 commits into from
Apr 26, 2024

Conversation

Acepie
Copy link
Contributor

@Acepie Acepie commented Apr 7, 2024

An initial implementation for making annotations distinct from the thing being annotated. Currently in most places where the user hovers over an annotation it either doesn't work or hovers the larger object being annotated (function head, constant, custom type). This change is the first part of separating that out and allowing for operations on the located annotation. Currently that means hovering will display the type itself, completions will always be type completions (this seems to be exactly the same as current in all cases), and goto definition now will go to where the type is declared. There are a couple known gaps here but overall its still an improvement so want to start with this and get feedback

Known things to improve:

  • Type aliases seem to be stored in the ast directly as the type being aliased meaning that hover/goto will use the underlying type
  • Hovering over a type won't show documentation
  • Because of how locations/labels for CustomType constructor args are implemented hovering over the arg label will be treated as hovering over the annotation
    • This is probably fixable but honestly I kinda like it so wanna get thoughts

@lpil
Copy link
Member

lpil commented Apr 8, 2024

Because of how locations/labels for CustomType constructor args are implemented hovering over the arg label will be treated as hovering over the annotation

Does that mean if I have this code (][ is cursor)

pub type Thing {
  Thing(value: ][ String)
}

then it shows documentation for type String?

@Acepie
Copy link
Contributor Author

Acepie commented Apr 8, 2024

Yeah because of how the parser sets the location the entire arg is String. We could add some additional information to the AST if we wanna split it but I think the way it seemed to me rn "value" is treated as a label in the AST

@Acepie
Copy link
Contributor Author

Acepie commented Apr 8, 2024

Huh actually, I wanted to test this out after I typed that and I guess somewhere along the line that statement became no longer true and I'm not sure why. Wondering if maybe I had a bug in an earlier version or if there is something I am not remembering rn

@Acepie
Copy link
Contributor Author

Acepie commented Apr 8, 2024

Ok yeah the actual behavior is that the label has no hover (which matches the previous behavior). I think i had a bug at some middle phase that meant i was checking the wrong location and fixed it when I switched to do the TypeAst method of finding the location

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.

Smashing it as always! Thank you!

Left some v small questions

compiler-core/src/type_.rs Outdated Show resolved Hide resolved
@Acepie
Copy link
Contributor Author

Acepie commented Apr 9, 2024

Ok figured out how to get the documentation on hover for custom types. Ultimately ended up adding documentation to TypeConstructors and then passing it through and looking up the type on hover

Screenshot 2024-04-09 at 8 24 06 AM

@Acepie
Copy link
Contributor Author

Acepie commented Apr 9, 2024

I pulled the windows path fix into #2941

@Acepie Acepie force-pushed the gotoTypes branch 5 times, most recently from 6cf0a8a to 3bd6463 Compare April 19, 2024 12:01
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.

Fab! Really like this, will be a huge improvement to the language server.

There's a few tests I would like to have added here (autocompletions for the various places that annotations can happen) and I'd like to go back to how the definition location method worked before. See the note inline.

Thank you so much!!

compiler-core/src/build.rs Outdated Show resolved Hide resolved
compiler-core/src/language_server/engine.rs Outdated Show resolved Hide resolved
compiler-core/src/metadata/tests.rs Outdated Show resolved Hide resolved
compiler-core/src/type_.rs Outdated Show resolved Hide resolved
compiler-core/src/language_server/engine.rs Show resolved Hide resolved
@Acepie Acepie force-pushed the gotoTypes branch 2 times, most recently from 91ec94e to 3301ff1 Compare April 23, 2024 02:37
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.

Beautiful! Thank you so much!!!

@lpil lpil merged commit b1d4884 into gleam-lang:main Apr 26, 2024
11 checks passed
@Acepie Acepie deleted the gotoTypes branch May 1, 2024 14:16
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