cmd/vet: check for use of json/xml struct tags with unexported fields
This is a common source of bugs, particularly for those new to Go. Running this on a corpus of public code flagged 114 instances.
This check may need to be updated once issue 7363 is resolved.
R=r Nice. Assigning review to Rob. On Tue, Jun 10, 2014 at 6:30 PM, <josharian@gmail.com> ...
9 years, 9 months ago
(2014-06-11 18:29:42 UTC)
#2
R=r
Nice. Assigning review to Rob.
On Tue, Jun 10, 2014 at 6:30 PM, <josharian@gmail.com> wrote:
> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go.tools
>
>
> Description:
> cmd/vet: check for use of json/xml struct tags with unexported fields
>
> This is a common source of bugs, particularly for those new to Go.
> Running this on a corpus of public code flagged 114 instances.
>
> Please review this at https://codereview.appspot.com/91010047/
>
> Affected files (+37, -2 lines):
> M cmd/vet/doc.go
> M cmd/vet/main.go
> M cmd/vet/structtag.go
> M cmd/vet/testdata/structtag.go
>
>
> Index: cmd/vet/doc.go
> ===================================================================
> --- a/cmd/vet/doc.go
> +++ b/cmd/vet/doc.go
> @@ -72,6 +72,7 @@
> Flag: -structtags
>
> Struct tags that do not follow the format understood by
> reflect.StructTag.Get.
> +Well-known encoding struct tags (json, xml) used with unexported fields.
>
> Unkeyed composite literals
>
> Index: cmd/vet/main.go
> ===================================================================
> --- a/cmd/vet/main.go
> +++ b/cmd/vet/main.go
> @@ -49,7 +49,7 @@
> "printf": triStateFlag("printf", unset, "check printf-like
> invocations"),
> "rangeloops": triStateFlag("rangeloops", unset, "check that range
> loop variables are used correctly"),
> "shadow": triStateFlag("shadow", unset, "check for shadowed
> variables (experimental; must be set explicitly)"),
> - "structtags": triStateFlag("structtags", unset, "check that
> struct field tags have canonical format"),
> + "structtags": triStateFlag("structtags", unset, "check that
> struct field tags have canonical format and are exported if needed"),
> "unreachable": triStateFlag("unreachable", unset, "check for
> unreachable code"),
> "unsafeptr": triStateFlag("unsafeptr", unset, "check for misuse
> of unsafe.Pointer"),
> }
> Index: cmd/vet/structtag.go
> ===================================================================
> --- a/cmd/vet/structtag.go
> +++ b/cmd/vet/structtag.go
> @@ -30,8 +30,28 @@
> // Check tag for validity by appending
> // new key:value to end and checking that
> // the tag parsing code can find it.
> - if reflect.StructTag(tag+` _gofix:"_magic"`).Get("_gofix") !=
> "_magic" {
> + st := reflect.StructTag(tag + ` _gofix:"_magic"`)
> + if st.Get("_gofix") != "_magic" {
> f.Badf(field.Pos(), "struct field tag %s not compatible
> with reflect.StructTag.Get", field.Tag.Value)
> return
> }
> +
> + // Check for use of json or xml tags with unexported fields.
> +
> + // Embedded struct. Nothing to do for now, but that
> + // may change, depending on what happens with issue 7363.
> + if len(field.Names) == 0 {
> + return
> + }
> +
> + if field.Names[0].IsExported() {
> + return
> + }
> +
> + for _, enc := range [...]string{"json", "xml"} {
> + if st.Get(enc) != "" {
> + f.Badf(field.Pos(), "struct field %s has %s tag
> but is not exported", field.Names[0].Name, enc)
> + return
> + }
> + }
> }
> Index: cmd/vet/testdata/structtag.go
> ===================================================================
> --- a/cmd/vet/testdata/structtag.go
> +++ b/cmd/vet/testdata/structtag.go
> @@ -11,3 +11,17 @@
> type StructTagTest struct {
> X int "hello" // ERROR "not compatible with reflect.StructTag.Get"
> }
> +
> +type UnexportedEncodingTagTest struct {
> + x int `json:"xx"` // ERROR "struct field x has json tag but is not
> exported"
> + y int `xml:"yy"` // ERROR "struct field y has xml tag but is not
> exported"
> + z int
> + A int `json:"aa" xml:"bb"`
> +}
> +
> +type unexp struct{}
> +
> +type Embedded struct {
> + UnexportedEncodingTagTest `is:"embedded"`
> + unexp `is:"embedded,notexported" json:"unexp"`
> // OK for now, see issue 7363
> +}
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>
https://codereview.appspot.com/91010047/diff/130001/cmd/vet/structtag.go File cmd/vet/structtag.go (right): https://codereview.appspot.com/91010047/diff/130001/cmd/vet/structtag.go#newcode17 cmd/vet/structtag.go:17: "check that struct field tags have canonical format and ...
9 years, 9 months ago
(2014-06-13 22:05:41 UTC)
#4
*** Submitted as https://code.google.com/p/go/source/detail?r=dfa149f4c2fc&repo=tools *** cmd/vet: check for use of json/xml struct tags with unexported ...
9 years, 9 months ago
(2014-06-14 01:44:34 UTC)
#7
*** Submitted as
https://code.google.com/p/go/source/detail?r=dfa149f4c2fc&repo=tools ***
cmd/vet: check for use of json/xml struct tags with unexported fields
This is a common source of bugs, particularly for those new to Go. Running this
on a corpus of public code flagged 114 instances.
This check may need to be updated once issue 7363 is resolved.
LGTM=r
R=golang-codereviews, r
CC=bradfitz, golang-codereviews
https://codereview.appspot.com/91010047
Issue 91010047: code review 91010047: cmd/vet: check for use of json/xml struct tags with une...
(Closed)
Created 9 years, 11 months ago by josharian
Modified 9 years, 9 months ago
Reviewers:
Base URL:
Comments: 6