Merge pull request #314 from thockin/master

Fine-tune the type-alias comment handling

Author: k8s-ci-robot

v2/Makefile

@@ -11,4 +11,4 @@ test:
 verify:
 	GODEBUG=gotypesalias=0 ./hack/verify-examples.sh
 	GODEBUG=gotypesalias=1 ./hack/verify-examples.sh
-	./hack/verify-go-directive.sh 1.20
+	./hack/verify-go-directive.sh 1.23

v2/go.mod

@@ -1,15 +1,15 @@
 module k8s.io/gengo/v2
 
-go 1.20
+go 1.23
 
 require (
+	github.com/google/go-cmp v0.6.0
 	github.com/spf13/pflag v1.0.5
 	golang.org/x/tools v0.16.1
 	k8s.io/klog/v2 v2.2.0
 )
 
 require (
 	github.com/go-logr/logr v0.2.0 // indirect
-	github.com/google/go-cmp v0.6.0 // indirect
 	golang.org/x/mod v0.14.0 // indirect
 )

v2/go.sum

@@ -7,6 +7,7 @@ github.com/spf13/pflag v1.0.5/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An
 golang.org/x/mod v0.14.0 h1:dGoOF9QVLYng8IHTm7BAyWqCqSheQ5pYWGhzW00YJr0=
 golang.org/x/mod v0.14.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c=
 golang.org/x/sync v0.5.0 h1:60k92dhOjHxJkrqnwsfl8KuaHbn/5dl0lUPUklKo3qE=
+golang.org/x/sync v0.5.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk=
 golang.org/x/tools v0.16.1 h1:TLyB3WofjdOEepBHAU20JdNC1Zbg87elYofWYAY5oZA=
 golang.org/x/tools v0.16.1/go.mod h1:kYVVN6I1mBNoB1OX+noeBjbRk4IUEPa7JJ+TJMEooJ0=
 k8s.io/klog/v2 v2.2.0 h1:XRvcwJozkgZ1UQJmfMGpvRthQHOvihEhYtDfAaxMz/A=

v2/parser/parse.go

@@ -385,66 +385,117 @@ func (p *Parser) NewUniverse() (types.Universe, error) {
 	return u, nil
 }
 
