Background: generated Go source files may contain //line directives that relate each line of Go source to the line of some other file from which it was generated. If the file named by the line directive is a relative path, cmd/compile currently interprets it like any other relative path, that is, relative to its own os.Getwd. This demands that the tool that generated the //line directive has the same working directory as the compiler (e.g. GOROOT/src), or that it creates the line directive in anticipation of the consuming process having (say) GOROOT/src as its working directory.

By contrast, go/scanner takes a different approach: it interprets relative line directives relative to the scanned file's directory. So, if it encounters the line directive //line a.yacc:1 in a file named a/a.go, the relative line directive is resolved as a/a.yacc. (In March 2018 go/scanner was changed to resemble the compiler in CL 100235, but this was reverted in August 2018 by CL 127658.)

The difference in behavior is easily observed by introducing a dummy line directive with a compiler error into an arbitrary Go source file. In this case, we'll append a relative reference from gopls/main.go to its README.md file:

xtools$ git diff
diff --git a/gopls/main.go b/gopls/main.go
index 083c4efd8..b63784ba8 100644
--- a/gopls/main.go
+++ b/gopls/main.go
@@ -34,3 +34,6 @@ func main() {
        ctx := context.Background()
        tool.Main(ctx, cmd.New(), os.Args[1:])
 }
+
+//line README.md:1
+invalid

When we build the package, the error message names a non-existent file:

xtools$ go build -o /dev/null ./gopls
# golang.org/x/tools/gopls
README.md:1: syntax error: non-declaration statement outside function body

When we use tools based on go/scanner, the error message names the correct file:

xtools$ gopackages -mode=allsyntax -deps ./gopls 2>&1 | grep README
    /Users/adonovan/w/xtools/gopls/README.md:1: expected declaration, found invalid

This is true even when we run the same commands from a different directory. Again, go build names the non-existent file, and go/scanner names the correct file:

xtools$ cd internal/
internal$ go build -o /dev/null ../gopls
# golang.org/x/tools/gopls
README.md:1: syntax error: non-declaration statement outside function body

internal$ gopackages -mode=allsyntax -deps ../gopls 2>&1 | grep README
    /Users/adonovan/w/xtools/gopls/README.md:1: expected declaration, found invalid

Proposal: The compiler's scanner should interpret relative line directives relative to the directory of the scanned file, and this behavior should be documented at https://pkg.go.dev/cmd/compile#hdr-Compiler_Directives.

This may require changes to the line directives emitted by cgo and other tools (including unparam and unconvert, whatever they are).

Arguably, this is an incompatible change, but we notice that more often than not, when published Go modules use relative line directives, they already follow the proposed discipline.

See also:

@griesemer @ianlancetaylor @findleyr @timothy-king

Comment From: gabyhelp

Related Issues

Related Code Changes

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

Comment From: ianlancetaylor

Since, as you say, the change is not backward compatible, and since we actually tried a related change in 2018 and had to roll it back, I think that we need a GODEBUG setting that can be used to disable the change.

Comment From: adonovan

Since, as you say, the change is not backward compatible, and since we actually tried a related change in 2018 and had to roll it back, I think that we need a GODEBUG setting that can be used to disable the change.

Perhaps, though the related change was in the reverse direction, so was a clear regression in functionality for source tools based on go/scanner, whereas the proposed change should mostly affect the behavior of builds that fail, which by definition are exempt from compatibility.

Comment From: prattmic

whereas the proposed change should mostly affect the behavior of builds that fail, which by definition are exempt from compatibility.

Wouldn’t it also affect the output of traceback, profiles, debuggers (unsure of this one?), etc?

Comment From: adonovan

Wouldn’t it also affect the output of traceback, profiles, debuggers (unsure of this one?), etc?

