Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
mateusz834 opened this issue Jan 14, 2025 · 10 comments
Closed

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

mateusz834 opened this issue Jan 14, 2025 · 10 comments
Assignees
@adonovan
Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted. NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Go1.24

Comments

@mateusz834
Copy link
Member

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 mateusz834 added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 14, 2025
@mateusz834
Copy link
Member Author

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
Copy link
Member

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

@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

@adonovan
Copy link
Member

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.

@adonovan adonovan self-assigned this Jan 22, 2025
gopherbot pushed a commit that referenced this issue Jan 22, 2025
@adonovan @gopherbot
go/importer: document limitations of this API
70b603f 
Arguably it should be deprecated, but that's a process.

Updates #71272

Change-Id: I69de1f9709c45dfea0fe67d96a7bd15d3df4e2f1
Reviewed-on: https://go-review.googlesource.com/c/go/+/643795
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. FixPending Issues that have a fix which has not yet been reviewed or submitted. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jan 22, 2025
@dmitshur
Copy link
Contributor

dmitshur commented Jan 22, 2025 •
edited
Loading

@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).

@dmitshur dmitshur added the Testing An issue that has been verified to require only test changes, not just a test failure. label Jan 22, 2025
@dmitshur dmitshur added this to the Go1.24 milestone Jan 22, 2025
@adonovan
Copy link
Member

Indeed, it's just commentary (first CL) and test cleanups (second CL). I agree test CLs are not entirely without risk, but this one seems very safe to me. Either way it's definitely not critical for 1.24.

@griesemer
Copy link
Contributor

I was contemplating when I reviewed this whether it should be wait-release, but it I think since these are deterministic tests, this is safe even for 1.24.

@mateusz834
Copy link
Member Author

The docs of Fset in *types.Error is still wrong:

go/src/go/types/api.go

Lines 46 to 48 in 608acff

type Error struct {
Fset *token.FileSet // file set for interpretation of Pos
Pos token.Pos // error position

right? It might be a file set for interpretation of Pos, it does not have to be a right one. It is basically the same fset as passed to the (*types.Config).Check().

@adonovan
Copy link
Member

The only FileSet the type-checker knows about is the one passed to NewChecker; clients may assume this is the same one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@adonovan adonovan

Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted. NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Go1.24
Development

No branches or pull requests
6 participants
@dmitshur @adonovan @griesemer @gopherbot @mateusz834 @findleyr