Bug 5056 – Warning against virtual method call from constructor

I suggest to add a compiler suggestion/tip (a kind of warning) against (transitively) calling virtual methods inside its constructors, because doing so causes the most-derived override to be called, regardless of whether the constructor for the type that defines the most-derived override has been called.

It should be noted that unlike C++ this isn't necessarily a problem in D. In C++, each class in the class's hierarchy is constructed one by one until the most derived class constructor is called. So, if you call a virtual function in a constructor, the object is not in a safe state and won't even correctly know what type it is. In D, however, the object is in a safe state. All of the classes in its hierarchy are properly put together with the default values for all their variables already initialiazed, _then_ the constructors are called. So, virtual functions will work correctly. It is perfectly safe to call them. Now, it's quite possible that you write your class poorly and a virtual function that you call in your constructor assumes that the super classes' constructors have already been called, and it doesn't do what you expect, but it's nowhere near the same problem as it is in C++. And since, unlike C++, nearly all member functions in D are virtual, you have somewhat less control over whether a function is or isn't virtual, and it will be that much harder to guarantee that a function isn't virtual. You'd be restricted to private functions and public final functions which don't override base class functions. In many cases, that is unnecessarily restrictive. So, while this is definitely a concern in C++, I'm not sure that there's much pont in worrying about it in D. D has specifically been designed to avoid the worst problems associated with calling virtual functions in a constructor.

I'd say, this can be more serious issue with D than with C++ because D's execution is repeatable, so if you have a security issue, it becomes deterministic. Just a thought: if you have a password field, by default it's initialized to null, which can cause unauthorized access.

