www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.bugs - [Issue 3258] New: Calling private or package override methods calls the base implementation

reply d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3258

           Summary: Calling private or package override methods calls the
                    base implementation
           Product: D
           Version: 1.045
          Platform: x86
        OS/Version: Linux
            Status: NEW
          Keywords: accepts-invalid, wrong-code
          Severity: major
          Priority: P2
         Component: DMD
        AssignedTo: nobody puremagic.com
        ReportedBy: diggory.hardy gmail.com


(Same as: http://www.dsource.org/projects/ldc/ticket/355
Exactly the same problem with dmd 1.045, dmd 2.031, ldc 0.9.1.)


base class, from a reference of type base class, the wrong implementations get
called.

OK, so overriding private functions is illegal according to the spec, but since
the compiler doesn't complain, I included those to show the similarity between
package and private functions (same error with both).

{{{

// Declare and define some methods...
class A {
    private void iPriv ()    { assert(false, "iPriv"); }
    protected void iProt ()    { assert(false, "iProt"); }
    package void iPack ()    { assert(false, "iPack"); }
    public void iPub ()        { assert(false, "iPub"); }
}

// ... but override them all here:
class B : A {
    private override void iPriv() {}    // (lesser issue: shouldn't the
compiler complain here?)
    protected override void iProt() {}
    package override void iPack() {}
    public override void iPub() {}
}

void main() {
    // None of these assert of course (overriden functions are called):
    B b = new B;
    b.iPriv;
    b.iProt;
    b.iPack;
    b.iPub;

    // But when we call from a base class with an implementation:
    A a = b;
    a.iPriv;    // This calls A.iPriv and asserts
    a.iProt;
    a.iPack;    // This calls A.iPack and asserts
    a.iPub;
}
}}}
Unless the {{{a.iPriv;}}} and {{{a.iPack;}}} calls are removed, the program
asserts when run.

 1. With private functions this is nearly OK, but the compiler should complain
about trying to override them instead of silently calling the base
implementation.
 2. With package protection, the wrong implementation of iPack() is called.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Aug 18 2009
next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3258


Stewart Gordon <smjg iname.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |smjg iname.com





private and package methods aren't virtual and so don't override.  There should
be something in the spec to this effect, but I can't seem to find it.  The bug
is that the compiler accepts the override attribute on these, simple as that.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Aug 19 2009
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3258


Matti Niemenmaa <matti.niemenmaa+dbugzilla iki.fi> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |matti.niemenmaa+dbugzilla i
                   |                            |ki.fi





2009-08-19 13:32:06 PDT ---
http://www.digitalmars.com/d/1.0/attribute.html#ProtectionAttribute says:
"Private members cannot be overridden."

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Aug 19 2009
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3258






---
With private functions, yes. But I've never heard that package functions can't
be virtual. Stewart Gordan, are you saying package functions aren't virtual by
design (I hope not), or that this is just the current implementation?

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Aug 19 2009
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3258


Jarrett Billingsley <jarrett.billingsley gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jarrett.billingsley gmail.c
                   |                            |om





2009-08-19 23:47:28 PDT ---


 With private functions, yes. But I've never heard that package functions can't
 be virtual. Stewart Gordan, are you saying package functions aren't virtual by
 design (I hope not), or that this is just the current implementation?


Whether it's by design or not is not entirely clear.  The only thing the spec
says about 'package' in this regard is that it is basically an extension of
'private', in which case it makes sense (in a strange way) that 'package' would
make a method nonvirtual.  But it just seems like it's conflating two entirely
different concepts: access and virtuality.  Private methods can and should be
nonvirtual, since that's a valid optimization.  But it's entirely reasonable to
have a package method that can be overridden.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Aug 19 2009
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3258


Mike Shulman <viritrilbia+d gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |viritrilbia+d gmail.com



PDT ---
Since private methods are visible to the entire module in which they are
defined, and thus could reasonably be overridden by derived classes defined in
the same module, I don't see why making them all non-virtual is any more valid
of an optimization, in principle, than doing the same for package methods.

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


Andrej Mitrovic <andrej.mitrovich gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |andrej.mitrovich gmail.com
            Version|D1 & D2                     |D1



10:39:11 PST ---
I don't know the state of D1 but in D2 these overloads are flagged as errors,
so I'm removing D2 from the list. As for non-virtual private/package - this
should be an enhancement request, but it's probably been debated many times
already (I personally believe access specifiers should not mess with the
virtuality of a method).

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




---
Andrej, that seems like a funny change to me. Unless D2 either allows virtual
package functions or states that these are not allowed it seems that it's still
broken to me. The spec says "All non-sta­tic non-pri­vate non-tem­plate mem­ber
func­tions are vir­tual." which I interpret to include package members.

