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

Jump to bottom

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

Closed
myitcv opened this issue Aug 14, 2019 · 5 comments
Closed

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

myitcv opened this issue Aug 14, 2019 · 5 comments
Labels
FrozenDueToAge Thinking
Milestone
Go1.14

Comments

@myitcv
Copy link
Member

myitcv 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
Copy link
Contributor

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 this to the Unplanned milestone Aug 14, 2019
@griesemer
Copy link
Contributor

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
Copy link
Member Author

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

@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@gopherbot
Copy link

Change https://golang.org/cl/202581 mentions this issue: go/parser, go/ast: correctly take into account presence of } in block

@griesemer griesemer modified the milestones: Backlog, Go1.14 Oct 22, 2019
@gopherbot
Copy link

Change https://golang.org/cl/204041 mentions this issue: go/parser: use valid position when reporting an error (copy-paste bug)

gopherbot pushed a commit that referenced this issue Oct 29, 2019
@griesemer
go/parser: use valid position when reporting an error (copy-paste bug)
07b72d9 
This is a follow-up on https://golang.org/cl/202581.

Updates #33649.

Change-Id: Ib078fed983792c5493bdbed6d33e21b86856894a
Reviewed-on: https://go-review.googlesource.com/c/go/+/204041
Run-TryBot: Robert Griesemer <gri@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@golang golang locked and limited conversation to collaborators Oct 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Thinking
Projects
None yet
Milestone
Go1.14
Development

No branches or pull requests
4 participants
@rsc @myitcv @griesemer @gopherbot