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: duplicate struct field tag check does not handle XMLName #18256

Open
tdilo opened this Issue Dec 8, 2016 · 3 comments

Comments

Projects
None yet
5 participants
@tdilo
Contributor

tdilo commented Dec 8, 2016

In Go 1.8beta1 a new check to detect duplicate names used in struct field tags was introduced to go vet (CL 16704). This check produces false positive warnings when XML attributes and the special XMLName field name are used:

  • When fields are encoded as XML attributes, a warning is produced when an attribute reuses a name previously used for an element.
type Foo struct {
    First int `xml:"a"
    NoDup int `xml:"a,attr"` // warning about reuse of "a"
}
  • When XMLName is used to set the name of the enclosing struct element, it is treated as a regular struct field.
type Bar struct {
    XMLName xml.Name `xml:"a"`
    NoDup   int      `xml:"a"` // warning about reuse of "a"
}

Both cases are addressed in CL 34070.

The original implementation does not take into account the specific behavior of encoding/xml regarding the XMLName field, and the fix only does so in order to eliminate false positive warnings.

No warning is currently generated for a struct field with XMLName field:

type Foo struct {
    Bar int `xml:"a"`
    Baz struct {
        XMLName xml.Name `xml:"a"`
    }
}

As go vet generates a warning about other naming collisions in tags, it should generate a warning in this case as well.

I am currently working on an implementation.

EDIT: added detailed description of original problem addressed by CL 34070

@bradfitz bradfitz added this to the Go1.8Maybe milestone Dec 9, 2016

@bradfitz

This comment has been minimized.

Member

bradfitz commented Dec 9, 2016

If you have a fix ready soon and it's not invasive, then we could perhaps still get it in for 1.8.

@bradfitz bradfitz modified the milestones: Go1.9, Go1.8Maybe Dec 12, 2016

@quentinmit quentinmit modified the milestones: Go1.8Maybe, Go1.9 Dec 12, 2016

@gopherbot

This comment has been minimized.

gopherbot commented Dec 12, 2016

CL https://golang.org/cl/34070 mentions this issue.

@tdilo

This comment has been minimized.

Contributor

tdilo commented Dec 13, 2016

Detecting duplicate names defined through XMLName is harder than anticipated. An example:

type Foo struct {
        DupAnon struct {
                XMLName xml.Name `xml:"a"` // error: duplicate of DupField (last in struct)
                // Arguably it might not be a duplicate but the first occurrence of 'a'.
        }
        DupAttr1 Bar // no tag here, but an error nonetheless (also AST traversal trainwreck).
        // Report error for line above? (see Bar)
        DupAttr2 Qux // same as above, but adds complexity for detection:
        // XMLName tags cannot simply be read-only with regard to seen map in current implementation.
        DupField int `xml:"a"` // Duplicate of DupAnon or first real occurrence of 'a'?
}

type Bar struct {
        XMLName xml.Name `xml:"a"` // should the error be attributed to this line?
}

type Qux struct {
        XMLName xml.Name `xml:"a"` // same as Bar
}

By now I have a hacky proof of concept using a two-pass approach that does not detect DupAttr2 and generates duplicate error messages when one of the duplicate tags is malformed. On the upside, that approach mostly leaves checkCanonicalFieldTag untouched and just adds a new function and some plumbing.

All in all I do not think that I will be able to get this ready in time for Go 1.8. As the original issue of false warnings is addressed I do not think it is critical to include it either.

gopherbot pushed a commit that referenced this issue Dec 13, 2016

cmd/vet: fix panic and handling of XML in struct field tag check
The check for duplicate struct field tags introduced in CL 16704
triggers a panic when an anonymous struct field with a duplicate name
is encountered. For such a field, the names slice of the ast.Field is
nil but accessed regardless to generate the warning message.

Additionally, the check produces false positives for XML tags in some
cases:

- When fields are encoded as XML attributes, a warning is produced when
  an attribute reuses a name previously used for an element.

  Example:
    type Foo struct {
        First int `xml:"a"`
        NoDup int `xml:"a,attr"` // warning about reuse of "a"
    }

- When XMLName is used to set the name of the enclosing struct element,
  it is treated as a regular struct field.

  Example:
    type Bar struct {
        XMLName xml.Name `xml:"a"`
        NoDup   int      `xml:"a"` // warning about reuse of "a"
    }

This commit addresses all three issues. The panic is avoided by using
the type name instead of the field name for anonymous struct fields when
generating the warning message. An additional namespace for checking XML
attribute names separately from element names is introduced. Lastly,
fields named XMLName are excluded from the check for duplicate tags.

Updates #18256

Change-Id: Ida48ea8584b56bd4d12ae3ebd588a66ced2594cc
Reviewed-on: https://go-review.googlesource.com/34070
Run-TryBot: Rob Pike <r@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>

@bradfitz bradfitz modified the milestones: Go1.9, Go1.8Maybe Dec 15, 2016

@rsc rsc modified the milestones: Go1.10, Go1.9 Jun 22, 2017

@rsc rsc modified the milestones: Go1.10, Go1.11 Nov 22, 2017

@gopherbot gopherbot modified the milestones: Go1.11, Unplanned May 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment