Skip to content

Add PluginId to CommandFunction & make getNormalizedFilePath takes Uri as pram instead of TextDocumentIdentifier #2906

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

Closed
wants to merge 3 commits into from
Closed

Add PluginId to CommandFunction & make getNormalizedFilePath takes Uri as pram instead of TextDocumentIdentifier #2906

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0c187d4
Add PluginId to CommandFunction
July541 May 14, 2022
396642c
Add missing imports
July541 May 14, 2022
5f27422
Remove redundant imports
July541 May 14, 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 ghcide/src/Development/IDE/Plugin/Completions.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ extendImportCommand =
PluginCommand (CommandId extendImportCommandId) "additional edits for a completion" extendImportHandler

extendImportHandler :: CommandFunction IdeState ExtendImport
extendImportHandler ideState edit@ExtendImport {..} = do
extendImportHandler ideState _ edit@ExtendImport {..} = do
res <- liftIO $ runMaybeT $ extendImportHandler' ideState edit
whenJust res $ \(nfp, wedit@WorkspaceEdit {_changes}) -> do
let (_, List (head -> TextEdit {_range})) = fromJust $ _changes >>= listToMaybe . toList
Expand Down
4 changes: 2 additions & 2 deletions ghcide/src/Development/IDE/Plugin/HLS.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@ import Ide.Plugin.Config
import Ide.PluginUtils (getClientConfig)
import Ide.Types as HLS
import qualified Language.LSP.Server as LSP
import Language.LSP.VFS
import Language.LSP.Types
import qualified Language.LSP.Types as J
import Language.LSP.VFS
import Text.Regex.TDFA.Text ()
import UnliftIO (MonadUnliftIO)
import UnliftIO.Async (forConcurrently)
Expand Down Expand Up @@ -149,7 +149,7 @@ executeCommandHandlers ecs = requestHandler SWorkspaceExecuteCommand execCmd
ResponseError InvalidParams ("error while parsing args for " <> com' <> " in plugin " <> p'
<> ": " <> T.pack err
<> "\narg = " <> T.pack (show arg)) Nothing
J.Success a -> f ide a
J.Success a -> f ide p a

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

Expand Down
4 changes: 2 additions & 2 deletions ghcide/src/Development/IDE/Plugin/Test.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,10 @@ import Data.Maybe (isJust)
import Data.String
import Data.Text (Text, pack)
import Development.IDE.Core.OfInterest (getFilesOfInterest)
import Development.IDE.Core.Rules
import Development.IDE.Core.RuleTypes
import Development.IDE.Core.Service
import Development.IDE.Core.Shake
import Development.IDE.Core.Rules
import Development.IDE.GHC.Compat
import Development.IDE.Graph (Action)
import qualified Development.IDE.Graph as Graph
Expand Down Expand Up @@ -170,7 +170,7 @@ blockCommandDescriptor plId = (defaultPluginDescriptor plId) {
}

blockCommandHandler :: CommandFunction state ExecuteCommandParams
blockCommandHandler _ideState _params = do
blockCommandHandler _ideState _plId _params = do
LSP.sendNotification (SCustomMethod "ghcide/blocking/command") Null
liftIO $ threadDelay maxBound
return (Right Null)
4 changes: 2 additions & 2 deletions ghcide/src/Development/IDE/Plugin/TypeLenses.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@ import Development.IDE (GhcSession (..),
RuleResult, Rules, define,
srcSpanToRange)
import Development.IDE.Core.Compile (TcModuleResult (..))
import Development.IDE.Core.Rules (IdeState, runAction)
import Development.IDE.Core.RuleTypes (GetBindings (GetBindings),
TypeCheck (TypeCheck))
import Development.IDE.Core.Rules (IdeState, runAction)
import Development.IDE.Core.Service (getDiagnostics)
import Development.IDE.Core.Shake (getHiddenDiagnostics, use)
import qualified Development.IDE.Core.Shake as Shake
Expand Down Expand Up @@ -146,7 +146,7 @@ generateLens pId _range title edit =
in CodeLens _range (Just cId) Nothing

