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 <[email protected]>
Reviewed-by: Jonathan Amsterdam <[email protected]>
Run-TryBot: Jonathan Amsterdam <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>

Author: maxb

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))

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)

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")
 			}
 		}
 	}

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,18 +68,36 @@ 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 {
 			tn := types.TypeString(recv.Type(), types.RelativeTo(obj.Pkg()))
 			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 {

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.