Skip to content

Support text-2.0 #392

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 22, 2022
Merged

Support text-2.0 #392

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
65c83ed
Switch from rope-utf16-splay to text-rope
Bodigrim Jan 14, 2022
82bc411
Merge branch 'master' into master
michaelpj Jan 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lsp-types/lsp-types.cabal
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,10 +88,10 @@ library
, mtl
, network-uri
, mod
, rope-utf16-splay >= 0.3.1.0
, scientific
, some
, text
, text-rope
, template-haskell
, temporary
, unordered-containers
Expand Down
39 changes: 15 additions & 24 deletions lsp-types/src/Language/LSP/VFS.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ import Data.Ord
import qualified Data.HashMap.Strict as HashMap
import qualified Data.Map.Strict as Map
import Data.Maybe
import Data.Rope.UTF16 ( Rope )
import qualified Data.Rope.UTF16 as Rope
import Data.Text.Utf16.Rope ( Rope )
import qualified Data.Text.Utf16.Rope as Rope
import qualified Language.LSP.Types as J
import qualified Language.LSP.Types.Lens as J
import System.FilePath
Expand Down Expand Up @@ -136,7 +136,7 @@ applyCreateFile (J.CreateFile uri options _ann) =
updateVFS $ Map.insertWith
(\ new old -> if shouldOverwrite then new else old)
(J.toNormalizedUri uri)
(VirtualFile 0 0 (Rope.fromText ""))
(VirtualFile 0 0 mempty)
where
shouldOverwrite :: Bool
shouldOverwrite = case options of
Expand Down Expand Up @@ -260,7 +260,7 @@ persistFileVFS vfs uri =
action = do
exists <- doesFileExist tfn
unless exists $ do
let contents = Rope.toString (_text vf)
let contents = T.unpack (Rope.toText (_text vf))
writeRaw h = do
-- We honour original file line endings
hSetNewlineMode h noNewlineTranslation
Expand Down Expand Up @@ -291,26 +291,18 @@ applyChanges = foldl' applyChange
applyChange :: Rope -> J.TextDocumentContentChangeEvent -> Rope
applyChange _ (J.TextDocumentContentChangeEvent Nothing Nothing str)
= Rope.fromText str
applyChange str (J.TextDocumentContentChangeEvent (Just (J.Range (J.Position sl sc) _to)) (Just len) txt)
= changeChars str start (fromIntegral len) txt
where
start = Rope.rowColumnCodeUnits (Rope.RowColumn (fromIntegral sl) (fromIntegral sc)) str
applyChange str (J.TextDocumentContentChangeEvent (Just (J.Range (J.Position sl sc) (J.Position el ec))) Nothing txt)
= changeChars str start len txt
where
start = Rope.rowColumnCodeUnits (Rope.RowColumn (fromIntegral sl) (fromIntegral sc)) str
end = Rope.rowColumnCodeUnits (Rope.RowColumn (fromIntegral el) (fromIntegral ec)) str
len = end - start
applyChange str (J.TextDocumentContentChangeEvent (Just (J.Range (J.Position sl sc) (J.Position fl fc))) _ txt)
= changeChars str (Rope.Position (fromIntegral sl) (fromIntegral sc)) (Rope.Position (fromIntegral fl) (fromIntegral fc)) txt
applyChange str (J.TextDocumentContentChangeEvent Nothing (Just _) _txt)
= str

-- ---------------------------------------------------------------------

