search

golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
120.96k stars 17.36k forks source link

x/tools/gopls: semantic tokens: highlight control flow tokens #67663

Open jmg-duarte opened 1 month ago

jmg-duarte commented 1 month ago

gopls version

Build info

golang.org/x/tools/gopls v0.15.3 golang.org/x/tools/gopls@v0.15.3 h1:zbdOidFrPTc8Bx0YrN5QKgJ0zCjyGi0L27sKQ/bDG5o= github.com/BurntSushi/toml@v1.2.1 h1:9F2/+DoOYIOksmaJFPw1tGFy1eDnIJXg+UHjuD8lTak= github.com/google/go-cmp@v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= golang.org/x/exp/typeparams@v0.0.0-20221212164502-fae10dda9338 h1:2O2DON6y3XMJiQRAS1UWU+54aec2uopH3x7MAiqGW6Y= golang.org/x/mod@v0.15.0 h1:SernR4v+D55NyBH2QiEQrlBAnj1ECL6AGrA5+dPaMY8= golang.org/x/sync@v0.6.0 h1:5BMeUDZ7vkXGfEr1x9B4bRcTH4lpkTkpdh0T/J+qjbQ= golang.org/x/telemetry@v0.0.0-20240209200032-7b892fcb8a78 h1:vcVnuftN4J4UKLRcgetjzfU9FjjgXUUYUc3JhFplgV4= golang.org/x/text@v0.14.0 h1:ScX5w1eTa3QqT8oi6+ziP7dTV1S2+ALU0bI+0zXKWiQ= golang.org/x/tools@v0.18.1-0.20240412183611-d92ae0781217 h1:uH9jJYgeLCvblH0S+03kFO0qUDxRkbLRLFiKVVDl7ak= golang.org/x/vuln@v1.0.1 h1:KUas02EjQK5LTuIx1OylBQdKKZ9jeugs+HiqO5HormU= honnef.co/go/tools@v0.4.6 h1:oFEHCKeID7to/3autwsWfnuv69j3NsfcXbvJKuIcep8= mvdan.cc/gofumpt@v0.6.0 h1:G3QvahNDmpD+Aek/bNOLrFR2XC6ZAdo62dZu65gmwGo= mvdan.cc/xurls/v2@v2.5.0 h1:lyBNOm8Wo71UknhUs4QTFUNNMyxy2JEIaKKo0RWOh+8= go: go1.22.3

go env

GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/jmgd/Library/Caches/go-build'
GOENV='/Users/jmgd/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/jmgd/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/jmgd/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.22.3'
GCCGO='gccgo'
AR='ar'
CC='clang'
CXX='clang++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/4d/v4vg0mc15_gf9skpnjgmmhdm0000gn/T/go-build2911028327=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

Write code with some sort of control flow:

func controlFlow(i int64) {
    if i < 0 {
        return true
    } else {
        return false
    }
}

What did you see happen?

return, if and else (i.e. control flow keywords) are marked as keyword only, while the TextMate grammar already has support for control flow

Screenshot 2024-05-27 at 08 49 34

What did you expect to see?

The controlFlow modifier along with the respective keywords, like in this Rust example

Screenshot 2024-05-27 at 08 50 38

Editor and settings

  "go.toolsManagement.autoUpdate": true,
  "gopls": {
    "ui.semanticTokens": true
  },

Logs

No response

hyangah commented 1 month ago

I don't see controlFlow from https://code.visualstudio.com/api/language-extensions/semantic-highlight-guide#semantic-token-scope-map or LSP spec.

It looks like this is rust-analyzer's VS Code extension specific modifier (https://github.com/rust-lang/rust-analyzer/blob/b32f181f477576bb203879f7539608f3327b6178/editors/code/package.json#L2092).

jmg-duarte commented 1 month ago

Hmm might be... That's unfortunate, but I think it would still be useful to add it (and I'm open to do so if there's interest). For a bit more context: my themes highlight control flow as they're a really important part of the code, the idea is based on https://lunacookies.github.io/sema/

Anyway, I also found the issue on my theme, it was that pesky "keyword": "".

If you think the controlFlow addition is not useful, we can close this.

adonovan commented 1 month ago

The LSP semantic tokens design is extensible, so servers are not bound to emit all and only the tokens enumerated in the standard; this seems like a well-defined and reasonable class of tokens to highlight; and it's easy enough to add. Would you care to send us a CL? The function you need to change is golang.org/x/tools/gopls/internal/golang.(*tokenVisitor).inspect.

jmg-duarte commented 1 month ago

The following diff doesn't work and I don't understand why, isn't it the modifier that I want to change?

I'm testing using const semDebug = true and gopls semtok foo.go > /dev/null but pointing to the gopls/internal/protocol/semtok/semtok.go and looking for ReturnStmts as they're the most prolific ones.

@adonovan if you could take a look, that would be amazing! Thanks in advance!

