Merge definitions from all plugins for Document(Type)Definition message #3846

JiriLojda · 2023-10-16T06:38:04Z

This PR fixes issue #3634 by merging the definition responses from all plugins into one definition response. It (only when necessary) transforms Location to LocationLink as suggested by @michaelpj in the issue.
I didn't refactor the type the plugins return as suggested by @joyfulmantis, because I think it will be better to address it in a separate PR and have a straightforward fix merged before that.

fendor

LGTM, thank you for the addition!

One nitpick and a little bit more documentation, looks good to me as is otherwise :)

fendor · 2023-10-16T07:22:15Z

hls-plugin-api/src/Ide/Types.hs

+mergeDefinitions :: Definitions -> Definitions -> Definitions
+mergeDefinitions definitions1 definitions2 = case (definitions1, definitions2) of


Some haddock explaining what this does, especially what the merging rules are would be great!

fendor · 2023-10-16T07:26:38Z

hls-plugin-api/src/Ide/Types.hs

+        defToLinks (Definition (InL location)) = [DefinitionLink $ locationToLocationLink location]
+        defToLinks (Definition (InR locations)) = map (DefinitionLink . locationToLocationLink) locations
+
+        locationToLocationLink :: Location -> LocationLink


Nitpick

How about:

Suggested change

locationToLocationLink :: Location -> LocationLink

locationToDefinitionLink :: Location -> DefinitionLink

Since you use locationToLocationLink exclusively to create a DefinitionLink.

michaelpj

I know we don't have tests for any of the other response combination, but if you felt motivated it would be great to start some!

michaelpj · 2023-10-16T09:31:38Z

hls-plugin-api/src/Ide/Types.hs

+        defToLinks (Definition (InR locations)) = map (DefinitionLink . locationToLocationLink) locations
+
+        locationToLocationLink :: Location -> LocationLink
+        locationToLocationLink Location{_uri, _range} = LocationLink{_originSelectionRange = Just _range, _targetUri = _uri, _targetRange = _range, _targetSelectionRange = _range}


This is wrong, originSelectiomRange is quite different and we can't come up with a definition for it, so we should put Nothing.

michaelpj · 2023-10-16T09:33:30Z

hls-plugin-api/src/Ide/Types.hs

+type Definitions = (Definition |? ([DefinitionLink] |? Null))
+
+mergeDefinitions :: Definitions -> Definitions -> Definitions
+mergeDefinitions definitions1 definitions2 = case (definitions1, definitions2) of


There's are client capabilities controlling whether location links are supported. Ideally we should check those and if they're not sent we should instead downgrade the LocationLinks.

I created a new downgradeLinks function instead and used it to downgrade all LocationLinks in the implementation of combineResponses when required by the missing capability.

JiriLojda · 2023-10-16T10:40:46Z

Thank you both for the review. I will address the comments as soon as possible, most likely this week.

- enables multiple plugins to provide Document(Type)Definition for the same message

…nLink in combineResponses of plugins to TextDocumentDefinition message

…on tests

…eResponses (plugin API) - Upgrade locations to links only when necessary (some responses are links)

JiriLojda · 2023-10-30T10:14:21Z

Oh, it seems I was too eager with the new extensions. I thought I could use GHC 9.6 (probably since it is in the snapshot in the default stack.yml). I'll fix that.

- to support GHC < 9.2

michaelpj

Code looks good, some of this stuff probably belongs in lsp, but we can move that later if we want.

Haven't reviewed the tests yet, but happy to merge regardless.

michaelpj · 2023-11-07T11:03:46Z

Build failures are genuine.

JiriLojda · 2023-11-13T21:23:50Z

I suspect that thanks to the CI cache cabal doesn't know about the focus@1.0.3.2 which is necessary for the build to work on ghc@9.8 (the cause of the previous build failures). I will find a way to clear the cache later.

By the way, do you generally use the cabal-fmt? It is configured to run with stylish-haskell, but all the cabal files I format are completely changed. I commit it anyway because I suppose this is the correct format that we should follow, but I wonder why it wasn't formatted before and also worry that I will cause someone ugly merge conflicts with the changes. :)

michaelpj · 2023-11-15T12:40:49Z

CI issues should be worked out, can you fix the conflicts and try again?

michaelpj · 2023-11-17T09:33:22Z

Thanks!

JiriLojda · 2023-11-17T09:39:26Z

Great, thank you for your help!

* hls-notes-plugin: Initial implementation * hls-notes-plugin: add to feature list and plugin table * hls-notes-plugin: Add more documentation comments * hls-notes-plugin: Fix tests after #3846, add CI job * hls-notes-plugin: Address review comments * hls-notes-plugin: Allow Note definition within single line comments * hls-notes-plugin: Improve "Note not found" error message * hls-notes-plugin: Allow single line notes to be indented * treewide: Add missing underscores to note definitions * hls-notes-plugin: Wait until HLS is done in tests * hls-notes-plugin: Fix tests on windows The regex did not allow windows line endings in note definitions --------- Co-authored-by: Jan Hrcek <2716069+jhrcek@users.noreply.github.com> Co-authored-by: fendor <fendor@users.noreply.github.com>

JiriLojda requested a review from berberman as a code owner October 16, 2023 06:38

fendor requested review from joyfulmantis and fendor October 16, 2023 07:20

fendor approved these changes Oct 16, 2023

View reviewed changes

michaelpj requested changes Oct 16, 2023

View reviewed changes

JiriLojda requested a review from pepeiborra as a code owner October 30, 2023 09:06

JiriLojda force-pushed the definition_from_multiple_plugins branch from 295f25b to a3bf0bf Compare October 30, 2023 09:09

