Skip to content
This repository was archived by the owner on Oct 7, 2020. It is now read-only.

Fix TextDocumentContentChangeEvents getting applied in the wrong order #603

Merged
merged 2 commits into from
Jun 1, 2018

Conversation

lukel97
Copy link
Collaborator

@lukel97 lukel97 commented May 31, 2018

When there are multiple events in one didChange notification, they were getting applied from the last event to the first event which caused the text getting out of sync when undoing/redoing in VS Code. Fixes #566, possibly #531 but not #538

When there are multiple events in one didChange notification
@wz1000
Copy link
Collaborator

wz1000 commented May 31, 2018

What is the problem in #538?

@lukel97
Copy link
Collaborator Author

lukel97 commented May 31, 2018

@wz1000 The problem in #538 is with duplicate didChange events being sent, either by VS Code or vscode-hie-server

@wz1000
Copy link
Collaborator

wz1000 commented May 31, 2018

BTW, this code has nothing to do with the file that ghc/ghc-mod views. That is handled by the VFS portion of haskell-lsp. So it shouldn't effect those issues you listed.

The code that you changed only deals with correlating positions in the new version of the file as compared to the old one which we have a TypecheckedModule for. It has no effect on the file that is sent to GHC for compilation.

The purpose of this code is the following:

Suppose the original version of the file is

main = do
  putStrLn "Hello World"

HIE will compile this file and generate a typechecked module for it.

now suppose the user edits it to

main = do
  garbage
  putStrLn "Hello World"

HIE will fail to compile this file, but we will still have the typechecked module for the old file.

Now suppose the user hovers over putStrLn. HIE will get a hover event for line 3, column 3. newPosToOld will tell us that L3C3 corresponds to L2C3 in the old file we have the typechecked module for.

Similarly, oldPosToNew will tell us that L2C3 in the old file corresponds to L3C3 in the new file.

@lukel97
Copy link
Collaborator Author

lukel97 commented May 31, 2018

I see, that makes sense. I'm not sure how the position map is affecting the diagnostics then. Where do the contentChanges from the didChange event get applied to the temporary file then? The only place I can see changes get used in the snippet below is through updatePositionMap

Core.NotDidChangeTextDocument notification -> do
        liftIO $ U.logm "****** reactor: processing NotDidChangeTextDocument"
        let
            params = notification ^. J.params
            vtdi = params ^. J.textDocument
            uri  = vtdi ^. J.uri
            ver  = vtdi ^. J.version
            J.List changes = params ^. J.contentChanges
        mapFileFromVfs versionTVar cin vtdi
        makeRequest $ GReq (Just uri) Nothing Nothing (const $ return ()) $
          -- mark this module's cache as stale
          pluginGetFile "markCacheStale:" uri $ \fp -> do
            markCacheStale fp
            -- Important - Call this before requestDiagnostics
            updatePositionMap uri changes
        requestDiagnostics cin uri ver

@wz1000
Copy link
Collaborator

wz1000 commented May 31, 2018

haskell-lsp automatically processes changes to maintain a VFS here: https://github.com/alanz/haskell-lsp/blob/master/src/Language/Haskell/LSP/VFS.hs

mapFileFromVfs grabs the contents of the file from haskell lsp and hands it to ghc-mod for it to create a mapped file that GHC can then process..

@lukel97
Copy link
Collaborator Author

lukel97 commented May 31, 2018

Looks like the changes are getting sorted by range here. However the specification seems to vaguely imply that they should be applied in the order they come in.

/**
	 * The actual content changes. The content changes describe single state changes
	 * to the document. So if there are two content changes c1 and c2 for a document
	 * in state S10 then c1 move the document to S11 and c2 to S12.
	 */
	contentChanges: TextDocumentContentChangeEvent[];
}

And the behaviour from VS Code would suggest this too. I still think the position mapping would have been in the wrong order, since it doesn't look like the changes there are getting sorted but they are in the VFS. Should we just apply the changes with the order as-is?

@wz1000
Copy link
Collaborator

wz1000 commented May 31, 2018

I think the best approach would be to see what vscode actually sends, then write tests that confirm that our implementation conforms to that.

@lukel97
Copy link
Collaborator Author

lukel97 commented May 31, 2018

From typing abcdef, then undoing and then redoing:

