sublimelsp / LSP-typescript

TypeScript, JavaScript support for Sublime LSP plugin
MIT License
132 stars 11 forks source link

No replacement provided for "Find Callers" (but should be one available?) #186

Closed JGJP closed 1 year ago

JGJP commented 1 year ago

After having LSP-typescript automatically updated to 2.0.0 by Package Control, I found that the Find Callers command didn't work anymore. Checking the history of this repo and this release of typescript-language-server it seems that it was removed because support for the underlying method was removed, but what are we supposed to use as an alternative? LSP: Find References doesn't work the same way at all.

The TLS 3.0.0 release seems to provide an alternative API, so maybe it would be appropriate to roll back the LSP-typescript 2.0.0 release until that is implemented? I don't see any other advantage to upgrading to TLS 3.0.0 and for now I've had to manually roll back LSP-typescript to 1.20.0 to maintain my productivity.

JGJP commented 1 year ago

I would like to add that I'm very grateful for the maintenance of this package, as it's a crucial one for me and many others. Would like it to be clear that I don't want to come off as a whiner. I would submit the commits myself but don't have the schedule or ecosystem know-how to make it happen, unfortunately...

rchl commented 1 year ago

I've updated to new version maybe a bit prematurely but that's because implementation of the Call Hierarchy in LSP is gonna be released soon. There is currently a PR for it here: https://github.com/sublimelsp/LSP/pull/2151

rchl commented 1 year ago

New versions of LSP and LSP-typescript were released.

JGJP commented 1 year ago

So is the alternative to use LSP: Show Call Hierarchy? It's quite different from what we had before...

Going to have to downgrade again for now

rchl commented 1 year ago

Relevant commentary: https://xkcd.com/1172/

Previous implementation was a hack and did not work anything like the spec intended call hierarchy to work. It's cool that you found it useful but i think new way is an improvement (sure, it has keyword accessibility issue due to ST limitations).

If you know LSP code well enough then you could add a custom sublime command to re add this functionality without downgrading. You'll lose any future improvements to LSP-typescript and underlying typescript server if you decide to downgrade.

JGJP commented 1 year ago

Would a pull request adding a separate command for this be welcomed? I get that the xkcd could be seen as relevant but all I and my devs want to do is to press a key and go to wherever a symbol is referenced. The new API opens up a tab on the side and I need to click around there to even expand each file? And then close the tab when I'm done? Not ideal but please let me know if there's something I'm missing. LSP: Show Call Hierarchy doesn't even work for variables, only functions (AFAICT)

JGJP commented 1 year ago

Just looking for lsp_symbol_definition in reverse

rwols commented 1 year ago

I and my devs want to do is to press a key and go to wherever a symbol is referenced.

LSP: Find References?

rchl commented 1 year ago

It's mentioned in the earlier comment that "find references" doesn't work the same but I'd like to know exact case where it doesn't so that I can understand what you are missing exactly. Can you provide either a repo and steps to reproduce or at least write in a comments what files are involved and with what content.

The old "Find callers" functionality is no longer available in TLS so it might no longer be possible to reproduce what it did but I can't say for sure.

JGJP commented 1 year ago

Find Callers offered this kind of functionality, which was very pleasant:

Last part is crucial I guess, as most of the time I'm just looking around at how my subject variable/function is being used.

Find References:

rwols commented 1 year ago

There’s a boolean option in the LSP settings to use a quick panel for Find References. Maybe that approximates your workflow suitably?

JGJP commented 1 year ago

Ah, this seems to be exactly what I wanted, thanks a lot! I don't know how I missed this on my end. I've now upgraded to the latest LSP-Typescript. Looking forward to getting the intended utility out of LSP: Show Call Hierarchy

jwortmann commented 1 year ago

There’s a boolean option in the LSP settings to use a quick panel for Find References.

I wonder if the default value for this should be changed to use the quick panel. I find the quick panel for this much more convenient than the output panel. I assume the output panel was designed after Sublime's "Find in Files", but if you use the built-in "Goto Reference..." from ST, then it will also use the quick panel if there are multiple locations. So using the quick panel for LSP "Find References" would actually be in line with default ST behavior. To reduce confusion for users you could add to the next changelog message:

Breaking:

  • The "LSP: Find References" feature now uses the quick panel by default, instead of the output panel at the bottom. To revert to the former behavior, adjust the "show_references_in_quick_panel" setting.

Regarding "LSP: Show Call Hierarchy", yes it is only for functions and it's primarily to explore the hierarchy of which functions are involved, i.e. I think it can be more useful if you want to get a quick overview. So it can show multiple levels of function calls, while "Find References" only shows direct references to a particular function. See also https://code.visualstudio.com/updates/v1_33#_call-hierarchy. But to be honest, I also haven't used it much in practice. There is also a new "Show Type Hierarchy" for child and parent classes or similar, if the language server supports it (I think not supported by this server).

rchl commented 1 year ago

I wonder if the default value for this should be changed to use the quick panel.

I do believe so.

But I still find the panel approach useful in some cases when I want to keep a list of references and go through them one by one. But I still believe that the default should be the quick panel.

JGJP commented 1 year ago

Regarding "LSP: Show Call Hierarchy", yes it is only for functions and it's primarily to explore the hierarchy of which functions are involved, i.e. I think it can be more useful if you want to get a quick overview. So it can show multiple levels of function calls, while "Find References" only shows direct references to a particular function.

I've found myself reaching for this a few times so far, but just found a couple aspects of it lacking:

  1. if I click on one of the files it just brings me to the calling scope, and not to an actual line that's making the call (I can see it might have been done this way because there might be multiple calls, still...)
  2. the hierarchy is folded by default more than 2 levels deep and I need to click
jwortmann commented 1 year ago

Good feedback.

  1. if I click on one of the files it just brings me to the calling scope, and not to an actual line that's making the call (I can see it might have been done this way because there might be multiple calls, still...)

Yes, I have also thought about this. I see now that VSCode highlights the line where the call occurs (first occurence in case of multiple calls). I used the CallHierarchyItem's selectionRange for the location, but now I think the CallHierarchyIncomingCall's fromRanges would be the right choice. I will test it and see if I can make a PR later, or on the weekend.

For the outgoing calls (i.e. after you clicked the small toggle button), VSCode does show the definition location (not where the call occurs), similar to what this client does now. I think this should be okay.

  1. the hierarchy is folded by default more than 2 levels deep and I need to click

Hm, how many levels would you unfold the hierarchy by default? Note that the results are fetched from the language server on demand, i.e. each time you click on the disclosure icon to expand a new entry, there will be a request to the server.

Also, for example the folders in the sidebar, or in the file manager of the OS are folded by default too. And if we show more than the first level unfolded by default, it could become unclear for functions with many calls, and might not fit onto the screen.

JGJP commented 1 year ago

Hm, how many levels would you unfold the hierarchy by default? Note that the results are fetched from the language server on demand, i.e. each time you click on the disclosure icon to expand a new entry, there will be a request to the server.

Could I suggest that this be set as a preference by the user? or an arg that can be passed with the command? If it had to be a default that would work for everybody then it gets a bit more difficult, but maybe something like just don't show more than 30 rows or something, regardless of the hierarchy itself. I think this would be useful enough for limiting the amount of work that it will do at one time to show results.