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

x/tools/gopls: FoldingRange omitempty causes EndCharacter=0 to be omitted, changing its semantics in clients that distinguish 0 from missing #71489

Open
jansorg opened this issue Jan 30, 2025 · 8 comments
Assignees
Labels
BugReport Issues describing a possible bug in the Go implementation. Documentation Issues describing a change to documentation. gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@jansorg
Copy link

jansorg commented Jan 30, 2025

gopls version

Build info
----------
golang.org/x/tools/gopls (devel)
    golang.org/x/tools/gopls@(devel)
    github.com/BurntSushi/toml@v1.4.1-0.20240526193622-a339e1f7089c h1:pxW6RcqyfI9/kWtOwnv/G+AzdKuy2ZrqINhenH4HyNs=
    github.com/google/go-cmp@v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
    golang.org/x/exp/typeparams@v0.0.0-20231108232855-2478ac86f678 h1:1P7xPZEwZMoBoz0Yze5Nx2/4pxj6nw9ZqHWXqP0iRgQ=
    golang.org/x/mod@v0.22.0 h1:D4nJWe9zXqHOmWqj4VMOJhvzj7bEZg4wEYa759z1pH4=
    golang.org/x/sync@v0.9.0 h1:fEo0HyrW1GIgZdpbhCRO0PkJajUS5H9IFUztCgEo2jQ=
    golang.org/x/telemetry@v0.0.0-20241106142447-58a1122356f5 h1:TCDqnvbBsFapViksHcHySl/sW4+rTGNIAoJJesHRuMM=
    golang.org/x/text@v0.20.0 h1:gK/Kv2otX8gz+wn7Rmb3vT96ZwuoxnQlY+HlJVj7Qug=
    golang.org/x/tools@v0.27.1-0.20241219162658-575221bfbda3 h1:kgwdasJRsdDWYgWcEgMF424DiXwwXHSb3V8xVTi//i8=
    golang.org/x/vuln@v1.0.4 h1:SP0mPeg2PmGCu03V+61EcQiOjmpri2XijexKdzv8Z1I=
    honnef.co/go/tools@v0.5.1 h1:4bH5o3b5ZULQ4UrBmP+63W9r7qIkqJClEA9ko5YKx+I=
    mvdan.cc/gofumpt@v0.7.0 h1:bg91ttqXmi9y2xawvkuMXyvAA/1ZGJqYAEGjXuP0JXU=
    mvdan.cc/xurls/v2@v2.5.0 h1:lyBNOm8Wo71UknhUs4QTFUNNMyxy2JEIaKKo0RWOh+8=
go: go1.23.4

go env

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/jansorg/.cache/go-build'
GOENV='/home/jansorg/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/jansorg/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/jansorg/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/lib/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/lib/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.23.5'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/home/jansorg/.config/go/telemetry'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
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 -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build2443791054=/tmp/go-build -gno-record-gcc-switches'

What did you do?

I requested the folding ranges for this .go file:

package main

import (
	"flag"
	"fmt"
)

func foo() {
}

What did you see happen?

The folding range returned by gopls for the import lines include the closing ) parenthesis.

For this sample code the response returned by gopls is shown in the image (IntelliJ debugger visualization):

foldingRange response:
Image

What did you expect to see?

The folding should end before the closing parenthesis.

I'm writing an LSP client for JetBrains IDEs.
I believe that the response returned by textDocument/foldingRange is off by +1, i.e. it should be one less.

In the example, the import folding range has endLine = 5, which is the line of the closing parenthesis ).

But according to the gopls source code it's supposed to be the end of the line before the closing parenthesis:
https://github.com/golang/tools/blob/e7bd2274d184f7579a8c7a1a12d8ad0351aeee8a/gopls/internal/golang/folding_range.go#L190

As far as I understand, the line returned by LineStart is 1-based.
But LSP's lines are 0-based.

That means that a 1-based line number is returned as the value of a 0-based line number.

AFAIK VSCode is applying some workarounds, so it's showing something else. And it's only supporting line-based folding, but my client does support all kinds of ranges.

