cmd/compile: position information not properly decoded by non-compiler importers

[mateusz834]

Reproducer (using the x/tools/go/packages testing API):

func TestReproducer(t *testing.T) {
	exported := packagestest.Export(t, packagestest.Modules, []packagestest.Module{
		{
			Name: "test",
			Files: map[string]any{
				"test.go": `package test; import "test/other"; var _ = other.Test;`,
				"other/file.go": `package other

//line other.go:1:1
var Test int
`,
			},
		},
	})
	t.Cleanup(exported.Cleanup)

	exported.Config.Mode = packages.LoadSyntax
	exported.Config.Fset = token.NewFileSet()
	pkgs, err := packages.Load(exported.Config, "test")
	if err != nil {
		t.Fatal(err)
	}
	packages.PrintErrors(pkgs)

	p := pkgs[0].Imports["test/other"]
	pos := p.Types.Scope().Lookup("Test").Pos()
	t.Log(exported.Config.Fset.Position(pos)) // other.go:4:1
}

The Mode is set to packages.LoadSyntax, thus go/packages is going to load test/other from the ExportFile.

$ go test ./go/packages/ -run TestReproducer -v
=== RUN   TestReproducer
    (....)
    packages_test.go:3455: other.go:4:1
--- PASS: TestReproducer (0.15s)

The file name (other.go) comes from the line directive, but the line/col (4:1) information does not.

I think that the issue is somewhere in cmd/go, while debugging, i saw the same values here (i.e. other.go:4:1):

https://github.com/golang/tools/blob/4f820dbaf9859eaafa01a17d133583f4d8c5a73c/internal/gcimporter/ureader_yes.go#L201-L205

[gabyhelp]

Related Issues

• x/tools/cmd/goimports: line directive can cause file corruption #51119

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

[ianlancetaylor]

CC @griesemer @adonovan @findleyr

[adonovan]

Thanks for investigating. Here's a smaller reproducer that doesn't depend on x/tools/go/packages:

xtools$ cat a/a.go 
package a

//line b.go:1:1
var A int
xtools$ go run ./go/gcexportdata/main.go $(go list -export -f {{.Export}} ./a)
package a
b.go:4:1: var A int

Adding logging to reader.pos in x/tools/internal/gcimporter/ureader_yes.go reveals that the export data contains (file "b.go", line 4, col 5) for this declaration, so the problem is on the encoding side, i.e. in the compiler.

The following change to the compiler fixes the problem:

// pos writes the position of p into the element bitstream.
func (w *writer) pos(p poser) {
	w.Sync(pkgbits.SyncPos)
	pos := p.Pos()

	// TODO(mdempsky): Track down the remaining cases here and fix them.
	if !w.Bool(pos.IsKnown()) {
		return
	}

	// TODO(mdempsky): Delta encoding.
	w.posBase(pos.Base())
-	w.Uint(pos.Line())
+	w.Uint(pos.RelLine())
-	w.Uint(pos.Col())
+	w.Uint(pos.RelCol())
}

but I'm not sure whether it is the right fix overall.

@griesemer

[mateusz834]

but I'm not sure whether it is the right fix overall.

It fixes it to some extent, because still the package loaded from the export data does not have the fset.Position(pos).Column valid (it is always one), and also fset.PositionFor(pos, false) will report the line-adjusted position, not the non-adjusted.

[adonovan]

but I'm not sure whether it is the right fix overall.

It fixes it to some extent, because still the package loaded from the export data does not have the fset.Position(pos).Column valid (it is always one), and also fset.PositionFor(pos, false) will report the line-adjusted position, not the non-adjusted.

I think the first issue (column=1) arises from this restriction in cmd/compile/internal/syntax/pos.go:

func (pos Pos) RelCol() uint {
	b := pos.base
	if b.Col() == 0 {
		// base column is unknown => relative column is unknown
		// (the current specification for line directives requires
		// this to apply until the next PosBase/line directive,
		// not just until the new newline)
		return 0
	}
...

The unified export data doesn't go to the lengths that the old indexed import data does to record the line/col information precisely; see for example iimporter.decodeFile in x/tools/internal/gcimporter/iimport.go. (The historical reason is that gopls has more exacting demands than the compiler for source position information, and gopls uses indexed import data to save packages in its cache. But there's no reason the same approach couldn't be used for unified export data too.)

And the second issue (no non-adjusted info) is a consequence of all export data formats discarding the alternative //line mapping information entirely. There's no fundamental reason why all export data couldn't save the alternative mapping information too, but it's a lot of work, and in gopls we basically stopped using //line directives because it created more problems than it solved.

[adonovan]

I'm not sure whether it is the right fix overall.

In short, I think it might be the right fix for today.