+// minimize returns a copy of lines with "irrelevant" lines removed.  This
+// includes blank lines and paragraphs starting with "Deprecated:".
+func minimize(lines []string) []string {
+	out := make([]string, 0, len(lines))
+	inDeprecated := false // paragraph tracking
+	prevWasBlank := false
+	for _, line := range lines {
+		if len(strings.TrimSpace(line)) == 0 {
+			prevWasBlank = true
+			inDeprecated = false
+			continue
+		}
+		if inDeprecated {
+			continue
+		}
+		if prevWasBlank && strings.HasPrefix(strings.TrimSpace(line), "Deprecated:") {
+			prevWasBlank = false
+			inDeprecated = true
+			continue
+		}
+		prevWasBlank = false
+		out = append(out, line)
+	}
+	return out
+}
+
 // addCommentsToType takes any accumulated comment lines prior to obj and
 // attaches them to the type t.
 func (p *Parser) addCommentsToType(obj gotypes.Object, t *types.Type) {
 	if newLines, oldLines := p.docComment(obj.Pos()), t.CommentLines; len(newLines) > 0 {
 		switch {
-		case len(oldLines) == 0, reflect.DeepEqual(oldLines, newLines):
+		case reflect.DeepEqual(oldLines, newLines):
+			// nothing needed
+
+		case len(oldLines) == 0:
 			// no comments associated, or comments match exactly
 			t.CommentLines = newLines
 
 		case isTypeAlias(obj.Type()):
-			// ignore mismatched comments from obj because it's an alias
-			klog.Warningf(
-				"Mismatched comments seen for type %v. Using comments:\n%s\nIgnoring comments from type alias:\n%s\n",
-				t.GoType,
-				formatCommentBlock(oldLines),
-				formatCommentBlock(newLines),
-			)
+			// 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 !isTypeAlias(obj.Type()):
-			// overwrite existing comments with ones from obj because obj is not an alias
+			// 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
-			klog.Warningf(
-				"Mismatched comments seen for type %v. Using comments:\n%s\nIgnoring comments from type alias:\n%s\n",
-				t.GoType,
-				formatCommentBlock(newLines),
-				formatCommentBlock(oldLines),
-			)
+			if !reflect.DeepEqual(minimize(oldLines), minimize(newLines)) {
+				klog.Warningf(
+					"Mismatched comments on type %v.\n  Using comments:\n%s\n  Ignoring comments from possible type alias:\n%s\n",
+					t.GoType,
+					formatCommentBlock(newLines),
+					formatCommentBlock(oldLines),
+				)
+			}
 		}
 	}
 
 	if newLines, oldLines := p.priorDetachedComment(obj.Pos()), t.SecondClosestCommentLines; len(newLines) > 0 {
 		switch {
-		case len(oldLines) == 0, reflect.DeepEqual(oldLines, newLines):
+		case reflect.DeepEqual(oldLines, newLines):
+			// nothing needed
+
+		case len(oldLines) == 0:
 			// no comments associated, or comments match exactly
 			t.SecondClosestCommentLines = newLines
 
 		case isTypeAlias(obj.Type()):
-			// ignore mismatched comments from obj because it's an alias
-			klog.Warningf(
-				"Mismatched secondClosestCommentLines seen for type %v. Using comments:\n%s\nIgnoring comments from type alias:\n%s\n",
-				t.GoType,
-				formatCommentBlock(oldLines),
-				formatCommentBlock(newLines),
-			)
+			// 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 !isTypeAlias(obj.Type()):
-			// overwrite existing comments with ones from obj because obj is not an alias
+			// 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
-			klog.Warningf(
-				"Mismatched secondClosestCommentLines seen for type %v. Using comments:\n%s\nIgnoring comments from type alias:\n%s\n",
-				t.GoType,
-				formatCommentBlock(newLines),
-				formatCommentBlock(oldLines),
-			)
+			if !reflect.DeepEqual(minimize(oldLines), minimize(newLines)) {
+				klog.Warningf(
+					"Mismatched secondClosestCommentLines on type %v.\n  Using comments:\n%s\n  Ignoring comments from possible type alias:\n%s\n",
+					t.GoType,
+					formatCommentBlock(newLines),
+					formatCommentBlock(oldLines),
+				)
+			}
 		}
 	}
 }
 
 func formatCommentBlock(lines []string) string {
-	return "```\n" + strings.Join(lines, "\n") + "\n```"
+	return "    ```\n    " + strings.Join(lines, "\n    ") + "\n    ```"
 }
 
 // packageDir tries to figure out the directory of the specified package.

v2/parser/parse_test.go

@@ -1120,7 +1120,7 @@ func TestStructParse(t *testing.T) {
 				"k8s.io/gengo/v2/parser/testdata/type-alias/v2",
 			},
 			expected: func() *types.Type {
-				expectedTypeComments := []string{"Blah is a test.", "A test, I tell you."}
+				expectedTypeComments := []string{"Blah is a test.", "A test, I tell you.", "", "Another paragraph of comments."}
 				if !typeAliasEnabled {
 					// Comments from the last processed package wins.
 					expectedTypeComments = []string{"This is an alias for v1.Blah."}
@@ -1230,7 +1230,7 @@ func TestCommentsWithAliasedType(t *testing.T) {
 			continue
 		}
 
-		expectedTypeComments := []string{"Blah is a test.", "A test, I tell you."}
+		expectedTypeComments := []string{"Blah is a test.", "A test, I tell you.", "", "Another paragraph of comments."}
 		if !typeAliasEnabled {
 			// Comments from the last processed package wins.
 			expectedTypeComments = []string{"This is an alias for v1.Blah."}

v2/parser/testdata/type-alias/v1/file.go

@@ -1,7 +1,13 @@
 package v1
 
+// This is an alias within the same package.
+// It comes before the type it aliases.
+type LocalBlahAlias = Blah
+
 // Blah is a test.
 // A test, I tell you.
+//
+// Another paragraph of comments.
 type Blah struct {
 	// A is the first field.
 	A int64 `json:"a"`
@@ -11,5 +17,19 @@ type Blah struct {
 	B string `json:"b"`
 }
 
-// This is an alias within the same package.
-type LocalBlahAlias = Blah
+// Blah is a test.
+// A test, I tell you.
+//
+// Deprecated: use Blah instead. This is another alias within the same package.
+//
+// Another paragraph of comments.
+type LocalBlahAliasDeprecated = Blah
+
+// Blah is a test.
+// A test, I tell you.
+//
+// Deprecated: use Blah instead. This is a third alias within the same package.
+// It's a whole paragraph of deprecated notes
+//
+// Another paragraph of comments.
+type LocalBlahAliasDeprecatedLong = Blah