cmd/protoc-gen-go-grpc: test the embedded struct at registration time for proper usage (#7438)

Author: dfawley

balancer/grpclb/grpc_lb_v1/load_balancer_grpc.pb.go

@@ -19,3 +19,10 @@ E.g.:
 
 Note that this is not recommended, and the option is only provided to restore
 backward compatibility with previously-generated code.
+
+When embedding the `Unimplemented<ServiceName>Server` in a struct that
+implements the service, it should be embedded by _value_ instead of as a
+_pointer_.  If it is embedded as a pointer, it must be assigned to a valid,
+non-nil pointer or else unimplemented methods would panic when called.  This is
+tested at service registration time, and will lead to a panic in
+`Register<ServiceName>Server` if it is not embedded properly.

channelz/grpc_channelz_v1/channelz_grpc.pb.go

@@ -2,4 +2,16 @@ module google.golang.org/grpc/cmd/protoc-gen-go-grpc
 
 go 1.21
 
-require google.golang.org/protobuf v1.34.1
+require (
+	google.golang.org/grpc v1.65.0
+	google.golang.org/protobuf v1.34.1
+)
+
+require (
+	golang.org/x/net v0.26.0 // indirect
+	golang.org/x/sys v0.21.0 // indirect
+	golang.org/x/text v0.16.0 // indirect
+	google.golang.org/genproto/googleapis/rpc v0.0.0-20240604185151-ef581f913117 // indirect
+)
+
+replace google.golang.org/grpc => ../..

cmd/protoc-gen-go-grpc/README.md

@@ -1,6 +1,12 @@
-github.com/google/go-cmp v0.5.5 h1:Khx7svrCpmxxtHBq5j2mp/xVjsi8hQMfNLvJFAlrGgU=
-github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
-golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543 h1:E7g+9GITq07hpfrRu66IVDexMakfv52eLZ2CXBWiKr4=
-golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
+github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
+github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
+golang.org/x/net v0.26.0 h1:soB7SVo0PWrY4vPW/+ay0jKDNScG2X9wFeYlXIvJsOQ=
+golang.org/x/net v0.26.0/go.mod h1:5YKkiSynbBIh3p6iOc/vibscux0x38BZDkn8sCUPxHE=
+golang.org/x/sys v0.21.0 h1:rF+pYz3DAGSQAxAu1CbC7catZg4ebC4UIeIhKxBZvws=
+golang.org/x/sys v0.21.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
+golang.org/x/text v0.16.0 h1:a94ExnEXNtEwYLGJSIUxnWoxoRz/ZcCsV63ROupILh4=
+golang.org/x/text v0.16.0/go.mod h1:GhwF1Be+LQoKShO3cGOHzqOgRrGaYc9AvblQOmPVHnI=
+google.golang.org/genproto/googleapis/rpc v0.0.0-20240604185151-ef581f913117 h1:1GBuWVLM/KMVUv1t1En5Gs+gFZCNd360GGb4sSxtrhU=
+google.golang.org/genproto/googleapis/rpc v0.0.0-20240604185151-ef581f913117/go.mod h1:EfXuqaE1J41VCDicxHzUDm+8rk+7ZdXzHV0IhO/I6s0=
 google.golang.org/protobuf v1.34.1 h1:9ddQBjfCyZPOHPUiPxpYESBLc+T8P3E+Vo4IbKZgFWg=
 google.golang.org/protobuf v1.34.1/go.mod h1:c6P6GXX6sHbq/GpV6MGZEdwhWPcYBgnhAHhKbcUYpos=

cmd/protoc-gen-go-grpc/go.mod

@@ -84,9 +84,12 @@ func (serviceGenerateHelper) generateUnimplementedServerType(gen *protogen.Plugi
 		mustOrShould = "should"
 	}
 	// Server Unimplemented struct for forward compatibility.
-	g.P("// Unimplemented", serverType, " ", mustOrShould, " be embedded to have forward compatible implementations.")
-	g.P("type Unimplemented", serverType, " struct {")
-	g.P("}")
+	g.P("// Unimplemented", serverType, " ", mustOrShould, " be embedded to have")
+	g.P("// forward compatible implementations.")
+	g.P("//")
+	g.P("// NOTE: this should be embedded by value instead of pointer to avoid a nil")
+	g.P("// pointer dereference when methods are called.")
+	g.P("type Unimplemented", serverType, " struct {}")
 	g.P()
 	for _, method := range service.Methods {
 		nilArg := ""
@@ -100,6 +103,7 @@ func (serviceGenerateHelper) generateUnimplementedServerType(gen *protogen.Plugi
 	if *requireUnimplemented {
 		g.P("func (Unimplemented", serverType, ") mustEmbedUnimplemented", serverType, "() {}")
 	}
+	g.P("func (Unimplemented", serverType, ") testEmbeddedByValue() {}")
 	g.P()
 }
 
@@ -306,6 +310,13 @@ func genService(gen *protogen.Plugin, file *protogen.File, g *protogen.Generated
 	}
 	serviceDescVar := service.GoName + "_ServiceDesc"
 	g.P("func Register", service.GoName, "Server(s ", grpcPackage.Ident("ServiceRegistrar"), ", srv ", serverType, ") {")
+	g.P("// If the following call pancis, it indicates Unimplemented", serverType, " was")
+	g.P("// embedded by pointer and is nil.  This will cause panics if an")
+	g.P("// unimplemented method is ever invoked, so we test this at initialization")
+	g.P("// time to prevent it from happening at runtime later due to I/O.")
+	g.P("if t, ok := srv.(interface { testEmbeddedByValue() }); ok {")
+	g.P("t.testEmbeddedByValue()")
+	g.P("}")
 	g.P("s.RegisterService(&", serviceDescVar, `, srv)`)
 	g.P("}")
 	g.P()

cmd/protoc-gen-go-grpc/go.sum

@@ -0,0 +1,46 @@
+/*
+ *
+ * Copyright 2024 gRPC 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.
+ *
+ */
+
+package main_test
+
+import (
+	"testing"
+
+	"google.golang.org/grpc"
+	testgrpc "google.golang.org/grpc/interop/grpc_testing"
+)
+
+type unimplEmbeddedByPointer struct {
+	*testgrpc.UnimplementedTestServiceServer
+}
+
+type unimplEmbeddedByValue struct {
+	testgrpc.UnimplementedTestServiceServer
+}
+
+func TestUnimplementedEmbedding(t *testing.T) {
+	// Embedded by value, this should succeed.
+	testgrpc.RegisterTestServiceServer(grpc.NewServer(), &unimplEmbeddedByValue{})
+	defer func() {
+		if recover() == nil {
+			t.Fatalf("Expected panic; received none")
+		}
+	}()
+	// Embedded by pointer, this should panic.
+	testgrpc.RegisterTestServiceServer(grpc.NewServer(), &unimplEmbeddedByPointer{})
+}