diff --git a/cmd/vet/types.go b/cmd/vet/types.go index 05af1037..561f936a 100644 --- a/cmd/vet/types.go +++ b/cmd/vet/types.go @@ -326,7 +326,8 @@ func (f *File) isErrorMethodCall(call *ast.CallExpr) bool { // that workaround is no longer necessary. // TODO: This could be better once issue 6259 is fixed. func (f *File) hasMethod(typ types.Type, name string) bool { - obj, _, _ := types.LookupFieldOrMethod(typ, f.pkg.typesPkg, name) + // assume we have an addressable variable of type typ + obj, _, _ := types.LookupFieldOrMethod(typ, true, f.pkg.typesPkg, name) _, ok := obj.(*types.Func) return ok } diff --git a/go/ssa/interp/external.go b/go/ssa/interp/external.go index 9289696a..a52d61ef 100644 --- a/go/ssa/interp/external.go +++ b/go/ssa/interp/external.go @@ -140,7 +140,7 @@ func wrapError(err error) value { func ext۰sync۰Pool۰Get(fr *frame, args []value) value { Pool := fr.i.prog.ImportedPackage("sync").Type("Pool").Object() - _, newIndex, _ := types.LookupFieldOrMethod(Pool.Type(), Pool.Pkg(), "New") + _, newIndex, _ := types.LookupFieldOrMethod(Pool.Type(), false, Pool.Pkg(), "New") if New := (*args[0].(*value)).(structure)[newIndex[0]]; New != nil { return call(fr.i, fr, 0, New, nil) diff --git a/go/types/api_test.go b/go/types/api_test.go index eb41842d..87f220f8 100644 --- a/go/types/api_test.go +++ b/go/types/api_test.go @@ -740,12 +740,12 @@ func main() { "new(A).f": {"method (*main.A) f(int)", "->[0 0]"}, "A{}.g": {"method (main.A) g()", ".[1 0]"}, "new(A).g": {"method (*main.A) g()", "->[1 0]"}, - "new(A).h": {"method (*main.A) h()", "->[1 1]"}, + "new(A).h": {"method (*main.A) h()", "->[1 1]"}, // TODO(gri) should this report .[1 1] ? "B{}.f": {"method (main.B) f(int)", ".[0]"}, "new(B).f": {"method (*main.B) f(int)", "->[0]"}, "C{}.g": {"method (main.C) g()", ".[0]"}, "new(C).g": {"method (*main.C) g()", "->[0]"}, - "new(C).h": {"method (*main.C) h()", "->[1]"}, + "new(C).h": {"method (*main.C) h()", "->[1]"}, // TODO(gri) should this report .[1] ? "A.f": {"method expr (main.A) f(main.A, int)", "->[0 0]"}, "(*A).f": {"method expr (*main.A) f(*main.A, int)", "->[0 0]"}, @@ -827,3 +827,77 @@ var _ = a.C2 makePkg("a", libSrc) makePkg("main", mainSrc) // don't crash when type-checking this package } + +func TestLookupFieldOrMethod(t *testing.T) { + // Test cases assume a lookup of the form a.f or x.f, where a stands for an + // addressable value, and x for a non-addressable value (even though a variable + // for ease of test case writing). + var tests = []struct { + src string + found bool + index []int + indirect bool + }{ + // field lookups + {"var x T; type T struct{}", false, nil, false}, + {"var x T; type T struct{ f int }", true, []int{0}, false}, + {"var x T; type T struct{ a, b, f, c int }", true, []int{2}, false}, + + // method lookups + {"var a T; type T struct{}; func (T) f() {}", true, []int{0}, false}, + {"var a *T; type T struct{}; func (T) f() {}", true, []int{0}, true}, + {"var a T; type T struct{}; func (*T) f() {}", true, []int{0}, false}, + {"var a *T; type T struct{}; func (*T) f() {}", true, []int{0}, true}, // TODO(gri) should this report indirect = false? + + // collisions + {"type ( E1 struct{ f int }; E2 struct{ f int }; x struct{ E1; *E2 })", false, []int{1, 0}, false}, + {"type ( E1 struct{ f int }; E2 struct{}; x struct{ E1; *E2 }); func (E2) f() {}", false, []int{1, 0}, false}, + + // outside methodset + // (*T).f method exists, but value of type T is not addressable + {"var x T; type T struct{}; func (*T) f() {}", false, nil, true}, + } + + for _, test := range tests { + pkg, err := pkgFor("test", "package p;"+test.src, nil) + if err != nil { + t.Errorf("%s: incorrect test case: %s", test.src, err) + continue + } + + obj := pkg.Scope().Lookup("a") + if obj == nil { + if obj = pkg.Scope().Lookup("x"); obj == nil { + t.Errorf("%s: incorrect test case - no object a or x", test.src) + continue + } + } + + f, index, indirect := LookupFieldOrMethod(obj.Type(), obj.Name() == "a", pkg, "f") + if (f != nil) != test.found { + if f == nil { + t.Errorf("%s: got no object; want one", test.src) + } else { + t.Errorf("%s: got object = %v; want none", test.src, f) + } + } + if !sameSlice(index, test.index) { + t.Errorf("%s: got index = %v; want %v", test.src, index, test.index) + } + if indirect != test.indirect { + t.Errorf("%s: got indirect = %v; want %v", test.src, indirect, test.indirect) + } + } +} + +func sameSlice(a, b []int) bool { + if len(a) != len(b) { + return false + } + for i, x := range a { + if x != b[i] { + return false + } + } + return true +} diff --git a/go/types/builtins.go b/go/types/builtins.go index fae00863..43e808f4 100644 --- a/go/types/builtins.go +++ b/go/types/builtins.go @@ -489,7 +489,7 @@ func (check *Checker) builtin(x *operand, call *ast.CallExpr, id builtinId) (_ b } base := derefStructPtr(x.typ) sel := selx.Sel.Name - obj, index, indirect := LookupFieldOrMethod(base, check.pkg, sel) + obj, index, indirect := LookupFieldOrMethod(base, false, check.pkg, sel) switch obj.(type) { case nil: check.invalidArg(x.pos(), "%s has no single field %s", base, sel) diff --git a/go/types/call.go b/go/types/call.go index 8f77620f..093a2c57 100644 --- a/go/types/call.go +++ b/go/types/call.go @@ -302,12 +302,15 @@ func (check *Checker) selector(x *operand, e *ast.SelectorExpr) { goto Error } - obj, index, indirect = LookupFieldOrMethod(x.typ, check.pkg, sel) + obj, index, indirect = LookupFieldOrMethod(x.typ, x.mode == variable, check.pkg, sel) if obj == nil { - if index != nil { + switch { + case index != nil: // TODO(gri) should provide actual type where the conflict happens check.invalidOp(e.Pos(), "ambiguous selector %s", sel) - } else { + case indirect: + check.invalidOp(e.Pos(), "%s is not in method set of %s", sel, x.typ) + default: check.invalidOp(e.Pos(), "%s has no field or method %s", x, sel) } goto Error @@ -321,12 +324,6 @@ func (check *Checker) selector(x *operand, e *ast.SelectorExpr) { goto Error } - // verify that m is in the method set of x.typ - if !indirect && ptrRecv(m) { - check.invalidOp(e.Pos(), "%s is not in method set of %s", sel, x.typ) - goto Error - } - check.recordSelection(e, MethodExpr, x.typ, m, index, indirect) // the receiver type becomes the type of the first function @@ -358,18 +355,8 @@ func (check *Checker) selector(x *operand, e *ast.SelectorExpr) { x.typ = obj.typ case *Func: - // TODO(gri) This code appears elsewhere, too. Factor! - // verify that obj is in the method set of x.typ (or &(x.typ) if x is addressable) - // - // spec: "A method call x.m() is valid if the method set of (the type of) x - // contains m and the argument list can be assigned to the parameter - // list of m. If x is addressable and &x's method set contains m, x.m() - // is shorthand for (&x).m()". - if !indirect && x.mode != variable && ptrRecv(obj) { - check.invalidOp(e.Pos(), "%s is not in method set of %s", sel, x) - goto Error - } - + // TODO(gri) If we needed to take into account the receiver's + // addressability, should we report the type &(x.typ) instead? check.recordSelection(e, MethodVal, x.typ, obj, index, indirect) if debug { diff --git a/go/types/lookup.go b/go/types/lookup.go index 323b6c03..b4fc74ce 100644 --- a/go/types/lookup.go +++ b/go/types/lookup.go @@ -6,15 +6,11 @@ package types -// TODO(gri) The named type consolidation and seen maps below must be -// indexed by unique keys for a given type. Verify that named -// types always have only one representation (even when imported -// indirectly via different packages.) - // LookupFieldOrMethod looks up a field or method with given package and name // in T and returns the corresponding *Var or *Func, an index sequence, and a // bool indicating if there were any pointer indirections on the path to the -// field or method. +// field or method. If addressable is set, T is the type of an addressable +// variable (only matters for method lookups). // // The last index entry is the field or method index in the (possibly embedded) // type where the entry was found, either: @@ -23,14 +19,21 @@ package types // 2) the list of all methods (method set) of an interface type; or // 3) the list of fields of a struct type. // -// The earlier index entries are the indices of the embedded fields traversed -// to get to the found entry, starting at depth 0. +// The earlier index entries are the indices of the anonymous struct fields +// traversed to get to the found entry, starting at depth 0. // // If no entry is found, a nil object is returned. In this case, the returned -// index sequence points to an ambiguous entry if it exists, or it is nil. +// index and indirect values have the following meaning: // -func LookupFieldOrMethod(T Type, pkg *Package, name string) (obj Object, index []int, indirect bool) { - obj, index, indirect = lookupFieldOrMethod(T, pkg, name) +// - If index != nil, the index sequence points to an ambiguous entry +// (the same name appeared more than once at the same embedding level). +// +// - If indirect is set, a method with a pointer receiver type was found +// but there was no pointer on the path from the actual receiver type to +// the method's formal receiver base type, nor was the receiver addressable. +// +func LookupFieldOrMethod(T Type, addressable bool, pkg *Package, name string) (obj Object, index []int, indirect bool) { + obj, index, indirect = lookupFieldOrMethod(T, addressable, pkg, name) if obj != nil { return } @@ -50,16 +53,13 @@ func LookupFieldOrMethod(T Type, pkg *Package, name string) (obj Object, index [ // of P (i.e., *S). We cannot do this always because we might find // methods that don't exist for P but for S (e.g., m). Thus, if the // result is a method we need to discard it. - // - // TODO(gri) WTF? There isn't a more direct way? Perhaps we should - // outlaw named types to pointer types - they are almost - // never what one wants, anyway. + // (See also issue 8590). if t, _ := T.(*Named); t != nil { u := t.underlying if _, ok := u.(*Pointer); ok { // typ is a named type with an underlying type of the form *T, // start the search with the underlying type *T - if obj2, index2, indirect2 := lookupFieldOrMethod(u, pkg, name); obj2 != nil { + if obj2, index2, indirect2 := lookupFieldOrMethod(u, false, pkg, name); obj2 != nil { // only if the result is a field can we keep it if _, ok := obj2.(*Var); ok { return obj2, index2, indirect2 @@ -71,7 +71,12 @@ func LookupFieldOrMethod(T Type, pkg *Package, name string) (obj Object, index [ return } -func lookupFieldOrMethod(T Type, pkg *Package, name string) (obj Object, index []int, indirect bool) { +// TODO(gri) The named type consolidation and seen maps below must be +// indexed by unique keys for a given type. Verify that named +// types always have only one representation (even when imported +// indirectly via different packages.) + +func lookupFieldOrMethod(T Type, addressable bool, pkg *Package, name string) (obj Object, index []int, indirect bool) { // WARNING: The code in this function is extremely subtle - do not modify casually! // This function and NewMethodSet should be kept in sync. @@ -129,8 +134,7 @@ func lookupFieldOrMethod(T Type, pkg *Package, name string) (obj Object, index [ assert(m.typ != nil) index = concat(e.index, i) if obj != nil || e.multiples { - obj = nil // collision - return + return nil, index, false // collision } obj = m indirect = e.indirect @@ -149,8 +153,7 @@ func lookupFieldOrMethod(T Type, pkg *Package, name string) (obj Object, index [ assert(f.typ != nil) index = concat(e.index, i) if obj != nil || e.multiples { - obj = nil // collision - return + return nil, index, false // collision } obj = f indirect = e.indirect @@ -182,8 +185,7 @@ func lookupFieldOrMethod(T Type, pkg *Package, name string) (obj Object, index [ assert(m.typ != nil) index = concat(e.index, i) if obj != nil || e.multiples { - obj = nil // collision - return + return nil, index, false // collision } obj = m indirect = e.indirect @@ -192,15 +194,21 @@ func lookupFieldOrMethod(T Type, pkg *Package, name string) (obj Object, index [ } if obj != nil { - return // found a match + // found a potential match + // spec: "A method call x.m() is valid if the method set of (the type of) x + // contains m and the argument list can be assigned to the parameter + // list of m. If x is addressable and &x's method set contains m, x.m() + // is shorthand for (&x).m()". + if f, _ := obj.(*Func); f != nil && ptrRecv(f) && !indirect && !addressable { + return nil, nil, true // pointer/addressable receiver required + } + return } current = consolidateMultiples(next) } - index = nil - indirect = false - return // not found + return nil, nil, false // not found } // embeddedType represents an embedded named type @@ -270,22 +278,14 @@ func MissingMethod(V Type, T *Interface, static bool) (method *Func, wrongType b // A concrete type implements T if it implements all methods of T. for _, m := range T.allMethods { - obj, _, indirect := lookupFieldOrMethod(V, m.pkg, m.name) - if obj == nil { - return m, false - } + obj, _, _ := lookupFieldOrMethod(V, false, m.pkg, m.name) f, _ := obj.(*Func) if f == nil { return m, false } - // verify that f is in the method set of typ - if !indirect && ptrRecv(f) { - return m, false - } - - if !Identical(obj.Type(), m.typ) { + if !Identical(f.typ, m.typ) { return m, true } } diff --git a/go/types/methodset.go b/go/types/methodset.go index 77297ea0..8aff6f9b 100644 --- a/go/types/methodset.go +++ b/go/types/methodset.go @@ -242,6 +242,10 @@ func (s methodSet) add(list []*Func, index []int, indirect bool, multiples bool) key := f.Id() // if f is not in the set, add it if !multiples { + // TODO(gri) A found method may not be added because it's not in the method set + // (!indirect && ptrRecv(f)). A 2nd method on the same level may be in the method + // set and may not collide with the first one, thus leading to a false positive. + // Is that possible? Investigate. if _, found := s[key]; !found && (indirect || !ptrRecv(f)) { s[key] = &Selection{MethodVal, nil, f, concat(index, i), indirect} continue diff --git a/refactor/eg/match.go b/refactor/eg/match.go index 88d5ade9..b1c0cf3c 100644 --- a/refactor/eg/match.go +++ b/refactor/eg/match.go @@ -166,7 +166,7 @@ func (tr *Transformer) matchSelectorExpr(x, y *ast.SelectorExpr) bool { if xobj, ok := tr.wildcardObj(x.X); ok { field := x.Sel.Name yt := tr.info.TypeOf(y.X) - o, _, _ := types.LookupFieldOrMethod(yt, tr.currentPkg, field) + o, _, _ := types.LookupFieldOrMethod(yt, true, tr.currentPkg, field) if o != nil { tr.env[xobj.Name()] = y.X // record binding return true