Implement selection range request lsp feature

[sir4ur0n]

Hi,

I started toying with the "Expand selection" (Alt + Shift + Right arrow by default on Linux) feature in VS Code and I noticed some quirks here and there. As usual, I am unsure if this is an HLS or Ghcide issue, sorry 😅

To reproduce:

{-# LANGUAGE DataKinds #-}
module Foo where

import Prelude

newtype Foo = Foo {
    foo :: String
}

bar :: String -> Foo
bar "" = undefined
bar s = Foo {
    foo = s -- start with your cursor on this line, somewhere on the foo field
}

baz :: Bool -> ()
baz _ = ()

When using "Expand selection" feature several times, here's what happens:

1. Selects foo
2. Selects foo = s -- start with your cursor on this line, somewhere on the foo field
3. Selects foo = s -- start with your cursor on this line, somewhere on the foo field (the left spaces are now included, but Github doesn't display them...)
4. Selects everything between the brackets excluded (including new lines)
5. Selects everything between the brackets included
6. 💥 Selects the pattern case of bar (i.e. does not include the empty string case)
7. 💥 Selects the whole file

IMHO 6 is weird, there should be additional steps:

• Between 5 and 6: Select the whole Foo {} expression
• TBD: Between 6 and 7: Select the whole implementation but not the signature (I am much less opinionated on this one)
• Between 6 and 7: Select the whole implementation, including the signature
• Between 6 and 7: Select all declarations (types, functions, etc.) but not imports
• Between 6 and 7: Select all declarations (types, functions, etc.) and imports, but not module declaration

[lukel97]

At first I was about to put this down as just a vscode thing but apparently it does use LSP? https://code.visualstudio.com/updates/v1_31#_smart-selections

[sir4ur0n]

From what I understand, yes, it relies on LSP. Also, I think it makes sense, since it's really language-dependent to know what token spans are "bigger than/including" smaller token spans. E.g. a language/framework may use an opening bracket { without closing one, and only an LSP server can be aware of that, not VS Code globally

[lukel97]

I'm wondering what specific LSP features it uses though, since as far as I'm aware there's no "smart selection" language feature in LSP. Perhaps it's using the semantic highlighting stuff?

[Avi-D-coder]

@bubba There is now, @maklad championed it for rust-analyzer.
https://microsoft.github.io/language-server-protocol/specification#textDocument_selectionRange
microsoft/language-server-protocol#613

[lukel97]

@Avi-D-coder nice! I think we should be able to implement this seeing as it just looks like a matter of dealing with the parsed ast. Perhaps we can add it at the ghcide level

[alok]

@bubba what would have to be changed to implement this? i'm a big fan of this feature in every language i use.

[jneira]

@alok
haskell-lsp supports that feature: https://github.com/haskell/lsp/blob/5422dd13f0362917e5981e07d60617ca6e233833/lsp-types/src/Language/LSP/Types/SelectionRange.hs

and the link to rust code handling SelectionRange could be helpful to do the same here:
https://github.com/rust-analyzer/rust-analyzer/blob/3b02aec0cce563e21c3545d434c14df17b1d392a/crates/rust-analyzer/src/handlers.rs#L154-L195

matter of dealing with the parsed ast.

So we should analyze the ghc parsed ast and match the desired boundaries (based on delimiters ans significant whitespace)

[kokobd]

@jneira I'm new to this codebase and trying to implement this feature. If I understand it correctly, a new lsp handler should be added to ideHandlers of Development.IDE.LSP.LanguageServer module? Because STextDocumentSelectionRange is not an instance of PluginMethod typeclass, it can not be added as a PluginHandler.

[jneira]

@kokobd thanks for planning work in hls, i would say this is a core feature with one possible useful implementation which make sense to have in ghcide. But maybe it could be added to plugin methods too.
Maybe @michaelpj and @pepeiborra could help us with the analysis.

[michaelpj]

I think it shouldn't be problematic to add an implementation of PluginMehtod for the selection range method. combineResposes is a bit questionable, since I'm not sure we can combine them (the indices in the result list mean something). (Actually, quite a few of the implementations have questionable combineResponses...)

[michaelpj]

It would be nice to have a way to give an error from combineResponses if they can't be combined, as it is it seems like a bunch of the implementations just arbitrarily take the first thing. Probably out of scope, however.

[pepeiborra]

For this LSP feature, if the responses are not combinable, it probably doesn't make sense to build up an HLS Plugin infrastructure. Instead, just register an LSP handler directly in ghcide as done for other core features:

haskell-language-server/ghcide/src/Development/IDE/LSP/HoverDefinition.hs

Lines 56 to 66 in 54afb31

	setIdeHandlers :: LSP.Handlers (ServerM c)
	setIdeHandlers = mconcat
	[ requestHandler STextDocumentDefinition $ \ide DefinitionParams{..} ->
	gotoDefinition ide TextDocumentPositionParams{..}
	, requestHandler STextDocumentTypeDefinition $ \ide TypeDefinitionParams{..} ->
	gotoTypeDefinition ide TextDocumentPositionParams{..}
	, requestHandler STextDocumentDocumentHighlight $ \ide DocumentHighlightParams{..} ->
	documentHighlight ide TextDocumentPositionParams{..}
	, requestHandler STextDocumentReferences references
	, requestHandler SWorkspaceSymbol wsSymbols
	]

[michaelpj]

For this LSP feature, if the responses are not combinable, it probably doesn't make sense to build up an HLS Plugin infrastructure.

I don't see why not. We already have several non-combinable methods in there, notably the document formatting methods, which just take the first response from the list!

haskell-language-server/hls-plugin-api/src/Ide/Types.hs

Line 189 in 54afb31

combineResponses _ _ _ _ (x :| _) = x

[jneira]

yeah but agree with @pepeiborra: if the feature only has one possible sensible implementation, it is better to hardcode it in ghcide itself, why do you think it would be better to place it in a plugin, code organization?

[michaelpj]

To be clear, my suggestion for combineResponses is that in some cases we might want to give an error, e.g. "you have enabled two plugins that give responses for method X, turn one of them off".

why do you think it would be better to place it in a plugin, code organization?

Yeah, basically. I don't know that we have a hard and fast policy for what goes in plugins, but I'd been assuming we want to do as much as possible to stop growing ghcide more, but maybe that's not right 🤷 Maybe if we think it'll only ever have one implementation and it doesn't pull in new deps, we can just shove it in ghcide.

[pepeiborra]

Plugins do enable more gradual upgrades when a new GHC version is released. I have been working on upgrading the exactprint code actions for the last couple of weeks, and wishing they had been factored out of ghcide already

[kokobd]

There doesn't seem to be a sensible way to combine responses. But as different users might prefer different paces for the "expand selection" action, I think leaving a posibility to override the behavior in plugins is not bad.
To allow the posiblilty to override, maybe this feature can be added as a default plugin in ghcide (like those existing ones below). Implemetation of combineResponses would simply take the first response from the list then.

haskell-language-server/ghcide/src/Development/IDE/Plugin/HLS/GhcIde.hs

Lines 22 to 32 in ee0a0cc

	descriptors :: [PluginDescriptor IdeState]
	descriptors =
	[ descriptor "ghcide-hover-and-symbols",
	CodeAction.iePluginDescriptor "ghcide-code-actions-imports-exports",
	CodeAction.typeSigsPluginDescriptor "ghcide-code-actions-type-signatures",
	CodeAction.bindingsPluginDescriptor "ghcide-code-actions-bindings",
	CodeAction.fillHolePluginDescriptor "ghcide-code-actions-fill-holes",
	Completions.descriptor "ghcide-completions",
	TypeLenses.descriptor "ghcide-type-lenses",
	Notifications.descriptor "ghcide-core"
	]

[michaelpj]

To allow the posiblilty to override, maybe this feature can be added as a default plugin in ghcide (like those existing ones below). Implemetation of combineResponses would simply take the first response from the list then.

I think that's fine for now. We can worry about the weird behaviour of combineResponses separately.