add Method_TextDocumentSemanticTokensFullDelta#4073
Conversation
|
I'm on holiday but I'll take a look next week! |
Co-authored-by: fendor <fendor@users.noreply.github.com>
…ns/Internal.hs Co-authored-by: fendor <fendor@users.noreply.github.com>
Thanx for the reviews @fendor . Fixed most of them.
Sure. |
michaelpj
left a comment
There was a problem hiding this comment.
Mostly worried about adding lots of stuff to ShakeExtras if we don't have to.
| ,publishedDiagnostics :: STM.Map NormalizedUri [Diagnostic] | ||
| -- ^ This represents the set of diagnostics that we have published. | ||
| -- Due to debouncing not every change might get published. | ||
| ,semanticTokensCache:: STM.Map NormalizedUri SemanticTokens |
There was a problem hiding this comment.
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.
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 to idePlugins :: 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.
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.
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.
I do not have any specific thoughts, if the variable could be easily plugin local, I think I would prefer that.
| 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.
For semantic tokens,
lastValueIO return [](case with persistent key) or Nothing(case without persistent key).
Between faking a stale value [] and mark the semantic tokens rules' value actually Failed in shake state.
The former would hide the failure in both user of the rule and in the shake caching result(I don't think it is wise, we want to know if we fail in this case).
Also we are not using IdeAction with useWithStaleFastMT(Which look directly into the caching and demand instance response) for semantic tokens as in other IdeAction that requires instance response(e.g. hover, this fix startup of hover when the rule first compute without any stale value in the cach).
The persistent rule is rather useless for us and might shadow some actual failures.
There was a problem hiding this comment.
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.
yes, as we have been. Persistent rule won't help us here(Since it is not ideAction, the persistent rule serves only as a fall back when the actual computation of the rule failed and not already having a cache) .
|
Thanx for the reviews, welcom back from vacation! @michaelpj , I fixed some of them, and some might need discussion. |
| ,hiddenDiagnostics :: STMDiagnosticStore | ||
| ,publishedDiagnostics :: STM.Map NormalizedUri [Diagnostic] | ||
|
|
||
| -- storing semantic tokens for each file in shakeExtras might |
There was a problem hiding this comment.
this comment is in the wrong place, it's interrupting the haddock for the field above
There was a problem hiding this comment.
Maybe put it in a Note or something
* add Method_TextDocumentSemanticTokensFullDelta * remove persistentGetSemanticTokensRule * add doc about semanticTokensCache location * add Note [Semantic Tokens Cache Location] --------- Co-authored-by: fendor <fendor@users.noreply.github.com>
Mainly the following things have been done:
semanticTokensCacheandsemanticTokensIdto shake extras.Method_TextDocumentSemanticTokensFullDeltaMethod_TextDocumentSemanticTokensFullDeltapersistentGetSemanticTokensRulesince it is of no use since we do not useuseWithStaleFastMTfor semantic tokens rule.