proposal: cmd/{go,cgo,compile}: change interpretation of relative //line directives

adonovan commented 1 day ago

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

gabyhelp commented 1 day ago

Related Issues

Related Code Changes

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

ianlancetaylor commented 1 day ago

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.

adonovan commented 1 day ago

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.

prattmic commented 1 day ago

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?

adonovan commented 16 hours ago

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.

podtserkovskiy commented 6 hours ago

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.

golang / go

proposal: cmd/{go,cgo,compile}: change interpretation of relative //line directives #70478