Thrift
  1. Thrift
  2. THRIFT-1127

C# should not generate default constructor

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 0.5
    • Fix Version/s: None
    • Component/s: C# - Compiler
    • Labels:
      None

      Description

      The C# code generator should not produce a default constructor.

      Thrift generates partial classes for thrift structs, meaning that the class may be spread across multiple files and csc will link them to be a separate file. When the thrift generated class has the partial constructor, it cannot be added in other files. This is a problem if you want to implement a default constructor that does some initialization to the data in the class.

      For example, this thrift code:

      struct DateTime
      {
          1: required i64 ticks,
      }
      

      produces

      /**
       * Autogenerated by Thrift
       *
       * DO NOT EDIT UNLESS YOU ARE SURE THAT YOU KNOW WHAT YOU ARE DOING
       */
      using System;
      using System.Collections;
      using System.Collections.Generic;
      using System.Text;
      using System.IO;
      using Thrift;
      using Thrift.Collections;
      using Thrift.Protocol;
      using Thrift.Transport;
      namespace Thrift.Generated
      {
      
        [Serializable]
        public partial class DateTime : TBase
        {
          private long _ticks;
      
          public long Ticks
          {
            get
            {
              return _ticks;
            }
            set
            {
              __isset.ticks = true;
              this._ticks = value;
            }
          }
      
      
          public Isset __isset;
          [Serializable]
          public struct Isset {
            public bool ticks;
          }
      
          public DateTime() {
          }
      
          public void Read (TProtocol iprot)
      ...
      

      It would be great if it instead produced code like this:

      /**
       * Autogenerated by Thrift
       *
       * DO NOT EDIT UNLESS YOU ARE SURE THAT YOU KNOW WHAT YOU ARE DOING
       */
      using System;
      using System.Collections;
      using System.Collections.Generic;
      using System.Text;
      using System.IO;
      using Thrift;
      using Thrift.Collections;
      using Thrift.Protocol;
      using Thrift.Transport;
      namespace Thrift.Generated
      {
      
        [Serializable]
        public partial class DateTime : TBase
        {
          private long _ticks;
      
          public long Ticks
          {
            get
            {
              return _ticks;
            }
            set
            {
              __isset.ticks = true;
              this._ticks = value;
            }
          }
      
      
          public Isset __isset;
          [Serializable]
          public struct Isset {
            public bool ticks;
          }
      
          public void Read (TProtocol iprot)
      

        Activity

        Hide
        Thunder Stumpges added a comment -

        The point of the default constructor is to initialize member default values. However in a case where you have none (or need to have none in order to provide your own), I agree that it should not generate one. I am working on a patch that will generate the default constructor (as it does today) only if there are one or more members that have default values specified. I believe this should work for you (it works for me!)

        Show
        Thunder Stumpges added a comment - The point of the default constructor is to initialize member default values. However in a case where you have none (or need to have none in order to provide your own), I agree that it should not generate one. I am working on a patch that will generate the default constructor (as it does today) only if there are one or more members that have default values specified. I believe this should work for you (it works for me!)
        Hide
        Thunder Stumpges added a comment -

        Here is a patch which will only generate a default constructor if there are some field values to set. This SHOULD work however I have not compiled it since I do not yet have a working build environment. Any chance someone could review it and build it for me? Really need this one fixed!

        Show
        Thunder Stumpges added a comment - Here is a patch which will only generate a default constructor if there are some field values to set. This SHOULD work however I have not compiled it since I do not yet have a working build environment. Any chance someone could review it and build it for me? Really need this one fixed!
        Hide
        Thunder Stumpges added a comment -

        OK, well I got my environment to build, and the attached patch works wonderfully for me! Hope to see this one get into a build soon!

        Show
        Thunder Stumpges added a comment - OK, well I got my environment to build, and the attached patch works wonderfully for me! Hope to see this one get into a build soon!
        Hide
        Vadim Chekan added a comment -

        Why not init fields with default values instead of doing it in default constructor?
        If init fields with default values, it allows dropping default constructor unconditionally. Which allows more flexibility in partial classes, for example override default value conditionaly:
        "partial class C { C()

        {if(Something) _x = NOT_DEFAULT;}

        }

        Show
        Vadim Chekan added a comment - Why not init fields with default values instead of doing it in default constructor? If init fields with default values, it allows dropping default constructor unconditionally. Which allows more flexibility in partial classes, for example override default value conditionaly: "partial class C { C() {if(Something) _x = NOT_DEFAULT;} }
        Hide
        Thunder Stumpges added a comment -

        Been a long time now, any chance someone could look at this? I have to manually re-apply this patch each time I upgrade versions, and it seems like a simple fix.

        Show
        Thunder Stumpges added a comment - Been a long time now, any chance someone could look at this? I have to manually re-apply this patch each time I upgrade versions, and it seems like a simple fix.
        Hide
        Jens Geyer added a comment -

        Ok, I will, but tomorrow.

        Show
        Jens Geyer added a comment - Ok, I will, but tomorrow.
        Hide
        Jens Geyer added a comment -

        [...]and it seems like a simple fix.

        I tend to agree with what Vadim said. What exactly prevents you from using defaults?

        Next, your proposed change would introduce inconsistent behaviour (IMHO), since the default no-args CTOR would be created when there are any defaults, but would be omitted if there aren't any defaults. Although this behaviour might suit your particular needs, it may break existing code that relies on the existing default no-args CTOR.

        From my point of view, even making this an csharp option would add unecessary complexity. Unnecessary, since to me it looks very much like if you are trying to solve a non-problem.

        But maybe I overlook something important here, so I would really like to hear your replies on this and on Vadim's comment, before we continue here.

        Show
        Jens Geyer added a comment - [...] and it seems like a simple fix. I tend to agree with what Vadim said. What exactly prevents you from using defaults? Next, your proposed change would introduce inconsistent behaviour (IMHO), since the default no-args CTOR would be created when there are any defaults, but would be omitted if there aren't any defaults. Although this behaviour might suit your particular needs, it may break existing code that relies on the existing default no-args CTOR. From my point of view, even making this an csharp option would add unecessary complexity. Unnecessary, since to me it looks very much like if you are trying to solve a non-problem. But maybe I overlook something important here, so I would really like to hear your replies on this and on Vadim's comment, before we continue here.
        Hide
        Thunder Stumpges added a comment -

        Jens, thanks for taking a look. While I do agree with you about the inconsistent behavior, I do have (I think) an issue that I cannot resolve without having the constructor for initialization.

        I am fairly certain that what Vadim is suggesting is for Thrift to use defaults to initialize fields and NEVER write a default constructor (to solve your issue of inconsistency). My issue is that I need to perform initialization (conditionally or not, but usually not) on Thrift generated members which I cannot do from the partial class. The only place I have to even simply initialize a struct member to a new instance is in the partial class in a constructor.

        Please let me know if you have another suggestion for this, or if you're open to solving it with Vadim's suggestion of performing Thrift initialization in the field defaults, and leaving the constructor to the partial class.

        Thanks!
        Thunder

        Show
        Thunder Stumpges added a comment - Jens, thanks for taking a look. While I do agree with you about the inconsistent behavior, I do have (I think) an issue that I cannot resolve without having the constructor for initialization. I am fairly certain that what Vadim is suggesting is for Thrift to use defaults to initialize fields and NEVER write a default constructor (to solve your issue of inconsistency). My issue is that I need to perform initialization (conditionally or not, but usually not) on Thrift generated members which I cannot do from the partial class. The only place I have to even simply initialize a struct member to a new instance is in the partial class in a constructor. Please let me know if you have another suggestion for this, or if you're open to solving it with Vadim's suggestion of performing Thrift initialization in the field defaults, and leaving the constructor to the partial class. Thanks! Thunder
        Hide
        Jens Geyer added a comment -

        Thunder,

        The field initialization approach looks promising. Could you upgrade your patch accordingly?

        While testing this out, I found another problem with the currently generated initialization code:

        const list<string> TESTLIST =  ["holy","moley"]
        
        struct mine {
          1 : optional string Type = "Default",
          2 : required string Name = "Unnamed entity",
          3 : list<string> Test = TESTLIST
        }
        

        The current code does init the Type and Name members properly, also a list for the "Test" member is created. But that list is never filled with the data specified in TESTLIST. I did not create a new ticket yet, since it probably can be fixed along with the other changes to be made. If this is not an option, file a new ticket.

        Show
        Jens Geyer added a comment - Thunder, The field initialization approach looks promising. Could you upgrade your patch accordingly? While testing this out, I found another problem with the currently generated initialization code: const list<string> TESTLIST = [ "holy" , "moley" ] struct mine { 1 : optional string Type = "Default" , 2 : required string Name = "Unnamed entity" , 3 : list<string> Test = TESTLIST } The current code does init the Type and Name members properly, also a list for the "Test" member is created. But that list is never filled with the data specified in TESTLIST. I did not create a new ticket yet, since it probably can be fixed along with the other changes to be made. If this is not an option, file a new ticket.
        Hide
        Thunder Stumpges added a comment -

        Hi Jens, I put quite a number of hours into this tonight, and ran into several issues. See below:

        1. Use of auto-properties for required members with default values:

        I had to change the logic in deciding between private field backed property and auto-property for Required members. If we use auto-property, we can't do the field level initialization. Any time we have a default, we need a field backed property.

        This undoes a little bit of the work done in bug THRIFT-1783. What I did was add the field-backed property if a default value was present, but leave out the stuff for _isset when it is required. I am somewhat uncomfortable doing this, though I don't fully understand why the fixing of bug 1783 required removal of the field-backed properties, or the _isset either. Was it just an optimization? Does this sound OK?

        2. Required Member Constructor (from bug THRIFT-1783):

        Bug THRIFT-1783 creates a constructor taking all required members. Not sure if this is also just a convenience thing, but it also makes the "no-args constructor" required. I would like to get rid of this constructor. It's easy enough to just use object initialization syntax. Is this OK?

        3. Set initialization with const value:

        For some reason (not sure why, do you?) The type name rendered for Set is not just the .net HashSet<T> but it uses a thrift version called THashSet<T> which does not have a constructor taking IEnumerable<T> like the List<T>, Dictionary<K,V>, and HashSet<T> .net versions do. Therefore my code which initialized the collections defaults fails:

            private List<string> _OptListWDef = new List<string>(Constants.CONSTLIST);  // WORKS
            private Dictionary<string, string> _OptMapWDef = new Dictionary<string, string>(Constants.CONSTMAP); // WORKS
            private THashSet<string> _OptSetWDef = new THashSet<string>(Constants.CONSTSET);  // FAILS!
        

        If I change the type name from THashSet to HashSet, everything compiles fine. Not sure how you want to handle this.

        Let me know about these three things, and I think I can have a patch in to you tomorrow sometime.
        Thanks,
        Thunder

        Show
        Thunder Stumpges added a comment - Hi Jens, I put quite a number of hours into this tonight, and ran into several issues. See below: 1. Use of auto-properties for required members with default values: I had to change the logic in deciding between private field backed property and auto-property for Required members. If we use auto-property, we can't do the field level initialization. Any time we have a default, we need a field backed property. This undoes a little bit of the work done in bug THRIFT-1783 . What I did was add the field-backed property if a default value was present, but leave out the stuff for _isset when it is required. I am somewhat uncomfortable doing this, though I don't fully understand why the fixing of bug 1783 required removal of the field-backed properties, or the _isset either. Was it just an optimization? Does this sound OK? 2. Required Member Constructor (from bug THRIFT-1783 ): Bug THRIFT-1783 creates a constructor taking all required members. Not sure if this is also just a convenience thing, but it also makes the "no-args constructor" required. I would like to get rid of this constructor. It's easy enough to just use object initialization syntax. Is this OK? 3. Set initialization with const value: For some reason (not sure why, do you?) The type name rendered for Set is not just the .net HashSet<T> but it uses a thrift version called THashSet<T> which does not have a constructor taking IEnumerable<T> like the List<T>, Dictionary<K,V>, and HashSet<T> .net versions do. Therefore my code which initialized the collections defaults fails: private List<string> _OptListWDef = new List<string>(Constants.CONSTLIST); // WORKS private Dictionary<string, string> _OptMapWDef = new Dictionary<string, string>(Constants.CONSTMAP); // WORKS private THashSet<string> _OptSetWDef = new THashSet<string>(Constants.CONSTSET); // FAILS! If I change the type name from THashSet to HashSet, everything compiles fine. Not sure how you want to handle this. Let me know about these three things, and I think I can have a patch in to you tomorrow sometime. Thanks, Thunder
        Hide
        Thunder Stumpges added a comment -

        Re-reading number 1. in my post above, I think it may have been confusing. What I did in the case of a required member with a default value, is use the field-backed property instead of auto-property, but leave out the _isset stuff. required members with no defaults still behave as they did per THRIFT-1783 so:

          1 : optional string OptSNoDef,
          2 : required string ReqSWDef = "I am required",
          3 : required string ReqSNoDef
        

        becomes:

            private string _OptSNoDef;
            private string _ReqSWDef = "I am required";
        
            public string OptSNoDef
            {
              get
              {
                return _OptSNoDef;
              }
              set
              {
                __isset.OptSNoDef = true;
                this._OptSNoDef = value;
              }
            }
        
            public string ReqSWDef
            {
              get
              {
                return _ReqSWDef;
              }
              set
              {
                this._ReqSWDef = value;
              }
            }
        
            public string ReqSNoDef { get; set; }
        
        
        Show
        Thunder Stumpges added a comment - Re-reading number 1. in my post above, I think it may have been confusing. What I did in the case of a required member with a default value, is use the field-backed property instead of auto-property, but leave out the _isset stuff. required members with no defaults still behave as they did per THRIFT-1783 so: 1 : optional string OptSNoDef, 2 : required string ReqSWDef = "I am required" , 3 : required string ReqSNoDef becomes: private string _OptSNoDef; private string _ReqSWDef = "I am required" ; public string OptSNoDef { get { return _OptSNoDef; } set { __isset.OptSNoDef = true ; this ._OptSNoDef = value; } } public string ReqSWDef { get { return _ReqSWDef; } set { this ._ReqSWDef = value; } } public string ReqSNoDef { get; set; }
        Hide
        Jens Geyer added a comment -

        Hi Tunder,

        1. Use of auto-properties for required members with default values:
        I had to change the logic in deciding between private field backed property and auto-property for Required members. If we use auto-property, we can't do the field level initialization. Any time we have a default, we need a field backed property.

        I know, and it's fine with me.

        This undoes a little bit of the work done in bug THRIFT-1783. What I did was add the field-backed property if a default value was present, but leave out the stuff for _isset when it is required. I am somewhat uncomfortable doing this, though I don't fully understand why the fixing of bug 1783 required removal of the field-backed properties, or the _isset either. Was it just an optimization? Does this sound OK?

        Optimization, intended as an attempt to keep the code concise wherever possible. Note that in the cases where a backing field is needed (e.g. optionals), it is still generated. So this is fine with me, I will not lose much sleep over that.

        2. Required Member Constructor (from bug THRIFT-1783):
        Bug THRIFT-1783 creates a constructor taking all required members. [...] I would like to get rid of this constructor. It's easy enough to just use object initialization syntax. Is this OK?

        For me personally it would be ok, but I don't know if there might be any requirements that I'm not aware of. Maybe Carl can shed some light on this.

        3. Set initialization with const value:
        For some reason (not sure why, do you?) The type name rendered for Set is not just the .net HashSet<T> but it uses a thrift version called THashSet<T> which does not have a constructor taking IEnumerable<T> like the List<T>, Dictionary<K,V>, and HashSet<T> .net versions do.

        The repo says, that THashSet.cs has been introduced with THRIFT-309 for Mono compatibility. I don't know much about that issue. But how about just adding the missing CTOR in THashSet?

        Jens

        Show
        Jens Geyer added a comment - Hi Tunder, 1. Use of auto-properties for required members with default values: I had to change the logic in deciding between private field backed property and auto-property for Required members. If we use auto-property, we can't do the field level initialization. Any time we have a default, we need a field backed property. I know, and it's fine with me. This undoes a little bit of the work done in bug THRIFT-1783 . What I did was add the field-backed property if a default value was present, but leave out the stuff for _isset when it is required. I am somewhat uncomfortable doing this, though I don't fully understand why the fixing of bug 1783 required removal of the field-backed properties, or the _isset either. Was it just an optimization? Does this sound OK? Optimization, intended as an attempt to keep the code concise wherever possible. Note that in the cases where a backing field is needed (e.g. optionals), it is still generated. So this is fine with me, I will not lose much sleep over that. 2. Required Member Constructor (from bug THRIFT-1783 ): Bug THRIFT-1783 creates a constructor taking all required members. [...] I would like to get rid of this constructor. It's easy enough to just use object initialization syntax. Is this OK? For me personally it would be ok, but I don't know if there might be any requirements that I'm not aware of. Maybe Carl can shed some light on this. 3. Set initialization with const value: For some reason (not sure why, do you?) The type name rendered for Set is not just the .net HashSet<T> but it uses a thrift version called THashSet<T> which does not have a constructor taking IEnumerable<T> like the List<T>, Dictionary<K,V>, and HashSet<T> .net versions do. The repo says, that THashSet.cs has been introduced with THRIFT-309 for Mono compatibility. I don't know much about that issue. But how about just adding the missing CTOR in THashSet? Jens
        Hide
        Vadim Chekan added a comment -

        > The repo says, that THashSet.cs has been introduced with THRIFT-309 for Mono compatibility. I don't know much about that issue

        HashSet was introduced into .net framework-3.5 so in 2009 (thift-309), two years after framework-3.5 introduction there was still significant amount of projects on framework-2.0 which did not have HashSet implementation. Is compilation under Visual Studio 2005 still a requirement?

        IMHO Mono is not an issue because projects in linux land have tendency to migrate to new frameworks much faster.

        Show
        Vadim Chekan added a comment - > The repo says, that THashSet.cs has been introduced with THRIFT-309 for Mono compatibility. I don't know much about that issue HashSet was introduced into .net framework-3.5 so in 2009 (thift-309), two years after framework-3.5 introduction there was still significant amount of projects on framework-2.0 which did not have HashSet implementation. Is compilation under Visual Studio 2005 still a requirement? IMHO Mono is not an issue because projects in linux land have tendency to migrate to new frameworks much faster.

          People

          • Assignee:
            Unassigned
            Reporter:
            William Blinn
          • Votes:
            2 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:

              Development