Uploaded image for project: 'Thrift'
  1. Thrift
  2. THRIFT-4715

C# union "data" should be strongly-typed

Details

    • Improvement
    • Status: Closed
    • Minor
    • Resolution: Fixed
    • 0.12.0
    • 0.13.0
    • C# - Compiler
    • None
    • Patch Available

    Description

      `union` option for netcore generator was fixed in 0.12, but what it generates doesn't seem very user-friendly.

      Following thrift:
      ```thrift
      struct PlayMsg

      { 1: string url, }

      union RequestMsg

      { 1: PlayMsg Play, }

      ```

      Generates:

      public abstract partial class RequestMsg : TAbstractBase {
      public abstract void Write(TProtocol protocol);
      public readonly bool Isset;
      public abstract object Data { get; }
      protected RequestMsg(bool isset) {
      Isset = isset;
      }
      
      public class ___undefined : RequestMsg {
      public override object Data { get { return null; } }
      public ___undefined() : base(false) {}
      
      public override void Write(TProtocol protocol) {
      throw new TProtocolException( TProtocolException.INVALID_DATA, "Cannot persist an union type which is not set.");
      }
      
      }
      
      public class Play : RequestMsg {
      private PlayMsg _data;
      public override object Data { get { return _data; } }
      public Play(PlayMsg data) : base(true) {
      this._data = data;
      }
      //SNIP

      Usage:

      // RequestMsg message = ...
      switch (message)
      {
      case RequestMsg.Play msg:
          // Need a cast here T_T
          PlayMsg play = (PlayMsg)msg.Data;
      

      Is there a reason for the `public abstract object Data`?

      If we get rid of that and instead generate a strongly-type getter we don't need to cast `Data`:

      public class Play : RequestMsg{ 
          public PlayMsg Data { get; private set; }
          public Play(PlayMsg data) : base(true)
          { this.Data = data; }
      //SNIP
      

       

       

      I could have sworn it worked like that with the "csharp" generator in 0.11.0 but it generates the same now.

      Is the intended usage different from what I'm doing?

      Attachments

        Issue Links

          Activity

            vgotra, jensg any thoughts?

            jking3 James E. King III added a comment - vgotra , jensg any thoughts?
            jensg Jens Geyer added a comment -

            I did not check the example given yet, but object sounds not like a type I would like to have in my code. So I guess the point is legit. Can we have two separate tickets please?

             

            jensg Jens Geyer added a comment - I did not check the example given yet, but object sounds not like a type I would like to have in my code. So I guess the point is legit. Can we have two separate tickets please?  
            jeikabu J W added a comment -

            Created THRIFT-4722 for netcore

            jeikabu J W added a comment - Created THRIFT-4722 for netcore
            jensg Jens Geyer added a comment - - edited

            I try to make the best out of it w/o breaking too much code.

            You always have the option to use standard struct handling by omitting the union flag when invoking the Thrift compiler.

            jensg Jens Geyer added a comment - - edited I try to make the best out of it w/o breaking too much code. You always have the option to use standard struct handling by omitting the union flag when invoking the Thrift compiler.
            jensg Jens Geyer added a comment -

            jeikabu comments?

            jensg Jens Geyer added a comment - jeikabu comments?

            Note: lib/csharp and lib/netcore are likely to be deprecated or dropped in 0.13.0 in favor of netstd which would incorporate this fix.

            jking3 James E. King III added a comment - Note: lib/csharp and lib/netcore are likely to be deprecated or dropped in 0.13.0 in favor of netstd which would incorporate this fix.

            People

              jensg Jens Geyer
              jeikabu J W
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Time Tracking

                  Estimated:
                  Original Estimate - Not Specified
                  Not Specified
                  Remaining:
                  Remaining Estimate - 0h
                  0h
                  Logged:
                  Time Spent - 20m
                  20m