From d98519c114953aa372c373aa4c7122a39d7932af Mon Sep 17 00:00:00 2001 From: Max Bowsher Date: Sun, 23 Jul 2023 10:27:23 +0100 Subject: [PATCH] x/exp/apidiff: add package path to change messages Following the recent change to add module support to apidiff, it is now no longer the case that all objects reported on in one apidiff execution are in the same package. Therefore, add the package path, made relative to the root of the apidiff execution, to object names when reporting changes. Fixes golang/go#61387 Change-Id: Idd748d46b68c0122f9f28ea3b51cc0519448b672 Reviewed-on: https://go-review.googlesource.com/c/exp/+/512295 TryBot-Result: Gopher Robot Reviewed-by: Jonathan Amsterdam Run-TryBot: Jonathan Amsterdam Reviewed-by: Ian Lance Taylor --- apidiff/apidiff.go | 53 ++++++++++++++--------- apidiff/apidiff_test.go | 4 +- apidiff/compatibility.go | 32 +++++++------- apidiff/messageset.go | 48 ++++++++++++++++---- cmd/gorelease/testdata/private/break.test | 2 +- 5 files changed, 91 insertions(+), 48 deletions(-) diff --git a/apidiff/apidiff.go b/apidiff/apidiff.go index 5132a588a..3b6a9dfd9 100644 --- a/apidiff/apidiff.go +++ b/apidiff/apidiff.go @@ -22,13 +22,24 @@ import ( // It classifies each difference as either compatible or incompatible (breaking.) For // a detailed discussion of what constitutes an incompatible change, see the README. func Changes(old, new *types.Package) Report { + return changesInternal(old, new, old.Path(), new.Path()) +} + +// changesInternal contains the core logic for comparing a single package, shared +// between Changes and ModuleChanges. The root package path arguments refer to the +// context of this apidiff invocation - when diffing a single package, they will be +// that package, but when diffing a whole module, they will be the root path of the +// module. This is used to give change messages appropriate context for object names. +// The old and new root must be tracked independently, since each side of the diff +// operation may be a different path. +func changesInternal(old, new *types.Package, oldRootPackagePath, newRootPackagePath string) Report { d := newDiffer(old, new) - d.checkPackage() + d.checkPackage(oldRootPackagePath) r := Report{} - for _, m := range d.incompatibles.collect() { + for _, m := range d.incompatibles.collect(oldRootPackagePath, newRootPackagePath) { r.Changes = append(r.Changes, Change{Message: m, Compatible: false}) } - for _, m := range d.compatibles.collect() { + for _, m := range d.compatibles.collect(oldRootPackagePath, newRootPackagePath) { r.Changes = append(r.Changes, Change{Message: m, Compatible: true}) } return r @@ -54,7 +65,7 @@ func ModuleChanges(old, new *Module) Report { for n, op := range oldPkgs { if np, ok := newPkgs[n]; ok { // shared package, compare surfaces - rr := Changes(op, np) + rr := changesInternal(op, np, old.Path, new.Path) r.Changes = append(r.Changes, rr.Changes...) } else { // old package was removed @@ -114,19 +125,19 @@ func newDiffer(old, new *types.Package) *differ { } } -func (d *differ) incompatible(obj types.Object, part, format string, args ...interface{}) { +func (d *differ) incompatible(obj objectWithSide, part, format string, args ...interface{}) { addMessage(d.incompatibles, obj, part, format, args) } -func (d *differ) compatible(obj types.Object, part, format string, args ...interface{}) { +func (d *differ) compatible(obj objectWithSide, part, format string, args ...interface{}) { addMessage(d.compatibles, obj, part, format, args) } -func addMessage(ms messageSet, obj types.Object, part, format string, args []interface{}) { +func addMessage(ms messageSet, obj objectWithSide, part, format string, args []interface{}) { ms.add(obj, part, fmt.Sprintf(format, args...)) } -func (d *differ) checkPackage() { +func (d *differ) checkPackage(oldRootPackagePath string) { // Old changes. for _, name := range d.old.Scope().Names() { oldobj := d.old.Scope().Lookup(name) @@ -135,7 +146,7 @@ func (d *differ) checkPackage() { } newobj := d.new.Scope().Lookup(name) if newobj == nil { - d.incompatible(oldobj, "", "removed") + d.incompatible(objectWithSide{oldobj, false}, "", "removed") continue } d.checkObjects(oldobj, newobj) @@ -144,7 +155,7 @@ func (d *differ) checkPackage() { for _, name := range d.new.Scope().Names() { newobj := d.new.Scope().Lookup(name) if newobj.Exported() && d.old.Scope().Lookup(name) == nil { - d.compatible(newobj, "", "added") + d.compatible(objectWithSide{newobj, true}, "", "added") } } @@ -168,7 +179,7 @@ func (d *differ) checkPackage() { continue } if types.Implements(otn2.Type(), oIface) && !types.Implements(nt2, nIface) { - d.incompatible(otn2, "", "no longer implements %s", objectString(otn1)) + d.incompatible(objectWithSide{otn2, false}, "", "no longer implements %s", objectString(otn1, oldRootPackagePath)) } } } @@ -183,30 +194,30 @@ func (d *differ) checkObjects(old, new types.Object) { } case *types.Var: if new, ok := new.(*types.Var); ok { - d.checkCorrespondence(old, "", old.Type(), new.Type()) + d.checkCorrespondence(objectWithSide{old, false}, "", old.Type(), new.Type()) return } case *types.Func: switch new := new.(type) { case *types.Func: - d.checkCorrespondence(old, "", old.Type(), new.Type()) + d.checkCorrespondence(objectWithSide{old, false}, "", old.Type(), new.Type()) return case *types.Var: - d.compatible(old, "", "changed from func to var") - d.checkCorrespondence(old, "", old.Type(), new.Type()) + d.compatible(objectWithSide{old, false}, "", "changed from func to var") + d.checkCorrespondence(objectWithSide{old, false}, "", old.Type(), new.Type()) return } case *types.TypeName: if new, ok := new.(*types.TypeName); ok { - d.checkCorrespondence(old, "", old.Type(), new.Type()) + d.checkCorrespondence(objectWithSide{old, false}, "", old.Type(), new.Type()) return } default: panic("unexpected obj type") } // Here if kind of type changed. - d.incompatible(old, "", "changed from %s to %s", + d.incompatible(objectWithSide{old, false}, "", "changed from %s to %s", objectKindString(old), objectKindString(new)) } @@ -216,13 +227,13 @@ func (d *differ) constChanges(old, new *types.Const) { nt := new.Type() // Check for change of type. if !d.correspond(ot, nt) { - d.typeChanged(old, "", ot, nt) + d.typeChanged(objectWithSide{old, false}, "", ot, nt) return } // Check for change of value. // We know the types are the same, so constant.Compare shouldn't panic. if !constant.Compare(old.Val(), token.EQL, new.Val()) { - d.incompatible(old, "", "value changed from %s to %s", old.Val(), new.Val()) + d.incompatible(objectWithSide{old, false}, "", "value changed from %s to %s", old.Val(), new.Val()) } } @@ -241,13 +252,13 @@ func objectKindString(obj types.Object) string { } } -func (d *differ) checkCorrespondence(obj types.Object, part string, old, new types.Type) { +func (d *differ) checkCorrespondence(obj objectWithSide, part string, old, new types.Type) { if !d.correspond(old, new) { d.typeChanged(obj, part, old, new) } } -func (d *differ) typeChanged(obj types.Object, part string, old, new types.Type) { +func (d *differ) typeChanged(obj objectWithSide, part string, old, new types.Type) { old = removeNamesFromSignature(old) new = removeNamesFromSignature(new) olds := types.TypeString(old, types.RelativeTo(d.old)) diff --git a/apidiff/apidiff_test.go b/apidiff/apidiff_test.go index 86e7955e1..e54efd6ab 100644 --- a/apidiff/apidiff_test.go +++ b/apidiff/apidiff_test.go @@ -53,7 +53,7 @@ func testModuleChanges(t *testing.T, x packagestest.Exporter) { t.Fatal("expected some changes, but got none") } wanti := []string{ - "Version: value changed from 1 to 2", + "./foo.Version: value changed from 1 to 2", "package example.com/moda/foo/baz: removed", } sort.Strings(wanti) @@ -66,7 +66,7 @@ func testModuleChanges(t *testing.T, x packagestest.Exporter) { } wantc := []string{ - "Other: added", + "./foo.Other: added", "package example.com/modb/bar: added", } sort.Strings(wantc) diff --git a/apidiff/compatibility.go b/apidiff/compatibility.go index 44238fbd2..3c7ff8b29 100644 --- a/apidiff/compatibility.go +++ b/apidiff/compatibility.go @@ -16,7 +16,7 @@ func (d *differ) checkCompatible(otn *types.TypeName, old, new types.Type) { case *types.Struct: if new, ok := new.(*types.Struct); ok { - d.checkCompatibleStruct(otn, old, new) + d.checkCompatibleStruct(objectWithSide{otn, false}, old, new) return } @@ -36,21 +36,21 @@ func (d *differ) checkCompatible(otn *types.TypeName, old, new types.Type) { panic("unreachable") default: - d.checkCorrespondence(otn, "", old, new) + d.checkCorrespondence(objectWithSide{otn, false}, "", old, new) return } // Here if old and new are different kinds of types. - d.typeChanged(otn, "", old, new) + d.typeChanged(objectWithSide{otn, false}, "", old, new) } func (d *differ) checkCompatibleChan(otn *types.TypeName, old, new *types.Chan) { - d.checkCorrespondence(otn, ", element type", old.Elem(), new.Elem()) + d.checkCorrespondence(objectWithSide{otn, false}, ", element type", old.Elem(), new.Elem()) if old.Dir() != new.Dir() { if new.Dir() == types.SendRecv { - d.compatible(otn, "", "removed direction") + d.compatible(objectWithSide{otn, false}, "", "removed direction") } else { - d.incompatible(otn, "", "changed direction") + d.incompatible(objectWithSide{otn, false}, "", "changed direction") } } } @@ -63,9 +63,9 @@ func (d *differ) checkCompatibleBasic(otn *types.TypeName, old, new *types.Basic return } if compatibleBasics[[2]types.BasicKind{old.Kind(), new.Kind()}] { - d.compatible(otn, "", "changed from %s to %s", old, new) + d.compatible(objectWithSide{otn, false}, "", "changed from %s to %s", old, new) } else { - d.typeChanged(otn, "", old, new) + d.typeChanged(objectWithSide{otn, false}, "", old, new) } } @@ -118,7 +118,7 @@ func (d *differ) checkCompatibleInterface(otn *types.TypeName, old, new *types.I // Perform an equivalence check, but with more information. d.checkMethodSet(otn, old, new, additionsIncompatible) if u := unexportedMethod(new); u != nil { - d.incompatible(otn, u.Name(), "added unexported method") + d.incompatible(objectWithSide{otn, false}, u.Name(), "added unexported method") } } } @@ -150,7 +150,7 @@ func unexportedMethod(t *types.Interface) *types.Func { // struct. // // Field tags are ignored: they have no compile-time implications. -func (d *differ) checkCompatibleStruct(obj types.Object, old, new *types.Struct) { +func (d *differ) checkCompatibleStruct(obj objectWithSide, old, new *types.Struct) { d.checkCompatibleObjectSets(obj, exportedFields(old), exportedFields(new)) d.checkCompatibleObjectSets(obj, exportedSelectableFields(old), exportedSelectableFields(new)) // Removing comparability from a struct is an incompatible change. @@ -242,7 +242,7 @@ func unambiguousFields(structs []*types.Struct) map[string]*types.Var { // Anything removed or change from the old set is an incompatible change. // Anything added to the new set is a compatible change. -func (d *differ) checkCompatibleObjectSets(obj types.Object, old, new map[string]types.Object) { +func (d *differ) checkCompatibleObjectSets(obj objectWithSide, old, new map[string]types.Object) { for name, oldo := range old { newo := new[name] if newo == nil { @@ -303,16 +303,16 @@ func (d *differ) checkMethodSet(otn *types.TypeName, oldt, newt types.Type, addc if receiverNamedType(oldMethod).Obj() != otn { part = fmt.Sprintf(", method set of %s", msname) } - d.incompatible(oldMethod, part, "removed") + d.incompatible(objectWithSide{oldMethod, false}, part, "removed") } else { - obj := oldMethod + obj := objectWithSide{oldMethod, false} // If a value method is changed to a pointer method and has a signature // change, then we can get two messages for the same method definition: one // for the value method set that says it's removed, and another for the // pointer method set that says it changed. To keep both messages (since // messageSet dedups), use newMethod for the second. (Slight hack.) if !hasPointerReceiver(oldMethod) && hasPointerReceiver(newMethod) { - obj = newMethod + obj = objectWithSide{newMethod, true} } d.checkCorrespondence(obj, "", oldMethod.Type(), newMethod.Type()) } @@ -322,9 +322,9 @@ func (d *differ) checkMethodSet(otn *types.TypeName, oldt, newt types.Type, addc for name, newMethod := range newMethodSet { if oldMethodSet[name] == nil { if addcompat { - d.compatible(newMethod, "", "added") + d.compatible(objectWithSide{newMethod, true}, "", "added") } else { - d.incompatible(newMethod, "", "added") + d.incompatible(objectWithSide{newMethod, true}, "", "added") } } } diff --git a/apidiff/messageset.go b/apidiff/messageset.go index 135479053..633ae583a 100644 --- a/apidiff/messageset.go +++ b/apidiff/messageset.go @@ -10,35 +10,49 @@ import ( "strings" ) +// objectWithSide contains an object, and information on which side (old or new) +// of the comparison it relates to. This matters when need to express the object's +// package path, relative to the root path of the comparison, as the old and new +// sides can have different roots (e.g. comparing somepackage/v2 vs. somepackage/v3). +type objectWithSide struct { + object types.Object + isNew bool +} + // There can be at most one message for each object or part thereof. // Parts include interface methods and struct fields. // // The part thing is necessary. Method (Func) objects have sufficient info, but field // Vars do not: they just have a field name and a type, without the enclosing struct. -type messageSet map[types.Object]map[string]string +type messageSet map[objectWithSide]map[string]string // Add a message for obj and part, overwriting a previous message // (shouldn't happen). // obj is required but part can be empty. -func (m messageSet) add(obj types.Object, part, msg string) { +func (m messageSet) add(obj objectWithSide, part, msg string) { s := m[obj] if s == nil { s = map[string]string{} m[obj] = s } if f, ok := s[part]; ok && f != msg { - fmt.Printf("! second, different message for obj %s, part %q\n", obj, part) + fmt.Printf("! second, different message for obj %s, isNew %v, part %q\n", obj.object, obj.isNew, part) fmt.Printf(" first: %s\n", f) fmt.Printf(" second: %s\n", msg) } s[part] = msg } -func (m messageSet) collect() []string { +func (m messageSet) collect(oldRootPackagePath, newRootPackagePath string) []string { var s []string for obj, parts := range m { + rootPackagePath := oldRootPackagePath + if obj.isNew { + rootPackagePath = newRootPackagePath + } + // Format each object name relative to its own package. - objstring := objectString(obj) + objstring := objectString(obj.object, rootPackagePath) for part, msg := range parts { var p string @@ -54,7 +68,25 @@ func (m messageSet) collect() []string { return s } -func objectString(obj types.Object) string { +func objectString(obj types.Object, rootPackagePath string) string { + thisPackagePath := obj.Pkg().Path() + + var packagePrefix string + if thisPackagePath == rootPackagePath { + // obj is in same package as the diff operation root - no prefix + packagePrefix = "" + } else if strings.HasPrefix(thisPackagePath, rootPackagePath+"/") { + // obj is in a child package compared to the diff operation root - use a + // prefix starting with "./" to emphasise the relative nature + packagePrefix = "./" + thisPackagePath[len(rootPackagePath)+1:] + "." + } else { + // obj is outside the diff operation root - display full path. This can + // happen if there is a need to report a change in a type in an unrelated + // package, because it has been used as the underlying type in a type + // definition in the package being processed, for example. + packagePrefix = thisPackagePath + "." + } + if f, ok := obj.(*types.Func); ok { sig := f.Type().(*types.Signature) if recv := sig.Recv(); recv != nil { @@ -62,10 +94,10 @@ func objectString(obj types.Object) string { if tn[0] == '*' { tn = "(" + tn + ")" } - return fmt.Sprintf("%s.%s", tn, obj.Name()) + return fmt.Sprintf("%s%s.%s", packagePrefix, tn, obj.Name()) } } - return obj.Name() + return fmt.Sprintf("%s%s", packagePrefix, obj.Name()) } func dotjoin(s1, s2 string) string { diff --git a/cmd/gorelease/testdata/private/break.test b/cmd/gorelease/testdata/private/break.test index 3e4ec8716..e996a9b5d 100644 --- a/cmd/gorelease/testdata/private/break.test +++ b/cmd/gorelease/testdata/private/break.test @@ -5,7 +5,7 @@ success=false -- want -- # example.com/private/p ## incompatible changes -I.Foo: added +example.com/private/internal/i.I.Foo: added # summary Cannot suggest a release version.