diff --git a/cmd/gorename/main.go b/cmd/gorename/main.go index b2ce7b5b..c321432a 100644 --- a/cmd/gorename/main.go +++ b/cmd/gorename/main.go @@ -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: diff --git a/refactor/rename/check.go b/refactor/rename/check.go index dc25b5cc..e028e969 100644 --- a/refactor/rename/check.go +++ b/refactor/rename/check.go @@ -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 { diff --git a/refactor/rename/rename.go b/refactor/rename/rename.go index f03bd10a..907cf74b 100644 --- a/refactor/rename/rename.go +++ b/refactor/rename/rename.go @@ -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. diff --git a/refactor/rename/rename_test.go b/refactor/rename/rename_test.go index a4169b3d..4bad4cc6 100644 --- a/refactor/rename/rename_test.go +++ b/refactor/rename/rename_test.go @@ -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) `, }, }, diff --git a/refactor/satisfy/find.go b/refactor/satisfy/find.go index 8671cc58..a54dc706 100644 --- a/refactor/satisfy/find.go +++ b/refactor/satisfy/find.go @@ -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: