Custom Managed Records and Default(T)

[rgdawson 7]

I have been experimenting with Custom Managed Records (CMR) where I have a record type with defined class operators for Initialize, Finalize, and Assign.  My record type contains a TStringList object, which gets created in Initialize and freed in Finalize.  It all works beautifully, but I notice that it does not work as I would have expected with Default(T).  For example, I have (abbreviated obviously)

 

  Stringlist = record
  private
    {Internal TStringlist...}
    FData: TStringList;
    {Getters/Setters...}
    {...}
  public
    class operator Initialize(out Dest: Stringlist);
    class operator Finalize(var Dest: Stringlist);
    class operator Assign(var Dest: Stringlist; const[ref] Source: Stringlist);
    class operator Implicit(const AValue: Stringlist): TPersistent;
    class operator Implicit(const AValue: Stringlist): TStrings;
  {"Lifted" TStringlist Methods and Properties...}
    {...}
  end;

class operator Stringlist.Initialize(out Dest: Stringlist);
begin
  Dest.FData := TStringList.Create;
  //Log(IntToHex(IntPtr(@Dest)) + ' Initialized');
end;

class operator Stringlist.Finalize(var Dest: Stringlist);
begin
  Dest.FData.Free;
  //Log(IntToHex(IntPtr(@Dest)) + ' Finalized');
end;

class operator Stringlist.Assign(var Dest: Stringlist; const[ref] Source: Stringlist);
begin
  Dest.FData.Assign(Source.FData);
  //Log(IntToHex(IntPtr(@Source)) + ' Assigned to ' + IntToHex(IntPtr(@Dest)));
end;

{...}

function AllCaps(const Strings: Stringlist): StringList;
begin
  Result := Default(TMyRecord);  {Causes memory leak due to my own Finalize not getting called and A.FData not being freed before getting re-created}
  Result.Text := UpperCase(Strings.Text)
  {...}
end;

{...}
  MyStrings := AllCaps(TheirStrings);

Default(Stringlist) does not Finalize the variable Result using my own Finalize class operator.  Instead Initialize gets called on Result, creating a new TStringlist without freeing the previous one, and thus a memory leak.  There are simple work-arounds for this, such as

 

class function Stringlist.Default: Stringlist;
var
  Default: Stringlist;
begin
  Result := Default;
end;

function AllCaps(const Strings: Stringlist): StringList;
begin
  Result := Stringlist.Default;
  Result.Text := UpperCase(Strings.Text);
  {...}
end;

or,

function AllCaps(const Strings: Stringlist): StringList;
begin
  Finalize(Result);
  Result := Default(Stringlist);  
  Result.Text := UpperCase(Strings.Text);
  {...}
end;

Default(T) is a bit of a mysterious function, but it looks to me like its Finalize code ignores the CMR type's class operator Finalize.

 

I'm just wondering about all this and if this is intended behavior for a good reason that I might be missing or a bug.

 

Thoughts, anyone?

 

 

Edited August 28 by rgdawson
typo

[Remy Lebeau 1333]

When I run your code, I see your Default() statement is calling Initialize() two times on the same temporary Stringlist instance, then that temporary is Assign()'ed to the Result record, and then the temporary is Finalized() only one time.  Clearly one of the Initialize() calls is erroneous.  I have reported this to Embarcadero:

 

RSS-1647: System.Default() on a Custom Managed Record calls its Initalize operator too many times

 

[Stefan Glienke 1976]

Already reported in 2021

[corneliusdavid 187]

16 hours ago, rgdawson said:

My record type contains a TStringList object, which gets created in Initialize and freed in Finalize.

I suppose this is legal but if you need to create/free a TStringList, why not use a class instead of a record? I always thought of records as a collection of simple types--it never even dawned on me to add a field that required it to be created at runtime.

[rgdawson 7]

14 hours ago, Remy Lebeau said:

When I run your code, I see your Default() statement is calling Initialize() two times on the same temporary Stringlist instance, then that temporary is Assign()'ed to the Result record, and then the temporary is Finalized() only one time.  Clearly one of the Initialize() calls is erroneous.  I have reported this to Embarcadero:

 

RSS-1647: System.Default() on a Custom Managed Record calls its Initalize operator too many times

 

Yes, exactly.  

[rgdawson 7]

14 hours ago, Remy Lebeau said:

When I run your code, I see your Default() statement is calling Initialize() two times on the same temporary Stringlist instance, then that temporary is Assign()'ed to the Result record, and then the temporary is Finalized() only one time.  Clearly one of the Initialize() calls is erroneous.  I have reported this to Embarcadero:

 

RSS-1647: System.Default() on a Custom Managed Record calls its Initalize operator too many times

 

Actually, I think that the problem is that Default(T) is not finalizing the variable using the class operator, before the first Initialize.  I'm guessing that it is just doing its old default finalize operation as it always has and needs to be CMR aware.  That's my guess, at least.

