go/types: dot imports might cause errors where `(types.Error).Pos` is not part of the `(types.Error).Fset`

[mateusz834]

Reproducer:

package main

import (
	"fmt"
	"go/ast"
	"go/importer"
	"go/parser"
	"go/token"
	"go/types"
)

func main() {
	fmt.Println(run())
}

const src = `package main

import . "net/http"

type Request struct{}
`

func run() error {
	fset := token.NewFileSet()
	f, err := parser.ParseFile(fset, "test.go", src, parser.SkipObjectResolution)
	if err != nil {
		return err
	}

	cfg := types.Config{
		Importer: importer.Default(),
		Error: func(err error) {
			e := err.(types.Error)
			fmt.Println(e.Msg, e.Pos, e.Fset.Position(e.Pos))
		},
	}

	cfg.Check("test", fset, []*ast.File{f}, nil)

	return nil
}

$ go run .
Request already declared through dot-import of package http ("net/http") 41 test.go:5:6
        other declaration of Request 2883740 -
"net/http" imported and not used 22 test.go:3:8
<nil>

Note the second error other declaration of Request 2883740 -, which tries to reference the file in the net/http package.

CC @adonovan @griesemer @findleyr

[mateusz834]

This happens because of the default FileSet here:

go/src/go/importer/importer.go

Lines 72 to 84 in bd80d89

	// For calls [ForCompiler] with a new FileSet.
	//
	// Deprecated: Use [ForCompiler], which populates a FileSet
	// with the positions of objects created by the importer.
	func For(compiler string, lookup Lookup) types.Importer {
	return ForCompiler(token.NewFileSet(), compiler, lookup)
	}
	// Default returns an Importer for the compiler that built the running binary.
	// If available, the result implements [types.ImporterFrom].
	func Default() types.Importer {
	return For(runtime.Compiler, nil)
	}

It can be fixed by using the importer.ForCompiler, with the same fileset as provided to the type checker.

Importer: importer.ForCompiler(fset, runtime.Compiler, nil),

Also (not tested) but i guess it cannot happen with go/pacakges, since it uses one fileset for all operations.

[findleyr]

Thanks. Superficially this seems like it may also be a source of gopls bug reports.

[gopherbot]

Change https://go.dev/cl/643795 mentions this issue: go/importer: document limitations of this API

[gopherbot]

Change https://go.dev/cl/643777 mentions this issue: go/types: avoid importer.Default

[adonovan]

I don't think this anything to do with dot imports: it's entirely about the use of the ugly old importer.Default function in go/types tests (and similar tests). I've sent two CLs to clarify that the importer package is old and bad, and to avoid using the Default function in go/types tests, instead passing the correct FileSet where available.

FWIW gopls does not use importer.Default, so I don't think this affects it at all.

[dmitshur]

@adonovan You have a fix ready, do you intend to land it for Go 1.24 or 1.25? (Asking because the CL doesn't have a wait-release hashtag, and there's no milestone set on this issue.) It's quite late in the Go 1.24 release cycle, so the balance favors rejecting a fix, unless a case can be made that the fix is both low risk and high reward, and this doesn't seem critical at a glance. Thanks.

Edit: When asking the question above, I missed that the fix was entirely a test-only change, making it much easier to be confident in its safety (keeping in mind even test-only changes carry some risk, e.g., of introducing new flakiness or new failures).