Skip to content

Conversation

@MichaReiser
Copy link
Member

Summary

This PR fixes an issue where the semantic token provider returned tokens that
started at the end offset of the selected range. That is, the
token and requested range intersect, but the intersecting range is empty.

This isn't an issue for most documents because the only outcome is tha the server
sends one additional token to the client. However, this is an issue for notebooks
where the response can only contain tokens that all belong to the same cell. Sending one extra token can then have the effect of sending the first token of the next cell,
which isn't allowed.

Test Plan

Added test. My in draft notebook PR no longer panics during semantic syntax highlighting

@MichaReiser MichaReiser added server Related to the LSP server ty Multi-file analysis & type inference labels Nov 1, 2025
@MichaReiser MichaReiser added the server Related to the LSP server label Nov 1, 2025
@MichaReiser MichaReiser requested a review from dcreager as a code owner November 1, 2025 23:00
@MichaReiser MichaReiser added the ty Multi-file analysis & type inference label Nov 1, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Nov 1, 2025

Diagnostic diff on typing conformance tests

No changes detected when running ty on typing conformance tests ✅

@github-actions
Copy link
Contributor

github-actions bot commented Nov 1, 2025

mypy_primer results

No ecosystem changes detected ✅
No memory usage changes detected ✅

@MichaReiser MichaReiser force-pushed the micha/semantic-tokens-fix branch from b6ec0b2 to b3dc5ea Compare November 1, 2025 23:04
Fix semantic tokens to exclude ranges that only touch (are empty) when
filtering by range. Previously, ranges that touched the filter range
but were empty were incorrectly included.

This ensures that empty ranges at range boundaries are properly filtered out.
@MichaReiser MichaReiser force-pushed the micha/semantic-tokens-fix branch from b3dc5ea to cc54de8 Compare November 1, 2025 23:08
@AlexWaygood AlexWaygood removed their request for review November 1, 2025 23:26
Copy link
Contributor

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

"
x = 1
y = 2
z = 3<CURSOR>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside: I found the <CURSOR> here distracting, as I initially thought it would somehow influence or determine the target range. Can it be removed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that CURSOR is ony confusing here and that semantic tokens should probably not use the cursor infrastructure to begin with. But I'd rather clean this up in a separate PR

Co-authored-by: David Peter <sharkdp@users.noreply.github.com>
@MichaReiser MichaReiser enabled auto-merge (squash) November 2, 2025 14:54
@MichaReiser MichaReiser merged commit 6c3d612 into main Nov 2, 2025
40 checks passed
@MichaReiser MichaReiser deleted the micha/semantic-tokens-fix branch November 2, 2025 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

server Related to the LSP server ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants