From 2e0ca3aded9465405194f01b5523bed9f49f16d8 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Mon, 24 Oct 2022 16:51:25 -0400 Subject: [PATCH] go/internal/gcimporter: fix bug in struct iexport The iexport function was emitting the wrong implicit qualifying package name for unexported struct fields. The logic was deceptively similar to that in the compiler, and that was the bug: the compiler records which package a struct{...} syntax node appears in, and uses that (correctly) as the qualifier for unexported fields; but go/types only records a package in each Object, such as the field Vars, which led the author of the previous code to use the current package, not the struct's package, as the implicit qualifier. The solution is to use the package of any field; they should all be the same. (Unlike interfaces, where embedding is flattened out, leading to interface types in which method Func objects may belong to different packages, struct embedding is not flattened out, so all field Vars belong to the same package in which the struct was declared.) Also, a regression test. Thanks to Rob Findley for identifying the cause. I don't understand how this hasn't shown up sooner, since the test case is far from obscure. Change-Id: I0a6c58a566b87a148827fb0ab4655a020806c31a Reviewed-on: https://go-review.googlesource.com/c/tools/+/445097 gopls-CI: kokoro Reviewed-by: Robert Findley TryBot-Result: Gopher Robot Reviewed-by: Robert Griesemer Run-TryBot: Alan Donovan --- go/internal/gcimporter/iexport.go | 9 +++-- go/internal/gcimporter/iexport_test.go | 48 ++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 3 deletions(-) diff --git a/go/internal/gcimporter/iexport.go b/go/internal/gcimporter/iexport.go index 9a4ff329e1..db2753217a 100644 --- a/go/internal/gcimporter/iexport.go +++ b/go/internal/gcimporter/iexport.go @@ -602,14 +602,17 @@ func (w *exportWriter) doTyp(t types.Type, pkg *types.Package) { case *types.Struct: w.startType(structType) - w.setPkg(pkg, true) - n := t.NumFields() + if n > 0 { + w.setPkg(t.Field(0).Pkg(), true) // qualifying package for field objects + } else { + w.setPkg(pkg, true) + } w.uint64(uint64(n)) for i := 0; i < n; i++ { f := t.Field(i) w.pos(f.Pos()) - w.string(f.Name()) + w.string(f.Name()) // unexported fields implicitly qualified by prior setPkg w.typ(f.Type(), pkg) w.bool(f.Anonymous()) w.string(t.Tag(i)) // note (or tag) diff --git a/go/internal/gcimporter/iexport_test.go b/go/internal/gcimporter/iexport_test.go index f0e83e519f..899c9af7a4 100644 --- a/go/internal/gcimporter/iexport_test.go +++ b/go/internal/gcimporter/iexport_test.go @@ -30,6 +30,7 @@ import ( "golang.org/x/tools/go/ast/inspector" "golang.org/x/tools/go/buildutil" + "golang.org/x/tools/go/gcexportdata" "golang.org/x/tools/go/internal/gcimporter" "golang.org/x/tools/go/loader" "golang.org/x/tools/internal/typeparams/genericfeatures" @@ -403,3 +404,50 @@ func valueToRat(x constant.Value) *big.Rat { } return new(big.Rat).SetInt(new(big.Int).SetBytes(bytes)) } + +// This is a regression test for a bug in iexport of types.Struct: +// unexported fields were losing their implicit package qualifier. +func TestUnexportedStructFields(t *testing.T) { + fset := token.NewFileSet() + export := make(map[string][]byte) + + // process parses and type-checks a single-file + // package and saves its export data. + process := func(path, content string) { + syntax, err := parser.ParseFile(fset, path+"/x.go", content, 0) + if err != nil { + t.Fatal(err) + } + packages := make(map[string]*types.Package) // keys are package paths + cfg := &types.Config{ + Importer: importerFunc(func(path string) (*types.Package, error) { + data, ok := export[path] + if !ok { + return nil, fmt.Errorf("missing export data for %s", path) + } + return gcexportdata.Read(bytes.NewReader(data), fset, packages, path) + }), + } + pkg := types.NewPackage(path, syntax.Name.Name) + check := types.NewChecker(cfg, fset, pkg, nil) + if err := check.Files([]*ast.File{syntax}); err != nil { + t.Fatal(err) + } + var out bytes.Buffer + if err := gcexportdata.Write(&out, fset, pkg); err != nil { + t.Fatal(err) + } + export[path] = out.Bytes() + } + + // Historically this led to a spurious error: + // "cannot convert a.M (variable of type a.MyTime) to type time.Time" + // because the private fields of Time and MyTime were not identical. + process("time", `package time; type Time struct { x, y int }`) + process("a", `package a; import "time"; type MyTime time.Time; var M MyTime`) + process("b", `package b; import ("a"; "time"); var _ = time.Time(a.M)`) +} + +type importerFunc func(path string) (*types.Package, error) + +func (f importerFunc) Import(path string) (*types.Package, error) { return f(path) }