From 50bcdb967717d4887ed5882b09d4e26005618ee0 Mon Sep 17 00:00:00 2001 From: Wojciech Tyczynski Date: Wed, 26 Oct 2016 10:21:45 +0200 Subject: [PATCH] Revert "Generate unsafe conversions" This reverts commit 2b1790cc5f2d297e6d935ffac4d6a3fdaae702bd. --- .../conversion-gen/generators/conversion.go | 265 ++++++------------ cmd/libs/go2idl/conversion-gen/main.go | 1 + 2 files changed, 80 insertions(+), 186 deletions(-) diff --git a/cmd/libs/go2idl/conversion-gen/generators/conversion.go b/cmd/libs/go2idl/conversion-gen/generators/conversion.go index 61f89b8fbe7..e1c62c247b0 100644 --- a/cmd/libs/go2idl/conversion-gen/generators/conversion.go +++ b/cmd/libs/go2idl/conversion-gen/generators/conversion.go @@ -21,7 +21,6 @@ import ( "fmt" "io" "path/filepath" - "sort" "strings" "k8s.io/gengo/args" @@ -37,11 +36,10 @@ import ( // generator. type CustomArgs struct { ExtraPeerDirs []string // Always consider these as last-ditch possibilities for conversions. - // Skipunsafe indicates whether to generate unsafe conversions to improve the efficiency - // of these operations. The unsafe operation is a direct pointer assignment via unsafe - // (within the allowed uses of unsafe) and is equivalent to a proposed Golang change to - // allow structs that are identical to be assigned to each other. - SkipUnsafe bool + // SkipDefaulters indicates whether defaulter functions should be a part of conversion + // This field was introduced to ease the transition to removing defaulters from conversion. + // It will be removed in 1.6. + SkipDefaulters bool } // This is the comment tag that carries parameters for conversion generation. @@ -51,16 +49,6 @@ func extractTag(comments []string) []string { return types.ExtractCommentTags("+", comments)[tagName] } -func isCopyOnly(comments []string) bool { - values := types.ExtractCommentTags("+", comments)["k8s:conversion-fn"] - return len(values) == 1 && values[0] == "copy-only" -} - -func isDrop(comments []string) bool { - values := types.ExtractCommentTags("+", comments)["k8s:conversion-fn"] - return len(values) == 1 && values[0] == "drop" -} - // TODO: This is created only to reduce number of changes in a single PR. // Remove it and use PublicNamer instead. func conversionNamer() *namer.NameStrategy { @@ -168,6 +156,56 @@ func getManualConversionFunctions(context *generator.Context, pkg *types.Package } } +// All of the types in conversions map are of type "DeclarationOf" with +// the underlying type being "Func". +type defaulterFuncMap map[*types.Type]*types.Type + +// Returns all manually-defined defaulting functions in the package. +func getManualDefaultingFunctions(context *generator.Context, pkg *types.Package, manualMap defaulterFuncMap) { + buffer := &bytes.Buffer{} + sw := generator.NewSnippetWriter(buffer, context, "$", "$") + + for _, f := range pkg.Functions { + if f.Underlying == nil || f.Underlying.Kind != types.Func { + glog.Errorf("Malformed function: %#v", f) + continue + } + if f.Underlying.Signature == nil { + glog.Errorf("Function without signature: %#v", f) + continue + } + signature := f.Underlying.Signature + // Check whether the function is conversion function. + // Note that all of them have signature: + // func Convert_inType_To_outType(inType, outType, conversion.Scope) error + if signature.Receiver != nil { + continue + } + if len(signature.Parameters) != 1 { + continue + } + if len(signature.Results) != 0 { + continue + } + inType := signature.Parameters[0] + if inType.Kind != types.Pointer { + continue + } + // Now check if the name satisfies the convention. + args := defaultingArgsFromType(inType.Elem) + sw.Do("$.inType|defaultfn$", args) + if f.Name.Name == buffer.String() { + key := inType.Elem + // We might scan the same package twice, and that's OK. + if v, ok := manualMap[key]; ok && v != nil && v.Name.Package != pkg.Path { + panic(fmt.Sprintf("duplicate static defaulter defined: %#v", key)) + } + manualMap[key] = f + } + buffer.Reset() + } +} + func Packages(context *generator.Context, arguments *args.GeneratorArgs) generator.Packages { boilerplate, err := arguments.LoadGoBoilerplate() if err != nil { @@ -179,17 +217,10 @@ func Packages(context *generator.Context, arguments *args.GeneratorArgs) generat header := append([]byte(fmt.Sprintf("// +build !%s\n\n", arguments.GeneratedBuildTag)), boilerplate...) header = append(header, []byte("\n// This file was autogenerated by conversion-gen. Do not edit it manually!\n\n")...) - // Accumulate pre-existing conversion functions. + // Accumulate pre-existing conversion and default functions. // TODO: This is too ad-hoc. We need a better way. manualConversions := conversionFuncMap{} - - // Record types that are memory equivalent. A type is memory equivalent - // if it has the same memory layout and no nested manual conversion is - // defined. - // TODO: in the future, relax the nested manual conversion requirement - // if we can show that a large enough types are memory identical but - // have non-trivial conversion - memoryEquivalentTypes := equalMemoryTypes{} + manualDefaults := defaulterFuncMap{} // We are generating conversions only for packages that are explicitly // passed as InputDir. @@ -203,6 +234,7 @@ func Packages(context *generator.Context, arguments *args.GeneratorArgs) generat // Add conversion and defaulting functions. getManualConversionFunctions(context, pkg, manualConversions) + getManualDefaultingFunctions(context, pkg, manualDefaults) // Only generate conversions for packages which explicitly request it // by specifying one or more "+k8s:conversion-gen=" @@ -214,23 +246,18 @@ func Packages(context *generator.Context, arguments *args.GeneratorArgs) generat glog.V(5).Infof(" no tag") continue } - skipUnsafe := false + skipDefaulters := false if customArgs, ok := arguments.CustomArgs.(*CustomArgs); ok { if len(customArgs.ExtraPeerDirs) > 0 { peerPkgs = append(peerPkgs, customArgs.ExtraPeerDirs...) } - skipUnsafe = customArgs.SkipUnsafe + skipDefaulters = customArgs.SkipDefaulters } - // Make sure our peer-packages are added and fully parsed. for _, pp := range peerPkgs { context.AddDir(pp) getManualConversionFunctions(context, context.Universe[pp], manualConversions) - } - - unsafeEquality := TypesEqual(memoryEquivalentTypes) - if skipUnsafe { - unsafeEquality = noEquality{} + getManualDefaultingFunctions(context, context.Universe[pp], manualDefaults) } packages = append(packages, @@ -240,7 +267,7 @@ func Packages(context *generator.Context, arguments *args.GeneratorArgs) generat HeaderText: header, GeneratorFunc: func(c *generator.Context) (generators []generator.Generator) { return []generator.Generator{ - NewGenConversion(arguments.OutputFileBaseName, pkg.Path, manualConversions, peerPkgs, unsafeEquality), + NewGenConversion(arguments.OutputFileBaseName, pkg.Path, manualConversions, manualDefaults, peerPkgs, !skipDefaulters), } }, FilterFunc: func(c *generator.Context, t *types.Type) bool { @@ -248,79 +275,9 @@ func Packages(context *generator.Context, arguments *args.GeneratorArgs) generat }, }) } - - // If there is a manual conversion defined between two types, exclude it - // from being a candidate for unsafe conversion - for k, v := range manualConversions { - if isCopyOnly(v.CommentLines) { - glog.V(5).Infof("Conversion function %s will not block memory copy because it is copy-only", v.Name) - continue - } - // this type should be excluded from all equivalence, because the converter must be called. - memoryEquivalentTypes.Skip(k.inType, k.outType) - } - return packages } -type equalMemoryTypes map[conversionPair]bool - -func (e equalMemoryTypes) Skip(a, b *types.Type) { - e[conversionPair{a, b}] = false - e[conversionPair{b, a}] = false -} - -func (e equalMemoryTypes) Equal(a, b *types.Type) bool { - if a == b { - return true - } - if equal, ok := e[conversionPair{a, b}]; ok { - return equal - } - if equal, ok := e[conversionPair{b, a}]; ok { - return equal - } - result := e.equal(a, b) - e[conversionPair{a, b}] = result - e[conversionPair{b, a}] = result - return result -} - -func (e equalMemoryTypes) equal(a, b *types.Type) bool { - in, out := unwrapAlias(a), unwrapAlias(b) - switch { - case in == out: - return true - case in.Kind == out.Kind: - switch in.Kind { - case types.Struct: - if len(in.Members) != len(out.Members) { - return false - } - for i, inMember := range in.Members { - outMember := out.Members[i] - if !e.Equal(inMember.Type, outMember.Type) { - return false - } - } - return true - case types.Pointer: - return e.Equal(in.Elem, out.Elem) - case types.Map: - return e.Equal(in.Key, out.Key) && e.Equal(in.Elem, out.Elem) - case types.Slice: - return e.Equal(in.Elem, out.Elem) - case types.Interface: - // TODO: determine whether the interfaces are actually equivalent - for now, they must have the - // same type. - return false - case types.Builtin: - return in.Name.Name == out.Name.Name - } - } - return false -} - func findMember(t *types.Type, name string) (types.Member, bool) { if t.Kind != types.Struct { return types.Member{}, false @@ -346,27 +303,20 @@ const ( conversionPackagePath = "k8s.io/kubernetes/pkg/conversion" ) -type noEquality struct{} - -func (noEquality) Equal(_, _ *types.Type) bool { return false } - -type TypesEqual interface { - Equal(a, b *types.Type) bool -} - // genConversion produces a file with a autogenerated conversions. type genConversion struct { generator.DefaultGen targetPackage string peerPackages []string manualConversions conversionFuncMap + manualDefaulters defaulterFuncMap imports namer.ImportTracker types []*types.Type skippedFields map[*types.Type][]string - useUnsafe TypesEqual + includeDefaulters bool } -func NewGenConversion(sanitizedName, targetPackage string, manualConversions conversionFuncMap, peerPkgs []string, useUnsafe TypesEqual) generator.Generator { +func NewGenConversion(sanitizedName, targetPackage string, manualConversions conversionFuncMap, manualDefaulters defaulterFuncMap, peerPkgs []string, includeDefaulters bool) generator.Generator { return &genConversion{ DefaultGen: generator.DefaultGen{ OptionalName: sanitizedName, @@ -374,10 +324,11 @@ func NewGenConversion(sanitizedName, targetPackage string, manualConversions con targetPackage: targetPackage, peerPackages: peerPkgs, manualConversions: manualConversions, + manualDefaulters: manualDefaulters, imports: generator.NewImportTracker(), types: []*types.Type{}, skippedFields: map[*types.Type][]string{}, - useUnsafe: useUnsafe, + includeDefaulters: includeDefaulters, } } @@ -488,22 +439,6 @@ func (g *genConversion) preexists(inType, outType *types.Type) (*types.Type, boo } func (g *genConversion) Init(c *generator.Context, w io.Writer) error { - if glog.V(5) { - if m, ok := g.useUnsafe.(equalMemoryTypes); ok { - var result []string - glog.Infof("All objects without identical memory layout:") - for k, v := range m { - if v { - continue - } - result = append(result, fmt.Sprintf(" %s -> %s = %t", k.inType, k.outType, v)) - } - sort.Strings(result) - for _, s := range result { - glog.Infof(s) - } - } - } sw := generator.NewSnippetWriter(w, c, "$", "$") sw.Do("func init() {\n", nil) sw.Do("SchemeBuilder.Register(RegisterConversions)\n", nil) @@ -542,6 +477,10 @@ func (g *genConversion) generateConversion(inType, outType *types.Type, sw *gene With("Scope", types.Ref(conversionPackagePath, "Scope")) sw.Do("func auto"+nameTmpl+"(in *$.inType|raw$, out *$.outType|raw$, s $.Scope|raw$) error {\n", args) + // if no defaulter of form SetDefaults_XXX is defined, do not inline a check for defaulting. + if function, ok := g.manualDefaulters[inType]; ok && g.includeDefaulters { + sw.Do("$.|raw$(in)\n", function) + } g.generateFor(inType, outType, sw) sw.Do("return nil\n", nil) sw.Do("}\n\n", nil) @@ -569,7 +508,6 @@ func (g *genConversion) generateConversion(inType, outType *types.Type, sw *gene 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: f = g.doBuiltin @@ -586,7 +524,6 @@ func (g *genConversion) generateFor(inType, outType *types.Type, sw *generator.S default: f = g.doUnknown } - f(inType, outType, sw) } @@ -701,51 +638,18 @@ func (g *genConversion) doStruct(inType, outType *types.Type, sw *generator.Snip outMemberType = &copied } - args := argsFromType(inMemberType, outMemberType).With("name", inMember.Name) - - // try a direct memory copy for any type that has exactly equivalent values - if g.useUnsafe.Equal(inMemberType, outMemberType) { - args = args. - With("Pointer", types.Ref("unsafe", "Pointer")). - With("SliceHeader", types.Ref("reflect", "SliceHeader")) - switch inMemberType.Kind { - case types.Pointer: - sw.Do("out.$.name$ = ($.outType|raw$)($.Pointer|raw$(in.$.name$))\n", args) - continue - case types.Map: - sw.Do("{\n", nil) - sw.Do("m := (*$.outType|raw$)($.Pointer|raw$(&in.$.name$))\n", args) - sw.Do("out.$.name$ = *m\n", args) - sw.Do("}\n", nil) - continue - case types.Slice: - sw.Do("{\n", nil) - sw.Do("outHdr := (*$.SliceHeader|raw$)($.Pointer|raw$(&out.$.name$))\n", args) - sw.Do("inHdr := (*$.SliceHeader|raw$)($.Pointer|raw$(&in.$.name$))\n", args) - sw.Do("*outHdr = *inHdr\n", nil) - sw.Do("}\n", nil) - continue - } + args := map[string]interface{}{ + "inType": inMemberType, + "outType": outMemberType, + "name": inMember.Name, } - // check based on the top level name, not the underlying names if function, ok := g.preexists(inMember.Type, outMember.Type); ok { - if isDrop(function.CommentLines) { - continue - } - // copy-only functions that are directly assignable can be inlined instead of invoked. - // As an example, conversion functions exist that allow types with private fields to be - // correctly copied between types. These functions are equivalent to a memory assignment, - // and are necessary for the reflection path, but should not block memory conversion. - // Convert_unversioned_Time_to_unversioned_Time is an example of this logic. - if !isCopyOnly(function.CommentLines) || !g.isFastConversion(inMemberType, outMemberType) { - 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 - } - glog.V(5).Infof("Skipped function %s because it is copy-only and we can use direct assignment", function.Name) + 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 } // If we can't auto-convert, punt before we emit any code. @@ -818,17 +722,6 @@ func (g *genConversion) doStruct(inType, outType *types.Type, sw *generator.Snip } } -func (g *genConversion) isFastConversion(inType, outType *types.Type) bool { - switch inType.Kind { - case types.Builtin: - return true - case types.Map, types.Slice, types.Pointer, types.Struct, types.Alias: - return g.isDirectlyAssignable(inType, outType) - default: - return false - } -} - func (g *genConversion) isDirectlyAssignable(inType, outType *types.Type) bool { return unwrapAlias(inType) == unwrapAlias(outType) } diff --git a/cmd/libs/go2idl/conversion-gen/main.go b/cmd/libs/go2idl/conversion-gen/main.go index 79f515e5ad0..097c0ea8dba 100644 --- a/cmd/libs/go2idl/conversion-gen/main.go +++ b/cmd/libs/go2idl/conversion-gen/main.go @@ -60,6 +60,7 @@ func main() { "k8s.io/kubernetes/pkg/conversion", "k8s.io/kubernetes/pkg/runtime", }, + SkipDefaulters: true, } pflag.CommandLine.StringSliceVar(&customArgs.ExtraPeerDirs, "extra-peer-dirs", customArgs.ExtraPeerDirs, "Comma-separated list of import paths which are considered, after tag-specified peers, for conversions.")