Improve no plugin messages (#3864)

Author: joyfulmantis

ghcide/src/Development/IDE/Plugin/HLS.hs

@@ -23,6 +23,7 @@ import qualified Data.List                     as List
 import           Data.List.NonEmpty            (NonEmpty, nonEmpty, toList)
 import qualified Data.List.NonEmpty            as NE
 import qualified Data.Map                      as Map
+import           Data.Maybe                    (mapMaybe)
 import           Data.Some
 import           Data.String
 import           Data.Text                     (Text)
@@ -36,6 +37,7 @@ import qualified Development.IDE.Plugin        as P
 import           Ide.Logger
 import           Ide.Plugin.Config
 import           Ide.Plugin.Error
+import           Ide.Plugin.HandleRequestTypes
 import           Ide.PluginUtils               (getClientConfig)
 import           Ide.Types                     as HLS
 import           Language.LSP.Protocol.Message
@@ -65,23 +67,29 @@ instance Pretty Log where
     LogResponseError (PluginId pId) err ->
       pretty pId <> ":" <+> pretty err
     LogNoPluginForMethod (Some method) ->
-        "No plugin enabled for " <> pretty method
+        "No plugin handles this " <> pretty method <> " request."
     LogInvalidCommandIdentifier-> "Invalid command identifier"
     ExceptionInPlugin plId (Some method) exception ->
         "Exception in plugin " <> viaShow plId <> " while processing "
           <> pretty method <> ": " <> viaShow exception
 instance Show Log where show = renderString . layoutCompact . pretty
 
-noPluginEnabled :: Recorder (WithPriority Log) -> SMethod m -> [PluginId] -> IO (Either ResponseError c)
-noPluginEnabled recorder m fs' = do
+noPluginHandles :: Recorder (WithPriority Log) -> SMethod m -> [(PluginId, HandleRequestResult)] -> IO (Either ResponseError c)
+noPluginHandles recorder m fs' = do
   logWith recorder Warning (LogNoPluginForMethod $ Some m)
   let err = ResponseError (InR ErrorCodes_MethodNotFound) msg Nothing
-      msg = pluginNotEnabled m fs'
+      msg = noPluginHandlesMsg m fs'
   return $ Left err
-  where pluginNotEnabled :: SMethod m -> [PluginId] -> Text
-        pluginNotEnabled method availPlugins =
-            "No plugin enabled for " <> T.pack (show method) <> ", potentially available: "
-                <> (T.intercalate ", " $ map (\(PluginId plid) -> plid) availPlugins)
+  where noPluginHandlesMsg :: SMethod m -> [(PluginId, HandleRequestResult)] -> Text
+        noPluginHandlesMsg method [] = "No plugins are available to handle this " <> T.pack (show method) <> " request."
+        noPluginHandlesMsg method availPlugins =
+            "No plugins are available to handle this " <> T.pack (show method) <> " request.\n Plugins installed for this method, but not available to handle this request are:\n"
+                <> (T.intercalate "\n" $
+                      map (\(PluginId plid, pluginStatus) ->
+                              plid
+                              <> " "
+                              <> (renderStrict . layoutCompact . pretty) pluginStatus)
+                          availPlugins)
 
 pluginDoesntExist :: PluginId -> Text
 pluginDoesntExist (PluginId pid) = "Plugin " <> pid <> " doesn't exist"
@@ -213,8 +221,8 @@ executeCommandHandlers recorder ecs = requestHandler SMethod_WorkspaceExecuteCom
               res <- runExceptT (f ide a) `catchAny` -- See Note [Exception handling in plugins]
                 (\e -> pure $ Left $ PluginInternalError (exceptionInPlugin p SMethod_WorkspaceExecuteCommand e))
               case res of
-                (Left (PluginRequestRefused _)) ->
-                  liftIO $ noPluginEnabled recorder SMethod_WorkspaceExecuteCommand (fst <$> ecs)
+                (Left (PluginRequestRefused r)) ->
+                  liftIO $ noPluginHandles recorder SMethod_WorkspaceExecuteCommand [(p,DoesNotHandleRequest r)]
                 (Left pluginErr) -> do
                   liftIO $ logErrors recorder [(p, pluginErr)]
                   pure $ Left $ toResponseError (p, pluginErr)
@@ -236,11 +244,13 @@ extensiblePlugins recorder plugins = mempty { P.pluginHandlers = handlers }
       (IdeMethod m :=> IdeHandler fs') <- DMap.assocs handlers'
       pure $ requestHandler m $ \ide params -> do
         config <- Ide.PluginUtils.getClientConfig
-        -- Only run plugins that are allowed to run on this request
-        let fs = filter (\(_, desc, _) -> pluginEnabled m params desc config) fs'
+        -- Only run plugins that are allowed to run on this request, save the
+        -- list of disabled plugins incase that's all we have
+        let (fs, dfs) = List.partition (\(_, desc, _) -> handlesRequest m params desc config == HandlesRequest) fs'
+        let disabledPluginsReason = (\(x, desc, _) -> (x, handlesRequest m params desc config)) <$> dfs
         -- Clients generally don't display ResponseErrors so instead we log any that we come across
         case nonEmpty fs of
-          Nothing -> liftIO $ noPluginEnabled recorder m ((\(x, _, _) -> x) <$> fs')
+          Nothing -> liftIO $ noPluginHandles recorder m disabledPluginsReason
           Just neFs -> do
             let  plidsAndHandlers = fmap (\(plid,_,handler) -> (plid,handler)) neFs
             es <- runConcurrently exceptionInPlugin m plidsAndHandlers ide params
@@ -251,9 +261,12 @@ extensiblePlugins recorder plugins = mempty { P.pluginHandlers = handlers }
               Nothing -> do
                 let noRefused (_, PluginRequestRefused _) = False
                     noRefused (_, _)                      = True
-                    filteredErrs = filter noRefused errs
-                case nonEmpty filteredErrs of
-                  Nothing -> liftIO $ noPluginEnabled recorder m ((\(x, _, _) -> x) <$> fs')
+                    (asErrors, asRefused) = List.partition noRefused errs
+                    convertPRR (pId, PluginRequestRefused r) = Just (pId, DoesNotHandleRequest r)
+                    convertPRR _ = Nothing
+                    asRefusedReason = mapMaybe convertPRR asRefused
+                case nonEmpty asErrors of
+                  Nothing -> liftIO $ noPluginHandles recorder m  (disabledPluginsReason <> asRefusedReason)
                   Just xs -> pure $ Left $ combineErrors xs
               Just xs -> do
                 pure $ Right $ combineResponses m config caps params xs
@@ -274,8 +287,8 @@ extensibleNotificationPlugins recorder xs = mempty { P.pluginHandlers = handlers
       (IdeNotification m :=> IdeNotificationHandler fs') <- DMap.assocs handlers'
       pure $ notificationHandler m $ \ide vfs params -> do
         config <- Ide.PluginUtils.getClientConfig
-        -- Only run plugins that are allowed to run on this request
-        let fs = filter (\(_, desc, _) -> pluginEnabled m params desc config) fs'
+        -- Only run plugins that are enabled for this request
+        let fs = filter (\(_, desc, _) -> handlesRequest m params desc config == HandlesRequest) fs'
         case nonEmpty fs of
           Nothing -> do
             logWith recorder Warning (LogNoPluginForMethod $ Some m)

ghcide/test/exe/ExceptionTests.hs

@@ -18,6 +18,7 @@ import           GHC.Base                          (coerce)
 import           Ide.Logger                        (Logger, Recorder,
                                                     WithPriority, cmapWithPrio)
 import           Ide.Plugin.Error
+import           Ide.Plugin.HandleRequestTypes     (RejectionReason (DisabledGlobally))
 import           Ide.PluginUtils                   (idePluginsToPluginDesc,
                                                     pluginDescToIdePlugins)
 import           Ide.Types
@@ -106,9 +107,9 @@ tests recorder logger = do
                 _ -> liftIO $ assertFailure $ "We should have had an empty list" <> show lens]
 
    , testGroup "Testing PluginError order..."
-      [ pluginOrderTestCase recorder logger  "InternalError over InvalidParams" PluginInternalError PluginInvalidParams
-      , pluginOrderTestCase recorder logger  "InvalidParams over InvalidUserState" PluginInvalidParams PluginInvalidUserState
-      , pluginOrderTestCase recorder logger  "InvalidUserState over RequestRefused" PluginInvalidUserState PluginRequestRefused
+      [ pluginOrderTestCase recorder logger  "InternalError over InvalidParams" (PluginInternalError "error test") (PluginInvalidParams "error test")
+      , pluginOrderTestCase recorder logger  "InvalidParams over InvalidUserState" (PluginInvalidParams "error test") (PluginInvalidUserState "error test")
+      , pluginOrderTestCase recorder logger  "InvalidUserState over RequestRefused" (PluginInvalidUserState "error test") (PluginRequestRefused DisabledGlobally)
       ]
    ]
 
@@ -132,24 +133,24 @@ testingLite recorder logger plugins =
       , IDE.argsIdeOptions = ideOptions
       }
 
-pluginOrderTestCase :: Recorder (WithPriority Log) -> Logger -> TestName -> (T.Text -> PluginError) -> (T.Text -> PluginError) -> TestTree
+pluginOrderTestCase :: Recorder (WithPriority Log) -> Logger -> TestName -> PluginError -> PluginError -> TestTree
 pluginOrderTestCase recorder logger msg err1 err2 =
   testCase msg $ do
       let pluginId = "error-order-test"
           plugins = pluginDescToIdePlugins $
               [ (defaultPluginDescriptor pluginId "")
                   { pluginHandlers = mconcat
                       [ mkPluginHandler SMethod_TextDocumentCodeLens $ \_ _ _-> do
-                          throwError $ err1 "error test"
+                          throwError err1
                         ,mkPluginHandler SMethod_TextDocumentCodeLens $ \_ _ _-> do
-                          throwError $ err2 "error test"
+                          throwError err2
                       ]
                   }]
       testIde recorder (testingLite recorder logger plugins) $ do
           doc <- createDoc "A.hs" "haskell" "module A where"
           waitForProgressDone
           (view L.result -> lens) <- request SMethod_TextDocumentCodeLens (CodeLensParams Nothing Nothing doc)
           case lens of
-            Left re | toResponseError (pluginId, err1 "error test") == re -> pure ()
+            Left re | toResponseError (pluginId, err1) == re -> pure ()
                     | otherwise -> liftIO $ assertFailure "We caught an error, but it wasn't ours!"
             _ -> liftIO $ assertFailure $ show lens

hls-plugin-api/hls-plugin-api.cabal

@@ -38,6 +38,7 @@ library
     Ide.Plugin.Config
     Ide.Plugin.ConfigUtils
     Ide.Plugin.Error
+    Ide.Plugin.HandleRequestTypes
     Ide.Plugin.Properties
     Ide.Plugin.RangeMap
     Ide.Plugin.Resolve

hls-plugin-api/src/Ide/Plugin/Error.hs

@@ -11,11 +11,12 @@ module Ide.Plugin.Error (
     getNormalizedFilePathE,
 ) where
 
-import           Control.Monad.Extra         (maybeM)
-import           Control.Monad.Trans.Class   (lift)
-import           Control.Monad.Trans.Except  (ExceptT (..), throwE)
-import qualified Data.Text                   as T
+import           Control.Monad.Extra           (maybeM)
+import           Control.Monad.Trans.Class     (lift)
+import           Control.Monad.Trans.Except    (ExceptT (..), throwE)
+import qualified Data.Text                     as T
 import           Ide.Logger
+import           Ide.Plugin.HandleRequestTypes (RejectionReason)
 import           Language.LSP.Protocol.Types
 
 -- ----------------------------------------------------------------------------
@@ -79,13 +80,13 @@ data PluginError
   | PluginInvalidUserState T.Text
     -- |PluginRequestRefused allows your handler to inspect a request before
     -- rejecting it. In effect it allows your plugin to act make a secondary
-    -- `pluginEnabled` decision after receiving the request. This should only be
+    -- `handlesRequest` decision after receiving the request. This should only be
     -- used if the decision to accept the request can not be made in
-    -- `pluginEnabled`.
+    -- `handlesRequest`.
     --
     -- This error will be with Debug. If it's the only response to a request,
-    -- HLS will respond as if no plugins passed the `pluginEnabled` stage.
-  | PluginRequestRefused T.Text
+    -- HLS will respond as if no plugins passed the `handlesRequest` stage.
+  | PluginRequestRefused RejectionReason
     -- |PluginRuleFailed should be thrown when a Rule your response depends on
     -- fails.
     --

hls-plugin-api/src/Ide/Plugin/HandleRequestTypes.hs

@@ -0,0 +1,46 @@
+{-# LANGUAGE OverloadedStrings #-}
+
+module Ide.Plugin.HandleRequestTypes where
+
+import           Data.Text
+import           Prettyprinter
+
+-- | Reasons why a plugin could reject a specific request.
+data RejectionReason =
+  -- | The resolve request is not meant for this plugin or handler. The text
+  -- field should contain the identifier for the plugin who owns this resolve
+  -- request.
+  NotResolveOwner Text
+  -- | The plugin is disabled globally in the users config.
+  | DisabledGlobally
+  -- | The feature in the plugin that responds to this request is disabled in
+  -- the users config
+  | FeatureDisabled
+  -- | This plugin is not the formatting provider selected in the users config.
+  -- The text should be the formatting provider in your config.
+  | NotFormattingProvider Text
+  -- | This plugin does not support the file type. The text field here should
+  -- contain the filetype of the rejected request.
+  | DoesNotSupportFileType Text
+  deriving (Eq)
+
+-- | Whether a plugin will handle a request or not.
+data HandleRequestResult = HandlesRequest | DoesNotHandleRequest RejectionReason
+  deriving (Eq)
+
+instance Pretty HandleRequestResult where
+  pretty HandlesRequest                = "handles this request"
+  pretty (DoesNotHandleRequest reason) = pretty reason
+
+instance Pretty RejectionReason where
+  pretty (NotResolveOwner s) = "does not handle resolve requests for " <> pretty s <> ")."
+  pretty DisabledGlobally = "is disabled globally in your config."
+  pretty FeatureDisabled = "'s feature that handles this request is disabled in your config."
+  pretty (NotFormattingProvider s) = "is not the formatting provider ("<> pretty s<>") you chose in your config."
+  pretty (DoesNotSupportFileType s) = "does not support " <> pretty s <> " filetypes)."
+
+-- We always want to keep the leftmost disabled reason
+instance Semigroup HandleRequestResult where
+  HandlesRequest <> HandlesRequest = HandlesRequest
+  DoesNotHandleRequest r <> _      = DoesNotHandleRequest r
+  _ <> DoesNotHandleRequest r      = DoesNotHandleRequest r

hls-plugin-api/src/Ide/PluginUtils.hs

@@ -20,7 +20,7 @@ module Ide.PluginUtils
     getClientConfig,
     getPluginConfig,
     configForPlugin,
-    pluginEnabled,
+    handlesRequest,
     extractTextInRange,
     fullRange,
     mkLspCommand,