1
0
mirror of https://github.com/golang/go synced 2024-11-18 08:54:45 -07:00

cmd/gorename: support renaming of methods with consequences for other types, iff initiated at an abstract method.

Previously, gorename rejected all method renamings if it would
change the assignability relation.

Now, so long as the renaming was initiated at an abstract
method, the renaming proceeds, changing concrete methods (and
possibly other abstract methods) as needed.  The user
intention is clear.

The intention of a renaming initiated at a concrete method is
less clear, so we still reject it if it would change the
assignability relation.  The diagnostic advises the user to
rename the abstract method if that was the intention.

Additional safety checks are required: for each
satisfy.Constraint that couples a concrete type C and an
interface type I, we must treat it just like a set of implicit
selections C.f, one per abstract method f of I, and ensure the
selections' meanings are unchanged.

The satisfy package no longer canonicalizes types, since this
substitutes one interface for another (equivalent) one, which
is sound, but makes the type names random and the error
messages confusing.

Also, fixed a bug in 'satisfy' relating to map keys.

+ Lots more tests.

LGTM=sameer
R=sameer
CC=golang-codereviews
https://golang.org/cl/173430043
This commit is contained in:
Alan Donovan 2014-12-04 09:37:50 -05:00
parent 7e91bb35f8
commit c6ec5ea66d
5 changed files with 481 additions and 105 deletions

View File

@ -73,11 +73,13 @@ affected. For a local renaming, this is just the package specified by
-from or -offset, but for a potentially exported name, gorename scans
the workspace ($GOROOT and $GOPATH).
gorename rejects renamings of concrete methods that would change the
assignability relation between types and interfaces. If the interface
change was intentional, initiate the renaming at the interface method.
gorename rejects any renaming that would create a conflict at the point
of declaration, or a reference conflict (ambiguity or shadowing), or
anything else that could cause the resulting program not to compile.
Currently, it also rejects any method renaming that would change the
assignability relation between types and interfaces.
Examples:

View File

@ -39,7 +39,7 @@ func (r *renamer) check(from types.Object) {
r.checkInPackageBlock(from)
} else if v, ok := from.(*types.Var); ok && v.IsField() {
r.checkStructField(v)
} else if f, ok := from.(*types.Func); ok && f.Type().(*types.Signature).Recv() != nil {
} else if f, ok := from.(*types.Func); ok && recv(f) != nil {
r.checkMethod(f)
} else if isLocal(from) {
r.checkInLocalScope(from)
@ -430,7 +430,7 @@ func (r *renamer) selectionConflict(from types.Object, delta int, syntax *ast.Se
// analogous to sub-block conflict
r.errorf(syntax.Sel.Pos(),
"\twould change the referent of this selection")
r.errorf(obj.Pos(), "\tto this %s", objectKind(obj))
r.errorf(obj.Pos(), "\tof this %s", objectKind(obj))
case delta == 0:
// analogous to same-block conflict
r.errorf(syntax.Sel.Pos(),
@ -440,7 +440,7 @@ func (r *renamer) selectionConflict(from types.Object, delta int, syntax *ast.Se
// analogous to super-block conflict
r.errorf(syntax.Sel.Pos(),
"\twould shadow this selection")
r.errorf(obj.Pos(), "\tto the %s declared here",
r.errorf(obj.Pos(), "\tof the %s declared here",
objectKind(obj))
}
}
@ -449,7 +449,11 @@ func (r *renamer) selectionConflict(from types.Object, delta int, syntax *ast.Se
// There are three hazards:
// - declaration conflicts
// - selection ambiguity/changes
// - entailed renamings of assignable concrete/interface types (for now, just reject)
// - entailed renamings of assignable concrete/interface types.
// We reject renamings initiated at concrete methods if it would
// change the assignability relation. For renamings of abstract
// methods, we rename all methods transitively coupled to it via
// assignability.
func (r *renamer) checkMethod(from *types.Func) {
// e.g. error.Error
if from.Pkg() == nil {
@ -457,50 +461,20 @@ func (r *renamer) checkMethod(from *types.Func) {
return
}
// As always, having to support concrete methods with pointer
// and non-pointer receivers, and named vs unnamed types with
// methods, makes tooling fun.
// ASSIGNABILITY
//
// For now, if any method renaming breaks a required
// assignability to another type, we reject it.
//
// TODO(adonovan): probably we should compute the entailed
// renamings so that an interface method renaming causes
// concrete methods to change too. But which ones?
//
// There is no correct answer, only heuristics, because Go's
// "duck typing" doesn't distinguish intentional from contingent
// assignability. There are two obvious approaches:
//
// (1) Update the minimum set of types to preserve the
// assignability of types all syntactic assignments
// (incl. implicit ones in calls, returns, sends, etc).
// The satisfy.Finder enumerates these.
// This is likely to be an underapproximation.
//
// (2) Update all types that are assignable to/from the changed
// type. This requires computing the "implements" relation
// for all pairs of types (as godoc and oracle do).
// This is likely to be an overapproximation.
//
// If a concrete type is renamed, we probably do not want to
// rename corresponding interfaces; interface renamings should
// probably be initiated at the interface. (But what if a
// concrete type implements multiple interfaces with the same
// method? Then the user is stuck.)
//
// We need some experience before we decide how to implement this.
// ASSIGNABILITY: We reject renamings of concrete methods that
// would break a 'satisfy' constraint; but renamings of abstract
// methods are allowed to proceed, and we rename affected
// concrete and abstract methods as necessary. It is the
// initial method that determines the policy.
// Check for conflict at point of declaration.
// Check to ensure preservation of assignability requirements.
recv := from.Type().(*types.Signature).Recv().Type()
if isInterface(recv) {
R := recv(from).Type()
if isInterface(R) {
// Abstract method
// declaration
prev, _, _ := types.LookupFieldOrMethod(recv, false, from.Pkg(), r.to)
prev, _, _ := types.LookupFieldOrMethod(R, false, from.Pkg(), r.to)
if prev != nil {
r.errorf(from.Pos(), "renaming this interface method %q to %q",
from.Name(), r.to)
@ -542,30 +516,100 @@ func (r *renamer) checkMethod(from *types.Func) {
}
// assignability
for T := range r.findAssignments(recv) {
if obj, _, _ := types.LookupFieldOrMethod(T, false, from.Pkg(), from.Name()); obj == nil {
//
// Find the set of concrete or abstract methods directly
// coupled to abstract method 'from' by some
// satisfy.Constraint, and rename them too.
for key := range r.satisfy() {
// key = (lhs, rhs) where lhs is always an interface.
lsel := r.msets.MethodSet(key.LHS).Lookup(from.Pkg(), from.Name())
if lsel == nil {
continue
}
rmethods := r.msets.MethodSet(key.RHS)
rsel := rmethods.Lookup(from.Pkg(), from.Name())
if rsel == nil {
continue
}
r.errorf(from.Pos(), "renaming this method %q to %q",
from.Name(), r.to)
var pos token.Pos
var other string
if named, ok := T.(*types.Named); ok {
pos = named.Obj().Pos()
other = named.Obj().Name()
} else {
pos = from.Pos()
other = T.String()
// If both sides have a method of this name,
// and one of them is m, the other must be coupled.
var coupled *types.Func
switch from {
case lsel.Obj():
coupled = rsel.Obj().(*types.Func)
case rsel.Obj():
coupled = lsel.Obj().(*types.Func)
default:
continue
}
r.errorf(pos, "\twould make %s no longer assignable to it", other)
return
// We must treat concrete-to-interface
// constraints like an implicit selection C.f of
// each interface method I.f, and check that the
// renaming leaves the selection unchanged and
// unambiguous.
//
// Fun fact: the implicit selection of C.f
// type I interface{f()}
// type C struct{I}
// func (C) g()
// var _ I = C{} // here
// yields abstract method I.f. This can make error
// messages less than obvious.
//
if !isInterface(key.RHS) {
// The logic below was derived from checkSelections.
rtosel := rmethods.Lookup(from.Pkg(), r.to)
if rtosel != nil {
rto := rtosel.Obj().(*types.Func)
delta := len(rsel.Index()) - len(rtosel.Index())
if delta < 0 {
continue // no ambiguity
}
// TODO(adonovan): record the constraint's position.
keyPos := token.NoPos
r.errorf(from.Pos(), "renaming this method %q to %q",
from.Name(), r.to)
if delta == 0 {
// analogous to same-block conflict
r.errorf(keyPos, "\twould make the %s method of %s invoked via interface %s ambiguous",
r.to, key.RHS, key.LHS)
r.errorf(rto.Pos(), "\twith (%s).%s",
recv(rto).Type(), r.to)
} else {
// analogous to super-block conflict
r.errorf(keyPos, "\twould change the %s method of %s invoked via interface %s",
r.to, key.RHS, key.LHS)
r.errorf(coupled.Pos(), "\tfrom (%s).%s",
recv(coupled).Type(), r.to)
r.errorf(rto.Pos(), "\tto (%s).%s",
recv(rto).Type(), r.to)
}
return // one error is enough
}
}
if !r.changeMethods {
// This should be unreachable.
r.errorf(from.Pos(), "internal error: during renaming of abstract method %s", from)
r.errorf(coupled.Pos(), "\tchangedMethods=false, coupled method=%s", coupled)
r.errorf(from.Pos(), "\tPlease file a bug report")
return
}
// Rename the coupled method to preserve assignability.
r.check(coupled)
}
} else {
// Concrete method
// declaration
prev, indices, _ := types.LookupFieldOrMethod(recv, true, from.Pkg(), r.to)
prev, indices, _ := types.LookupFieldOrMethod(R, true, from.Pkg(), r.to)
if prev != nil && len(indices) == 1 {
r.errorf(from.Pos(), "renaming this method %q to %q",
from.Name(), r.to)
@ -574,17 +618,44 @@ func (r *renamer) checkMethod(from *types.Func) {
return
}
// assignability (of both T and *T)
recvBase := deref(recv)
for _, R := range []types.Type{recvBase, types.NewPointer(recvBase)} {
for I := range r.findAssignments(R) {
if obj, _, _ := types.LookupFieldOrMethod(I, true, from.Pkg(), from.Name()); obj == nil {
continue
}
// assignability
//
// Find the set of abstract methods coupled to concrete
// method 'from' by some satisfy.Constraint, and rename
// them too.
//
// Coupling may be indirect, e.g. I.f <-> C.f via type D.
//
// type I interface {f()}
// type C int
// type (C) f()
// type D struct{C}
// var _ I = D{}
//
for key := range r.satisfy() {
// key = (lhs, rhs) where lhs is always an interface.
if isInterface(key.RHS) {
continue
}
rsel := r.msets.MethodSet(key.RHS).Lookup(from.Pkg(), from.Name())
if rsel == nil || rsel.Obj() != from {
continue // rhs does not have the method
}
lsel := r.msets.MethodSet(key.LHS).Lookup(from.Pkg(), from.Name())
if lsel == nil {
continue
}
imeth := lsel.Obj().(*types.Func)
// imeth is the abstract method (e.g. I.f)
// and key.RHS is the concrete coupling type (e.g. D).
if !r.changeMethods {
r.errorf(from.Pos(), "renaming this method %q to %q",
from.Name(), r.to)
var pos token.Pos
var iface string
I := recv(imeth).Type()
if named, ok := I.(*types.Named); ok {
pos = named.Obj().Pos()
iface = "interface " + named.Obj().Name()
@ -592,9 +663,15 @@ func (r *renamer) checkMethod(from *types.Func) {
pos = from.Pos()
iface = I.String()
}
r.errorf(pos, "\twould make it no longer assignable to %s", iface)
return // one is enough
r.errorf(pos, "\twould make %s no longer assignable to %s",
key.RHS, iface)
r.errorf(imeth.Pos(), "\t(rename %s.%s if you intend to change both types)",
I, from.Name())
return // one error is enough
}
// Rename the coupled interface method to preserve assignability.
r.check(imeth)
}
}
@ -618,9 +695,8 @@ func (r *renamer) checkExport(id *ast.Ident, pkg *types.Package, from types.Obje
return true
}
// findAssignments returns the set of types to or from which type T is
// assigned in the program syntax.
func (r *renamer) findAssignments(T types.Type) map[types.Type]bool {
// satisfy returns the set of interface satisfaction constraints.
func (r *renamer) satisfy() map[satisfy.Constraint]bool {
if r.satisfyConstraints == nil {
// Compute on demand: it's expensive.
var f satisfy.Finder
@ -629,23 +705,16 @@ func (r *renamer) findAssignments(T types.Type) map[types.Type]bool {
}
r.satisfyConstraints = f.Result
}
result := make(map[types.Type]bool)
for key := range r.satisfyConstraints {
// key = (lhs, rhs) where lhs is always an interface.
if types.Identical(key.RHS, T) {
result[key.LHS] = true
}
if isInterface(T) && types.Identical(key.LHS, T) {
// must check both sides
result[key.RHS] = true
}
}
return result
return r.satisfyConstraints
}
// -- helpers ----------------------------------------------------------
// recv returns the method's receiver.
func recv(meth *types.Func) *types.Var {
return meth.Type().(*types.Signature).Recv()
}
// someUse returns an arbitrary use of obj within info.
func someUse(info *loader.PackageInfo, obj types.Object) *ast.Ident {
for id, o := range info.Uses {

View File

@ -50,6 +50,8 @@ type renamer struct {
to string
satisfyConstraints map[satisfy.Constraint]bool
packages map[*types.Package]*loader.PackageInfo // subset of iprog.AllPackages to inspect
msets types.MethodSetCache
changeMethods bool
}
var reportError = func(posn token.Position, message string) {
@ -151,6 +153,19 @@ func Main(ctxt *build.Context, offsetFlag, fromFlag, to string) error {
packages: make(map[*types.Package]*loader.PackageInfo),
}
// A renaming initiated at an interface method indicates the
// intention to rename abstract and concrete methods as needed
// to preserve assignability.
for _, obj := range fromObjects {
if obj, ok := obj.(*types.Func); ok {
recv := obj.Type().(*types.Signature).Recv()
if recv != nil && isInterface(recv.Type().Underlying()) {
r.changeMethods = true
break
}
}
}
// Only the initially imported packages (iprog.Imported) and
// their external tests (iprog.Created) should be inspected or
// modified, as only they have type-checked functions bodies.

View File

@ -241,14 +241,14 @@ func f() {
from: "(main.U).u", to: "w",
want: `renaming this field "u" to "w".*` +
`would change the referent of this selection.*` +
`to this field`,
`of this field`,
},
{
// field/field shadowing at different promotion levels ('to' selection)
from: "(main.W).w", to: "u",
want: `renaming this field "w" to "u".*` +
`would shadow this selection.*` +
`to the field declared here`,
`of the field declared here`,
},
{
from: "(main.V).v", to: "w",
@ -323,17 +323,65 @@ var _ interface {f()} = C(0)
{
from: "(main.C).f", to: "e",
want: `renaming this method "f" to "e".*` +
`would make it no longer assignable to interface{f..}`,
`would make main.C no longer assignable to interface{f..}.*` +
`(rename interface{f..}.f if you intend to change both types)`,
},
{
from: "(main.D).g", to: "e",
want: `renaming this method "g" to "e".*` +
`would make it no longer assignable to interface I`,
`would make \*main.D no longer assignable to interface I.*` +
`(rename main.I.g if you intend to change both types)`,
},
{
from: "(main.I).f", to: "e",
want: `renaming this method "f" to "e".*` +
`would make \*main.D no longer assignable to it`,
want: `OK`,
},
// Indirect C/I method coupling via another concrete type D.
{
ctxt: main(`
package main
type I interface { f() }
type C int
func (C) f()
type D struct{C}
var _ I = D{}
`),
from: "(main.C).f", to: "F",
want: `renaming this method "f" to "F".*` +
`would make main.D no longer assignable to interface I.*` +
`(rename main.I.f if you intend to change both types)`,
},
// Renaming causes promoted method to become shadowed; C no longer satisfies I.
{
ctxt: main(`
package main
type I interface { f() }
type C struct { I }
func (C) g() int
var _ I = C{}
`),
from: "main.I.f", to: "g",
want: `renaming this method "f" to "g".*` +
`would change the g method of main.C invoked via interface main.I.*` +
`from \(main.I\).g.*` +
`to \(main.C\).g`,
},
// Renaming causes promoted method to become ambiguous; C no longer satisfies I.
{
ctxt: main(`
package main
type I interface{f()}
type C int
func (C) f()
type D int
func (D) g()
type E struct{C;D}
var _ I = E{}
`),
from: "main.I.f", to: "g",
want: `renaming this method "f" to "g".*` +
`would make the g method of main.E invoked via interface main.I ambiguous.*` +
`with \(main.D\).g`,
},
} {
var conflicts []string
@ -672,6 +720,257 @@ type U int
import "foo"
type _ struct{ *foo.U }
`,
},
},
// Interface method renaming.
{
ctxt: fakeContext(map[string][]string{
"main": {`
package main
type I interface { f() }
type J interface { f(); g() }
type A int
func (A) f()
type B int
func (B) f()
func (B) g()
type C int
func (C) f()
func (C) g()
var _, _ I = A(0), B(0)
var _, _ J = B(0), C(0)
`,
},
}),
offset: "/go/src/main/0.go:#33", to: "F", // abstract method I.f
want: map[string]string{
"/go/src/main/0.go": `package main
type I interface {
F()
}
type J interface {
F()
g()
}
type A int
func (A) F()
type B int
func (B) F()
func (B) g()
type C int
func (C) F()
func (C) g()
var _, _ I = A(0), B(0)
var _, _ J = B(0), C(0)
`,
},
},
{
offset: "/go/src/main/0.go:#58", to: "F", // abstract method J.f
want: map[string]string{
"/go/src/main/0.go": `package main
type I interface {
F()
}
type J interface {
F()
g()
}
type A int
func (A) F()
type B int
func (B) F()
func (B) g()
type C int
func (C) F()
func (C) g()
var _, _ I = A(0), B(0)
var _, _ J = B(0), C(0)
`,
},
},
{
offset: "/go/src/main/0.go:#63", to: "G", // abstract method J.g
want: map[string]string{
"/go/src/main/0.go": `package main
type I interface {
f()
}
type J interface {
f()
G()
}
type A int
func (A) f()
type B int
func (B) f()
func (B) G()
type C int
func (C) f()
func (C) G()
var _, _ I = A(0), B(0)
var _, _ J = B(0), C(0)
`,
},
},
// Indirect coupling of I.f to C.f from D->I assignment and anonymous field of D.
{
ctxt: fakeContext(map[string][]string{
"main": {`
package main
type I interface { f() }
type C int
func (C) f()
type D struct{C}
var _ I = D{}
`,
},
}),
offset: "/go/src/main/0.go:#33", to: "F", // abstract method I.f
want: map[string]string{
"/go/src/main/0.go": `package main
type I interface {
F()
}
type C int
func (C) F()
type D struct{ C }
var _ I = D{}
`,
},
},
// Interface embedded in struct. No conflict if C need not satisfy I.
{
ctxt: fakeContext(map[string][]string{
"main": {`
package main
type I interface {f()}
type C struct{I}
func (C) g() int
var _ int = C{}.g()
`,
},
}),
offset: "/go/src/main/0.go:#32", to: "g", // abstract method I.f
want: map[string]string{
"/go/src/main/0.go": `package main
type I interface {
g()
}
type C struct{ I }
func (C) g() int
var _ int = C{}.g()
`,
},
},
// A type assertion causes method coupling iff signatures match.
{
ctxt: fakeContext(map[string][]string{
"main": {`package main
type I interface{f()}
type J interface{f()}
var _ = I(nil).(J)
`,
},
}),
offset: "/go/src/main/0.go:#30", to: "g", // abstract method I.f
want: map[string]string{
"/go/src/main/0.go": `package main
type I interface {
g()
}
type J interface {
g()
}
var _ = I(nil).(J)
`,
},
},
// Impossible type assertion: no method coupling.
{
ctxt: fakeContext(map[string][]string{
"main": {`package main
type I interface{f()}
type J interface{f()int}
var _ = I(nil).(J)
`,
},
}),
offset: "/go/src/main/0.go:#30", to: "g", // abstract method I.f
want: map[string]string{
"/go/src/main/0.go": `package main
type I interface {
g()
}
type J interface {
f() int
}
var _ = I(nil).(J)
`,
},
},
// Impossible type assertion: no method coupling C.f<->J.f.
{
ctxt: fakeContext(map[string][]string{
"main": {`package main
type I interface{f()}
type C int
func (C) f()
type J interface{f()int}
var _ = I(C(0)).(J)
`,
},
}),
offset: "/go/src/main/0.go:#30", to: "g", // abstract method I.f
want: map[string]string{
"/go/src/main/0.go": `package main
type I interface {
g()
}
type C int
func (C) g()
type J interface {
f() int
}
var _ = I(C(0)).(J)
`,
},
},

View File

@ -50,7 +50,6 @@ import (
"go/token"
"golang.org/x/tools/go/types"
"golang.org/x/tools/go/types/typeutil"
)
// A Constraint records the fact that the RHS type does and must
@ -72,7 +71,6 @@ type Constraint struct {
type Finder struct {
Result map[Constraint]bool
msetcache types.MethodSetCache
canon typeutil.Map // maps types to canonical type
// per-Find state
info *types.Info
@ -82,6 +80,9 @@ type Finder struct {
// Find inspects a single package, populating Result with its pairs of
// constrained types.
//
// The result is non-canonical and thus may contain duplicates (but this
// tends to preserves names of interface types better).
//
// The package must be free of type errors, and
// info.{Defs,Uses,Selections,Types} must have been populated by the
// type-checker.
@ -281,25 +282,15 @@ func (f *Finder) assign(lhs, rhs types.Type) {
if !isInterface(lhs) {
return
}
if f.msetcache.MethodSet(lhs).Len() == 0 {
return
}
if f.msetcache.MethodSet(rhs).Len() == 0 {
return
}
// canonicalize types
lhsc, ok := f.canon.At(lhs).(types.Type)
if !ok {
lhsc = lhs
f.canon.Set(lhs, lhsc)
}
rhsc, ok := f.canon.At(rhs).(types.Type)
if !ok {
rhsc = rhs
f.canon.Set(rhs, rhsc)
}
// record the pair
f.Result[Constraint{lhsc, rhsc}] = true
f.Result[Constraint{lhs, rhs}] = true
}
// typeAssert must be called for each type assertion x.(T) where x has
@ -417,7 +408,7 @@ func (f *Finder) expr(e ast.Expr) types.Type {
x := f.expr(e.X)
i := f.expr(e.Index)
if ux, ok := x.Underlying().(*types.Map); ok {
f.assign(ux.Elem(), i)
f.assign(ux.Key(), i)
}
case *ast.SliceExpr: