www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.bugs - [Issue 3581] New: "private" attribute breaks "override"

reply d-bugmail puremagic.com writes:
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



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: -------
Dec 05 2009
next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3581


Vladimir <thecybershadow gmail.com> changed:

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



---
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: -------
Mar 09 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
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: -------
Jan 06 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
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: -------
Jan 06 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
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



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: -------
Feb 28 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3581


Jacob Carlborg <doob me.com> changed:

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



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: -------
Feb 28 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3581


Jonathan M Davis <jmdavisProg gmx.com> changed:

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



PST ---

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: -------
Mar 01 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
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



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: -------
Mar 01 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3581




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: -------
Mar 01 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3581




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: -------
Mar 01 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3581




---


 They can't be kept apart as long as you can't specify whether a function is
 virtual or not.


What do you mean? Isn't that exactly what "final" does? Up until the moment you
mention "final", your message reads as if it was written by someone who didn't
know "final" existed.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Mar 01 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3581




PST ---
Final says that a function can't be overridden. That doesn't necessarily make
it non-virtual. For instance, if that function is already overriding another
function, then final isn't going to make it non-virtual. It does give you some
indirect control over whether a function is virtual or not, but it's not quite
the same thing. Really, final is intended for preventing overiding, not for
making a function non-virtual. It just does that as an optimization, because it
knows that it can. You don't explicitly have control over whether a function is
virtual or not.

Regardless, the fact that the only way to make a function non-virtual (assuming
that private is overridable) is if you specifically mark it is a final means
that most private functions _will_ be virtual and will _not_ be inlinable,
because the average programmer won't realize that they have to do that to make
the function inlinable. So, it's going to be a definite performance hit if
private becomes overridable. It makes the default inefficient for essentially
no gain. You can still do NVI with protected, so the only real reason IMO to
make private overridable is simply because TDPL said that you could in its
discussion of NVI. I think that it would be better in this case to fix TDPL
than change dmd. Otherwise, the cost in efficiency is too high for essentially
no gain.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Mar 01 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3581




PST ---
Can "protected" replace "private" in all intended usage of virtual private
functions? If so, has it been considered to simply forbid virtual private
functions (with a warning or error)? Then programmers should clarify whether
they need NVI, or a non-virtual, inlinable private function (by adding
"final").

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Mar 01 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3581




Regardless of this, the patch is still valid (if a function isn't virtual, you
shouldn't be allowed to mark it as override). The question of whether private
means virtual is an independent issue. Could someone open a new bug for that
please?

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Mar 01 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3581




PST ---
True. The problem is that dmd doesn't generally complain about invalid
attributes. And so this case is included. The patch does fix this one instance

bug on whether private functions should be overridable or not.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Mar 01 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3581


Walter Bright <bugzilla digitalmars.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bugzilla digitalmars.com



14:59:55 PDT ---
See proposed patch and problems with patch:

https://github.com/donc/dmd/commit/9f7b2f8cfe5d7482f2de7f9678c176d54abe237f#commitcomment-321724

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Mar 29 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3581




The failure is probably related to bug 2525 and 3525: functions in interfaces
aren't dealt with correctly.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Mar 30 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3581




PDT ---


 See proposed patch and problems with patch:
 
 https://github.com/donc/dmd/commit/9f7b2f8cfe5d7482f2de7f9678c176d54abe237f#commitcomment-321724


Copying my comment on github for better visibility:

As I happen to have the patch installed, I stumbled over this problem today
when running the unittests. The problem is that the "override" sets the
attribute for the complete mixin, including auto-implemented constructors.

Here's a patch that moves the override attribute to each generated function if
it is not a constructor:

diff --git a/std/typecons.d b/std/typecons.d
index e0868b0..979b1d1 100644
--- a/std/typecons.d
+++ b/std/typecons.d
   -1593,7 +1593,7    class AutoImplement(Base, alias how, alias what =
isAbstractFunction) : Base
     private alias AutoImplement_Helper!(
             "autoImplement_helper_", "Base", Base, how, what )
              autoImplement_helper_;
-    override mixin(autoImplement_helper_.code);
+    mixin(autoImplement_helper_.code);
 }

 /*
   -2081,6 +2081,8    private static:
             enum storageClass = make_storageClass();

             //
+        if(isAbstractFunction!func)
+        code ~= "override ";
             code ~= Format!("extern(%s) %s %s(%s) %s %s\n",
                     functionLinkage!(func),
                     returnType,

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
May 06 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3581


yebblies <yebblies gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |yebblies gmail.com



I've submitted the phobos patch as:
https://github.com/D-Programming-Language/phobos/pull/137

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jul 07 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3581


yebblies <yebblies gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |blazej.podsiadlo gmail.com



*** Issue 6266 has been marked as a duplicate of this issue. ***

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jul 08 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3581




*** Issue 6266 has been marked as a duplicate of this issue. ***

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jul 10 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3581


yebblies <yebblies gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Version|1.051                       |D1
         AssignedTo|nobody puremagic.com        |bugzilla digitalmars.com



Fixed for D2 only:

https://github.com/D-Programming-Language/dmd/commit/07fccae099cf93bffd5f5459e43a1e908213d7f4

Assigning to Walter for D1 merge or closing.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jan 29 2012
prev sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3581


yebblies <yebblies gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rayerd.wiz gmail.com



*** Issue 4855 has been marked as a duplicate of this issue. ***

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