go/scanner: file's dir lost in go1.11

[jirfag]

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

go version go1.11beta2 darwin/amd64

Does this issue reproduce with the latest release?

beta

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

GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/denis/Library/Caches/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/denis/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/nl/54f5x38s4m53mkzzj92zsj340000gn/T/go-build997483412=/tmp/go-build -gno-record-gcc-switches -fno-common"

Some linters (golangci-lint, unparam, unconvert, unused, megacheck) extract file name from *ast.File by calling fset.Position(f.Pos()).Filename. It was broken in go1.11 by this commit.

Demo: https://play.golang.org/p/-r4T4-EtjUu

The bug is that this code block was removed:

filename = filepath.Clean(filename)	
if !filepath.IsAbs(filename) {	
  // make filename relative to current directory	
  filename = filepath.Join(s.dir, filename)	
}

and we've lost dir in file's name.

[mvdan]

Marking as needs-decision for 1.11, to see if this was indeed an intended change in behavior.

[odeke-em]

/cc @mdempsky too.

[kolyshkin]

It looks like this is not the only issue; also column info is broken by the above commit, see mdempsky/unconvert#30

[kolyshkin]

if this was indeed an intended change in behavior.

Looks like not, as the code to have either relative or absolute path (rather than just a file name) is still there, but it is not working (for some cases?).

update: the code to prepend the path to file name is not there, commit 546bab8 removes it. The commit message says:

The filenames in line directives now remain untouched by the scanner;
there is no cleanup or conversion of relative into absolute paths
anymore, in sync with what the compiler's scanner/parser are doing.
Any kind of filename transformation has to be done by a client. This
makes the scanner code simpler and also more predictable.

I.e. it was intended behavior (although I don't see traces of discussion about that in #24183, maybe it is elsewhere?).

Ergo, packages relying on it now need to do the conversion.

[kolyshkin]

One thing I still don't understand is why the column info disappears (see mdempsky/unconvert#30).

[mvdan]

Note that "go/scanner client" is up for interpretation. That could mean the packages more widely used by tools and linters, like go/parser and go/loader, or it might mean any of the third-party tools relying on filenames from any of these libraries.

I'm not sure which was meant, but I'd be a bit surprised if changes were required of all the tools out there for 1.11. That sounds like a backwards incompatible change to me.

[kolyshkin]

One thing I still don't understand is why the column info disappears (see mdempsky/unconvert#30).

Filed #26692

[ianlancetaylor]

This was an intentional change that is mentioned in the release notes.

[dominikh]

This was an intentional change that is mentioned in the release notes.

Mentioning it in the release notes doesn't magically make it exempt from the backwards compatibility promise.

It doesn't even qualify as a bug fix, considering the reason given:

The filenames in line directives now remain untouched by the scanner;
there is no cleanup or conversion of relative into absolute paths
anymore, in sync with what the compiler's scanner/parser are doing.
Any kind of filename transformation has to be done by a client. This
makes the scanner code simpler and also more predictable.

Users of go/* frankly don't care what the compiler's internal packages are doing, and making existing code simpler isn't a valid reason for breaking backwards compatibility.

[ianlancetaylor]

Yes, it's a backward incompatible change for packages that use go/scanner to read input files with //line directives that use relative paths in the file names. The new code seems clearer and easier to reason about for users of that code. The old approach lost information, in that there was no way to tell whether the current directory was added or not.

That said, we should certainly be pragmatic about this. What is the real world effect? After all, it only affects files with //line directives. What current workflows will break? Why hasn't anybody reported this as a problem until three days ago?

[kolyshkin]

What current workflows will break?

All I know is it broke a few linters (unparam and unconvert mdempsky/unconvert#31), which will certainly break a number of CI workflows once go 1.11 is be out of beta and more people start using it. This is also the reason why there is relative silence about the issue -- most users do not try betas.

I have noticed the issue since 1.11beta1 but was aiming for a fix in unconvert, as I failed (or neglected) to track the fix down to golang.

[mdempsky]

Can/should cgo write out absolute file paths in its //line directives? I think that would address mdempsky/unconvert#31.

Edit: Nevermind, it seems to do that already.

[dominikh]

One prominent example of relative line directives in Go code are yacc-based parsers, for example https://github.com/dinedal/textql/tree/master/sqlparser

More generally, any transformative offline code generation uses relative line directives, since it wouldn't make sense to check in absolute paths that are specific to someone's development machine.

It will affect any 3rd party tool that prints positions in Go code, e.g. all the popular linters.

[jirfag]

@ianlancetaylor it breaks at least 5 popular linters, isn't it enough to not do backward-incompatible change?

[jirfag]

Why hasn't anybody reported this as a problem until three days ago?

because linters binaries are usually built by go1.10

[gopherbot]

Change https://golang.org/cl/127658 mentions this issue: go/scanner: continue adding directory to file name

[gopherbot]

Change https://golang.org/cl/127659 mentions this issue: doc/go1.11: remove go/scanner note