microsoft / language-server-protocol

Defines a common protocol for language servers.
https://microsoft.github.io/language-server-protocol/
Creative Commons Attribution 4.0 International
11.09k stars 776 forks source link

Clarifications for the intended type hierarchy behaviour #1984

Open vinistock opened 1 month ago

vinistock commented 1 month ago

We are starting to ship type hierarchy for Ruby in the Ruby LSP and we hit a few doubts about the intended experience is on certain cases. I'm going to divide the doubts below so that I can provide a detailed explanation for each.

Re-opening of an existing declaration

In Ruby, it's common to re-open the same namespace in different files, adding more declarations inside of it. For example:

# file1.rb
module Foo
  class Bar
  end
end

# file2.rb
module Foo
  class Bar
  end
end

# In this scenario, the declarations of both `Foo` and `Bar` are re-opened in the two files

A TypeHierarchyItem only supports a single document URI attached to it.

What is the expectation in this case? Should the server return the complete type hierarchy information, but pick one of the document URIs? Or should all re-openings be returned in the list?

Is this a limitation and multiple document URIs should be allowed for type hierarchy items?

Returning more than one item in the prepare request

The prepare request mentions the following: ...return a type hierarchy for the language element of given text document positions. However, it seems that only a single position is sent as part of the request parameters, but the response expected is a list of entries.

While trying to understand what to do for the doubt above this one, we attempted to return all re-openings of the same namespace in the prepare request, but noticed that it doesn't work. The only way to make it work is to return only one item inside of the response array.

Is this intentional? And if so, should we document that in the spec?

Show linearized type hierarchy or only direct ancestors

Another doubt that came up regarding the user experience is whether we should display the linearized ancestor chain or show only the direct chain. For example:

module Foo; end
module Bar
  # Add Foo as an ancestor of Bar
  include Foo
end

class Baz
  # Add Bar as an ancestor of Baz
  include Bar
end

In this example, the direct ancestor chain of Baz is just [Bar, Bar, Object] (Object is implicitly the parent class of all Ruby classes).

The linearized ancestor chain is [Baz, Bar, Foo, Object, Kernel, BasicObject], which includes the linearized ancestors of each ancestor present in the chain for Baz on top of the class itself.

Is the intention behind the feature to display the linearization or only the direct ancestors of a namespace? Or is it implementors choice?

Singleton classes

Finally, the last doubt is concerning Ruby's singleton class ancestors. In Ruby, every class or module has the regular ancestors and the singleton ancestors.

The regular ancestor chain is where instance methods are defined and searched for. The singleton ancestor chain is where static (class) methods are defined and searched for. For example:

class Foo
end

# Ancestors of Foo
[Foo, Object, Kernel, BasicObject]

# Ancestors of the singleton class of Foo
[<Class:Foo>, <Class:Object>, <Class:BasicObject>, Class, Module, Object, Kernel, BasicObject]

The distinction is important because Ruby developers can add ancestors that will only exist in the regular chain or in the singleton chain (through include or extend).

class Foo
  include IncludedModule
  extend ExtendedModule
end

# Ancestors of Foo
[Foo, IncludedModule, Object, Kernel, BasicObject]

# Ancestors of the singleton class of Foo
[<Class:Foo>, ExtendedModule, <Class:Object>, <Class:BasicObject>, Class, Module, Object, Kernel, BasicObject]

I'm not sure this concept exists for other languages, but for Ruby it would make perfect sense to have two actions: show attached type hierarchy (the regular one) and show singleton type hierarchy.

What is the recommendation to handle this case? Do we implement this as a custom menu item for the Ruby extension only? Is there interest in having something generic to handle these multiple hierarchy chains associated with the same namespace? Should we always display both at the same time in the UI?

dbaeumer commented 1 month ago

I think there isn't a single answer to this since it depends on how these concepts are implemented in the programming language. For example if a class extends another class, can't the re-open inherit from a different class. Does it need to enumerate the same extends, implements, ....

If for example on one declaration can extends / implements I would take that as a primary declaration and would support navigation to other re-opens from the primary declaration.

So I would experiment with different implementations and gather feedback from the users to understand what they need to get visualized.

Regarding the prepare request: I opened https://github.com/microsoft/vscode/issues/224944 since this looks like something VS Code needs to handle better.

vinistock commented 1 month ago

@dbaeumer thank you for the feedback, I appreciate your thoughts on it.

Since the ancestor chains for Ruby end up being so rich and complex, the challenge is indeed figuring out how to display it in a way that's intuitive and useful.

For the namespace re-openings, what are your thoughts on making VS Code accept multiple URIs for the same namespace? So then, when a user clicked on that namespace, they would see a list of possible URIs to open.

For reference, here is the current behaviour. Notice how Kernel ends up appearing many many times. This is because the Kernel module is included into every object in Ruby and it's a somewhat common pattern to re-open it for making more methods available globally.

There's not a lot of value in showing all of these duplicate entries for Kernel. Expanding them shows the exact same ancestor chain. However, they are separate definitions made in different files, so simply selecting the first file and ignoring the rest would not communicate to the user that the module is being re-opened multiple times.

image
dbaeumer commented 4 weeks ago

For the namespace re-openings, what are your thoughts on making VS Code accept multiple URIs for the same namespace?

This is something we would need to discuss with the VS Code.

This being said from a UI perspective I don't thing it is nice, since you only see the re-opening when click on an entry in a second step. Have you tried to add an Kernel node with all the re-openings as sub nodes. One could argue that Kernel implements all re-openings.

vinistock commented 3 weeks ago

Have you tried to add an Kernel node with all the re-openings as sub nodes

I think I'm getting a bit more clarity. At the core of my confusion is what the intended meaning for sibling vs nested nodes are. When you think about the relationship between a sibling vs a nested node, what meaning do you assign to it?

For example, is this how you think about the feature?

# Foo inherits from Bar because they are sibling nodes
- Foo
- Bar

# In that case, what do nested nodes mean? Are they meant for re-openings of the same type?
- Foo
  - Foo (defined elsewhere) # A re-opening
  - Bar # What would this mean? Is this straight out incorrect usage of the spec?
dbaeumer commented 3 weeks ago

Sorry, there was a typo in my last statement: Have you tried to add an Kernel node with all the re-openings as SUPER nodes. One could argue that Kernel implements all re-openings.

That would mean that if you want to see all reopening you switch to super types. You would however need to decorate the node somehow (in a first step textual) to indicate that the type has re-opening.