[rgdawson 7]

53 minutes ago, corneliusdavid said:

I suppose this is legal but if you need to create/free a TStringList, why not use a class instead of a record? I always thought of records as a collection of simple types--it never even dawned on me to add a field that required it to be created at runtime.

No reason, I am experimenting.  Afterall, I have done fine without Custom Managed Records forever.  I just was trying to understand how Custom Managed Records work and how they might be useful, since Embarcadero saw fit to implement them.

 

I do use string lists alot (usually small ones) and I find it nice to not have to create/free them in try/finally blocks all the time.  I have tried Spring4D's smart pointers, which are also very nice (though you still have to create them).

 

This exercise was inspired by https://github.com/toppermitz/StringVar, an older similar thing where the author uses TInterfacedStringlist = class(TStringlist, IInterface) and embeds that into a record type.  So, I thought to implement the concept as a Custom Managed Record and see what happens.  The full implementation is here, https://github.com/rgdawson/Rgd.Stringlist, for anyone interested and includes a demo that logs what is going on.

 

Having done so, I really like it.  The Default(T) behavior was unexpected, but not a big deal, just something to be aware of until Embarcadero fixes it.

 

[Remy Lebeau 1333]

19 minutes ago, rgdawson said:

Actually, I think that the problem is that Default(T) is not finalizing the variable using the class operator, before the first Initialize.

Maybe I'm just having a brain fart, but why would a new record instance need to be FINALIZED before it is INITIALIZED?  The System.Default documentation even states:

Quote

Note: When T is a record type, and Default(T) is assigned to a variable of T type, the compiler generates the code to finalize the record.

Finalizing implies destruction, not creation.  And if a record contains compiler-managed types (interfaces, strings, dynarrays, etc), finalizing it before initializing it would invoke refcounting semantics on things that aren't valid yet.

 

What am I missing here?

[Kas Ob. 113]

21 minutes ago, rgdawson said:

I think that the problem is that Default(T) is not finalizing the variable using the class operator, before the first Initialize.

No, hold your horses here!

 

Default is right with its own definition, there is no default value in Pascal/Delphi to begin with except nil and zero, asking Default to finalize what by might not be initialized is wrong and outside of its usage, Default can't every know what it is clearing, because this what should be called Clearing or Zeroing.

Untill we have default value for declared fields and the compiler put these values when ever a field/variable used then Default should be used as zeroing only.

[Kas Ob. 113]

I posted about default language extension specially for the default value for fields and variables, but this completely will depend on compiler intervention, will of course to simplify and solidify the generated code and prevent cascade of malpractices and bugs. 

[Dalija Prasnikar 1386]

37 minutes ago, Remy Lebeau said:

Maybe I'm just having a brain fart, but why would a new record instance need to be FINALIZED before it is INITIALIZED?  The System.Default documentation even states:

Because whole point of custom managed records is automatic initialization/finalization.

 

So if you merely declare such record as local variable and you don't ever use it, its initialization and finalization routines will run.

 

Running following code makes the issue more obvious.

 

type
  TMyRec = record
    x: Integer;
    class operator Initialize(out rec: TMyRec);
    class operator Finalize(var Dest: TMyRec);
    class operator Assign(var left, right: TMyRec);
  end;

class operator TMyRec.Initialize(out rec: TMyRec);
begin
  Writeln('init');
end;

class operator TMyRec.Finalize(var Dest: TMyRec);
begin
  Writeln('finalize');
end;

class operator TMyRec.Assign(var left, right: TMyRec);
begin
  Writeln('assign');
end;

procedure Main;
var
  r: TMyRec;
begin
  r := Default(TMyRec);
end;

begin
  Main;
end.

If assignment is commented out then result will be

 

init

finalize

 

But running code as-is will result with:

 

init
init
init
assign
finalize
finalize

 

 

 

[rgdawson 7]

1 hour ago, Remy Lebeau said:

Maybe I'm just having a brain fart, but why would a new record instance need to be FINALIZED before it is INITIALIZED?  The System.Default documentation even states:

Finalizing implies destruction, not creation.  And if a record contains compiler-managed types (interfaces, strings, dynarrays, etc), finalizing it before initializing it would invoke refcounting semantics on things that aren't valid yet.

 

What am I missing here?

As Dalija shows above, the CMR guaranteed to already be INITIALIZED, so FINALIZE would not be called before INITIALIZE.  So, it still seems to me that Default(T) should call the CMR's FINALIZE before calling the CMR's INITIALIZE.

[Remy Lebeau 1333]

3 hours ago, Dalija Prasnikar said:

Because whole point of custom managed records is automatic initialization/finalization.

I'm aware of that.  You seem to have missed the point I was trying to make regarding rgdawson's earlier comment about the order of initialization/finalization.