2018-05-31 02:28:23.003017 [ThreadId 4] - ---> {"jsonrpc":"2.0","method":"textDocument/didChange","params":{"textDocument":{"uri":"file:///Users/luke/Desktop/test.hs","version":2},"contentChanges":[{"range":{"start":{"line":0,"character":0},"end":{"line":0,"character":0}},"rangeLength":0,"text":"a"}]}}
2018-05-31 02:28:23.586186 [ThreadId 4] - ---> {"jsonrpc":"2.0","method":"textDocument/didChange","params":{"textDocument":{"uri":"file:///Users/luke/Desktop/test.hs","version":3},"contentChanges":[{"range":{"start":{"line":0,"character":1},"end":{"line":0,"character":1}},"rangeLength":0,"text":"b"}]}}
2018-05-31 02:28:24.0989 [ThreadId 4] - ---> {"jsonrpc":"2.0","method":"textDocument/didChange","params":{"textDocument":{"uri":"file:///Users/luke/Desktop/test.hs","version":4},"contentChanges":[{"range":{"start":{"line":0,"character":2},"end":{"line":0,"character":2}},"rangeLength":0,"text":"c"}]}}
2018-05-31 02:28:24.407643 [ThreadId 4] - ---> {"jsonrpc":"2.0","method":"textDocument/didChange","params":{"textDocument":{"uri":"file:///Users/luke/Desktop/test.hs","version":5},"contentChanges":[{"range":{"start":{"line":0,"character":3},"end":{"line":0,"character":3}},"rangeLength":0,"text":"d"}]}}
2018-05-31 02:28:24.894433 [ThreadId 4] - ---> {"jsonrpc":"2.0","method":"textDocument/didChange","params":{"textDocument":{"uri":"file:///Users/luke/Desktop/test.hs","version":6},"contentChanges":[{"range":{"start":{"line":0,"character":4},"end":{"line":0,"character":4}},"rangeLength":0,"text":"e"}]}}
2018-05-31 02:28:25.42075 [ThreadId 4] - ---> {"jsonrpc":"2.0","method":"textDocument/didChange","params":{"textDocument":{"uri":"file:///Users/luke/Desktop/test.hs","version":7},"contentChanges":[{"range":{"start":{"line":0,"character":5},"end":{"line":0,"character":5}},"rangeLength":0,"text":"f"}]}}

2018-05-31 02:28:27.646542 [ThreadId 4] - ---> {"jsonrpc":"2.0","method":"textDocument/didChange","params":{"textDocument":{"uri":"file:///Users/luke/Desktop/test.hs","version":13},"contentChanges":[{"range":{"start":{"line":0,"character":5},"end":{"line":0,"character":6}},"rangeLength":1,"text":""},{"range":{"start":{"line":0,"character":4},"end":{"line":0,"character":5}},"rangeLength":1,"text":""},{"range":{"start":{"line":0,"character":3},"end":{"line":0,"character":4}},"rangeLength":1,"text":""},{"range":{"start":{"line":0,"character":2},"end":{"line":0,"character":3}},"rangeLength":1,"text":""},{"range":{"start":{"line":0,"character":1},"end":{"line":0,"character":2}},"rangeLength":1,"text":""},{"range":{"start":{"line":0,"character":0},"end":{"line":0,"character":1}},"rangeLength":1,"text":""}]}}

2018-05-31 02:28:29.2255 [ThreadId 4] - ---> {"jsonrpc":"2.0","method":"textDocument/didChange","params":{"textDocument":{"uri":"file:///Users/luke/Desktop/test.hs","version":19},"contentChanges":[{"range":{"start":{"line":0,"character":0},"end":{"line":0,"character":0}},"rangeLength":0,"text":"a"},{"range":{"start":{"line":0,"character":1},"end":{"line":0,"character":1}},"rangeLength":0,"text":"b"},{"range":{"start":{"line":0,"character":2},"end":{"line":0,"character":2}},"rangeLength":0,"text":"c"},{"range":{"start":{"line":0,"character":3},"end":{"line":0,"character":3}},"rangeLength":0,"text":"d"},{"range":{"start":{"line":0,"character":4},"end":{"line":0,"character":4}},"rangeLength":0,"text":"e"},{"range":{"start":{"line":0,"character":5},"end":{"line":0,"character":5}},"rangeLength":0,"text":"f"}]}}

Since currently it's being sorted in descending order then the undo part will get processed in the right order, i.e. remove f then e etc. But the redo will get processed in reverse order, and we end up with fedcba in the temporary file. There is another example in #566

@alanz
Copy link
Collaborator

alanz commented May 31, 2018

There is discussion on the lsp spec repo about these changes, and that they should be applied in reverse order, basically because the origin locations are wrt the original document, so applying an edit at the front will invalidate one at the end.

How does this show up as a problem for us?

@lukel97
Copy link
Collaborator Author

lukel97 commented May 31, 2018

@alanz
Copy link
Collaborator

alanz commented May 31, 2018

@bubba, what needs to happen here? I merged the haskell-lsp commit.

@alanz
Copy link
Collaborator

alanz commented Jun 1, 2018

@wz1000 are you happy with this?

@wz1000
Copy link
Collaborator

wz1000 commented Jun 1, 2018

yes

@alanz alanz merged commit 2aa2cf3 into haskell:master Jun 1, 2018
@alanz alanz added this to the prehistory milestone Feb 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants