From a07a8442513904dd32d078d1ef5418ededb4b620 Mon Sep 17 00:00:00 2001 From: Jonathan Amsterdam Date: Tue, 25 Jul 2023 16:37:03 -0400 Subject: [PATCH] apidiff: support instantiated generic types Use types.Types as keys in the map of correspondences, rather than *types.TypeNames. There was no reason not do this before, except convenience; prior to generics, named types were the same iff their TypeNames were. Also, update the definition of correspondence to handle instantiated generic types, by checking that their TypeArg lists and origins correspond. Also, we now don't need the hack that was typesEquivalent. We can always compare two types from the same world with types.Identical. Change-Id: Ic5c7bb0be052376367c84da8680ca86b4a1dafc4 Reviewed-on: https://go-review.googlesource.com/c/exp/+/513136 TryBot-Result: Gopher Robot Run-TryBot: Jonathan Amsterdam Reviewed-by: Robert Findley --- apidiff/apidiff.go | 27 ++++++++---- apidiff/correspondence.go | 84 ++++++++++++++++-------------------- apidiff/testdata/generics.go | 34 +++++++++++++++ 3 files changed, 88 insertions(+), 57 deletions(-) diff --git a/apidiff/apidiff.go b/apidiff/apidiff.go index 3b6a9dfd9..92c24fee6 100644 --- a/apidiff/apidiff.go +++ b/apidiff/apidiff.go @@ -16,6 +16,8 @@ import ( "go/token" "go/types" "strings" + + "golang.org/x/tools/go/types/typeutil" ) // Changes reports on the differences between the APIs of the old and new packages. @@ -108,7 +110,7 @@ type differ struct { // Even though it is the named types (*types.Named) that correspond, we use // *types.TypeName as a map key because they are canonical. // The values can be either named types or basic types. - correspondMap map[*types.TypeName]types.Type + correspondMap typeutil.Map // Messages. incompatibles messageSet @@ -119,7 +121,6 @@ func newDiffer(old, new *types.Package) *differ { return &differ{ old: old, new: new, - correspondMap: map[*types.TypeName]types.Type{}, incompatibles: messageSet{}, compatibles: messageSet{}, } @@ -161,28 +162,36 @@ func (d *differ) checkPackage(oldRootPackagePath string) { // Whole-package satisfaction. // For every old exposed interface oIface and its corresponding new interface nIface... - for otn1, nt1 := range d.correspondMap { + d.correspondMap.Iterate(func(k1 types.Type, v1 any) { + ot1 := k1.(*types.Named) + otn1 := ot1.Obj() + nt1 := v1.(types.Type) oIface, ok := otn1.Type().Underlying().(*types.Interface) if !ok { - continue + return } nIface, ok := nt1.Underlying().(*types.Interface) if !ok { // If nt1 isn't an interface but otn1 is, then that's an incompatibility that // we've already noticed, so there's no need to do anything here. - continue + return } // For every old type that implements oIface, its corresponding new type must implement // nIface. - for otn2, nt2 := range d.correspondMap { + d.correspondMap.Iterate(func(k2 types.Type, v2 any) { + ot2 := k2.(*types.Named) + otn2 := ot2.Obj() + nt2 := v2.(types.Type) if otn1 == otn2 { - continue + return } if types.Implements(otn2.Type(), oIface) && !types.Implements(nt2, nIface) { + // TODO(jba): the type name is not sufficient information here; we need the type args + // if this is an instantiated generic type. d.incompatible(objectWithSide{otn2, false}, "", "no longer implements %s", objectString(otn1, oldRootPackagePath)) } - } - } + }) + }) } func (d *differ) checkObjects(old, new types.Object) { diff --git a/apidiff/correspondence.go b/apidiff/correspondence.go index 4ef96f83f..f92b2891c 100644 --- a/apidiff/correspondence.go +++ b/apidiff/correspondence.go @@ -124,7 +124,6 @@ func (d *differ) corr(old, new types.Type, p *ifacePair) bool { } if new, ok := new.(*types.Basic); ok { // Basic types are defined types, too, so we have to support them. - return d.establishCorrespondence(old, new) } @@ -167,8 +166,8 @@ func (d *differ) establishCorrespondence(old *types.Named, new types.Type) bool oldname := old.Obj() // If there already is a corresponding new type for old, check that they // are the same. - if c := d.correspondMap[oldname]; c != nil { - return typesEquivalent(c, new) + if c := d.correspondMap.At(old); c != nil { + return types.Identical(c.(types.Type), new) } // Attempt to establish a correspondence. // Assume the types don't correspond unless they have the same @@ -196,69 +195,58 @@ func (d *differ) establishCorrespondence(old *types.Named, new types.Type) bool if old.Obj().Pkg() != d.old || newn.Obj().Pkg() != d.new { return old.Obj().Id() == newn.Obj().Id() } - // Prior to generics, any two named types could correspond. - // Two named types cannot correspond if their type parameter lists don't match. - if !d.typeParamListsCorrespond(old.TypeParams(), newn.TypeParams()) { - return false + // Two generic named types correspond if their type parameter lists correspond. + // Since one or the other of those lists will be empty, it doesn't hurt + // to check both. + oldOrigin := old.Origin() + newOrigin := newn.Origin() + if oldOrigin != old { + // old is an instantiated type. + if newOrigin == newn { + // new is not; they cannot correspond. + return false + } + // Two instantiated types correspond if their origins correspond and + // their type argument lists correspond. + if !d.correspond(oldOrigin, newOrigin) { + return false + } + if !d.typeListsCorrespond(old.TypeArgs(), newn.TypeArgs()) { + return false + } + } else { + if !d.typeParamListsCorrespond(old.TypeParams(), newn.TypeParams()) { + return false + } } } // If there is no correspondence, create one. - d.correspondMap[oldname] = new + d.correspondMap.Set(old, new) // Check that the corresponding types are compatible. d.checkCompatibleDefined(oldname, old, new) return true } -// Two list of type parameters correspond if they are the same length, and -// the constraints of corresponding type parameters correspond. -func (d *differ) typeParamListsCorrespond(tps1, tps2 *types.TypeParamList) bool { - if tps1.Len() != tps2.Len() { +func (d *differ) typeListsCorrespond(tl1, tl2 *types.TypeList) bool { + if tl1.Len() != tl2.Len() { return false } - for i := 0; i < tps1.Len(); i++ { - if !d.correspond(tps1.At(i).Constraint(), tps2.At(i).Constraint()) { + for i := 0; i < tl1.Len(); i++ { + if !d.correspond(tl1.At(i), tl2.At(i)) { return false } } return true } -// typesEquivalent reports whether two types are identical, or if -// the types have identical type param lists except that one type has nil -// constraints. -// -// This allows us to match a Type from a method receiver or arg to the Type from -// the declaration. -func typesEquivalent(t1, t2 types.Type) bool { - if types.Identical(t1, t2) { - return true - } - // Handle two types with the same type params, one - // having constraints and one not. - oldn, ok := t1.(*types.Named) - if !ok { - return false - } - newn, ok := t2.(*types.Named) - if !ok { - return false - } - oldps := oldn.TypeParams() - newps := newn.TypeParams() - if oldps.Len() != newps.Len() { - return false - } - if oldps.Len() == 0 { - // Not generic types. +// Two list of type parameters correspond if they are the same length, and +// the constraints of corresponding type parameters correspond. +func (d *differ) typeParamListsCorrespond(tps1, tps2 *types.TypeParamList) bool { + if tps1.Len() != tps2.Len() { return false } - for i := 0; i < oldps.Len(); i++ { - oldp := oldps.At(i) - newp := newps.At(i) - if oldp.Constraint() == nil || newp.Constraint() == nil { - return true - } - if !types.Identical(oldp.Constraint(), newp.Constraint()) { + for i := 0; i < tps1.Len(); i++ { + if !d.correspond(tps1.At(i).Constraint(), tps2.At(i).Constraint()) { return false } } diff --git a/apidiff/testdata/generics.go b/apidiff/testdata/generics.go index 7dc096e26..12e1c780e 100644 --- a/apidiff/testdata/generics.go +++ b/apidiff/testdata/generics.go @@ -48,3 +48,37 @@ type custom interface { } type GT3[E custom] map[E]int + +// Instantiated types: +// Two instantiations of generic types +// with different type parameters are different. + +// both +type H[T any] []T + +// old +var V1 H[int] + +type T int + +var V2 H[T] + +// new +// i V1: changed from H[int] to H[bool] +var V1 H[bool] + +// i T: changed from int to bool +type T bool + +// OK: we reported on T, so we don't need to here. +var V2 H[T] + +// old +type t int + +// new +// i t: changed from int to byte +type t byte + +// both +var V3 H[t]