3 hours ago, Dalija Prasnikar said:

Running following code makes the issue more obvious.

Default() creates a NEW INSTANCE, which is then assigned to the EXISTING instance, ie this code:

procedure Main;
var
  r: TMyRec;
begin
  r := Default(TMyRec);
end;

SHOULD BE roughly equivalent to this:

procedure Main;
var
  r: TMyRec; // allocation only
begin
  TMyRec.Initialize(r);
  try
    var temp: TMyRec;  // allocation only
    TMyRec.Initialize(temp);
    try
      TMyRec.Assign(r, temp);
    finally
      TMyRec.Finalize(temp);
     end;
  finally
    TMyRec.Finalize(r);
  end;
end;

But what it is actually doing this more like this:

procedure Main;
var
  r: TMyRec; // allocation only
begin
  TMyRec.Initialize(r);
  try
    var temp: TMyRec; // allocation only
    TMyRec.Initialize(temp);
    try
      TMyRec.Initialize(temp); // <-- SHOULD NOT BE HERE!
      TMyRec.Assign(r, temp);
    finally
      TMyRec.Finalize(temp);
     end;
  finally
    TMyRec.Finalize(r);
  end;
end;

3 hours ago, Dalija Prasnikar said:

If assignment is commented out then result will be

 

init

finalize

Yes, because Default() is not being called.

3 hours ago, Dalija Prasnikar said:

But running code as-is will result with:

 

init
init
init
assign
finalize
finalize

