Always emit autoConvert funcs, but call for help

Previously we refused to emit 'autoConvert_*' functions if any field was not
convertible.  The way around this was to write manual Conversion functions, but
to do so safely you must handle every fields.  Huge opportunity for errors.

This PR cleans up the filtering such that it only operates on types that should
be converted (remove a lot of code) and tracks when fields are skipped.  In
that case, it emits an 'autoConvert' function but not a public 'Convert'
function.  If there is no manual function, the compile will fail.

This also means that manual conversion functions can call autoConvert functions
and then "patch up" what they need.

cmd/libs/go2idl/conversion-gen/generators/conversion.go

@@ -284,87 +284,14 @@ func findMember(t *types.Type, name string) (types.Member, bool) {
 	return types.Member{}, false
 }
 
-func isConvertible(in, out *types.Type, manualConversions conversionFuncMap) bool {
-	// If there is pre-existing conversion function, return true immediately.
-	if _, ok := manualConversions[conversionPair{in, out}]; ok {
-		return true
-	}
-	return isDirectlyConvertible(in, out, manualConversions)
-}
-
-func isDirectlyConvertible(in, out *types.Type, manualConversions conversionFuncMap) bool {
-	// If one of the types is Alias, resolve it.
-	if in.Kind == types.Alias {
-		return isConvertible(in.Underlying, out, manualConversions)
-	}
-	if out.Kind == types.Alias {
-		return isConvertible(in, out.Underlying, manualConversions)
-	}
-
-	if in == out {
-		return true
-	}
-	if in.Kind != out.Kind {
-		return false
-	}
-	switch in.Kind {
-	case types.Builtin, types.Struct, types.Map, types.Slice, types.Pointer:
-	default:
-		// We don't support conversion of other types yet.
-		return false
-	}
-
-	switch in.Kind {
-	case types.Builtin:
-		// TODO: Support more conversion types.
-		return types.IsInteger(in) && types.IsInteger(out)
-	case types.Struct:
-		convertible := true
-		for _, inMember := range in.Members {
-			// Check if this member is excluded from conversion
-			if tagvals := extractTag(inMember.CommentLines); tagvals != nil && tagvals[0] == "false" {
-				continue
-			}
-			// Check if there is an out member with that name.
-			outMember, found := findMember(out, inMember.Name)
-			if !found {
-				glog.V(5).Infof("%s is not directly convertible to %s because the destination field %q did not exist", in.Name, out.Name, inMember.Name)
-				return false
-			}
-			convertible = convertible && isConvertible(inMember.Type, outMember.Type, manualConversions)
-		}
-		return convertible
-	case types.Map:
-		return isConvertible(in.Key, out.Key, manualConversions) && isConvertible(in.Elem, out.Elem, manualConversions)
-	case types.Slice:
-		return isConvertible(in.Elem, out.Elem, manualConversions)
-	case types.Pointer:
-		return isConvertible(in.Elem, out.Elem, manualConversions)
-	}
-	glog.Fatalf("All other types should be filtered before")
-	return false
-}
-
 // unwrapAlias recurses down aliased types to find the bedrock type.
 func unwrapAlias(in *types.Type) *types.Type {
-	if in.Kind == types.Alias {
-		return unwrapAlias(in.Underlying)
+	for in.Kind == types.Alias {
+		in = in.Underlying
 	}
 	return in
 }
 
-func areTypesAliased(in, out *types.Type) bool {
-	// If one of the types is Alias, resolve it.
-	if in.Kind == types.Alias {
-		return areTypesAliased(in.Underlying, out)
-	}
-	if out.Kind == types.Alias {
-		return areTypesAliased(in, out.Underlying)
-	}
-
-	return in == out
-}
-
 const (
 	runtimePackagePath    = "k8s.io/kubernetes/pkg/runtime"
 	conversionPackagePath = "k8s.io/kubernetes/pkg/conversion"
@@ -378,7 +305,8 @@ type genConversion struct {
 	manualConversions conversionFuncMap
 	manualDefaulters  defaulterFuncMap
 	imports           namer.ImportTracker
-	typesForInit      []conversionPair
+	types             []*types.Type
+	skippedFields     map[*types.Type][]string
 }
 
 func NewGenConversion(sanitizedName, targetPackage string, manualConversions conversionFuncMap, manualDefaulters defaulterFuncMap, peerPkgs []string) generator.Generator {
@@ -391,7 +319,8 @@ func NewGenConversion(sanitizedName, targetPackage string, manualConversions con
 		manualConversions: manualConversions,
 		manualDefaulters:  manualDefaulters,
 		imports:           generator.NewImportTracker(),
-		typesForInit:      make([]conversionPair, 0),
+		types:             []*types.Type{},
+		skippedFields:     map[*types.Type][]string{},
 	}
 }
 
@@ -456,18 +385,9 @@ func (g *genConversion) Filter(c *generator.Context, t *types.Type) bool {
 	if !g.convertibleOnlyWithinPackage(t, peerType) {
 		return false
 	}
-	// We explicitly return true if any conversion is possible - this needs
-	// to be checked again while generating code for that type.
-	convertible := false
-	if isConvertible(t, peerType, g.manualConversions) {
-		g.typesForInit = append(g.typesForInit, conversionPair{t, peerType})
-		convertible = true
-	}
-	if isConvertible(peerType, t, g.manualConversions) {
-		g.typesForInit = append(g.typesForInit, conversionPair{peerType, t})
-		convertible = true
-	}
-	return convertible
+
+	g.types = append(g.types, t)
+	return true
 }
 
 func (g *genConversion) isOtherPackage(pkg string) bool {
@@ -525,8 +445,10 @@ func (g *genConversion) Init(c *generator.Context, w io.Writer) error {
 	sw.Do("// Public to allow building arbitrary schemes.\n", nil)
 	sw.Do("func RegisterConversions(scheme $.|raw$) error {\n", schemePtr)
 	sw.Do("return scheme.AddGeneratedConversionFuncs(\n", nil)
-	for _, conv := range g.typesForInit {
-		sw.Do(nameTmpl+",\n", argsFromType(conv.inType, conv.outType))
+	for _, t := range g.types {
+		peerType := getPeerTypeFor(c, t, g.peerPackages)
+		sw.Do(nameTmpl+",\n", argsFromType(t, peerType))
+		sw.Do(nameTmpl+",\n", argsFromType(peerType, t))
 	}
 	sw.Do(")\n", nil)
 	sw.Do("}\n\n", nil)
@@ -535,33 +457,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 {
 	glog.V(5).Infof("generating for type %v", t)
-	sw := generator.NewSnippetWriter(w, c, "$", "$")
 	peerType := getPeerTypeFor(c, t, g.peerPackages)
-	didForward, didBackward := false, false
-	if isDirectlyConvertible(t, peerType, g.manualConversions) {
-		didForward = true
-		g.generateConversion(t, peerType, sw)
-	}
-	if isDirectlyConvertible(peerType, t, g.manualConversions) {
-		didBackward = true
-		g.generateConversion(peerType, t, sw)
-	}
-	if didForward != didBackward {
-		if didForward {
-			glog.Fatalf("Could only generate one direction of conversion for %v -> %v", t, peerType)
-		} else {
-			glog.Fatalf("Could only generate one direction of conversion for %v <- %v", t, peerType)
-		}
-	}
-	if !didForward && !didBackward {
-		// TODO: This should be fatal but we have at least 8 types that
-		// currently fail this.  The right thing to do is to figure out why they
-		// can't be generated and mark those fields as
-		// +k8s:conversion-gen=false, and ONLY do manual conversions for those
-		// fields, with the manual Convert_...() calling autoConvert_...()
-		// first.
-		glog.Errorf("Warning: could not generate autoConvert functions for %v <-> %v", t, peerType)
-	}
+	sw := generator.NewSnippetWriter(w, c, "$", "$")
+	g.generateConversion(t, peerType, sw)
+	g.generateConversion(peerType, t, sw)
 	return sw.Error()
 }
 
@@ -578,8 +477,17 @@ func (g *genConversion) generateConversion(inType, outType *types.Type, sw *gene
 	sw.Do("return nil\n", nil)
 	sw.Do("}\n\n", nil)
 
-	// If there is no public manual Conversion method, generate it.
-	if _, ok := g.preexists(inType, outType); !ok {
+	if _, found := g.preexists(inType, outType); found {
+		// There is a public manual Conversion method: use it.
+	} else if skipped := g.skippedFields[inType]; len(skipped) != 0 {
+		// The inType had some fields we could not generate.
+		glog.Errorf("Warning: could not find nor generate a final Conversion function for %v -> %v", inType, outType)
+		glog.Errorf("  the following fields need manual conversion:")
+		for _, f := range skipped {
+			glog.Errorf("      - %v", f)
+		}
+	} else {
+		// Emit a public conversion function.
 		sw.Do("func "+nameTmpl+"(in *$.inType|raw$, out *$.outType|raw$, s $.Scope|raw$) error {\n", args)
 		sw.Do("return auto"+nameTmpl+"(in, out, s)\n", args)
 		sw.Do("}\n\n", nil)
@@ -590,6 +498,7 @@ func (g *genConversion) generateConversion(inType, outType *types.Type, sw *gene
 // at any nesting level. This makes the autogenerator easy to understand, and
 // the compiler shouldn't care.
 func (g *genConversion) generateFor(inType, outType *types.Type, sw *generator.SnippetWriter) {
+	glog.V(5).Infof("generating %v -> %v", inType, outType)
 	var f func(*types.Type, *types.Type, *generator.SnippetWriter)
 	switch inType.Kind {
 	case types.Builtin:
@@ -717,6 +626,7 @@ func (g *genConversion) doStruct(inType, outType *types.Type, sw *generator.Snip
 			copied.Name = outMemberType.Name
 			outMemberType = &copied
 		}
+
 		args := map[string]interface{}{
 			"inType":  inMemberType,
 			"outType": outMemberType,
@@ -730,6 +640,14 @@ func (g *genConversion) doStruct(inType, outType *types.Type, sw *generator.Snip
 			sw.Do("}\n", nil)
 			continue
 		}
+
+		// If we can't auto-convert, punt before we emit any code.
+		if inMemberType.Kind != outMemberType.Kind {
+			sw.Do("// WARNING: field '"+inMember.Name+"' requires manual conversion\n", nil)
+			g.skippedFields[inType] = append(g.skippedFields[inType], inMember.Name)
+			continue
+		}
+
 		switch inMemberType.Kind {
 		case types.Builtin:
 			if inMemberType == outMemberType {
@@ -793,7 +711,7 @@ func (g *genConversion) doStruct(inType, outType *types.Type, sw *generator.Snip
 }
 
 func (g *genConversion) isDirectlyAssignable(inType, outType *types.Type) bool {
-	return inType == outType || areTypesAliased(inType, outType)
+	return unwrapAlias(inType) == unwrapAlias(outType)
 }
 
 func (g *genConversion) doPointer(inType, outType *types.Type, sw *generator.SnippetWriter) {

federation/apis/core/v1/zz_generated.conversion.go

@@ -0,0 +1,35 @@
+// +build !ignore_autogenerated
+
+/*
+Copyright 2016 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+// This file was autogenerated by conversion-gen. Do not edit it manually!
+
+package v1
+
+import (
+	runtime "k8s.io/kubernetes/pkg/runtime"
+)
+
+func init() {
+	SchemeBuilder.Register(RegisterConversions)
+}
+
+// RegisterConversions adds conversion functions to the given scheme.
+// Public to allow building arbitrary schemes.
+func RegisterConversions(scheme *runtime.Scheme) error {
+	return scheme.AddGeneratedConversionFuncs()
+}

pkg/apis/apps/v1alpha1/zz_generated.conversion.go

@@ -22,6 +22,7 @@ package v1alpha1
 
 import (
 	api "k8s.io/kubernetes/pkg/api"
+	v1 "k8s.io/kubernetes/pkg/api/v1"
 	apps "k8s.io/kubernetes/pkg/apis/apps"
 	conversion "k8s.io/kubernetes/pkg/conversion"
 	runtime "k8s.io/kubernetes/pkg/runtime"
@@ -139,6 +140,50 @@ func Convert_apps_PetSetList_To_v1alpha1_PetSetList(in *apps.PetSetList, out *Pe
 	return autoConvert_apps_PetSetList_To_v1alpha1_PetSetList(in, out, s)
 }
 
+func autoConvert_v1alpha1_PetSetSpec_To_apps_PetSetSpec(in *PetSetSpec, out *apps.PetSetSpec, s conversion.Scope) error {
+	// WARNING: field 'Replicas' requires manual conversion
+	out.Selector = in.Selector
+	if err := v1.Convert_v1_PodTemplateSpec_To_api_PodTemplateSpec(&in.Template, &out.Template, s); err != nil {
+		return err
+	}
+	if in.VolumeClaimTemplates != nil {
+		in, out := &in.VolumeClaimTemplates, &out.VolumeClaimTemplates
+		*out = make([]api.PersistentVolumeClaim, len(*in))
+		for i := range *in {
+			// TODO: Inefficient conversion - can we improve it?
+			if err := s.Convert(&(*in)[i], &(*out)[i], 0); err != nil {
+				return err
+			}
+		}
+	} else {
+		out.VolumeClaimTemplates = nil
+	}
+	out.ServiceName = in.ServiceName
+	return nil
+}
+
+func autoConvert_apps_PetSetSpec_To_v1alpha1_PetSetSpec(in *apps.PetSetSpec, out *PetSetSpec, s conversion.Scope) error {
+	// WARNING: field 'Replicas' requires manual conversion
+	out.Selector = in.Selector
+	if err := v1.Convert_api_PodTemplateSpec_To_v1_PodTemplateSpec(&in.Template, &out.Template, s); err != nil {
+		return err
+	}
+	if in.VolumeClaimTemplates != nil {
+		in, out := &in.VolumeClaimTemplates, &out.VolumeClaimTemplates
+		*out = make([]v1.PersistentVolumeClaim, len(*in))
+		for i := range *in {
+			// TODO: Inefficient conversion - can we improve it?
+			if err := s.Convert(&(*in)[i], &(*out)[i], 0); err != nil {
+				return err
+			}
+		}
+	} else {
+		out.VolumeClaimTemplates = nil
+	}
+	out.ServiceName = in.ServiceName
+	return nil
+}
+
 func autoConvert_v1alpha1_PetSetStatus_To_apps_PetSetStatus(in *PetSetStatus, out *apps.PetSetStatus, s conversion.Scope) error {
 	out.ObservedGeneration = in.ObservedGeneration
 	out.Replicas = int(in.Replicas)

pkg/apis/extensions/v1beta1/zz_generated.conversion.go

@@ -941,6 +941,18 @@ func Convert_autoscaling_HorizontalPodAutoscalerList_To_v1beta1_HorizontalPodAut
 	return autoConvert_autoscaling_HorizontalPodAutoscalerList_To_v1beta1_HorizontalPodAutoscalerList(in, out, s)
 }
 
+func autoConvert_v1beta1_HorizontalPodAutoscalerSpec_To_autoscaling_HorizontalPodAutoscalerSpec(in *HorizontalPodAutoscalerSpec, out *autoscaling.HorizontalPodAutoscalerSpec, s conversion.Scope) error {
+	out.MinReplicas = in.MinReplicas
+	out.MaxReplicas = in.MaxReplicas
+	return nil
+}
+
+func autoConvert_autoscaling_HorizontalPodAutoscalerSpec_To_v1beta1_HorizontalPodAutoscalerSpec(in *autoscaling.HorizontalPodAutoscalerSpec, out *HorizontalPodAutoscalerSpec, s conversion.Scope) error {
+	out.MinReplicas = in.MinReplicas
+	out.MaxReplicas = in.MaxReplicas
+	return nil
+}
+
 func autoConvert_v1beta1_HorizontalPodAutoscalerStatus_To_autoscaling_HorizontalPodAutoscalerStatus(in *HorizontalPodAutoscalerStatus, out *autoscaling.HorizontalPodAutoscalerStatus, s conversion.Scope) error {
 	out.ObservedGeneration = in.ObservedGeneration
 	out.LastScaleTime = in.LastScaleTime
@@ -1432,6 +1444,44 @@ func Convert_batch_JobList_To_v1beta1_JobList(in *batch.JobList, out *JobList, s
 	return autoConvert_batch_JobList_To_v1beta1_JobList(in, out, s)
 }
 
+func autoConvert_v1beta1_JobSpec_To_batch_JobSpec(in *JobSpec, out *batch.JobSpec, s conversion.Scope) error {
+	out.Parallelism = in.Parallelism
+	out.Completions = in.Completions
+	out.ActiveDeadlineSeconds = in.ActiveDeadlineSeconds
+	if in.Selector != nil {
+		in, out := &in.Selector, &out.Selector
+		*out = new(unversioned.LabelSelector)
+		if err := Convert_v1beta1_LabelSelector_To_unversioned_LabelSelector(*in, *out, s); err != nil {
+			return err
+		}
+	} else {
+		out.Selector = nil
+	}
+	if err := v1.Convert_v1_PodTemplateSpec_To_api_PodTemplateSpec(&in.Template, &out.Template, s); err != nil {
+		return err
+	}
+	return nil
+}
+
+func autoConvert_batch_JobSpec_To_v1beta1_JobSpec(in *batch.JobSpec, out *JobSpec, s conversion.Scope) error {
+	out.Parallelism = in.Parallelism
+	out.Completions = in.Completions
+	out.ActiveDeadlineSeconds = in.ActiveDeadlineSeconds
+	if in.Selector != nil {
+		in, out := &in.Selector, &out.Selector
+		*out = new(LabelSelector)
+		if err := Convert_unversioned_LabelSelector_To_v1beta1_LabelSelector(*in, *out, s); err != nil {
+			return err
+		}
+	} else {
+		out.Selector = nil
+	}
+	if err := v1.Convert_api_PodTemplateSpec_To_v1_PodTemplateSpec(&in.Template, &out.Template, s); err != nil {
+		return err
+	}
+	return nil
+}
+
 func autoConvert_v1beta1_JobStatus_To_batch_JobStatus(in *JobStatus, out *batch.JobStatus, s conversion.Scope) error {
 	if in.Conditions != nil {
 		in, out := &in.Conditions, &out.Conditions
@@ -2282,6 +2332,18 @@ func Convert_extensions_RollbackConfig_To_v1beta1_RollbackConfig(in *extensions.
 	return autoConvert_extensions_RollbackConfig_To_v1beta1_RollbackConfig(in, out, s)
 }
 
+func autoConvert_v1beta1_RollingUpdateDeployment_To_extensions_RollingUpdateDeployment(in *RollingUpdateDeployment, out *extensions.RollingUpdateDeployment, s conversion.Scope) error {
+	// WARNING: field 'MaxUnavailable' requires manual conversion
+	// WARNING: field 'MaxSurge' requires manual conversion
+	return nil
+}
+
+func autoConvert_extensions_RollingUpdateDeployment_To_v1beta1_RollingUpdateDeployment(in *extensions.RollingUpdateDeployment, out *RollingUpdateDeployment, s conversion.Scope) error {
+	// WARNING: field 'MaxUnavailable' requires manual conversion
+	// WARNING: field 'MaxSurge' requires manual conversion
+	return nil
+}
+
 func autoConvert_v1beta1_RunAsUserStrategyOptions_To_extensions_RunAsUserStrategyOptions(in *RunAsUserStrategyOptions, out *extensions.RunAsUserStrategyOptions, s conversion.Scope) error {
 	out.Rule = extensions.RunAsUserStrategy(in.Rule)
 	if in.Ranges != nil {
@@ -2420,6 +2482,18 @@ func Convert_extensions_ScaleSpec_To_v1beta1_ScaleSpec(in *extensions.ScaleSpec,
 	return autoConvert_extensions_ScaleSpec_To_v1beta1_ScaleSpec(in, out, s)
 }
 
+func autoConvert_v1beta1_ScaleStatus_To_extensions_ScaleStatus(in *ScaleStatus, out *extensions.ScaleStatus, s conversion.Scope) error {
+	out.Replicas = in.Replicas
+	// WARNING: field 'Selector' requires manual conversion
+	return nil
+}
+
+func autoConvert_extensions_ScaleStatus_To_v1beta1_ScaleStatus(in *extensions.ScaleStatus, out *ScaleStatus, s conversion.Scope) error {
+	out.Replicas = in.Replicas
+	// WARNING: field 'Selector' requires manual conversion
+	return nil
+}
+
 func autoConvert_v1beta1_SupplementalGroupsStrategyOptions_To_extensions_SupplementalGroupsStrategyOptions(in *SupplementalGroupsStrategyOptions, out *extensions.SupplementalGroupsStrategyOptions, s conversion.Scope) error {
 	out.Rule = extensions.SupplementalGroupsStrategyType(in.Rule)
 	if in.Ranges != nil {