Yes, it would certainly be an observable change, but I suspect that very few existing tests of such tools rely on scenarios as complex and fragile as the specific outputs (file/line etc) of source generation tools, since they can change for all kinds of reasons. (In the specific case of cgo, we wouldn't expect any changes because the generator and consumer would change together.)

I'm not opposed to a GODEBUG flag, but we should assess whether one is needed before adding one.

Comment From: podtserkovskiy

This change also will affect compilation errors reported by Bazel and other build systems, but AFAIU this won't impact the successful build results.

It might be a bit inconvenient to generate correct relative //line directives in the build-systems like Buck or Bazel. At this moment we can easily add an argument like this -timpath=$(pwd) to cmd/cgo and others, so all the paths reported during the build be relative to "the project root".

In case when paths //line directives are relative to the generated files, we'll need to pass -timpath that adds required number of .. to go up in the directory structure to go outside of buck-out/bazel-out and get to the project root. As a result the //line derictives will look roughly like this //line ../../../../foo/bar/source_cgo.go.

BTW, I can recall one more request to clarify/fix //line behaviour #26207 from @zombiezen. The ideas from this issue align with your proposal.

Comment From: mateusz834

This would also mean that -trimpath would also work properly with line directives, right? Currently the behavior looks like this:

$ cat main.go
//line other-file.sth:1:1
package main

func main() {
        panic("here")
}
$ go run .
panic: here

goroutine 1 [running]:
main.main()
        other-file.sth:4 +0x25
exit status 2

$ cat main.go
//line /tmp/test/other-file.sth:1:1
package main

func main() {
        panic("here")
}
$ go run .
panic: here

goroutine 1 [running]:
main.main()
        /tmp/test/other-file.sth:4 +0x25
exit status 2
$ go run -trimpath .
panic: here

goroutine 1 [running]:
main.main()
        /tmp/test/other-file.sth:4 +0x25
exit status 2

Comment From: podtserkovskiy

@adonovan Is there a chance this proposal will be reviewed this week?

Comment From: adonovan

Not yet, sorry; the committee moves slowly and has a large backlog.

Comment From: podtserkovskiy

@adonovan Is there a way to roughly understand when it will be reviewed? In a month, two, etc? Is there specific order like FIFO?

Comment From: ianlancetaylor

@podtserkovskiy You can track the work of the proposal committee by following the minutes posted at #33502. You can see the status of all proposals at https://github.com/orgs/golang/projects/17.

Comment From: seankhliao

maybe an incompatible change is reason to bring in #68061 ? * //line keeps the old meaning (and gets deprecated over time?) * //go:line gets the new one

Comment From: aclements

This proposal has been added to the active column of the proposals project and will now be reviewed at the weekly proposal review meetings. — aclements for the proposal review group

Comment From: adonovan

@griesemer and I agree that the proposed behavior is correct: the litmus test should be that a line directive //line foo.go:123 at line 123 of a file whose base name is foo.go should effectively be a no-op. (Similarly //line :123.) Currently, that is not the case.

The only question is how disruptive it is. We propose to implement it and see how much of x/tools or google3 observes the change and calibrate our defensiveness (e.g. use of GODEBUG) accordingly.

Comment From: griesemer

As an aside, the various permitted //line directive syntaxes are outlined here: #24183.

Comment From: griesemer

For reference, the C #line directive appears to behave like the current (not newly proposed) Go implementation: - #line 100 sets the line line number but leaves the filename alone - #line 100 "./foo.c" sets both the line number and filename, but the filename (string) appears to be simply copied into error messages w/o any interpretation of relativeness, and irrespective of the directory within which the compiler is invoked.

Perhaps somebody with deep C history background might have some input here. @robpike ?

Comment From: podtserkovskiy

Thank you for working on this proposal 😀

The only question is how disruptive it is. We propose to implement it and see how much of x/tools or google3 observes the change and calibrate our defensiveness (e.g. use of GODEBUG) accordingly.

Bazel will need modify how it uses -trimpath and we'll need to do similar for Buck. I'm not sure, if go build will require any changes.

Comment From: aclements

@adonovan and @griesemer are going to do some empirical analysis to see how much this breaks. :)

Comment From: podtserkovskiy

Bazel will need modify how it uses -trimpath and we'll need to do similar for Buck.

Actually I think it might be better to keep cmd/cgo -trimpath interface unchanged and generate relative //line directives inside cmd/cgo as it has all the information required to do this correctly.

Comment From: aclements

Placed on hold. — aclements for the proposal review group