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

Update semantic tokens #2168

Merged
merged 3 commits into from
Nov 4, 2024

Conversation

MartinGC94
Copy link
Contributor

PR Summary

Updates the semantic token mapping so attributes now get the "Decorator" token type and loop labels get the "Label" type. They seem to best match the mappings listed here: https://code.visualstudio.com/api/language-extensions/semantic-highlight-guide#standard-token-types-and-modifiers

I also removed the Generic -> Function mapping as I feel it does more harm than good to provide an inaccurate mapping. Unfortunately there's no fitting token for them, but feel free to give a thumbs up here: microsoft/vscode#97063 (comment) so it may get added in the future.

I didn't change this, but I see there's this mapping:

case TokenKind.Parameter:
case TokenKind.Generic when token is StringLiteralToken slt && slt.Text.StartsWith("--"):
    return SemanticTokenType.Parameter;

This seems wrong. The point of the syntax highlighting is to show what the parser sees, and if SomeApp --Parameter is not seen as a parameter by the parser then it shouldn't be highlighted as such. Can this be changed?

Also is the token kind check really necessary here?

if (token.Kind != TokenKind.Generic && (token.TokenFlags &
    (TokenFlags.BinaryOperator | TokenFlags.UnaryOperator | TokenFlags.AssignmentOperator)) != 0)

All the various operators have their own token kind and I can't think of a scenario where a generic token would have those those flags.

PR Context

I've been stubbornly holding on to ISE partially because of its superior syntax highlighting and while this doesn't fix it completely, it's a step in the right direction.

@SydneyhSmith
Copy link
Collaborator

Thanks @MartinGC94 we would love to get this in but there may be delays in review/release as we are backlogged with other work

@andyleejordan
Copy link
Member

@MartinGC94 do you regularly use the semantic highlighting feature? We turned it off by default after too many bug reports and too few resources to overhaul it. How has your experience been?

@MartinGC94
Copy link
Contributor Author

@andyleejordan No, I hardly use VS code at all. I've just decided to give it another shot if I can get the syntax highlighting fixed.
In my testing the feature seems to work fine on a technical level and when I look over the reported issues I only see 3 problems that stick out:

1: Generic tokens are highlighted as functions: PowerShell/vscode-powershell#3997 and PowerShell/vscode-powershell#2860
2: People that want special cases handled (highlighting constants like $true and $false differently from other variables or highlighting different parts of comments: PowerShell/vscode-powershell#3211
3: Themes with bad token mappings: PowerShell/vscode-powershell#2959 and PowerShell/vscode-powershell#3221

The first one will be fixed by this PR.
The second one seems more like a feature request that I don't personally care about because I'm only interested in seeing what the parser sees. It seems like it would require you to update the token handling so instead of mapping the PowerShell parser tokens 1:1 with the semantic tokens, you'd have to process certain tokens like variables and comments so you can create multiple semantic tokens like splitting: $global:test into a namespace and a variable token.

The third issue isn't really your problem to solve, though you can help a little. The problem demonstrated in: PowerShell/vscode-powershell#3221 where most elements turn blue is because the theme authors decided that variables and properties should both be blue but members should be yellow. Unfortunately there's no member semantic token so you have to pick between property and method. The way you could help would be to check if the next token is a ( and if so report it as a method semantic token instead of a property.

@andyleejordan andyleejordan changed the title Update sematic tokens Update semantic tokens Sep 4, 2024
Copy link
Member

@andyleejordan andyleejordan left a comment

Choose a reason for hiding this comment

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

Thanks! This was small and to a feature we have off by default, so happy to merge.

@andyleejordan
Copy link
Member

Ah, it looks like the unit tests need to be updated for these changes.

@MartinGC94
Copy link
Contributor Author

Tests don't run automatically here? Thanks cryptobros for ruining free compute.
I have updated the tests but I haven't had a chance to run them locally so let's hope I did everything right.

@MartinGC94
Copy link
Contributor Author

Finally got around to doing it and found an error that I've now fixed. All tests now pass on my own machine at least.

@JustinGrote
Copy link
Collaborator

Rebased to re-run tests, they should pass now that the supply chain stuff has been fixed up.

@andyleejordan
Copy link
Member

Awesome, thanks both. Merging!

@andyleejordan andyleejordan added this pull request to the merge queue Nov 4, 2024
Merged via the queue into PowerShell:main with commit 7e2d864 Nov 4, 2024
6 checks passed
@JustinGrote
Copy link
Collaborator

@andyleejordan may want to enforce squash merges in the queue, this wasn't squashed so we have some slightly ugly commits in the tree now (not that it's that big a deal)
image

@andyleejordan
Copy link
Member

I hate squash merging 😅 I'd rather have silly commits that are real than "oh we squashed and all your (pretty or not!) commits are gone."

@JustinGrote
Copy link
Collaborator

Fair enough, I'm still going to squash my PRs once they are ready (like that 200 commit rename one) so we can leave that to the PR author if you don't care from a main perspective.

@andyleejordan
Copy link
Member

Exactly, and totally fine. If you want to squash commits do it! I don't like the blanket "squash everyone's commits" (especially since I personally craft usually a few indepenent hopefully well thought out commits...habits from patches on a mailing list days).

@andyleejordan andyleejordan added the Issue-Enhancement A feature request (enhancement). label Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Enhancement A feature request (enhancement).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants