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

Resolve 1: Support for resolve in overloaded-record-dot #3658

Merged
merged 24 commits into from
Jun 30, 2023
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
bb742b3
resolve for overloaded-record-dot (checkpoint)
joyfulmantis Jun 13, 2023
be71eb9
resolve support works on VSCode (tests need to be redone)
joyfulmantis Jun 14, 2023
fb21134
Tests for both resolve and non resolve variants
joyfulmantis Jun 15, 2023
f347ebc
Added more tests
joyfulmantis Jun 19, 2023
c19480d
Fix merge mistakes; move function to hls-test-utils
joyfulmantis Jun 19, 2023
5e37f6f
Remove codeLens resolve
joyfulmantis Jun 19, 2023
4bcd45b
Don't use partial functions
joyfulmantis Jun 21, 2023
7d4f01e
Implement michaelpj's suggestions
joyfulmantis Jun 22, 2023
225152e
Make owned resolve data transparent to the plugins
joyfulmantis Jun 26, 2023
4b34265
Improve ord's resolve handler's error handling
joyfulmantis Jun 26, 2023
9985195
Oh well, if only we had MonadFail
joyfulmantis Jun 26, 2023
355e95c
Generic support for resolve in hls packages
joyfulmantis Jun 27, 2023
2e4d14c
Merge branch 'resolve-support' into ord-resolve
joyfulmantis Jun 27, 2023
fb49c31
Add a new code action resolve helper that falls backs to commands
joyfulmantis Jun 27, 2023
d1d299b
add resolve capability set to hls-test-utils
joyfulmantis Jun 28, 2023
e025840
Merge branch 'resolve-support' into ord-resolve
joyfulmantis Jun 28, 2023
0b57d5a
use caps defined at hls-test-utils
joyfulmantis Jun 28, 2023
735feca
Add code lens resolve support
joyfulmantis Jun 29, 2023
6b3b915
Merge branch 'master' into resolve-support
michaelpj Jun 29, 2023
1ba6098
Merge branch 'resolve-support' into ord-resolve
joyfulmantis Jun 29, 2023
7e9bf1d
Merge branch 'master' into ord-resolve
michaelpj Jun 29, 2023
0271ce2
Improve comments
joyfulmantis Jun 29, 2023
794034b
remove Benchmark as it wasn't that useful and triggered a lsp-test bug
joyfulmantis Jun 30, 2023
7790755
Merge branch 'master' into ord-resolve
joyfulmantis Jun 30, 2023
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
63 changes: 36 additions & 27 deletions hls-plugin-api/src/Ide/Types.hs
Original file line number Diff line number Diff line change
Expand Up @@ -403,32 +403,8 @@ instance PluginMethod Request Method_TextDocumentCodeAction where
where
uri = msgParams ^. L.textDocument . L.uri

instance PluginRequestMethod Method_TextDocumentCodeAction where
combineResponses _method _config (ClientCapabilities _ textDocCaps _ _ _ _) (CodeActionParams _ _ _ _ context) resps =
InL $ fmap compat $ filter wasRequested $ concat $ mapMaybe nullToMaybe $ toList resps
where
compat :: (Command |? CodeAction) -> (Command |? CodeAction)
compat x@(InL _) = x
compat x@(InR action)
| Just _ <- textDocCaps >>= _codeAction >>= _codeActionLiteralSupport
= x
| otherwise = InL cmd
where
cmd = mkLspCommand "hls" "fallbackCodeAction" (action ^. L.title) (Just cmdParams)
cmdParams = [toJSON (FallbackCodeActionParams (action ^. L.edit) (action ^. L.command))]

wasRequested :: (Command |? CodeAction) -> Bool
wasRequested (InL _) = True
wasRequested (InR ca)
| Nothing <- _only context = True
| Just allowed <- _only context
-- See https://github.com/microsoft/language-server-protocol/issues/970
-- This is somewhat vague, but due to the hierarchical nature of action kinds, we
-- should check whether the requested kind is a *prefix* of the action kind.
-- That means, for example, we will return actions with kinds `quickfix.import` and
-- `quickfix.somethingElse` if the requested kind is `quickfix`.
, Just caKind <- ca ^. L.kind = any (\k -> k `codeActionKindSubsumes` caKind) allowed
| otherwise = False
instance PluginMethod Request Method_CodeActionResolve where
pluginEnabled _ msgParams pluginDesc config = pluginEnabledConfig plcCodeActionsOn (configForPlugin config pluginDesc)

