Skip to content

Semantic tokens support #314

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

Merged
merged 2 commits into from
Jun 22, 2021
Merged

Semantic tokens support #314

merged 2 commits into from
Jun 22, 2021

Conversation

michaelpj
Copy link
Collaborator

@michaelpj michaelpj commented Apr 2, 2021

This adds support for semantic tokens. Concretely, that means:

  • A bevy of new types, in SemanticTokens.hs.
  • Four new methods.
  • A number of utility functions for working with semantic tokens, based
    on what I expect any user of this functionality would need
    (absolute/relative token positions, encoding tokens into arrays, and
    producing deltas)

Notes:

  • I have no idea what I'm doing with the method stuff, I just kept going until I ran out of compiler errors, hope I got everything.
  • The string/number enums are extremely tedious to write: you have to list all the constructors multiple times for the instances etc. Maybe worth some TH at some point.
  • Possibly the semantic tokens utilities should live somewhere else, but I'm not sure where. Various other modules in lsp-types seem to have utility functions in them, so it doesn't seem wildly out of place.
  • The performance of the diffing algorithm isn't great (see the test). I'm not sure if this is a problem in practice, but we can probably put something higher performance in there later if people actually use it.

@banacorn
Copy link
Contributor

This looks great, can't wait to give it a try!

@banacorn
Copy link
Contributor

banacorn commented Apr 17, 2021

I think both SemanticTokenAbsolute and SemanticTokenRelative need to have their record fields renamed (or prefixed).

I get clashes when I have length from Prelude in my code, and I can't seem to resolve it with those record field extension pragmas.

Edit: @wz1000 suggests that they should be imported qualified

@banacorn
Copy link
Contributor

Question: do I need to turn anything "on" in the server configs to enable this? Because I can't trigger any of my semantic highlighting handlers:

@wz1000
Copy link
Collaborator

wz1000 commented Apr 18, 2021

@banacorn Language.LSP.Types is meant to be imported qualified, we don't use prefixes for any of the fields unless they are haskell keywords ("data" -> "xdata", "type" -> "xtype").

@michaelpj
Copy link
Collaborator Author

It's possible that I failed to hook something up properly. I should write a test that actually sends some requests and responses!

@jneira
Copy link
Member

jneira commented Jun 1, 2021

@michaelpj i am afraid that this needs a rebase (plus the test you commented)

@michaelpj
Copy link
Collaborator Author

Thanks for the ping, I'm just not succeeding in spending much time on OSS recently! I'll try and get it polished up soon.

@michaelpj
Copy link
Collaborator Author

Okay, pushed an update. Has some more tests, but could do with even more. Things that I'd appreciate eyes on:

  • Have I changed all the right places? I found Call hierarchy support #332 very helpful, since I learned e.g. about Registration, which I had missed!
  • There are some fiddly utilities for encoding tokens, it would be good if someone could check them against the spec.
  • The diffing is still bad. I'm inclined to not care for now.
  • Actually using it! @banacorn it sounded like you'd actually tried using this in something? Is that available somewhere so I could try it?

@michaelpj
Copy link
Collaborator Author

I have no idea what's up with the Windows CI, cabal is having some incomprehensible difficulties and trying to pick a version of some that it's constrained not to do by the cabal.project. And someone just did that on master so I'm not sure why it's happening. If anyone with a Windows machine could try and reproduce that would be helpful, since I can't reproduce it without just running the CI lots of times...

@jneira
Copy link
Member

jneira commented Jun 19, 2021

Trying the build locally 🤞

@jneira
Copy link
Member

jneira commented Jun 19, 2021

  • removing constraints from cabal.project worked for me locally

@michaelpj
Copy link
Collaborator Author

Thanks!

This adds support for semantic tokens. Concretely, that menas:
- A bevy of new types, in `SemanticTokens.hs`.
- Four new methods.
- A number of utility functions for working with semantic tokens, based
on what I expect any user of this functionality would need
(absolute/relative token positions, encoding tokens into arrays, and
producing deltas)
@jneira
Copy link
Member

jneira commented Jun 19, 2021

@michaelpj the pr with support for ghc-9.0.1 does not have the problem with some, so i merged it (it was pending since several days ago) and i will rebase yours, if you dont mind

@michaelpj
Copy link
Collaborator Author

Sure. I accidentally reverted your constraint removal when I got rid of that extra commit :) Feel free to rebase, or I will tomorrow.

@jneira jneira force-pushed the semantic-tokens branch 2 times, most recently from 6a15ed2 to f2e1956 Compare June 19, 2021 22:16
@michaelpj
Copy link
Collaborator Author

Okay, CI is passing now.

Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for tests and comments
will merge soon if nobody disagree

@jneira jneira merged commit b484c2f into haskell:master Jun 22, 2021
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.

4 participants