http://d.puremagic.com/issues/show_bug.cgi?id=5056

           Summary: Warning against virtual method call from constructor
           Product: D
           Version: D1 & D2
          Platform: Other
        OS/Version: Windows
            Status: NEW
          Keywords: diagnostic
          Severity: minor
          Priority: P2
         Component: DMD
        AssignedTo: nobody@puremagic.com
        ReportedBy: bearophile_hugs@eml.cc


--- Comment #0 from bearophile_hugs@eml.cc 2010-10-15 05:02:51 PDT ---
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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5056


Jonathan M Davis <jmdavisProg@gmx.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jmdavisProg@gmx.com


--- Comment #1 from Jonathan M Davis <jmdavisProg@gmx.com> 2010-10-15 09:47:59 PDT ---
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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5056



--- Comment #2 from Sobirari Muhomori <dfj1esp02@sneakemail.com> 2010-10-16 07:36:52 PDT ---
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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5056


Steven Schveighoffer <schveiguy@yahoo.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |schveiguy@yahoo.com


--- Comment #3 from Steven Schveighoffer <schveiguy@yahoo.com> 2010-10-16 09:50:22 PDT ---
(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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5056


nfxjfg@gmail.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |nfxjfg@gmail.com


--- Comment #4 from nfxjfg@gmail.com 2010-10-16 10:14:32 PDT ---
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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5056



--- Comment #5 from Steven Schveighoffer <schveiguy@yahoo.com> 2010-10-16 10:43:10 PDT ---
(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

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5056



--- Comment #6 from nfxjfg@gmail.com 2010-10-16 11:38:09 PDT ---
(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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5056



--- Comment #7 from Steven Schveighoffer <schveiguy@yahoo.com> 2010-10-16 12:09:12 PDT ---
(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?

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5056



--- Comment #8 from Sobirari Muhomori <dfj1esp02@sneakemail.com> 2010-10-16 12:26:11 PDT ---
see bug 3393

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5056



--- Comment #9 from bearophile_hugs@eml.cc 2010-10-17 16:58:48 PDT ---
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

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------