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

[READY] Implement LSP type/call hierarchies as actual hierarchies #1733

Merged
merged 13 commits into from
Jun 18, 2024

Conversation

bstaletic
Copy link
Collaborator

@bstaletic bstaletic commented Feb 11, 2024

This is the implementation of the server side changes needed to have useful support for LSP call/type hierarchies.
While the clien side changes deserve more attention, the server side should be fine to review and merge.

What's definitely missing are tests and API documentation.
On top of that, what do we want to do with the old GoToCollers/GoToCallees? I see three options:

  • Remove them completely, which might break some users, but the new alternative should be better.
  • Make them an alias for the new subcommands. This option wouldn't break users, but would break clients who would not adapt.
  • Do nothing and maintain the old API forever, like with have done with other things in the past.

This change is Reviewable

@puremourning
Copy link
Member

I need to schedule some time to play with this poc

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic)

a discussion (no related file):
Ideally, we would also update the openapi.yaml for the docs. No need to duplicate anything that's in the LSP spec, but we maybe should document our modifications/normalisattions on our ycmd API.


Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 7 files at r3, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic)

@bstaletic bstaletic force-pushed the hierarchies branch 2 times, most recently from e13a04a to a4b4bdc Compare June 9, 2024 19:39
Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 15 of 15 files at r4, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic)

@bstaletic bstaletic changed the title [RFC] Implement LSP type/call hierarchies as actual hierarchies [READY] Implement LSP type/call hierarchies as actual hierarchies Jun 9, 2024
Copy link
Collaborator Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

This should finally be ready.
If only Microsoft had not borked windows-2022 C runtime...

Reviewable status: 1 of 2 LGTMs obtained (waiting on @puremourning)

a discussion (no related file):

Previously, puremourning (Ben Jackson) wrote…

Ideally, we would also update the openapi.yaml for the docs. No need to duplicate anything that's in the LSP spec, but we maybe should document our modifications/normalisattions on our ycmd API.

Done.


Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @bstaletic)


ycmd/completers/language_server/language_server_completer.py line 3413 at r5 (raw file):

    file_contents = []

  range = location.get( 'selectionRange' ) or location[ 'range' ]

I think this might be surprising when used in other contexts. There are other request that have 'selectionRange' , such as "DocumentSymbol" In that case I'm not 100% sure that 'selectionRange' is better than 'range' for our purposes:

	/**
	 * The range enclosing this symbol not including leading/trailing whitespace
	 * but everything else like comments. This information is typically used to
	 * determine if the clients cursor is inside the symbol to reveal in the
	 * symbol in the UI.
	 */
	range: Range;

	/**
	 * The range that should be selected and revealed when this symbol is being
	 * picked, e.g. the name of a function. Must be contained by the `range`.
	 */
	selectionRange: Range;

perhaps we should pass a 'range_property="range"' argument to this function instead?

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 7 files at r5, all commit messages.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @bstaletic)

Copy link
Collaborator Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)


ycmd/completers/language_server/language_server_completer.py line 3413 at r5 (raw file):

Previously, puremourning (Ben Jackson) wrote…

I think this might be surprising when used in other contexts. There are other request that have 'selectionRange' , such as "DocumentSymbol" In that case I'm not 100% sure that 'selectionRange' is better than 'range' for our purposes:

	/**
	 * The range enclosing this symbol not including leading/trailing whitespace
	 * but everything else like comments. This information is typically used to
	 * determine if the clients cursor is inside the symbol to reveal in the
	 * symbol in the UI.
	 */
	range: Range;

	/**
	 * The range that should be selected and revealed when this symbol is being
	 * picked, e.g. the name of a function. Must be contained by the `range`.
	 */
	selectionRange: Range;

perhaps we should pass a 'range_property="range"' argument to this function instead?

I was considering that too. I was not sure about DocumentSymbol either.

Implemented your suggestion. That should also generalize to originSelectionRange and targetSelectionRange, were we ever to need a LocationLink LSP type.

```go
package main

func f() (int) {
    return 5;
}
func g() (int) {
    return f() + f();
}
func h() (int) {
    var x = g();
    return f() + x;
}
```

With incoming calls request on `g()`, the response should be something
like this

```python
{
  'locations': [ location_of_call_to_g_inside_h ],
  'root_location': location_of_h_function_name
}
```

This allows clients to:

1. Jump to the actual call site, using `locations` list.
2. Switch to outgoing calls, by preparing a new hierarchy, using
   `root_location` object.
…ion-like objects

This fixes all of the remaining bugs when requesting outgoing calls of
an incoming call hierarchy item.

Specifically, we *want* `selectionRange` for the hierarchy root and for
`root_location` of incoming call hierarchy items.

JDT is special and needs `range` in the second case.
Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 15 of 15 files at r6, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic)


ycmd/completers/typescript/typescript_completer.py line 823 at r6 (raw file):

  def _Hierarchy( self, request_data, args ):
    self._Reload( request_data )

is this reload needed? I suspect that we can rely on the reload from the prepare step?


ycmd/tests/typescript/subcommands_test.py line 1203 at r6 (raw file):

  @SharedYcmd
  def test_Subcommands_OutgoingCallHierarchy( self, app ):

I take it there is no type hierarchy for typescript?

Copy link
Collaborator Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

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

Dismissed @puremourning from a discussion.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)


ycmd/completers/typescript/typescript_completer.py line 823 at r6 (raw file):

Previously, puremourning (Ben Jackson) wrote…

is this reload needed? I suspect that we can rely on the reload from the prepare step?

Good point.
I started thinking about a potential scenario where a client would keep the hierarchy tree around and then after editing stuff go back to it, but

  1. Then _Reload() would be called for a different reason anyway.
  2. If the call site changes the hierarchy will be wrong.

Dropped the reload.


ycmd/tests/typescript/subcommands_test.py line 1203 at r6 (raw file):

Previously, puremourning (Ben Jackson) wrote…

I take it there is no type hierarchy for typescript?

That's right.

@bstaletic bstaletic added the Ship It! Manual override to merge a PR by maintainer label Jun 17, 2024
Copy link
Contributor

mergify bot commented Jun 18, 2024

Thanks for sending a PR!

@mergify mergify bot merged commit e81c15e into ycm-core:master Jun 18, 2024
14 of 15 checks passed
@bstaletic bstaletic deleted the hierarchies branch June 18, 2024 01:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ship It! Manual override to merge a PR by maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants