Fully support use of workspace/configuration #512
Conversation
`lsp` will now rely primarily on `workspace/configuration` to get configuration from the client. See `Note [LSP configuration]` for details. `lsp-test` also now handles `workspace/configuration` properly.
lsp/src/Language/LSP/Server/Core.hs
Outdated
| import Language.LSP.Protocol.Utils.SMethodMap (SMethodMap) | ||
| import qualified Language.LSP.Protocol.Utils.SMethodMap as SMethodMap | ||
| import Language.LSP.VFS | ||
| import Language.LSP.Diagnostics | ||
| import System.Random hiding (next) | ||
| import Control.Monad.Trans.Identity | ||
| import Control.Monad.Catch (MonadMask, MonadCatch, MonadThrow) | ||
| import Prettyprinter |
| pretty (ConfigurationNotSupported) = "LSP: not requesting configuration since the client does not support workspace/configuration" | ||
| pretty (ConfigurationParseError settings err) = | ||
| vsep [ | ||
| "LSP: configuration parse error:" |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| , viaShow settings | ||
| ] | ||
| pretty (BadConfigurationResponse err) = "LSP: error when requesting configuration: " <+> viaShow err | ||
| pretty (WrongConfigSections sections) = "LSP: expected only one configuration section, got: " <+> viaShow sections |
There was a problem hiding this comment.
| pretty (WrongConfigSections sections) = "LSP: expected only one configuration section, got: " <+> viaShow sections | |
| pretty (WrongConfigSections sections) = "LSP: expect only one configuration section, got: " <+> viaShow sections |
Maybe?
| SMethod_WorkspaceDidChangeConfiguration -> handle' logger (Just $ handleConfigChange logger) m msg | ||
| SMethod_WorkspaceDidChangeConfiguration -> handle' logger (Just $ handleDidChangeConfiguration logger) m msg | ||
| -- See Note [LSP configuration] | ||
| SMethod_Initialized -> handle' logger (Just $ \_ -> requestConfigUpdate (cmap (fmap LspCore) logger)) m msg |
There was a problem hiding this comment.
We are requesting config update for every time initialized, considering downstream users, should we mark this as optional?
There was a problem hiding this comment.
initialized gets sent only once, which is when client receives our response to initialize. So this is a one-off thing.
There was a problem hiding this comment.
Right, it's not a big deal.
| section <- LspT $ asks resConfigSection | ||
| tryChangeConfig (cmap (fmap LspCore) logger) (lookForConfigSection section $ req ^. L.params . L.settings) | ||
| -- See Note [LSP configuration] | ||
| requestConfigUpdate (cmap (fmap LspCore) logger) |
There was a problem hiding this comment.
I'm confused about why also we need requestConfigUpdate if tryChangeConfig succeeds.
There was a problem hiding this comment.
Hmm, that's a good question. I guess if we actually manage to parse out what's in didChangeConfiguration then we probably don't need to request new config 🤔
Or maybe we should just fully commit to using workspace/configuration and not look in the body of didChangeConfiguration at all. That seems to be the way that the spec maintainers want us to go...
There was a problem hiding this comment.
Oh that's right, I wanted to support clients that don't handle workspace/configuration. So then parsing it out of didChangeConfiguration is important.
There was a problem hiding this comment.
I'm still confused here.. tryChangeConfig has updated the config, is there any situation we still need requestConfigUpdate?
There was a problem hiding this comment.
I'll write it out in detail in the doc. Basically, all of the following are "allowable" scenarios:
- Client supports
workspace/configuration, sends an empty update indidChangeConfiguration. This is what new vscode will do, I believe. Then the first attempt will fail (nothing indidChange), and the second will succeed (requesting config). - Client does not support
workspace/configuration, and sends updated config indidChangeConfiguration. This is rare, I think, but we should support it. Then the first attempt will succeed and the second attempt will fail (or not even try).
We do get some redundancy in the slightly odd case of a client that supports both ways, but I'm not too worried about that tbh.
| import Control.Monad.State | ||
| import Control.Monad.Writer.Strict | ||
| import Data.Foldable (traverse_) | ||
| import Data.String (fromString) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| -- indicating what went wrong. The parsed configuration object will be | ||
| -- stored internally and can be accessed via 'config'. | ||
| -- It is also called on the `initializationOptions` field of the InitializeParams | ||
| , configSection :: T.Text |
There was a problem hiding this comment.
Is this always have the same value with resConfigSection?
There was a problem hiding this comment.
Yeah, we copy over a bunch of fields from ServerDefinition into LanguageContextEnv...
| | ProgressCancel ProgressToken | ||
| | Exiting | ||
|
|
||
| deriving instance Show LspProcessingLog | ||
|
|
||
| instance Pretty LspProcessingLog where | ||
| pretty (VfsLog l) = pretty l | ||
| pretty (LspCore l) = pretty l | ||
| pretty (MessageProcessingError bs err) = | ||
| vsep [ | ||
| "LSP: incoming message parse error:" |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
lspwill now rely primarily onworkspace/configurationto get configuration from the client. SeeNote [LSP configuration]for details.lsp-testalso now handlesworkspace/configurationproperly.Fixes #510
I'm working on the HLS patch, we shouldn't merge this until that's working. But I'm fairly sure that the
lspandlsp-testchanges are good, so I'm putting this up for review.