diff --git a/rule/struct_tag.go b/rule/struct_tag.go index edb6f95..fd8ad23 100644 --- a/rule/struct_tag.go +++ b/rule/struct_tag.go @@ -23,6 +23,7 @@ type tagKey string const ( keyASN1 tagKey = "asn1" keyBSON tagKey = "bson" + keyCodec tagKey = "codec" keyDatastore tagKey = "datastore" keyDefault tagKey = "default" keyJSON tagKey = "json" @@ -38,11 +39,12 @@ const ( keyYAML tagKey = "yaml" ) -type tagChecker func(checkCtx *checkContext, tag *structtag.Tag, fieldType ast.Expr) (message string, succeeded bool) +type tagChecker func(checkCtx *checkContext, tag *structtag.Tag, field *ast.Field) (message string, succeeded bool) var tagCheckers = map[tagKey]tagChecker{ keyASN1: checkASN1Tag, keyBSON: checkBSONTag, + keyCodec: checkCodecTag, keyDatastore: checkDatastoreTag, keyDefault: checkDefaultTag, keyJSON: checkJSONTag, @@ -62,6 +64,7 @@ type checkContext struct { userDefined map[tagKey][]string // map: key -> []option usedTagNbr map[int]bool // list of used tag numbers usedTagName map[string]bool // list of used tag keys + commonOptions map[string]bool // list of options defined for all fields isAtLeastGo124 bool } @@ -74,6 +77,23 @@ func (checkCtx checkContext) isUserDefined(key tagKey, opt string) bool { return slices.Contains(options, opt) } +func (checkCtx *checkContext) isCommonOption(opt string) bool { + if checkCtx.commonOptions == nil { + return false + } + + _, ok := checkCtx.commonOptions[opt] + return ok +} + +func (checkCtx *checkContext) addCommonOption(opt string) { + if checkCtx.commonOptions == nil { + checkCtx.commonOptions = map[string]bool{} + } + + checkCtx.commonOptions[opt] = true +} + // Configure validates the rule configuration, and configures the rule accordingly. // // Configuration implements the [lint.ConfigurableRule] interface. @@ -164,36 +184,66 @@ func (w lintStructTagRule) Visit(node ast.Node) ast.Visitor { // checkTaggedField checks the tag of the given field. // precondition: the field has a tag -func (w lintStructTagRule) checkTaggedField(checkCtx *checkContext, f *ast.Field) { - if len(f.Names) > 0 && !f.Names[0].IsExported() { - w.addFailuref(f, "tag on not-exported field %s", f.Names[0].Name) - } - - tags, err := structtag.Parse(strings.Trim(f.Tag.Value, "`")) +func (w lintStructTagRule) checkTaggedField(checkCtx *checkContext, field *ast.Field) { + tags, err := structtag.Parse(strings.Trim(field.Tag.Value, "`")) if err != nil || tags == nil { - w.addFailuref(f.Tag, "malformed tag") + w.addFailuref(field.Tag, "malformed tag") return } + analyzedTags := map[tagKey]struct{}{} for _, tag := range tags.Tags() { if msg, ok := w.checkTagNameIfNeed(checkCtx, tag); !ok { - w.addFailureWithTagKey(f.Tag, msg, tag.Key) + w.addFailureWithTagKey(field.Tag, msg, tag.Key) } if msg, ok := checkOptionsOnIgnoredField(tag); !ok { - w.addFailureWithTagKey(f.Tag, msg, tag.Key) + w.addFailureWithTagKey(field.Tag, msg, tag.Key) } - checker, ok := w.tagCheckers[tagKey(tag.Key)] + key := tagKey(tag.Key) + checker, ok := w.tagCheckers[key] if !ok { continue // we don't have a checker for the tag } - msg, ok := checker(checkCtx, tag, f.Type) + msg, ok := checker(checkCtx, tag, field) if !ok { - w.addFailureWithTagKey(f.Tag, msg, tag.Key) + w.addFailureWithTagKey(field.Tag, msg, tag.Key) + } + + analyzedTags[key] = struct{}{} + } + + if w.shallWarnOnUnexportedField(field.Names, analyzedTags) { + w.addFailuref(field, "tag on not-exported field %s", field.Names[0].Name) + } +} + +// tagKeyToSpecialField maps tag keys to their "special" meaning struct fields. +var tagKeyToSpecialField = map[tagKey]string{ + "codec": structTagCodecSpecialField, +} + +func (lintStructTagRule) shallWarnOnUnexportedField(fieldNames []*ast.Ident, tags map[tagKey]struct{}) bool { + if len(fieldNames) != 1 { // only handle the case of single field name (99.999% of cases) + return false + } + + if fieldNames[0].IsExported() { + return false + } + + fieldNameStr := fieldNames[0].Name + + for key := range tags { + specialField, ok := tagKeyToSpecialField[key] + if ok && specialField == fieldNameStr { + return false } } + + return true } func (w lintStructTagRule) checkTagNameIfNeed(checkCtx *checkContext, tag *structtag.Tag) (message string, succeeded bool) { @@ -204,7 +254,7 @@ func (w lintStructTagRule) checkTagNameIfNeed(checkCtx *checkContext, tag *struc key := tagKey(tag.Key) switch key { - case keyBSON, keyJSON, keyXML, keyYAML, keyProtobuf, keySpanner: + case keyBSON, keyCodec, keyJSON, keyProtobuf, keySpanner, keyXML, keyYAML: // keys that need to check for duplicated tags default: return "", true } @@ -241,7 +291,8 @@ func (lintStructTagRule) getTagName(tag *structtag.Tag) string { } } -func checkASN1Tag(checkCtx *checkContext, tag *structtag.Tag, fieldType ast.Expr) (message string, succeeded bool) { +func checkASN1Tag(checkCtx *checkContext, tag *structtag.Tag, field *ast.Field) (message string, succeeded bool) { + fieldType := field.Type checkList := slices.Concat(tag.Options, []string{tag.Name}) for _, opt := range checkList { switch opt { @@ -282,7 +333,7 @@ func checkCompoundANS1Option(checkCtx *checkContext, opt string, fieldType ast.E return "", true } -func checkDatastoreTag(checkCtx *checkContext, tag *structtag.Tag, _ ast.Expr) (message string, succeeded bool) { +func checkDatastoreTag(checkCtx *checkContext, tag *structtag.Tag, _ *ast.Field) (message string, succeeded bool) { for _, opt := range tag.Options { switch opt { case "flatten", "noindex", "omitempty": @@ -297,15 +348,15 @@ func checkDatastoreTag(checkCtx *checkContext, tag *structtag.Tag, _ ast.Expr) ( return "", true } -func checkDefaultTag(_ *checkContext, tag *structtag.Tag, fieldType ast.Expr) (message string, succeeded bool) { - if !typeValueMatch(fieldType, tag.Name) { +func checkDefaultTag(_ *checkContext, tag *structtag.Tag, field *ast.Field) (message string, succeeded bool) { + if !typeValueMatch(field.Type, tag.Name) { return msgTypeMismatch, false } return "", true } -func checkBSONTag(checkCtx *checkContext, tag *structtag.Tag, _ ast.Expr) (message string, succeeded bool) { +func checkBSONTag(checkCtx *checkContext, tag *structtag.Tag, _ *ast.Field) (message string, succeeded bool) { for _, opt := range tag.Options { switch opt { case "inline", "minsize", "omitempty": @@ -320,7 +371,32 @@ func checkBSONTag(checkCtx *checkContext, tag *structtag.Tag, _ ast.Expr) (messa return "", true } -func checkJSONTag(checkCtx *checkContext, tag *structtag.Tag, _ ast.Expr) (message string, succeeded bool) { +const structTagCodecSpecialField = "_struct" + +func checkCodecTag(checkCtx *checkContext, tag *structtag.Tag, field *ast.Field) (message string, succeeded bool) { + fieldNames := field.Names + mustAddToCommonOptions := len(fieldNames) == 1 && fieldNames[0].Name == structTagCodecSpecialField // see https://github.com/mgechev/revive/issues/1477#issuecomment-3191493076 + for _, opt := range tag.Options { + if mustAddToCommonOptions { + checkCtx.addCommonOption(opt) + } else if checkCtx.isCommonOption(opt) { + return fmt.Sprintf("redundant option %q, already set for all fields", opt), false + } + + switch opt { + case "omitempty", "toarray", "int", "uint", "float", "-", "omitemptyarray": + default: + if checkCtx.isUserDefined(keyCodec, opt) { + continue + } + return fmt.Sprintf(msgUnknownOption, opt), false + } + } + + return "", true +} + +func checkJSONTag(checkCtx *checkContext, tag *structtag.Tag, _ *ast.Field) (message string, succeeded bool) { for _, opt := range tag.Options { switch opt { case "omitempty", "string": @@ -345,7 +421,7 @@ func checkJSONTag(checkCtx *checkContext, tag *structtag.Tag, _ ast.Expr) (messa return "", true } -func checkMapstructureTag(checkCtx *checkContext, tag *structtag.Tag, _ ast.Expr) (message string, succeeded bool) { +func checkMapstructureTag(checkCtx *checkContext, tag *structtag.Tag, _ *ast.Field) (message string, succeeded bool) { for _, opt := range tag.Options { switch opt { case "omitempty", "reminder", "squash": @@ -360,13 +436,14 @@ func checkMapstructureTag(checkCtx *checkContext, tag *structtag.Tag, _ ast.Expr return "", true } -func checkPropertiesTag(_ *checkContext, tag *structtag.Tag, fieldType ast.Expr) (message string, succeeded bool) { +func checkPropertiesTag(_ *checkContext, tag *structtag.Tag, field *ast.Field) (message string, succeeded bool) { options := tag.Options if len(options) == 0 { return "", true } seenOptions := map[string]bool{} + fieldType := field.Type for _, opt := range options { msg, ok := fmt.Sprintf("unknown or malformed option %q", opt), false if key, value, found := strings.Cut(opt, "="); found { @@ -405,7 +482,7 @@ func checkCompoundPropertiesOption(key, value string, fieldType ast.Expr, seenOp return "", true } -func checkProtobufTag(checkCtx *checkContext, tag *structtag.Tag, _ ast.Expr) (message string, succeeded bool) { +func checkProtobufTag(checkCtx *checkContext, tag *structtag.Tag, _ *ast.Field) (message string, succeeded bool) { // check name switch tag.Name { case "bytes", "fixed32", "fixed64", "group", "varint", "zigzag32", "zigzag64": @@ -458,7 +535,7 @@ func checkProtobufOptions(checkCtx *checkContext, options []string) (message str return "", true } -func checkRequiredTag(_ *checkContext, tag *structtag.Tag, _ ast.Expr) (message string, succeeded bool) { +func checkRequiredTag(_ *checkContext, tag *structtag.Tag, _ *ast.Field) (message string, succeeded bool) { switch tag.Name { case "true", "false": return "", true @@ -467,7 +544,7 @@ func checkRequiredTag(_ *checkContext, tag *structtag.Tag, _ ast.Expr) (message } } -func checkTOMLTag(checkCtx *checkContext, tag *structtag.Tag, _ ast.Expr) (message string, succeeded bool) { +func checkTOMLTag(checkCtx *checkContext, tag *structtag.Tag, _ *ast.Field) (message string, succeeded bool) { for _, opt := range tag.Options { switch opt { case "omitempty": @@ -482,7 +559,7 @@ func checkTOMLTag(checkCtx *checkContext, tag *structtag.Tag, _ ast.Expr) (messa return "", true } -func checkURLTag(checkCtx *checkContext, tag *structtag.Tag, _ ast.Expr) (message string, succeeded bool) { +func checkURLTag(checkCtx *checkContext, tag *structtag.Tag, _ *ast.Field) (message string, succeeded bool) { var delimiter = "" for _, opt := range tag.Options { switch opt { @@ -505,7 +582,7 @@ func checkURLTag(checkCtx *checkContext, tag *structtag.Tag, _ ast.Expr) (messag return "", true } -func checkValidateTag(checkCtx *checkContext, tag *structtag.Tag, _ ast.Expr) (message string, succeeded bool) { +func checkValidateTag(checkCtx *checkContext, tag *structtag.Tag, _ *ast.Field) (message string, succeeded bool) { previousOption := "" seenKeysOption := false options := append([]string{tag.Name}, tag.Options...) @@ -534,7 +611,7 @@ func checkValidateTag(checkCtx *checkContext, tag *structtag.Tag, _ ast.Expr) (m return "", true } -func checkXMLTag(checkCtx *checkContext, tag *structtag.Tag, _ ast.Expr) (message string, succeeded bool) { +func checkXMLTag(checkCtx *checkContext, tag *structtag.Tag, _ *ast.Field) (message string, succeeded bool) { for _, opt := range tag.Options { switch opt { case "any", "attr", "cdata", "chardata", "comment", "innerxml", "omitempty", "typeattr": @@ -549,7 +626,7 @@ func checkXMLTag(checkCtx *checkContext, tag *structtag.Tag, _ ast.Expr) (messag return "", true } -func checkYAMLTag(checkCtx *checkContext, tag *structtag.Tag, _ ast.Expr) (message string, succeeded bool) { +func checkYAMLTag(checkCtx *checkContext, tag *structtag.Tag, _ *ast.Field) (message string, succeeded bool) { for _, opt := range tag.Options { switch opt { case "flow", "inline", "omitempty": @@ -564,7 +641,7 @@ func checkYAMLTag(checkCtx *checkContext, tag *structtag.Tag, _ ast.Expr) (messa return "", true } -func checkSpannerTag(checkCtx *checkContext, tag *structtag.Tag, _ ast.Expr) (message string, succeeded bool) { +func checkSpannerTag(checkCtx *checkContext, tag *structtag.Tag, _ *ast.Field) (message string, succeeded bool) { for _, opt := range tag.Options { if !checkCtx.isUserDefined(keySpanner, opt) { return fmt.Sprintf(msgUnknownOption, opt), false diff --git a/test/struct_tag_test.go b/test/struct_tag_test.go index 40cc8a8..a645bde 100644 --- a/test/struct_tag_test.go +++ b/test/struct_tag_test.go @@ -22,6 +22,7 @@ func TestStructTagWithUserOptions(t *testing.T) { "validate,displayName", "toml,unknown", "spanner,mySpannerOption", + "codec,myCodecOption", }, }) } diff --git a/testdata/struct_tag.go b/testdata/struct_tag.go index 432fc51..1fc551f 100644 --- a/testdata/struct_tag.go +++ b/testdata/struct_tag.go @@ -196,3 +196,21 @@ type SpannerUser struct { CreatedAt time.Time `spanner:"created_at"` UpdatedAt time.Time `spanner:"updated_at,unknown"` // MATCH /unknown option "unknown" in spanner tag/ } + +type Codec struct { + _something struct{} `codec:",omitempty,int"` // MATCH /tag on not-exported field _something/ + _struct struct{} `codec:",omitempty,int"` // do not match, _struct has special meaning for codec tag + Field1 string `codec:"-"` + Field2 int `codec:"myName"` + Field3 int32 `codec:",omitempty"` // MATCH /redundant option "omitempty", already set for all fields in codec tag/ + Field4 bool `codec:"f4,int"` // MATCH /redundant option "int", already set for all fields in codec tag/ + field5 bool // unexported, so skipped + Anon +} + +type TestDuplicatedCodecTags struct { + _struct struct{} `json:",omitempty"` // MATCH /tag on not-exported field _struct/ + A int `codec:"field_a"` + B int `codec:"field_a"` // MATCH /duplicated tag name "field_a" in codec tag/ + C int `codec:"field_c"` +} diff --git a/testdata/struct_tag_user_options.go b/testdata/struct_tag_user_options.go index 32bd830..a60fa3d 100644 --- a/testdata/struct_tag_user_options.go +++ b/testdata/struct_tag_user_options.go @@ -97,3 +97,8 @@ type uselessOptions struct { // MATCH:83 /unknown option "" in xml tag/ // MATCH:86 /unknown option "" in yaml tag/ } + +type CodecUserOptions struct { + ID int `codec:"user_id,myCodecOption"` + Name string `codec:"full_name,unknownOption"` // MATCH /unknown option "unknownOption" in codec tag/ +}