diff --git a/gopls/internal/golang/semtok.go b/gopls/internal/golang/semtok.go
index ec628f501..a23cf92a7 100644
--- a/gopls/internal/golang/semtok.go
+++ b/gopls/internal/golang/semtok.go
@@ -356,9 +356,9 @@ func (tv *tokenVisitor) inspect(n ast.Node) (descend bool) {
        tv.token(n.OpPos, len(n.Op.String()), semtok.TokOperator, nil)
    case *ast.BlockStmt:
    case *ast.BranchStmt:
-       tv.token(n.TokPos, len(n.Tok.String()), semtok.TokKeyword, nil)
+       tv.token(n.TokPos, len(n.Tok.String()), semtok.TokKeyword, []string{"controlFlow"})
        if n.Label != nil {
-           tv.token(n.Label.Pos(), len(n.Label.Name), semtok.TokLabel, nil)
+           tv.token(n.Label.Pos(), len(n.Label.Name), semtok.TokLabel, []string{"controlFlow"})
        }
    case *ast.CallExpr:
        if n.Ellipsis.IsValid() {
@@ -369,7 +369,7 @@ func (tv *tokenVisitor) inspect(n ast.Node) (descend bool) {
        if n.List == nil {
            iam = "default"
        }
-       tv.token(n.Case, len(iam), semtok.TokKeyword, nil)
+       tv.token(n.Case, len(iam), semtok.TokKeyword, []string{"controlFlow"})
    case *ast.ChanType:
        // chan | chan <- | <- chan
        switch {
@@ -392,7 +392,7 @@ func (tv *tokenVisitor) inspect(n ast.Node) (descend bool) {
    case *ast.CompositeLit:
    case *ast.DeclStmt:
    case *ast.DeferStmt:
-       tv.token(n.Defer, len("defer"), semtok.TokKeyword, nil)
+       tv.token(n.Defer, len("defer"), semtok.TokKeyword, []string{"controlFlow"})
    case *ast.Ellipsis:
        tv.token(n.Ellipsis, len("..."), semtok.TokOperator, nil)
    case *ast.EmptyStmt:
@@ -400,7 +400,7 @@ func (tv *tokenVisitor) inspect(n ast.Node) (descend bool) {
    case *ast.Field:
    case *ast.FieldList:
    case *ast.ForStmt:
-       tv.token(n.For, len("for"), semtok.TokKeyword, nil)
+       tv.token(n.For, len("for"), semtok.TokKeyword, []string{"controlFlow"})
    case *ast.FuncDecl:
    case *ast.FuncLit:
    case *ast.FuncType:
@@ -410,15 +410,15 @@ func (tv *tokenVisitor) inspect(n ast.Node) (descend bool) {
    case *ast.GenDecl:
        tv.token(n.TokPos, len(n.Tok.String()), semtok.TokKeyword, nil)
    case *ast.GoStmt:
-       tv.token(n.Go, len("go"), semtok.TokKeyword, nil)
+       tv.token(n.Go, len("go"), semtok.TokKeyword, []string{"controlFlow"})
    case *ast.Ident:
        tv.ident(n)
    case *ast.IfStmt:
-       tv.token(n.If, len("if"), semtok.TokKeyword, nil)
+       tv.token(n.If, len("if"), semtok.TokKeyword, []string{"controlFlow"})
        if n.Else != nil {
            // x.Body.End() or x.Body.End()+1, not that it matters
            pos := tv.findKeyword("else", n.Body.End(), n.Else.Pos())
-           tv.token(pos, len("else"), semtok.TokKeyword, nil)
+           tv.token(pos, len("else"), semtok.TokKeyword, []string{"controlFlow"})
        }
    case *ast.ImportSpec:
        tv.importSpec(n)
@@ -445,9 +445,9 @@ func (tv *tokenVisitor) inspect(n ast.Node) (descend bool) {
        pos := tv.findKeyword("range", offset, n.X.Pos())
        tv.token(pos, len("range"), semtok.TokKeyword, nil)
    case *ast.ReturnStmt:
-       tv.token(n.Return, len("return"), semtok.TokKeyword, nil)
+       tv.token(n.Return, len("return"), semtok.TokKeyword, []string{"controlFlow"})
    case *ast.SelectStmt:
-       tv.token(n.Select, len("select"), semtok.TokKeyword, nil)
+       tv.token(n.Select, len("select"), semtok.TokKeyword, []string{"controlFlow"})
    case *ast.SelectorExpr:
    case *ast.SendStmt:
        tv.token(n.Arrow, len("<-"), semtok.TokOperator, nil)
@@ -457,7 +457,7 @@ func (tv *tokenVisitor) inspect(n ast.Node) (descend bool) {
    case *ast.StructType:
        tv.token(n.Struct, len("struct"), semtok.TokKeyword, nil)
    case *ast.SwitchStmt:
-       tv.token(n.Switch, len("switch"), semtok.TokKeyword, nil)
+       tv.token(n.Switch, len("switch"), semtok.TokKeyword, []string{"controlFlow"})
    case *ast.TypeAssertExpr:
        if n.Type == nil {
            pos := tv.findKeyword("type", n.Lparen, n.Rparen)
@@ -465,7 +465,7 @@ func (tv *tokenVisitor) inspect(n ast.Node) (descend bool) {
        }
    case *ast.TypeSpec:
    case *ast.TypeSwitchStmt:
-       tv.token(n.Switch, len("switch"), semtok.TokKeyword, nil)
+       tv.token(n.Switch, len("switch"), semtok.TokKeyword, []string{"controlFlow"})
    case *ast.UnaryExpr:
        tv.token(n.OpPos, len(n.Op.String()), semtok.TokOperator, nil)
    case *ast.ValueSpec:
adonovan commented 1 month ago

The Encode function computes the intersection of the tokens and modifiers that were enumerated with the tokens and modifiers that were requested by the client. You'll need to add the new modified to this list in the protocol package

    semanticModifiers = [...]string{
        "declaration", "definition", "readonly", "static",
        "deprecated", "abstract", "async", "modification", "documentation", "defaultLibrary",
    }

This list determines the set of modifiers used in the fake clients in the tests. You will need to make your real client send the new modifier too (though apparently it already does, at least when configured to talk to the Rust LSP server).