Repository: mgechev/revive
Pull request #707 (2022-07-14 09:43:43)

adds checks for protobuf struct tags
This PR adds support of protobuf struct tags to the rule ` struct-tag` 

chavacava: @git-hulk reviews are welcome

Patch:
diff --git a/rule/empty-lines.go b/rule/empty-lines.go
index b443476..1286607 100644
--- a/rule/empty-lines.go
+++ b/rule/empty-lines.go
@@ -11,14 +11,14 @@ import (
 type EmptyLinesRule struct{}
 
 // Apply applies the rule to given file.
-func (*EmptyLinesRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {
+func (r *EmptyLinesRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {
 	var failures []lint.Failure
 
 	onFailure := func(failure lint.Failure) {
 		failures = append(failures, failure)
 	}
 
-	w := lintEmptyLines{file, file.CommentMap(), onFailure}
+	w := lintEmptyLines{file, r.commentLines(file.CommentMap(), file), onFailure}
 	ast.Walk(w, file.AST)
 	return failures
 }
@@ -30,13 +30,13 @@ func (*EmptyLinesRule) Name() string {
 
 type lintEmptyLines struct {
 	file      *lint.File
-	cmap      ast.CommentMap
+	cmap      map[int]struct{}
 	onFailure func(lint.Failure)
 }
 
 func (w lintEmptyLines) Visit(node ast.Node) ast.Visitor {
 	block, ok := node.(*ast.BlockStmt)
-	if !ok {
+	if !ok || len(block.List) == 0 {
 		return w
 	}
 
@@ -47,67 +47,59 @@ func (w lintEmptyLines) Visit(node ast.Node) ast.Visitor {
 }
 
 func (w lintEmptyLines) checkStart(block *ast.BlockStmt) {
-	if len(block.List) == 0 {
-		return
-	}
-
-	start := w.position(block.Lbrace)
+	blockStart := w.position(block.Lbrace)
 	firstNode := block.List[0]
+	firstStmt := w.position(firstNode.Pos())
 
-	if w.commentBetween(start, firstNode) {
+	firstBlockLineIsStmt := firstStmt.Line-(blockStart.Line+1) == 0
+	_, firstBlockLineIsComment := w.cmap[blockStart.Line+1]
+	if firstBlockLineIsStmt || firstBlockLineIsComment {
 		return
 	}
 
-	first := w.position(firstNode.Pos())
-	if first.Line-start.Line > 1 {
-		w.onFailure(lint.Failure{
-			Confidence: 1,
-			Node:       block,
-			Category:   "style",
-			Failure:    "extra empty line at the start of a block",
-		})
-	}
+	w.onFailure(lint.Failure{
+		Confidence: 1,
+		Node:       block,
+		Category:   "style",
+		Failure:    "extra empty line at the start of a block",
+	})
 }
 
 func (w lintEmptyLines) checkEnd(block *ast.BlockStmt) {
-	if len(block.List) < 1 {
-		return
-	}
-
-	end := w.position(block.Rbrace)
+	blockEnd := w.position(block.Rbrace)
 	lastNode := block.List[len(block.List)-1]
+	lastStmt := w.position(lastNode.End())
 
-	if w.commentBetween(end, lastNode) {
+	lastBlockLineIsStmt := (blockEnd.Line-1)-lastStmt.Line == 0
+	_, lastBlockLineIsComment := w.cmap[blockEnd.Line-1]
+	if lastBlockLineIsStmt || lastBlockLineIsComment {
 		return
 	}
 
-	last := w.position(lastNode.End())
-	if end.Line-last.Line > 1 {
-		w.onFailure(lint.Failure{
-			Confidence: 1,
-			Node:       lastNode,
-			Category:   "style",
-			Failure:    "extra empty line at the end of a block",
-		})
-	}
+	w.onFailure(lint.Failure{
+		Confidence: 1,
+		Node:       block,
+		Category:   "style",
+		Failure:    "extra empty line at the end of a block",
+	})
 }
 
-func (w lintEmptyLines) commentBetween(position token.Position, node ast.Node) bool {
-	comments := w.cmap.Filter(node).Comments()
-	if len(comments) == 0 {
-		return false
-	}
+func (w lintEmptyLines) position(pos token.Pos) token.Position {
+	return w.file.ToPosition(pos)
+}
+
+func (*EmptyLinesRule) commentLines(cmap ast.CommentMap, file *lint.File) map[int]struct{} {
+	result := map[int]struct{}{}
 
-	for _, comment := range comments {
-		start, end := w.position(comment.Pos()), w.position(comment.End())
-		if start.Line-position.Line == 1 || position.Line-end.Line == 1 {
-			return true
+	for _, comments := range cmap {
+		for _, comment := range comments {
+			start := file.ToPosition(comment.Pos())
+			end := file.ToPosition(comment.End())
+			for i := start.Line; i <= end.Line; i++ {
+				result[i] = struct{}{}
+			}
 		}
 	}
 
-	return false
-}
-
-func (w lintEmptyLines) position(pos token.Pos) token.Position {
-	return w.file.ToPosition(pos)
+	return result
 }
diff --git a/rule/struct-tag.go b/rule/struct-tag.go
index 6e3e00b..3accf58 100644
--- a/rule/struct-tag.go
+++ b/rule/struct-tag.go
@@ -35,7 +35,7 @@ func (*StructTagRule) Name() string {
 
 type lintStructTagRule struct {
 	onFailure   func(lint.Failure)
-	usedTagNbr  map[string]bool // list of used tag numbers
+	usedTagNbr  map[int]bool    // list of used tag numbers
 	usedTagName map[string]bool // list of used tag keys
 }
 
@@ -45,7 +45,7 @@ func (w lintStructTagRule) Visit(node ast.Node) ast.Visitor {
 		if n.Fields == nil || n.Fields.NumFields() < 1 {
 			return nil // skip empty structs
 		}
-		w.usedTagNbr = map[string]bool{}  // init
+		w.usedTagNbr = map[int]bool{}     // init
 		w.usedTagName = map[string]bool{} // init
 		for _, f := range n.Fields.List {
 			if f.Tag != nil {
@@ -74,6 +74,10 @@ func (w lintStructTagRule) checkTagNameIfNeed(tag *structtag.Tag) (string, bool)
 	}
 
 	tagName := w.getTagName(tag)
+	if tagName == "" {
+		return "", true // No tag name found
+	}
+
 	// We concat the key and name as the mapping key here
 	// to allow the same tag name in different tag type.
 	key := tag.Key + ":" + tagName
@@ -94,7 +98,7 @@ func (lintStructTagRule) getTagName(tag *structtag.Tag) string {
 				return strings.TrimLeft(option, "name=")
 			}
 		}
-		return "protobuf tag lacks name"
+		return "" //protobuf tag lacks 'name' option
 	default:
 		return tag.Name
 	}
@@ -139,7 +143,10 @@ func (w lintStructTagRule) checkTaggedField(f *ast.Field) {
 				w.addFailure(f.Tag, msg)
 			}
 		case "protobuf":
-			// Not implemented yet
+			msg, ok := w.checkProtobufTag(tag)
+			if !ok {
+				w.addFailure(f.Tag, msg)
+			}
 		case "required":
 			if tag.Name != "true" && tag.Name != "false" {
 				w.addFailure(f.Tag, "required should be 'true' or 'false'")
@@ -170,10 +177,14 @@ func (w lintStructTagRule) checkASN1Tag(t ast.Expr, tag *structtag.Tag) (string,
 			if strings.HasPrefix(opt, "tag:") {
 				parts := strings.Split(opt, ":")
 				tagNumber := parts[1]
-				if w.usedTagNbr[tagNumber] {
-					return fmt.Sprintf("duplicated tag number %s", tagNumber), false
+				number, err := strconv.Atoi(tagNumber)
+				if err != nil {
+					return fmt.Sprintf("ASN1 tag must be a number, got '%s'", tagNumber), false
+				}
+				if w.usedTagNbr[number] {
+					return fmt.Sprintf("duplicated tag number %v", number), false
 				}
-				w.usedTagNbr[tagNumber] = true
+				w.usedTagNbr[number] = true
 
 				continue
 			}
@@ -275,6 +286,57 @@ func (lintStructTagRule) typeValueMatch(t ast.Expr, val string) bool {
 	return typeMatches
 }
 
+func (w lintStructTagRule) checkProtobufTag(tag *structtag.Tag) (string, bool) {
+	// check name
+	switch tag.Name {
+	case "bytes", "fixed32", "fixed64", "group", "varint", "zigzag32", "zigzag64":
+		// do nothing
+	default:
+		return fmt.Sprintf("invalid protobuf tag name '%s'", tag.Name), false
+	}
+
+	// check options
+	seenOptions := map[string]bool{}
+	for _, opt := range tag.Options {
+		if number, err := strconv.Atoi(opt); err == nil {
+			_, alreadySeen := w.usedTagNbr[number]
+			if alreadySeen {
+				return fmt.Sprintf("duplicated tag number %v", number), false
+			}
+			w.usedTagNbr[number] = true
+			continue // option is an integer
+		}
+
+		switch {
+		case opt == "opt" || opt == "proto3" || opt == "rep" || opt == "req":
+			// do nothing
+		case strings.Contains(opt, "="):
+			o := strings.Split(opt, "=")[0]
+			_, alreadySeen := seenOptions[o]
+			if alreadySeen {
+				return fmt.Sprintf("protobuf tag has duplicated option '%s'", o), false
+			}
+			seenOptions[o] = true
+			continue
+		}
+	}
+	_, hasName := seenOptions["name"]
+	if !hasName {
+		return "protobuf tag lacks mandatory option 'name'", false
+	}
+
+	for k := range seenOptions {
+		switch k {
+		case "name", "json":
+			// do nothing
+		default:
+			return fmt.Sprintf("unknown option '%s' in protobuf tag", k), false
+		}
+	}
+
+	return "", true
+}
+
 func (w lintStructTagRule) addFailure(n ast.Node, msg string) {
 	w.onFailure(lint.Failure{
 		Node:       n,


Test patch:
diff --git a/testdata/empty-lines.go b/testdata/empty-lines.go
index 0e9d57c..a37a778 100644
--- a/testdata/empty-lines.go
+++ b/testdata/empty-lines.go
@@ -9,14 +9,14 @@ func f1(x *int) bool { // MATCH /extra empty line at the start of a block/
 	return x > 2
 }
 
-func f2(x *int) bool {
-	return x > 2 // MATCH /extra empty line at the end of a block/
+func f2(x *int) bool { // MATCH /extra empty line at the end of a block/
+	return x > 2
 
 }
 
 func f3(x *int) bool { // MATCH /extra empty line at the start of a block/
 
-	return x > 2 // MATCH /extra empty line at the end of a block/
+	return x > 2 // MATCH:17 /extra empty line at the end of a block/
 
 }
 
@@ -36,8 +36,8 @@ func f6(x *int) bool {
 	// This is fine.
 }
 
-func f7(x *int) bool {
-	return x > 2 // MATCH /extra empty line at the end of a block/
+func f7(x *int) bool { // MATCH /extra empty line at the end of a block/
+	return x > 2
 	// This is _not_ fine.
 
 }
@@ -52,8 +52,8 @@ func f8(*int) bool {
 }
 
 func f9(*int) bool {
-	if x > 2 {
-		return true // MATCH /extra empty line at the end of a block/
+	if x > 2 { // MATCH /extra empty line at the end of a block/
+		return true
 
 	}
 
@@ -75,8 +75,8 @@ func f11(x *int) bool {
 	}
 }
 
-func f12(x *int) bool {
-	if x > 2 { // MATCH /extra empty line at the end of a block/
+func f12(x *int) bool { // MATCH /extra empty line at the end of a block/
+	if x > 2 {
 		return true
 	}
 
@@ -89,8 +89,8 @@ func f13(x *int) bool {
 	}
 }
 
-func f14(x *int) bool {
-	switch { // MATCH /extra empty line at the end of a block/
+func f14(x *int) bool { // MATCH /extra empty line at the end of a block/
+	switch {
 	case x == 2:
 		return false
 	}
@@ -98,8 +98,8 @@ func f14(x *int) bool {
 }
 
 func f15(x *int) bool {
-	switch {
-	case x == 2: // MATCH /extra empty line at the end of a block/
+	switch { // MATCH /extra empty line at the end of a block/
+	case x == 2:
 		return false
 
 	}
@@ -111,8 +111,8 @@ func f16(x *int) bool {
 	).Execute()
 }
 
-func f17(x *int) bool {
-	return Query( // MATCH /extra empty line at the end of a block/
+func f17(x *int) bool { // MATCH /extra empty line at the end of a block/
+	return Query(
 		qm("x = ?", x),
 	).Execute()
 
@@ -120,7 +120,7 @@ func f17(x *int) bool {
 
 func f18(x *int) bool {
 	if true {
-		if true { // MATCH /extra empty line at the end of a block/
+		if true {
 			return true
 		}
 
@@ -160,3 +160,11 @@ func x() {
 		}
 	}
 }
+
+func ShouldNotWarn() {
+	// comment
+
+	println()
+
+	// comment
+}
diff --git a/testdata/struct-tag.go b/testdata/struct-tag.go
index 600895a..21e73aa 100644
--- a/testdata/struct-tag.go
+++ b/testdata/struct-tag.go
@@ -46,8 +46,9 @@ type TestContextSpecificTags2 struct {
 	B       int       `asn1:"tag:2"`
 	S       string    `asn1:"tag:0,utf8"`
 	Ints    []int     `asn1:"set"`
-	Version int       `asn1:"optional,explicit,default:0,tag:0"` // MATCH /duplicated tag number 0/
-	Time    time.Time `asn1:"explicit,tag:4,other"`              // MATCH /unknown option 'other' in ASN1 tag/
+	Version int       `asn1:"optional,explicit,default:0,tag:000"` // MATCH /duplicated tag number 0/
+	Time    time.Time `asn1:"explicit,tag:4,other"`                // MATCH /unknown option 'other' in ASN1 tag/
+	X       int       `asn1:"explicit,tag:invalid"`                // MATCH /ASN1 tag must be a number, got 'invalid'/
 }
 
 type VirtualMachineRelocateSpecDiskLocator struct {
@@ -102,3 +103,20 @@ type HelmChartArgs struct {
 	ReleaseNamespace string                 `json:"releaseNamespace,omitempty" yaml:"releaseNamespace,omitempty"`
 	ExtraArgs        []string               `json:"extraArgs,omitempty" yaml:"extraArgs,omitempty"`
 }
+
+// Test message for holding primitive types.
+type Simple struct {
+	OBool                *bool    `protobuf:"varint,1,req,json=oBool"`                           // MATCH /protobuf tag lacks mandatory option 'name'/
+	OInt32               *int32   `protobuf:"varint,2,opt,name=o_int32,jsonx=oInt32"`            // MATCH /unknown option 'jsonx' in protobuf tag/
+	OInt32Str            *int32   `protobuf:"varint,3,rep,name=o_int32_str,name=oInt32Str"`      // MATCH /protobuf tag has duplicated option 'name'/
+	OInt64               *int64   `protobuf:"varint,4,opt,json=oInt64,name=o_int64,json=oInt64"` // MATCH /protobuf tag has duplicated option 'json'/
+	OSint32Str           *int32   `protobuf:"zigzag32,11,opt,name=o_sint32_str,json=oSint32Str"`
+	OSint64Str           *int64   `protobuf:"zigzag64,13,opt,name=o_sint32_str,json=oSint64Str"` // MATCH /duplicate tag name: 'o_sint32_str'/
+	OFloat               *float32 `protobuf:"fixed32,14,opt,name=o_float,json=oFloat"`
+	ODouble              *float64 `protobuf:"fixed64,014,opt,name=o_double,json=oDouble"`      // MATCH /duplicated tag number 14/
+	ODoubleStr           *float64 `protobuf:"fixed6,17,opt,name=o_double_str,json=oDoubleStr"` // MATCH /invalid protobuf tag name 'fixed6'/
+	OString              *string  `protobuf:"bytes,18,opt,name=o_string,json=oString"`
+	XXX_NoUnkeyedLiteral struct{} `json:"-"`
+	XXX_unrecognized     []byte   `json:"-"`
+	XXX_sizecache        int32    `json:"-"`
+}