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

Author: jhendrixMSFT

.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

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

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

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.

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

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

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
+}

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

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

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