sempare / sempare-delphi-template-engine

Sempare Template (scripting) Engine for Delphi allows for flexible dynamic text generation. It can be used for generating email, html, reports, source code, xml, configuration, etc.
Apache License 2.0
144 stars 18 forks source link

Invalid class typecast error calling virtual methods #145

Closed sbogie-gdi closed 1 year ago

sbogie-gdi commented 1 year ago

When iterating over a collection of objects derived from a common base type, attempting to call a virtual method which has different overridden implementations results in an 'Invalid class typecast' error. The issue is occurs because the rtti method is cached on the IMethodCallExpr when invoking. This is a useful optimization, but assumes it will only ever be invoked with a single type.

A simple fix is to just check the type against the cached method and redo the lookup on mismatch.

diff --git a/src/Sempare.Template.Evaluate.pas b/src/Sempare.Template.Evaluate.pas
index dca9891..3d024bf 100644
--- a/src/Sempare.Template.Evaluate.pas
+++ b/src/Sempare.Template.Evaluate.pas
@@ -1012,15 +1012,18 @@ end;
 function TEvaluationTemplateVisitor.Invoke(const AExpr: IMethodCallExpr; const AObject: TValue; const AArgs: TArray<TValue>; out AHasResult: boolean): TValue;
 var
   LObjType: TRttiType;
+  LMethod: TRttiMethod;
 begin
-  if AExpr.RttiMethod = nil then
+  LMethod := AExpr.RttiMethod;
+  if (LMethod = nil) or (LMethod.Parent.Handle <> AObject.TypeInfo) then
   begin
     LObjType := GRttiContext.GetType(AObject.TypeInfo);
-    AExpr.RttiMethod := LObjType.GetMethod(AExpr.Method);
+    LMethod := LObjType.GetMethod(AExpr.Method);
+    AExpr.RttiMethod := LMethod;
   end;

-  AHasResult := AExpr.RttiMethod.ReturnType <> nil;
-  exit(AExpr.RttiMethod.Invoke(AObject, AArgs));
+  AHasResult := LMethod.ReturnType <> nil;
+  exit(LMethod.Invoke(AObject, AArgs));
 end;

 function TEvaluationTemplateVisitor.ResolveTemplate(const APosition: IPosition; const AName: string): ITemplate;

Sample test case:

type
  TBase = class
  public
    function Name: string; virtual; abstract;
  end;

  TFoo = class(TBase)
  public
    function Name: string; override;
  end;

  TBar = class(TBase)
  public
    function Name: string; override;
  end;

function TFoo.Name: string;
begin
  Result := 'Foo';
end;

function TBar.Name: string;
begin
  Result := 'Bar';
end;

procedure TTestTemplateCall.TestVirtualCall;
var
  l: array [0..1] of TBase;
begin
  l[0] := TFoo.Create;
  l[1] := TBar.Create;
  Assert.AreEqual('FooBar', Template.Eval('<% for v of _ %><% v.Name() %><% end %>', l));
  l[0].Free;
  l[1].Free;
end;
darnocian commented 1 year ago

There is definitely something funky going on. I suspect related to #146 . I recall removing some if LValue.IsType then LValue := LValue.AsType. these things seemed unnecessary actually, but I've been on another project where I've seen that LValue does at some point need this to get hold of the original type. I may have forgotten and unfortunately didn't have enough coverage to catch this... I appologise.

The reason why I think it is related and not necessarily an issue with caching is that the following works:

 Assert.AreEqual('FooBar', Template.Eval('<% print(_[0].Name() + _[1].Name()) %>', l));

I will probe more.

Another test I've created that failed similar to what you described:

    Assert.AreEqual('FooBar', Template.Eval('<% for i := 0 to 1; print( _[i].Name()); end %>', l));
darnocian commented 1 year ago

The caching is definitely aggravating the situation, but the passing test case is a bit strange.

I've done a test now with the caching removed, and the array dereferencing and for of loops work.

Is this blocking you at present - just want to get a sense of urgency and if I can review a bit more till next week?

sbogie-gdi commented 1 year ago

Thanks for the quick response.

I believe your first test is passing because it's generating two separate IMethodCallExprs which each have their own RttiMethod cache.

No, this isn't a hard blocker for us. We've already adjusted the affected template to just pass an array with the precomputed values instead.

darnocian commented 1 year ago

Thanks for also 'deep diving' ;) I'll try have these addressed for next week.

darnocian commented 1 year ago

Regarding: if (LMethod = nil) or (LMethod.Parent.Handle <> AObject.TypeInfo) then

This might not behave well when multi threaded. I think probably best to just remove the caching for now.

darnocian commented 1 year ago

Will be in next release. Currently in the dev branch.