instance PluginMethod Request Method_TextDocumentDefinition where
pluginEnabled _ msgParams pluginDesc _ =
Expand Down Expand Up @@ -535,6 +511,38 @@ instance PluginMethod Request (Method_CustomMethod m) where
pluginEnabled _ _ _ _ = True

---
instance PluginRequestMethod Method_TextDocumentCodeAction where
combineResponses _method _config (ClientCapabilities _ textDocCaps _ _ _ _) (CodeActionParams _ _ _ _ context) resps =
InL $ fmap compat $ filter wasRequested $ concat $ mapMaybe nullToMaybe $ toList resps
where
compat :: (Command |? CodeAction) -> (Command |? CodeAction)
compat x@(InL _) = x
compat x@(InR action)
| Just _ <- textDocCaps >>= _codeAction >>= _codeActionLiteralSupport
= x
| otherwise = InL cmd
where
cmd = mkLspCommand "hls" "fallbackCodeAction" (action ^. L.title) (Just cmdParams)
cmdParams = [toJSON (FallbackCodeActionParams (action ^. L.edit) (action ^. L.command))]

wasRequested :: (Command |? CodeAction) -> Bool
wasRequested (InL _) = True
wasRequested (InR ca)
| Nothing <- _only context = True
| Just allowed <- _only context
-- See https://github.com/microsoft/language-server-protocol/issues/970
-- This is somewhat vague, but due to the hierarchical nature of action kinds, we
-- should check whether the requested kind is a *prefix* of the action kind.
-- That means, for example, we will return actions with kinds `quickfix.import` and
-- `quickfix.somethingElse` if the requested kind is `quickfix`.
, Just caKind <- ca ^. L.kind = any (\k -> k `codeActionKindSubsumes` caKind) allowed
| otherwise = False

instance PluginRequestMethod Method_CodeActionResolve where
-- CodeAction resolve is currently only used to changed the edit field, thus
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm, this made me realize that something funny may happen here. Suppose we have N code action handlers, each of which has a corresponding resolve handler. Now if one of the code action handlers produces a code action, then when the client asks to resolve it... it's going to get sent to every resolve handler. So we'll need to make sure that resolve handlers "know" if it's one of "their" code actions...

Copy link
Collaborator

Choose a reason for hiding this comment

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

And indeed, I think we therefore don't want to combine the results of multiple resolve handlers firing. What would that even mean? Something has gone wrong if that has happened! We should get exactly one response.

Hard to do for now, but I do think that combineResponses should be able to throw an error in cases like this. For now I'd probably do the crappy "just take the first result" thing we do elsewhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So even if every plugin's resolve handler used the same type theoretically Data.Unique should allow types to know whether a resolve belongs to them (resolvable ones eventually anyways). To make sure no extra processing was needed I was thinking of having the plugin record their name or id in the type serialized to the data field, and then first match on that, to make sure no extra processing was needed.

Regarding combineResponses, I was actually originally just returning the unmodified codeAction if I was unable to resolve with the provided resolve data, which is why combineRespones would still be needed to find the modified codeAction from the list. I guess I could/should just throw an error, and that would be equivalent to returning Nothing? The plugin ultimately responsible for the resolve also needs to throw a ContentModified responseError if it can't resolve it. So not actually sure we can use pluginResponse in the end (or at least we need to modify it)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, if we use standard infrastructure and tag every code action with a Unique then we probably should be fairly okay. Although it requires some state on the plugin side somewhere to remember which uniques it produced. The advantage of the "just pass the CodeActionParams again" approach is that it's easier to be stateless if you want to. Not sure if it's possible to be stateless in general, though 🤔

I guess I could/should just throw an error, and that would be equivalent to returning Nothing?

I think that's fine. I think we run all the handlers, throw away any that failed and then combine the results of the rest. We could potentially also use the pluginResponsible method for this, I think? We use it in a similar way to restrict the scope of some handlers. So if we put the responsible plugin ID in the data then I think we could restrict to just that plugin in pluginResponsible.

-- that's the only field we are combining.
combineResponses _ _ _ codeAction (toList -> codeActions) = codeAction & L.edit .~ mconcat ((^. L.edit) <$> codeActions)

