icerpc / icerpc-csharp

A C# RPC framework built for QUIC, with bidirectional streaming, first-class async/await, and Protobuf support.
https://docs.icerpc.dev
Apache License 2.0
107 stars 13 forks source link

Simplify TypeContext #3883

Open bernardnormier opened 9 months ago

bernardnormier commented 9 months ago

The TypeContext enum is too complicated:

pub enum TypeContext {
    /// Used when generating the types of fields in structs, classes, and exceptions.
    Field,
    /// Used when generating the types of operation parameters, and return types in places where
    /// they're being decoded.
    Decode,
    /// Used when generating the types of operation parameters, and return types in places where
    /// they're being encoded.
    Encode,
    /// Used when generating types that are parts of other types, such as the success & failure types of results,
    /// the key & value types of dictionaries, or the element type of a sequence.
    Nested,
}

First, we should always treat Field and Nested the same, so there is no need for 2 enumerators. It should always be Field. Then, Encode/Decode are confusing. I propose to rename them IncomingParam / OutgoingParam.

bernardnormier commented 9 months ago

We should also review how we use TypeContext.

It looks like we use it for at least 2 separate purposes: 1) in cs_type_string, to figure out how to map a type based on the "context" (which seems fine) 2) in encode_type and friends, for the special handling for sequence parameters; this does not seem correct to me.

InsertCreativityHere commented 9 months ago

The original proposal was implemented in 2685accd63ad2deb8dccc7523a217a1b7ebd78ca.

But I agree, more simplification seems possible here.

InsertCreativityHere commented 8 months ago

More cleanup has been done, the biggest being #3893. Now, we no longer use TypeContext for the functions that have replaced cs_type_string.

Now it's really only used in the proxy and dispatch generation code, and the functions that it calls into. But, there's room for extra improvement, since the proxy side only uses OutgoingParam and Field. And the dispatch side only uses IncomingParam and Field.

So in reality, it behaves more like a bool, and we only need 2 of it's values at any given time.