Update version while editing to conform lsp spec #3566

July541 · 2023-04-19T14:22:59Z

Tests is broken due to lsp-test's limitation.

michaelpj · 2023-04-22T15:52:23Z

I confess I'm very hazy about how file versions are supposed to work at all, and the LSP spec doesn't really tell you. But shouldn't there be somewhere in the code where you bump the version?

July541 · 2023-04-22T15:55:06Z

I confess I'm very hazy about how file versions are supposed to work at all, and the LSP spec doesn't really tell you. But shouldn't there be somewhere in the code where you bump the version?

I've consulted with the client developer, see details here helix-editor/helix#6543 (comment)

michaelpj · 2023-04-22T16:30:19Z

Okay, sounds good. Maybe we should write something about how to use it? Or audit the other uses now you know what we should be doing?

July541 · 2023-04-23T12:20:19Z

I think we can add version checking in lsp-test for every editing after haskell/lsp#474 addressed.

July541 · 2023-04-23T12:23:09Z

Ideally, I want we can track version info in the plugin descriptor, then we can remove some parameters passing, but it's not easy.

michaelpj

Okay, I think I'm convinced that this is the right thing to do. It would be nice to write down the logic somewhere, I'm not sure where. Alternatively, if we go the route of passing the VTDI everywhere instead of reconstructing it, we could add a hlint rule that warns if people use the constructor?

michaelpj · 2023-06-10T11:33:11Z

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

@@ -161,16 +161,16 @@ diffTextEdit fText f2Text withDeletions = J.List r


 -- | A pure version of 'diffText' for testing
-diffText' :: Bool -> (Uri,T.Text) -> T.Text -> WithDeletions -> WorkspaceEdit
-diffText' supports (f,fText) f2Text withDeletions  =
+diffText' :: Bool -> (Uri,T.Text) -> T.Text -> WithDeletions -> TextDocumentVersion -> WorkspaceEdit


Suggested change

diffText' :: Bool -> (Uri,T.Text) -> T.Text -> WithDeletions -> TextDocumentVersion -> WorkspaceEdit

diffText' :: Bool -> (VersionedTextDocmentIdentifier, T.Text) -> T.Text -> WithDeletions -> TextDocumentVersion -> WorkspaceEdit

what about something like this? Since really the idea is that we want to use the same identifier as the one we were given - at the moment we're taking the parts and then putting it back together, we could just take the identifier.

michaelpj · 2023-06-10T11:33:52Z

plugins/hls-class-plugin/src/Ide/Plugin/Class/Types.hs

@@ -38,6 +38,7 @@ data AddMinimalMethodsParams = AddMinimalMethodsParams
    , methodGroup :: List (T.Text, T.Text)
    -- ^ (name text, signature text)
    , withSig     :: Bool
+    , textVersion :: TextDocumentVersion


similarly, replace the uri field with a VerisonedTextDocumentIdentifier

michaelpj · 2023-06-10T11:34:25Z

plugins/hls-hlint-plugin/src/Ide/Plugin/Hlint.hs

@@ -451,8 +455,8 @@ codeActionProvider ideState pluginId (CodeActionParams _ _ documentId _ context)

 -- | Convert a hlint diagnostic into an apply and an ignore code action
 -- if applicable
-diagnosticToCodeActions :: DynFlags -> T.Text -> PluginId -> TextDocumentIdentifier -> LSP.Diagnostic -> [LSP.CodeAction]
-diagnosticToCodeActions dynFlags fileContents pluginId documentId diagnostic
+diagnosticToCodeActions :: DynFlags -> T.Text -> PluginId -> TextDocumentIdentifier -> TextDocumentVersion -> LSP.Diagnostic -> [LSP.CodeAction]


Suggested change

diagnosticToCodeActions :: DynFlags -> T.Text -> PluginId -> TextDocumentIdentifier -> TextDocumentVersion -> LSP.Diagnostic -> [LSP.CodeAction]

diagnosticToCodeActions :: DynFlags -> T.Text -> PluginId -> VersionedTextDocumentIdentifier -> LSP.Diagnostic -> [LSP.CodeAction]

michaelpj · 2023-06-10T11:34:47Z

plugins/hls-hlint-plugin/src/Ide/Plugin/Hlint.hs

@@ -511,13 +515,13 @@ mkSuppressHintTextEdits dynFlags fileContents hint =
    combinedTextEdit : lineSplitTextEditList
 -- ---------------------------------------------------------------------

-applyAllCmd :: Recorder (WithPriority Log) -> CommandFunction IdeState Uri
-applyAllCmd recorder ide uri = do
+applyAllCmd :: Recorder (WithPriority Log) -> CommandFunction IdeState (Uri, TextDocumentVersion)


Suggested change

applyAllCmd :: Recorder (WithPriority Log) -> CommandFunction IdeState (Uri, TextDocumentVersion)

applyAllCmd :: Recorder (WithPriority Log) -> CommandFunction IdeState VersionedTextDocumentIdentifier

michaelpj · 2023-06-10T11:34:56Z

plugins/hls-hlint-plugin/src/Ide/Plugin/Hlint.hs

  -- | There can be more than one hint suggested at the same position, so HintTitle is used to distinguish between them.
-  , hintTitle :: HintTitle
+  , hintTitle   :: HintTitle
+  , textVersion :: TextDocumentVersion