JiriLojda added 7 commits October 30, 2023 10:11

Merge definitions from all plugins for Document(Type)Definition message

e57c19c

- enables multiple plugins to provide Document(Type)Definition for the same message

Remove unnecessary head usage in ghcide TestUtils

613fd5a

Use Nothing for original selection when upgrading Location to Locatio…

415ce13

…nLink in combineResponses of plugins to TextDocumentDefinition message

Share combineResponses document definition and document type definiti…

941b8f4

…on tests

Downgrade locations to links when missing client capability in combin…

b7ee494

…eResponses (plugin API) - Upgrade locations to links only when necessary (some responses are links)

Test preserving link data in combineResponses of Definition message

b696f09

Add haddock to mergeDefinitions in plugin API

f1a69a5

JiriLojda force-pushed the definition_from_multiple_plugins branch from a3bf0bf to f1a69a5 Compare October 30, 2023 09:11

JiriLojda requested a review from michaelpj October 30, 2023 09:15

JiriLojda added 2 commits October 30, 2023 18:53

Replace usage of OverloadedRecordDot with lenses

8b1eb65

- to support GHC < 9.2

Add TypeFamilies extension to TypesTests to support GHC < 9.4

08ccbd4

michaelpj approved these changes Nov 7, 2023

View reviewed changes

Merge branch 'master' into definition_from_multiple_plugins

6751e93

michaelpj added the merge me Label to trigger pull request merge label Nov 7, 2023

fendor removed the merge me Label to trigger pull request merge label Nov 7, 2023

Require focus >= 1.0.3.2 to fix 9.8 build for ghcide and hls-graph

27caea4

JiriLojda added 2 commits November 15, 2023 20:08

Merge branch 'master' into definition_from_multiple_plugins

30bfe49

Merge branch 'master' into definition_from_multiple_plugins

d04091e

michaelpj merged commit 20a37ec into haskell:master Nov 17, 2023
40 checks passed

JiriLojda deleted the definition_from_multiple_plugins branch November 17, 2023 09:39

This was referenced Nov 29, 2023

hls-notes-plugin: Initial implementation #3629

Closed

Plugins cannot define jump-to-definition handlers (responses are not combined) #3634

Closed

jvanbruegge added a commit to jvanbruegge/haskell-language-server that referenced this pull request Dec 1, 2023

hls-notes-plugin: Fix tests after haskell#3846, add CI job

5120417

Porkepix mentioned this pull request Dec 2, 2023

haskell-language-server 2.5.0.0 Homebrew/homebrew-core#156170

Closed

jvanbruegge added a commit to jvanbruegge/haskell-language-server that referenced this pull request Feb 27, 2024

hls-notes-plugin: Fix tests after haskell#3846, add CI job

162e0a3

jvanbruegge added a commit to jvanbruegge/haskell-language-server that referenced this pull request Feb 27, 2024

hls-notes-plugin: Fix tests after haskell#3846, add CI job

d750e79

jvanbruegge added a commit to jvanbruegge/haskell-language-server that referenced this pull request Feb 28, 2024

hls-notes-plugin: Fix tests after haskell#3846, add CI job

3c6d8fc

jvanbruegge added a commit to jvanbruegge/haskell-language-server that referenced this pull request Mar 9, 2024

hls-notes-plugin: Fix tests after haskell#3846, add CI job

3428f6c

jvanbruegge added a commit to jvanbruegge/haskell-language-server that referenced this pull request Mar 9, 2024

hls-notes-plugin: Fix tests after haskell#3846, add CI job

07dd6db

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge definitions from all plugins for Document(Type)Definition message #3846

Merge definitions from all plugins for Document(Type)Definition message #3846

JiriLojda commented Oct 16, 2023

fendor left a comment

fendor Oct 16, 2023

fendor Oct 16, 2023

michaelpj left a comment

michaelpj Oct 16, 2023

michaelpj Oct 16, 2023

JiriLojda Oct 30, 2023

JiriLojda commented Oct 16, 2023

JiriLojda commented Oct 30, 2023

michaelpj left a comment

michaelpj commented Nov 7, 2023

JiriLojda commented Nov 13, 2023

michaelpj commented Nov 15, 2023

michaelpj commented Nov 17, 2023

JiriLojda commented Nov 17, 2023

		mergeDefinitions :: Definitions -> Definitions -> Definitions
		mergeDefinitions definitions1 definitions2 = case (definitions1, definitions2) of

	locationToLocationLink :: Location -> LocationLink
	locationToDefinitionLink :: Location -> DefinitionLink

Merge definitions from all plugins for Document(Type)Definition message #3846

Merge definitions from all plugins for Document(Type)Definition message #3846

Conversation

JiriLojda commented Oct 16, 2023

fendor left a comment

Choose a reason for hiding this comment

fendor Oct 16, 2023

Choose a reason for hiding this comment

fendor Oct 16, 2023

Choose a reason for hiding this comment

michaelpj left a comment

Choose a reason for hiding this comment

michaelpj Oct 16, 2023

Choose a reason for hiding this comment

michaelpj Oct 16, 2023

Choose a reason for hiding this comment

JiriLojda Oct 30, 2023

Choose a reason for hiding this comment

JiriLojda commented Oct 16, 2023

JiriLojda commented Oct 30, 2023

michaelpj left a comment

Choose a reason for hiding this comment

michaelpj commented Nov 7, 2023

JiriLojda commented Nov 13, 2023

michaelpj commented Nov 15, 2023

michaelpj commented Nov 17, 2023

JiriLojda commented Nov 17, 2023