Provide an option to mark a TdwsClass as "internal"

Issue #83 resolved
Anders Melander created an issue

Proposal

I propose that TdwsClass be extended with a property that marks the class as being "internal".

An internal class:

  • Cannot be overridden from script.
  • Cannot be instantiated from script.

Background

Let's say I have a class TMyStuff, declared in a TdwsUnit, that wraps a Delphi side object. I also have a function that returns an instance of this class: function GetStuff: TMyStuff. The class can only be instantiated through this function and thus has no constructor.

Now in order to prevent my users from instantiating the class via the TObject constructor I can declare the class abstract and sealed:

type
  TMyStuff = class abstract sealed
  end;

If the user tries to instantiate via script they get a compile time error:

Error: Trying to create an instance of an abstract class

This works, inasmuch as it prevents the user from instantiating the class via the TObject constructor, but is a bit misleading since the class is in fact not abstract.

The problem with the above solution is that it requires that I declare the base class sealed.

If I need to declare another class TMyOtherStuff derived from TMyStuff then TMyStuff can no longer be declared sealed:

type
  TMyStuff = class abstract
  end;

  TMyOtherStuff = class abstract sealed(TMyStuff)
  end;

...and it is now possible for the user to instantiate an class that was never meant to be instantiated from script. Even though I declare TMyOtherStuff abstract and sealed:

type
  TMyUserStuff = class(TMyStuff);

var Stuff := TMyUserStuff.Create;

The above compiles without any warning or errors but as soon as the user calls any method on the object it fails with an access violation since the object wasn't instantiated correctly (the internal ExternalObject pointer was never assigned a value).

The only solution I have found to the above problem is to add a constructor to the base class and raise an exception in this constructor if the user tries to use it (i.e. instantiate the object from script). This works but is a really poor solution because:

  • It requires us to write these do-nothing constructors for every class that the restrictions applies to.
  • It causes a run time error instead of a compile time error.
  • It generates confusion among the users which in turn leads to an increase in support.

