Repository: mgechev/revive (go)

Issue:
struct-tag: support "properties" tag
"properties" was added to the [well-known struct tags on Go wiki](https://go.dev/wiki/Well-known-struct-tags#list-of-well-known-struct-tags)

- https://github.com/golang/go/issues/72759

- https://github.com/golang/wiki/pull/44

It could be added to revive to the very sane way TOML support was added recently to [struct-tag](https://github.com/mgechev/revive/blob/master/RULES_DESCRIPTIONS.md#struct-tag) rule

- https://github.com/mgechev/revive/pull/1255

Here is the documentation of the format of this tag

https://pkg.go.dev/github.com/magiconair/properties#Properties.Decode

Based on my understanding of the code and documentation only the following options are supported
- "-" ignored
- "default" (without it a field is required)
   Note: There is a special notation using `;` for slice/array default value.
- "layout" (only for time.Time type)

cc @magiconair (properties lib owner)


Pull request description:
feature: add support for properties tag in struct-tag rule
Closes #1269 

type checking default values for slices is omitted on purpose (might be added later)

A PR for cleaning up the whole rule's code will follow

Patch:
diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md
index 2b795ef..6868890 100644
--- a/RULES_DESCRIPTIONS.md
+++ b/RULES_DESCRIPTIONS.md
@@ -64,8 +64,8 @@ List of all available rules.
   - [range](#range)
   - [receiver-naming](#receiver-naming)
   - [redefines-builtin-id](#redefines-builtin-id)
-  - [redundant-import-alias](#redundant-import-alias)
   - [redundant-build-tag](#redundant-build-tag)
+  - [redundant-import-alias](#redundant-import-alias)
   - [redundant-test-main-exit](#redundant-test-main-exit)
   - [string-format](#string-format)
   - [string-of-int](#string-of-int)
@@ -866,16 +866,16 @@ Even if possible, redefining these built in names can lead to bugs very difficul
 
 _Configuration_: N/A
 
-## redundant-import-alias
+## redundant-build-tag
 
-_Description_: This rule warns on redundant import aliases. This happens when the alias used on the import statement matches the imported package name.
+_Description_: This rule warns about redundant build tag comments `// +build` when `//go:build` is present.
+`gofmt` in Go 1.17+ automatically adds the `//go:build` constraint, making the `// +build` comment unnecessary.
 
 _Configuration_: N/A
 
-## redundant-build-tag
+## redundant-import-alias
 
-_Description_: This rule warns about redundant build tag comments `// +build` when `//go:build` is present.
-`gofmt` in Go 1.17+ automatically adds the `//go:build` constraint, making the `// +build` comment unnecessary.
+_Description_: This rule warns on redundant import aliases. This happens when the alias used on the import statement matches the imported package name.
 
 _Configuration_: N/A
 
@@ -924,7 +924,7 @@ _Configuration_: N/A
 ## struct-tag
 
 _Description_: Struct tags are not checked at compile time.
-This rule spots errors in struct tags of the following types: asn1, bson, datastore, default, json, mapstructure, protobuf, required, toml, url, validate, xml, yaml.
+This rule spots errors in struct tags of the following types: asn1, bson, datastore, default, json, mapstructure, properties, protobuf, required, toml, url, validate, xml, yaml.
 
 _Configuration_: (optional) list of user defined options.
 
diff --git a/revivelib/core.go b/revivelib/core.go
index 4dd760a..86aff1a 100755
--- a/revivelib/core.go
+++ b/revivelib/core.go
@@ -163,7 +163,7 @@ func (r *Revive) Format(
 	<-exitChan
 
 	if formatErr != nil {
-		return "", exitCode, fmt.Errorf("formatting: %w", err)
+		return "", exitCode, fmt.Errorf("formatting: %w", formatErr)
 	}
 
 	return output, exitCode, nil
diff --git a/rule/struct_tag.go b/rule/struct_tag.go
index d6a7708..e6a69e6 100644
--- a/rule/struct_tag.go
+++ b/rule/struct_tag.go
@@ -105,6 +105,7 @@ const (
 	keyDefault      = "default"
 	keyJSON         = "json"
 	keyMapstructure = "mapstructure"
+	keyProperties   = "properties"
 	keyProtobuf     = "protobuf"
 	keyRequired     = "required"
 	keyTOML         = "toml"
@@ -205,6 +206,11 @@ func (w lintStructTagRule) checkTaggedField(f *ast.Field) {
 			if !ok {
 				w.addFailure(f.Tag, msg)
 			}
+		case keyProperties:
+			msg, ok := w.checkPropertiesTag(f.Type, tag.Options)
+			if !ok {
+				w.addFailure(f.Tag, msg)
+			}
 		case keyProtobuf:
 			msg, ok := w.checkProtobufTag(tag)
 			if !ok {
@@ -506,6 +512,43 @@ func (lintStructTagRule) typeValueMatch(t ast.Expr, val string) bool {
 
 	return typeMatches
 }
+func (w lintStructTagRule) checkPropertiesTag(t ast.Expr, options []string) (string, bool) {
+	if len(options) == 0 {
+		return "", true
+	}
+
+	hasDefault := false
+	for _, opt := range options {
+		key, val, found := strings.Cut(opt, "=")
+		switch key {
+		case "default":
+			if hasDefault {
+				return "properties tag accepts only one default option", false
+			}
+			hasDefault = true
+
+			if !found {
+				return "malformed default for properties tag", false
+			}
+
+			if !w.typeValueMatch(t, val) {
+				return "field type and default value type mismatch in properties tag", false
+			}
+		case "layout":
+			if !found || strings.TrimSpace(val) == "" {
+				return "malformed layout option for properties tag", false
+			}
+
+			if gofmt(t) != "time.Time" {
+				return "layout option is only applicable to fields of type time.Time in properties tag", false
+			}
+		default:
+			return fmt.Sprintf("unknown option %q in properties tag", opt), false
+		}
+	}
+
+	return "", true
+}
 
 func (w lintStructTagRule) checkProtobufTag(tag *structtag.Tag) (string, bool) {
 	// check name
diff --git a/rule/useless_break.go b/rule/useless_break.go
index 8db20c9..b99cc81 100644
--- a/rule/useless_break.go
+++ b/rule/useless_break.go
@@ -47,36 +47,39 @@ func (w *lintUselessBreak) Visit(node ast.Node) ast.Visitor {
 		w.inLoopBody = false
 		return nil
 	case *ast.CommClause:
-		for _, n := range v.Body {
-			w.inspectCaseStatement(n)
-		}
+		w.inspectCaseStatement(v.Body)
 		return nil
 	case *ast.CaseClause:
-		for _, n := range v.Body {
-			w.inspectCaseStatement(n)
-		}
+		w.inspectCaseStatement(v.Body)
 		return nil
 	}
 	return w
 }
 
-func (w *lintUselessBreak) inspectCaseStatement(n ast.Stmt) {
-	switch s := n.(type) {
-	case *ast.BranchStmt:
-		if s.Tok != token.BREAK {
-			return // not a break statement
-		}
-		if s.Label != nil {
-			return // labeled break statement, usually affects a nesting loop
-		}
-		msg := "useless break in case clause"
-		if w.inLoopBody {
-			msg += " (WARN: this break statement affects this switch or select statement and not the loop enclosing it)"
-		}
-		w.onFailure(lint.Failure{
-			Confidence: 1,
-			Node:       s,
-			Failure:    msg,
-		})
+func (w *lintUselessBreak) inspectCaseStatement(body []ast.Stmt) {
+	l := len(body)
+	if l == 0 {
+		return // empty body, nothing to do
 	}
+
+	s := body[l-1] // pick the last statement
+	if !isUnlabelledBreak(s) {
+		return
+	}
+
+	msg := "useless break in case clause"
+	if w.inLoopBody {
+		msg += " (WARN: this break statement affects this switch or select statement and not the loop enclosing it)"
+	}
+
+	w.onFailure(lint.Failure{
+		Confidence: 1,
+		Node:       s,
+		Failure:    msg,
+	})
+}
+
+func isUnlabelledBreak(stmt ast.Stmt) bool {
+	s, ok := stmt.(*ast.BranchStmt)
+	return ok && s.Tok == token.BREAK && s.Label == nil
 }


Test patch:
diff --git a/testdata/struct_tag.go b/testdata/struct_tag.go
index 330d631..4518140 100644
--- a/testdata/struct_tag.go
+++ b/testdata/struct_tag.go
@@ -163,3 +163,19 @@ type TomlUser struct {
 	Username string `toml:"username,omitempty"`
 	Location string `toml:"location,unknown"` // MATCH /unknown option 'unknown' in TOML tag/
 }
+
+type PropertiesTags struct {
+	Field int               `properties:"-"`
+	Field int               `properties:"myName"`
+	Field int               `properties:"myName,default=15"`
+	Field int               `properties:"myName,default=sString"` // MATCH /field type and default value type mismatch in properties tag/
+	Field int               `properties:",default:15"`            // MATCH /unknown option "default:15" in properties tag/
+	Field int               `properties:",default=15,default=2"`  // MATCH /properties tag accepts only one default option/
+	Field time.Time         `properties:"date,layout=2006-01-02"`
+	Field time.Time         `properties:",layout=2006-01-02"`
+	Field time.Time         `properties:"date,layout"`            // MATCH /malformed layout option for properties tag/
+	Field time.Time         `properties:"date,layout=  "`         // MATCH /malformed layout option for properties tag/
+	Field string            `properties:"date,layout=2006-01-02"` // MATCH /layout option is only applicable to fields of type time.Time in properties tag/
+	Field []string          `properties:",default=a;b;c"`
+	Field map[string]string `properties:"myName,omitempty"` // MATCH /unknown option "omitempty" in properties tag/
+}
diff --git a/testdata/useless_break.go b/testdata/useless_break.go
index aa62178..f8b5261 100644
--- a/testdata/useless_break.go
+++ b/testdata/useless_break.go
@@ -87,4 +87,16 @@ func UselessBreaks() {
 	default:
 		s.errorf("range can't iterate over %v", val)
 	}
+
+	// issue #1281
+	switch 1 {
+	case 1:
+		fmt.Println("foo")
+		break // MATCH /useless break in case clause/
+	case 2:
+		fmt.Println("bar")
+		break 
+		fmt.Println("baz")
+	case 3:
+	}
 }