Skip to content

support module name and split out module name in qualified name #3957

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

Closed
Tracked by #3931
soulomoon opened this issue Jan 15, 2024 · 5 comments · Fixed by #3958
Closed
Tracked by #3931

support module name and split out module name in qualified name #3957

soulomoon opened this issue Jan 15, 2024 · 5 comments · Fixed by #3958
Assignees

Comments

@soulomoon
Copy link
Collaborator

soulomoon commented Jan 15, 2024

Mainly for quolified names.
Semantic highlighting looks worse than syntax highlighting for quolified name.

  1. A Syntax highlighting easily support splitting the module part and the name part of a quolified name.
  2. Semantic token is not identifing them, since the range of Name for quolified name include the module part and the name part as a whole.

Two steps to solve the problems.

  1. First add suppport for module names in the import section, it is easy since there is module name as identifier in hieAst Semantic tokens: add module name support and improve performance and accuracy by traversing the hieAst along with source code #3958
  2. split out the module part and name part, handles (Prelude.+), `Data.List.elem` Semantic tokens: add module name support and improve performance and accuracy by traversing the hieAst along with source code #3958
@soulomoon soulomoon self-assigned this Jan 15, 2024
@soulomoon soulomoon changed the title support module name and split out namespace as in Data.List.length support module name and split out module name as in Data.List.length Jan 15, 2024
@soulomoon soulomoon changed the title support module name and split out module name as in Data.List.length support module name and split out module name in qualified name Jan 15, 2024
@soulomoon
Copy link
Collaborator Author

soulomoon commented Jan 16, 2024

Two design decisions of step 2 need to be nail down.

  1. Step 2 requires peeking into the source code, which somewhat breaks the abstraction of LSP and reach into the LSP's package dependency Text.Rope, I am not quite sure if it is a good idea to implement Rope related function here. Excepcially when Update Data.Text.Utf16.Rope to Data.Text.Utf16.Rope.Mixed lsp#542 get into release and HLS picks it up, Rope related function implemented in HLS would surely break. Maybe it would be nice to have API such as getTextByCodePointRangeFromVfs in LSP(Like the one already there rangeLinesFromVfs) ? And I was wondering when would the next LSP version get release @michaelpj
  2. for (Prelude.+), it is original highlight (Prelude.+) as a whole, now we need to split them.
    1. break into (Prelude. and +).
    2. drop the () and break into Prelude. and +

@michaelpj
Copy link
Collaborator

Step 2 requires peeking into the source code

Hmm. Why do we need to go into the source code? Don't we have the OccName which I think includes the full name text? Then we could just look at that, count how many characters until the last ., and split the Range at that point? Will need a bit of care about what "character" means, but I don't think we necessarily need the whole file source.

for (Prelude.+), it is original highlight (Prelude.+) as a whole, now we need to split them.

Wait, we currently highlight the parens? That's surprising! Is that because GHC gives that as part of the span?

@soulomoon
Copy link
Collaborator Author

soulomoon commented Jan 16, 2024

Wait, we currently highlight the parens? That's surprising! Is that because GHC gives that as part of the span?

Yes, HieAst gives span of (Prelude.+), and OccName gives only + in its Fastring.
GHC handling of qualified name is not ideal for us.
This is the same reason we need to peek into the source code, since we don't have the full text (Prelude.+) anywhere in Name nor OccName(Or we should say, not anywhere in the HieAst).

@michaelpj
Copy link
Collaborator

That seems... surprising to me. The span includes all the other text, but the OccName doesn't? how odd. Is that expected @wz1000 ?

@soulomoon
Copy link
Collaborator Author

soulomoon commented Jan 18, 2024

You can see that from the renamed source as well. If my memory does not fail me, I was once saw an GHC issue about this handling of qualified name long time ago, but could not find it since I forget about the keyword to search for it. Turn out it is the same for (operator), `function`

soulomoon added a commit that referenced this issue Jan 29, 2024
…accuracy by traversing the hieAst along with source code (#3958)

fix #3957

Things have been done:
1. Switch `Name` to `Identifier` in the implementation and add `ModuleName` to the `HsSemanticTokenType`
2. Strip ``` ` ` ``` and `()`, and split out qualified names. e.g.``` `Preclude.length` ``` to ```Preclude.```  `length`
3. add tokenizer to walk ast with the souce rope to get more accurate result and faster. Should fix #3983.
4. add type sig to semanticConfig's TH result
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants