Skip to content
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

Semantic tokens allow for a single provider #135580

Closed
rchiodo opened this issue Oct 21, 2021 · 5 comments · Fixed by #135602
Closed

Semantic tokens allow for a single provider #135580

rchiodo opened this issue Oct 21, 2021 · 5 comments · Fixed by #135602
Assignees
Labels
feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders semantic-tokens Semantic tokens issues verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@rchiodo
Copy link
Contributor

rchiodo commented Oct 21, 2021

This request is to allow semantic tokens to not just pick the first most provider in the list of providers. But instead pick the provider that actually has tokens to return.

The use case for this is to support jupyter notebook token colorization.

Right now there are at least 2 language servers returning tokens for python notebooks:

  1. Python extension has a language server registered for all 'python' files. There's no way to 'exclude' files from a DocumentFilter so it has a middleware piece that excludes all notebook cells.

  2. Jupyter extension has one ore more language servers registered for all notebook cells. One or more is because the language server in use can only support one python interpreter at a time, so one language server is created per python interpreter in use for any notebook (multiple interpreters can be in use in one VS code session). When semantic tokens are requested the middleware piece in the juptyer extension will skip sending token requests to pylance if they're not for the appropriate document.

This creates a problem with the current implementation because the first registered language server wins. This can randomly be python's (where no tokens are returned) or one of the jupyter one's (where it might return tokens if the interpreter for the document matches).

I believe this could all be accomplished by making a compositing token provider in the VS code source. Something like so:

class CompositeDocumentSemanticTokensProvider implements DocumentSemanticTokensProvider {
	private didChangeEmitter = new Emitter<void>();
	constructor(model: ITextModel, private readonly providerGroup: DocumentSemanticTokensProvider[]) {
		// Lifetime of this provider is tied to the text model
		model.onWillDispose(() => this.didChangeEmitter.dispose());

		// Mirror did change events
		providerGroup.forEach(p => {
			if (p.onDidChange) {
				p.onDidChange(() => this.didChangeEmitter.fire(), this, undefined);
			}
		});
	}
	public async provideDocumentSemanticTokens(model: ITextModel, lastResultId: string | null, token: CancellationToken): Promise<SemanticTokens | SemanticTokensEdits | null | undefined> {
		// Get tokens from the group all at the same time. Return the first
		// that actually returned tokens
		const list = await Promise.all(this.providerGroup.map(async provider => {
			try {
				return await provider.provideDocumentSemanticTokens(model, lastResultId, token);
			} catch (err) {
				onUnexpectedExternalError(err);
			}
			return undefined;
		}));

		return list.find(l => l);
	}
	public get onDidChange(): Event<void> {
		return this.didChangeEmitter.event;
	}
	getLegend(): SemanticTokensLegend {
		return this.providerGroup[0].getLegend();
	}
	releaseDocumentSemanticTokens(resultId: string | undefined): void {
		this.providerGroup.forEach(p => p.releaseDocumentSemanticTokens(resultId));
	}
}
@rchiodo
Copy link
Contributor Author

rchiodo commented Oct 21, 2021

I'm going to try out the composite idea and submit a PR if it works.

@alexdima alexdima added feature-request Request for new features or functionality semantic-tokens Semantic tokens issues labels Oct 22, 2021
@alexdima alexdima added this to the Backlog milestone Oct 22, 2021
@rzhao271 rzhao271 added the verification-needed Verification of issue is requested label Oct 26, 2021
@rzhao271 rzhao271 modified the milestones: Backlog, October 2021 Oct 26, 2021
@rchiodo rchiodo added the verified Verification succeeded label Oct 26, 2021
@rchiodo
Copy link
Contributor Author

rchiodo commented Oct 26, 2021

Verified it works with a notebook

image

@REYNEP
Copy link

REYNEP commented Oct 28, 2021

@rchiodo, hey, uhh, I opened an feature-request about semantic tokens the other day, just right after 4 hours from you.... I was trying to make a SemanticTokensProvider that overrides or works on top of already registered another one....

#135599

Wouldn't it be better if there could be multiple SemanticTokensProvider to work alongside, maybe override one another by User-Specified priority levels?

I was struggling to override some of SemanticTokens that the C/C++ extension provides, but then I found out its not really possible to have multiple running Provider for SemanticTokens....

Sorry to bother you.... But as there's a Bot notice, I don't think this Feature request can make it up to 20 Upvotes.... I am not part of any big communities,

@rchiodo
Copy link
Contributor Author

rchiodo commented Oct 28, 2021

Wouldn't it be better if there could be multiple SemanticTokensProvider to work alongside, maybe override one another by User-Specified priority levels?

This is a different use case than what I had envisioned. My use case was there were multiple providers but only one was actually returning anything.

Combining them sounds like an even better way to handle it. It would handle it for my case and your case. I upvoted your request.

@REYNEP
Copy link

REYNEP commented Oct 28, 2021

@rchiodo Thank you.... 19 More people and I really dont have any idea how to get there tho, haha.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders semantic-tokens Semantic tokens issues verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants
@alexdima @rzhao271 @rchiodo @REYNEP and others