PowerShell / PowerShellEditorServices

A common platform for PowerShell development support in any editor or application!
MIT License
637 stars 219 forks source link

Update semantic tokens #2168

Closed MartinGC94 closed 3 weeks ago

MartinGC94 commented 4 months ago

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: https://github.com/microsoft/vscode/issues/97063#issuecomment-2240581375 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 commented 4 months ago

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 commented 3 months ago

@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 commented 3 months ago

@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: https://github.com/PowerShell/vscode-powershell/issues/3997 and https://github.com/PowerShell/vscode-powershell/issues/2860
2: People that want special cases handled (highlighting constants like $true and $false differently from other variables or highlighting different parts of comments: https://github.com/PowerShell/vscode-powershell/issues/3211 3: Themes with bad token mappings: https://github.com/PowerShell/vscode-powershell/issues/2959 and https://github.com/PowerShell/vscode-powershell/issues/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: https://github.com/PowerShell/vscode-powershell/issues/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 commented 2 months ago

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

MartinGC94 commented 2 months ago

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 commented 2 months ago

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 commented 3 weeks ago

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

andyleejordan commented 3 weeks ago

Awesome, thanks both. Merging!

JustinGrote commented 2 weeks ago

@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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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