Move SliceException from ZeroC.Slice to IceRpc.Slice

[bernardnormier]

@bentoi is proposing to move SliceException to IceRpc.Slice.

See https://github.com/icerpc/slicec/issues/624#issuecomment-1660739811 and following comments.

[bernardnormier]

Let's assume Benoit's code works:

• the ZeroC.Slice code helps decode exceptions like classes but doesn't know the base SliceException (only System.Exception)
• some helper code in IceRpc.Slice decodes the IceRpc.Slice.Exception using the ZeroC.Slice code

Making exception code generation RPC-dependent would not create a proxy-like difficulty because struct/class etc can't reference ("encode") exceptions.

It would however create a different difficulty: the exception code generation is somewhat complex and very class-like. It would be odd to have the class code generation in one code generator (the "base" code generator) and the code generation for exceptions in a different code generator (the IceRPC code generator). Currently, these 2 code generators are combined, but the goal is to separate them.

So, say doable, but with downsides.

@bentoi, what would we gain with this move?

[bentoi]

My understanding of the discussion on https://github.com/icerpc/slicec/issues/624#issuecomment-1660739811 is that we'd have two exception kinds

• the DispatchException<T> holder for Slice2 errors
• the SliceException base class for Slice1 exceptions

I find this confusing and I'd rather have a single base SliceException class with a derived SliceException<T> error holder for Slice2 and a derived generated exception for Slice1 exceptions.

[bentoi]

This base class could also be named DispatchException.

[bernardnormier]

Are you proposing the following:

classDiagram
    class DispatchException["IceRpc.Slice.DispatchException"] {
        +StatusCode StatusCode
    }
    class DispatchExceptionT["DispatchException{TError}"] {
       +TError Error
    }
    class SliceException {
        +Encode()
        +Decode()
    }
    SystemException <|-- DispatchException
    DispatchException <|-- DispatchExceptionT
    DispatchException  <|-- SliceException
    SliceException <|-- AdapterNotFoundException

Where SliceException is the base class for all Slice-defined exceptions (and is defined in IceRpc.Slice), with Encode/Decode helper methods used by the code generator. Just like the code generator relies on ZeroC.Slice.SliceClass for class encoding/decoding.

AdapterNotFoundException is some random Slice-defined exception mapped to C#.

The advantage of this setup is the common base (DispatchException). We don't have such a common base now. Is it worth the difficulty associated with moving SliceException to IceRpc.Slice?

[bentoi]

I was thinking we would get rid of SliceException in the diagram above. I suppose the reason we want keep it is because we don't want to (or can't?) have Decode/Encode on DispatchException.

[bernardnormier]

Without SliceException, the public Encode & Decode methods on DispatchException would throw NotSupportedException/NotImplementedException unless overridden by a generated Slice exception? This is not very elegant.

[bernardnormier]

For the record, the alternative (and my current proposal) is:

classDiagram
    class DispatchException["IceRpc.Slice.DispatchException"] {
        +StatusCode StatusCode
    }
    class DispatchExceptionT["IceRpc.Slice.DispatchException{TError}"] {
       +TError Error
    }
    class SliceException["ZeroC.Slice.SliceException"] {
        +Encode()
        +Decode()
    }
    SystemException <|-- DispatchException
    SystemException  <|-- DispatchExceptionT
    SystemException <|-- SliceException
    SliceException <|-- AdapterNotFoundException

I.e. similar classes but with less inheritance.

[bernardnormier]

It would however create a different difficulty: the exception code generation is somewhat complex and very class-like

Probably not a big deal. Another RPC framework that uses the Slice1 compilation mode is unlikely. Such a RPC framework could also decide to not support exceptions at all, especially if they no longer appear in Slice operations (see https://github.com/icerpc/slicec/issues/645).

[bentoi]

Without SliceException, the public Encode & Decode methods on DispatchException would throw NotSupportedException/NotImplementedException unless overridden by a generated Slice exception? This is not very elegant.

If they aren't needed for DispatchException<T>, I guess they would.

It would indeed be better to avoid having such methods if they are only implemented by a specific set of extensions.

[bernardnormier]

DispatchException<TError> doesn't need an Encode/Decode: the generated code knows how to encode/decode TError.

[bentoi]

I have a preference for the hierarchy from your https://github.com/icerpc/icerpc-csharp/issues/3577#issuecomment-1662392186 It provides a catch-all common base class and is more logical from an application perspective given that exceptions are really only useful for the IceRPC + Slice integration. I can't comment on the difficulty to support this with the future plugin based slice compiler. If it's really too complicated, let's go with the non-common base class option.

[bernardnormier]

I can't comment on the difficulty to support this with the future plugin based slice compiler

It's hard to say. We have currently separate generators for classes and exceptions: https://github.com/icerpc/icerpc-csharp/blob/main/tools/slicec-cs/src/generators/exception_generator.rs

So it's not like we use the same code. It's just the logic that is similar.

I think it's fine to make IceRpc.Slice.DispatchException the base class for all dispatch exceptions thrown by a Slice invocation, including Slice-defined exceptions. Your draft PR did it mostly already.

[bernardnormier]

We decided not to do it after all.