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.

Sign up for GitHub

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

Jump to bottom

Fix bunch of issues with conversion generator - part 3 #23373

Merged
merged 1 commit into from
Mar 24, 2016
Merged

Fix bunch of issues with conversion generator - part 3 #23373

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
128 changes: 80 additions & 48 deletions cmd/libs/go2idl/conversion-gen/generators/conversion.go
View file
Open in desktop
Expand Up @@ -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)
Expand All @@ -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 {
Expand Down Expand Up @@ -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()
Expand All @@ -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
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nifty!

}
}

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.Key == outType.Key {
sw.Do("out[key] = ", nil)
} else {
sw.Do("out[$.|raw$(key)] = ", outType.Key)
}
if inType.Elem == outType.Elem {
sw.Do("(*out)[key] = val\n", nil)
sw.Do("val\n", nil)
} else {
sw.Do("(*out)[key] = $.|raw$(val)\n", outType.Elem)
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.
Expand All @@ -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)
Expand All @@ -526,56 +556,58 @@ 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I take back what I said in the other PR now that I see this-- adding function name to args is better than nesting two templating systems :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could make Do optionally take a list of pair of parameters, key + value, and have it construct a combined map?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah - that sounds good. Although, since I already did that, I will just add a TODO to fix that in the future :)

} 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)
case types.Alias:
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)
Expand All @@ -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)
Expand Down
2 changes: 2 additions & 0 deletions cmd/libs/go2idl/conversion-gen/main.go
View file
Open in desktop
Expand Up @@ -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(
Expand Down
3 changes: 3 additions & 0 deletions cmd/libs/go2idl/generator/snippet_writer.go
View file
Open in desktop
Expand Up @@ -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
Expand Down
9 changes: 9 additions & 0 deletions cmd/libs/go2idl/types/types.go
View file
Open in desktop
Expand Up @@ -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
}
}
3 changes: 3 additions & 0 deletions hack/after-build/run-codegen.sh
View file
Open in desktop
Expand Up @@ -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")

Expand All @@ -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} "$@"

Expand Down
1 change: 1 addition & 0 deletions hack/update-codegen.sh
View file
Open in desktop
Expand Up @@ -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

Expand Down
4 changes: 2 additions & 2 deletions pkg/api/v1/conversion_generated.go
View file
Open in desktop
Expand Up @@ -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
}
}
Expand Down Expand Up @@ -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
}
}
Expand Down