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.
jirfag
All 17 comments
Marking as needs-decision for 1.11, to see if this was indeed an intended change in behavior.
mvdan
on 29 Jul 2018
/cc @mdempsky too.
odeke-em
on 30 Jul 2018
It looks like this is not the only issue; also column info is broken by the above commit, see https://github.com/mdempsky/unconvert/pull/30
kolyshkin
on 30 Jul 2018
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 546bab8c29 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 https://github.com/golang/go/issues/24183, maybe it is elsewhere?).
Ergo, packages relying on it now need to do the conversion.
kolyshkin
on 30 Jul 2018
One thing I still don't understand is why the column info disappears (see https://github.com/mdempsky/unconvert/pull/30).
kolyshkin
on 30 Jul 2018
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.
mvdan
on 30 Jul 2018
One thing I still don't understand is why the column info disappears (see mdempsky/unconvert#30).
kolyshkin
on 30 Jul 2018
This was an intentional change that is mentioned in the release notes.
ianlancetaylor
on 1 Aug 2018
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.
dominikh
on 1 Aug 2018
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?
ianlancetaylor
on 1 Aug 2018
What current workflows will break?
All I know is it broke a few linters (unparam and unconvert https://github.com/mdempsky/unconvert/issues/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.
kolyshkin
on 1 Aug 2018
~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.
mdempsky
on 1 Aug 2018
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.
dominikh
on 1 Aug 2018
@ianlancetaylor it breaks at least 5 popular linters, isn't it enough to not do backward-incompatible change?
jirfag
on 2 Aug 2018
Why hasn't anybody reported this as a problem until three days ago?
because linters binaries are usually built by go1.10
jirfag
on 2 Aug 2018
Change https://golang.org/cl/127658 mentions this issue: go/scanner: continue adding directory to file name
gopherbot
on 2 Aug 2018
Change https://golang.org/cl/127659 mentions this issue: doc/go1.11: remove go/scanner note
gopherbot
on 2 Aug 2018
Related issues
longzhizhi
路
3Comments
ashb
路
3Comments
ajstarks
路
3Comments
enoodle
路
3Comments
gopherbot
路
3Comments
Most helpful comment
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:
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.