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

cmd/vet: structtag false positives about json:"-,omitempty" for unexported fields #51298

Open
dolmen opened this issue Feb 21, 2022 · 7 comments
Open
Labels
NeedsFix

Comments

@dolmen
Copy link
Contributor

@dolmen dolmen commented Feb 21, 2022

go vet -structtag skips private fields with JSON tag -, but doesn't skip those with tag -,omitempty. This is inconsistent.

What version of Go are you using (go version)?

$ go version
1.17.6

Does this issue reproduce with the latest release?

Yes, also with go1.18beta2.

What did you do?

package main

type X struct {
	a int `json:"-"`
	b int `json:"-"`
	c int `json:"-,omitempty"`
	d int `json:"-,omitempty"`
}

func main() {
}

What did you see?

$ go vet -structtag main.go
# command-line-arguments
./main.go:6:2: struct field c has json tag but is not exported
./main.go:7:2: struct field d repeats json tag "-" also at main.go:6
./main.go:7:2: struct field d has json tag but is not exported

What did you expect to see?

No error.

If a and b are not reported, neither should c or d.

Or maybe the error should be about the presence of omitempty.

@mengzhuo
Copy link
Contributor

@mengzhuo mengzhuo commented Feb 21, 2022

I think the root cause is in the tools.

github.com/golang/tools/go/analysis/passes/structtag

	for _, enc := range [...]string{"json", "xml"} {
		switch reflect.StructTag(tag).Get(enc) {
		// Ignore warning if the field not exported and the tag is marked as
		// ignored.
		case "", "-":
		default:
			pass.Reportf(field.Pos(), "struct field %s has %s tag but is not exported", field.Name(), enc)
			return
		}
	}

@mengzhuo mengzhuo added the NeedsFix label Feb 21, 2022
@gopherbot
Copy link

@gopherbot gopherbot commented Feb 21, 2022

Change https://go.dev/cl/387114 mentions this issue: go/analysis/passes/structtag: ignore "-" fields

@timothy-king
Copy link
Contributor

@timothy-king timothy-king commented Feb 22, 2022

My understanding is that the vet check is WAI.

c int `json:"-,omitempty"`

is naming the unexported field of X c as "-" within the json and adding the "omitempty" option to it. The same is then done with d. This also repeats a json name "-".

This is my reading of https://pkg.go.dev/encoding/json:

The encoding of each struct field can be customized by the format string stored under the "json" key in the struct field's tag. The format string gives the name of the field, possibly followed by a comma-separated list of options. The name may be empty in order to specify options without overriding the default field name.

and

As a special case, if the field tag is "-", the field is always omitted. Note that a field with name "-" can still be generated using the tag "-,".

Someone authoritative on "encoding/json" want to weight in?

If this is WAI, we might want to add additional checks so this case is easier to understand. Open to suggestions about how. (Complain about using "-" on "a" and "b" as they are unexported?) I think new checks would need a proposal.

@mengzhuo
Copy link
Contributor

@mengzhuo mengzhuo commented Feb 22, 2022

cc owner @rsc https://dev.golang.org/owners

@guodongli-google
Copy link

@guodongli-google guodongli-google commented Feb 22, 2022

is naming the unexported field of X c as "-" within the json and adding the "omitempty" option to it. The same is then done with d. This also repeats a json name "-".

Second this. "c" is interpreted as a json field with name "-" and an extra tag "omitempty"

  c int `json:"-,omitempty"`

In addition, using json:"-,omitempty" seems to be improper since "-" indicates to skip the field and "omitempty" indicates to skip the field when it is empty. It suffices to have only the "-". So this warning correctly reports something that shall be rectified.

@mengzhuo
Copy link
Contributor

@mengzhuo mengzhuo commented Mar 8, 2022

I agree with @timothy-king,
"-" is an valid field name according to the document which we should do a duplicate checks on.

https://pkg.go.dev/encoding/json@go1.17.8

// Field appears in JSON as key "-".
Field int `json:"-,"``

Is it ok to close this issue?

@timothy-king
Copy link
Contributor

@timothy-king timothy-king commented Mar 8, 2022

Case split in my mind:

  1. If we think this is WAI, closing this issue SGTM.
  2. If we think we want better diagnostics but not new ones, we can keep this open and close after we improve them.
  3. If we think we want new diagnostics, I would close this and open a proposal.

Line between "better" and "new" is fuzzy. But I would put warn on "-" on any exported field as "new" while having a special case for when a duplicate json tag is named "-" would be "better". Such as struct field d repeats json tag "-" also at main.go:6 (Note: json:"-," renames d to "-").

Open to all three choices though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix
Projects
None yet
Development

No branches or pull requests

5 participants