From 42e7ecda5a883966f2b49fde79fa8cfa59acf169 Mon Sep 17 00:00:00 2001 From: Wojciech Tyczynski Date: Wed, 23 Mar 2016 08:42:33 +0100 Subject: [PATCH] Fix bunch of issues with conversion generator. --- .../conversion-gen/generators/conversion.go | 130 +++++++++++------- cmd/libs/go2idl/conversion-gen/main.go | 2 + cmd/libs/go2idl/generator/snippet_writer.go | 3 + cmd/libs/go2idl/types/types.go | 9 ++ hack/after-build/run-codegen.sh | 3 + hack/update-codegen.sh | 1 + pkg/api/v1/conversion_generated.go | 4 +- pkg/runtime/embedded.go | 81 +++++------ 8 files changed, 143 insertions(+), 90 deletions(-) diff --git a/cmd/libs/go2idl/conversion-gen/generators/conversion.go b/cmd/libs/go2idl/conversion-gen/generators/conversion.go index 223773763fb..59bd7489bb1 100644 --- a/cmd/libs/go2idl/conversion-gen/generators/conversion.go +++ b/cmd/libs/go2idl/conversion-gen/generators/conversion.go @@ -207,7 +207,10 @@ func isConvertible(in, out *types.Type, preexisting conversions) bool { if _, ok := preexisting[conversionType{in, out}]; ok { return true } + return isDirectlyConvertible(in, out, preexisting) +} +func isDirectlyConvertible(in, out *types.Type, preexisting conversions) bool { // If one of the types is Alias, resolve it. if in.Kind == types.Alias { return isConvertible(in.Underlying, out, preexisting) @@ -234,8 +237,11 @@ func isConvertible(in, out *types.Type, preexisting conversions) bool { switch in.Kind { case types.Builtin: - // FIXME: Enough to be convertible - see AWSElastic - return in.Name == out.Name + if in == out { + return true + } + // TODO: Support more conversion types. + return types.IsInteger(in) && types.IsInteger(out) case types.Struct: convertible := true for _, inMember := range in.Members { @@ -404,10 +410,10 @@ func (g *genConversion) Init(c *generator.Context, w io.Writer) error { func (g *genConversion) GenerateType(c *generator.Context, t *types.Type, w io.Writer) error { sw := generator.NewSnippetWriter(w, c, "$", "$") internalType, _ := getInternalTypeFor(c, t) - if _, ok := g.preexists(t, internalType); !ok && isConvertible(t, internalType, g.preexisting) { + if isDirectlyConvertible(t, internalType, g.preexisting) { g.generateConversion(t, internalType, sw) } - if _, ok := g.preexists(internalType, t); !ok && isConvertible(internalType, t, g.preexisting) { + if isDirectlyConvertible(internalType, t, g.preexisting) { g.generateConversion(internalType, t, sw) } return sw.Error() @@ -416,16 +422,27 @@ func (g *genConversion) GenerateType(c *generator.Context, t *types.Type, w io.W func (g *genConversion) generateConversion(inType, outType *types.Type, sw *generator.SnippetWriter) { funcName := g.funcNameTmpl(inType, outType) if g.targetPackage == conversionPackagePath { - sw.Do(fmt.Sprintf("func %s(in $.inType|raw$, out *$.outType|raw$, s *Scope) error {\n", funcName), argsFromType(inType, outType)) + sw.Do(fmt.Sprintf("func auto%s(in *$.inType|raw$, out *$.outType|raw$, s Scope) error {\n", funcName), argsFromType(inType, outType)) } else { - sw.Do(fmt.Sprintf("func %s(in $.inType|raw$, out *$.outType|raw$, s *conversion.Scope) error {\n", funcName), argsFromType(inType, outType)) + sw.Do(fmt.Sprintf("func auto%s(in *$.inType|raw$, out *$.outType|raw$, s conversion.Scope) error {\n", funcName), argsFromType(inType, outType)) } sw.Do("if defaulting, found := s.DefaultingInterface(reflect.TypeOf(*in)); found {\n", nil) - sw.Do("defaulting.(func($.|raw$))(in)\n", inType) + sw.Do("defaulting.(func(*$.|raw$))(in)\n", inType) sw.Do("}\n", nil) g.generateFor(inType, outType, sw) sw.Do("return nil\n", nil) sw.Do("}\n\n", nil) + + // If there is no public preexisting Convert method, generate it. + if _, ok := g.preexists(inType, outType); !ok { + if g.targetPackage == conversionPackagePath { + sw.Do(fmt.Sprintf("func %s(in *$.inType|raw$, out *$.outType|raw$, s Scope) error {\n", funcName), argsFromType(inType, outType)) + } else { + sw.Do(fmt.Sprintf("func %s(in *$.inType|raw$, out *$.outType|raw$, s conversion.Scope) error {\n", funcName), argsFromType(inType, outType)) + } + sw.Do(fmt.Sprintf("return auto%s(in, out, s)\n", funcName), argsFromType(inType, outType)) + sw.Do("}\n\n", nil) + } } // we use the system of shadowing 'in' and 'out' so that the same code is valid @@ -453,33 +470,46 @@ func (g *genConversion) generateFor(inType, outType *types.Type, sw *generator.S } func (g *genConversion) doBuiltin(inType, outType *types.Type, sw *generator.SnippetWriter) { - sw.Do("*out = in\n", nil) + if inType == outType { + sw.Do("*out = *in\n", nil) + } else { + sw.Do("*out = $.|raw$(*in)\n", outType) + } } func (g *genConversion) doMap(inType, outType *types.Type, sw *generator.SnippetWriter) { - sw.Do("*out = make($.|raw$)\n", outType) + sw.Do("out = make($.|raw$, len(in))\n", outType) if outType.Key.IsAssignable() { sw.Do("for key, val := range in {\n", nil) if outType.Elem.IsAssignable() { - if inType.Elem == outType.Elem { - sw.Do("(*out)[key] = val\n", nil) + if inType.Key == outType.Key { + sw.Do("out[key] = ", nil) } else { - sw.Do("(*out)[key] = $.|raw$(val)\n", outType.Elem) + sw.Do("out[$.|raw$(key)] = ", outType.Key) + } + if inType.Elem == outType.Elem { + sw.Do("val\n", nil) + } else { + sw.Do("$.|raw$(val)\n", outType.Elem) } } else { sw.Do("newVal := new($.|raw$)\n", outType.Elem) if function, ok := g.preexists(inType.Elem, outType.Elem); ok { - sw.Do("if err := $.|raw$(val, newVal, s); err != nil {\n", function) + sw.Do("if err := $.|raw$(&val, newVal, s); err != nil {\n", function) } else if g.convertibleOnlyWithinPackage(inType.Elem, outType.Elem) { funcName := g.funcNameTmpl(inType.Elem, outType.Elem) - sw.Do(fmt.Sprintf("if err := %s(val, newVal, s); err != nil {\n", funcName), argsFromType(inType.Elem, outType.Elem)) + sw.Do(fmt.Sprintf("if err := %s(&val, newVal, s); err != nil {\n", funcName), argsFromType(inType.Elem, outType.Elem)) } else { sw.Do("// TODO: Inefficient conversion - can we improve it?\n", nil) - sw.Do("if err := s.Convert(val, newVal, 0); err != nil {\n", nil) + sw.Do("if err := s.Convert(&val, newVal, 0); err != nil {\n", nil) } sw.Do("return err\n", nil) sw.Do("}\n", nil) - sw.Do("(*out)[key] = *newVal\n", nil) + if inType.Key == outType.Key { + sw.Do("out[key] = *newVal\n", nil) + } else { + sw.Do("out[$.|raw$(key)] = *newVal\n", outType.Key) + } } } else { // TODO: Implement it when necessary. @@ -490,26 +520,26 @@ func (g *genConversion) doMap(inType, outType *types.Type, sw *generator.Snippet } func (g *genConversion) doSlice(inType, outType *types.Type, sw *generator.SnippetWriter) { - sw.Do("*out = make($.|raw$, len(in))\n", outType) + sw.Do("out = make($.|raw$, len(in))\n", outType) if inType.Elem == outType.Elem && inType.Elem.Kind == types.Builtin { - sw.Do("copy(*out, in)\n", nil) + sw.Do("copy(out, in)\n", nil) } else { sw.Do("for i := range in {\n", nil) if outType.Elem.IsAssignable() { if inType.Elem == outType.Elem { - sw.Do("(*out)[i] = in[i]\n", nil) + sw.Do("out[i] = in[i]\n", nil) } else { - sw.Do("(*out)[i] = $.|raw$(in[i])\n", outType.Elem) + sw.Do("out[i] = $.|raw$(in[i])\n", outType.Elem) } } else { if function, ok := g.preexists(inType.Elem, outType.Elem); ok { - sw.Do("if err := $.|raw$(&(*in)[i], &(*out)[i], s); err != nil {\n", function) + sw.Do("if err := $.|raw$(&in[i], &out[i], s); err != nil {\n", function) } else if g.convertibleOnlyWithinPackage(inType.Elem, outType.Elem) { funcName := g.funcNameTmpl(inType.Elem, outType.Elem) - sw.Do(fmt.Sprintf("if err := %s(in[i], &(*out)[i], c); err != nil {\n", funcName), argsFromType(inType.Elem, outType.Elem)) + sw.Do(fmt.Sprintf("if err := %s(&in[i], &out[i], s); err != nil {\n", funcName), argsFromType(inType.Elem, outType.Elem)) } else { sw.Do("// TODO: Inefficient conversion - can we improve it?\n", nil) - sw.Do("if err := s.Convert(in[i], &out[i], 0); err != nil {\n", nil) + sw.Do("if err := s.Convert(&in[i], &out[i], 0); err != nil {\n", nil) } sw.Do("return err\n", nil) sw.Do("}\n", nil) @@ -526,26 +556,34 @@ func (g *genConversion) doStruct(inType, outType *types.Type, sw *generator.Snip "outType": outMember.Type, "name": m.Name, } + if function, ok := g.preexists(m.Type, outMember.Type); ok { + args["function"] = function + sw.Do("if err := $.function|raw$(&in.$.name$, &out.$.name$, s); err != nil {\n", args) + sw.Do("return err\n", nil) + sw.Do("}\n", nil) + continue + } switch m.Type.Kind { case types.Builtin: - sw.Do("out.$.name$ = in.$.name$\n", args) + if m.Type == outMember.Type { + sw.Do("out.$.name$ = in.$.name$\n", args) + } else { + sw.Do("out.$.name$ = $.outType|raw$(in.$.name$)\n", args) + } case types.Map, types.Slice, types.Pointer: sw.Do("if in.$.name$ != nil {\n", args) - sw.Do("in, out := in.$.name$, &out.$.name$\n", args) + sw.Do("in, out := in.$.name$, out.$.name$\n", args) g.generateFor(m.Type, outMember.Type, sw) sw.Do("} else {\n", nil) sw.Do("out.$.name$ = nil\n", args) sw.Do("}\n", nil) case types.Struct: - if function, ok := g.preexists(m.Type, outMember.Type); ok { - args["function"] = function - sw.Do("if err := $.function|raw$(in.$.name$, &out.$.name$, s); err != nil {\n", args) - } else if g.convertibleOnlyWithinPackage(m.Type, outMember.Type) { + if g.convertibleOnlyWithinPackage(m.Type, outMember.Type) { funcName := g.funcNameTmpl(m.Type, outMember.Type) - sw.Do(fmt.Sprintf("if err := %s(in.$.name$, &out.$.name$, c); err != nil {\n", funcName), args) + sw.Do(fmt.Sprintf("if err := %s(&in.$.name$, &out.$.name$, s); err != nil {\n", funcName), args) } else { sw.Do("// TODO: Inefficient conversion - can we improve it?\n", nil) - sw.Do("if err := s.Convert(in.$.name$, &out.$.name$, 0); err != nil {\n", args) + sw.Do("if err := s.Convert(&in.$.name$, &out.$.name$, 0); err != nil {\n", args) } sw.Do("return err\n", nil) sw.Do("}\n", nil) @@ -553,29 +591,23 @@ func (g *genConversion) doStruct(inType, outType *types.Type, sw *generator.Snip if outMember.Type.IsAssignable() { sw.Do("out.$.name$ = $.outType|raw$(in.$.name$)\n", args) } else { - if function, ok := g.preexists(m.Type, outMember.Type); ok { - args["function"] = function - sw.Do("if err := $.function|raw$(in.$.name$, &out.$.name$, s); err != nil {\n", args) - } else if g.convertibleOnlyWithinPackage(m.Type, outMember.Type) { + if g.convertibleOnlyWithinPackage(m.Type, outMember.Type) { funcName := g.funcNameTmpl(m.Type, outMember.Type) - sw.Do(fmt.Sprintf("if err := %s(in.$.name$, &out.$.name$, c); err != nil {\n", funcName), args) + sw.Do(fmt.Sprintf("if err := %s(&in.$.name$, &out.$.name$, s); err != nil {\n", funcName), args) } else { sw.Do("// TODO: Inefficient conversion - can we improve it?\n", nil) - sw.Do("if err := s.Convert(in.$.name$, &out.$.name$, 0); err != nil {\n", args) + sw.Do("if err := s.Convert(&in.$.name$, &out.$.name$, 0); err != nil {\n", args) } sw.Do("return err\n", nil) sw.Do("}\n", nil) } default: - if function, ok := g.preexists(m.Type, outMember.Type); ok { - args["function"] = function - sw.Do("if err := $.function|raw$(in.$.name$, &out.$.name$, s); err != nil {\n", args) - } else if g.convertibleOnlyWithinPackage(m.Type, outMember.Type) { + if g.convertibleOnlyWithinPackage(m.Type, outMember.Type) { funcName := g.funcNameTmpl(m.Type, outMember.Type) - sw.Do(fmt.Sprintf("if err := %s(in.$.name$, &out.$.name$, c); err != nil {\n", funcName), args) + sw.Do(fmt.Sprintf("if err := %s(&in.$.name$, &out.$.name$, s); err != nil {\n", funcName), args) } else { sw.Do("// TODO: Inefficient conversion - can we improve it?\n", nil) - sw.Do("if err := s.Convert(in.$.name$, &out.$.name$, 0); err != nil {\n", args) + sw.Do("if err := s.Convert(&in.$.name$, &out.$.name$, 0); err != nil {\n", args) } sw.Do("return err\n", nil) sw.Do("}\n", nil) @@ -584,22 +616,22 @@ func (g *genConversion) doStruct(inType, outType *types.Type, sw *generator.Snip } func (g *genConversion) doPointer(inType, outType *types.Type, sw *generator.SnippetWriter) { - sw.Do("*out = new($.Elem|raw$)\n", outType) + sw.Do("out = new($.Elem|raw$)\n", outType) if outType.Elem.IsAssignable() { if inType.Elem == outType.Elem { - sw.Do("**out = *in\n", nil) + sw.Do("*out = *in\n", nil) } else { - sw.Do("**out = $.|raw$(*in)\n", outType.Elem) + sw.Do("*out = $.|raw$(*in)\n", outType.Elem) } } else { if function, ok := g.preexists(inType.Elem, outType.Elem); ok { - sw.Do("if err := $.|raw$(*in, out, s); err != nil {\n", function) + sw.Do("if err := $.|raw$(in, out, s); err != nil {\n", function) } else if g.convertibleOnlyWithinPackage(inType.Elem, outType.Elem) { funcName := g.funcNameTmpl(inType.Elem, outType.Elem) - sw.Do(fmt.Sprintf("if err := %s(*in, out, c); err != nil {\n", funcName), argsFromType(inType.Elem, outType.Elem)) + sw.Do(fmt.Sprintf("if err := %s(in, out, s); err != nil {\n", funcName), argsFromType(inType.Elem, outType.Elem)) } else { sw.Do("// TODO: Inefficient conversion - can we improve it?\n", nil) - sw.Do("if err := s.Convert(*in, out, 0); err != nil {\n", nil) + sw.Do("if err := s.Convert(in, out, 0); err != nil {\n", nil) } sw.Do("return err\n", nil) sw.Do("}\n", nil) diff --git a/cmd/libs/go2idl/conversion-gen/main.go b/cmd/libs/go2idl/conversion-gen/main.go index e1a177821ef..47dee55b0ef 100644 --- a/cmd/libs/go2idl/conversion-gen/main.go +++ b/cmd/libs/go2idl/conversion-gen/main.go @@ -34,6 +34,8 @@ func main() { // Override defaults. These are Kubernetes specific input locations. arguments.InputDirs = []string{ "k8s.io/kubernetes/pkg/api/v1", + "k8s.io/kubernetes/pkg/api", + "k8s.io/kubernetes/pkg/runtime", } if err := arguments.Execute( diff --git a/cmd/libs/go2idl/generator/snippet_writer.go b/cmd/libs/go2idl/generator/snippet_writer.go index 344b960fed4..31952ec6c0e 100644 --- a/cmd/libs/go2idl/generator/snippet_writer.go +++ b/cmd/libs/go2idl/generator/snippet_writer.go @@ -93,6 +93,9 @@ func NewSnippetWriter(w io.Writer, c *Context, left, right string) *SnippetWrite // becomes a requirement. You can do arbitrary logic inside these templates, // but you should consider doing the logic in go and stitching them together // for the sake of your readers. +// +// TODO: Change Do() to optionally take a list of pairt of parameters (key, value) +// and have it construct a combined map with that and args. func (s *SnippetWriter) Do(format string, args interface{}) *SnippetWriter { if s.err != nil { return s diff --git a/cmd/libs/go2idl/types/types.go b/cmd/libs/go2idl/types/types.go index c5344ae2ff1..e6794ac9921 100644 --- a/cmd/libs/go2idl/types/types.go +++ b/cmd/libs/go2idl/types/types.go @@ -417,3 +417,12 @@ var ( Name: "", } ) + +func IsInteger(t *Type) bool { + switch t { + case Int, Int64, Int32, Int16, Uint, Uint64, Uint32, Uint16, Byte: + return true + default: + return false + } +} diff --git a/hack/after-build/run-codegen.sh b/hack/after-build/run-codegen.sh index cae65cbc43a..e0439f9e168 100755 --- a/hack/after-build/run-codegen.sh +++ b/hack/after-build/run-codegen.sh @@ -24,6 +24,7 @@ source "${KUBE_ROOT}/hack/lib/init.sh" kube::golang::setup_env clientgen=$(kube::util::find-binary "client-gen") +conversiongen=$(kube::util::find-binary "conversion-gen") deepcopygen=$(kube::util::find-binary "deepcopy-gen") setgen=$(kube::util::find-binary "set-gen") @@ -34,6 +35,8 @@ setgen=$(kube::util::find-binary "set-gen") # update- and verify- scripts. ${clientgen} "$@" ${clientgen} -t "$@" +# TODO: Enable when conversion generator is ready. +# ${conversiongen} "$@" ${deepcopygen} "$@" ${setgen} "$@" diff --git a/hack/update-codegen.sh b/hack/update-codegen.sh index f4cc442ea7c..d0a034d8830 100755 --- a/hack/update-codegen.sh +++ b/hack/update-codegen.sh @@ -26,6 +26,7 @@ source "${KUBE_ROOT}/hack/lib/init.sh" kube::golang::setup_env "${KUBE_ROOT}/hack/build-go.sh" cmd/libs/go2idl/client-gen +"${KUBE_ROOT}/hack/build-go.sh" cmd/libs/go2idl/conversion-gen "${KUBE_ROOT}/hack/build-go.sh" cmd/libs/go2idl/deepcopy-gen "${KUBE_ROOT}/hack/build-go.sh" cmd/libs/go2idl/set-gen diff --git a/pkg/api/v1/conversion_generated.go b/pkg/api/v1/conversion_generated.go index 687da425ad7..1587527a3eb 100644 --- a/pkg/api/v1/conversion_generated.go +++ b/pkg/api/v1/conversion_generated.go @@ -1298,7 +1298,7 @@ func autoConvert_api_List_To_v1_List(in *api.List, out *List, s conversion.Scope if in.Items != nil { out.Items = make([]runtime.RawExtension, len(in.Items)) for i := range in.Items { - if err := s.Convert(&in.Items[i], &out.Items[i], 0); err != nil { + if err := runtime.Convert_runtime_Object_To_runtime_RawExtension(&in.Items[i], &out.Items[i], s); err != nil { return err } } @@ -4571,7 +4571,7 @@ func autoConvert_v1_List_To_api_List(in *List, out *api.List, s conversion.Scope if in.Items != nil { out.Items = make([]runtime.Object, len(in.Items)) for i := range in.Items { - if err := s.Convert(&in.Items[i], &out.Items[i], 0); err != nil { + if err := runtime.Convert_runtime_RawExtension_To_runtime_Object(&in.Items[i], &out.Items[i], s); err != nil { return err } } diff --git a/pkg/runtime/embedded.go b/pkg/runtime/embedded.go index 9e2ef82719c..5b91e854e96 100644 --- a/pkg/runtime/embedded.go +++ b/pkg/runtime/embedded.go @@ -88,46 +88,49 @@ func (re Unknown) MarshalJSON() ([]byte, error) { return re.Raw, nil } +func Convert_runtime_Object_To_runtime_RawExtension(in *Object, out *RawExtension, s conversion.Scope) error { + if in == nil { + out.Raw = []byte("null") + return nil + } + obj := *in + if unk, ok := obj.(*Unknown); ok { + if unk.Raw != nil { + out.Raw = unk.Raw + return nil + } + obj = out.Object + } + if obj == nil { + out.Raw = nil + return nil + } + out.Object = obj + return nil +} + +func Convert_runtime_RawExtension_To_runtime_Object(in *RawExtension, out *Object, s conversion.Scope) error { + if in.Object != nil { + *out = in.Object + return nil + } + data := in.Raw + if len(data) == 0 || (len(data) == 4 && string(data) == "null") { + *out = nil + return nil + } + *out = &Unknown{ + Raw: data, + // TODO: Set ContentEncoding and ContentType appropriately. + // Currently we set ContentTypeJSON to make tests passing. + ContentType: ContentTypeJSON, + } + return nil +} + func DefaultEmbeddedConversions() []interface{} { return []interface{}{ - func(in *Object, out *RawExtension, s conversion.Scope) error { - if in == nil { - out.Raw = []byte("null") - return nil - } - obj := *in - if unk, ok := obj.(*Unknown); ok { - if unk.Raw != nil { - out.Raw = unk.Raw - return nil - } - obj = out.Object - } - if obj == nil { - out.Raw = nil - return nil - } - out.Object = obj - return nil - }, - - func(in *RawExtension, out *Object, s conversion.Scope) error { - if in.Object != nil { - *out = in.Object - return nil - } - data := in.Raw - if len(data) == 0 || (len(data) == 4 && string(data) == "null") { - *out = nil - return nil - } - *out = &Unknown{ - Raw: data, - // TODO: Set ContentEncoding and ContentType appropriately. - // Currently we set ContentTypeJSON to make tests passing. - ContentType: ContentTypeJSON, - } - return nil - }, + Convert_runtime_Object_To_runtime_RawExtension, + Convert_runtime_RawExtension_To_runtime_Object, } }