icerpc / slicec

The Slice compiler library
Apache License 2.0
13 stars 5 forks source link

Refactor proxy-as-data-type #673

Closed bernardnormier closed 9 months ago

bernardnormier commented 10 months ago

Currently, when you define a Slice interface X, you get a data type that you can encode/decode - and for example use as a struct field or operation parameter. This data type is a "proxy" to a service that implements X.

This is problematic for a number of reasons: a) Unlike other user-defined types, the encoded representation of this type is not intuitive. You see an enum, you have some idea of what an enum parameter looks like "over the wire". The same isn't true for a proxy.

b) The mapping for interfaces is RPC-specific, but the encoding/decoding of data types is not, so we can't have the clean-separation:

since defining an interface automatically defines a proxy data type.

c) For many applications, this Slice-level proxy data type is never used. It's not that common to pass a proxy as a parameter or return value.

d) Passing proxies around in Slice operations may not be a good design choice. And arguably this feature (any Slice interface gives you automatically a proxy data type) encourages this design.

Proposal

a) Defining an interface no longer defines a Slice-level encodable proxy data type.

For example, the following definition would no longer compile:

interface Widget {} // no longer a data type

interface WidgetFactory {
    create() -> Widget // error
}

b) Allow the user to "opt-in" the old behavior with a custom type. For example:

module FooBar

interface Widget {}

// Encodes / decodes WidgetProxy as a service address; its C# API is FooBar.WidgetProxy
[cs::type("FooBar.WidgetProxy")]
custom WidgetProxy

interface WidgetFactory {
    create() -> WidgetProxy // returns a regular custom data type
}

It's just a regular custom data type that maps in C# to WidgetProxy.

The only special feature is as long as the user calls this custom type InterfaceNameProxy, he doesn't need to provide the Encode and Decode methods. slicec generates them automatically alongside the proxy struct for this interface.

This keeps the convenience of using proxy as data types, but makes this convenience opt-in. It also provides a clean separation between Slice data type (fully RPC-independent) and interfaces (RPC-dependent).

Note that it's a breaking change can since you need to: i) add this custom type to your Slice file (the opt-int part), and ii) call this custom type NameProxy - not Name

InsertCreativityHere commented 10 months ago

Sounds fine to me. This shouldn't cost any functionality, and will finally finish separating protocol constructs out of Slice. The only downsides I see are slight:


We briefly discussed splitting off a 0.2 branch for slicec to put this feature on, but honestly, I think the easiest path is we update slicec-cs to use the latest slicec. If it encounters an enum with associated values, we emit a warning/error saying "not supported".

This way we keep slicec to a single active branch.

pepone commented 9 months ago

The special use of cs::custom, relying on a naming convention, doesn't look great. I think we should consider not providing a special mechanism. You can return a ServiceAddress and let the user construct the proxy object manually.

bernardnormier commented 9 months ago

The proposed custom is not special:

You can return a ServiceAddress and let the user construct the proxy object manually.

That's true, and this proposal is consistent with this logic. Say you have:

interface WidgetFactory {
    create() -> ServiceAddress // returns the service address of the new Widget
}

but you wish this API would be a bit more typed and convenient. In C#, you wish you'd get an instance of WidgetProxy, with this service address and the invoker and encode options of your choice. The solution is for you to create a custom that gives this more convenient API.

That's a little bit of work because creating your own custom requires you provide encode and decode methods. My proposal is to make your life a little easier when you name this custom NameProxy.

pepone commented 9 months ago

That's a little bit of work because creating your own custom requires you provide encode and decode methods. My proposal is to make your life a little easier when you name this custom NameProxy.

I think is fine if the users has to do this extra work, as we are saying here using proxies as data types should not be that common.

bernardnormier commented 9 months ago

I think it would be impractical to require this full custom impl, especially in an Ice - IceRPC interop or migration scenario, where the Ice Slice files take full advantage of proxies as data type.

We don't want to generate c# code from ice2slice.

InsertCreativityHere commented 9 months ago

The parser side of this (disallowing them as types) was implemented in #675, and the C# side was implemented by https://github.com/icerpc/icerpc-csharp/issues/3803.