From a5fbdbe59021f62cec692204a5085447488823eb Mon Sep 17 00:00:00 2001 From: justinsb Date: Sun, 4 Aug 2024 15:20:53 -0400 Subject: [PATCH] WIP: Use MethodByName only with constants We would like to enable dead-code-elimination, as unblocked by https://go-review.googlesource.com/c/go/+/522436 --- upup/pkg/fi/cloudup/awstasks/render_test.go | 5 +- upup/pkg/fi/context.go | 56 +++++++++++++++------ util/pkg/reflectutils/walk.go | 47 ++++++++++++++--- 3 files changed, 86 insertions(+), 22 deletions(-) diff --git a/upup/pkg/fi/cloudup/awstasks/render_test.go b/upup/pkg/fi/cloudup/awstasks/render_test.go index ac12e21846846..028d80751eef8 100644 --- a/upup/pkg/fi/cloudup/awstasks/render_test.go +++ b/upup/pkg/fi/cloudup/awstasks/render_test.go @@ -26,6 +26,7 @@ import ( "k8s.io/kops/upup/pkg/fi" "k8s.io/kops/upup/pkg/fi/cloudup/awsup" "k8s.io/kops/upup/pkg/fi/cloudup/terraform" + "k8s.io/kops/util/pkg/reflectutils" ) type renderTest struct { @@ -59,14 +60,14 @@ func doRenderTests(t *testing.T, method string, cases []*renderTest) { err := func() error { // @step: invoke the rendering method of the target - resp := reflect.ValueOf(c.Resource).MethodByName(method).Call(inputs) + resp := reflectutils.GetMethodByName(reflect.ValueOf(c.Resource), method).Call(inputs) if err := resp[0].Interface(); err != nil { return err.(error) } // @step: invoke the target finish up in := []reflect.Value{reflect.ValueOf(make(map[string]fi.CloudupTask))} - resp = reflect.ValueOf(target).MethodByName("Finish").Call(in) + resp = reflectutils.GetMethodByName(reflect.ValueOf(target), "Finish").Call(in) if err := resp[0].Interface(); err != nil { return err.(error) } diff --git a/upup/pkg/fi/context.go b/upup/pkg/fi/context.go index 03e81c2f0050c..547513c9c2530 100644 --- a/upup/pkg/fi/context.go +++ b/upup/pkg/fi/context.go @@ -27,6 +27,7 @@ import ( "k8s.io/klog/v2" "k8s.io/kops/pkg/apis/kops" "k8s.io/kops/pkg/apis/nodeup" + "k8s.io/kops/util/pkg/reflectutils" "k8s.io/kops/util/pkg/vfs" ) @@ -196,19 +197,44 @@ func (c *Context[T]) Render(a, e, changes Task[T]) error { targetType := reflect.ValueOf(c.Target).Type() - var renderer *reflect.Method + var renderer *reflect.Value var rendererArgs []reflect.Value - - for i := 0; i < vType.NumMethod(); i++ { - method := vType.Method(i) - if !strings.HasPrefix(method.Name, "Render") { + rendererName := "" + + renderMethodNames := []string{"Render"} + + targetTypeName := fmt.Sprintf("%T", c.Target) + switch targetTypeName { + case "*awsup.AWSAPITarget": + renderMethodNames = append(renderMethodNames, "RenderAWS") + case "*azure.AzureAPITarget": + renderMethodNames = append(renderMethodNames, "RenderAzure") + case "*do.DOAPITarget": + renderMethodNames = append(renderMethodNames, "RenderDO") + case "*gce.GCEAPITarget": + renderMethodNames = append(renderMethodNames, "RenderGCE") + case "*hetzner.HetznerAPITarget": + renderMethodNames = append(renderMethodNames, "RenderHetzner") + case "*openstack.OpenstackAPITarget": + renderMethodNames = append(renderMethodNames, "RenderOpenstack") + case "*scaleway.ScwAPITarget": + renderMethodNames = append(renderMethodNames, "RenderScw") + case "*terraform.TerraformTarget": + renderMethodNames = append(renderMethodNames, "RenderTerraform") + default: + panic(fmt.Sprintf("targetType %q is not recognized", targetTypeName)) + } + for _, methodName := range renderMethodNames { + method := reflectutils.GetMethodByName(v, methodName) + if !method.IsValid() { continue } match := true + methodType := method.Type() var args []reflect.Value - for j := 0; j < method.Type.NumIn(); j++ { - arg := method.Type.In(j) + for j := 0; j < methodType.NumIn(); j++ { + arg := methodType.In(j) if arg.ConvertibleTo(vType) { continue } @@ -225,27 +251,29 @@ func (c *Context[T]) Render(a, e, changes Task[T]) error { } if match { if renderer != nil { - if method.Name == "Render" { + if methodName == "Render" { continue } - if renderer.Name != "Render" { - return fmt.Errorf("found multiple Render methods that could be involved on %T", e) + if rendererName != "Render" { + return fmt.Errorf("found multiple Render methods that could be invoked on %T", e) } } renderer = &method + rendererName = methodName rendererArgs = args } - } + if renderer == nil { return fmt.Errorf("could not find Render method on type %T (target %T)", e, c.Target) } + rendererArgs = append(rendererArgs, reflect.ValueOf(a)) rendererArgs = append(rendererArgs, reflect.ValueOf(e)) rendererArgs = append(rendererArgs, reflect.ValueOf(changes)) - klog.V(11).Infof("Calling method %s on %T", renderer.Name, e) - m := v.MethodByName(renderer.Name) - rv := m.Call(rendererArgs) + klog.V(11).Infof("Calling method %s on %T", rendererName, e) + // m := v.MethodByName(renderer.Name) + rv := (*renderer).Call(rendererArgs) var rvErr error if !rv[0].IsNil() { rvErr = rv[0].Interface().(error) diff --git a/util/pkg/reflectutils/walk.go b/util/pkg/reflectutils/walk.go index 9fd392d8ae4a5..446e70636b8ff 100644 --- a/util/pkg/reflectutils/walk.go +++ b/util/pkg/reflectutils/walk.go @@ -57,14 +57,50 @@ func JSONMergeStruct(dest, src interface{}) { } } +// GetMethodByName implements a switch statement so that we can help the compiler with dead-code-elimination. +// For background: https://github.com/golang/protobuf/issues/1561 +func GetMethodByName(reflectValue reflect.Value, methodName string) reflect.Value { + switch methodName { + case "CheckChanges": + return reflectValue.MethodByName("CheckChanges") + case "Find": + return reflectValue.MethodByName("Find") + case "ShouldCreate": + return reflectValue.MethodByName("ShouldCreate") + case "Finish": + return reflectValue.MethodByName("Finish") + case "Render": + return reflectValue.MethodByName("Render") + case "RenderAWS": + return reflectValue.MethodByName("RenderAWS") + case "RenderAzure": + return reflectValue.MethodByName("RenderAzure") + case "RenderDO": + return reflectValue.MethodByName("RenderDO") + case "RenderGCE": + return reflectValue.MethodByName("RenderGCE") + case "RenderHetzner": + return reflectValue.MethodByName("RenderHetzner") + case "RenderOpenstack": + return reflectValue.MethodByName("RenderOpenstack") + case "RenderScw": + return reflectValue.MethodByName("RenderScw") + case "RenderTerraform": + return reflectValue.MethodByName("RenderTerraform") + + default: + panic(fmt.Sprintf("need to add %q to getMethodByName helper", methodName)) + } +} + // InvokeMethod calls the specified method by reflection -func InvokeMethod(target interface{}, name string, args ...interface{}) ([]reflect.Value, error) { +func InvokeMethod(target interface{}, methodName string, args ...interface{}) ([]reflect.Value, error) { v := reflect.ValueOf(target) - method, found := v.Type().MethodByName(name) - if !found { + m := GetMethodByName(v, methodName) + if !m.IsValid() { return nil, &MethodNotFoundError{ - Name: name, + Name: methodName, Target: target, } } @@ -73,8 +109,7 @@ func InvokeMethod(target interface{}, name string, args ...interface{}) ([]refle for _, a := range args { argValues = append(argValues, reflect.ValueOf(a)) } - klog.V(12).Infof("Calling method %s on %T", method.Name, target) - m := v.MethodByName(method.Name) + klog.V(12).Infof("Calling method %s on %T", methodName, target) rv := m.Call(argValues) return rv, nil }