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

           Summary: "private" attribute breaks "override"
           Product: D
           Version: 1.051
          Platform: Other
        OS/Version: All
            Status: NEW
          Keywords: accepts-invalid, wrong-code
          Severity: normal
          Priority: P2
         Component: DMD
        AssignedTo: nobody@puremagic.com
        ReportedBy: nfxjfg@gmail.com


--- Comment #0 from nfxjfg@gmail.com 2009-12-05 23:01:54 PST ---
The compiler should error on the following code. Even though "override" is specified, nothing is overridden.

This should either run fine (without triggering the assertion), or not compile. The first if "override" disregards the current visibility attribute of the scope, and the second if "foo" is considered a private and thus non-virtual function. (I do not know which one, so I tagged the bug with both keywords; please remove this when it's known what should happen.)

Code:

abstract class A {
    void foo() {
        assert(false); //B.foo should be called instead!
    }
}

class B : A {
private:  //if you comment this out, it works

    void something() {
    }

    override void foo() {
        //never called, which is a bug
    }
}

void main() {
    A x = new B();
    x.foo();
}

-- 
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=3581


Vladimir <thecybershadow@gmail.com> changed:

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


--- Comment #1 from Vladimir <thecybershadow@gmail.com> 2010-03-09 14:33:06 PST ---
Simpler test case:

class C
{
    private override void f() {}
}

This code should not compile.
Removing "private" produces the expected "function test.C.f does not override
any function" error.

-- 
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=3581


nfxjfg@gmail.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |WONTFIX


-- 
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=3581


Brad Roberts <braddr@puremagic.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
                 CC|                            |braddr@puremagic.com
         Resolution|WONTFIX                     |


-- 
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=3581


Don <clugdbug@yahoo.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|wrong-code                  |
                 CC|                            |clugdbug@yahoo.com.au


--- Comment #2 from Don <clugdbug@yahoo.com.au> 2011-02-28 14:22:18 PST ---
Applies to 'static override' as well.

One possible patch is func.c, FuncDeclaration::semantic(), line 400:

        // if static function, do not put in vtbl[]
        if (!isVirtual())
        {
            //printf("\tnot virtual\n");
+            if (isOverride())
+                error("cannot use override with non-virtual functions");
            goto Ldone;
        }

But this should really be caught in the parser, I think. Anyway, it's accepts-invalid rather than wrong-code.

-- 
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=3581


Jacob Carlborg <doob@me.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |doob@me.com


--- Comment #3 from Jacob Carlborg <doob@me.com> 2011-02-28 23:24:44 PST ---
In TDPL, page 214-215, we can see examples of methods declared as private and override. In TDPL private methods are virtual, in DMD they're non-virtual.

-- 
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=3581


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

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


--- Comment #4 from Jonathan M Davis <jmdavisProg@gmx.com> 2011-03-01 00:08:33 PST ---
See bug# 4542. This issue is a highly debated one. TDPL says that you should be able to override private (primarily with the idea of using NVI). dmd is not currently implemented that way - private functions are _never_ virtual. NVI is highly useful, but it can be essentially done with protected instead of private, and if you make it so that you can override private methods, then you can't inline them anymore unless you declare them final. So, you can currently do NVI - you just have to use protected instead of private - and you get the higher efficiency of private functions being inlineable, but dmd is not in line with TDPL. On the other hand, if you make dmd in line with TDPL, then you can use private for NVI (which is of some benefit conceptually at least - though not really pratically-speaking), but then private functions are no longer inlineable, which could be a definite performance hit. I'm not sure that it has been definitely decided that dmd will be made to match TDPL in this case or if TDPL will have to be changed. Walter has never said anything on the matter as far as I recall, and I don't remember what Andrei said (if anything) the last time it was discussed.

At this point, I'm inclined to say that TDPL should be changed, since I don't like the idea of losing out on inlineable private functions without having to mark them all as final, but I don't know what the plan is at this point (if there even is one). I think that Andrei assumed that private was overridable, because it is because it is in C++ (though C++ doesn't force everything to be virtual like D does, so the cost there isn't the same) as oppose to having discussed it with Walter, but I don't know.

So, it's clear that per TDPL, private should be overridable, but I don't think that it's clear that TDPL is going to win out on this one. Walter (and possibly Andrei) need(s) to make a decision here, and I don't know if they've even discussed it.

-- 
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=3581


Rainer Schuetze <r.sagitario@gmx.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |r.sagitario@gmx.de


--- Comment #5 from Rainer Schuetze <r.sagitario@gmx.de> 2011-03-01 12:36:28 PST ---
I think it is often forgotten that protection flags in D are different from C. For example, "private" does not restrict access to the class, but to the module, so you can inherit from the class and still have full access to the private member functions of the base class. That means it makes sense to allow a private function to be virtual.

I'd also say that protection attributes should not interfere with attributes that deal with virtuality (abstract, override, final) - they are othogonal.

So I would vote for TDPL.

Note that the compiler has access to all overrides of the private functions (they must be in the same module), so it might still inline them if never overridden.

-- 
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=3581



--- Comment #6 from Rainer Schuetze <r.sagitario@gmx.de> 2011-03-01 12:49:09 PST ---
Thinking about it again, I might have missed the point completely. I don't have
TDPL handy right now, but IIRC it allowed overriding the private functions in
another mode, doesn't it?
That would make the inlining of private members impossible unless final is
specified. I still think the two concepts protection/virtuality should be kept
apart.

-- 
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=3581



--- Comment #7 from Jonathan M Davis <jmdavisProg@gmx.com> 2011-03-01 14:48:17 PST ---
They can't be kept apart as long as you can't specify whether a function is virtual or not. In C++, a member function is not virtual unless you mark it as virtual (or a base class has a function with the same name which is marked virtual). So, you can get into problems where you override functions but don't make them virtual, so which version of a function you call depends on what type of pointer you're using (yuck). D just makes all member functions virtual except those which it knows don't have to be. It currently treats private as non-virtual (and therefore non-overridable since all overridable functions must be virtual in D as just mentioned), presumably with the idea that there's no point in overriding a private function (forgetting NVI).

C++ does allow the overiding of private functions. The typical case where you do that would be NVI, where you make a public function which is non-virtual and a private one which is virtual and overridden in derived classes. That way, the private, virtual function can only be called in the base class, and you can enforce that certain things happen before or after the call to the private function, because the only place that it ever gets called is within the public, non-virtual function in the base class.

It's a nice idiom, but it can be pretty much done the same way using protected instead of private (particularly since there are ways to get around the uncallability, IIRC - probably by just casting the this pointer to the base class type). It also doesn't cause a penalty in C++, because your private functions are usually non-virtual and completely inlinable. In D, however, because you _can't_ specify a function as non-virtual, if you made private functions overridable, they must be virtual, and all of a sudden, the compiler can't inline _any_ private functions unless they're final. It doesn't know what all of the derived classes look like and whether they override a particular function, so it must assume that they'll be overriden and would have to make the virtual.

So, if we make it possible to override private functions in D, it would pretty much have to become common practice to mark all private functions as final unless you were specifically using NVI, otherwise you're going to take a definite performance hit in a lot of cases. On the other hand, we could leave private as unoverridabel and just say that NVI has to use protected instead of private. It works just as well and doesn't require that you mark almost every private function that you ever write as final.

You _can't_ separate access level from virtuality in D, because non-virtuality is done as an optimization rather than a function being virtual on non-virtual being at the programmer's request.

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