Appropriately deal with files that don't end with a line terminator - #5058
Appropriately deal with files that don't end with a line terminator#5058Aster89 wants to merge 1 commit into
Conversation
Aster89
changed the title
|
A small experiment that I should probably turn into a test: $ cabal repl /home/enrico/haskell-language-server/hls-plugin-api/src/Ide/PluginUtils.hs
λ> import Language.LSP.Protocol.Types
λ> import Ide.PluginUtils
λ> :set -XOverloadedStrings
λ> uri = Uri {getUri = "file:///home/enrico/haskell-language-server/plugins/hls-class-plugin/test/testdata/T1W.hs"}
λ> verTxtDocId = VersionedTextDocumentIdentifier uri 0
λ> old = "module T1 where\n\ndata X = X\n\ninstance Eq X where\n"
λ> new = "module T1 where\n\ndata X = X\n\ninstance Eq X where\n (==) = _\n"
λ> e1 = diffText' True (verTxtDocId, old) new IncludeDeletions
λ> e1
WorkspaceEdit {_changes = Nothing, _documentChanges = Just [InL (TextDocumentEdit {_textDocument = OptionalVersionedTextDocumentIdentifier {_uri = Uri {getUri = "file:///home/enrico/haskell-language-server/plugins/hls-class-plugin/test/tes
tdata/T1W.hs"}, _version = InL 0}, _edits = [InL (TextEdit {_range = Range {_start = Position {_line = 5, _character = 0}, _end = Position {_line = 5, _character = 0}}, _newText = " (==) = _\n"})]})], _changeAnnotations = Nothing}See that the This is correct. But look what happens if we remove the trailing λ> old = "module T1 where\n\ndata X = X\n\ninstance Eq X where"
λ> new = "module T1 where\n\ndata X = X\n\ninstance Eq X where\n (==) = _"
λ> e2 = diffText' True (verTxtDocId, old) new IncludeDeletions
λ> e1 == e2
Truewhich is wrong! The These are probably the shortest reproduction steps (but λ> d1 = diffTextEdit "foo" "foo\nbar" IncludeDeletions
λ> d2 = diffTextEdit "foo\n" "foo\nbar\n" IncludeDeletions
λ> d1 == d2
True
λ> d1
[TextEdit {_range = Range {_start = Position {_line = 1, _character = 0}, _end = Position {_line = 1, _character = 0}}, _newText = "bar\n"}] |
|
Well, the bug is clearly on this line: I mean, once you've done |
|
Building on my earlier experience with text diff tools, the solution I've attempted consisted of
The idea is that point 1 allows us not to throw away the line terminators, and point 2 preserves the equality used so far. The drawback is that the resulting [Both ["foo"] ["foo\n"],Second ["bar"]]from which the current code deduces that In the context of a full-fledged text diff tool, the direction I'd take is to perform a sub-comparison between the 2 sides of the [(Both ["foo"] ["foo\n"], Just (NonEmpty [Both "foo" "foo", Second "\n"])]),(Second ["bar"], Nothing)]where the If we were to diff [(Both ["foo\n"] ["foo\n"], Nothing]),(Second ["bar\n"], Nothing)]Anyway, I've taken note of the above to avoid forgetting, but it sounds too much of a complication considering that the only time that And maybe it would break several tests. Probably a simple hack is a better approach. Looking into it. But I also have to check what happens in case an action removes the last line of a file. |
|
This PR, in its current state, is
What do I mean?
The point is that we never use this
As long as GHC (well, and |
14477cb to
c7f5e54
Compare
As described in the PR, the issue with the existing `diffTextEdit`
algorithm is that it entirely throws away line breaks, so it can't
really make a difference between a file that ends with a line break and
a file that doesn't.
The road to the solution, in hindsight, was pretty simple:
1. Trust that, other than the issue described above, the algorithm is
sound.
2. Override the "classic" `lines` and `unlines` with lossless
counterparts (in other words, when splitting, don't throw away the
separators).
This was as easy as
```
lines = split (dropFinalBlank $ keepDelimsR $ whenElt (== '\n'))
unlines = concat
```
3. See what breaks and fix it.
This boiled down to just removing a call to `init` that was applied
to the result of `unlines`.
The ad-hoc tests I've written for the class- and case-split- plugins
both pass, but the tests I've added for `diffTextEdit`, and more
specifically the `diffTextEditComplete` helper function, deserve an
explanation:
- (All tests' `Text` triples (left, right, and expected
deleted/inserted text) are carefully aligned to help the eye detect
how they relate to each other.)
- When both inputs `Text`s to `diffTextEditComplete` end with `'\n'`,
the expected edit should not surprise, both in the tests that insert
something at EOF and in those that delete something at EOF.
- In all other cases, the expected edit might catch you off guard; at
least it did in my case.
Here follows one of those tests, together with an explanatory
commentary:
```haskell
…
$ diffTextEditComplete "foo"
"foo\nbar\n"
@?= [textEdit "foo\nbar\n"
(mkRange 0 0 0 3)]
```
The line-based tokenization will result in `["foo"]` for the left
file and in `["foo\n", "bar\n"]` for the second file. As you can
see, there's no entry in common between these two lists, because
`"foo" /= "foo\n"` (yes, we still use `getGroupedDiff`, i.e.
`getGroupedDiffBy (==)`). This translates to the fact that the
algorithm, rather than detecting that `"\nbar\n"` was inserted,
detects that `"foo"` was deleted, and `"foo\nbar\n"` was inserted,
which boils down to the same result.
|
All tests that fail are failing the same way. Here's an example failure: The given file is this {-# LANGUAGE TemplateHaskell #-}
{-# LANGUAGE QuasiQuotes #-}
module TSimpleDecl where
import Language.Haskell.TH ( mkName, clause, normalB, funD, sigD )
-- Foo
-- Bar
$(sequence
[sigD (mkName "foo") [t|Int|]
,funD (mkName "foo") [clause [] (normalB [|42|]) []]
]
)
-- Bar
-- ee
-- ddddand the expected is this {-# LANGUAGE TemplateHaskell #-}
{-# LANGUAGE QuasiQuotes #-}
module TSimpleDecl where
import Language.Haskell.TH ( mkName, clause, normalB, funD, sigD )
-- Foo
-- Bar
foo :: Int
foo = 42
-- Bar
-- ee
-- ddddI've logged the TextEdit {_range = Range {_start = Position {_line = 7, _character = 0}, _end = Position {_line = 11, _character = 5}}, _newText = "foo :: Int\nfoo = 42\n"}Its Given
Notice that there isn't a line break at the end, because a If we change that text for one that does have a line break at the end (see the I'm not trying to understand where that |
|
This snippet, haskell-language-server/plugins/hls-splice-plugin/src/Ide/Plugin/Splice.hs Lines 166 to 174 in 410747c is where the But For the very example in a previous message, of which I copy-and-paste the input file, {-# LANGUAGE TemplateHaskell #-}
{-# LANGUAGE QuasiQuotes #-}
module TSimpleDecl where
import Language.Haskell.TH ( mkName, clause, normalB, funD, sigD )
-- Foo
-- Bar
$(sequence
[sigD (mkName "foo") [t|Int|]
,funD (mkName "foo") [clause [] (normalB [|42|]) []]
]
)
-- Bar
-- ee
-- ddddI've printed the "\n\nmodule TSimpleDecl where\nimport Language.Haskell.TH ( mkName, clause, normalB, funD, sigD )\n\n\n\n$(sequence\n [sigD (mkName \"foo\") [t|Int|]\n ,funD (mkName \"foo\") [clause [] (normalB [|42|]) []]\n ]\n )\n\n\n\n"
-- 0 1 2 3 4 5 6 7 8 9 10 11 12131415
"\n\nmodule TSimpleDecl where\nimport Language.Haskell.TH ( mkName, clause, normalB, funD, sigD )\nfoo :: Int\nfoo = 42\n\n\n\n"
-- 0 1 2 3 4 5 6 7 8 9A couple of observations:
Maybe I'm getting somewhere. |
|
Oh, that's the culprit! haskell-language-server/plugins/hls-splice-plugin/src/Ide/Plugin/Splice.hs Lines 172 to 174 in 410747c Eh... Not sure what to do yet. Surely removing it does not good. Next to investigate: |
1 participant
Both the given and expected files don't have a line terminator at EOF.
The new test does fail, demonstrating there is a bug, just like I observe in VSCode, see GIF attached to #5059.
However, Vim+YCM and Neovim seem to be immune to it. (As far as Vim+YCM goes, I know why it's immune because I fixed another bug, ycm-core/YouCompleteMe#4311.)
I think that the test failing shows that the bug is in HLS, not in VSCode. Vim+YCM and Neovim are probably just being smart and sidestepping the bug entirely.
Fixes #5059.