-
-
Notifications
You must be signed in to change notification settings - Fork 391
Resolve 1: Support for resolve in overloaded-record-dot #3658
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
Changes from all commits
bb742b3
be71eb9
fb21134
f347ebc
c19480d
5e37f6f
4bcd45b
7d4f01e
225152e
4b34265
9985195
355e95c
2e4d14c
fb49c31
d1d299b
e025840
0b57d5a
735feca
6b3b915
1ba6098
7e9bf1d
0271ce2
794034b
7790755
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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.Except (ExceptT) | ||
import Control.Monad.Trans.Class (lift) | ||
import Control.Monad.Trans.Except (ExceptT, throwE) | ||
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 (..), | ||
|
@@ -75,18 +83,22 @@ import Ide.Types (PluginDescriptor (..), | |
PluginId (..), | ||
PluginMethodHandler, | ||
defaultPluginDescriptor, | ||
mkCodeActionHandlerWithResolve, | ||
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 (..), | ||
WorkspaceEdit (WorkspaceEdit), | ||
Uri (..), | ||
WorkspaceEdit (WorkspaceEdit, _changeAnnotations, _changes, _documentChanges), | ||
fromNormalizedUri, | ||
normalizedFilePathToUri, | ||
type (|?) (..)) | ||
import Language.LSP.Server (getClientCapabilities) | ||
data Log | ||
= LogShake Shake.Log | ||
| LogCollectedRecordSelectors [RecordSelectorExpr] | ||
|
@@ -105,7 +117,14 @@ instance Hashable CollectRecordSelectors | |
instance NFData CollectRecordSelectors | ||
|
||
data CollectRecordSelectorsResult = CRSR | ||
{ recordInfos :: RangeMap RecordSelectorExpr | ||
{ -- |We store everything in here that we need to create the unresolved | ||
-- codeAction: the range, an uniquely identifiable int, and the selector | ||
--selector expression (HSExpr) that we use to generate the name | ||
records :: RangeMap (Int, HsExpr (GhcPass 'Renamed)) | ||
-- |This is for when we need to fully generate a textEdit. It contains the | ||
-- whole expression we are interested in indexed to the unique id we got | ||
-- from the previous field | ||
, recordInfos :: IntMap.IntMap RecordSelectorExpr | ||
joyfulmantis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
, enabledExtensions :: [Extension] | ||
} | ||
deriving (Generic) | ||
|
@@ -135,56 +154,85 @@ instance Pretty RecordSelectorExpr where | |
instance NFData RecordSelectorExpr where | ||
rnf = rwhnf | ||
|
||
-- |The data that is serialized and placed in the data field of resolvable | ||
-- code actions | ||
data ORDResolveData = ORDRD { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WDYT about the idea of just reusing the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
-- |We need the uri to get shake results | ||
uri :: Uri | ||
-- |The unique id that allows us to find the specific codeAction we want | ||
, 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 | ||
mkCodeActionHandlerWithResolve codeActionProvider resolveProvider | ||
, pluginRules = collectRecSelsRule recorder | ||
} | ||
|
||
resolveProvider :: PluginMethodHandler IdeState 'Method_CodeActionResolve | ||
resolveProvider ideState pId ca@(CodeAction _ _ _ _ _ _ _ (Just resData)) = | ||
pluginResponse $ do | ||
case fromJSON resData of | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just flagging that this seems like it's going to be a pretty much universal pattern for resolve handlers: the first thing they're going to do is decode their data from the data field. So we might want to make it part of the generic machinery. |
||
Success (ORDRD uri int) -> do | ||
nfp <- getNormalizedFilePath uri | ||
CRSR _ crsDetails exts <- collectRecSelResult ideState nfp | ||
pragma <- getFirstPragma pId ideState nfp | ||
case IntMap.lookup int crsDetails of | ||
Just rse -> pure $ ca {_edit = mkWorkspaceEdit uri rse exts pragma} | ||
-- We need to throw a content modified error here, see | ||
-- https://github.com/microsoft/language-server-protocol/issues/1738 | ||
-- but we need fendor's plugin error response pr to make it | ||
-- convenient to use here, so we will wait to do that till that's merged | ||
_ -> throwE "Content Modified Error" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at the spec, I'm not sure this is the correct meaning of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was mentioned in a GitHub issue that this was the way to do it. microsoft/language-server-protocol#1738 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, let's link to that when we do this! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bonus points: make a PR upstream to clarify in the spec that this is what you should do! |
||
_ -> throwE "Unable to deserialize the data" | ||
|
||
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 = | ||
if OverloadedRecordDot `elem` exts | ||
then Nothing | ||
else Just $ insertNewPragma pragma OverloadedRecordDot | ||
edits crs = convertRecordSelectors crs : maybeToList pragmaEdit | ||
changes crs = | ||
Just $ Map.singleton (fromNormalizedUri | ||
(normalizedFilePathToUri nfp)) | ||
(edits crs) | ||
mkCodeAction crs = InR CodeAction | ||
CRSR crsMap crsDetails exts <- collectRecSelResult ideState nfp | ||
let mkCodeAction (crsM, nse) = 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 nse | ||
, _kind = Just CodeActionKind_RefactorRewrite | ||
, _diagnostics = Nothing | ||
, _isPreferred = Nothing | ||
, _disabled = Nothing | ||
, _edit = Just $ WorkspaceEdit (changes crs) Nothing Nothing | ||
, _edit = Nothing | ||
, _command = Nothing | ||
, _data_ = Nothing | ||
, _data_ = Just $ toJSON $ ORDRD (caDocId ^. L.uri) crsM | ||
} | ||
actions = map mkCodeAction (RangeMap.filterByRange caRange crsMap) | ||
pure $ InL actions | ||
where | ||
mkCodeActionTitle :: [Extension] -> RecordSelectorExpr-> Text | ||
mkCodeActionTitle exts (RecordSelectorExpr _ se _) = | ||
mkCodeActionTitle :: [Extension] -> Int -> HsExpr (GhcPass 'Renamed) -> Text | ||
mkCodeActionTitle exts crsM se = | ||
if OverloadedRecordDot `elem` exts | ||
then title | ||
else title <> " (needs extension: OverloadedRecordDot)" | ||
where | ||
title = "Convert `" <> name <> "` to record dot syntax" | ||
name = printOutputable se | ||
title = "Convert `" <> printOutputable se <> "` to record dot syntax" | ||
|
||
mkWorkspaceEdit:: Uri -> RecordSelectorExpr -> [Extension] -> NextPragmaInfo-> Maybe WorkspaceEdit | ||
mkWorkspaceEdit uri recSel exts pragma = | ||
Just $ WorkspaceEdit | ||
{ _changes = | ||
Just (Map.singleton uri (convertRecordSelectors recSel : maybeToList pragmaEdit)) | ||
, _documentChanges = Nothing | ||
, _changeAnnotations = Nothing} | ||
where pragmaEdit = | ||
if OverloadedRecordDot `elem` exts | ||
then Nothing | ||
else Just $ insertNewPragma pragma OverloadedRecordDot | ||
|
||
collectRecSelsRule :: Recorder (WithPriority Log) -> Rules () | ||
collectRecSelsRule recorder = define (cmapWithPrio LogShake recorder) $ | ||
|
@@ -201,11 +249,17 @@ collectRecSelsRule recorder = define (cmapWithPrio LogShake recorder) $ | |
-- the OverloadedRecordDot pragma | ||
exts = getEnabledExtensions tmr | ||
recSels = mapMaybe (rewriteRange pm) (getRecordSelectors tmr) | ||
-- We are creating a list as long as our rec selectors of unique int s | ||
-- created by calling hashUnique on a Unique. The reason why we are | ||
-- extracting the ints is because they don't need any work to serialize. | ||
uniques <- liftIO $ replicateM (length recSels) (hashUnique <$> newUnique) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. great There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 crsUniquesAndDetails = zip uniques recSels | ||
-- We need the rangeMap to be able to filter by range later | ||
rangeAndUnique = toRangeAndUnique <$> crsUniquesAndDetails | ||
crsMap :: RangeMap (Int, HsExpr (GhcPass 'Renamed)) | ||
crsMap = RangeMap.fromList' rangeAndUnique | ||
pure ([], CRSR <$> Just crsMap <*> Just (IntMap.fromList crsUniquesAndDetails) <*> Just exts) | ||
where getEnabledExtensions :: TcModuleResult -> [Extension] | ||
getEnabledExtensions = getExtensions . tmrParsed | ||
getRecordSelectors :: TcModuleResult -> [RecordSelectorExpr] | ||
|
@@ -217,6 +271,7 @@ collectRecSelsRule recorder = define (cmapWithPrio LogShake recorder) $ | |
case toCurrentRange pm (location recSel) of | ||
Just newLoc -> Just $ recSel{location = newLoc} | ||
Nothing -> Nothing | ||
toRangeAndUnique (id, RecordSelectorExpr l (unLoc -> se) _) = (l, (id, se)) | ||
|
||
convertRecordSelectors :: RecordSelectorExpr -> TextEdit | ||
convertRecordSelectors (RecordSelectorExpr l se re) = | ||
|
Uh oh!
There was an error while loading. Please reload this page.