Allow semantic tokens to be provided by more than one provider #135602
Conversation
rchiodo
requested review from
alexdima,
DonJayamanne,
IanMatthewHuff and
rebornix
There was a problem hiding this comment.
These suggested changes introduce subtle problems in ModelSemanticColoring. In ModelSemanticColoring, there is a SemanticTokensProviderStyling instantiated for each DocumentSemanticTokensProvider. The SemanticTokensProviderStyling instance is able to decode semantic tokens according to the provider's legend.
So although the newly introduced CompositeDocumentSemanticTokensProvider is looping through multiple providers, the getLegend() call will only be made once for the single CompositeDocumentSemanticTokensProvider instance here. So there will be a situation where the legend of the first provider would be attempted to be used to decode the tokens of a different provider.
The other subtle problem is that semantic tokens supports incremental updating of semantic tokens, and for this purpose lastResultId is used. For simple semantic providers, which do not support incremental semantic tokens, lastResultId will always be null, so it doesn't matter if the lastResultId from one provider is passed as the lastResultId of another provider, but in case of two advanced document semantic tokens providers, we need to make sure to never mix the lastResultId from one tokens provider and pass it by accident as the lastResultId to a different tokens provider.
|
@alexdima Thanks for the feedback. I've changed the ModelSemanticColoring to ask for the legend after it gets results so it should use the correct legend now. I've also added a weakmap that maps provider to their last result id to make sure it's allowed. |
| private disposables = new DisposableStore(); | ||
| private didChangeEmitter = new Emitter<void>(); | ||
| private lastUsedProvider: DocumentSemanticTokensProvider | undefined = undefined; | ||
| private static providerToLastResult = new WeakMap<DocumentSemanticTokensProvider, string>(); |
There was a problem hiding this comment.
This static introduces another subtle bug in the case of having two files opened at the same time, even when having a single provider.
If file1.ts and file2.ts are shown side-by-side, and if file1.ts makes a request and has the resultId 1, and then file2.ts makes a request and has the resultId 2, then after typing in file1.ts the request will not use resultId 1 (which would be correct) because the last request to this provider returned the resultId 2 for file2.ts.
IMHO there is a problem with the lifecycle of CompositeDocumentSemanticTokensProvider here. Before, getDocumentSemanticTokens could be implemented stateless, but IMHO that is no longer the case now, as some state needs to be captured between semantic tokens requests (like e.g. what was the last selected provider).
I will try to refactor how ModelSemanticColoring interacts with the DocumentSemanticTokensProviderRegistry, but IMHO this can no longer be done with a stateless getDocumentSemanticTokens call. I am working directly on your branch, please hold off from pushing for now.
… or more providers with the highest priority
|
Thank you! |
|
Thanks @alexdima |
This PR fixes #135580
Allow there to be more than one semantic token provider registered at the same time and pick the tokens from the provider that actually returns results.
This is necessary to get microsoft/vscode-jupyter#6799 working