(In reply to comment #1) > It should be noted that unlike C++ this isn't necessarily a problem in D. In > C++, each class in the class's hierarchy is constructed one by one until the > most derived class constructor is called. So, if you call a virtual function in > a constructor, the object is not in a safe state and won't even correctly know > what type it is. In C++, calling a virtual function from a constructor calls that class' version of the virtual function directly. In other words, virtual functions are not virtual inside the constructor. In D, the virtual function is called, so it could potentially be a problem. However, it's also possible in D (unlike in C++) to initialize data before calling the parent constructor through the use of super. But of course, the base class has no say in whether the derived class's version is safe to call. So yes, I think it would be a good thing to suggest as a "best practice" (certainly not a compiler error or warning), but there are ways to make it safe.

The really bad thing is that D doesn't allow you to set up a sub class before calling the super constructor. Consider this: class Foo : Moo { int stuff; this(int stuff_) { this.stuff = stuff_; super(); } override int get_stuff() { return stuff; } } This won't compile, because the compiler forces you to do the super ctor call _before_ setting up any members. This was probably thought as a feature to ensure "correct" construction, but it just doesn't work because the super ctor could call virtual functions. And it's really hard to work this around. This behaviour of super ctor calls and virtual functions has been badly thought-through and doesn't increase productivity in the slightest. Sucks a lot.

(In reply to comment #4) > The really bad thing is that D doesn't allow you to set up a sub class before > calling the super constructor. Consider this: > > class Foo : Moo { > int stuff; > this(int stuff_) { > this.stuff = stuff_; > super(); > } > override int get_stuff() { > return stuff; > } > } > > This won't compile, because the compiler forces you to do the super ctor call > _before_ setting up any members. This was probably thought as a feature to > ensure "correct" construction, but it just doesn't work because the super ctor > could call virtual functions. And it's really hard to work this around. Hm... I just tried it, it does work. Were you thinking of something else? example: import std.stdio; class A { this() { foo(1); } void foo(int x) { writeln("A.foo"); } } class B : A { int x; this() { writeln("B.this"); x = 5; super(); } override void foo(int x) { writeln("B: ", this.x); } } void main() { auto b = new B; } prints: B.this B: 5

(In reply to comment #5) > Hm... I just tried it, it does work. Were you thinking of something else? This confuses me. It works both in D1 and D2. Maybe it's a regression? The spec says: "3. It is illegal to refer to this implicitly or explicitly prior to making a constructor call." http://www.digitalmars.com/d/1.0/class.html#constructors And I thought past versions of dmd conform to this, but apparently I'm wrong. Anyway, if the program in comment #5 is actually valid in D2 (and with using writefln in D1), I retract my statement and apologize for the noise.

(In reply to comment #6) > (In reply to comment #5) > > Hm... I just tried it, it does work. Were you thinking of something else? > > This confuses me. It works both in D1 and D2. Maybe it's a regression? > The spec says: > > "3. It is illegal to refer to this implicitly or explicitly prior to making a > constructor call." > http://www.digitalmars.com/d/1.0/class.html#constructors I interpret that the same way you do. I think maybe that this is a rule that was removed, but someone forgot to remove it from the docs? I think you should file a separate doc bug on it. It doesn't strike me as a good requirement anyways. What problems does having that restriction prevent? > And I thought past versions of dmd conform to this, but apparently I'm wrong. BTW, the file compiled with the oldest compiler I had installed (2.033). So it certainly has been this way for some time. I expect at some point it was as the docs state or else why would that rule even be there?

see bug 3393

Thank you for all the comments. (In reply to comment #3) > So yes, I think it would be a good thing to suggest as a "best practice" > (certainly not a compiler error or warning), but there are ways to make it > safe. It may be a third class of compiler messages, beside the errors and the warnings. The compiler "tips" may warn against possible troubles like this one (also see bug 4924 and others), give suggestions for improvements or, like the CommonLisp compilers, warn against not efficient code. An example of those comments from a CLisp compiler: http://shootout.alioth.debian.org/u32/program.php?test=mandelbrot&lang=sbcl&id=1

> > This confuses me. It works both in D1 and D2. Maybe it's a regression? > > The spec says: > > > > "3. It is illegal to refer to this implicitly or explicitly prior to making a > > constructor call." > > http://www.digitalmars.com/d/1.0/class.html#constructors > > I interpret that the same way you do. I think maybe that this is a rule that > was removed, but someone forgot to remove it from the docs? I think you should > file a separate doc bug on it. > > It doesn't strike me as a good requirement anyways. What problems does having > that restriction prevent? Hmm. I was using this "feature", setting fields before calling super(), for VERY long time. It worked, so i never actually payed attention, if i call super() at the beginning or the end of constructor - both worked well.

dmd 2.063 - instance method is called before constructor. module test; import std.stdio; class Base { this () { writeln("Base.this"); foo(); // here should come warning: calling virtual method in constructor } void foo () { writeln("Base.foo"); } } class Derived : Base { this () { writeln("Derived.this"); } override void foo () { writeln("Derifed.foo"); } } void main () { auto d = new Derived; } Program output: Base.this Derifed.foo // method Derived.foo is called before object constructor Derived.this

(In reply to comment #11) > dmd 2.063 - instance method is called before constructor. There's a simple fix for this, simply call super() directly whenever you want to: class Derived : Base { this() { writeln("Derived.this"); super(); } override void foo () { writeln("Derifed.foo"); } } Result: Derived.this Base.this Derifed.foo

(In reply to comment #12) > (In reply to comment #11) > > dmd 2.063 - instance method is called before constructor. > > There's a simple fix for this, simply call super() directly whenever you want Slightly better, but hardly a fix. at best partial workaround. you exchange one invalid initialization order for other invalid order - now Derived is initialized before Base. I guess is a less of a problem as developer of Derived can handle this invalid order locally. Think of Base and Derived to be developed by independent developers not knowing the details of implementation: In current scheme, to ensure that derived class is initialized before any overridden method can be called: developer has to explicitly call the super() constructor as the last step in derived constructor. And that must be done for every derived class on which any method is overridden. Also this would not only for the most-derived class, if there is more derived classes in hierarchy - every one hast to do it. Two sides of problem are: - Developer of Base might not be aware that method called in constructor is virtual, so he accidentally allows it to be hijacked. - Developer of Derived might be not aware that method he has overridden is called in Base constructor, which makes his overridden method to be allays called before his constructor his called.

THIS ISSUE HAS BEEN MOVED TO GITHUB https://github.com/dlang/dmd/issues/18312 DO NOT COMMENT HERE ANYMORE, NOBODY WILL SEE IT, THIS ISSUE HAS BEEN MOVED TO GITHUB