Repository: mgechev/revive
Pull request #1403 (2025-06-07 16:19:52)

exported: improve error message
This PR improves the error reporting of exported rules

The idea was developed in #1235, we want to provide more information such as:

- the case changed
- the variable is now exported

Explore:
- #1226 
- #1235 


ccoVeille: I was also about to suggest this, but I was awaiting for feedbacks from others.

So yes, I'm fine with it. I'll proceed to the changes when I will have some free time.

I'm still interested by feedbacks from @denisvmedia and @chavacava


ccoVeille: @alexandear @denisvmedia @chavacava 

Can I have a feedback about the changes I'm suggesting here? 

Thanks

The PR is in Draft because I have multiple place where I will have to update the wordings.

@alexandear I don't think there is a need to postpone revive 1.11 version for this one.

It's a minor change

ccoVeille: I finally worked on it.

Please review

Side note: I opened #1431 to work on this as it was a nightmare to debug without changes.

- #1431 

Patch:
diff --git a/README.md b/README.md
index 9298d6a..267867d 100644
--- a/README.md
+++ b/README.md
@@ -2,6 +2,7 @@
 
 [![Build Status](https://github.com/mgechev/revive/actions/workflows/test.yaml/badge.svg)](https://github.com/mgechev/revive/actions/workflows/test.yaml)
 [![Go Reference](https://pkg.go.dev/badge/github.com/mgechev/revive.svg)](https://pkg.go.dev/github.com/mgechev/revive)
+[![Ask DeepWiki](https://deepwiki.com/badge.svg)](https://deepwiki.com/mgechev/revive)
 
 Fast, configurable, extensible, flexible, and beautiful linter for Go. Drop-in replacement of golint.
 **`Revive` provides a framework for development of custom rules,
diff --git a/rule/exported.go b/rule/exported.go
index 1ec4076..eb351cf 100644
--- a/rule/exported.go
+++ b/rule/exported.go
@@ -165,21 +165,36 @@ func (w *lintExported) lintFuncDoc(fn *ast.FuncDecl) {
 		return
 	}
 
-	firstCommentLine := firstCommentLine(fn.Doc)
-
-	if firstCommentLine == "" {
-		w.addFailuref(fn, 1, lint.FailureCategoryComments,
+	status := w.checkGoDocStatus(fn.Doc, fn.Name.Name)
+	switch status {
+	case exportedGoDocStatusOK:
+		return // comment is fine
+	case exportedGoDocStatusMissing:
+		w.addFailuref(fn, status.Confidence(), lint.FailureCategoryComments,
 			"exported %s %s should have comment or be unexported", kind, name,
 		)
 		return
 	}
 
-	prefix := fn.Name.Name + " "
-	if !strings.HasPrefix(firstCommentLine, prefix) {
-		w.addFailuref(fn.Doc, 0.8, lint.FailureCategoryComments,
-			`comment on exported %s %s should be of the form "%s..."`, kind, name, prefix,
-		)
+	firstCommentLine := w.firstCommentLine(fn.Doc)
+	w.addFailuref(fn.Doc, status.Confidence(), lint.FailureCategoryComments,
+		`comment on exported %s %s should be of the form "%s ..."%s`, kind, name, fn.Name.Name, status.CorrectionHint(firstCommentLine),
+	)
+}
+
+func (*lintExported) hasPrefixInsensitive(s, prefix string) bool {
+	return strings.HasPrefix(strings.ToLower(s), strings.ToLower(prefix))
+}
+
+func (*lintExported) stripFirstRune(s string) string {
+	// Decode the first rune to handle multi-byte characters.
+	firstRune, size := utf8.DecodeRuneInString(s)
+	if firstRune == utf8.RuneError {
+		return s // no valid first rune found
 	}
+
+	// Return the string without the first rune.
+	return s[size:]
 }
 
 func (w *lintExported) checkRepetitiveNames(id *ast.Ident, thing string) {
@@ -233,24 +248,24 @@ func (w *lintExported) lintTypeDoc(t *ast.TypeSpec, doc *ast.CommentGroup, first
 		return
 	}
 
+	expectedPrefix := typeName
 	for _, a := range articles {
 		if typeName == a {
 			continue
 		}
 		var found bool
 		if firstCommentLine, found = strings.CutPrefix(firstCommentLine, a+" "); found {
+			expectedPrefix = a + " " + typeName
 			break
 		}
 	}
 
-	// if comment starts with name of type and has some text after - it's ok
-	expectedPrefix := typeName + " "
-	if strings.HasPrefix(firstCommentLine, expectedPrefix) {
+	status := w.checkGoDocStatus(doc, expectedPrefix)
+	if status == exportedGoDocStatusOK {
 		return
 	}
-
-	w.addFailuref(doc, 1, lint.FailureCategoryComments,
-		`comment on exported type %v should be of the form "%s..." (with optional leading article)`, t.Name, expectedPrefix,
+	w.addFailuref(doc, status.Confidence(), lint.FailureCategoryComments,
+		`comment on exported type %v should be of the form "%s ..." (with optional leading article)%s`, t.Name, typeName, status.CorrectionHint(firstCommentLine),
 	)
 }
 
@@ -272,6 +287,7 @@ func (w *lintExported) checkValueNames(names []*ast.Ident, nodeToBlame ast.Node,
 
 	return true
 }
+
 func (w *lintExported) lintValueSpecDoc(vs *ast.ValueSpec, gd *ast.GenDecl, genDeclMissingComments map[*ast.GenDecl]bool) {
 	kind := "var"
 	if gd.Tok == token.CONST {
@@ -292,8 +308,8 @@ func (w *lintExported) lintValueSpecDoc(vs *ast.ValueSpec, gd *ast.GenDecl, genD
 		return
 	}
 
-	vsFirstCommentLine := firstCommentLine(vs.Doc)
-	gdFirstCommentLine := firstCommentLine(gd.Doc)
+	vsFirstCommentLine := w.firstCommentLine(vs.Doc)
+	gdFirstCommentLine := w.firstCommentLine(gd.Doc)
 	if vsFirstCommentLine == "" && gdFirstCommentLine == "" {
 		if genDeclMissingComments[gd] {
 			return
@@ -325,20 +341,77 @@ func (w *lintExported) lintValueSpecDoc(vs *ast.ValueSpec, gd *ast.GenDecl, genD
 	default:
 		doc = gd.Doc
 	}
+	firstCommentLine := w.firstCommentLine(doc)
 
-	prefix := name + " "
-	if !strings.HasPrefix(firstCommentLine(doc), prefix) {
-		w.addFailuref(doc, 1, lint.FailureCategoryComments,
-			`comment on exported %s %s should be of the form "%s..."`, kind, name, prefix,
-		)
+	status := w.checkGoDocStatus(doc, name)
+	if status == exportedGoDocStatusOK {
+		return
+	}
+	w.addFailuref(doc, status.Confidence(), lint.FailureCategoryComments,
+		`comment on exported %s %s should be of the form "%s ..."%s`, kind, name, name, status.CorrectionHint(firstCommentLine),
+	)
+}
+
+type exportedGoDocStatus int
+
+const (
+	exportedGoDocStatusOK exportedGoDocStatus = iota
+	exportedGoDocStatusMissing
+	exportedGoDocStatusCaseMismatch
+	exportedGoDocStatusFirstLetterMismatch
+	exportedGoDocStatusUnexpected
+)
+
+func (gds exportedGoDocStatus) Confidence() float64 {
+	if gds == exportedGoDocStatusUnexpected {
+		return 0.8
 	}
+	return 1
+}
+
+func (gds exportedGoDocStatus) CorrectionHint(firstCommentLine string) string {
+	firstWord := strings.Split(firstCommentLine, " ")[0]
+	switch gds {
+	case exportedGoDocStatusCaseMismatch:
+		return ` by using its correct casing, not "` + firstWord + ` ..."`
+	case exportedGoDocStatusFirstLetterMismatch:
+		return ` to match its exported status, not "` + firstWord + ` ..."`
+	}
+
+	return ""
+}
+
+func (w *lintExported) checkGoDocStatus(comment *ast.CommentGroup, name string) exportedGoDocStatus {
+	firstCommentLine := w.firstCommentLine(comment)
+	if firstCommentLine == "" {
+		return exportedGoDocStatusMissing
+	}
+
+	name = strings.TrimSpace(name)
+	// Make sure the expected prefix has a space at the end.
+	expectedPrefix := name + " "
+	if strings.HasPrefix(firstCommentLine, expectedPrefix) {
+		return exportedGoDocStatusOK
+	}
+
+	if !w.hasPrefixInsensitive(firstCommentLine, expectedPrefix) {
+		return exportedGoDocStatusUnexpected
+	}
+
+	if strings.HasPrefix(w.stripFirstRune(firstCommentLine), w.stripFirstRune(expectedPrefix)) {
+		// Only the first character differs, such as "sendJSON" became "SendJSON".
+		// so we consider the scope has changed.
+		return exportedGoDocStatusFirstLetterMismatch
+	}
+
+	return exportedGoDocStatusCaseMismatch
 }
 
 // firstCommentLine yields the first line of interest in comment group or "" if there is nothing of interest.
 // An "interesting line" is a comment line that is neither a directive (e.g. //go:...) or a deprecation comment
 // (lines from the first line with a prefix // Deprecated: to the end of the comment group)
 // Empty or spaces-only lines are discarded.
-func firstCommentLine(comment *ast.CommentGroup) (result string) {
+func (lintExported) firstCommentLine(comment *ast.CommentGroup) (result string) {
 	if comment == nil {
 		return ""
 	}
@@ -384,10 +457,10 @@ func (w *lintExported) Visit(n ast.Node) ast.Visitor {
 		// inside a GenDecl, which usually has the doc
 		doc := v.Doc
 
-		fcl := firstCommentLine(doc)
+		fcl := w.firstCommentLine(doc)
 		if fcl == "" {
 			doc = w.lastGenDecl.Doc
-			fcl = firstCommentLine(doc)
+			fcl = w.firstCommentLine(doc)
 		}
 		w.lintTypeDoc(v, doc, fcl)
 		w.checkRepetitiveNames(v.Name, "type")
@@ -421,21 +494,23 @@ func (w *lintExported) lintInterfaceMethod(typeName string, m *ast.Field) {
 	if !ast.IsExported(m.Names[0].Name) {
 		return
 	}
+
 	name := m.Names[0].Name
-	firstCommentLine := firstCommentLine(m.Doc)
-	if firstCommentLine == "" {
-		w.addFailuref(m, 1, lint.FailureCategoryComments,
+	status := w.checkGoDocStatus(m.Doc, name)
+	switch status {
+	case exportedGoDocStatusOK:
+		return // comment is fine
+	case exportedGoDocStatusMissing:
+		w.addFailuref(m, status.Confidence(), lint.FailureCategoryComments,
 			"public interface method %s.%s should be commented", typeName, name,
 		)
 		return
 	}
 
-	expectedPrefix := m.Names[0].Name + " "
-	if !strings.HasPrefix(firstCommentLine, expectedPrefix) {
-		w.addFailuref(m.Doc, 0.8, lint.FailureCategoryComments,
-			`comment on exported interface method %s.%s should be of the form "%s..."`, typeName, name, expectedPrefix,
-		)
-	}
+	firstCommentLine := w.firstCommentLine(m.Doc)
+	w.addFailuref(m.Doc, status.Confidence(), lint.FailureCategoryComments,
+		`comment on exported interface method %s.%s should be of the form "%s ..."%s`, typeName, name, name, status.CorrectionHint(firstCommentLine),
+	)
 }
 
 // mustCheckMethod returns true if the method must be checked by this rule, false otherwise.


Test patch:
diff --git a/test/exported_test.go b/test/exported_test.go
index bba148d..dd8ffc7 100644
--- a/test/exported_test.go
+++ b/test/exported_test.go
@@ -51,3 +51,7 @@ func TestCheckDeprecationComment(t *testing.T) {
 func TestExportedMainPackage(t *testing.T) {
 	testRule(t, "exported_main", &rule.ExportedRule{}, &lint.RuleConfig{})
 }
+
+func TestCommentVariations(t *testing.T) {
+	testRule(t, "exported_issue_1235", &rule.ExportedRule{}, &lint.RuleConfig{})
+}
diff --git a/test/utils_test.go b/test/utils_test.go
index 7ad3394..209851d 100644
--- a/test/utils_test.go
+++ b/test/utils_test.go
@@ -9,6 +9,7 @@ import (
 	"go/types"
 	"os"
 	"path/filepath"
+	"slices"
 	"strconv"
 	"strings"
 	"testing"
@@ -98,6 +99,13 @@ func assertFailures(t *testing.T, filePath string, rules []lint.Rule, config map
 		failures = append(failures, f)
 	}
 
+	type simplifiedFailure struct {
+		File    string
+		Line    int
+		Failure string
+	}
+	var reportedFailures []simplifiedFailure
+
 	for _, in := range ins {
 		ok := false
 		for i, p := range failures {
@@ -114,13 +122,21 @@ func assertFailures(t *testing.T, filePath string, rules []lint.Rule, config map
 						r = r[:i]
 					}
 					if r != in.Replacement {
-						t.Errorf("Lint failed at %s:%d; got replacement %q, want %q", filePath, in.Line, r, in.Replacement)
+						reportedFailures = append(reportedFailures, simplifiedFailure{
+							File:    filePath,
+							Line:    in.Line,
+							Failure: fmt.Sprintf("Replacement: got %q, want %q", r, in.Replacement),
+						})
 					}
 				}
 
 				if in.Confidence > 0 {
 					if in.Confidence != p.Confidence {
-						t.Errorf("Lint failed at %s:%d; got confidence %f, want %f", filePath, in.Line, p.Confidence, in.Confidence)
+						reportedFailures = append(reportedFailures, simplifiedFailure{
+							File:    filePath,
+							Line:    in.Line,
+							Failure: fmt.Sprintf("Confidence: got %f, want %f", p.Confidence, in.Confidence),
+						})
 					}
 				}
 
@@ -133,12 +149,52 @@ func assertFailures(t *testing.T, filePath string, rules []lint.Rule, config map
 			}
 		}
 		if !ok {
-			t.Errorf("Lint failed at %s:%d; /%v/ did not match", filePath, in.Line, in.Match)
+			reportedFailures = append(reportedFailures, simplifiedFailure{
+				File:    filePath,
+				Line:    in.Line,
+				Failure: "want: " + in.Match,
+			})
 		}
 	}
+
 	for _, p := range failures {
-		t.Errorf("Unexpected problem at %s:%d: %v", filePath, p.Position.Start.Line, p.Failure)
+		reportedFailures = append(reportedFailures, simplifiedFailure{
+			File:    filePath,
+			Line:    p.Position.Start.Line,
+			Failure: "got:  " + p.Failure,
+		})
+	}
+
+	slices.SortFunc(reportedFailures, func(a, b simplifiedFailure) int {
+		if a.File != b.File {
+			return strings.Compare(a.File, b.File)
+		}
+		if a.Line != b.Line {
+			return a.Line - b.Line
+		}
+
+		return strings.Compare(a.Failure, b.Failure)
+	})
+
+	errorMessage := ""
+	lastFileLine := ""
+	for _, p := range reportedFailures {
+		currentFileLine := fmt.Sprintf("%s:%d", p.File, p.Line)
+		if currentFileLine != lastFileLine {
+			if errorMessage != "" {
+				t.Error(errorMessage)
+			}
+			errorMessage = fmt.Sprintf("problem at %s: ", currentFileLine)
+			lastFileLine = currentFileLine
+		}
+
+		errorMessage += "\n" + p.Failure
+	}
+
+	if errorMessage != "" {
+		t.Error(errorMessage)
 	}
+
 	return nil
 }
 
diff --git a/testdata/exported_issue_1235.go b/testdata/exported_issue_1235.go
new file mode 100644
index 0000000..81ec2d4
--- /dev/null
+++ b/testdata/exported_issue_1235.go
@@ -0,0 +1,53 @@
+package golint
+
+import (
+	"errors"
+)
+
+// SendJson sends a JSON object to the server.
+func SendJSON(data interface{}) error {
+	return nil
+}
+
+// ErrInvalidJson is returned when the JSON is invalid.
+var ErrInvalidJSON = errors.New("invalid JSON")
+
+// StatusHTTP represents an HTTP status code.
+type StatusHTTP int
+
+// Foobar blah blah
+func (s StatusHTTP) FooBar() int {
+	return int(s)
+}
+
+// qux was previously unexported, but now it is exported.
+func (s StatusHTTP) Qux() int {
+	return int(s)
+}
+
+// SendJson sends a JSON object to the server.
+func SendJSON(data interface{}) error {
+	return nil
+}
+
+// errNotFound is returned when the requested resource is not found.
+var ErrNotFound = errors.New("not found")
+
+// scope changed
+// MATCH:23 /comment on exported method StatusHTTP.Qux should be of the form "Qux ..." to match its exported status, not "qux ..."/
+// MATCH:33 /comment on exported var ErrNotFound should be of the form "ErrNotFound ..." to match its exported status, not "errNotFound ..."/
+
+// case change
+// MATCH:7 /comment on exported function SendJSON should be of the form "SendJSON ..." by using its correct casing, not "SendJson ..."/
+// MATCH:12 /comment on exported var ErrInvalidJSON should be of the form "ErrInvalidJSON ..." by using its correct casing, not "ErrInvalidJson ..."/
+// MATCH:18 /comment on exported method StatusHTTP.FooBar should be of the form "FooBar ..." by using its correct casing, not "Foobar ..."/
+// MATCH:28 /comment on exported function SendJSON should be of the form "SendJSON ..." by using its correct casing, not "SendJson ..."/
+
+// VeryLongCommentThatCouldBeCJKThatCannotBeSplitOnSpaces is about the function F.
+func F() string {
+	return "F"
+}
+
+// This one is a safeguard against future changes in the way the error is reported.
+// Here we should never suggest something that would include VeryLongCommentThatCouldBeCJKThatCannotBeSplitOnSpaces in the error message.
+// MATCH:46 /comment on exported function F should be of the form "F ..."/