Comments (11)

  1. Eric Grange repo owner

    Committed a couple of fixes that allow using "Object" as ancestor rather than "TObject", this had been added a while ago (https://www.delphitools.info/2012/10/03/re-rooting-object-pascal/) but was not fully supported yet for TdwsUnit classes.

    If your classes inherit from "Object", they will have none of the default methods (constructors, destructors, etc.), so you can choose to introduce whatever capability you want wherever you want.

  2. Anders Melander reporter
    • changed status to open
    • edited description

    Although using Object as the ancestor class would indeed remove the default TObject constructor, this is easily circumvented by the user:

    type
      TMyUserStuff = class(TMyStuff)
        constructor Create; begin end;
      end;
    
    var Crash := TMyUserStuff.Create;
    Crash.SomeMethod; // Causes run-time Access Violation
    

    It also introduces the burden that I now have to write explicit constructors for all internal classes - even though most of them will be completely empty.

  3. Eric Grange repo owner

    You can prevent that by marking your classes as class 'sealed'.

    Also why do you need an empty constructor for your internal classes?

  4. Anders Melander reporter

    Going back to my original example, specifying Object as the base class, the TdwsUnit declaration now looks like this:

    type
      TMyStuff = class abstract (Object) // Cannot be marked sealed since it's an abstract base class
      end;
    
      TMyOtherStuff = class abstract sealed(TMyStuff)
      end;
    

    This means that although the user cannot directly create an instance of TMyStuff or TMyOtherStuff, the user can still derive from TMyStuff and add their own constructor.


    Deriving from Object I need to write explicit constructors since the default TObject is no longer available Delphi side.

    Let's say I have a TdwsUnit function that returns a TMyStuff object. Deriving from TObject I can return an instance of the object like this:

    Info.ResultAsVariant := Info.Vars[Info.ResultVars.TypeSym.Name].GetConstructor('Create', MyExternalObject).Call.Value;
    

    and I don't need to implement a explicit constructor.

    Deriving from Object I no longer have the default Create constructor, so GetConstructor('Create',...) doesn't work unless I explicitly implement that constructor.

  5. Eric Grange repo owner

    Calling the script-side 'Create' constructor is only required if you want it to be executed, in your case, since you have none, it would just be a long way to create the script object instance and do nothing on it. I also guess your external object does not have any script-side fields?

       var obj : TScriptObjInstance;
       ...
       obj := TScriptObjInstance.Create(info.Table.FindTypeSymbol('TMyStuff', cvMagic) as TClassSymbol);
       obj.ExternalObject := MyExternalObject;
       Info.ResultAsVariant := IScriptObj(obj);
    

    This will directly create the script object and assign it as result.

    In a practical implementation you would probably store the class symbol to avoid looking it up by name every time like in the snippet above.

  6. Anders Melander reporter

    Excellent! I must have missed that little detail in the documentation :-)

    For my use I had to also pass Info.Execution to the TScriptObjInstance constructor because I need it set before returning from the method (because I need access to Execution.Environment).

    At this time I don't think I have a single case where I need to call the script side constructor from the Delphi side, so this will make a big difference. It also relieves me of passing IInfo around when all I really need is IScriptObj.

    W.r.t. saving a reference to the symbol I guess the place to store that would be in the Environment, right?

  7. Eric Grange repo owner

    The reference to the symbol can be saved in the environment, but the environment is related to a script execution, while the symbol is related to a script compilation (IdwsProgram).

    There is not really any place to reference that kind of things from a script program at the moment.

    So far in my use cases I usually have some kind of structure above the script programs (typically what needs/uses the script), and I place that kind of data there. Since it's also the structure that will trigger a compilation, it can just lookup the symbols it needs after compiling. You can then "forward" those symbols through the environment (f.i. through OnExecutionStarted event, or just attaching it directly after a CreateNewExecution), another possibility would be using the (relatively) new CustomStates mechanism (where the environment is meant for execution-oriented data, custom states are meant for library-oriented data).

  8. Anders Melander reporter
    • changed status to open

    I have finally had an opportunity to try out the solution you outlined (using Object as the base class) and I'm afraid it doesn't work.

    I have the following class hierarchy declared in TdwsUnit:

    TPersistent = class abstract(Object)
    protected
      procedure AssignTo(Dest: TPersistent); virtual;
    public
      procedure Assign(Source. TPersistent); virtual;
      // No constructor, nor destructor declared here
    end;
    
    TStrings = class abstract(TPersistent)
      [... 43 methods - not relevant to problem ...]
      // No constructor, nor destructor declared here
    end;
    

    Now if I try to instantiate a TPersistent or TStrings in script:

    var Strings := TStrings.Create;
    

    then I correctly get an error at compile time: Syntax Error: There is no accessible member with name "Create"

    And if I derive a class from TStrings I get the same error:

    type
      TMyStrings = class(TStrings);
    var Strings := TStrings.Create;
    

    However if I add a constructor then I'm allowed to compile...

    type
      TMyStrings = class(TStrings)
      public
        constructor Create; begin end;
      end;
    
    var Strings := TMyStrings.Create;
    Strings.Add('test'); // Access Violation at run time
    

    ... but the script naturally bombs at runtime with an AV in the call to TStrings.Add since no Delphi side object has been instantiated.

    Here's my implementation of TStrings.Add:

    procedure TDataModuleScriptClasses.dwsUnitClassesTStringsMethodsAddEval(Info: TProgramInfo; ExtObject: TObject);
    begin
      Info.ResultAsInteger := TStrings(ExtObject).Add(Info.Params[0].ValueAsString);
    end;
    

    In the above ExtObject is nil.

    As a bonus I also get an error upon destruction of the TMyStrings instance: Runtime Error: TPersistent.AssignTo(nil) not allowed. Not that it's relevant to the problem but here's my implementation of TPersistent.AssignTo():

    procedure TDataModuleScriptClasses.dwsUnitClassesClassesTPersistentMethodsAssignToEval(Info: TProgramInfo; ExtObject: TObject);
    begin
      if (Info.Params[0].ScriptObj = nil) then
        raise Exception.Create('TPersistent.AssignTo(nil) not allowed');
      TPersistentCracker(ExtObject).AssignTo(TPersistent(Info.ParamAsObject[0]));
    end;
    

    My guess is that there's either an incorrect cast to TObject somewhere in the codegen or that the codegen assumes the first class method to be the destructor.


    Unless I've made a mistake in the above I'd like to reissue my initial proposal for an Internal flag.

  9. Log in to comment