changeChars :: Rope -> Int -> Int -> Text -> Rope
changeChars str start len new = mconcat [before, Rope.fromText new, after']
changeChars :: Rope -> Rope.Position -> Rope.Position -> Text -> Rope
changeChars str start finish new = mconcat [before', Rope.fromText new, after]
where
(before, after) = Rope.splitAt start str
after' = Rope.drop len after
(before, after) = fromJust $ Rope.splitAtPosition finish str
(before', _) = fromJust $ Rope.splitAtPosition start before
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the best way to handle malformed input here? Since LSP counts by UTF-16 code units (not code points), it is possible that split happens in the middle of a code point, in which case text-lines returns Nothing.
(rope-utf16-splay in such situation silently extends split region until the end of a code point)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alanz ? do you remember what's up here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. To be honest I have never really paid attention to issues of splits in the middle of code points, assuming the client will send meaningful input, since they are managing a cursor over the text and sending its positions.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could throw. Maybe blowing up with an informative error message is the most sensible thing to do here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be happy with either:

  • Throwing, based on judging this to be very unlikely.
  • Returning Nothing, and propagating that up to the exported functions. In changeFromClientVFS we can drop the change and log, as is done in the existing Nothing branch.

Thoughts @alanz ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, we can merge this with the fromJust and experiment with removing it with @Bodigrim out of the loop :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking more at the code, I think we could push these failures right the way up to the LSP message response, which is probably the right thing to do, but I think we should do it separately. So my vote goes for "leave fromJust for now and remove subsequently", which I volunteer to look into.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is unlikely to happen in practice, so leaving as is would likely be safe, as is pushing it up to an LSP response. For the latter it at least makes us aware it is happening, and hopefully that will show up in initial developer dogfooding, or not at all.


-- ---------------------------------------------------------------------

Expand All @@ -336,14 +328,11 @@ data PosPrefixInfo = PosPrefixInfo
getCompletionPrefix :: (Monad m) => J.Position -> VirtualFile -> m (Maybe PosPrefixInfo)
getCompletionPrefix pos@(J.Position l c) (VirtualFile _ _ ropetext) =
return $ Just $ fromMaybe (PosPrefixInfo "" "" "" pos) $ do -- Maybe monad
let headMaybe [] = Nothing
headMaybe (x:_) = Just x
lastMaybe [] = Nothing
let lastMaybe [] = Nothing
lastMaybe xs = Just $ last xs

curLine <- headMaybe $ T.lines $ Rope.toText
$ fst $ Rope.splitAtLine 1 $ snd $ Rope.splitAtLine (fromIntegral l) ropetext
let beforePos = T.take (fromIntegral c) curLine
let curRope = fst $ Rope.splitAtLine 1 $ snd $ Rope.splitAtLine (fromIntegral l) ropetext
beforePos <- Rope.toText . fst <$> Rope.splitAt (fromIntegral c) curRope
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW this fixes an existing bug: T.take counts code points, while LSP requires counting UTF-16 code units.

curWord <-
if | T.null beforePos -> Just ""
| T.last beforePos == ' ' -> Just "" -- don't count abc as the curword in 'abc '
Expand All @@ -357,6 +346,8 @@ getCompletionPrefix pos@(J.Position l c) (VirtualFile _ _ ropetext) =
let modParts = dropWhile (not . isUpper . T.head)
$ reverse $ filter (not .T.null) xs
modName = T.intercalate "." modParts
-- curRope is already a single line, but it may include an enclosing '\n'
let curLine = T.dropWhileEnd (== '\n') $ Rope.toText curRope
return $ PosPrefixInfo curLine modName x pos

-- ---------------------------------------------------------------------
Expand Down
2 changes: 1 addition & 1 deletion lsp/lsp.cabal
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,9 +122,9 @@ test-suite unit-test
, lens >= 4.15.2
, network-uri
, quickcheck-instances
, rope-utf16-splay >= 0.2
, sorted-list == 0.2.1.*
, text
, text-rope
, unordered-containers
-- For GHCI tests
-- , async
Expand Down
29 changes: 14 additions & 15 deletions lsp/test/VspSpec.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
{-# LANGUAGE OverloadedStrings #-}
module VspSpec where


import Data.String
import qualified Data.Rope.UTF16 as Rope
import qualified Data.Text.Utf16.Rope as Rope
import Language.LSP.VFS
import qualified Language.LSP.Types as J
import qualified Data.Text as T
Expand Down Expand Up @@ -65,7 +64,7 @@ vspSpec = do
]
new = applyChange (fromString orig)
$ J.TextDocumentContentChangeEvent (Just $ J.mkRange 2 1 2 5) (Just 4) ""
lines (Rope.toString new) `shouldBe`
Rope.lines new `shouldBe`
[ "abcdg"
, "module Foo where"
, "-oo"
Expand All @@ -82,7 +81,7 @@ vspSpec = do
]
new = applyChange (fromString orig)
$ J.TextDocumentContentChangeEvent (Just $ J.mkRange 2 1 2 5) Nothing ""
lines (Rope.toString new) `shouldBe`
Rope.lines new `shouldBe`
[ "abcdg"
, "module Foo where"
, "-oo"
Expand All @@ -102,7 +101,7 @@ vspSpec = do
]
new = applyChange (fromString orig)
$ J.TextDocumentContentChangeEvent (Just $ J.mkRange 2 0 3 0) (Just 8) ""
lines (Rope.toString new) `shouldBe`
Rope.lines new `shouldBe`
[ "abcdg"
, "module Foo where"
, "foo :: Int"
Expand All @@ -119,7 +118,7 @@ vspSpec = do
]
new = applyChange (fromString orig)
$ J.TextDocumentContentChangeEvent (Just $ J.mkRange 2 0 3 0) Nothing ""
lines (Rope.toString new) `shouldBe`
Rope.lines new `shouldBe`
[ "abcdg"
, "module Foo where"
, "foo :: Int"
Expand All @@ -137,7 +136,7 @@ vspSpec = do
]
new = applyChange (fromString orig)
$ J.TextDocumentContentChangeEvent (Just $ J.mkRange 1 0 3 0) (Just 19) ""
lines (Rope.toString new) `shouldBe`
Rope.lines new `shouldBe`
[ "module Foo where"
, "foo = bb"
]
Expand All @@ -153,7 +152,7 @@ vspSpec = do
]
new = applyChange (fromString orig)
$ J.TextDocumentContentChangeEvent (Just $ J.mkRange 1 0 3 0) Nothing ""
lines (Rope.toString new) `shouldBe`
Rope.lines new `shouldBe`
[ "module Foo where"
, "foo = bb"
]
Expand All @@ -170,7 +169,7 @@ vspSpec = do
]
new = applyChange (fromString orig)
$ J.TextDocumentContentChangeEvent (Just $ J.mkRange 1 16 1 16) (Just 0) "\n-- fooo"
lines (Rope.toString new) `shouldBe`
Rope.lines new `shouldBe`
[ "abcdg"
, "module Foo where"
, "-- fooo"
Expand All @@ -188,7 +187,7 @@ vspSpec = do
]
new = applyChange (fromString orig)
$ J.TextDocumentContentChangeEvent (Just $ J.mkRange 1 8 1 8) Nothing "\n-- fooo\nfoo :: Int"
lines (Rope.toString new) `shouldBe`
Rope.lines new `shouldBe`
[ "module Foo where"
, "foo = bb"
, "-- fooo"
Expand All @@ -215,7 +214,7 @@ vspSpec = do
-- new = changeChars (fromString orig) (J.Position 7 0) (J.Position 7 8) "baz ="
new = applyChange (fromString orig)
$ J.TextDocumentContentChangeEvent (Just $ J.mkRange 7 0 7 8) (Just 8) "baz ="
lines (Rope.toString new) `shouldBe`
Rope.lines new `shouldBe`
[ "module Foo where"
, "-- fooo"
, "foo :: Int"
Expand Down Expand Up @@ -243,7 +242,7 @@ vspSpec = do
-- new = changeChars (fromString orig) (J.Position 7 0) (J.Position 7 8) "baz ="
new = applyChange (fromString orig)
$ J.TextDocumentContentChangeEvent (Just $ J.mkRange 7 0 7 8) Nothing "baz ="
lines (Rope.toString new) `shouldBe`
Rope.lines new `shouldBe`
[ "module Foo where"
, "-- fooo"
, "foo :: Int"
Expand All @@ -262,7 +261,7 @@ vspSpec = do
]
new = applyChange (fromString orig)
$ J.TextDocumentContentChangeEvent (Just $ J.mkRange 1 0 1 3) (Just 3) "𐐀𐐀"
lines (Rope.toString new) `shouldBe`
Rope.lines new `shouldBe`
[ "a𐐀b"
, "𐐀𐐀b"
]
Expand All @@ -285,13 +284,13 @@ vspSpec = do
]
(left,right) = Rope.splitAtLine 4 (fromString orig)

lines (Rope.toString left) `shouldBe`
Rope.lines left `shouldBe`
[ "module Foo where"
, "-- fooo"
, "foo :: Int"
, "foo = bb"
]
lines (Rope.toString right) `shouldBe`
Rope.lines right `shouldBe`
[ ""
, "bb = 5"
, ""
Expand Down