-
-
Notifications
You must be signed in to change notification settings - Fork 352
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
add Method_TextDocumentSemanticTokensFullDelta #4073
Changes from 21 commits
43796d0
3f29007
ff93475
decd9e1
e1fd84e
41499b4
9b373f3
4f7546a
8af3b99
181b10a
9e7af72
cef51ed
fa908a0
e3ed49b
82b1eaf
e7a17e7
cc69775
49fd150
dcdbb27
7e7398a
e6ff123
9ef7796
b287639
154013b
42f6284
699dab7
a0b6ac7
4180c43
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 |
---|---|---|
|
@@ -10,19 +10,25 @@ | |
|
||
-- | | ||
-- This module provides the core functionality of the plugin. | ||
module Ide.Plugin.SemanticTokens.Internal (semanticTokensFull, getSemanticTokensRule, persistentGetSemanticTokensRule, semanticConfigProperties) where | ||
module Ide.Plugin.SemanticTokens.Internal (semanticTokensFull, getSemanticTokensRule, semanticConfigProperties, semanticTokensFullDelta) where | ||
|
||
import Control.Concurrent.STM (stateTVar) | ||
import Control.Concurrent.STM.Stats (atomically) | ||
import Control.Lens ((^.)) | ||
import Control.Monad.Except (ExceptT, liftEither, | ||
withExceptT) | ||
import Control.Monad.IO.Class (MonadIO (..)) | ||
import Control.Monad.Trans (lift) | ||
import Control.Monad.Trans.Except (runExceptT) | ||
import qualified Data.Map.Strict as M | ||
import Data.Text (Text) | ||
import qualified Data.Text as T | ||
import Development.IDE (Action, | ||
GetDocMap (GetDocMap), | ||
GetHieAst (GetHieAst), | ||
HieAstResult (HAR, hieAst, hieModule, refMap), | ||
IdeResult, IdeState, | ||
NormalizedUri, | ||
Priority (..), | ||
Recorder, Rules, | ||
WithPriority, | ||
|
@@ -31,10 +37,10 @@ import Development.IDE (Action, | |
hieKind, use_) | ||
import Development.IDE.Core.PluginUtils (runActionE, | ||
useWithStaleE) | ||
import Development.IDE.Core.PositionMapping (idDelta) | ||
import Development.IDE.Core.Rules (toIdeResult) | ||
import Development.IDE.Core.RuleTypes (DocAndTyThingMap (..)) | ||
import Development.IDE.Core.Shake (addPersistentRule, | ||
import Development.IDE.Core.Shake (ShakeExtras (..), | ||
getShakeExtras, | ||
getVirtualFile, | ||
useWithStale_) | ||
import Development.IDE.GHC.Compat hiding (Warning) | ||
|
@@ -51,11 +57,14 @@ import Ide.Plugin.SemanticTokens.Tokenize (computeRangeHsSemanti | |
import Ide.Plugin.SemanticTokens.Types | ||
import Ide.Types | ||
import qualified Language.LSP.Protocol.Lens as L | ||
import Language.LSP.Protocol.Message (Method (Method_TextDocumentSemanticTokensFull)) | ||
import Language.LSP.Protocol.Message (MessageResult, | ||
Method (Method_TextDocumentSemanticTokensFull, Method_TextDocumentSemanticTokensFullDelta)) | ||
import Language.LSP.Protocol.Types (NormalizedFilePath, | ||
SemanticTokens, | ||
type (|?) (InL)) | ||
normalizedFilePathToUri, | ||
type (|?) (InL, InR)) | ||
import Prelude hiding (span) | ||
import qualified StmContainers.Map as STM | ||
|
||
|
||
$mkSemanticConfigFunctions | ||
|
@@ -68,14 +77,38 @@ computeSemanticTokens :: Recorder (WithPriority SemanticLog) -> PluginId -> IdeS | |
computeSemanticTokens recorder pid _ nfp = do | ||
config <- lift $ useSemanticConfigAction pid | ||
logWith recorder Debug (LogConfig config) | ||
semanticId <- lift getAndIncreaseSemanticTokensId | ||
soulomoon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
(RangeHsSemanticTokenTypes {rangeSemanticList}, mapping) <- useWithStaleE GetSemanticTokens nfp | ||
withExceptT PluginInternalError $ liftEither $ rangeSemanticsSemanticTokens config mapping rangeSemanticList | ||
withExceptT PluginInternalError $ liftEither $ rangeSemanticsSemanticTokens semanticId config mapping rangeSemanticList | ||
|
||
semanticTokensFull :: Recorder (WithPriority SemanticLog) -> PluginMethodHandler IdeState 'Method_TextDocumentSemanticTokensFull | ||
semanticTokensFull recorder state pid param = do | ||
semanticTokensFull recorder state pid param = runActionE "SemanticTokens.semanticTokensFull" state computeSemanticTokensFullDelta | ||
where | ||
computeSemanticTokensFullDelta :: ExceptT PluginError Action (MessageResult Method_TextDocumentSemanticTokensFull) | ||
soulomoon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
computeSemanticTokensFullDelta = do | ||
nfp <- getNormalizedFilePathE (param ^. L.textDocument . L.uri) | ||
items <- computeSemanticTokens recorder pid state nfp | ||
lift $ setSemanticTokens (normalizedFilePathToUri nfp) items | ||
soulomoon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return $ InL items | ||
|
||
|
||
semanticTokensFullDelta :: Recorder (WithPriority SemanticLog) -> PluginMethodHandler IdeState 'Method_TextDocumentSemanticTokensFullDelta | ||
semanticTokensFullDelta recorder state pid param = do | ||
nfp <- getNormalizedFilePathE (param ^. L.textDocument . L.uri) | ||
items <- runActionE "SemanticTokens.semanticTokensFull" state $ computeSemanticTokens recorder pid state nfp | ||
return $ InL items | ||
let previousVersionFromParam = param ^. L.previousResultId | ||
runActionE "SemanticTokens.semanticTokensFullDelta" state $ computeSemanticTokensFullDelta recorder previousVersionFromParam pid state nfp | ||
where | ||
computeSemanticTokensFullDelta :: Recorder (WithPriority SemanticLog) -> Text -> PluginId -> IdeState -> NormalizedFilePath -> ExceptT PluginError Action (MessageResult Method_TextDocumentSemanticTokensFullDelta) | ||
computeSemanticTokensFullDelta recorder previousVersionFromParam pid state nfp = do | ||
semanticTokens <- computeSemanticTokens recorder pid state nfp | ||
previousSemanticTokensMaybe <- lift $ getPreviousSemanticTokens (normalizedFilePathToUri nfp) | ||
lift $ setSemanticTokens (normalizedFilePathToUri nfp) semanticTokens | ||
case previousSemanticTokensMaybe of | ||
Nothing -> return $ InL semanticTokens | ||
Just previousSemanticTokens -> | ||
if Just previousVersionFromParam == previousSemanticTokens^.L.resultId | ||
then return $ InR $ InL $ makeSemanticTokensDeltaWithId (semanticTokens^.L.resultId) previousSemanticTokens semanticTokens | ||
else return $ InL semanticTokens | ||
soulomoon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
-- | Defines the 'getSemanticTokensRule' function, compute semantic tokens for a Haskell source file. | ||
-- | ||
|
@@ -98,9 +131,6 @@ getSemanticTokensRule recorder = | |
let hsFinder = idSemantic getTyThingMap (hieKindFunMasksKind hieKind) refMap | ||
return $ computeRangeHsSemanticTokenTypeList hsFinder virtualFile ast | ||
|
||
-- | Persistent rule to ensure that semantic tokens doesn't block on startup | ||
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. why drop 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. For semantic tokens, Also we are not using IdeAction with The persistent rule is rather useless for us and might shadow some actual failures. 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. Hmm, so does this mean we have to wait quite a while to get responses when we start up? 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. yes, as we have been. Persistent rule won't help us here(Since it is not |
||
persistentGetSemanticTokensRule :: Rules () | ||
persistentGetSemanticTokensRule = addPersistentRule GetSemanticTokens $ \_ -> pure $ Just (RangeHsSemanticTokenTypes mempty, idDelta, Nothing) | ||
|
||
-- taken from /haskell-language-server/plugins/hls-code-range-plugin/src/Ide/Plugin/CodeRange/Rules.hs | ||
|
||
|
@@ -113,3 +143,20 @@ handleError recorder action' = do | |
logWith recorder Warning msg | ||
pure $ toIdeResult (Left []) | ||
Right value -> pure $ toIdeResult (Right value) | ||
|
||
----------------------- | ||
-- helper functions | ||
----------------------- | ||
|
||
getAndIncreaseSemanticTokensId :: Action SemanticTokenId | ||
getAndIncreaseSemanticTokensId = do | ||
ShakeExtras{semanticTokensId} <- getShakeExtras | ||
liftIO $ atomically $ do | ||
i <- stateTVar semanticTokensId (\val -> (val, val+1)) | ||
return $ T.pack $ show i | ||
|
||
getPreviousSemanticTokens :: NormalizedUri -> Action (Maybe SemanticTokens) | ||
getPreviousSemanticTokens uri = getShakeExtras >>= liftIO . atomically . STM.lookup uri . semanticTokensCache | ||
|
||
setSemanticTokens :: NormalizedUri -> SemanticTokens -> Action () | ||
setSemanticTokens uri tokens = getShakeExtras >>= liftIO . atomically . STM.insert tokens uri . semanticTokensCache | ||
fendor marked this conversation as resolved.
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these could probably be variables local to the plugin? I think we only need to put thing in here if they're really shared across multiple rules etc.
So the function that creates the plugin would run in
IO
or similar and create the TVars, which then get passed into the handlers.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree that putting it in shakeExtras is suboptimal, but I am also not sure about local state to plugin too.
Since no other plugins have their own plugin state. It would mean we need to change
idePlugins :: Recorder (WithPriority Log) -> IdePlugins IdeState
toidePlugins :: Recorder (WithPriority Log) -> IO (IdePlugins IdeState)
.And open a door to introduce state to plugins breaking the current centralized state management in the hls build system. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think plugin-local state is inherently problematic. But I admit that it's not totally clear to me what stuff should be in
ShakeExtras
🤔 thoughts @fendor ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a comment to the shakeExtra stating it might not be ideal to store the varaibles there, but we do not find a better place to store it yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not have any specific thoughts, if the variable could be easily plugin local, I think I would prefer that.