michaelpj · 2023-06-10T11:35:26Z

plugins/hls-hlint-plugin/src/Ide/Plugin/Hlint.hs

    logWith recorder Debug $ LogApplying file res
    case res of
      Left err -> pure $ Left (responseError (T.pack $ "hlint:applyOne: " ++ show err))
      Right fs -> do
        _ <- sendRequest SWorkspaceApplyEdit (ApplyWorkspaceEditParams Nothing fs) (\_ -> pure ())
        pure $ Right Null

-applyHint :: Recorder (WithPriority Log) -> IdeState -> NormalizedFilePath -> Maybe OneHint -> IO (Either String WorkspaceEdit)
-applyHint recorder ide nfp mhint =
+applyHint :: Recorder (WithPriority Log) -> IdeState -> NormalizedFilePath -> Maybe OneHint -> TextDocumentVersion -> IO (Either String WorkspaceEdit)


bit akward her because we've got a NFP here, but maybe we could get away with passing the VTDI instead?

maralorn · 2023-06-10T14:30:30Z

I can report that the PR in this form fixes code actions in helix for me.

@July541 Just so you know, I am trying to work on this here at zurihac. Will report back once I have made progress.

maralorn · 2023-06-11T20:36:28Z

Thank you, @July541 for doing most of the work on this. With #3643 brought over the finish line, this PR can be closed.

konn

LGTM foSplice Plugin 👍

Update version while editing to conform lsp spec

6df1c76

July541 mentioned this pull request Apr 20, 2023

Is "version" in lsp response used to assure file consistency (e.g. for renames)? #3498

Closed

July541 added 2 commits April 22, 2023 18:25

Init fields

21f5cda

Remove the empty line

d38f515

July541 marked this pull request as ready for review April 22, 2023 10:28

July541 requested review from konn, santiweight, OliverMadine, eddiemundo, Ailrun and berberman as code owners April 22, 2023 10:28

July541 added 4 commits April 22, 2023 18:28

Merge branch 'master' into doc-with-version

15e2a42

Merge branch 'master' into doc-with-version

09ebf17

modify for hls-tactics-plugin

4ce636b

name test

75f9d12

July541 requested a review from isovector as a code owner April 22, 2023 13:14

michaelpj approved these changes Jun 10, 2023

View reviewed changes

maralorn mentioned this pull request Jun 10, 2023

Correctly pass VersionedTextDocumentIdentifier through hls #3643

Merged

konn approved these changes Jun 12, 2023

View reviewed changes

michaelpj closed this Jun 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update version while editing to conform lsp spec #3566

Update version while editing to conform lsp spec #3566

July541 commented Apr 19, 2023 •

edited

Loading

michaelpj commented Apr 22, 2023

July541 commented Apr 22, 2023

michaelpj commented Apr 22, 2023

July541 commented Apr 23, 2023

July541 commented Apr 23, 2023

michaelpj left a comment

michaelpj Jun 10, 2023

michaelpj Jun 10, 2023

michaelpj Jun 10, 2023

michaelpj Jun 10, 2023

michaelpj Jun 10, 2023

michaelpj Jun 10, 2023

maralorn commented Jun 10, 2023

maralorn commented Jun 11, 2023

konn left a comment

	diffText' :: Bool -> (Uri,T.Text) -> T.Text -> WithDeletions -> TextDocumentVersion -> WorkspaceEdit
	diffText' :: Bool -> (VersionedTextDocmentIdentifier, T.Text) -> T.Text -> WithDeletions -> TextDocumentVersion -> WorkspaceEdit

	diagnosticToCodeActions :: DynFlags -> T.Text -> PluginId -> TextDocumentIdentifier -> TextDocumentVersion -> LSP.Diagnostic -> [LSP.CodeAction]
	diagnosticToCodeActions :: DynFlags -> T.Text -> PluginId -> VersionedTextDocumentIdentifier -> LSP.Diagnostic -> [LSP.CodeAction]

	applyAllCmd :: Recorder (WithPriority Log) -> CommandFunction IdeState (Uri, TextDocumentVersion)
	applyAllCmd :: Recorder (WithPriority Log) -> CommandFunction IdeState VersionedTextDocumentIdentifier

Update version while editing to conform lsp spec #3566

Update version while editing to conform lsp spec #3566

Conversation

July541 commented Apr 19, 2023 • edited Loading

michaelpj commented Apr 22, 2023

July541 commented Apr 22, 2023

michaelpj commented Apr 22, 2023

July541 commented Apr 23, 2023

July541 commented Apr 23, 2023

michaelpj left a comment

Choose a reason for hiding this comment

michaelpj Jun 10, 2023

Choose a reason for hiding this comment

michaelpj Jun 10, 2023

Choose a reason for hiding this comment

michaelpj Jun 10, 2023

Choose a reason for hiding this comment

michaelpj Jun 10, 2023

Choose a reason for hiding this comment

michaelpj Jun 10, 2023

Choose a reason for hiding this comment

michaelpj Jun 10, 2023

Choose a reason for hiding this comment

maralorn commented Jun 10, 2023

maralorn commented Jun 11, 2023

konn left a comment

Choose a reason for hiding this comment

July541 commented Apr 19, 2023 •

edited

Loading