add Method_TextDocumentSemanticTokensFullDelta #4073

soulomoon · 2024-02-13T12:13:29Z

Mainly the following things have been done:

add semanticTokensCache and semanticTokensId to shake extras.
implement Method_TextDocumentSemanticTokensFullDelta
add a few test to test Method_TextDocumentSemanticTokensFullDelta
remove persistentGetSemanticTokensRule since it is of no use since we do not use useWithStaleFastMT for semantic tokens rule.

michaelpj · 2024-02-14T18:04:12Z

I'm on holiday but I'll take a look next week!

fendor

Changes look good to me, I mostly have minor suggestions.

While I do approve, let's wait until michael also had time to look at this :)

plugins/hls-semantic-tokens-plugin/src/Ide/Plugin/SemanticTokens/Internal.hs

plugins/hls-semantic-tokens-plugin/test/SemanticTokensTest.hs

ghcide/src/Development/IDE/Core/Shake.hs

plugins/hls-semantic-tokens-plugin/src/Ide/Plugin/SemanticTokens/Internal.hs

Co-authored-by: fendor <[email protected]>

…ns/Internal.hs Co-authored-by: fendor <[email protected]>

soulomoon · 2024-02-14T20:03:42Z

Changes look good to me, I mostly have minor suggestions.

Thanx for the reviews @fendor . Fixed most of them.

While I do approve, let's wait until michael also had time to look at this :)

Sure.

michaelpj

Mostly worried about adding lots of stuff to ShakeExtras if we don't have to.

plugins/hls-semantic-tokens-plugin/src/Ide/Plugin/SemanticTokens/Internal.hs

michaelpj · 2024-02-18T20:29:38Z

ghcide/src/Development/IDE/Core/Shake.hs

@@ -259,6 +261,11 @@ data ShakeExtras = ShakeExtras
    ,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


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.

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. 🤔

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 ?

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.

I do not have any specific thoughts, if the variable could be easily plugin local, I think I would prefer that.

plugins/hls-semantic-tokens-plugin/src/Ide/Plugin/SemanticTokens/Internal.hs

michaelpj · 2024-02-18T20:37:29Z

plugins/hls-semantic-tokens-plugin/src/Ide/Plugin/SemanticTokens/Internal.hs

@@ -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


why drop this?

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.

Hmm, so does this mean we have to wait quite a while to get responses when we start up?

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) .

plugins/hls-semantic-tokens-plugin/src/Ide/Plugin/SemanticTokens/Internal.hs

soulomoon · 2024-02-19T07:13:15Z

Thanx for the reviews, welcom back from vacation! @michaelpj , I fixed some of them, and some might need discussion.

michaelpj

@wz1000 still interested in your feedback on the ShakeExtras vs plugin-local state vars question, but for now we're going to leave it as it is with a note.

michaelpj · 2024-02-21T17:05:49Z

ghcide/src/Development/IDE/Core/Shake.hs

@@ -257,8 +259,18 @@ data ShakeExtras = ShakeExtras
    ,diagnostics :: STMDiagnosticStore
    ,hiddenDiagnostics :: STMDiagnosticStore
    ,publishedDiagnostics :: STM.Map NormalizedUri [Diagnostic]
+
+    -- storing semantic tokens for each file in shakeExtras might


this comment is in the wrong place, it's interrupting the haddock for the field above

Maybe put it in a Note or something

Sure. Did it

fendor

Re-approving, LGTM :)

* add Method_TextDocumentSemanticTokensFullDelta * remove persistentGetSemanticTokensRule * add doc about semanticTokensCache location * add Note [Semantic Tokens Cache Location] --------- Co-authored-by: fendor <[email protected]>

add Method_TextDocumentSemanticTokensFullDelta

43796d0

soulomoon linked an issue Feb 13, 2024 that may be closed by this pull request

textDocument/semanticTokens/full/delta #4072

Closed

soulomoon added 9 commits February 13, 2024 20:49

fix cache and add test

3f29007

cleanpu

ff93475

add tset

decd9e1

add delete tokens test

e1fd84e

clean up

41499b4

clean up

9b373f3

add delete tokens test

4f7546a

cleanup

8af3b99

fix test

181b10a

soulomoon marked this pull request as ready for review February 13, 2024 16:01

soulomoon requested review from michaelpj, fendor and pepeiborra as code owners February 13, 2024 16:01

soulomoon added 4 commits February 14, 2024 02:27

cleanup

9e7af72

remove persistentGetSemanticTokensRule

cef51ed

fix export

fa908a0

cleanup

e3ed49b

fendor reviewed Feb 14, 2024

View reviewed changes

soulomoon and others added 6 commits February 15, 2024 03:08

Update plugins/hls-semantic-tokens-plugin/test/SemanticTokensTest.hs

82b1eaf

Co-authored-by: fendor <[email protected]>

cleanup

e7a17e7

add doc

cc69775

cleanup

49fd150

Update plugins/hls-semantic-tokens-plugin/src/Ide/Plugin/SemanticToke…

dcdbb27

…ns/Internal.hs Co-authored-by: fendor <[email protected]>

fix

7e7398a

Merge branch 'master' into 4072-textdocumentsemantictokensfulldelta

e6ff123

michaelpj reviewed Feb 18, 2024

View reviewed changes

soulomoon added 2 commits February 19, 2024 13:21

Merge branch 'master' into 4072-textdocumentsemantictokensfulldelta

9ef7796

neat

b287639

soulomoon added 3 commits February 19, 2024 19:31

add doc

154013b

Merge branch 'master' into 4072-textdocumentsemantictokensfulldelta

42f6284

add doc about semanticTokensCache location

699dab7

michaelpj approved these changes Feb 21, 2024

View reviewed changes

soulomoon added 2 commits February 22, 2024 01:38

add Note [Semantic Tokens Cache Location]

a0b6ac7

fix doc

4180c43

fendor approved these changes Feb 21, 2024

View reviewed changes

fendor merged commit 24b40ca into master Feb 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add Method_TextDocumentSemanticTokensFullDelta #4073

add Method_TextDocumentSemanticTokensFullDelta #4073

soulomoon commented Feb 13, 2024 •

edited

Loading

michaelpj commented Feb 14, 2024

fendor left a comment •

edited

Loading

soulomoon commented Feb 14, 2024 •

edited

Loading

michaelpj left a comment

michaelpj Feb 18, 2024

soulomoon Feb 19, 2024 •

edited

Loading

michaelpj Feb 19, 2024

soulomoon Feb 21, 2024

fendor Feb 21, 2024

michaelpj Feb 18, 2024

soulomoon Feb 19, 2024 •

edited

Loading

michaelpj Feb 19, 2024

soulomoon Feb 19, 2024 •

edited

Loading

soulomoon commented Feb 19, 2024 •

edited

Loading

michaelpj left a comment

michaelpj Feb 21, 2024

michaelpj Feb 21, 2024

soulomoon Feb 21, 2024

fendor left a comment

add Method_TextDocumentSemanticTokensFullDelta #4073

add Method_TextDocumentSemanticTokensFullDelta #4073

Conversation

soulomoon commented Feb 13, 2024 • edited Loading

michaelpj commented Feb 14, 2024

fendor left a comment • edited Loading

Choose a reason for hiding this comment

soulomoon commented Feb 14, 2024 • edited Loading

michaelpj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soulomoon Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soulomoon Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soulomoon Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

soulomoon commented Feb 19, 2024 • edited Loading

michaelpj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fendor left a comment

Choose a reason for hiding this comment

soulomoon commented Feb 13, 2024 •

edited

Loading

fendor left a comment •

edited

Loading

soulomoon commented Feb 14, 2024 •

edited

Loading

soulomoon Feb 19, 2024 •

edited

Loading

soulomoon Feb 19, 2024 •

edited

Loading

soulomoon Feb 19, 2024 •

edited

Loading

soulomoon commented Feb 19, 2024 •

edited

Loading