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

jirfag · 2018-07-29T09:37:34Z

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.

The text was updated successfully, but these errors were encountered:

mvdan · 2018-07-29T10:28:59Z

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

odeke-em · 2018-07-29T22:40:02Z

/cc @mdempsky too.

kolyshkin · 2018-07-30T04:41:06Z

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

kolyshkin · 2018-07-30T04:43:26Z

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 · 2018-07-30T09:00:36Z

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

mvdan · 2018-07-30T09:54:50Z

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 · 2018-07-30T14:25:45Z

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

Filed #26692

unconvert output is broken when using Go 1.11. A quick example: With Go 1.10: > go run unconvert.go github.com/docker/docker/daemon/graphdriver/copy > /home/kir/go/src/github.com/docker/docker/daemon/graphdriver/copy/copy.go:242:28: unnecessary conversion With Go 1.11 beta 2: > go run unconvert.go github.com/docker/docker/daemon/graphdriver/copy > copy.go:242:0: unnecessary conversion Note that there are two issues: - column is 0; - file names have path stripped. The issue with column is filed as golang/go#26692; there is no way to fix it in unconvert. The second issue is currently discussed in golang/go#26671, but it might not be fixed since the new documentation 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. So, here's the alternative: if the file name is not absolute, prepend the path as we know it. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

ianlancetaylor · 2018-08-01T01:19:33Z

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

dominikh · 2018-08-01T02:52:43Z

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 · 2018-08-01T17:48:29Z

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 · 2018-08-01T19:00:07Z

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 · 2018-08-01T19:21:49Z

~~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 · 2018-08-01T21:38:23Z

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 · 2018-08-02T18:37:45Z

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

jirfag · 2018-08-02T18:39:16Z

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

because linters binaries are usually built by go1.10

gopherbot · 2018-08-02T19:42:42Z

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

gopherbot · 2018-08-02T19:43:19Z

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

The relevant change was reverted in CL 127658. Updates #26671 Change-Id: I0c555c8e18f4c7e289de56d3ef840d79cf0adac2 Reviewed-on: https://go-review.googlesource.com/127659 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

jirfag changed the title ~~go/scanner: file's dir lost~~ go/scanner: file's dir lost in go1.11 Jul 29, 2018

This was referenced Jul 29, 2018

Fix reporting for Go 1.11 mdempsky/unconvert#30

Closed

Panic on go1.11 environment golangci/golangci-lint#157

Closed

mvdan added this to the Go1.11 milestone Jul 29, 2018

mvdan added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jul 29, 2018

mvdan assigned griesemer Jul 29, 2018

jirfag mentioned this issue Jul 29, 2018

Unparam prints nothing in go1.11beta2 golangci/golangci-lint#168

Closed

cristaloleg mentioned this issue Jul 29, 2018

all: migrate to golangci-lint go-critic/go-critic#503

Merged

josharian added the release-blocker label Jul 30, 2018

kolyshkin mentioned this issue Jul 30, 2018

Fix file name reporting for Go 1.11 [v2] mdempsky/unconvert#31

Closed

ianlancetaylor closed this as completed Aug 1, 2018

dominikh reopened this Aug 1, 2018

kolyshkin mentioned this issue Aug 1, 2018

Bump golang to 1.11.0 moby/moby#37358

Merged

andybons assigned ianlancetaylor Aug 2, 2018

gopherbot closed this as completed in d6597ad Aug 2, 2018

kolyshkin mentioned this issue Oct 4, 2018

file names and column info broken by go1.11beta mdempsky/unconvert#29

Closed

golang locked and limited conversation to collaborators Aug 2, 2019

gopherbot added the FrozenDueToAge label Aug 2, 2019

rsc unassigned ianlancetaylor and griesemer Jun 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jirfag commented Jul 29, 2018 •

edited

mvdan commented Jul 29, 2018

odeke-em commented Jul 29, 2018

kolyshkin commented Jul 30, 2018

kolyshkin commented Jul 30, 2018 •

edited

kolyshkin commented Jul 30, 2018

mvdan commented Jul 30, 2018

kolyshkin commented Jul 30, 2018

ianlancetaylor commented Aug 1, 2018

dominikh commented Aug 1, 2018

ianlancetaylor commented Aug 1, 2018

kolyshkin commented Aug 1, 2018

mdempsky commented Aug 1, 2018 •

edited

dominikh commented Aug 1, 2018

jirfag commented Aug 2, 2018

jirfag commented Aug 2, 2018 •

edited

gopherbot commented Aug 2, 2018

gopherbot commented Aug 2, 2018

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

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

Comments

jirfag commented Jul 29, 2018 • edited

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

Does this issue reproduce with the latest release?

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

mvdan commented Jul 29, 2018

odeke-em commented Jul 29, 2018

kolyshkin commented Jul 30, 2018

kolyshkin commented Jul 30, 2018 • edited

kolyshkin commented Jul 30, 2018

mvdan commented Jul 30, 2018

kolyshkin commented Jul 30, 2018

ianlancetaylor commented Aug 1, 2018

dominikh commented Aug 1, 2018

ianlancetaylor commented Aug 1, 2018

kolyshkin commented Aug 1, 2018

mdempsky commented Aug 1, 2018 • edited

dominikh commented Aug 1, 2018

jirfag commented Aug 2, 2018

jirfag commented Aug 2, 2018 • edited

gopherbot commented Aug 2, 2018

gopherbot commented Aug 2, 2018

jirfag commented Jul 29, 2018 •

edited

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

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

kolyshkin commented Jul 30, 2018 •

edited

mdempsky commented Aug 1, 2018 •

edited

jirfag commented Aug 2, 2018 •

edited