From: David Chase Date: Thu, 11 Aug 2022 13:58:23 +0000 (-0400) Subject: cmd/compile: package-annotate structs when error would be ambiguous X-Git-Tag: go1.20rc1~155 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=ea2c27fe8204225b4d76ed4a541c6b92afec8dde;p=gostls13.git cmd/compile: package-annotate structs when error would be ambiguous Before emitting a "wanted Foo but got Bar" message for an interface type match failure, check that Foo and Bar are different. If they are not, add package paths to first unexported struct field seen, because that is the cause (a cause, there could be more than one). Replicated in go/types. Added tests to go/types and cmd/compile/internal/types2 Fixes #54258. Change-Id: Ifc2b2067d62fe2138996972cdf3b6cb7ca0ed456 Reviewed-on: https://go-review.googlesource.com/c/go/+/422914 TryBot-Result: Gopher Robot Run-TryBot: David Chase Reviewed-by: Robert Griesemer --- diff --git a/src/cmd/compile/internal/typecheck/expr.go b/src/cmd/compile/internal/typecheck/expr.go index 96f368363a..0cd69abb80 100644 --- a/src/cmd/compile/internal/typecheck/expr.go +++ b/src/cmd/compile/internal/typecheck/expr.go @@ -516,6 +516,18 @@ func tcDot(n *ir.SelectorExpr, top int) ir.Node { return n } +func wrongTypeFor(haveSym *types.Sym, haveType *types.Type, wantSym *types.Sym, wantType *types.Type) string { + haveT := fmt.Sprintf("%S", haveType) + wantT := fmt.Sprintf("%S", wantType) + if haveT == wantT { + // Add packages instead of reporting "got Foo but wanted Foo", see #54258. + haveT = haveType.Pkg().Path + "." + haveT + wantT = wantType.Pkg().Path + "." + wantT + } + return fmt.Sprintf("(wrong type for %v method)\n"+ + "\t\thave %v%s\n\t\twant %v%s", wantSym, haveSym, haveT, wantSym, wantT) +} + // tcDotType typechecks an ODOTTYPE node. func tcDotType(n *ir.TypeAssertExpr) ir.Node { n.X = Expr(n.X) @@ -539,8 +551,8 @@ func tcDotType(n *ir.TypeAssertExpr) ir.Node { var ptr int if !implements(n.Type(), t, &missing, &have, &ptr) { if have != nil && have.Sym == missing.Sym { - base.Errorf("impossible type assertion:\n\t%v does not implement %v (wrong type for %v method)\n"+ - "\t\thave %v%S\n\t\twant %v%S", n.Type(), t, missing.Sym, have.Sym, have.Type, missing.Sym, missing.Type) + base.Errorf("impossible type assertion:\n\t%v does not implement %v %s", n.Type(), t, + wrongTypeFor(have.Sym, have.Type, missing.Sym, missing.Type)) } else if ptr != 0 { base.Errorf("impossible type assertion:\n\t%v does not implement %v (%v method has pointer receiver)", n.Type(), t, missing.Sym) } else if have != nil { diff --git a/src/cmd/compile/internal/typecheck/stmt.go b/src/cmd/compile/internal/typecheck/stmt.go index 5eeab4115e..9d57edb39f 100644 --- a/src/cmd/compile/internal/typecheck/stmt.go +++ b/src/cmd/compile/internal/typecheck/stmt.go @@ -604,8 +604,8 @@ func tcSwitchType(n *ir.SwitchStmt) { } if !n1.Type().IsInterface() && !implements(n1.Type(), t, &missing, &have, &ptr) { if have != nil { - base.ErrorfAt(ncase.Pos(), "impossible type switch case: %L cannot have dynamic type %v"+ - " (wrong type for %v method)\n\thave %v%S\n\twant %v%S", guard.X, n1.Type(), missing.Sym, have.Sym, have.Type, missing.Sym, missing.Type) + base.ErrorfAt(ncase.Pos(), "impossible type switch case: %L cannot have dynamic type %v %s", guard.X, n1.Type(), + wrongTypeFor(have.Sym, have.Type, missing.Sym, missing.Type)) } else if ptr != 0 { base.ErrorfAt(ncase.Pos(), "impossible type switch case: %L cannot have dynamic type %v"+ " (%v method has pointer receiver)", guard.X, n1.Type(), missing.Sym) diff --git a/src/cmd/compile/internal/typecheck/subr.go b/src/cmd/compile/internal/typecheck/subr.go index fd8c027cf4..9760e366b5 100644 --- a/src/cmd/compile/internal/typecheck/subr.go +++ b/src/cmd/compile/internal/typecheck/subr.go @@ -397,8 +397,7 @@ func Assignop1(src, dst *types.Type) (ir.Op, string) { } else if have != nil && have.Sym == missing.Sym && have.Nointerface() { why = fmt.Sprintf(":\n\t%v does not implement %v (%v method is marked 'nointerface')", src, dst, missing.Sym) } else if have != nil && have.Sym == missing.Sym { - why = fmt.Sprintf(":\n\t%v does not implement %v (wrong type for %v method)\n"+ - "\t\thave %v%S\n\t\twant %v%S", src, dst, missing.Sym, have.Sym, have.Type, missing.Sym, missing.Type) + why = fmt.Sprintf(":\n\t%v does not implement %v %s", src, dst, wrongTypeFor(have.Sym, have.Type, missing.Sym, missing.Type)) } else if ptr != 0 { why = fmt.Sprintf(":\n\t%v does not implement %v (%v method has pointer receiver)", src, dst, missing.Sym) } else if have != nil { diff --git a/src/cmd/compile/internal/types2/issues_test.go b/src/cmd/compile/internal/types2/issues_test.go index 1fda04b9c5..4d0dcfd672 100644 --- a/src/cmd/compile/internal/types2/issues_test.go +++ b/src/cmd/compile/internal/types2/issues_test.go @@ -10,6 +10,7 @@ import ( "cmd/compile/internal/syntax" "fmt" "internal/testenv" + "regexp" "sort" "strings" "testing" @@ -703,3 +704,123 @@ func TestIssue51093(t *testing.T) { } } } + +func TestIssue54258(t *testing.T) { + tests := []struct{ main, b, want string }{ + { //--------------------------------------------------------------- + `package main +import "b" +type I0 interface { + M0(w struct{ f string }) +} +var _ I0 = b.S{} +`, + `package b +type S struct{} +func (S) M0(struct{ f string }) {} +`, + `6:12: cannot use b[.]S{} [(]value of type b[.]S[)] as I0 value in variable declaration: b[.]S does not implement I0 [(]wrong type for method M0[)] +.*have M0[(]struct{f string /[*] package b [*]/ }[)] +.*want M0[(]struct{f string /[*] package main [*]/ }[)]`}, + + { //--------------------------------------------------------------- + `package main +import "b" +type I1 interface { + M1(struct{ string }) +} +var _ I1 = b.S{} +`, + `package b +type S struct{} +func (S) M1(struct{ string }) {} +`, + `6:12: cannot use b[.]S{} [(]value of type b[.]S[)] as I1 value in variable declaration: b[.]S does not implement I1 [(]wrong type for method M1[)] +.*have M1[(]struct{string /[*] package b [*]/ }[)] +.*want M1[(]struct{string /[*] package main [*]/ }[)]`}, + + { //--------------------------------------------------------------- + `package main +import "b" +type I2 interface { + M2(y struct{ f struct{ f string } }) +} +var _ I2 = b.S{} +`, + `package b +type S struct{} +func (S) M2(struct{ f struct{ f string } }) {} +`, + `6:12: cannot use b[.]S{} [(]value of type b[.]S[)] as I2 value in variable declaration: b[.]S does not implement I2 [(]wrong type for method M2[)] +.*have M2[(]struct{f struct{f string} /[*] package b [*]/ }[)] +.*want M2[(]struct{f struct{f string} /[*] package main [*]/ }[)]`}, + + { //--------------------------------------------------------------- + `package main +import "b" +type I3 interface { + M3(z struct{ F struct{ f string } }) +} +var _ I3 = b.S{} +`, + `package b +type S struct{} +func (S) M3(struct{ F struct{ f string } }) {} +`, + `6:12: cannot use b[.]S{} [(]value of type b[.]S[)] as I3 value in variable declaration: b[.]S does not implement I3 [(]wrong type for method M3[)] +.*have M3[(]struct{F struct{f string /[*] package b [*]/ }}[)] +.*want M3[(]struct{F struct{f string /[*] package main [*]/ }}[)]`}, + + { //--------------------------------------------------------------- + `package main +import "b" +type I4 interface { + M4(_ struct { *string }) +} +var _ I4 = b.S{} +`, + `package b +type S struct{} +func (S) M4(struct { *string }) {} +`, + `6:12: cannot use b[.]S{} [(]value of type b[.]S[)] as I4 value in variable declaration: b[.]S does not implement I4 [(]wrong type for method M4[)] +.*have M4[(]struct{[*]string /[*] package b [*]/ }[)] +.*want M4[(]struct{[*]string /[*] package main [*]/ }[)]`}, + + { //--------------------------------------------------------------- + `package main +import "b" +type t struct{ A int } +type I5 interface { + M5(_ struct {b.S;t}) +} +var _ I5 = b.S{} +`, + `package b +type S struct{} +type t struct{ A int } +func (S) M5(struct {S;t}) {} +`, + `7:12: cannot use b[.]S{} [(]value of type b[.]S[)] as I5 value in variable declaration: b[.]S does not implement I5 [(]wrong type for method M5[)] +.*have M5[(]struct{b[.]S; b[.]t}[)] +.*want M5[(]struct{b[.]S; t}[)]`}, + } + + test := func(main, imported, want string) { + re := regexp.MustCompile(want) + a := mustTypecheck("b", imported, nil) + bast := mustParse("", main) + conf := Config{Importer: importHelper{pkg: a}} + _, err := conf.Check(bast.PkgName.Value, []*syntax.File{bast}, nil) + if err == nil { + t.Errorf("Expected failure, but it did not") + } else if got := err.Error(); !re.MatchString(got) { + t.Errorf("Wanted match for\n%s\n but got \n%s", want, got) + } else if testing.Verbose() { + t.Logf("Saw expected\n%s", err.Error()) + } + } + for _, t := range tests { + test(t.main, t.b, t.want) + } +} diff --git a/src/cmd/compile/internal/types2/lookup.go b/src/cmd/compile/internal/types2/lookup.go index 21cad04433..f66f2ef98e 100644 --- a/src/cmd/compile/internal/types2/lookup.go +++ b/src/cmd/compile/internal/types2/lookup.go @@ -390,15 +390,21 @@ func (check *Checker) missingMethodCause(V, T Type, m, alt *Func) string { if alt != nil { if m.Name() != alt.Name() { return check.sprintf("(missing %s)\n\t\thave %s\n\t\twant %s", - mname, check.funcString(alt), check.funcString(m)) + mname, check.funcString(alt, false), check.funcString(m, false)) } if Identical(m.typ, alt.typ) { return check.sprintf("(%s has pointer receiver)", mname) } + altS, mS := check.funcString(alt, false), check.funcString(m, false) + if altS == mS { + // Would tell the user that Foo isn't a Foo, add package information to disambiguate. See #54258. + altS, mS = check.funcString(alt, true), check.funcString(m, true) + } + return check.sprintf("(wrong type for %s)\n\t\thave %s\n\t\twant %s", - mname, check.funcString(alt), check.funcString(m)) + mname, altS, mS) } if isInterfacePtr(V) { @@ -433,13 +439,14 @@ func (check *Checker) interfacePtrError(T Type) string { // funcString returns a string of the form name + signature for f. // check may be nil. -func (check *Checker) funcString(f *Func) string { +func (check *Checker) funcString(f *Func, pkgInfo bool) string { buf := bytes.NewBufferString(f.name) var qf Qualifier - if check != nil { + if check != nil && !pkgInfo { qf = check.qualifier } w := newTypeWriter(buf, qf) + w.pkgInfo = pkgInfo w.paramNames = false w.signature(f.typ.(*Signature)) return buf.String() diff --git a/src/cmd/compile/internal/types2/typestring.go b/src/cmd/compile/internal/types2/typestring.go index 94b8ba4ac6..2307b6139d 100644 --- a/src/cmd/compile/internal/types2/typestring.go +++ b/src/cmd/compile/internal/types2/typestring.go @@ -58,9 +58,8 @@ func WriteType(buf *bytes.Buffer, typ Type, qf Qualifier) { } // WriteSignature writes the representation of the signature sig to buf, -// without a leading "func" keyword. -// The Qualifier controls the printing of -// package-level objects, and may be nil. +// without a leading "func" keyword. The Qualifier controls the printing +// of package-level objects, and may be nil. func WriteSignature(buf *bytes.Buffer, sig *Signature, qf Qualifier) { newTypeWriter(buf, qf).signature(sig) } @@ -73,15 +72,16 @@ type typeWriter struct { tparams *TypeParamList // local type parameters paramNames bool // if set, write function parameter names, otherwise, write types only tpSubscripts bool // if set, write type parameter indices as subscripts + pkgInfo bool // package-annotate first unexported-type field to avoid confusing type description } func newTypeWriter(buf *bytes.Buffer, qf Qualifier) *typeWriter { - return &typeWriter{buf, make(map[Type]bool), qf, nil, nil, true, false} + return &typeWriter{buf, make(map[Type]bool), qf, nil, nil, true, false, false} } func newTypeHasher(buf *bytes.Buffer, ctxt *Context) *typeWriter { assert(ctxt != nil) - return &typeWriter{buf, make(map[Type]bool), nil, ctxt, nil, false, false} + return &typeWriter{buf, make(map[Type]bool), nil, ctxt, nil, false, false, false} } func (w *typeWriter) byte(b byte) { @@ -148,6 +148,16 @@ func (w *typeWriter) typ(typ Type) { if i > 0 { w.byte(';') } + + // If disambiguating one struct for another, look for the first unexported field. + // Do this first in case of nested structs; tag the first-outermost field. + pkgAnnotate := false + if w.qf == nil && w.pkgInfo && !isExported(f.name) { + // note for embedded types, type name is field name, and "string" etc are lower case hence unexported. + pkgAnnotate = true + w.pkgInfo = false // only tag once + } + // This doesn't do the right thing for embedded type // aliases where we should print the alias name, not // the aliased type (see issue #44410). @@ -156,6 +166,11 @@ func (w *typeWriter) typ(typ Type) { w.byte(' ') } w.typ(f.typ) + if pkgAnnotate { + w.string(" /* package ") + w.string(f.pkg.Path()) + w.string(" */ ") + } if tag := t.Tag(i); tag != "" { w.byte(' ') // TODO(gri) If tag contains blanks, replacing them with '#' diff --git a/src/go/types/issues_test.go b/src/go/types/issues_test.go index 02ec67ff84..debe3216d4 100644 --- a/src/go/types/issues_test.go +++ b/src/go/types/issues_test.go @@ -12,6 +12,7 @@ import ( "go/importer" "go/token" "internal/testenv" + "regexp" "sort" "strings" "testing" @@ -729,3 +730,124 @@ func TestIssue51093(t *testing.T) { } } } + +func TestIssue54258(t *testing.T) { + + tests := []struct{ main, b, want string }{ + { //--------------------------------------------------------------- + `package main +import "b" +type I0 interface { + M0(w struct{ f string }) +} +var _ I0 = b.S{} +`, + `package b +type S struct{} +func (S) M0(struct{ f string }) {} +`, + `6:12: cannot use b[.]S{} [(]value of type b[.]S[)] as I0 value in variable declaration: b[.]S does not implement I0 [(]wrong type for method M0[)] +.*have M0[(]struct{f string /[*] package b [*]/ }[)] +.*want M0[(]struct{f string /[*] package main [*]/ }[)]`}, + + { //--------------------------------------------------------------- + `package main +import "b" +type I1 interface { + M1(struct{ string }) +} +var _ I1 = b.S{} +`, + `package b +type S struct{} +func (S) M1(struct{ string }) {} +`, + `6:12: cannot use b[.]S{} [(]value of type b[.]S[)] as I1 value in variable declaration: b[.]S does not implement I1 [(]wrong type for method M1[)] +.*have M1[(]struct{string /[*] package b [*]/ }[)] +.*want M1[(]struct{string /[*] package main [*]/ }[)]`}, + + { //--------------------------------------------------------------- + `package main +import "b" +type I2 interface { + M2(y struct{ f struct{ f string } }) +} +var _ I2 = b.S{} +`, + `package b +type S struct{} +func (S) M2(struct{ f struct{ f string } }) {} +`, + `6:12: cannot use b[.]S{} [(]value of type b[.]S[)] as I2 value in variable declaration: b[.]S does not implement I2 [(]wrong type for method M2[)] +.*have M2[(]struct{f struct{f string} /[*] package b [*]/ }[)] +.*want M2[(]struct{f struct{f string} /[*] package main [*]/ }[)]`}, + + { //--------------------------------------------------------------- + `package main +import "b" +type I3 interface { + M3(z struct{ F struct{ f string } }) +} +var _ I3 = b.S{} +`, + `package b +type S struct{} +func (S) M3(struct{ F struct{ f string } }) {} +`, + `6:12: cannot use b[.]S{} [(]value of type b[.]S[)] as I3 value in variable declaration: b[.]S does not implement I3 [(]wrong type for method M3[)] +.*have M3[(]struct{F struct{f string /[*] package b [*]/ }}[)] +.*want M3[(]struct{F struct{f string /[*] package main [*]/ }}[)]`}, + + { //--------------------------------------------------------------- + `package main +import "b" +type I4 interface { + M4(_ struct { *string }) +} +var _ I4 = b.S{} +`, + `package b +type S struct{} +func (S) M4(struct { *string }) {} +`, + `6:12: cannot use b[.]S{} [(]value of type b[.]S[)] as I4 value in variable declaration: b[.]S does not implement I4 [(]wrong type for method M4[)] +.*have M4[(]struct{[*]string /[*] package b [*]/ }[)] +.*want M4[(]struct{[*]string /[*] package main [*]/ }[)]`}, + + { //--------------------------------------------------------------- + `package main +import "b" +type t struct{ A int } +type I5 interface { + M5(_ struct {b.S;t}) +} +var _ I5 = b.S{} +`, + `package b +type S struct{} +type t struct{ A int } +func (S) M5(struct {S;t}) {} +`, + `7:12: cannot use b[.]S{} [(]value of type b[.]S[)] as I5 value in variable declaration: b[.]S does not implement I5 [(]wrong type for method M5[)] +.*have M5[(]struct{b[.]S; b[.]t}[)] +.*want M5[(]struct{b[.]S; t}[)]`}, + } + + test := func(main, imported, want string) { + re := regexp.MustCompile(want) + a := mustTypecheck("b", imported, nil) + bast := mustParse(fset, "", main) + conf := Config{Importer: importHelper{pkg: a}} + _, err := conf.Check(bast.Name.Name, fset, []*ast.File{bast}, nil) + if err == nil { + t.Errorf("Expected failure, but it did not") + } else if got := err.Error(); !re.MatchString(got) { + t.Errorf("Wanted match for\n%s\n but got \n%s", want, got) + } else if testing.Verbose() { + t.Logf("Saw expected\n%s", err.Error()) + } + } + for _, t := range tests { + test(t.main, t.b, t.want) + } +} diff --git a/src/go/types/lookup.go b/src/go/types/lookup.go index 2fac097ccb..4eedcc23a1 100644 --- a/src/go/types/lookup.go +++ b/src/go/types/lookup.go @@ -390,15 +390,21 @@ func (check *Checker) missingMethodCause(V, T Type, m, alt *Func) string { if alt != nil { if m.Name() != alt.Name() { return check.sprintf("(missing %s)\n\t\thave %s\n\t\twant %s", - mname, check.funcString(alt), check.funcString(m)) + mname, check.funcString(alt, false), check.funcString(m, false)) } if Identical(m.typ, alt.typ) { return check.sprintf("(%s has pointer receiver)", mname) } + altS, mS := check.funcString(alt, false), check.funcString(m, false) + if altS == mS { + // Would tell the user that Foo isn't a Foo, add package information to disambiguate. See #54258. + altS, mS = check.funcString(alt, true), check.funcString(m, true) + } + return check.sprintf("(wrong type for %s)\n\t\thave %s\n\t\twant %s", - mname, check.funcString(alt), check.funcString(m)) + mname, altS, mS) } if isInterfacePtr(V) { @@ -431,14 +437,16 @@ func (check *Checker) interfacePtrError(T Type) string { return check.sprintf("type %s is pointer to interface, not interface", T) } +// funcString returns a string of the form name + signature for f. // check may be nil. -func (check *Checker) funcString(f *Func) string { +func (check *Checker) funcString(f *Func, pkgInfo bool) string { buf := bytes.NewBufferString(f.name) var qf Qualifier - if check != nil { + if check != nil && !pkgInfo { qf = check.qualifier } w := newTypeWriter(buf, qf) + w.pkgInfo = pkgInfo w.paramNames = false w.signature(f.typ.(*Signature)) return buf.String() diff --git a/src/go/types/object.go b/src/go/types/object.go index 6e63948680..f5f4859999 100644 --- a/src/go/types/object.go +++ b/src/go/types/object.go @@ -9,6 +9,8 @@ import ( "fmt" "go/constant" "go/token" + "unicode" + "unicode/utf8" ) // An Object describes a named language entity such as a package, @@ -57,6 +59,11 @@ type Object interface { setScopePos(pos token.Pos) } +func isExported(name string) bool { + ch, _ := utf8.DecodeRuneInString(name) + return unicode.IsUpper(ch) +} + // Id returns name if it is exported, otherwise it // returns the name qualified with the package path. func Id(pkg *Package, name string) string { diff --git a/src/go/types/typestring.go b/src/go/types/typestring.go index bf541fc263..33251d779c 100644 --- a/src/go/types/typestring.go +++ b/src/go/types/typestring.go @@ -59,9 +59,8 @@ func WriteType(buf *bytes.Buffer, typ Type, qf Qualifier) { } // WriteSignature writes the representation of the signature sig to buf, -// without a leading "func" keyword. -// The Qualifier controls the printing of -// package-level objects, and may be nil. +// without a leading "func" keyword. The Qualifier controls the printing +// of package-level objects, and may be nil. func WriteSignature(buf *bytes.Buffer, sig *Signature, qf Qualifier) { newTypeWriter(buf, qf).signature(sig) } @@ -74,15 +73,16 @@ type typeWriter struct { tparams *TypeParamList // local type parameters paramNames bool // if set, write function parameter names, otherwise, write types only tpSubscripts bool // if set, write type parameter indices as subscripts + pkgInfo bool // package-annotate first unexported-type field to avoid confusing type description } func newTypeWriter(buf *bytes.Buffer, qf Qualifier) *typeWriter { - return &typeWriter{buf, make(map[Type]bool), qf, nil, nil, true, false} + return &typeWriter{buf, make(map[Type]bool), qf, nil, nil, true, false, false} } func newTypeHasher(buf *bytes.Buffer, ctxt *Context) *typeWriter { assert(ctxt != nil) - return &typeWriter{buf, make(map[Type]bool), nil, ctxt, nil, false, false} + return &typeWriter{buf, make(map[Type]bool), nil, ctxt, nil, false, false, false} } func (w *typeWriter) byte(b byte) { @@ -149,6 +149,16 @@ func (w *typeWriter) typ(typ Type) { if i > 0 { w.byte(';') } + + // If disambiguating one struct for another, look for the first unexported field. + // Do this first in case of nested structs; tag the first-outermost field. + pkgAnnotate := false + if w.qf == nil && w.pkgInfo && !isExported(f.name) { + // note for embedded types, type name is field name, and "string" etc are lower case hence unexported. + pkgAnnotate = true + w.pkgInfo = false // only tag once + } + // This doesn't do the right thing for embedded type // aliases where we should print the alias name, not // the aliased type (see issue #44410). @@ -157,6 +167,11 @@ func (w *typeWriter) typ(typ Type) { w.byte(' ') } w.typ(f.typ) + if pkgAnnotate { + w.string(" /* package ") + w.string(f.pkg.Path()) + w.string(" */ ") + } if tag := t.Tag(i); tag != "" { w.byte(' ') // TODO(rfindley) If tag contains blanks, replacing them with '#'