Exactly. r is initialized, then a temporary record is created and initialized TWICE (that's the bug), then the temp is assigned to r, then the temp and r are finalized.

 

Here is the complete code I tested with, which makes it clearer which record is being initialized and finalized.  You can clearly see the temporary record created by Default() is initialized twice:

program CustomManagedRecordDefaultTest;

{$APPTYPE CONSOLE}

{$R *.res}

uses
  System.SysUtils, System.Classes;

type
  Stringlist = record
  private
    FData: TStringList;
    function GetText: string;
    procedure SetText(const AValue: string);
  public
    class operator Initialize(out Dest: Stringlist);
    class operator Finalize(var Dest: Stringlist);
    class operator Assign(var Dest: Stringlist; const[ref] Source: Stringlist);
    property Text: string read GetText write SetText;
  end;

class operator Stringlist.Initialize(out Dest: Stringlist);
begin
  Dest.FData := TStringList.Create;
  WriteLn(IntToHex(IntPtr(@Dest)) + ' Initialized');
end;

class operator Stringlist.Finalize(var Dest: Stringlist);
begin
  Dest.FData.Free;
  WriteLn(IntToHex(IntPtr(@Dest)) + ' Finalized');
end;

class operator Stringlist.Assign(var Dest: Stringlist; const[ref] Source: Stringlist);
begin
  Dest.FData.Assign(Source.FData);
  WriteLn(IntToHex(IntPtr(@Source)) + ' Assigned to ' + IntToHex(IntPtr(@Dest)));
end;

function Stringlist.GetText: string;
begin
  Result := FData.Text;
end;

procedure Stringlist.SetText(const AValue: string);
begin
 FData.Text := AValue;
end;

function AllCaps(const Strings: Stringlist): Stringlist;
var
  sl: Stringlist;
begin
  WriteLn('    Strings=', IntToHex(IntPtr(@Strings)), ' sl=', IntToHex(IntPtr(@sl)), ' Result=', IntToHex(IntPtr(@Result)));
  WriteLn('    Calling: sl := Default(Stringlist)');
  sl := Default(Stringlist);
  WriteLn('    Default Returned, sl Assigned');
  sl.Text := UpperCase(sl.Text);
  WriteLn('    Returning sl');
  Result := sl;
end;

procedure DoTest;
var
  MyStrings, TheirStrings: Stringlist;
begin
  WriteLn('  MyStrings=', IntToHex(IntPtr(@MyStrings)), ' TheirStrings=', IntToHex(IntPtr(@TheirStrings)));
  WriteLn('  Calling: MyStrings := AllCaps(TheirStrings)');
  MyStrings := AllCaps(TheirStrings);
  WriteLn('  AllCaps Returned, MyStrings Assigned');
end;

begin
  WriteLn('Calling: DoTest');
  DoTest;
  WriteLn('DoTest Returned');
  ReadLn;
end.

Calling: DoTest
008FFD0C Initialized
008FFD08 Initialized
  MyStrings=008FFD0C TheirStrings=008FFD08
  Calling: MyStrings := AllCaps(TheirStrings)
008FFCC8 Initialized
    Strings=008FFD08 sl=008FFCC8 Result=008FFD0C
    Calling: sl := Default(Stringlist)
008FFCC4 Initialized // <-- TEMP CREATED AND INIT HERE
008FFCC4 Initialized // <-- TEMP INIT AGAIN!
008FFCC4 Assigned to 008FFCC8
008FFCC4 Finalized // <-- TEMP DESTROYED HERE
    Default Returned, sl Assigned
    Returning sl
008FFCC8 Assigned to 008FFD0C
008FFCC8 Finalized
  AllCaps Returned, MyStrings Assigned
008FFD08 Finalized
008FFD0C Finalized
DoTest Returned

 

Edited August 29 by Remy Lebeau

[Stefan Glienke 1976]

There should not be any temp, nor assign call tbh. Check out this code:

{$APPTYPE CONSOLE}

uses
  SysUtils;

type
  TMyRec = record
    x: Integer;
    class operator Initialize(out dest: TMyRec);
    class operator Finalize(var dest: TMyRec);
    class operator Assign(var left: TMyRec; const[ref] right: TMyRec);
  end;

function MyDefault: TMyRec;
begin
  Writeln('default - ', IntToHex(IntPtr(@Result)));
  Result.x := 0;
end;

class operator TMyRec.Initialize(out dest: TMyRec);
begin
  Writeln('init - ', IntToHex(IntPtr(@dest)));
end;

class operator TMyRec.Finalize(var dest: TMyRec);
begin
  Writeln('finalize - ', IntToHex(IntPtr(@dest)));
end;

class operator TMyRec.Assign(var left: TMyRec; const[ref] right: TMyRec);
begin
  Writeln('assign');
end;

procedure Main;
var
  r: TMyRec;
begin
  r := MyDefault;
end;

begin
  Main;
end.

Now we can argue why there is no assign call because we are assigning the result of the MyDefault function to r. But even though we have declared an assign operator it does what it always does with managed return type - passing it as hidden var parameter.

I said this before and I say it again - CMR are broken as designed and personally I stay the heck away from them.

 

I remember some years ago there was a question on SO about struct default ctors serving as default initializers and why C# does not have them and I don't remember if it was Jon or Eric explained that it would cause all kinds of trouble.

 

Edit: Ah, here it is: https://stackoverflow.com/a/333840/587106 - eventually they added them in C# 10 but there is quite some extensive language spec - when I asked for the CMR spec it was all crickets.

FWIW the Delphi implementation suffers from exactly the situation that Jon describes - allocate a dynamic array of some CMR and watch all the initializers run. Which is completely bonkers - C# 10 does not do any of that for array allocation - see https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-10.0/parameterless-struct-constructors#array-allocation

Edited August 29 by Stefan Glienke

[Dalija Prasnikar 1386]

8 hours ago, Remy Lebeau said:

You seem to have missed the point I was trying to make regarding rgdawson's earlier comment about the order of initialization/finalization.

Yes, sorry. I missed the context you are replying to. We agree on the rest, and that was the point of my example. 

 

To show that custom managed are initialized/finalized even if not used and that there is extra initialization for Default call on implicit temporary variable. And compiler should be smart enough not to require any temporary variable at all.

 

In any way thing is broken.

[rgdawson 7]

On 8/29/2024 at 5:00 PM, Stefan Glienke said:

...

I said this before and I say it again - CMR are broken as designed and personally I stay the heck away from them.

...

 

Thank you, Stefan.  (and Dalija, and Remy.  My top 3 Delphi heroes.)

 

Anyway, I think your advice above is good advice at the end of the day. Though, other than the unexpected/broken Default(T) behavior, I was at least beginning to think they might be useful in some ways.  I assumed that the purpose of CMR's Initialization and Finalization class operators was to create/free things.  It is hard for me to imagine any other important benefit.

 

I did another experiment where I replaced my CMR's regular TStringlist object with a Shared<TStringlist> using your Spring4D library, thinking that might avoid a memory leak with Default(T), and thinking the Shared<TStringlist> might free itself, but the Shared<TStringlist> does not get freed in that case either.

 

So, at the end of the day, I'm thinking CMR's are of dubious benefit and just not compatible with the Default(T) function.

[Rollo62 524]

On 8/30/2024 at 1:00 AM, Stefan Glienke said:

I said this before and I say it again - CMR are broken as designed and personally I stay the heck away from them.

Personally I see advantage in using them as simple "SmartPointers", already now in uncritical situations, avoiding any further complexity like Default(T).
That should be fair enough, although I still hesitate to use them, until perhaps more positive feedback occurs.
It is a pity that such a basic and useful feature cannot be used out-of-the-box, without a bad feeling.
Unfortunately, it may take 2-3 update versions until the hard corners and edges have been sanded down a little.

[Attila Kovacs 626]

2 minutes ago, Rollo62 said:

It is a pity that such a basic and useful feature cannot be used out-of-the-box, without a bad feeling.

Same here. I was so excited when I upgraded to 12, thinking there would finally be something amazing, but when I checked it out, bleh. I’d rather not.