Skip to content
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

Fix error in code range #3229

Merged
merged 7 commits into from
Sep 29, 2022
Merged
Show file tree
Hide file tree
Changes from 5 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
64 changes: 41 additions & 23 deletions plugins/hls-code-range-plugin/src/Ide/Plugin/CodeRange.hs
Original file line number Diff line number Diff line change
Expand Up @@ -13,23 +13,23 @@ module Ide.Plugin.CodeRange (
) where

import Control.Monad.Except (ExceptT (ExceptT),
runExceptT)
mapExceptT)
import Control.Monad.IO.Class (liftIO)
import Control.Monad.Trans.Maybe (MaybeT (MaybeT),
maybeToExceptT)
import Data.Either.Extra (maybeToEither)
import Data.Maybe (fromMaybe)
import Data.Vector (Vector)
import qualified Data.Vector as V
import Development.IDE (IdeAction,
import Development.IDE (Action, IdeAction,
IdeState (shakeExtras),
Range (Range), Recorder,
WithPriority,
cmapWithPrio,
cmapWithPrio, runAction,
runIdeAction,
toNormalizedFilePath',
uriToFilePath')
import Development.IDE.Core.Actions (useE)
uriToFilePath', use,
useWithStaleFast)
import Development.IDE.Core.PositionMapping (PositionMapping,
fromCurrentPosition,
toCurrentRange)
Expand All @@ -44,7 +44,7 @@ import Ide.Types (PluginDescriptor (pluginH
PluginId,
defaultPluginDescriptor,
mkPluginHandler)
import Language.LSP.Server (LspM)
import Language.LSP.Server (LspM, LspT)
import Language.LSP.Types (FoldingRange (..),
FoldingRangeParams (..),
List (List),
Expand Down Expand Up @@ -77,49 +77,62 @@ foldingRangeHandler ide _ FoldingRangeParams{..} = do
pluginResponse $ do
filePath <- ExceptT . pure . maybeToEither "fail to convert uri to file path" $
toNormalizedFilePath' <$> uriToFilePath' uri
foldingRanges <- ExceptT . liftIO . runIdeAction "FoldingRange" (shakeExtras ide) . runExceptT $
foldingRanges <- liftIO . runAction "FoldingRange" ide $
getFoldingRanges filePath
pure . List $ foldingRanges
where
uri :: Uri
TextDocumentIdentifier uri = _textDocument

getFoldingRanges :: NormalizedFilePath -> ExceptT String IdeAction [FoldingRange]
getFoldingRanges file = do
(codeRange, _) <- maybeToExceptT "fail to get code range" $ useE GetCodeRange file

-- removing first node because it folds the entire file
pure $ drop 1 $ findFoldingRanges codeRange
getFoldingRanges :: NormalizedFilePath -> Action [FoldingRange]
getFoldingRanges file = fmap (maybe [] findFoldingRanges) $ use GetCodeRange file

selectionRangeHandler :: IdeState -> PluginId -> SelectionRangeParams -> LspM c (Either ResponseError (List SelectionRange))
selectionRangeHandler ide _ SelectionRangeParams{..} = do
pluginResponse $ do
filePath <- ExceptT . pure . maybeToEither "fail to convert uri to file path" $
toNormalizedFilePath' <$> uriToFilePath' uri
selectionRanges <- ExceptT . liftIO . runIdeAction "SelectionRange" (shakeExtras ide) . runExceptT $
getSelectionRanges filePath positions
pure . List $ selectionRanges
fmap List . mapExceptT runIdeAction' . getSelectionRanges filePath $ positions
where
uri :: Uri
TextDocumentIdentifier uri = _textDocument

positions :: [Position]
List positions = _positions

getSelectionRanges :: NormalizedFilePath -> [Position] -> ExceptT String IdeAction [SelectionRange]
runIdeAction' :: IdeAction (Either SelectionRangeError [SelectionRange]) -> LspT c IO (Either String [SelectionRange])
runIdeAction' action = do
result <- liftIO $ runIdeAction "SelectionRange" (shakeExtras ide) action
pure $ case result of
Left err -> maybe (Right []) Left (showError err)
Right list -> Right list

showError :: SelectionRangeError -> Maybe String
-- This might happen if the HieAst is not ready, so we give it a default value instead of throwing an error
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd probably just inline this function, then it would look less weird that you're kind of doing different things in the different branches?

showError SelectionRangeBadDependency = Nothing
showError SelectionRangeInputPositionMappingFailure = Just "failed to apply position mapping to input positions"
showError SelectionRangeOutputPositionMappingFailure = Just "failed to apply position mapping to output positions"

data SelectionRangeError = SelectionRangeBadDependency
| SelectionRangeInputPositionMappingFailure
| SelectionRangeOutputPositionMappingFailure

getSelectionRanges :: NormalizedFilePath -> [Position] -> ExceptT SelectionRangeError IdeAction [SelectionRange]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@michaelpj Did I get it correctly?

getSelectionRanges file positions = do
(codeRange, positionMapping) <- maybeToExceptT "fail to get code range" $ useE GetCodeRange file
(codeRange, positionMapping) <- maybeToExceptT SelectionRangeBadDependency . MaybeT $
useWithStaleFast GetCodeRange file
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we should include the dependency that failed in the error so we can log it later? I guess useWithStaleFast doesn't log if it fails?

Copy link
Collaborator Author

@kokobd kokobd Sep 29, 2022

Choose a reason for hiding this comment

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

Makes sense. At first, I thought it was too annoying to see a lot of error logs when creating a new Haskell source file, so I wanted to get rid of them completely.
Maybe we should just log it in warning level (I remember at least one other plugin does this), and take a look at why HieAst is missing for files with only a line of module header.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, I realised when thinking about this that a plugin returning a ResponseError can be "normal" and not necessarily something that HLS needs to log as an error. I still think it's good to log them so you know what's happening, but maybe it doesn't need to be an error log.

-- 'positionMapping' should be appied to the input before using them
positions' <- maybeToExceptT "fail to apply position mapping to input positions" . MaybeT . pure $
positions' <- maybeToExceptT SelectionRangeInputPositionMappingFailure . MaybeT . pure $
traverse (fromCurrentPosition positionMapping) positions

let selectionRanges = flip fmap positions' $ \pos ->
-- We need a default selection range if the lookup fails, so that other positions can still have valid results.
-- We need a default selection range if the lookup fails,
-- so that other positions can still have valid results.
let defaultSelectionRange = SelectionRange (Range pos pos) Nothing
in fromMaybe defaultSelectionRange . findPosition pos $ codeRange

-- 'positionMapping' should be applied to the output ranges before returning them
maybeToExceptT "fail to apply position mapping to output positions" . MaybeT . pure $
maybeToExceptT SelectionRangeOutputPositionMappingFailure . MaybeT . pure $
traverse (toCurrentSelectionRange positionMapping) selectionRanges

-- | Find 'Position' in 'CodeRange'. This can fail, if the given position is not covered by the 'CodeRange'.
Expand Down Expand Up @@ -169,8 +182,13 @@ findPosition pos root = go Nothing root
--
-- Discussion reference: https://github.com/haskell/haskell-language-server/pull/3058#discussion_r973737211
findFoldingRanges :: CodeRange -> [FoldingRange]
findFoldingRanges r@(CodeRange _ children _) =
let frChildren :: [FoldingRange] = concat $ V.toList $ fmap findFoldingRanges children
findFoldingRanges codeRange =
-- removing the first node because it folds the entire file
drop 1 $ findFoldingRangesRec codeRange

findFoldingRangesRec :: CodeRange -> [FoldingRange]
findFoldingRangesRec r@(CodeRange _ children _) =
let frChildren :: [FoldingRange] = concat $ V.toList $ fmap findFoldingRangesRec children
in case createFoldingRange r of
Just x -> x:frChildren
Nothing -> frChildren
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ handleError recorder action' = do
valueEither <- runExceptT action'
case valueEither of
Left msg -> do
logWith recorder Error msg
logWith recorder Warning msg
pure $ toIdeResult (Left [])
Right value -> pure $ toIdeResult (Right value)

Expand Down
9 changes: 5 additions & 4 deletions plugins/hls-code-range-plugin/test/Main.hs
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,9 @@ main = do
defaultTestRunner $
testGroup "Code Range" [
testGroup "Integration Tests" [
makeSelectionRangeGoldenTest recorder "Import" [(4, 36), (1, 8)],
makeSelectionRangeGoldenTest recorder "Function" [(5, 19), (5, 12), (4, 4), (3, 5)],
selectionRangeGoldenTest recorder "Import" [(4, 36), (1, 8)],
selectionRangeGoldenTest recorder "Function" [(5, 19), (5, 12), (4, 4), (3, 5)],
selectionRangeGoldenTest recorder "Empty" [(1, 5)],
foldingRangeGoldenTest recorder "Function"
],
testGroup "Unit Tests" [
Expand All @@ -37,8 +38,8 @@ main = do
]
]

makeSelectionRangeGoldenTest :: Recorder (WithPriority Log) -> TestName -> [(UInt, UInt)] -> TestTree
makeSelectionRangeGoldenTest recorder testName positions = goldenGitDiff testName (testDataDir </> testName <.> "golden" <.> "txt") $ do
selectionRangeGoldenTest :: Recorder (WithPriority Log) -> TestName -> [(UInt, UInt)] -> TestTree
selectionRangeGoldenTest recorder testName positions = goldenGitDiff testName (testDataDir </> testName <.> "golden" <.> "txt") $ do
res <- runSessionWithServer (plugin recorder) testDataDir $ do
doc <- openDoc (testName <.> "hs") "haskell"
resp <- request STextDocumentSelectionRange $ SelectionRangeParams Nothing Nothing doc
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
module Empty where
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@ cradle:
direct:
arguments:
- "Function"
- "Empty"
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
module Empty where
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@ cradle:
arguments:
- "Import"
- "Function"
- "Empty"