instance PluginRequestMethod Method_TextDocumentDefinition where
combineResponses _ _ _ _ (x :| _) = x

Expand Down Expand Up @@ -949,7 +957,8 @@ instance HasTracing WorkspaceSymbolParams where
instance HasTracing CallHierarchyIncomingCallsParams
instance HasTracing CallHierarchyOutgoingCallsParams
instance HasTracing CompletionItem

instance HasTracing CodeLens
instance HasTracing CodeAction
-- ---------------------------------------------------------------------

{-# NOINLINE pROCESS_ID #-}
Expand Down
22 changes: 22 additions & 0 deletions hls-test-utils/src/Test/Hls.hs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ module Test.Hls
defaultTestRunner,
goldenGitDiff,
goldenWithHaskellDoc,
goldenWithHaskellAndCaps,
goldenWithCabalDoc,
goldenWithHaskellDocFormatter,
goldenWithCabalDocFormatter,
Expand Down Expand Up @@ -143,6 +144,27 @@ goldenWithHaskellDoc
-> TestTree
goldenWithHaskellDoc = goldenWithDoc "haskell"

goldenWithHaskellAndCaps
:: Pretty b
=> ClientCapabilities
-> PluginTestDescriptor b
-> TestName
-> FilePath
-> FilePath
-> FilePath
-> FilePath
-> (TextDocumentIdentifier -> Session ())
-> TestTree
goldenWithHaskellAndCaps clientCaps plugin title testDataDir path desc ext act =
goldenGitDiff title (testDataDir </> path <.> desc <.> ext)
$ runSessionWithServerAndCaps plugin clientCaps testDataDir
$ TL.encodeUtf8 . TL.fromStrict
<$> do
doc <- openDoc (path <.> ext) "haskell"
void waitForBuildQueue
act doc
documentContents doc

goldenWithCabalDoc
:: Pretty b
=> PluginTestDescriptor b
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ library
exposed-modules: Ide.Plugin.OverloadedRecordDot
build-depends:
, base >=4.16 && <5
, aeson
, ghcide
, hls-plugin-api
, lsp
Expand Down Expand Up @@ -58,8 +59,11 @@ test-suite tests
build-depends:
, base
, filepath
, ghcide
, text
, hls-overloaded-record-dot-plugin
, lens
, lsp-test
, lsp-types
, hls-test-utils

Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,24 @@ module Ide.Plugin.OverloadedRecordDot

-- based off of Berk Okzuturk's hls-explicit-records-fields-plugin

import Control.Lens ((^.))
import Control.Lens (_Just, (^.), (^?))
import Control.Monad (replicateM)
import Control.Monad.IO.Class (MonadIO, liftIO)
import Control.Monad.Trans.Class (lift)
import Control.Monad.Trans.Except (ExceptT)
import Data.Aeson (FromJSON, Result (..),
ToJSON, fromJSON, toJSON)
import Data.Generics (GenericQ, everything,
everythingBut, mkQ)
import qualified Data.IntMap.Strict as IntMap
import qualified Data.Map as Map
import Data.Maybe (mapMaybe, maybeToList)
import Data.Maybe (fromJust, mapMaybe,
maybeToList)
import Data.Text (Text)
import Data.Unique (hashUnique, newUnique)
import Development.IDE (IdeState,
NormalizedFilePath,
NormalizedUri,
Pretty (..), Range,
Recorder (..), Rules,
WithPriority (..),
Expand Down Expand Up @@ -76,17 +84,20 @@ import Ide.Types (PluginDescriptor (..),
PluginMethodHandler,
defaultPluginDescriptor,
mkPluginHandler)
import Language.LSP.Protocol.Lens (HasChanges (changes))
import qualified Language.LSP.Protocol.Lens as L
import Language.LSP.Protocol.Message (Method (..),
SMethod (..))
import Language.LSP.Protocol.Types (CodeAction (..),
CodeActionKind (CodeActionKind_RefactorRewrite),
CodeActionParams (..),
Command, TextEdit (..),
Uri (..),
WorkspaceEdit (WorkspaceEdit),
fromNormalizedUri,
normalizedFilePathToUri,
type (|?) (..))
import Language.LSP.Server (getClientCapabilities)
data Log
= LogShake Shake.Log
| LogCollectedRecordSelectors [RecordSelectorExpr]
Expand All @@ -105,7 +116,8 @@ instance Hashable CollectRecordSelectors
instance NFData CollectRecordSelectors

data CollectRecordSelectorsResult = CRSR
{ recordInfos :: RangeMap RecordSelectorExpr
{ records :: RangeMap Int
joyfulmantis marked this conversation as resolved.
Show resolved Hide resolved
, recordInfos :: IntMap.IntMap RecordSelectorExpr
joyfulmantis marked this conversation as resolved.
Show resolved Hide resolved
, enabledExtensions :: [Extension]
}
deriving (Generic)
Expand Down Expand Up @@ -135,56 +147,100 @@ instance Pretty RecordSelectorExpr where
instance NFData RecordSelectorExpr where
rnf = rwhnf

data ORDResolveData = ORDRD {
Copy link
Collaborator

Choose a reason for hiding this comment

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

WDYT about the idea of just reusing the CodeActionParams?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

At least in this case, the CodeActionParams doesn't make sense. The problem with the codeActionParams is we are going to need to do processing anyways to know whether we can provide the codeAction, and right now it's a title too, so it makes sense to just process once instead of once to present and once to execute

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 think it definitely makes sense that we'll want a "stateful version" of resolve-based handlers. Maybe we'll also want a stateless one... I think there are some plugins that don't define any of their own rules so don't even have anywhere to put state. But perhaps easier to do a few and then refactor afterwards.

uri :: Uri
, uniqueID :: Int
} deriving (Generic, Show)
instance ToJSON ORDResolveData
instance FromJSON ORDResolveData

descriptor :: Recorder (WithPriority Log) -> PluginId
-> PluginDescriptor IdeState
descriptor recorder plId = (defaultPluginDescriptor plId)
{ pluginHandlers =
mkPluginHandler SMethod_TextDocumentCodeAction codeActionProvider
mkPluginHandler SMethod_TextDocumentCodeAction codeActionProvider
<> mkPluginHandler SMethod_CodeActionResolve resolveProvider

, pluginRules = collectRecSelsRule recorder
}

resolveProvider :: PluginMethodHandler IdeState 'Method_CodeActionResolve
resolveProvider ideState pId ca@(CodeAction _ _ _ _ _ _ _ (Just resData)) =
pluginResponse $ do
case fromJSON $ resData of
Success (ORDRD uri int) -> do
nfp <- getNormalizedFilePath uri
CRSR _ crsDetails exts <- collectRecSelResult ideState nfp
pragma <- getFirstPragma pId ideState nfp
let pragmaEdit =
if OverloadedRecordDot `elem` exts
then Nothing
else Just $ insertNewPragma pragma OverloadedRecordDot
edits (Just crs) = convertRecordSelectors crs : maybeToList pragmaEdit
edits _ = []
changes = Just $ WorkspaceEdit
(Just (Map.singleton (fromNormalizedUri
(normalizedFilePathToUri nfp))
(edits (IntMap.lookup int crsDetails))))
Nothing Nothing
pure $ ca {_edit = changes}
_ -> pure ca

codeActionProvider :: PluginMethodHandler IdeState 'Method_TextDocumentCodeAction
codeActionProvider ideState pId (CodeActionParams _ _ caDocId caRange _) =
pluginResponse $ do
nfp <- getNormalizedFilePath (caDocId ^. L.uri)
pragma <- getFirstPragma pId ideState nfp
CRSR crsMap exts <- collectRecSelResult ideState nfp
let pragmaEdit =
caps <- lift getClientCapabilities
CRSR crsMap crsDetails exts <- collectRecSelResult ideState nfp
let supportsResolve :: Maybe Bool
supportsResolve = caps ^? L.textDocument . _Just . L.codeAction . _Just . L.dataSupport . _Just
joyfulmantis marked this conversation as resolved.
Show resolved Hide resolved
pragmaEdit =
if OverloadedRecordDot `elem` exts
then Nothing
else Just $ insertNewPragma pragma OverloadedRecordDot
edits crs = convertRecordSelectors crs : maybeToList pragmaEdit
changes crs =
Just $ Map.singleton (fromNormalizedUri
edits (Just crs) = convertRecordSelectors crs : maybeToList pragmaEdit
edits _ = []
changes crsM crsD =
case supportsResolve of
Just False -> Just $ WorkspaceEdit
joyfulmantis marked this conversation as resolved.
Show resolved Hide resolved
(Just (Map.singleton (fromNormalizedUri
(normalizedFilePathToUri nfp))
(edits crs)
mkCodeAction crs = InR CodeAction
(edits (IntMap.lookup crsM crsD))))
Nothing Nothing
_ -> Nothing
resolveData crsM =
case supportsResolve of
Just True -> Just $ toJSON $ ORDRD (caDocId ^. L.uri) crsM
_ -> Nothing
mkCodeAction crsD crsM = InR CodeAction
{ -- We pass the record selector to the title function, so that
-- we can have the name of the record selector in the title of
-- the codeAction. This allows the user can easily distinguish
-- between the different codeActions when using nested record
-- selectors, the disadvantage is we need to print out the
-- name of the record selector which will decrease performance
_title = mkCodeActionTitle exts crs
_title = mkCodeActionTitle exts crsM crsD
, _kind = Just CodeActionKind_RefactorRewrite
, _diagnostics = Nothing
, _isPreferred = Nothing
, _disabled = Nothing
, _edit = Just $ WorkspaceEdit (changes crs) Nothing Nothing
, _edit = changes crsM crsD
, _command = Nothing
, _data_ = Nothing
, _data_ = resolveData crsM
}
actions = map mkCodeAction (RangeMap.filterByRange caRange crsMap)
actions = map (mkCodeAction crsDetails) (RangeMap.filterByRange caRange crsMap)
pure $ InL actions
where
mkCodeActionTitle :: [Extension] -> RecordSelectorExpr-> Text
mkCodeActionTitle exts (RecordSelectorExpr _ se _) =
mkCodeActionTitle :: [Extension] -> Int -> IntMap.IntMap RecordSelectorExpr-> Text
mkCodeActionTitle exts crsM crsD =
if OverloadedRecordDot `elem` exts
then title
else title <> " (needs extension: OverloadedRecordDot)"
where
title = "Convert `" <> name <> "` to record dot syntax"
name = printOutputable se
title = "Convert `" <> name (IntMap.lookup crsM crsD) <> "` to record dot syntax"
name (Just (RecordSelectorExpr _ se _)) = printOutputable se
name _ = ""
joyfulmantis marked this conversation as resolved.
Show resolved Hide resolved

collectRecSelsRule :: Recorder (WithPriority Log) -> Rules ()
collectRecSelsRule recorder = define (cmapWithPrio LogShake recorder) $
Expand All @@ -201,11 +257,15 @@ collectRecSelsRule recorder = define (cmapWithPrio LogShake recorder) $
-- the OverloadedRecordDot pragma
exts = getEnabledExtensions tmr
recSels = mapMaybe (rewriteRange pm) (getRecordSelectors tmr)
uniques <- liftIO $ replicateM (length recSels) (hashUnique <$> newUnique)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is newUnique threadsafe? We run handlers in individual threads, so there could well be multiple instances of this running at once.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

great

Copy link
Collaborator

Choose a reason for hiding this comment

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

we might have to worry about contention on the ioref if we end up using this everywhere, but I expect that won't manifest until we're using it a lot. We should remember to try and check when we're nearly done

logWith recorder Debug (LogCollectedRecordSelectors recSels)
let -- We need the rangeMap to be able to filter by range later
crsMap :: RangeMap RecordSelectorExpr
crsMap = RangeMap.fromList location recSels
pure ([], CRSR <$> Just crsMap <*> Just exts)
let crsDetails = IntMap.fromList $ zip uniques recSels
-- We need the rangeMap to be able to filter by range later
rangeAndUnique = mapM (\x -> (, x) . location <$> IntMap.lookup x crsDetails) uniques
crsMap :: Maybe (RangeMap Int)
crsMap = RangeMap.fromList' <$> rangeAndUnique
crsDetails :: IntMap.IntMap RecordSelectorExpr
pure ([], CRSR <$> crsMap <*> Just crsDetails <*> Just exts)
where getEnabledExtensions :: TcModuleResult -> [Extension]
getEnabledExtensions = getExtensions . tmrParsed
getRecordSelectors :: TcModuleResult -> [RecordSelectorExpr]
Expand Down
Loading