C# protoc plugin: deprecated option in proto definitions should add Obsolete attribute to generated c# code

[mrt181]

Is your feature request related to a problem? Please describe.

Adding the Obsolete attribute to an rpc method that is overriden warns that the base method is non-obselete:
Obsolete member 'SomeService.SomeMethod(SomeRequest, ServerCallContext)' overrides non-obsolete member 'SomeService.SomeServiceBase.SomeMethod(SomeRequest, ServerCallContext)' [SomeService]csharp(CS0809)

The proto definitions look like this:

service SomeService {
  rpc SomeMethod (SomeRequest) returns (SomeReply) {
    option deprecated = true;
  }

This is ignored in the generated code.

      [global::System.CodeDom.Compiler.GeneratedCode("grpc_csharp_plugin", null)]
      public virtual global::System.Threading.Tasks.Task<global::GrpcSomeService.SomeReply> SomeMethod(global::GrpcSomeService.SomeRequest request, grpc::ServerCallContext context)

It also does not work on the enum level:

enum SomeEnum {
  option deprecated = true;
  A = 0 [deprecated = true];
}

No attribute is added:

#region Enums
  public enum SomeEnum {
    [pbr::OriginalName("A")] A = 0,
}

It works on the message level:

message SomeRequest {
  option deprecated = true;
  SomeEnum somefield = 1;
}

Which adds the Obsolete attribute:

  [global::System.ObsoleteAttribute]
  public sealed partial class SomeRequest : pb::IMessage<SomeRequest>

Describe the solution you'd like

Add the Obsolete attribute on the generated code when the deprecated option is set in the proto definition.

Describe alternatives you've considered

ignore warnings and use additional documentation and communication.

[jtattermusch]

Since the issue description mentions deprecated option for Enum values, see also protocolbuffers/protobuf#10513

[tonydnewell]

For info: I've looked at the different places that the deprecated option can be used in proto files and compared the code generated by C# and Java. In all cases Java generates code with the correct @Deprecated attribute but the C# generator only currently supports a few cases:

• file level - not supported
• message - supported - [global::System.ObsoleteAttribute] added to the class
• field - supported - [global::System.ObsoleteAttribute] added to the class that wraps access to the field
• enum - not supported - but support added by Apply Obsolete attribute to deprecated enums and enum values in C# generated code protocolbuffers/protobuf#10520 but that version is not in the gRPC github repo
• enum value - not supported - but support added by Apply Obsolete attribute to deprecated enums and enum values in C# generated code protocolbuffers/protobuf#10520 but that version is not in the gRPC github repo
• service - not supported - would need to be done in gRPC plugin
• method - not supported - would need to be done in gRPC plugin

@jskeet Can you think of any downside or gotchas if we add deprecated option support everywhere for C#? Anything we would break?

[jskeet]

Not sure what the file level would look like - on the reflection class, maybe?

Other than that, as you say I think it's only a matter of adding it to service and method, then updating the version of protoc in the gRPC github repo.

[tonydnewell]

FYI: the support for deprecating enums and enum values is not yet released - it is in Protocol Buffers v22.0-rc1. The gRPC repo currently has 21.12 as the protocol buffers submodule.

[jskeet]

Note: Google.Protobuf 3.22.0 has now been released.

Additionally, I'd like to bring attention to the current state of affairs when a request/response pair are deprecated:

• The message types are marked as obsolete in the "plain" protoc code
• This causes warnings or errors on the gRPC generated code, on the private static marshaller/method fields, as well as the public methods that accept/return those types

Currently for Google Cloud libraries we have to work around that by inserting a suppression pragma at the start of the gRPC-generated code.

Marking the methods and fields (in the gRPC generated code) as obsolete removes this warning - but it does lead to an interesting contradiction: if the RPC is not marked as deprecated, but the request and/or response messages are, should the message be marked as obsolete? (I think it's easiest to mark it as obsolete if any of the request/response/rpc is deprecated.)

[tonydnewell]

@jskeet Propagating the deprecated ([Obsolete]) attribute up to the service method if any of the messages are deprecated is possible. However this does push the warnings elsewhere in the generated code - to within the BindService methods.

Do we want to marked then bind methods as deprecated too? Or just disable obsolete warnings for those methods? Or we could just disable obsolete warnings for all the generated code (like you are doing by inserting the pragma yourself)?

Is there any advantage in not disabling obsolete warnings for all the generated code since the user will find out the deprecated methods and messages themselves in their own code?

[jskeet]

Is there any advantage in not disabling obsolete warnings for all the generated code since the user will find out the deprecated methods and messages themselves in their own code?

Personally I think this is the simplest approach. You could even do it unconditionally, whether there's anything obsolete or not - with the downside that it would introduce a change to generated code which is unnecessary for most users.

[apolcyn]

fixed by #32414