Add BindServiceAttribute

[JamesNK]

Add a BindServiceAttribute so .NET libraries can easily resolve the location of the bind service method for a gRPC service.

// @jtattermusch

grpc/grpc-dotnet#107

[JamesNK]

btw I don't have all of the C++ tools setup on this computer. Hopefully it compiles 🤞

[jtattermusch]

Formatting is off (we enforce clang format on C/C++ code).

+ tools/dockerfile/grpc_clang_format/clang_format_all_the_things.sh
--- /var/local/git/grpc/src/compiler/csharp_generator.cc	2019-03-24 20:29:22.482677999 +0000
+++ /tmp/tmp.b6a8NEqD8T	2019-03-24 20:32:14.650677999 +0000
@@ -383,7 +383,8 @@
       "$servicename$</summary>\n",
       "servicename", GetServiceClassName(service));
   out->Print(
-      "[grpc::BindService(typeof($classname$), nameof($classname$.BindService))]\n",
+      "[grpc::BindService(typeof($classname$), "
+      "nameof($classname$.BindService))]\n",
       "classname", GetServiceClassName(service));
   out->Print("public abstract partial class $name$\n", "name",
              GetServerClassName(service));

[jtattermusch]

A few more comments (and I pushed a fix for one of them and the regenerated code).

[jtattermusch]

nameof() is a C# 6 concept and as of now, the generated code does not require C# 6. I know C# 6 is pretty old already, but we can easily use a string "BindService" instead of nameof() and remove the concern.

[jtattermusch]

GetServiceClassName(service) only gives the name of the class, but we should probably use a fully qualified name to eliminate potential name collisions.

[JamesNK]

but we should probably use a fully qualified name to eliminate potential name collisions.

I thought about that but there are other places in the generated service that reference class names without fully qualifying them.

[jtattermusch]

nit: TBH i'm still not 100% convinced by the name [BindService], I don't feel the name fully conveys what the attribute stands for. Have we considered some alternative names?

[JamesNK]

I'm open to alternatives.

[ServiceBind(...)]
[ServiceBinder(...)]
[ServiceBindMethod(...)]
[BindMethod(...)]

[JunTaoLuo]

nit: 2019

[JamesNK]

Does anything else need to be done here? Can we merge this for 1.20? I'm ok with [BindServiceMethod] as the attribute name.

[jtattermusch]

@JamesNK I thought we left the exact attribute name and arguments open in relation to Server Reflection as "needs more thought".

[JamesNK]

I implemented server reflection here - https://github.com/grpc/grpc-dotnet/pull/192/files#diff-811a02dca4701b0d502640e0d18407b7R48

I've thought about it and there are three choices:

1. Change attribute name to something generic like ServiceDefinition and add DescriptorName to it. The problem with this is if there is ever a situation where the Descriptor and BindMethod are on different types then you're stuck because you're specifying the same type for both. Will this ever happen? Probably not, but might as well not limit ourselves unnecessarily.
2. Add a new attribute for the descriptor and have two attributes on the base class. ServiceDescriptor? Its clean but IMO overkill. BindMethod and Descriptor are always generated on the same class. If that ever changes we could always add a new attribute in the future.
3. Reuse BindMethodName to get the type. Simplest thing and it will always work unless codegen radically changes.

I'm ok with 3.

[JamesNK]

I slightly perfer [ServiceBindMethod] over the current [BindServiceMethod] name. But it isn't important to me.

[jtattermusch]

I slightly perfer [ServiceBindMethod] over the current [BindServiceMethod] name. But it isn't important to me.

I chose [BindServiceMethod] because the method is called BindService and I felt reversing the word order would only create confusion.

[jtattermusch]

LGTM.

[jtattermusch]

Known failures:
#18637
#18655
#18416