Not that I particularly care any more...

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


Jonathan M Davis <jmdavisProg gmx.com> changed:

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



PST ---
It's been discussed many times, and the spec is wrong in this case (I keep
meaning to update it but haven't done so yet). public and protected member
functions are _always_ virtual unless the compiler can determine that they're
never overridden (which can only happen when they're final). private and
package member functions are _never_ virtual.

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




15:09:12 PST ---



Yeah I know, the spec tells one thing on one page, then another thing on
another. It says "Package extends private" on the protection attributes page,
implying it's final like private.

Small note: In terms of the compiler implementation, the private and package
set to non-virtual boils down to a single `if` check in the front-end.

Forcing private and package non-virtual must have a rationale in the spec.
There isn't one, so Walter and Andrei should state exactly why this decision
was made.

Actually, Walter and Andrei obviously had a miscommunication, because Andrei
expected private to allow being virtual in TDPL. And 'package' being
non-virtual isn't mentioned in TDPL either (lots of people get confused by
this). So maybe Walter is the one to write the rationale.

I also believe it's not too late to change the state of things. We have the
`final` keyword after all. Yes, making private and package virtual could
potentially slow down code, but fixing it is trivial, it just requires a
`final` keyword. Perhaps the the compiler can be made smart enough to optimize
non-overriden private/package methods so they're made final. Maybe if -lib or
-c are not used it can do such optimizations.

It's ironic that the argument against private/package being virtual is because
of performance reasons when we've already made the mistake of making all
methods virtual by default. I *still* can't get used to the fact that a public
method is virtual by default, it really lacks that documentation aspect of
having a 'virtual' keyword right next to it. 

And as argumented before, you could easily forget to mark something as 'final'
in a library, the user overriding such a method, followed by an API update
where you do a bugfix and mark it as final, which ends up breaking the user
code. Unfortunately this also becomes an argument against making
private/package virtual by default too.

Non-virtual by default + a virtual keyword + no limits on virtuality based on
access specifiers => dream come true (for me).

We're in an odd status-quo, all I can do is sigh at the situation.

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



 Dec 21 2012



prev sibling next sibling parent  d-bugmail puremagic.com  writes:


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




17:04:28 PST ---


 Non-virtual by default + a virtual keyword + no limits on virtuality based on
 access specifiers => dream come true (for me).


I forgot one thing that completes the circle, and that is friend declarations.
That way private inheritance actually becomes useful beyond the module the
class is declared in. I never liked the idea of having private leak into the
entire module to begin with, I want fine-grained access via friend declarations
and access specifiers which don't mess with virtuality. Otherwise I'm left
using PIMPL idioms or mixin templates (yuck!).

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



 Dec 21 2012



prev sibling next sibling parent  d-bugmail puremagic.com  writes:


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






 I forgot one thing that completes the circle, and that is friend declarations.


There are no friend declarations in D.  So what exactly are you talking about?

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



 Dec 21 2012



prev sibling next sibling parent  d-bugmail puremagic.com  writes:


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




---




 I forgot one thing that completes the circle, and that is friend declarations.


 
 There are no friend declarations in D.  So what exactly are you talking about?


Look up friend classes/functions in C++ and his meaning should be clear.

For the problem at hand though, without causing a lot of breakage, that means
either private/package become virtual by default, private/package can never be
virtual, or a 'virtual' (or 'overridable') keyword is introduced. In theory I
agree with Andrej, but would probably vote that handling all protection methods
in the same way is preferable.

In an ideal world though, I think I'd choose to make overriding explicit and
shadowing illegal: i.e. unless 'virtual'/'override' is specified names must not
clash with those from a base class.

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



 Dec 22 2012



prev sibling  parent  d-bugmail puremagic.com  writes:


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






 For the problem at hand though, without causing a lot of breakage, 
 that means either private/package become virtual by default, 
 private/package can never be virtual, or a 'virtual' (or 
 'overridable') keyword is introduced.  In theory I agree with 
 Andrej, but would probably vote that handling all protection 
 methods in the same way is preferable.


At the moment, we have final, which is roughly the negation of virtual.


 In an ideal world though, I think I'd choose to make overriding 
 explicit and shadowing illegal: i.e.  unless 'virtual'/'override' 
 is specified names must not clash with those from a base class.


I agree that it would be a way forward to kill overriding without the override
attribute once and for all.  Once this is done, we can implement this
principle:

- If a private symbol is redeclared in a subclass in the same module, it must
have the override attribute, and it overrides the superclass method.

- If a private symbol is redeclared in a subclass in a different module, it
must not have the override attribute, and the two are distinct symbols with no
relation to each other.  Basically, the subclass has no knowledge of the
superclass's private members (as it should be).

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



 Dec 22 2012