From a7f4cb12e7abc62d4f73893127284d7e840cc853 Mon Sep 17 00:00:00 2001 From: Dominik Honnef Date: Sun, 31 Oct 2021 23:05:28 +0100 Subject: [PATCH] SA1026, SA5008: reimplement XML checking using encoding/xml rules Same as previous commit, but for XML instead of JSON. Because we rely on encoding/xml, we can now flag a plethora of invalid struct tags. Closes gh-1088 (cherry picked from commit a62bc8eaa02612318e45388d7c83b07e93f86523) --- staticcheck/fakexml/marshal.go | 375 +++++++++++++++++ staticcheck/fakexml/typeinfo.go | 383 ++++++++++++++++++ staticcheck/fakexml/xml.go | 33 ++ staticcheck/lint.go | 90 ++-- .../src/CheckStructTags/CheckStructTags.go | 26 +- .../src/CheckStructTags3/CheckStructTags.go | 5 +- .../CheckUnsupportedMarshal.go | 85 +++- 7 files changed, 921 insertions(+), 76 deletions(-) create mode 100644 staticcheck/fakexml/marshal.go create mode 100644 staticcheck/fakexml/typeinfo.go create mode 100644 staticcheck/fakexml/xml.go diff --git a/staticcheck/fakexml/marshal.go b/staticcheck/fakexml/marshal.go new file mode 100644 index 000000000..a5ff21d62 --- /dev/null +++ b/staticcheck/fakexml/marshal.go @@ -0,0 +1,375 @@ +// Copyright 2011 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// This file contains a modified copy of the encoding/xml encoder. +// All dynamic behavior has been removed, and reflecttion has been replaced with go/types. +// This allows us to statically find unmarshable types +// with the same rules for tags, shadowing and addressability as encoding/xml. +// This is used for SA1026 and SA5008. + +// NOTE(dh): we do not check CanInterface in various places, which means we'll accept more marshaler implementations than encoding/xml does. This will lead to a small amount of false negatives. + +package fakexml + +import ( + "fmt" + "go/token" + "go/types" + + "honnef.co/go/tools/go/types/typeutil" + "honnef.co/go/tools/staticcheck/fakereflect" +) + +func Marshal(v types.Type) error { + return NewEncoder().Encode(v) +} + +type Encoder struct { + seen map[fakereflect.TypeAndCanAddr]struct{} +} + +func NewEncoder() *Encoder { + e := &Encoder{ + seen: map[fakereflect.TypeAndCanAddr]struct{}{}, + } + return e +} + +func (enc *Encoder) Encode(v types.Type) error { + rv := fakereflect.TypeAndCanAddr{Type: v} + return enc.marshalValue(rv, nil, nil, "x") +} + +func implementsMarshaler(v fakereflect.TypeAndCanAddr) bool { + t := v.Type + named, ok := t.(*types.Named) + if !ok { + return false + } + obj, _, _ := types.LookupFieldOrMethod(named, false, nil, "MarshalXML") + if obj == nil { + return false + } + fn, ok := obj.(*types.Func) + if !ok { + return false + } + params := fn.Type().(*types.Signature).Params() + if params.Len() != 2 { + return false + } + if !typeutil.IsType(params.At(0).Type(), "*encoding/xml.Encoder") { + return false + } + if !typeutil.IsType(params.At(1).Type(), "encoding/xml.StartElement") { + return false + } + rets := fn.Type().(*types.Signature).Results() + if rets.Len() != 1 { + return false + } + if !typeutil.IsType(rets.At(0).Type(), "error") { + return false + } + return true +} + +func implementsMarshalerAttr(v fakereflect.TypeAndCanAddr) bool { + t := v.Type + named, ok := t.(*types.Named) + if !ok { + return false + } + obj, _, _ := types.LookupFieldOrMethod(named, false, nil, "MarshalXMLAttr") + if obj == nil { + return false + } + fn, ok := obj.(*types.Func) + if !ok { + return false + } + params := fn.Type().(*types.Signature).Params() + if params.Len() != 1 { + return false + } + if !typeutil.IsType(params.At(0).Type(), "encoding/xml.Name") { + return false + } + rets := fn.Type().(*types.Signature).Results() + if rets.Len() != 2 { + return false + } + if !typeutil.IsType(rets.At(0).Type(), "encoding/xml.Attr") { + return false + } + if !typeutil.IsType(rets.At(1).Type(), "error") { + return false + } + return true +} + +var textMarshalerType = types.NewInterfaceType([]*types.Func{ + types.NewFunc(token.NoPos, nil, "MarshalText", types.NewSignature(nil, + types.NewTuple(), + types.NewTuple( + types.NewVar(token.NoPos, nil, "", types.NewSlice(types.Typ[types.Byte])), + types.NewVar(0, nil, "", types.Universe.Lookup("error").Type())), + false, + )), +}, nil).Complete() + +var N = 0 + +// marshalValue writes one or more XML elements representing val. +// If val was obtained from a struct field, finfo must have its details. +func (e *Encoder) marshalValue(val fakereflect.TypeAndCanAddr, finfo *fieldInfo, startTemplate *StartElement, stack string) error { + if _, ok := e.seen[val]; ok { + return nil + } + e.seen[val] = struct{}{} + + // Drill into interfaces and pointers. + for val.IsInterface() || val.IsPtr() { + if val.IsInterface() { + return nil + } + val = val.Elem() + } + + // Check for marshaler. + if implementsMarshaler(val) { + return nil + } + if val.CanAddr() { + pv := fakereflect.PtrTo(val) + if implementsMarshaler(pv) { + return nil + } + } + + // Check for text marshaler. + if val.Implements(textMarshalerType) { + return nil + } + if val.CanAddr() { + pv := fakereflect.PtrTo(val) + if pv.Implements(textMarshalerType) { + return nil + } + } + + // Slices and arrays iterate over the elements. They do not have an enclosing tag. + if (val.IsSlice() || val.IsArray()) && !isByteArray(val) && !isByteSlice(val) { + if err := e.marshalValue(val.Elem(), finfo, startTemplate, stack+"[0]"); err != nil { + return err + } + return nil + } + + tinfo, err := getTypeInfo(val) + if err != nil { + return err + } + + // Create start element. + // Precedence for the XML element name is: + // 0. startTemplate + // 1. XMLName field in underlying struct; + // 2. field name/tag in the struct field; and + // 3. type name + var start StartElement + + if startTemplate != nil { + start.Name = startTemplate.Name + start.Attr = append(start.Attr, startTemplate.Attr...) + } else if tinfo.xmlname != nil { + xmlname := tinfo.xmlname + if xmlname.name != "" { + start.Name.Space, start.Name.Local = xmlname.xmlns, xmlname.name + } + } + + // Attributes + for i := range tinfo.fields { + finfo := &tinfo.fields[i] + if finfo.flags&fAttr == 0 { + continue + } + fv := finfo.value(val) + + name := Name{Space: finfo.xmlns, Local: finfo.name} + if err := e.marshalAttr(&start, name, fv, stack+pathByIndex(val, finfo.idx)); err != nil { + return err + } + } + + if val.IsStruct() { + return e.marshalStruct(tinfo, val, stack) + } else { + return e.marshalSimple(val, stack) + } +} + +func isSlice(v fakereflect.TypeAndCanAddr) bool { + _, ok := v.Type.Underlying().(*types.Slice) + return ok +} + +func isByteSlice(v fakereflect.TypeAndCanAddr) bool { + slice, ok := v.Type.Underlying().(*types.Slice) + if !ok { + return false + } + basic, ok := slice.Elem().Underlying().(*types.Basic) + if !ok { + return false + } + return basic.Kind() == types.Uint8 +} + +func isByteArray(v fakereflect.TypeAndCanAddr) bool { + slice, ok := v.Type.Underlying().(*types.Array) + if !ok { + return false + } + basic, ok := slice.Elem().Underlying().(*types.Basic) + if !ok { + return false + } + return basic.Kind() == types.Uint8 +} + +// marshalAttr marshals an attribute with the given name and value, adding to start.Attr. +func (e *Encoder) marshalAttr(start *StartElement, name Name, val fakereflect.TypeAndCanAddr, stack string) error { + if implementsMarshalerAttr(val) { + return nil + } + + if val.CanAddr() { + pv := fakereflect.PtrTo(val) + if implementsMarshalerAttr(pv) { + return nil + } + } + + if val.Implements(textMarshalerType) { + return nil + } + + if val.CanAddr() { + pv := fakereflect.PtrTo(val) + if pv.Implements(textMarshalerType) { + return nil + } + } + + // Dereference or skip nil pointer + if val.IsPtr() { + val = val.Elem() + } + + // Walk slices. + if isSlice(val) && !isByteSlice(val) { + if err := e.marshalAttr(start, name, val.Elem(), stack+"[0]"); err != nil { + return err + } + return nil + } + + if typeutil.IsType(val.Type, "encoding/xml.Attr") { + return nil + } + + return e.marshalSimple(val, stack) +} + +func (e *Encoder) marshalSimple(val fakereflect.TypeAndCanAddr, stack string) error { + switch val.Type.Underlying().(type) { + case *types.Basic, *types.Interface: + return nil + case *types.Slice, *types.Array: + basic, ok := val.Elem().Type.Underlying().(*types.Basic) + if !ok || basic.Kind() != types.Uint8 { + return &UnsupportedTypeError{val.Type, stack} + } + return nil + default: + return &UnsupportedTypeError{val.Type, stack} + } +} + +func indirect(vf fakereflect.TypeAndCanAddr) fakereflect.TypeAndCanAddr { + for vf.IsPtr() { + vf = vf.Elem() + } + return vf +} + +func pathByIndex(t fakereflect.TypeAndCanAddr, index []int) string { + path := "" + for _, i := range index { + if t.IsPtr() { + t = t.Elem() + } + path += "." + t.Field(i).Name + t = t.Field(i).Type + } + return path +} + +func (e *Encoder) marshalStruct(tinfo *typeInfo, val fakereflect.TypeAndCanAddr, stack string) error { + for i := range tinfo.fields { + finfo := &tinfo.fields[i] + if finfo.flags&fAttr != 0 { + continue + } + vf := finfo.value(val) + + switch finfo.flags & fMode { + case fCDATA, fCharData: + if vf.Implements(textMarshalerType) { + continue + } + if vf.CanAddr() { + pv := fakereflect.PtrTo(vf) + if pv.Implements(textMarshalerType) { + continue + } + } + + vf = indirect(vf) + continue + + case fComment: + vf = indirect(vf) + if !(isByteSlice(vf) || isByteArray(vf)) { + return fmt.Errorf("xml: bad type for comment field of %s", val) + } + continue + + case fInnerXML: + vf = indirect(vf) + if typeutil.IsType(vf.Type, "[]byte") || typeutil.IsType(vf.Type, "string") { + continue + } + + case fElement, fElement | fAny: + } + if err := e.marshalValue(vf, finfo, nil, stack+pathByIndex(val, finfo.idx)); err != nil { + return err + } + } + return nil +} + +// UnsupportedTypeError is returned when Marshal encounters a type +// that cannot be converted into XML. +type UnsupportedTypeError struct { + Type types.Type + Path string +} + +func (e *UnsupportedTypeError) Error() string { + return fmt.Sprintf("xml: unsupported type %s, via %s ", e.Type, e.Path) +} diff --git a/staticcheck/fakexml/typeinfo.go b/staticcheck/fakexml/typeinfo.go new file mode 100644 index 000000000..63f48c418 --- /dev/null +++ b/staticcheck/fakexml/typeinfo.go @@ -0,0 +1,383 @@ +// Copyright 2011 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package fakexml + +import ( + "fmt" + "go/types" + "strconv" + "strings" + "sync" + + "honnef.co/go/tools/staticcheck/fakereflect" +) + +// typeInfo holds details for the xml representation of a type. +type typeInfo struct { + xmlname *fieldInfo + fields []fieldInfo +} + +// fieldInfo holds details for the xml representation of a single field. +type fieldInfo struct { + idx []int + name string + xmlns string + flags fieldFlags + parents []string +} + +type fieldFlags int + +const ( + fElement fieldFlags = 1 << iota + fAttr + fCDATA + fCharData + fInnerXML + fComment + fAny + + fOmitEmpty + + fMode = fElement | fAttr | fCDATA | fCharData | fInnerXML | fComment | fAny + + xmlName = "XMLName" +) + +func (f fieldFlags) String() string { + switch f { + case fAttr: + return "attr" + case fCDATA: + return "cdata" + case fCharData: + return "chardata" + case fInnerXML: + return "innerxml" + case fComment: + return "comment" + case fAny: + return "any" + case fOmitEmpty: + return "omitempty" + case fAny | fAttr: + return "any,attr" + default: + return strconv.Itoa(int(f)) + } +} + +var tinfoMap sync.Map // map[reflect.Type]*typeInfo + +// getTypeInfo returns the typeInfo structure with details necessary +// for marshaling and unmarshaling typ. +func getTypeInfo(typ fakereflect.TypeAndCanAddr) (*typeInfo, error) { + if ti, ok := tinfoMap.Load(typ); ok { + return ti.(*typeInfo), nil + } + + tinfo := &typeInfo{} + named, ok := typ.Type.(*types.Named) + if typ.IsStruct() && !(ok && named.Obj().Pkg().Path() == "encoding/xml" && named.Obj().Name() == "Name") { + n := typ.NumField() + for i := 0; i < n; i++ { + f := typ.Field(i) + if (!f.IsExported() && !f.Anonymous) || f.Tag.Get("xml") == "-" { + continue // Private field + } + + // For embedded structs, embed its fields. + if f.Anonymous { + t := f.Type + if t.IsPtr() { + t = t.Elem() + } + if t.IsStruct() { + inner, err := getTypeInfo(t) + if err != nil { + return nil, err + } + if tinfo.xmlname == nil { + tinfo.xmlname = inner.xmlname + } + for _, finfo := range inner.fields { + finfo.idx = append([]int{i}, finfo.idx...) + if err := addFieldInfo(typ, tinfo, &finfo); err != nil { + return nil, err + } + } + continue + } + } + + finfo, err := StructFieldInfo(f) + if err != nil { + return nil, err + } + + if f.Name == xmlName { + tinfo.xmlname = finfo + continue + } + + // Add the field if it doesn't conflict with other fields. + if err := addFieldInfo(typ, tinfo, finfo); err != nil { + return nil, err + } + } + } + + ti, _ := tinfoMap.LoadOrStore(typ, tinfo) + return ti.(*typeInfo), nil +} + +// StructFieldInfo builds and returns a fieldInfo for f. +func StructFieldInfo(f fakereflect.StructField) (*fieldInfo, error) { + finfo := &fieldInfo{idx: f.Index} + + // Split the tag from the xml namespace if necessary. + tag := f.Tag.Get("xml") + if i := strings.Index(tag, " "); i >= 0 { + finfo.xmlns, tag = tag[:i], tag[i+1:] + } + + // Parse flags. + tokens := strings.Split(tag, ",") + if len(tokens) == 1 { + finfo.flags = fElement + } else { + tag = tokens[0] + for _, flag := range tokens[1:] { + switch flag { + case "attr": + finfo.flags |= fAttr + case "cdata": + finfo.flags |= fCDATA + case "chardata": + finfo.flags |= fCharData + case "innerxml": + finfo.flags |= fInnerXML + case "comment": + finfo.flags |= fComment + case "any": + finfo.flags |= fAny + case "omitempty": + finfo.flags |= fOmitEmpty + } + } + + // Validate the flags used. + switch mode := finfo.flags & fMode; mode { + case 0: + finfo.flags |= fElement + case fAttr, fCDATA, fCharData, fInnerXML, fComment, fAny, fAny | fAttr: + if f.Name == xmlName { + return nil, fmt.Errorf("cannot use option %s on XMLName field", mode) + } else if tag != "" && mode != fAttr { + return nil, fmt.Errorf("cannot specify name together with option ,%s", mode) + } + default: + // This will also catch multiple modes in a single field. + return nil, fmt.Errorf("invalid combination of options: %q", f.Tag.Get("xml")) + } + if finfo.flags&fMode == fAny { + finfo.flags |= fElement + } + if finfo.flags&fOmitEmpty != 0 && finfo.flags&(fElement|fAttr) == 0 { + return nil, fmt.Errorf("can only use omitempty on elements and attributes") + } + } + + // Use of xmlns without a name is not allowed. + if finfo.xmlns != "" && tag == "" { + return nil, fmt.Errorf("namespace without name: %q", f.Tag.Get("xml")) + } + + if f.Name == xmlName { + // The XMLName field records the XML element name. Don't + // process it as usual because its name should default to + // empty rather than to the field name. + finfo.name = tag + return finfo, nil + } + + if tag == "" { + // If the name part of the tag is completely empty, get + // default from XMLName of underlying struct if feasible, + // or field name otherwise. + if xmlname := lookupXMLName(f.Type); xmlname != nil { + finfo.xmlns, finfo.name = xmlname.xmlns, xmlname.name + } else { + finfo.name = f.Name + } + return finfo, nil + } + + // Prepare field name and parents. + parents := strings.Split(tag, ">") + if parents[0] == "" { + parents[0] = f.Name + } + if parents[len(parents)-1] == "" { + return nil, fmt.Errorf("trailing '>'") + } + finfo.name = parents[len(parents)-1] + if len(parents) > 1 { + if (finfo.flags & fElement) == 0 { + return nil, fmt.Errorf("%s chain not valid with %s flag", tag, strings.Join(tokens[1:], ",")) + } + finfo.parents = parents[:len(parents)-1] + } + + // If the field type has an XMLName field, the names must match + // so that the behavior of both marshaling and unmarshaling + // is straightforward and unambiguous. + if finfo.flags&fElement != 0 { + ftyp := f.Type + xmlname := lookupXMLName(ftyp) + if xmlname != nil && xmlname.name != finfo.name { + return nil, fmt.Errorf("name %q conflicts with name %q in %s.XMLName", finfo.name, xmlname.name, ftyp) + } + } + return finfo, nil +} + +// lookupXMLName returns the fieldInfo for typ's XMLName field +// in case it exists and has a valid xml field tag, otherwise +// it returns nil. +func lookupXMLName(typ fakereflect.TypeAndCanAddr) (xmlname *fieldInfo) { + for typ.IsPtr() { + typ = typ.Elem() + } + if !typ.IsStruct() { + return nil + } + for i, n := 0, typ.NumField(); i < n; i++ { + f := typ.Field(i) + if f.Name != xmlName { + continue + } + finfo, err := StructFieldInfo(f) + if err == nil && finfo.name != "" { + return finfo + } + // Also consider errors as a non-existent field tag + // and let getTypeInfo itself report the error. + break + } + return nil +} + +func min(a, b int) int { + if a <= b { + return a + } + return b +} + +// addFieldInfo adds finfo to tinfo.fields if there are no +// conflicts, or if conflicts arise from previous fields that were +// obtained from deeper embedded structures than finfo. In the latter +// case, the conflicting entries are dropped. +// A conflict occurs when the path (parent + name) to a field is +// itself a prefix of another path, or when two paths match exactly. +// It is okay for field paths to share a common, shorter prefix. +func addFieldInfo(typ fakereflect.TypeAndCanAddr, tinfo *typeInfo, newf *fieldInfo) error { + var conflicts []int +Loop: + // First, figure all conflicts. Most working code will have none. + for i := range tinfo.fields { + oldf := &tinfo.fields[i] + if oldf.flags&fMode != newf.flags&fMode { + continue + } + if oldf.xmlns != "" && newf.xmlns != "" && oldf.xmlns != newf.xmlns { + continue + } + minl := min(len(newf.parents), len(oldf.parents)) + for p := 0; p < minl; p++ { + if oldf.parents[p] != newf.parents[p] { + continue Loop + } + } + if len(oldf.parents) > len(newf.parents) { + if oldf.parents[len(newf.parents)] == newf.name { + conflicts = append(conflicts, i) + } + } else if len(oldf.parents) < len(newf.parents) { + if newf.parents[len(oldf.parents)] == oldf.name { + conflicts = append(conflicts, i) + } + } else { + if newf.name == oldf.name { + conflicts = append(conflicts, i) + } + } + } + // Without conflicts, add the new field and return. + if conflicts == nil { + tinfo.fields = append(tinfo.fields, *newf) + return nil + } + + // If any conflict is shallower, ignore the new field. + // This matches the Go field resolution on embedding. + for _, i := range conflicts { + if len(tinfo.fields[i].idx) < len(newf.idx) { + return nil + } + } + + // Otherwise, if any of them is at the same depth level, it's an error. + for _, i := range conflicts { + oldf := &tinfo.fields[i] + if len(oldf.idx) == len(newf.idx) { + f1 := typ.FieldByIndex(oldf.idx) + f2 := typ.FieldByIndex(newf.idx) + return &TagPathError{typ, f1.Name, f1.Tag.Get("xml"), f2.Name, f2.Tag.Get("xml")} + } + } + + // Otherwise, the new field is shallower, and thus takes precedence, + // so drop the conflicting fields from tinfo and append the new one. + for c := len(conflicts) - 1; c >= 0; c-- { + i := conflicts[c] + copy(tinfo.fields[i:], tinfo.fields[i+1:]) + tinfo.fields = tinfo.fields[:len(tinfo.fields)-1] + } + tinfo.fields = append(tinfo.fields, *newf) + return nil +} + +// A TagPathError represents an error in the unmarshaling process +// caused by the use of field tags with conflicting paths. +type TagPathError struct { + Struct fakereflect.TypeAndCanAddr + Field1, Tag1 string + Field2, Tag2 string +} + +func (e *TagPathError) Error() string { + return fmt.Sprintf("%s field %q with tag %q conflicts with field %q with tag %q", e.Struct, e.Field1, e.Tag1, e.Field2, e.Tag2) +} + +// value returns v's field value corresponding to finfo. +// It's equivalent to v.FieldByIndex(finfo.idx), but when passed +// initNilPointers, it initializes and dereferences pointers as necessary. +// When passed dontInitNilPointers and a nil pointer is reached, the function +// returns a zero reflect.Value. +func (finfo *fieldInfo) value(v fakereflect.TypeAndCanAddr) fakereflect.TypeAndCanAddr { + for i, x := range finfo.idx { + if i > 0 { + t := v + if t.IsPtr() && t.Elem().IsStruct() { + v = v.Elem() + } + } + v = v.Field(x).Type + } + return v +} diff --git a/staticcheck/fakexml/xml.go b/staticcheck/fakexml/xml.go new file mode 100644 index 000000000..0f8fb27b8 --- /dev/null +++ b/staticcheck/fakexml/xml.go @@ -0,0 +1,33 @@ +// Copyright 2009 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package fakexml + +// References: +// Annotated XML spec: https://www.xml.com/axml/testaxml.htm +// XML name spaces: https://www.w3.org/TR/REC-xml-names/ + +// TODO(rsc): +// Test error handling. + +// A Name represents an XML name (Local) annotated +// with a name space identifier (Space). +// In tokens returned by Decoder.Token, the Space identifier +// is given as a canonical URL, not the short prefix used +// in the document being parsed. +type Name struct { + Space, Local string +} + +// An Attr represents an attribute in an XML element (Name=Value). +type Attr struct { + Name Name + Value string +} + +// A StartElement represents an XML start element. +type StartElement struct { + Name Name + Attr []Attr +} diff --git a/staticcheck/lint.go b/staticcheck/lint.go index 10aa866b7..d3fa5441f 100644 --- a/staticcheck/lint.go +++ b/staticcheck/lint.go @@ -35,6 +35,8 @@ import ( "honnef.co/go/tools/pattern" "honnef.co/go/tools/printf" "honnef.co/go/tools/staticcheck/fakejson" + "honnef.co/go/tools/staticcheck/fakereflect" + "honnef.co/go/tools/staticcheck/fakexml" "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/passes/inspect" @@ -256,9 +258,9 @@ var ( checkUnsupportedMarshal = map[string]CallCheck{ "encoding/json.Marshal": checkUnsupportedMarshalJSON, - "encoding/xml.Marshal": checkUnsupportedMarshalImpl(knowledge.Arg("xml.Marshal.v"), "xml", "MarshalXML", "MarshalText"), + "encoding/xml.Marshal": checkUnsupportedMarshalXML, "(*encoding/json.Encoder).Encode": checkUnsupportedMarshalJSON, - "(*encoding/xml.Encoder).Encode": checkUnsupportedMarshalImpl(knowledge.Arg("(*encoding/xml.Encoder).Encode.v"), "xml", "MarshalXML", "MarshalText"), + "(*encoding/xml.Encoder).Encode": checkUnsupportedMarshalXML, } checkAtomicAlignment = map[string]CallCheck{ @@ -882,59 +884,26 @@ func checkUnsupportedMarshalJSON(call *Call) { } } -func checkUnsupportedMarshalImpl(argN int, tag string, meths ...string) CallCheck { - // TODO(dh): flag slices and maps of unsupported types - return func(call *Call) { - msCache := &call.Instr.Parent().Prog.MethodSets - - arg := call.Args[argN] - T := arg.Value.Value.Type() - Ts, ok := typeutil.Dereference(T).Underlying().(*types.Struct) - if !ok { - return - } - ms := msCache.MethodSet(T) - // TODO(dh): we're not checking the signature, which can cause false negatives. - // This isn't a huge problem, however, since vet complains about incorrect signatures. - for _, meth := range meths { - if ms.Lookup(nil, meth) != nil { - return - } - } - fields := typeutil.FlattenFields(Ts) - for _, field := range fields { - if !(field.Var.Exported()) { - continue - } - if reflect.StructTag(field.Tag).Get(tag) == "-" { - continue - } - ms := msCache.MethodSet(field.Var.Type()) - // TODO(dh): we're not checking the signature, which can cause false negatives. - // This isn't a huge problem, however, since vet complains about incorrect signatures. - for _, meth := range meths { - if ms.Lookup(nil, meth) != nil { - return - } - } - switch field.Var.Type().Underlying().(type) { - case *types.Chan, *types.Signature: - arg.Invalid(fmt.Sprintf("trying to marshal chan or func value, field %s", fieldPath(T, field.Path))) +func checkUnsupportedMarshalXML(call *Call) { + arg := call.Args[0] + T := arg.Value.Value.Type() + if err := fakexml.Marshal(T); err != nil { + switch err := err.(type) { + case *fakexml.UnsupportedTypeError: + typ := types.TypeString(err.Type, types.RelativeTo(arg.Value.Value.Parent().Pkg.Pkg)) + if err.Path == "x" { + arg.Invalid(fmt.Sprintf("trying to marshal unsupported type %s", typ)) + } else { + arg.Invalid(fmt.Sprintf("trying to marshal unsupported type %s, via %s", typ, err.Path)) } + case *fakexml.TagPathError: + // Vet does a better job at reporting this error, because it can flag the actual struct tags, not just the call to Marshal + default: + // These errors get reported by SA5008 instead, which can flag the actual fields, independently of calls to xml.Marshal } } } -func fieldPath(start types.Type, indices []int) string { - p := start.String() - for _, idx := range indices { - field := typeutil.Dereference(start).Underlying().(*types.Struct).Field(idx) - start = field.Type() - p += "." + field.Name() - } - return p -} - func isInLoop(b *ir.BasicBlock) bool { sets := irutil.FindLoops(b.Parent()) for _, set := range sets { @@ -3877,7 +3846,12 @@ func CheckStructTags(pass *analysis.Pass) (interface{}, error) { } fn := func(node ast.Node) { - for _, field := range node.(*ast.StructType).Fields.List { + structNode := node.(*ast.StructType) + T := pass.TypesInfo.Types[structNode].Type.(*types.Struct) + rt := fakereflect.TypeAndCanAddr{ + Type: T, + } + for i, field := range structNode.Fields.List { if field.Tag == nil { continue } @@ -3899,6 +3873,9 @@ func CheckStructTags(pass *analysis.Pass) (interface{}, error) { case "json": checkJSONTag(pass, field, v[0]) case "xml": + if _, err := fakexml.StructFieldInfo(rt.Field(i)); err != nil { + report.Report(pass, field.Tag, fmt.Sprintf("invalid XML tag: %s", err)) + } checkXMLTag(pass, field, v[0]) } } @@ -3962,29 +3939,22 @@ func checkXMLTag(pass *analysis.Pass, field *ast.Field, tag string) { } fields := strings.Split(tag, ",") counts := map[string]int{} - var exclusives []string for _, s := range fields[1:] { switch s { case "attr", "chardata", "cdata", "innerxml", "comment": counts[s]++ - if counts[s] == 1 { - exclusives = append(exclusives, s) - } case "omitempty", "any": counts[s]++ case "": default: - report.Report(pass, field.Tag, fmt.Sprintf("unknown XML option %q", s)) + report.Report(pass, field.Tag, fmt.Sprintf("invalid XML tag: unknown option %q", s)) } } for k, v := range counts { if v > 1 { - report.Report(pass, field.Tag, fmt.Sprintf("duplicate XML option %q", k)) + report.Report(pass, field.Tag, fmt.Sprintf("invalid XML tag: duplicate option %q", k)) } } - if len(exclusives) > 1 { - report.Report(pass, field.Tag, fmt.Sprintf("XML options %s are mutually exclusive", strings.Join(exclusives, " and "))) - } } func CheckImpossibleTypeAssertion(pass *analysis.Pass) (interface{}, error) { diff --git a/staticcheck/testdata/src/CheckStructTags/CheckStructTags.go b/staticcheck/testdata/src/CheckStructTags/CheckStructTags.go index 9781426ed..11be5ea37 100644 --- a/staticcheck/testdata/src/CheckStructTags/CheckStructTags.go +++ b/staticcheck/testdata/src/CheckStructTags/CheckStructTags.go @@ -1,6 +1,10 @@ package pkg -import _ "github.com/jessevdk/go-flags" +import ( + "encoding/xml" + + _ "github.com/jessevdk/go-flags" +) type T1 struct { B int `foo:"" foo:""` // want `duplicate struct tag` @@ -27,10 +31,9 @@ type T2 struct { E int `xml:",comment"` F int `xml:",omitempty"` G int `xml:",any"` - H int `xml:",unknown"` // want `unknown XML option` - I int `xml:",any,any"` // want `duplicate XML option` + H int `xml:",unknown"` // want `unknown option` + I int `xml:",any,any"` // want `duplicate option` J int `xml:"a>b>c,"` - K int `xml:",attr,cdata"` // want `mutually exclusive` } type T3 struct { @@ -44,3 +47,18 @@ type T4 struct { C []int `default:"foo" default:"bar"` D int `json:"foo" json:"bar"` // want `duplicate struct tag` } + +func xmlTags() { + type T1 struct { + A int `xml:",attr,innerxml"` // want `invalid combination of options: ",attr,innerxml"` + XMLName xml.Name `xml:"ns "` // want `namespace without name: "ns "` + B int `xml:"a>"` // want `trailing '>'` + C int `xml:"a>b,attr"` // want `a>b chain not valid with attr flag` + } + type T6 struct { + XMLName xml.Name `xml:"foo"` + } + type T5 struct { + F T6 `xml:"f"` // want `name "f" conflicts with name "foo" in CheckStructTags\.T6\.XMLName` + } +} diff --git a/staticcheck/testdata/src/CheckStructTags3/CheckStructTags.go b/staticcheck/testdata/src/CheckStructTags3/CheckStructTags.go index 9781426ed..38abcb0ae 100644 --- a/staticcheck/testdata/src/CheckStructTags3/CheckStructTags.go +++ b/staticcheck/testdata/src/CheckStructTags3/CheckStructTags.go @@ -27,10 +27,9 @@ type T2 struct { E int `xml:",comment"` F int `xml:",omitempty"` G int `xml:",any"` - H int `xml:",unknown"` // want `unknown XML option` - I int `xml:",any,any"` // want `duplicate XML option` + H int `xml:",unknown"` // want `unknown option` + I int `xml:",any,any"` // want `duplicate option` J int `xml:"a>b>c,"` - K int `xml:",attr,cdata"` // want `mutually exclusive` } type T3 struct { diff --git a/staticcheck/testdata/src/CheckUnsupportedMarshal/CheckUnsupportedMarshal.go b/staticcheck/testdata/src/CheckUnsupportedMarshal/CheckUnsupportedMarshal.go index ee0c86bae..ee6461ef6 100644 --- a/staticcheck/testdata/src/CheckUnsupportedMarshal/CheckUnsupportedMarshal.go +++ b/staticcheck/testdata/src/CheckUnsupportedMarshal/CheckUnsupportedMarshal.go @@ -91,27 +91,32 @@ func fn() { xml.Marshal(t1) xml.Marshal(t2) - xml.Marshal(t3) // want `trying to marshal chan or func value, field CheckUnsupportedMarshal\.T3\.C` + xml.Marshal(t3) // want `unsupported type chan int, via x\.Ch` xml.Marshal(t4) xml.Marshal(t5) - xml.Marshal(t6) // want `trying to marshal chan or func value, field CheckUnsupportedMarshal\.T6\.B` + xml.Marshal(t6) // want `unsupported type func\(\), via x\.B` (*xml.Encoder)(nil).Encode(t1) (*xml.Encoder)(nil).Encode(t2) - (*xml.Encoder)(nil).Encode(t3) // want `trying to marshal chan or func value, field CheckUnsupportedMarshal\.T3\.C` + (*xml.Encoder)(nil).Encode(t3) // want `unsupported type chan int, via x\.C` (*xml.Encoder)(nil).Encode(t4) (*xml.Encoder)(nil).Encode(t5) - (*xml.Encoder)(nil).Encode(t6) // want `trying to marshal chan or func value, field CheckUnsupportedMarshal\.T6\.B` + (*xml.Encoder)(nil).Encode(t6) // want `unsupported type func\(\), via x\.B` json.Marshal(t8) // want `unsupported type chan int, via x\.T7\.T3\.Ch` json.Marshal(t9) // want `unsupported type PointerMarshaler, via x\.F` json.Marshal(&t9) // this is fine, t9 is addressable, therefore T9.D is, too json.Marshal(t10) // this is fine, T10.F.D is addressable + xml.Marshal(t8) // want `unsupported type chan int, via x\.T7\.T3\.Ch` + xml.Marshal(t9) // want `unsupported type PointerMarshaler, via x\.F` + xml.Marshal(&t9) // this is fine, t9 is addressable, therefore T9.D is, too + xml.Marshal(t10) // this is fine, T10.F.D is addressable + json.Marshal(t11) xml.Marshal(t11) } -func addressability() { +func addressabilityJSON() { var a PointerMarshaler var b []PointerMarshaler var c struct { @@ -138,7 +143,25 @@ func addressability() { json.Marshal([]map[string]*PointerMarshaler{m2}) } -func maps() { +func addressabilityXML() { + var a PointerMarshaler + var b []PointerMarshaler + var c struct { + XMLName xml.Name `json:"foo"` + F PointerMarshaler + } + var d [4]PointerMarshaler + xml.Marshal(a) // want `unsupported type PointerMarshaler$` + xml.Marshal(&a) + xml.Marshal(b) + xml.Marshal(&b) + xml.Marshal(c) // want `unsupported type PointerMarshaler, via x\.F` + xml.Marshal(&c) + xml.Marshal(d) // want `unsupported type PointerMarshaler, via x\[0\]` + xml.Marshal(&d) +} + +func mapsJSON() { var good map[int]string var bad map[interface{}]string // the map key has to be statically known good; it must be a number or a string @@ -176,7 +199,13 @@ func maps() { json.Marshal(m8) } -func fieldPriority() { +func mapsXML() { + // encoding/xml doesn't support any maps + var bad map[string]string + xml.Marshal(bad) // want `unsupported type` +} + +func fieldPriorityJSON() { // In this example, the channel doesn't matter, because T1.F has higher priority than T1.T2.F type lT2 struct { F chan int @@ -198,7 +227,29 @@ func fieldPriority() { json.Marshal(lT3{}) // want `unsupported type chan int, via x\.lT4\.C` } -func longPath() { +func fieldPriorityXML() { + // In this example, the channel doesn't matter, because T1.F has higher priority than T1.T2.F + type lT2 struct { + F chan int + } + type lT1 struct { + F int + lT2 + } + xml.Marshal(lT1{}) + + // In this example, it does matter + type lT4 struct { + C chan int + } + type lT3 struct { + F int + lT4 + } + xml.Marshal(lT3{}) // want `unsupported type chan int, via x\.lT4\.C` +} + +func longPathJSON() { var foo struct { Field struct { Field2 []struct { @@ -209,7 +260,23 @@ func longPath() { json.Marshal(foo) // want `unsupported type chan int, via x\.Field\.Field2\[0\].Map\[k\]` } -func otherPackage() { +func otherPackageJSON() { var x time.Ticker json.Marshal(x) // want `unsupported type <-chan time\.Time, via x\.C` } + +func longPathXML() { + var foo struct { + Field struct { + Field2 []struct { + Map map[string]chan int + } + } + } + xml.Marshal(foo) // want `unsupported type map\[string\]chan int, via x\.Field\.Field2\[0\].Map` +} + +func otherPackageXML() { + var x time.Ticker + xml.Marshal(x) // want `unsupported type <-chan time\.Time, via x\.C` +}