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

go/ast: End() for *BlockStmt incorrect when there is no closing brace #33649

Open
myitcv opened this issue Aug 14, 2019 · 3 comments

Comments

@myitcv
Copy link
Member

commented Aug 14, 2019

What version of Go are you using (go version)?

$ go version
go version devel +61bb56ad63 Mon Aug 12 23:12:29 2019 +0000 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/myitcv/.cache/go-build"
GOENV="/home/myitcv/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/myitcv/gostuff"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/myitcv/gos"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/myitcv/gos/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/myitcv/gostuff/src/github.com/myitcv/playground/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build692430293=/tmp/go-build -gno-record-gcc-switches"

What did you do?

package main

import (
	"fmt"
	"go/ast"
	"go/parser"
	"go/token"
)

func main() {
	src := `package main

func main() {
`

	fset := token.NewFileSet()
	f, err := parser.ParseFile(fset, "", src, parser.AllErrors)
	if f == nil {
		panic(err)
	}

	var tf *token.File
	fset.Iterate(func(f *token.File) bool {
		tf = f
		return true
	})

	fd := f.Decls[len(f.Decls)-1].(*ast.FuncDecl)

	fmt.Printf("tf.Base() + tf.Size(): %v\n", tf.Base()+tf.Size())
	fmt.Printf("fd.End(): %v\n", fd.End())
}

https://play.golang.org/p/A56nT3ypuyJ

What did you expect to see?

tf.Base() + tf.Size(): 29
fd.End(): 29

What did you see instead?

tf.Base() + tf.Size(): 29
fd.End(): 30

cc @griesemer

FYI @rogpeppe @mvdan

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

Thanks for the nice test case. I looked at this a bit and it's not obviously clear (to me) what the correct solution is here. I suppose we don't want an End() position that is past the source's length. The problem is that the parser doesn't know what's going to happen with the position it collects: in this case it simply assumes the position of the (missing) } is at EOF, which is arguably a "correct" assumption. It's the go/ast which adds 1 (for the length of }) to that position. But the go/ast doesn't know that this is past EOF.

One way of handling this in the parser is to assume that the position of a missing token is the current position minus the length of the missing token - then, any correction made by the go/ast would be "correct". But that could also place a missing token before a preceding token, which may have other unexpected side-effects.

@myitcv What is the specific use case where this poses a problem? Perhaps this can help identifying a good solution.

@griesemer griesemer self-assigned this Aug 14, 2019

@griesemer griesemer added the Thinking label Aug 14, 2019

@griesemer griesemer added this to the Unplanned milestone Aug 14, 2019

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

Actually, the correct fix would probably be to not record a position for the closing } if it is missing. Then the go/ast could detect that and determine the End() of the BlockStmt as the end of the last statement inside.

The same problem affects a few other nodes as well.

@griesemer griesemer modified the milestones: Unplanned, Go1.14 Aug 14, 2019

@myitcv

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

What is the specific use case where this poses a problem? Perhaps this can help identifying a good solution.

The use case is providing AST-oriented motions within an editor, in this case Vim using govim. For example, the key combination ]] is mapped to "move to the next End() (as in go/ast.Node.End()) of the *ast.File's top level declarations." These motions obviously need to work in the case of incomplete files, i.e. with syntax errors.

Actually, the correct fix would probably be to not record a position for the closing } if it is missing. Then the go/ast could detect that and determine the End() of the BlockStmt as the end of the last statement inside.

Given the use case I've described, I agree this sounds correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.