Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix issue #520 #802

Merged
merged 1 commit into from
Mar 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -465,7 +465,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a
| [`add-constant`](./RULES_DESCRIPTIONS.md#add-constant) | map | Suggests using constant for magic numbers and string literals | no | no |
| [`flag-parameter`](./RULES_DESCRIPTIONS.md#flag-parameter) | n/a | Warns on boolean parameters that create a control coupling | no | no |
| [`unnecessary-stmt`](./RULES_DESCRIPTIONS.md#unnecessary-stmt) | n/a | Suggests removing or simplifying unnecessary statements | no | no |
| [`struct-tag`](./RULES_DESCRIPTIONS.md#struct-tag) | n/a | Checks common struct tags like `json`,`xml`,`yaml` | no | no |
| [`struct-tag`](./RULES_DESCRIPTIONS.md#struct-tag) | []string | Checks common struct tags like `json`,`xml`,`yaml` | no | no |
| [`modifies-value-receiver`](./RULES_DESCRIPTIONS.md#modifies-value-receiver) | n/a | Warns on assignments to value-passed method receivers | no | yes |
| [`constant-logical-expr`](./RULES_DESCRIPTIONS.md#constant-logical-expr) | n/a | Warns on constant logical expressions | no | no |
| [`bool-literal-in-expr`](./RULES_DESCRIPTIONS.md#bool-literal-in-expr)| n/a | Suggests removing Boolean literals from logic expressions | no | no |
Expand Down
11 changes: 10 additions & 1 deletion RULES_DESCRIPTIONS.md
Original file line number Diff line number Diff line change
Expand Up @@ -608,7 +608,16 @@ Example:
_Description_: Struct tags are not checked at compile time.
This rule, checks and warns if it finds errors in common struct tags types like: asn1, default, json, protobuf, xml, yaml.

_Configuration_: N/A
_Configuration_: (optional) list of user defined options.

Example:
To accept the `inline` option in JSON tags (and `outline` and `gnu` in BSON tags) you must provide the following configuration

```toml
[rule.struct-tag]
arguments = ["json,inline","bson,outline,gnu"]
```


## superfluous-else

Expand Down
123 changes: 99 additions & 24 deletions rule/struct-tag.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,55 @@ import (
"go/ast"
"strconv"
"strings"
"sync"

"github.com/fatih/structtag"
"github.com/mgechev/revive/lint"
)

// StructTagRule lints struct tags.
type StructTagRule struct{}
type StructTagRule struct {
userDefined map[string][]string // map: key -> []option
sync.Mutex
}

func (r *StructTagRule) configure(arguments lint.Arguments) {
r.Lock()
defer r.Unlock()
if r.userDefined == nil && len(arguments) > 0 {
checkNumberOfArguments(1, arguments, r.Name())
r.userDefined = make(map[string][]string, len(arguments))
for _, arg := range arguments {
item, ok := arg.(string)
if !ok {
panic(fmt.Sprintf("Invalid argument to the %s rule. Expecting a string, got %v (of type %T)", r.Name(), arg, arg))
}
parts := strings.Split(item, ",")
if len(parts) < 2 {
panic(fmt.Sprintf("Invalid argument to the %s rule. Expecting a string of the form key[,option]+, got %s", r.Name(), item))
}
key := strings.TrimSpace(parts[0])
for i := 1; i < len(parts); i++ {
option := strings.TrimSpace(parts[i])
r.userDefined[key] = append(r.userDefined[key], option)
}
}
}
}

// Apply applies the rule to given file.
func (*StructTagRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {
var failures []lint.Failure
func (r *StructTagRule) Apply(file *lint.File, args lint.Arguments) []lint.Failure {
r.configure(args)

var failures []lint.Failure
onFailure := func(failure lint.Failure) {
failures = append(failures, failure)
}

w := lintStructTagRule{onFailure: onFailure}
w := lintStructTagRule{
onFailure: onFailure,
userDefined: r.userDefined,
}

ast.Walk(w, file.AST)

Expand All @@ -35,8 +67,9 @@ func (*StructTagRule) Name() string {

type lintStructTagRule struct {
onFailure func(lint.Failure)
usedTagNbr map[int]bool // list of used tag numbers
usedTagName map[string]bool // list of used tag keys
userDefined map[string][]string // map: key -> []option
usedTagNbr map[int]bool // list of used tag numbers
usedTagName map[string]bool // list of used tag keys
}

func (w lintStructTagRule) Visit(node ast.Node) ast.Visitor {
Expand All @@ -57,17 +90,26 @@ func (w lintStructTagRule) Visit(node ast.Node) ast.Visitor {
return w
}

const keyASN1 = "asn1"
const keyBSON = "bson"
const keyDefault = "default"
const keyJSON = "json"
const keyProtobuf = "protobuf"
const keyRequired = "required"
const keyXML = "xml"
const keyYAML = "yaml"

func (w lintStructTagRule) checkTagNameIfNeed(tag *structtag.Tag) (string, bool) {
isUnnamedTag := tag.Name == "" || tag.Name == "-"
if isUnnamedTag {
return "", true
}

needsToCheckTagName := tag.Key == "bson" ||
tag.Key == "json" ||
tag.Key == "xml" ||
tag.Key == "yaml" ||
tag.Key == "protobuf"
needsToCheckTagName := tag.Key == keyBSON ||
tag.Key == keyJSON ||
tag.Key == keyXML ||
tag.Key == keyYAML ||
tag.Key == keyProtobuf

if !needsToCheckTagName {
return "", true
Expand All @@ -92,7 +134,7 @@ func (w lintStructTagRule) checkTagNameIfNeed(tag *structtag.Tag) (string, bool)

func (lintStructTagRule) getTagName(tag *structtag.Tag) string {
switch tag.Key {
case "protobuf":
case keyProtobuf:
for _, option := range tag.Options {
if strings.HasPrefix(option, "name=") {
return strings.TrimLeft(option, "name=")
Expand Down Expand Up @@ -123,40 +165,40 @@ func (w lintStructTagRule) checkTaggedField(f *ast.Field) {
}

switch key := tag.Key; key {
case "asn1":
case keyASN1:
msg, ok := w.checkASN1Tag(f.Type, tag)
if !ok {
w.addFailure(f.Tag, msg)
}
case "bson":
case keyBSON:
msg, ok := w.checkBSONTag(tag.Options)
if !ok {
w.addFailure(f.Tag, msg)
}
case "default":
case keyDefault:
if !w.typeValueMatch(f.Type, tag.Name) {
w.addFailure(f.Tag, "field's type and default value's type mismatch")
}
case "json":
case keyJSON:
msg, ok := w.checkJSONTag(tag.Name, tag.Options)
if !ok {
w.addFailure(f.Tag, msg)
}
case "protobuf":
case keyProtobuf:
msg, ok := w.checkProtobufTag(tag)
if !ok {
w.addFailure(f.Tag, msg)
}
case "required":
case keyRequired:
if tag.Name != "true" && tag.Name != "false" {
w.addFailure(f.Tag, "required should be 'true' or 'false'")
}
case "xml":
case keyXML:
msg, ok := w.checkXMLTag(tag.Options)
if !ok {
w.addFailure(f.Tag, msg)
}
case "yaml":
case keyYAML:
msg, ok := w.checkYAMLTag(tag.Options)
if !ok {
w.addFailure(f.Tag, msg)
Expand Down Expand Up @@ -201,26 +243,33 @@ func (w lintStructTagRule) checkASN1Tag(t ast.Expr, tag *structtag.Tag) (string,
continue
}

if w.isUserDefined(keyASN1, opt) {
continue
}

return fmt.Sprintf("unknown option '%s' in ASN1 tag", opt), false
}
}

return "", true
}

func (lintStructTagRule) checkBSONTag(options []string) (string, bool) {
func (w lintStructTagRule) checkBSONTag(options []string) (string, bool) {
for _, opt := range options {
switch opt {
case "inline", "minsize", "omitempty":
default:
if w.isUserDefined(keyBSON, opt) {
continue
}
return fmt.Sprintf("unknown option '%s' in BSON tag", opt), false
}
}

return "", true
}

func (lintStructTagRule) checkJSONTag(name string, options []string) (string, bool) {
func (w lintStructTagRule) checkJSONTag(name string, options []string) (string, bool) {
for _, opt := range options {
switch opt {
case "omitempty", "string":
Expand All @@ -230,30 +279,39 @@ func (lintStructTagRule) checkJSONTag(name string, options []string) (string, bo
return "option can not be empty in JSON tag", false
}
default:
if w.isUserDefined(keyJSON, opt) {
continue
}
return fmt.Sprintf("unknown option '%s' in JSON tag", opt), false
}
}

return "", true
}

func (lintStructTagRule) checkXMLTag(options []string) (string, bool) {
func (w lintStructTagRule) checkXMLTag(options []string) (string, bool) {
for _, opt := range options {
switch opt {
case "any", "attr", "cdata", "chardata", "comment", "innerxml", "omitempty", "typeattr":
default:
if w.isUserDefined(keyXML, opt) {
continue
}
return fmt.Sprintf("unknown option '%s' in XML tag", opt), false
}
}

return "", true
}

func (lintStructTagRule) checkYAMLTag(options []string) (string, bool) {
func (w lintStructTagRule) checkYAMLTag(options []string) (string, bool) {
for _, opt := range options {
switch opt {
case "flow", "inline", "omitempty":
default:
if w.isUserDefined(keyYAML, opt) {
continue
}
return fmt.Sprintf("unknown option '%s' in YAML tag", opt), false
}
}
Expand Down Expand Up @@ -330,6 +388,9 @@ func (w lintStructTagRule) checkProtobufTag(tag *structtag.Tag) (string, bool) {
case "name", "json":
// do nothing
default:
if w.isUserDefined(keyProtobuf, k) {
continue
}
return fmt.Sprintf("unknown option '%s' in protobuf tag", k), false
}
}
Expand All @@ -344,3 +405,17 @@ func (w lintStructTagRule) addFailure(n ast.Node, msg string) {
Confidence: 1,
})
}

func (w lintStructTagRule) isUserDefined(key, opt string) bool {
if w.userDefined == nil {
return false
}

options := w.userDefined[key]
for _, o := range options {
if opt == o {
return true
}
}
return false
}
7 changes: 7 additions & 0 deletions test/struct-tag_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,17 @@ package test
import (
"testing"

"github.com/mgechev/revive/lint"
"github.com/mgechev/revive/rule"
)

// TestStructTag tests struct-tag rule
func TestStructTag(t *testing.T) {
testRule(t, "struct-tag", &rule.StructTagRule{})
}

func TestStructTagWithUserOptions(t *testing.T) {
testRule(t, "struct-tag-useroptions", &rule.StructTagRule{}, &lint.RuleConfig{
Arguments: []interface{}{"json,inline,outline", "bson,gnu"},
})
}
15 changes: 15 additions & 0 deletions testdata/struct-tag-useroptions.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package fixtures

type RangeAllocation struct {
metav1.TypeMeta `json:",inline"`
metav1.ObjectMeta `json:"metadata,omitempty"`
Range string `json:"range,outline"`
Data []byte `json:"data,flow"` // MATCH /unknown option 'flow' in JSON tag/
}

type RangeAllocation struct {
metav1.TypeMeta `bson:",minsize,gnu"`
metav1.ObjectMeta `bson:"metadata,omitempty"`
Range string `bson:"range,flow"` // MATCH /unknown option 'flow' in BSON tag/
Data []byte `bson:"data,inline"`
}