Revert to alias handling which does not assume visit order

Author: thockin

v2/parser/parse.go

@@ -21,7 +21,6 @@ import (
 	"fmt"
 	"go/ast"
 	"go/constant"
-	"go/parser"
 	"go/token"
 	gotypes "go/types"
 	"maps"
@@ -38,8 +37,6 @@ import (
 	"k8s.io/klog/v2"
 )
 
-var typeAliasEnabled = goTypeAliasEnabled()
-
 // Parser lets you add all the go files in all the packages that you care
 // about, then constructs the type source data.
 type Parser struct {
@@ -426,33 +423,31 @@ func (p *Parser) addCommentsToType(obj gotypes.Object, t *types.Type) {
 			// no comments associated, or comments match exactly
 			t.CommentLines = newLines
 
-		case typeAliasEnabled:
-			if isTypeAlias(obj.Type()) {
-				if !reflect.DeepEqual(minimize(oldLines), minimize(newLines)) {
-					// ignore mismatched comments from obj because it's an alias
-					klog.Warningf(
-						"Mismatched comments on type %v.\n  Using comments:\n%s\n  Ignoring comments from type alias:\n%s\n",
-						t.GoType,
-						formatCommentBlock(oldLines),
-						formatCommentBlock(newLines),
-					)
-				}
-			} else {
-				panic(fmt.Sprintf("type %v already has comments; somehow we processed it again, but it's not an alias\n  old:\n%s\n  new:\n%s",
+		case isTypeAlias(obj.Type()):
+			// Ignore mismatched comments from obj because it's an alias.
+			// This can only be hit if gotypesalias is enabled.
+			if !reflect.DeepEqual(minimize(oldLines), minimize(newLines)) {
+				klog.Warningf(
+					"Mismatched comments on type %v.\n  Using comments:\n%s\n  Ignoring comments from type alias:\n%s\n",
 					t.GoType,
 					formatCommentBlock(oldLines),
 					formatCommentBlock(newLines),
-				))
+				)
 			}
 
-		case !typeAliasEnabled:
-			// ignore mismatched comments from obj because it's an PROBABLY alias
+		case !isTypeAlias(obj.Type()):
+			// Overwrite existing comments with ones from obj because obj is not an alias.
+			// If gotypesalias is enabled, this should mean we found the "real"
+			// type, not an alias.  If gotypesalias is disabled, we can end up
+			// overwriting the "real" comments with an alias's comments, but
+			// it is not clear if we can assume which one is the "real" one.
+			t.CommentLines = newLines
 			if !reflect.DeepEqual(minimize(oldLines), minimize(newLines)) {
 				klog.Warningf(
-					"Mismatched comments on type %v.\n  Using comments:\n%s\n  Ignoring comments from presumed type alias:\n%s\n",
+					"Mismatched comments on type %v.\n  Using comments:\n%s\n  Ignoring comments from possible type alias:\n%s\n",
 					t.GoType,
-					formatCommentBlock(oldLines),
 					formatCommentBlock(newLines),
+					formatCommentBlock(oldLines),
 				)
 			}
 		}
@@ -467,33 +462,32 @@ func (p *Parser) addCommentsToType(obj gotypes.Object, t *types.Type) {
 			// no comments associated, or comments match exactly
 			t.SecondClosestCommentLines = newLines
 
-		case typeAliasEnabled:
-			if isTypeAlias(obj.Type()) {
-				if !reflect.DeepEqual(minimize(oldLines), minimize(newLines)) {
-					// ignore mismatched comments from obj because it's an alias
-					klog.Warningf(
-						"Mismatched secondClosestCommentLines on type %v.\n  Using comments:\n%s\n  Ignoring comments from type alias:\n%s\n",
-						t.GoType,
-						formatCommentBlock(oldLines),
-						formatCommentBlock(newLines),
-					)
-				}
-			} else {
-				panic(fmt.Sprintf("type %v already has comments; somehow we processed it again, but it's not an alias\n  old:\n%s\n  new:\n%s",
+		case isTypeAlias(obj.Type()):
+			// Ignore mismatched comments from obj because it's an alias.
+			// This can only be hit if gotypesalias is enabled.
+			if !reflect.DeepEqual(minimize(oldLines), minimize(newLines)) {
+				// ignore mismatched comments from obj because it's an alias
+				klog.Warningf(
+					"Mismatched secondClosestCommentLines on type %v.\n  Using comments:\n%s\n  Ignoring comments from type alias:\n%s\n",
 					t.GoType,
 					formatCommentBlock(oldLines),
 					formatCommentBlock(newLines),
-				))
+				)
 			}
 
-		case !typeAliasEnabled:
-			// ignore mismatched comments from obj because it's an PROBABLY alias
+		case !isTypeAlias(obj.Type()):
+			// Overwrite existing comments with ones from obj because obj is not an alias.
+			// If gotypesalias is enabled, this should mean we found the "real"
+			// type, not an alias.  If gotypesalias is disabled, we can end up
+			// overwriting the "real" comments with an alias's comments, but
+			// it is not clear if we can assume which one is the "real" one.
+			t.SecondClosestCommentLines = newLines
 			if !reflect.DeepEqual(minimize(oldLines), minimize(newLines)) {
 				klog.Warningf(
-					"Mismatched secondClosestCommentLines on type %v.\n  Using comments:\n%s\n  Ignoring comments from presumed type alias:\n%s\n",
+					"Mismatched secondClosestCommentLines on type %v.\n  Using comments:\n%s\n  Ignoring comments from possible type alias:\n%s\n",
 					t.GoType,
-					formatCommentBlock(oldLines),
 					formatCommentBlock(newLines),
+					formatCommentBlock(oldLines),
 				)
 			}
 		}
@@ -1007,22 +1001,3 @@ func (p *Parser) addConstant(u types.Universe, useName *types.Name, in *gotypes.
 	out.ConstValue = &constval
 	return out
 }
-
-// Copied from https://github.com/golang/tools/blob/3e377036196f644e59e757af8a38ea6afa07677c/internal/aliases/aliases_go122.go#L64
-func goTypeAliasEnabled() bool {
-	// The only reliable way to compute the answer is to invoke go/types.
-	// We don't parse the GODEBUG environment variable, because
-	// (a) it's tricky to do so in a manner that is consistent
-	//     with the godebug package; in particular, a simple
-	//     substring check is not good enough. The value is a
-	//     rightmost-wins list of options. But more importantly:
-	// (b) it is impossible to detect changes to the effective
-	//     setting caused by os.Setenv("GODEBUG"), as happens in
-	//     many tests. Therefore any attempt to cache the result
-	//     is just incorrect.
-	fset := token.NewFileSet()
-	f, _ := parser.ParseFile(fset, "a.go", "package p; type A = int", parser.SkipObjectResolution)
-	pkg, _ := new(gotypes.Config).Check("p", fset, []*ast.File{f}, nil)
-	_, enabled := pkg.Scope().Lookup("A").Type().(*gotypes.Alias)
-	return enabled
-}

v2/parser/parse_test.go

@@ -19,6 +19,10 @@ package parser
 import (
 	"encoding/json"
 	"fmt"
+	"go/ast"
+	"go/parser"
+	"go/token"
+	gotypes "go/types"
 	"path/filepath"
 	"reflect"
 	"sort"
@@ -30,6 +34,8 @@ import (
 	"k8s.io/gengo/v2/types"
 )
 
+var typeAliasEnabled = goTypeAliasEnabled()
+
 func TestNew(t *testing.T) {
 	parser := New()
 	if parser.goPkgs == nil {
@@ -1115,6 +1121,10 @@ func TestStructParse(t *testing.T) {
 			},
 			expected: func() *types.Type {
 				expectedTypeComments := []string{"Blah is a test.", "A test, I tell you."}
+				if !typeAliasEnabled {
+					// Comments from the last processed package wins.
+					expectedTypeComments = []string{"This is an alias for v1.Blah."}
+				}
 
 				return &types.Type{
 					Name: types.Name{
@@ -1221,6 +1231,10 @@ func TestCommentsWithAliasedType(t *testing.T) {
 		}
 
 		expectedTypeComments := []string{"Blah is a test.", "A test, I tell you."}
+		if !typeAliasEnabled {
+			// Comments from the last processed package wins.
+			expectedTypeComments = []string{"This is an alias for v1.Blah."}
+		}
 		for _, typ := range pkg.Types {
 			if typ.Name.Name != "Blah" {
 				continue
@@ -1232,3 +1246,22 @@ func TestCommentsWithAliasedType(t *testing.T) {
 		}
 	}
 }
+
+// Copied from https://github.com/golang/tools/blob/3e377036196f644e59e757af8a38ea6afa07677c/internal/aliases/aliases_go122.go#L64
+func goTypeAliasEnabled() bool {
+	// The only reliable way to compute the answer is to invoke go/types.
+	// We don't parse the GODEBUG environment variable, because
+	// (a) it's tricky to do so in a manner that is consistent
+	//     with the godebug package; in particular, a simple
+	//     substring check is not good enough. The value is a
+	//     rightmost-wins list of options. But more importantly:
+	// (b) it is impossible to detect changes to the effective
+	//     setting caused by os.Setenv("GODEBUG"), as happens in
+	//     many tests. Therefore any attempt to cache the result
+	//     is just incorrect.
+	fset := token.NewFileSet()
+	f, _ := parser.ParseFile(fset, "a.go", "package p; type A = int", parser.SkipObjectResolution)
+	pkg, _ := new(gotypes.Config).Check("p", fset, []*ast.File{f}, nil)
+	_, enabled := pkg.Scope().Lookup("A").Type().(*gotypes.Alias)
+	return enabled
+}