Skip to content

Avoid couple partial functions in lsp-test #546

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.

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

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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-test/src/Language/LSP/Test.hs
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,7 @@ sendRequest method params = do
reqMap <- requestMap <$> ask
liftIO $
modifyMVar_ reqMap $
\r -> return $ fromJust $ updateRequestMap r id method
\r -> return $ fromMaybe r $ updateRequestMap r id method
Copy link
Collaborator Author

@jhrcek jhrcek Jan 22, 2024

Choose a reason for hiding this comment

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

updateRequestMap returns Nothing when request with given LspId is already in it.
I don't see how it makes sense to crash in such cases as opposed to returning the origial map unmodified. Similarly in the file below.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's quite bad: that means we've somehow reused an ID which means stuff will break. So I think this probably should throw, but maybe not by using fromJust

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alright I'll play around with it locally to see if I can reproduce the me play around with how this works to see if I can reproduce the Timed out waiting to receive a message from the server. locally and then get back to this or close this.


~() <- case splitClientMethod method of
IsClientReq -> sendMessage mess
Expand Down
9 changes: 5 additions & 4 deletions lsp-test/src/Language/LSP/Test/Decoding.hs
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,15 @@ import Prelude hiding (id)

import Data.IxMap
import Data.Kind
import Text.Read (readMaybe)

{- | Fetches the next message bytes based on
the Content-Length header
-}
getNextMessage :: Handle -> IO B.ByteString
getNextMessage h = do
headers <- getHeaders h
case read . init <$> lookup "Content-Length" headers of
case readMaybe =<< lookup "Content-Length" headers of
Nothing -> throw NoContentLengthHeader
Just size -> B.hGet h size

Expand Down Expand Up @@ -68,11 +69,11 @@ getRequestMap = foldl' helper emptyIxMap
helper acc msg = case msg of
FromClientMess m mess -> case splitClientMethod m of
IsClientNot -> acc
IsClientReq -> fromJust $ updateRequestMap acc (mess ^. L.id) m
IsClientReq -> fromMaybe acc $ updateRequestMap acc (mess ^. L.id) m
IsClientEither -> case mess of
NotMess _ -> acc
ReqMess msg -> fromJust $ updateRequestMap acc (msg ^. L.id) m
_ -> acc
ReqMess msg -> fromMaybe acc $ updateRequestMap acc (msg ^. L.id) m
FromClientRsp{} -> acc

decodeFromServerMsg :: RequestMap -> B.ByteString -> (RequestMap, FromServerMessage)
decodeFromServerMsg reqMap bytes = unP $ parse p obj
Expand Down
11 changes: 5 additions & 6 deletions lsp-test/src/Language/LSP/Test/Session.hs
Original file line number Diff line number Diff line change
Expand Up @@ -187,9 +187,9 @@ data SessionState = SessionState
, vfs :: !VFS
, curDiagnostics :: !(Map.Map NormalizedUri [Diagnostic])
, overridingTimeout :: !Bool
, lastReceivedMessage :: !(Maybe FromServerMessage)
-- ^ The last received message from the server.
-- Used for providing exception information
, lastReceivedMessage :: !(Maybe FromServerMessage)
, curDynCaps :: !(Map.Map T.Text SomeRegistration)
-- ^ The capabilities that the server has dynamically registered with us so
-- far
Expand Down Expand Up @@ -250,7 +250,7 @@ runSessionMonad context state (Session session) = runReaderT (runStateT conduit
curId <- getCurTimeoutId
case msg of
ServerMessage sMsg -> yield sMsg
TimeoutMessage tId -> when (curId == tId) $ lastReceivedMessage <$> get >>= throw . Timeout
TimeoutMessage tId -> when (curId == tId) $ get >>= throw . Timeout . lastReceivedMessage

-- | An internal version of 'runSession' that allows for a custom handler to listen to the server.
-- It also does not automatically send initialize and exit messages.
Expand Down Expand Up @@ -468,11 +468,11 @@ updateState (FromServerMess SMethod_WorkspaceApplyEdit r) = do

textDocumentEdits uri edits = do
vers <- textDocumentVersions uri
pure $ map (\(v, e) -> TextDocumentEdit (review _versionedTextDocumentIdentifier v) [InL e]) $ zip vers edits
pure $ zipWith (\v e -> TextDocumentEdit (review _versionedTextDocumentIdentifier v) [InL e]) vers edits

getChangeParams uri edits = do
edits <- textDocumentEdits uri (reverse edits)
pure $ catMaybes $ map getParamsFromTextDocumentEdit edits
pure $ mapMaybe getParamsFromTextDocumentEdit edits

mergeParams :: [DidChangeTextDocumentParams] -> DidChangeTextDocumentParams
mergeParams params = let events = concat (toList (map (toList . (^. L.contentChanges)) params))
Expand Down Expand Up @@ -513,7 +513,7 @@ logMsg t msg = do
shouldColor <- asks $ logColor . config
liftIO $ when shouldLog $ do
when shouldColor $ setSGR [SetColor Foreground Dull color]
putStrLn $ arrow ++ showPretty msg
B.putStrLn $ arrow <> encodePretty msg
Copy link
Collaborator Author

@jhrcek jhrcek Jan 22, 2024

Choose a reason for hiding this comment

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

It seems pointless to convert pretty-printed json bytestring to String just to print it.
Wouldn't directly printing ByteString be less resource intensive?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have no idea, but seems sensible

when shouldColor $ setSGR [Reset]

where arrow
Expand All @@ -523,4 +523,3 @@ logMsg t msg = do
| t == LogServer = Magenta
| otherwise = Cyan

showPretty = B.unpack . encodePretty