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

Comment From: gabyhelp

Related Issues

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

Comment From: ianlancetaylor

CC @griesemer @adonovan @findleyr

Comment From: 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

Comment From: 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.

Comment From: 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.

Comment From: adonovan

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

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

Comment From: cherrymui

cc @mrkfrmn

Comment From: mateusz834

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

Not really, i believe it is because of the fakefset in gcimporter.

https://github.com/golang/tools/blob/2cae60e3f82b3e911eab6ef0cd7a7cf589568888/internal/gcimporter/bimport.go#L36

Comment From: griesemer

I believe the compiler is correct: the unified IR serializes complete information for a (syntax) Position, which consists of its PosBase, and (absolute) line and column information. Any kind of relative information (such as relative to a //line directive) can be derived from that. In fact, writing out the relative line and colums (as suggested) would destroy that information.

The loss must be happing on the (non-compiler) reader side: it probably doesn't handle PosBase information correctly (which itself contains filename and line/column information).

Comment From: mateusz834

I think that there is one remaining problem, does the unified IR serialize the file length? That information is needed to create a token.File and avoid the fakefset.

Comment From: griesemer

@mateusz834 I don't think the UIR serializes the file length. It could but it would require a format change.