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/go/ast/astutil: ast replace could not handled expression parsed from string #29615

Closed
anjmao opened this issue Jan 8, 2019 · 3 comments
Closed
Milestone

Comments

@anjmao
Copy link

@anjmao anjmao commented Jan 8, 2019

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

go version go1.11.2 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/anjmao/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/anjmao/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/l6/jh91jsw90fvbxtpm54k_5fgc0000gn/T/go-build462335630=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I'm using go tools astutil package to replace ast node with some new node which I'm getting from parser.ParseExpr.
Here is demo showing the issue https://github.com/anjmao/astpartup/blob/master/main.go#L79
If I create new ast node tree manually then it works fine and I get correctly formatted result back (also shown in demo).

What did you expect to see?

I expect to see correctly formatted file

package main

import "fmt"

func main() {
        log.Println("new value")

        var a int
}

What did you see instead?

package main

import "fmt"

func main() {
        log.
                Println("new value",
                )

        var a int
}
@gopherbot gopherbot added this to the Unreleased milestone Jan 8, 2019
@josharian
Copy link
Contributor

@josharian josharian commented Jan 9, 2019

Looks like the position information is wrong after replacement. I suspect this is because your manually created node has no position info, so go/printer treats it sensibly, whereas the ParseExpr node has position information, which go/printer tries to respect, to no avail.

I’m not sure what the right fix for this is without digging a bit, and realistically that isn’t going to happen soon. My apologies. Is there some form of workaround (aside from creating the AST manually) that I could advise on? Note that one convenient hack is that you can put just about anything in an ast.Ident. I often use them to smuggle in a bunch of pre-formatted code.

cc @griesemer as FYI

@anjmao
Copy link
Author

@anjmao anjmao commented Jan 9, 2019

@josharian Thanks for you input. From the docs on ParseExpr it mentions that position information is undefined.

ParseExpr is a convenience function for obtaining the AST of an expression x. The position information recorded in the AST is undefined. The filename used in error messages is the empty string.

Also there is another problem. Even if I replace ast node manually I need to somehow sync token.File which is inside token.FileSet otherwise token.File lines, linesInfo, size may be incorrect. Is there a way of dealing with this in easy way?

@griesemer Maybe it make sense to extend astutil.Apply so it can accept token.FileSet or token.File and do all the handling?

@griesemer
Copy link
Contributor

@griesemer griesemer commented Jan 9, 2019

@anjmao If you replace your call of parser.ParseExpr

newCallExpr, err := parser.ParseExpr("log.Println(\"new value\")")

with the newer function parser.ParseExprFrom which takes a file set

newCallExpr, err := parser.ParseExprFrom(fset, "", `log.Println("new value")`, 0)

then it works (at least for the example you are giving). The problem with parseExpr is that file positions exist but are incompatible with the existing file set. The new function parseExprFrom makes sure to use the same file set.

(I haven't looked into why it also inserts a new empty line, both with the hand-crafted and the parsed call replacement.)

There may be other issues still, but at least this seems to fix the problem. We are aware that all of this is suboptimal (the go/ast was probably one of the first if not the first Go package that made it into the std lib and it shows its age). There are (long term) plans to improve the situation significantly by making formatting independent of position information, but unfortunately there are more pressing issues at the moment.

I'm going to close this for now. Feel free to re-open (or file a new issue) if you find other problems.

@griesemer griesemer closed this Jan 9, 2019
@golang golang locked and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.