// LineStart returns the [Pos] value of the start of the specified line.
// It ignores any alternative positions set using [File.AddLineColumnInfo].
// LineStart panics if the 1-based line number is invalid.
func (f *File) LineStart(line int) Pos {

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocuments

A position inside a document (see Position definition below) is expressed as a zero-based line and character offset.

Editor and settings

No response

Logs

No response

@jansorg jansorg added gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository. labels Jan 30, 2025
@jansorg jansorg changed the title x/tools/gopls: foldRange endLine is off by 1 x/tools/gopls: endLine of textDocument/foldingRange is off by 1 Jan 30, 2025
@gopherbot gopherbot added this to the Unreleased milestone Jan 30, 2025
@gopherbot gopherbot added the Documentation Issues describing a change to documentation. label Jan 30, 2025
@gabyhelp gabyhelp added the BugReport Issues describing a possible bug in the Go implementation. label Jan 30, 2025
@adonovan
Copy link
Member

This session shows the output of the gopls CLI tool on the same input. The CLI tool and nl both use 1-based numbering.

$ nl -ba  a.go
     1	package main
     2	
     3	import (
     4		"flag"
     5		"fmt"
     6	)
     7	
     8	func foo() {
     9	}

$ gopls folding_ranges ./a.go 
3:9-6:1
8:13-9:1

The first range is between the (...) parens of the import declaration; the second range is between the {...} braces of the function. This all looks correct to me, so I plan to close this issue.

I don't know what tool you used to generate the screenshot in the issue. If you think gopls is at fault, please reproduce the problem using VS Code, or the gopls CLI tool, or show the incorrect JSON response from the server, and feel free to reopen this issue.

@adonovan adonovan closed this as not planned Won't fix, can't repro, duplicate, stale Jan 31, 2025
@jansorg
Copy link
Author

jansorg commented Jan 31, 2025

@adonovan
Here's the JSON response sent by gopls:

{
  "jsonrpc" : "2.0",
  "id" : "42",
  "result" : [ {
    "startLine" : 2,
    "endLine" : 5,
    "startCharacter" : 8,
    "kind" : "imports"
  }, {
    "startLine" : 7,
    "endLine" : 8,
    "startCharacter" : 12
  } ]
}

The range for the import is this:

{
    "startLine" : 2,
    "endLine" : 5,
    "startCharacter" : 8,
    "kind" : "imports"
  }

Property endCharacter is missing, which means it defaults to the length of the line, i.e. the folding range is ending after the closing ) character.

This is different to what gopls reports on the command line.
IMHO that's a bug, but I may have been wrong with my initial assumption that the line is off by one.

The LSP spec on endCharacter:

 /**
    * The zero-based character offset before the folded range ends. If not
    * defined, defaults to the length of the end line.
    */
endCharacter?: uinteger

@adonovan
Copy link
Member

adonovan commented Jan 31, 2025

Ah, I see what is happening: the encoding/json schema type is this:

type FoldingRange struct {
	// The zero-based start line of the range to fold. The folded area starts after the line's last character.
	// To be valid, the end must be zero or larger and smaller than the number of lines in the document.
	StartLine uint32 `json:"startLine"`
	// The zero-based character offset from where the folded range starts. If not defined, defaults to the length of the start line.
	StartCharacter uint32 `json:"startCharacter,omitempty"`
	// The zero-based end line of the range to fold. The folded area ends with the line's last character.
	// To be valid, the end must be zero or larger and smaller than the number of lines in the document.
	EndLine uint32 `json:"endLine"`
	// The zero-based character offset before the folded range ends. If not defined, defaults to the length of the end line.
	EndCharacter uint32 `json:"endCharacter,omitempty"`
...
}

and the actual value of EndCharacter is 0, but since the field is marked omitempty, it is not encoded; the client, following the comment, interprets the lack of a field a value as "the length of the end of the line". The problem is that 0 and missing have different semantics in the JSON protocol, but the Go representation cannot distinguish them. We should remove omitempty.

Thanks for reporting this.

@adonovan adonovan reopened this Jan 31, 2025
@adonovan adonovan changed the title x/tools/gopls: endLine of textDocument/foldingRange is off by 1 x/tools/gopls: FoldingRange omitempty causes EndCharacter=0 to be omitted, changing its semantics in clients that distinguish 0 from missing Jan 31, 2025
@jansorg
Copy link
Author

jansorg commented Jan 31, 2025

Ah, that makes sense.
Field StartCharacter has the same semantics and would need the same fix, I suppose.

@adonovan
Copy link
Member

adonovan commented Jan 31, 2025

Aside: I notice that the MappedRange abstraction is now almost entirely unnecessary. Will send a CL.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/645815 mentions this issue: gopls/internal/protocol: delete MappedRange

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/645855 mentions this issue: gopls/internal/golang: folding range: remove FoldingRangeInfo

gopherbot pushed a commit to golang/tools that referenced this issue Jan 31, 2025
It has slowly made itself entirely redundant.

Updates golang/go#71489

Change-Id: Ib15128b9dc9652c1138a1372ba89c507667e7fcd
Reviewed-on: https://go-review.googlesource.com/c/tools/+/645815
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BugReport Issues describing a possible bug in the Go implementation. Documentation Issues describing a change to documentation. gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

5 participants