From 044a1fa22177c88e1b100cc870996f823a429c34 Mon Sep 17 00:00:00 2001 From: Joel Hendrix Date: Fri, 14 Jun 2024 07:16:51 -0700 Subject: [PATCH] Fix sort ordering and type alias resolution (#8438) * Fix sort ordering and type alias resolution Sort diagnostics and navigation slices. This requires a custom JSON marshaler for Navigation as it contains a map and the default behavior is non-deterministic. Recursively resolve type aliases. This is to handle cases where a type alias is to another type alias. * remove custom JSON marshaler according to the docs, the keys *are* sorted * add unit test * add code owners for Go APIView tool --- .github/CODEOWNERS | 5 + src/go/cmd/api_view.go | 34 ++++ src/go/cmd/api_view_test.go | 17 ++ src/go/cmd/module.go | 174 ++++++++++-------- src/go/cmd/pkg.go | 3 +- .../test_multi_recursive_alias/go.mod | 3 + .../internal/exported/source.go | 13 ++ .../test_multi_recursive_alias/pkga/source.go | 9 + .../test_multi_recursive_alias/pkgb/source.go | 9 + .../test_multi_recursive_alias/pkgc/source.go | 9 + src/go/go.mod | 6 +- src/go/go.sum | 8 +- 12 files changed, 205 insertions(+), 85 deletions(-) create mode 100644 src/go/cmd/testdata/test_multi_recursive_alias/go.mod create mode 100644 src/go/cmd/testdata/test_multi_recursive_alias/internal/exported/source.go create mode 100644 src/go/cmd/testdata/test_multi_recursive_alias/pkga/source.go create mode 100644 src/go/cmd/testdata/test_multi_recursive_alias/pkgb/source.go create mode 100644 src/go/cmd/testdata/test_multi_recursive_alias/pkgc/source.go diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 28e2778eb7a..964f096a8f2 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -55,3 +55,8 @@ ########### /src/dotnet/Azure.ClientSdk.Analyzers @jsquire @pallavit @JoshLove-msft @christothes @annelo-msft @KrzysztofCwalina @tg-msft @heaths @m-nash /src/dotnet/APIView @chidozieononiwu + +########### +# Go Client Tools +########### +/src/go @jhendrixMSFT @chlowell @RickWinter diff --git a/src/go/cmd/api_view.go b/src/go/cmd/api_view.go index 9b0dd7cfad2..fceff244590 100644 --- a/src/go/cmd/api_view.go +++ b/src/go/cmd/api_view.go @@ -7,6 +7,7 @@ import ( "encoding/json" "os" "path/filepath" + "slices" "sort" "strings" ) @@ -74,6 +75,22 @@ func createReview(pkgDir string) (PackageReview, error) { }) diagnostics = append(diagnostics, p.diagnostics...) } + + slices.SortFunc(diagnostics, func(a Diagnostic, b Diagnostic) int { + targetCmp := strings.Compare(a.TargetID, b.TargetID) + if targetCmp != 0 { + return targetCmp + } + // if the target IDs are the same then fall back to the text. + // this accounts for cases where there are multiple diagnostics + // for the same target ID. + return strings.Compare(a.Text, b.Text) + }) + + for _, n := range nav { + recursiveSortNavigation(n) + } + return PackageReview{ Diagnostics: diagnostics, Language: "Go", @@ -83,3 +100,20 @@ func createReview(pkgDir string) (PackageReview, error) { PackageName: m.PackageName, }, nil } + +func recursiveSortNavigation(n Navigation) { + for _, nn := range n.ChildItems { + recursiveSortNavigation(nn) + } + slices.SortFunc(n.ChildItems, func(a Navigation, b Navigation) int { + aa, err := json.Marshal(a) + if err != nil { + panic(err) + } + bb, err := json.Marshal(b) + if err != nil { + panic(err) + } + return strings.Compare(string(aa), string(bb)) + }) +} diff --git a/src/go/cmd/api_view_test.go b/src/go/cmd/api_view_test.go index 622945a9c9f..7c8e80b1a22 100644 --- a/src/go/cmd/api_view_test.go +++ b/src/go/cmd/api_view_test.go @@ -4,6 +4,7 @@ package cmd import ( + "encoding/json" "os" "path/filepath" "testing" @@ -298,3 +299,19 @@ func Test_getPackageNameFromModPath(t *testing.T) { require.EqualValues(t, "sdk/foo/bar", getPackageNameFromModPath("github.com/Azure/azure-sdk-for-go/sdk/foo/bar")) require.EqualValues(t, "sdk/foo/bar", getPackageNameFromModPath("github.com/Azure/azure-sdk-for-go/sdk/foo/bar/v5")) } + +func TestDeterministicOutput(t *testing.T) { + for i := 0; i < 100; i++ { + review1, err := createReview(filepath.Clean("testdata/test_multi_recursive_alias")) + require.NoError(t, err) + review2, err := createReview(filepath.Clean("testdata/test_multi_recursive_alias")) + require.NoError(t, err) + + output1, err := json.MarshalIndent(review1, "", " ") + require.NoError(t, err) + output2, err := json.MarshalIndent(review2, "", " ") + require.NoError(t, err) + + require.EqualValues(t, string(output1), string(output2)) + } +} diff --git a/src/go/cmd/module.go b/src/go/cmd/module.go index 7d25b01bfef..fa35d925e6b 100644 --- a/src/go/cmd/module.go +++ b/src/go/cmd/module.go @@ -71,7 +71,7 @@ func NewModule(dir string) (*Module, error) { packageName := getPackageNameFromModPath(mf.Module.Mod.Path) fmt.Printf("Package Name: %s\n", packageName) - m := Module{Name: filepath.Base(dir), PackageName: packageName, packages: map[string]*Pkg{}} + m := &Module{Name: filepath.Base(dir), PackageName: packageName, packages: map[string]*Pkg{}} baseImportPath := path.Dir(mf.Module.Mod.Path) + "/" if baseImportPath == "./" { @@ -114,94 +114,114 @@ func NewModule(dir string) (*Module, error) { // the definition from azcore/internal/shared into the APIView for azcore, making the type's // fields visible there. externalPackages := map[string]*Pkg{} + + // tracks which packages have had their type aliases resolved. + // this prevents resolving dependent packages multiple times. + processedPackages := map[string]struct{}{} + for _, p := range m.packages { - for alias, qn := range p.typeAliases { - // qn is a type name qualified with import path like - // "github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/shared.TokenRequestOptions" - impPath := qn[:strings.LastIndex(qn, ".")] - typeName := qn[len(impPath)+1:] - var source *Pkg - var ok bool - if source, ok = m.packages[impPath]; !ok { - // must be a package external to this module - if source, ok = externalPackages[impPath]; !ok && sdkRoot != "" { - // figure out a path to the package, index it - if _, after, found := strings.Cut(impPath, "azure-sdk-for-go/sdk/"); found { - p := filepath.Join(sdkRoot, strings.TrimSuffix(versionReg.ReplaceAllString(after, "/"), "/")) - pkg, err := NewPkg(p, "github.com/Azure/azure-sdk-for-go/sdk/"+after) - if err == nil { - pkg.Index() - externalPackages[impPath] = pkg - source = pkg - } else { - // types from this module will appear in the review without their definitions - fmt.Printf("couldn't parse %s: %v\n", impPath, err) - } + recursiveResolveTypeAliases(m, p, externalPackages, sdkRoot, processedPackages) + } + return m, nil +} + +func recursiveResolveTypeAliases(m *Module, p *Pkg, externalPackages map[string]*Pkg, sdkRoot string, processedPackages map[string]struct{}) { + if _, ok := processedPackages[p.relName]; ok { + // already processed this package + return + } + + for alias, qn := range p.typeAliases { + // qn is a type name qualified with import path like + // "github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/shared.TokenRequestOptions" + impPath := qn[:strings.LastIndex(qn, ".")] + typeName := qn[len(impPath)+1:] + var source *Pkg + var ok bool + if source, ok = m.packages[impPath]; !ok { + // must be a package external to this module + if source, ok = externalPackages[impPath]; !ok && sdkRoot != "" { + // figure out a path to the package, index it + if _, after, found := strings.Cut(impPath, "azure-sdk-for-go/sdk/"); found { + p := filepath.Join(sdkRoot, strings.TrimSuffix(versionReg.ReplaceAllString(after, "/"), "/")) + pkg, err := NewPkg(p, "github.com/Azure/azure-sdk-for-go/sdk/"+after) + if err == nil { + pkg.Index() + externalPackages[impPath] = pkg + source = pkg + } else { + // types from this module will appear in the review without their definitions + fmt.Printf("couldn't parse %s: %v\n", impPath, err) } } } + } else if len(source.typeAliases) > 0 { + // if the source has type aliases we need to resolve them first. + // this is to handle recursive type aliases. + recursiveResolveTypeAliases(m, source, externalPackages, sdkRoot, processedPackages) + } - level := DiagnosticLevelInfo - originalName := qn - if _, after, found := strings.Cut(qn, m.Name); found { - originalName = strings.TrimPrefix(after, "/") - } else { - // this type is defined in another module - level = DiagnosticLevelWarning - } + level := DiagnosticLevelInfo + originalName := qn + if _, after, found := strings.Cut(qn, m.Name); found { + originalName = strings.TrimPrefix(after, "/") + } else { + // this type is defined in another module + level = DiagnosticLevelWarning + } - var t TokenMaker - if source == nil { - t = p.c.addSimpleType(*p, alias, p.Name(), originalName, nil) - } else if def, ok := recursiveFindTypeDef(typeName, source, m.packages); ok { - switch n := def.n.Type.(type) { - case *ast.InterfaceType: - t = p.c.addInterface(*def.p, alias, p.Name(), n, nil) - case *ast.StructType: - t = p.c.addStruct(*def.p, alias, p.Name(), def.n, nil) - hoistMethodsForType(source, alias, p) - // ensure that all struct field types that are structs are also aliased from this package - for _, field := range n.Fields.List { - fieldTypeName := unwrapStructFieldTypeName(field) - if fieldTypeName == "" { - // we can ignore this field - continue - } - - // ensure that our package exports this type - if _, ok := p.typeAliases[fieldTypeName]; ok { - // found an alias - continue - } - - // no alias, add a diagnostic - p.diagnostics = append(p.diagnostics, Diagnostic{ - Level: DiagnosticLevelError, - TargetID: t.ID(), - Text: missingAliasFor + fieldTypeName, - }) + var t TokenMaker + if source == nil { + t = p.c.addSimpleType(*p, alias, p.Name(), originalName, nil) + } else if def, ok := recursiveFindTypeDef(typeName, source, m.packages); ok { + switch n := def.n.Type.(type) { + case *ast.InterfaceType: + t = p.c.addInterface(*def.p, alias, p.Name(), n, nil) + case *ast.StructType: + t = p.c.addStruct(*def.p, alias, p.Name(), def.n, nil) + hoistMethodsForType(source, alias, p) + // ensure that all struct field types that are structs are also aliased from this package + for _, field := range n.Fields.List { + fieldTypeName := unwrapStructFieldTypeName(field) + if fieldTypeName == "" { + // we can ignore this field + continue } - case *ast.Ident: - t = p.c.addSimpleType(*p, alias, p.Name(), def.n.Type.(*ast.Ident).Name, nil) - hoistMethodsForType(source, alias, p) - default: - fmt.Printf("unexpected node type %T\n", def.n.Type) - t = p.c.addSimpleType(*p, alias, p.Name(), originalName, nil) + + // ensure that our package exports this type + if _, ok := p.typeAliases[fieldTypeName]; ok { + // found an alias + continue + } + + // no alias, add a diagnostic + p.diagnostics = append(p.diagnostics, Diagnostic{ + Level: DiagnosticLevelError, + TargetID: t.ID(), + Text: missingAliasFor + fieldTypeName, + }) } - } else { - fmt.Println("found no definition for " + qn) + case *ast.Ident: + t = p.c.addSimpleType(*p, alias, p.Name(), def.n.Type.(*ast.Ident).Name, nil) + hoistMethodsForType(source, alias, p) + default: + fmt.Printf("unexpected node type %T\n", def.n.Type) + t = p.c.addSimpleType(*p, alias, p.Name(), originalName, nil) } + } else { + fmt.Println("found no definition for " + qn) + } - if t != nil { - p.diagnostics = append(p.diagnostics, Diagnostic{ - Level: level, - TargetID: t.ID(), - Text: aliasFor + originalName, - }) - } + if t != nil { + p.diagnostics = append(p.diagnostics, Diagnostic{ + Level: level, + TargetID: t.ID(), + Text: aliasFor + originalName, + }) } } - return &m, nil + + processedPackages[p.relName] = struct{}{} } // returns the type name for the specified struct field. diff --git a/src/go/cmd/pkg.go b/src/go/cmd/pkg.go index fbce2298453..a68f75806eb 100644 --- a/src/go/cmd/pkg.go +++ b/src/go/cmd/pkg.go @@ -9,12 +9,13 @@ import ( "go/ast" "go/parser" "go/token" - "golang.org/x/exp/slices" "os" "path" "path/filepath" "strings" "unicode" + + "golang.org/x/exp/slices" ) // diagnostic messages diff --git a/src/go/cmd/testdata/test_multi_recursive_alias/go.mod b/src/go/cmd/testdata/test_multi_recursive_alias/go.mod new file mode 100644 index 00000000000..2196531ffff --- /dev/null +++ b/src/go/cmd/testdata/test_multi_recursive_alias/go.mod @@ -0,0 +1,3 @@ +module github.com/Azure/azure-sdk-for-go/sdk/test_multi_recursive_alias + +go 1.18 diff --git a/src/go/cmd/testdata/test_multi_recursive_alias/internal/exported/source.go b/src/go/cmd/testdata/test_multi_recursive_alias/internal/exported/source.go new file mode 100644 index 00000000000..16a4c2a85fe --- /dev/null +++ b/src/go/cmd/testdata/test_multi_recursive_alias/internal/exported/source.go @@ -0,0 +1,13 @@ +package exported + +type Type1 struct { + Foo string +} + +type Type2 struct { + Foo string +} + +type Type3 struct { + Foo string +} diff --git a/src/go/cmd/testdata/test_multi_recursive_alias/pkga/source.go b/src/go/cmd/testdata/test_multi_recursive_alias/pkga/source.go new file mode 100644 index 00000000000..c8c090558f3 --- /dev/null +++ b/src/go/cmd/testdata/test_multi_recursive_alias/pkga/source.go @@ -0,0 +1,9 @@ +package pkga + +import "github.com/Azure/azure-sdk-for-go/sdk/test_multi_recursive_alias/pkgb" + +type Type1 = pkgb.Type1 + +type Type2 = pkgb.Type2 + +type Type3 = pkgb.Type3 diff --git a/src/go/cmd/testdata/test_multi_recursive_alias/pkgb/source.go b/src/go/cmd/testdata/test_multi_recursive_alias/pkgb/source.go new file mode 100644 index 00000000000..2dc8f9a503f --- /dev/null +++ b/src/go/cmd/testdata/test_multi_recursive_alias/pkgb/source.go @@ -0,0 +1,9 @@ +package pkgb + +import "github.com/Azure/azure-sdk-for-go/sdk/test_multi_recursive_alias/pkgc" + +type Type1 = pkgc.Type1 + +type Type2 = pkgc.Type2 + +type Type3 = pkgc.Type3 diff --git a/src/go/cmd/testdata/test_multi_recursive_alias/pkgc/source.go b/src/go/cmd/testdata/test_multi_recursive_alias/pkgc/source.go new file mode 100644 index 00000000000..d2551c8852d --- /dev/null +++ b/src/go/cmd/testdata/test_multi_recursive_alias/pkgc/source.go @@ -0,0 +1,9 @@ +package pkgc + +import "github.com/Azure/azure-sdk-for-go/sdk/test_multi_recursive_alias/internal/exported" + +type Type1 = exported.Type1 + +type Type2 = exported.Type2 + +type Type3 = exported.Type3 diff --git a/src/go/go.mod b/src/go/go.mod index 440558e388f..f26977d3583 100644 --- a/src/go/go.mod +++ b/src/go/go.mod @@ -1,12 +1,12 @@ module apiviewgo -go 1.18 +go 1.21 require ( github.com/spf13/cobra v1.8.0 github.com/stretchr/testify v1.9.0 - golang.org/x/exp v0.0.0-20240222234643-814bf88cf225 - golang.org/x/mod v0.16.0 + golang.org/x/exp v0.0.0-20240604190554-fc45aab8b7f8 + golang.org/x/mod v0.18.0 ) require ( diff --git a/src/go/go.sum b/src/go/go.sum index a3eb35d4abc..73fc5a6783d 100644 --- a/src/go/go.sum +++ b/src/go/go.sum @@ -12,10 +12,10 @@ github.com/spf13/pflag v1.0.5 h1:iy+VFUOCP1a+8yFto/drg2CJ5u0yRoB7fZw3DKv/JXA= github.com/spf13/pflag v1.0.5/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg= github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg= github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= -golang.org/x/exp v0.0.0-20240222234643-814bf88cf225 h1:LfspQV/FYTatPTr/3HzIcmiUFH7PGP+OQ6mgDYo3yuQ= -golang.org/x/exp v0.0.0-20240222234643-814bf88cf225/go.mod h1:CxmFvTBINI24O/j8iY7H1xHzx2i4OsyguNBmN/uPtqc= -golang.org/x/mod v0.16.0 h1:QX4fJ0Rr5cPQCF7O9lh9Se4pmwfwskqZfq5moyldzic= -golang.org/x/mod v0.16.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= +golang.org/x/exp v0.0.0-20240604190554-fc45aab8b7f8 h1:LoYXNGAShUG3m/ehNk4iFctuhGX/+R1ZpfJ4/ia80JM= +golang.org/x/exp v0.0.0-20240604190554-fc45aab8b7f8/go.mod h1:jj3sYF3dwk5D+ghuXyeI3r5MFf+NT2An6/9dOA95KSI= +golang.org/x/mod v0.18.0 h1:5+9lSbEzPSdWkH32vYPBwEpX8KwDbM52Ud9xBUvNlb0= +golang.org/x/mod v0.18.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=