commandHandler :: CommandFunction IdeState WorkspaceEdit
commandHandler _ideState wedit = do
commandHandler _ideState _plId wedit = do
_ <- LSP.sendRequest SWorkspaceApplyEdit (ApplyWorkspaceEditParams Nothing wedit) (\_ -> pure ())
return $ Right Null

Expand Down
9 changes: 4 additions & 5 deletions hls-plugin-api/src/Ide/PluginUtils.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,13 +246,12 @@ allLspCmdIds pid commands = concatMap go commands

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

getNormalizedFilePath :: Monad m => PluginId -> TextDocumentIdentifier -> ExceptT String m NormalizedFilePath
getNormalizedFilePath (PluginId plId) docId = handleMaybe errMsg
getNormalizedFilePath :: Monad m => PluginId -> Uri -> ExceptT String m NormalizedFilePath
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is indeed much clear and easy to use, I saw many codes can use uri directly instead of TextDocumentIdentifier.🎉

getNormalizedFilePath (PluginId plId) uri = handleMaybe errMsg
$ uriToNormalizedFilePath
$ toNormalizedUri uri'
$ toNormalizedUri uri
where
errMsg = T.unpack $ "Error(" <> plId <> "): converting " <> getUri uri' <> " to NormalizedFilePath"
uri' = docId ^. uri
errMsg = T.unpack $ "Error(" <> plId <> "): converting " <> getUri uri <> " to NormalizedFilePath"

-- ---------------------------------------------------------------------
handleMaybe :: Monad m => e -> Maybe b -> ExceptT e m b
Expand Down
3 changes: 2 additions & 1 deletion hls-plugin-api/src/Ide/Types.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,10 @@ import System.Posix.Signals
#endif
import Control.Lens ((^.))
import Data.Aeson hiding (defaultOptions)
import qualified Data.DList as DList
import qualified Data.Default
import Data.Dependent.Map (DMap)
import qualified Data.Dependent.Map as DMap
import qualified Data.DList as DList
import Data.GADT.Compare
import Data.List.NonEmpty (NonEmpty (..), toList)
import qualified Data.Map as Map
Expand Down Expand Up @@ -389,6 +389,7 @@ data PluginCommand ideState = forall a. (FromJSON a) =>

type CommandFunction ideState a
= ideState
-> PluginId
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure this is an improvement. The other arguments to CommandFunction are things that are only known when the command is run, so we need to pass them in then. But the PluginId is known when the plugin is created, so we can easily pass it in there. Doing it that way is pretty straightforward, as the tactics plugin does today.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Plus, most of the existing ones don't use it, so we've also added a bunch of unused parameters...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only the command parameter is needed to be passed, I think ideState has the same condition as pluginId, we can also pass ideState while we only need it.

Here are the stat about the usage of CommandFunction.

8/11 use ideState, 3/11 use PluginId, and most of plugins don't use PluginId because they use MaybeT to wrap NormalizedFilePath conversion, I keep this pattern in the pr. I will respect your view if you still believe this change is of little significance.

plugin name ideState PluginId
completion ✓ ×
typelense × ×
class ✓ ×
eval ✓ ✓
explicit-import × ×
hlint ✓ ✓
module-name ✓ ×
refine-import × ×
retrie ✓ ×
splice ✓ ×
tactics ✓ ✓

Copy link
Collaborator

Choose a reason for hiding this comment

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

If I may add, like I mentioned earlier I want to clean up all of the duplicate code revolving around MaybeT. So I would plan on removing the MaybeT wrappers. It may end up being more daunting but I expect to need to use some sort of identifier to use in logging messages.

This would also extend to other commonly requested items as well. So it would be nice to have some generic identifier to pass around.

Copy link
Collaborator

Choose a reason for hiding this comment

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

TBH I also think it looks like we shouldn't pass IdeState like that, if it truly is globally constant. I'm a bit suspicious that it isn't truly constant though, we should check.

I think my argument is that it's unclear what should be part of CommandFunc. Just any convenient stuff? Then there's really no rule about what to put in there or not. Whereas with my proposal there is a simple rule: it should take exactly the stuff that's only available when you run the command, and nothing else. If you need anything else, just pass it in when you construct the plugin.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm a bit suspicious that it isn't truly constant though, we should check.

I think my changes have covered all usage of CommanFunc.

I think my argument is that it's unclear what should be part of CommandFunc. Just any convenient stuff? Then there's really no rule about what to put in there or not.

I believe CommandFunc is an abbreviation of its arguments and its result, and the arguments can be anything you may use.
I prefer to add something that may not be used to keep the CommandFunc be unified in most situations.

So I would plan on removing the MaybeT wrappers.

Agree with this.

As I said earlier, I'm going to close this and open a new pr to make getNormalizedFilePath takes Uri as pram instead of TextDocumentIdentifier only.

-> a
-> LspM Config (Either ResponseError Value)

Expand Down
4 changes: 2 additions & 2 deletions plugins/default/src/Ide/Plugin/Example.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ import Control.Monad.IO.Class
import Control.Monad.Trans.Maybe
import Data.Aeson
import Data.Functor
import qualified Data.HashMap.Strict as Map
import Data.Hashable
import qualified Data.HashMap.Strict as Map
import qualified Data.Text as T
import Data.Typeable
import Development.IDE as D
Expand Down Expand Up @@ -161,7 +161,7 @@ data AddTodoParams = AddTodoParams
deriving (Show, Eq, Generic, ToJSON, FromJSON)

addTodoCmd :: CommandFunction IdeState AddTodoParams
addTodoCmd _ide (AddTodoParams uri todoText) = do
addTodoCmd _ide _plId (AddTodoParams uri todoText) = do
let
pos = Position 3 0
textEdits = List
Expand Down
4 changes: 2 additions & 2 deletions plugins/default/src/Ide/Plugin/Example2.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ import Control.Monad.IO.Class
import Control.Monad.Trans.Maybe
import Data.Aeson
import Data.Functor
import qualified Data.HashMap.Strict as Map
import Data.Hashable
import qualified Data.HashMap.Strict as Map
import qualified Data.Text as T
import Data.Typeable
import Development.IDE as D
Expand Down Expand Up @@ -145,7 +145,7 @@ data AddTodoParams = AddTodoParams
deriving (Show, Eq, Generic, ToJSON, FromJSON)

addTodoCmd :: CommandFunction IdeState AddTodoParams
addTodoCmd _ide (AddTodoParams uri todoText) = do
addTodoCmd _ide _plId (AddTodoParams uri todoText) = do
let
pos = Position 5 0
textEdits = List
Expand Down
2 changes: 1 addition & 1 deletion plugins/hls-alternate-number-format-plugin/hls-alternate-number-format-plugin.cabal
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ library
, ghcide ^>=1.6 || ^>=1.7
, ghc-boot-th
, hls-graph
, hls-plugin-api ^>=1.3 || ^>=1.4
, hls-plugin-api ^>=1.4
, hie-compat
, lens
, lsp
Expand Down
19 changes: 6 additions & 13 deletions plugins/hls-alternate-number-format-plugin/src/Ide/Plugin/AlternateNumberFormat.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,8 @@ import Development.IDE (GetParsedModule (GetParsedModu
GhcSession (GhcSession),
IdeState, RuleResult, Rules,
define, getFileContents,
hscEnv, ideLogger,
realSrcSpanToRange, runAction,
use, useWithStale)
hscEnv, realSrcSpanToRange,
runAction, use, useWithStale)
import qualified Development.IDE.Core.Shake as Shake
import Development.IDE.GHC.Compat hiding (getSrcSpan)
import Development.IDE.GHC.Compat.Util (toList)
Expand All @@ -31,8 +30,8 @@ import Ide.Plugin.Conversion (AlternateFormat,
ExtensionNeeded (NeedsExtension, NoExtension),
alternateFormat)
import Ide.Plugin.Literals
import Ide.PluginUtils (handleMaybe, handleMaybeM,
response)
import Ide.PluginUtils (getNormalizedFilePath,
handleMaybeM, response)
import Ide.Types
import Language.LSP.Types
import Language.LSP.Types.Lens (uri)
Expand Down Expand Up @@ -84,8 +83,8 @@ collectLiteralsRule recorder = define (cmapWithPrio LogShake recorder) $ \Collec
getExtensions = map GhcExtension . toList . extensionFlags . ms_hspp_opts . pm_mod_summary

codeActionHandler :: PluginMethodHandler IdeState 'TextDocumentCodeAction
codeActionHandler state _ (CodeActionParams _ _ docId currRange _) = response $ do
nfp <- getNormalizedFilePath docId
codeActionHandler state plId (CodeActionParams _ _ docId currRange _) = response $ do
nfp <- getNormalizedFilePath plId (docId ^. uri)
CLR{..} <- requestLiterals state nfp
pragma <- getFirstPragma state nfp
-- remove any invalid literals (see validTarget comment)
Expand Down Expand Up @@ -151,12 +150,6 @@ getFirstPragma state nfp = handleMaybeM "Error: Could not get NextPragmaInfo" $
Just (hscEnv -> hsc_dflags -> sessionDynFlags, _) -> pure $ Just $ getNextPragmaInfo sessionDynFlags fileContents
Nothing -> pure Nothing


getNormalizedFilePath :: Monad m => TextDocumentIdentifier -> ExceptT String m NormalizedFilePath
getNormalizedFilePath docId = handleMaybe "Error: converting to NormalizedFilePath"
$ uriToNormalizedFilePath
$ toNormalizedUri (docId ^. uri)

requestLiterals :: MonadIO m => IdeState -> NormalizedFilePath -> ExceptT String m CollectLiteralsResult
requestLiterals state = handleMaybeM "Error: Could not Collect Literals"
. liftIO
Expand Down
2 changes: 1 addition & 1 deletion plugins/hls-change-type-signature-plugin/hls-change-type-signature-plugin.cabal
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ library
build-depends:
, base >=4.12 && < 5
, ghcide ^>=1.7
, hls-plugin-api ^>=1.3 || ^>=1.4
, hls-plugin-api ^>=1.4
, lsp-types
, regex-tdfa
, syb
Expand Down
2 changes: 1 addition & 1 deletion plugins/hls-change-type-signature-plugin/src/Ide/Plugin/ChangeTypeSignature.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ descriptor plId = (defaultPluginDescriptor plId) { pluginHandlers = mkPluginHand

codeActionHandler :: PluginMethodHandler IdeState 'TextDocumentCodeAction
codeActionHandler ideState plId CodeActionParams {_textDocument = TextDocumentIdentifier uri, _context = CodeActionContext (List diags) _} = response $ do
nfp <- getNormalizedFilePath plId (TextDocumentIdentifier uri)
nfp <- getNormalizedFilePath plId uri
decls <- getDecls ideState nfp
let actions = mapMaybe (generateAction uri decls) diags
pure $ List actions
Expand Down
6 changes: 3 additions & 3 deletions plugins/hls-class-plugin/src/Ide/Plugin/Class.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ import Data.Char
import Data.List
import qualified Data.Map.Strict as Map
import Data.Maybe
import qualified Data.Text as T
import qualified Data.Set as Set
import qualified Data.Text as T
import Development.IDE hiding (pluginHandlers)
import Development.IDE.Core.PositionMapping (fromCurrentRange,
toCurrentRange)
Expand All @@ -40,7 +40,7 @@ import Language.LSP.Types
import qualified Language.LSP.Types.Lens as J

#if MIN_VERSION_ghc(9,2,0)
import GHC.Hs (AnnsModule(AnnsModule))
import GHC.Hs (AnnsModule (AnnsModule))
import GHC.Parser.Annotation
#endif

Expand All @@ -64,7 +64,7 @@ data AddMinimalMethodsParams = AddMinimalMethodsParams
deriving (Show, Eq, Generics.Generic, ToJSON, FromJSON)

addMethodPlaceholders :: CommandFunction IdeState AddMinimalMethodsParams
addMethodPlaceholders state AddMinimalMethodsParams{..} = do
addMethodPlaceholders state _ AddMinimalMethodsParams{..} = do
caps <- getClientCapabilities
medit <- liftIO $ runMaybeT $ do
docPath <- MaybeT . pure . uriToNormalizedFilePath $ toNormalizedUri uri
Expand Down
2 changes: 1 addition & 1 deletion plugins/hls-eval-plugin/src/Ide/Plugin/Eval.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ descriptor :: Recorder (WithPriority Log) -> PluginId -> PluginDescriptor IdeSta
descriptor recorder plId =
(defaultPluginDescriptor plId)
{ pluginHandlers = mkPluginHandler STextDocumentCodeLens CL.codeLens
, pluginCommands = [CL.evalCommand plId]
, pluginCommands = [CL.evalCommand]
, pluginRules = rules (cmapWithPrio LogEvalRules recorder)
, pluginConfigDescriptor = defaultConfigDescriptor
{ configCustomConfig = mkCustomConfig properties
Expand Down
16 changes: 8 additions & 8 deletions plugins/hls-eval-plugin/src/Ide/Plugin/Eval/CodeLens.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,13 @@ import Development.IDE (GetModSummary (..),
GhcSessionIO (..), IdeState,
ModSummaryResult (..),
NeedsCompilation (NeedsCompilation),
evalGhcEnv,
VFSModified (..), evalGhcEnv,
hscEnvWithImportPaths,
printOutputable, runAction,
textToStringBuffer,
toNormalizedFilePath',
uriToFilePath', useNoFile_,
useWithStale_, use_,
VFSModified(..))
useWithStale_, use_)
import Development.IDE.Core.Rules (GhcSessionDepsConfig (..),
ghcSessionDepsDefinition)
import Development.IDE.GHC.Compat hiding (typeKind, unitState)
Expand Down Expand Up @@ -91,7 +90,8 @@ import Ide.Plugin.Eval.Code (Statement, asStatements,
evalSetup, myExecStmt,
propSetup, resultRange,
testCheck, testRanges)
import Ide.Plugin.Eval.Config (getEvalConfig, EvalConfig(..))
import Ide.Plugin.Eval.Config (EvalConfig (..),
getEvalConfig)
import Ide.Plugin.Eval.GHC (addImport, addPackages,
hasPackage, showDynFlags)
import Ide.Plugin.Eval.Parse.Comments (commentsToSections)
Expand Down Expand Up @@ -184,13 +184,13 @@ codeLens st plId CodeLensParams{_textDocument} =
evalCommandName :: CommandId
evalCommandName = "evalCommand"

evalCommand :: PluginId -> PluginCommand IdeState
evalCommand plId = PluginCommand evalCommandName "evaluate" (runEvalCmd plId)
evalCommand :: PluginCommand IdeState
evalCommand = PluginCommand evalCommandName "evaluate" runEvalCmd

type EvalId = Int

runEvalCmd :: PluginId -> CommandFunction IdeState EvalParams
runEvalCmd plId st EvalParams{..} =
runEvalCmd :: CommandFunction IdeState EvalParams
runEvalCmd st plId EvalParams{..} =
let dbg = logWith st
perf = timed dbg
cmd :: ExceptT String (LspM Config) WorkspaceEdit
Expand Down
4 changes: 2 additions & 2 deletions plugins/hls-explicit-imports-plugin/src/Ide/Plugin/ExplicitImports.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ import Data.IORef (readIORef)
import qualified Data.Map.Strict as Map
import Data.Maybe (catMaybes, fromMaybe,
isJust)
import qualified Data.Text as T
import Data.String (fromString)
import qualified Data.Text as T
import Development.IDE hiding (pluginHandlers,
pluginRules)
import Development.IDE.Core.PositionMapping
Expand Down Expand Up @@ -93,7 +93,7 @@ newtype ImportCommandParams = ImportCommandParams WorkspaceEdit

-- | The actual command handler
runImportCommand :: CommandFunction IdeState ImportCommandParams
runImportCommand _state (ImportCommandParams edit) = do
runImportCommand _state _ (ImportCommandParams edit) = do
-- This command simply triggers a workspace edit!
_ <- sendRequest SWorkspaceApplyEdit (ApplyWorkspaceEditParams Nothing edit) (\_ -> pure ())
return (Right Null)
Expand Down
Loading