gopls/internal/analysis/deprecated: support line comments

More generally, this adjusts astutil.Deprecation to also check for line comments that follow the following rules:
* The doc.Text() is a single line.
* The comment uses the "// ..." format.

This also restricts the deprecated analyzer to only look for deprecated line comments for struct fields and interface methods. We can expand this in the future if desired.

Will update the documentation in https://go.dev/wiki/Deprecated in a follow up cl.

Also added tests for more of the use cases of the deprecated analyzer and unit tests for the astutil.Deprecation function.

Fixes golang/go#79801

Change-Id: Iab35855bd24b38eb54159000b8b4945c6a6a6964
Reviewed-on: https://go-review.googlesource.com/c/tools/+/787100
Auto-Submit: Alex Putman <aputman@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

Author: aputman

gopls/internal/analysis/deprecated/deprecated.go

@@ -156,15 +156,19 @@ type deprecatedNames struct {
 // them both as Facts and the return value. This is a simplified copy
 // of staticcheck's fact_deprecated analyzer.
 func collectDeprecatedNames(pass *analysis.Pass, ins *inspector.Inspector) (deprecatedNames, error) {
-	doDocs := func(names []*ast.Ident, docs *ast.CommentGroup) {
-		alt := strings.TrimPrefix(internalastutil.Deprecation(docs), "Deprecated: ")
-		if alt == "" {
-			return
-		}
+	doDocs := func(names []*ast.Ident, docs ...*ast.CommentGroup) {
+		for _, doc := range docs {
+			// Find the first doc with a deprecation marker.
+			alt := strings.TrimPrefix(internalastutil.Deprecation(doc), "Deprecated: ")
+			if alt == "" {
+				continue
+			}
 
-		for _, name := range names {
-			obj := pass.TypesInfo.ObjectOf(name)
-			pass.ExportObjectFact(obj, &deprecationFact{alt})
+			for _, name := range names {
+				obj := pass.TypesInfo.ObjectOf(name)
+				pass.ExportObjectFact(obj, &deprecationFact{alt})
+			}
+			return
 		}
 	}
 
@@ -222,14 +226,16 @@ func collectDeprecatedNames(pass *analysis.Pass, ins *inspector.Inspector) (depr
 			names = node.Names
 		case *ast.StructType:
 			for _, field := range node.Fields.List {
-				doDocs(field.Names, field.Doc)
+				doDocs(field.Names, field.Doc, field.Comment)
 			}
 		case *ast.InterfaceType:
 			for _, field := range node.Methods.List {
-				doDocs(field.Names, field.Doc)
+				doDocs(field.Names, field.Doc, field.Comment)
 			}
 		}
 		if docs != nil && len(names) > 0 {
+			// Only consider line comments for struct fields and interface
+			// methods.
 			doDocs(names, docs)
 		}
 	})

gopls/internal/analysis/deprecated/testdata/src/a/a.go

@@ -4,11 +4,35 @@
 
 package usedeprecated
 
-import "io/ioutil" // want "\"io/ioutil\" is deprecated: .*"
+import (
+	"io/ioutil" // want "\"io/ioutil\" is deprecated: .*"
+
+	"legacy"
+)
 
 func x() {
 	_, _ = ioutil.ReadFile("") // want "ioutil.ReadFile is deprecated: As of Go 1.16, .*"
 	Legacy()                   // expect no deprecation notice.
+
+	x := legacy.Object{} // want "legacy.Object is deprecated: Use obj instead"
+
+	x.DocCommentMethod(1)  // expect no deprecation notice, deprecation is only on interface.
+	x.LineCommentMethod(1) // expect no deprecation notice, deprecation is only on interface.
+
+	_ = x.DocCommentField  // want "x.DocCommentField is deprecated: Use `Field` instead."
+	_ = x.LineCommentField // want "x.LineCommentField is deprecated: Use `Field` instead."
+
+	// Make sure that the doc comment is chosen over the line comment if both have
+	// deprecation tags.
+	_ = x.BothCommentField // want "x.BothCommentField is deprecated: Doc comment chosen"
+
+	legacy.Legacy() // want "Legacy is deprecated: use X instead."
+	y(x)
+}
+
+func y(i legacy.Interface) {
+	i.DocCommentMethod(1)  // want "i.DocCommentMethod is deprecated: Use Method instead."
+	i.LineCommentMethod(1) // want "i.LineCommentMethod is deprecated: Use Method instead."
 }
 
 // Legacy is deprecated.

gopls/internal/analysis/deprecated/testdata/src/legacy/legacy.go

@@ -0,0 +1,38 @@
+// Package legacy
+package legacy
+
+// Object docs
+//
+// Deprecated: Use obj instead.
+type Object struct {
+	// Don't use.
+	//
+	// Deprecated: Use `Field` instead.
+	//
+	// Paragraph after.
+	DocCommentField int
+
+	LineCommentField int // Deprecated: Use `Field` instead.
+
+	// Deprecated: Doc comment chosen
+	BothCommentField int // Deprecated: Line comment chosen
+}
+
+type Interface interface {
+	// Deprecated: Use Method instead.
+	DocCommentMethod(int) int
+	LineCommentMethod(int) int // Old method. Deprecated: Use Method instead.
+}
+
+// No deprecation notice in concrete method docs.
+func (Object) DocCommentMethod(int) int {
+	return 1
+}
+
+// No deprecation notice in concrete method docs.
+func (Object) LineCommentMethod(int) int {
+	return 1
+}
+
+// Deprecated: use X instead.
+func Legacy() {}

internal/astutil/comment.go

@@ -13,11 +13,17 @@ import (
 )
 
 // Deprecation returns the paragraph of the doc comment that starts with the
-// conventional "Deprecation: " marker, as defined by
-// https://go.dev/wiki/Deprecated, or "" if the documented symbol is not
-// deprecated.
+// conventional "Deprecation: " marker, or the end of a single-line comment
+// with the deprecation marker, as defined by https://go.dev/wiki/Deprecated.
+// Returns "" if the documented symbol is not deprecated.
+//
+// Deprecation(nil) returns the empty string.
 func Deprecation(doc *ast.CommentGroup) string {
-	for p := range strings.SplitSeq(doc.Text(), "\n\n") {
+	// doc.Text() is newline-terminated. For legacy reasons, this function will
+	// return as newline-terminated if is the last segment of the CommentGroup
+	// but not if it is a paragraph in the middle of the CommentGroup.
+	docText := doc.Text()
+	for p := range strings.SplitSeq(docText, "\n\n") {
 		// There is still some ambiguity for deprecation message. This function
 		// only returns the paragraph introduced by "Deprecated: ". More
 		// information related to the deprecation may follow in additional
@@ -27,6 +33,19 @@ func Deprecation(doc *ast.CommentGroup) string {
 			return p
 		}
 	}
+
+	// We also want to support deprecation markers in line comments. Not all
+	// call sites know whether they have a line comment or the type of AST node
+	// the comment is associated with; so to best match line deprecations,
+	// the CommentGroup must meet these criteria:
+	//   * The doc.Text() is a single line.
+	//   * The comment uses the "// ..." format.
+	if doc == nil || len(doc.List) != 1 || !strings.HasPrefix(doc.List[0].Text, "//") {
+		return ""
+	}
+	if i := strings.Index(docText, "Deprecated: "); i != -1 {
+		return docText[i:]
+	}
 	return ""
 }
 

internal/astutil/comment_test.go

@@ -5,6 +5,7 @@
 package astutil_test
 
 import (
+	"fmt"
 	"go/ast"
 	"go/parser"
 	"go/token"
@@ -57,3 +58,137 @@ func fn() { }`
 		})
 	}
 }
+
+func TestDeprecation(t *testing.T) {
+	testsCases := []struct {
+		name string
+		in   string
+		want string
+	}{
+		{
+			name: "doc_comment_only_paragraph",
+			in: `// Deprecated: Test
+			     // a whole paragraph.
+			     type A struct {}`,
+			want: "Deprecated: Test\na whole paragraph.\n",
+		},
+		{
+			name: "doc_comment_any_paragraph",
+			in: `// First paragraph
+			     //
+			     // Deprecated: Middle
+			     // paragraph.
+			     //
+			     // Last Paragraph
+			     type A struct {}`,
+			want: "Deprecated: Middle\nparagraph.",
+		},
+		{
+			name: "doc_comment_finds_the_first",
+			in: `// Deprecated: First
+			     //
+			     // Deprecated: second
+			     type A struct {}`,
+			want: "Deprecated: First",
+		},
+		{
+			name: "doc_comment_not_found_inside_paragraph",
+			in: `// First paragraph
+			     // Deprecated: Middle paragraph.
+			     type A struct {}`,
+			want: "",
+		},
+		{
+			name: "multi_line_doc_comment_supported_if_no_whitespace",
+			in: `
+/*First paragraph
+
+Deprecated: Middle paragraph
+
+Last Paragraph
+*/
+type A struct {}`,
+			want: "Deprecated: Middle paragraph",
+		},
+		{
+			name: "multi_line_doc_comment_weird_format_not_supported",
+			// This is what the go formatter formats when the text starts on
+			// the second line of a /* */ comment.
+			in: `
+			/*
+				First paragraph
+
+				Deprecated: Middle paragraph
+
+				Last Paragraph
+			*/
+			type A struct {}`,
+			// Not found, as the "Deprecated: ..." line has leading whitespace.
+			want: "",
+		},
+		{
+			name: "line_comment_just_deprecated_tag",
+			in: `type A interface {
+			         B(int) int // Deprecated: use 'C()'
+			     }`,
+			want: "Deprecated: use 'C()'\n",
+		},
+		{
+			name: "line_comment_comment_before_deprecated_tag",
+			in: `type A struct {
+			         b int // This does x. Deprecated: use 'c'. Will cleanup.
+			     }`,
+			want: "Deprecated: use 'c'. Will cleanup.\n",
+		},
+		{
+			name: "line_comment_finds_first_deprecated_tag",
+			in: `type A struct {
+			         int // Deprecated: use 'c'. Deprecated: use 'd'.
+			     }`,
+			want: "Deprecated: use 'c'. Deprecated: use 'd'.\n",
+		},
+		{
+			name: "line_comment_doesnt_support_multi_line_comment_type",
+			in: `type A struct {
+			         b int /*Deprecated: use 'c'*/
+			     }`,
+			// We can't prevent this, as ast.CommentGroup doesn't specify
+			// where the comment happened. We won't advertise this.
+			want: "Deprecated: use 'c'\n",
+		},
+		{
+			name: "multiline_comment_cant_have_comment_before_deprecated_tag",
+			in: `type A struct {
+			         b int /* test Deprecated: use 'c' */
+			     }`,
+			want: "",
+		},
+	}
+
+	for _, test := range testsCases {
+		t.Run(test.name, func(t *testing.T) {
+			src := fmt.Sprintf("package a; \n\n%s", test.in)
+			f, err := parser.ParseFile(token.NewFileSet(), "a.go", src, parser.ParseComments)
+			if err != nil {
+				t.Fatal(err)
+			}
+			switch len(f.Comments) {
+			case 0:
+				t.Error("No `ast.CommentGroup` found")
+			case 1:
+			default:
+				t.Errorf("%d `ast.CommentGroup`s found, only want one", len(f.Comments))
+			}
+			if got := astutil.Deprecation(f.Comments[0]); got != test.want {
+				// align 'got' and 'want' for easier inspection
+				t.Errorf("\nfound comment: %q\ngot:  %q\nwant: %q", f.Comments[0].Text(), got, test.want)
+			}
+		})
+	}
+
+	t.Run("Deprecation(nil)", func(t *testing.T) {
+		if got := astutil.Deprecation(nil); got != "" {
+			t.Errorf("Deprecation(nil) = %q, want: \"\"", got)
+		}
+	})
+}