Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discussion: Implicit base-method calls #9041

Closed
lachbaer opened this issue Feb 23, 2016 · 37 comments
Closed

Discussion: Implicit base-method calls #9041

lachbaer opened this issue Feb 23, 2016 · 37 comments

Comments

@lachbaer
Copy link
Contributor

I just read (in a good old paperback ;-) about extending derived classes by overwriting virtual methods, BUT that those methods must care about calling their base class at either the beginning or end of the method. In #8985 there came up something similar, for extending existing methods.

I think that it would be a good concept in C# as well, to force the programmer to induce the base method call.

I don't have a clear vision on how the syntax for this could be, but maybe something like this:

public class BaseClass {
  // : this keyword forces overwriting methods to call their parent's method
  public virtual void BaseMethod() : this { /* logic */ };
}
public class ChildClass : BaseClass {
  public override void BaseMethod()
    : base()
  {
    // like constructor calls, the base method is called before this body.
    // because the base method declares ': this' at the end
    // the override MUST declare ': base()'
  }
}

Unlike with constructors, that can be expanded with base(...) and this(...) with different parameters, the base call must have the same signature, to really call the same base method and not any overload.

Because the base call might be useful at the end (or middle) of the body, there must be some way to define that. Maybe

  public override void BaseMethod()
    : base  // no parenthesis!
  {
    /* compiler checks, whether a base call is made somewhere in the body */
    base(/* arguments  */);
  }

The base(...) call keyword cares about calling the right base method of the parent and gives the compiler the hint that the constraint is respected. To define or call a base() method is not allowed in current C#, so this extension would not break existing code.

I know, for just 1 derived class this can be done in different ways. But as soon as you have a hierarchy chain, things get messy and complicated and error prone.

@kishkk
Copy link

kishkk commented Feb 23, 2016

                                                                                                                                                                                                                                                                                                                                                                                                                          Sent from my BlackBerry 10 smartphone.                                                                                                                                                                                                                From: lachbaerSent: Tuesday 23 February 2016 15:19To: dotnet/roslynReply To: dotnet/roslynSubject: [roslyn] Discussion: Implicit base-method calls (#9041).n‎.n'‎I just read (in a good old paperback ;-) about extending derived classes by overwriting virtual methods, BUT that those methods must care about calling their base class at either the beginning or end of the method. In #8985 there came up something similar, for extending existing methods.

I think that it would be a good concept in C# as well, to force the programmer to induce the base method call.

I don't have a clear vision on how the syntax for this could be, but maybe something like this:

public class BaseClass {
public virtual void BaseMethod() : this { /* logic */ };
}
public class ChildClass : BaseClass {
public override void BaseMethod()
: base()
{
// like constructor calls, the base method is called before this body.
// because the base method declares ': this' at the end
// the override MUST declare ': base()'
}
}

Unlike with constructors, that can be expanded with base(...) and this(...) with different parameters, the base call must have the same signature, to really call the same base method and not any overload.

Because the base call might be useful at the end (or middle) of the body, there must be some way to define that. Maybe

public override void BaseMethod()
: base // no parenthesis!
{
/* compiler checks, whether a base call is made somewhere in the body /
base(/
arguments */);
}

The base(...) call keyword cares about calling the right base method of the parent and gives the compiler the hint that the constraint is respected. To define or call a base() method is not allowed in current C#, so this extension would not break existing code.

I know, for just 1 derived class this can be done in different ways. But as soon as you have a hierarchy chain, things get messy and complicated and error prone.

—Reply to this email directly or view it on GitHub.

@lachbaer
Copy link
Contributor Author

@kishkk can you please delete your comment, something went wrong with your blackberry mailer

@lachbaer
Copy link
Contributor Author

Oh, I should mention, that any overwrite in the hierarchy can declare ': this', as long as one of their ancestors hasn't done so before. base(...) always calls the nearest ancestor that defines/overwrites the method.
new methods can declare : this again - as they start a new hierarchy.

@dsaf
Copy link

dsaf commented Feb 23, 2016

Template method pattern baked into language? It might be hard to create a static analyzer for all situations.

@HaloFour
Copy link

Without CLR support there's nothing to prevent someone from using a different language and simply deciding to not call the base method. Choosing to not call the base method is a tenet of inheritance, that's why it's called "override" and not "chain" or something similar.

@DavidArno
Copy link

Probably no surprises here, but my take on this is simple: class inheritance is a really really bad legacy feature of C#. Therefore the language design team should waste no time on developing new features around overriding methods. Let's instead extend the language with new idioms (such as records) and make inheritance less idiomatic, so it can quietly die a well-deserved death through non-use.

@dsaf
Copy link

dsaf commented Feb 23, 2016

LOL :). Knives are bad because you may cut a finger.

@HaloFour
Copy link

@DavidArno No, it's not a "really really bad legacy feature", it's a core tenet of both the language and it's runtime. I'm sorry that you don't like it but functional programming is not the idiomatic way of doing everything. Not every problem requires a screwdriver.

@DavidArno
Copy link

@HaloFour,

Inheritance really is a train-wreck of a language feature. I subscribe firmly to the 'don’t just “favour composition over inheritance”, instead just don’t ever use inheritance' school of thought. If you want a through examination of everything wrong with inheritance, take a read of "Inheritance: just stop using it already!" (written by me).

A mixture of interfaces and composition can already replace everything anyone would want to do with inheritance. Functional features will just make it even less relevant.

@HaloFour
Copy link

@DavidArno Gee, an article that you wrote favors your opinion? Whodathunkit. Guess what, a mixture of inheritance and OOP language design can replace anything anyone would want to do with composition and functional programming, too. Not every problem is a nail. C# is, above all, an OOP language. Don't like it? Don't use it.

@lachbaer
Copy link
Contributor Author

@DavidArno Maybe you should favor another programming language over all? ;-)

@lachbaer
Copy link
Contributor Author

@HaloFour We should kill the overwrite keyword as well. Java doesn't need it, C# won't need it... sarcasm BTW, it was a book about Android using Java where not calling the super-Method can lead to unwanted side effects. As C# is also about avoiding such mistakes, I thought it might be worth a discussion.

@HaloFour
Copy link

@lachbaer

C# added the overrides keyword to prevent a common Java mistake of attempting to override a method but accidentally misspelling the name of the method. Java 5 added the @Override annotation which will cause the Java compiler to error if the decorated method doesn't actually override anything.

Yes, it's possible that by not calling the base method can cause unwanted side effects. However, that should be the exception, not the rule, and if anything is indicative of a poor type design. If the base method has functionality that must be invoked then rather than surface a single virtual method to be overridden it should keep that method sealed and potentially provide one or two virtual methods that it will invoke before and after that required functionality.

@lachbaer
Copy link
Contributor Author

@HaloFour

and potentially provide one or two virtual methods that it will invoke before and after that required functionality.

When you have an inheritance chain then that concept won't stand. Because the derived virtual method must seal itself and provide its own virtual methods, so that the mandatory call chain still stands.

I wonder if this could be done with attributes as well? Is there any generic compiler attribute in the framework with a string field that could give it a hint and issue at least a warning if it finds no base.Method()? Though this still does not force the overwriting method to call its base.

@DavidArno
Copy link

@lachbaer,

That's a good question. The simple answer is that C# is now open source and we all get a say in how it develops. Why walk away from that golden opportunity and use another language when I can help ensure it grows and changes to match current, modern, thinking on good development techniques?

The single most popular proposed change for C# 7 is to add pattern matching. That's a reflection on where the development community is heading. OO is widely (though clearly not, universally) recognised as a failed paradigm. C#'s future therefore lies in becoming more of a multi-paradigm language that puts immutable data at its heart; not state-riddled, inheritance-laden "rotten domain model" objects.

@orthoxerox
Copy link
Contributor

I think template methods are good enough.

public abstract class Base
{
    public void Foo()
    {
        //do cool stuff
        FooInternal();
        //more cool stuff
    }

    protected abstract void FooInternal();
}

public class Derived : Base
{
    protected override void FooInternal()
    {
        //derived code
    }
}

Yes, they get a little wordy when you have to nest them several levels deep.

@HaloFour
Copy link

@lachbaer This pattern works just fine in inheritance chains:

public class Foo {
    public virtual void DoSomething() { }
}

public class Bar : Foo {
    public override sealed void DoSomething() {
        OnBeforeDoSomething();
        // mandatory stuff here
        OnAfterDoSomething();
    }

    protected virtual void OnBeforeDoSomething() { }
    protected virtual void OnAfterDoSomething() { }
}

public class Baz : Bar {
    public override void DoSomething() { } // compiler error

    protected override void OnBeforeDoSomething() { }
    protected override void OnAfterDoSomething() { }
}

@lachbaer
Copy link
Contributor Author

@HaloFour I had this in mind also, before I wrote the initial statement of this discussion. Damn, it does not come to my mind now, but there was an issue with that design...

@lachbaer
Copy link
Contributor Author

@HaloFour Don't recall if it was this, but in a chain the programmer always has to take care that in OnAfterDoSomething he has to put base.OnAfterDoSomething(); first. The intend of this discussion is, whether it is a good idea to somehow force the programmer to follow this paradigm, so that no accidents can happen, because she left the instruction.

@dsaf
Copy link

dsaf commented Feb 23, 2016

@DavidArno

OO is widely (though clearly not, universally) recognised as a failed paradigm.

Do you include Alan Kay's definition of OO into this?

Also the most universal ("misunderstood") language is adding classes in ES6 I've heard...

I can help ensure it grows and changes to match current, modern, thinking on good development techniques?

All feedback is welcome I am sure. Also the current C# team is very sympathetic towards functional programming, so it's safe in that respect.

@DavidArno
Copy link

@dsaf,

Do you include Alan Kay's definition of OO into this?

No, in the same way I don't include "causing terror" as a meaning for the word "terrific". Language use drives the meaning of words, at least in English, not the other way around.

Also the most universal ("misunderstood") language is adding classes in ES6 I've heard...

Having spent a few years writing AS3 code (which also supported class and prototype inheritance), I can only extend my deepest sympathies to anyone who attempts to create inheritance models in ES6...

@HaloFour
Copy link

@DavidArno Not liking it doesn't make it fail. In any case this conversation is entirely pointless and useless on these forums in general as the OOPness of C# isn't changing. Adding features from other languages and paradigms where they fit is indeed useful, but there's nothing about pattern matching which affects OOP. If anything languages like Scala demonstrate that the two paradigms can fit nicely side-by-side. The CLR is an ecosystem that encourages many languages and paradigms.

@alrz
Copy link
Contributor

alrz commented Feb 23, 2016

You can use this pattern (I don't know if it has a name).

class Base {
  public void Method() {
    // required behavior
    MethodOverride();
     // ...
  }  
  protected virtual void MethodOverride() {
    // default behavior
  }
}

If with override your code may break, you mustn't make your method virtual in the first place, because virtual methods are not intended to care about the override. Perhaps you should reconsider your design not the language's.

PS: I think this is called the "template method pattern" as @dsaf said.

@dsaf
Copy link

dsaf commented Feb 23, 2016

@alrz
Copy link
Contributor

alrz commented Feb 23, 2016

@dsaf Yeah I mentioned it in a PS but it was a dupe. I lost my mind and didn't read the discussion first. Sorry.

@dsaf
Copy link

dsaf commented Feb 23, 2016

@alrz no problem, I was trying to be helpful rather than implying anything :).

@dsaf
Copy link

dsaf commented Feb 23, 2016

This is not going to be implemented because it would require Code Contracts level of static analysis.

Stuff like this is much easier said then done:

/* compiler checks, whether a base call is made somewhere in the body */

E.g. did I call base() here or not?

  public override void BaseMethod()
    : base  // no parenthesis!
  {
    if (_something && _somethingElse)
        base(/* arguments  */);
  }

#119

@lachbaer
Copy link
Contributor Author

Well, the compiler can check whether "not all path return a value". I'm not that much into the internals, but in my eyes this is something very similar.

I think having a modifier, like overwrite and virtual are, would be the best on addition to these.

base(arguments) as call would still be very suitable, because it checks for the correct signature and in case of a refactoring no name change is needed. As well as it protects against additional overloading and then accidentally not correcting the call in the derived method.

@axel-habermaier
Copy link
Contributor

@lachbaer: "all paths return a value" is trivial to check compared to "is base() ever called"; for the latter, you'd have to solve the halting problem, which is impossible.

@svick
Copy link
Contributor

svick commented Feb 23, 2016

@lachbaer

I wonder if this could be done with attributes as well? Is there any generic compiler attribute in the framework with a string field that could give it a hint and issue at least a warning if it finds no base.Method()?

That sounds like something that would work pretty well as an analyzer.

@lachbaer
Copy link
Contributor Author

@axel-habermaier What if I've solved it, already some years ago...? :-D Just kidding ;-)

I don't mean to inspect all code paths, just a check if the base is called anywhere in the block.That doesn't allow for nasty programmings, but they can always be done anyhow. It only ensures that the programmer thought of calling the base method somewhere.

Actually I can only think of 2 points where calling it really makes sense, at the very beginning (like a constructor chain) or at the end, when leaving, like a finally, e.g. on deserialisation.

After all, it might be good to have a base(arguments) that ensures the call to the ancestor's method of the same signature.

@manixrock
Copy link

Perhaps allow methods to behave more like events. Maybe have before and after events?

public class BaseClass {
    public sealed event int BaseMethod(int dice) { /* logic */ };
}
public class ChildClass : BaseClass {
    public int before BaseMethod(int dice) { ... }
    public int after BaseMethod(int dice) { ... }
}

Or maybe:

public class ChildClass : BaseClass {
    public int BaseMethod(int dice):before { ... }
    public int BaseMethod(int dice):after { ... }
}

Or maybe even:

public class ChildClass : BaseClass {
    public int BaseMethod(int dice) {
        before { ... }
        after { ... }
    }
}

This would allow returning non-void values, insure the base method remains unchanged and is callable, but still allows code to be wrapped around it without worrying whether the root method is called.

Events could even be chained in successive descendants so they can specify where in the call tree the call should take place:

public class GrandChildClass : ChildClass {
    public int ChildClass:before BaseClass:before BaseMethod(int dice) { ... }
    public int ChildClass:before BaseClass:after BaseMethod(int dice) { ... }
    public int ChildClass:after BaseClass:before BaseMethod(int dice) { ... }
    public int ChildClass:after BaseClass:after BaseMethod(int dice) { ... }
}

This would create a tree-like structure of event calls, with a level for each derived class. This would give options to the developer where to place the event so it interacts properly with other method-event listeners.

Edit: changed return type to int

@lachbaer
Copy link
Contributor Author

@manixrock

  public void ChildClass:before BaseClass:before BaseMethod(int dice) { ... }

One inheritance paradigm of C# (and most other OOP languages) is that only the parent can be called not the grandparent.

    public void BaseMethod {
        before(int dice) { ... }

Apart from the point that this could aready be done with the On... protected calling scheme the signature public void BaseMethod { would introduce a property to the compiler.

I don't think this is the way to go.

I guess, the most compliant way to make something like this work is to apply an attribute to the base class:

[BaseMethodMustCallAttribute(CallPosition = before)]
public virtual void BaseMethod() {}

then the compiler can check for a base() call (where base is a keyword) in the derived method; not the semantics, just if base is called somewhere.

Apart from that even the syntax

  public override void BaseMethod()
    : base() { /**/ }

makes sense to me, as it works like in the constructors, except that only methods with the same signature and void return type can be called like this. When the return type must be taken into account, var result = base(/**/); must be stated in the body explicitly.

@manixrock
Copy link

@lachbaer what about:

public class BaseClass {
    public sealed extendable int BaseMethod(int dice) { /* logic */ };
}
public class ChildClass : BaseClass {
    public extend int BaseMethod(int dice)
    {
        ...
    }
    : base() // compiler would ensure the base method is called (should arguments be passed?)
    {
        ...
    }
}

This way we can wrap methods that return non-void. The two extra code blocks would both be optional and be able to access the argument int dice, and both return void.

@HaloFour
Copy link

No C# changes are necessary, this entire proposal can be solved through an analyzer possibly combined with an attribute that can hint to the analyzer whether not calling the base member should be a warning, an error or ignored.

@lachbaer
Copy link
Contributor Author

lachbaer commented Mar 1, 2016

@HaloFour is probably right, that an analyser would be better than some static code syntax.

But after all I would like to have a base(arguments); call made possible, that calls the corresponding base method with the same signature. In my eyes this makes reading easier and saves from hard to find issues when refactoring the code.

Also what objects about appending the method signature with base?

void Method(int FirstParameter, string SecondParameter) 
        : base(FirstParameter, SecondParameter) { }

@gafter
Copy link
Member

gafter commented Apr 21, 2017

We are now taking language feature discussion in other repositories:

Features that are under active design or development, or which are "championed" by someone on the language design team, have already been moved either as issues or as checked-in design documents. For example, the proposal in this repo "Proposal: Partial interface implementation a.k.a. Traits" (issue 16139 and a few other issues that request the same thing) are now tracked by the language team at issue 52 in https://github.com/dotnet/csharplang/issues, and there is a draft spec at https://github.com/dotnet/csharplang/blob/master/proposals/default-interface-methods.md and further discussion at issue 288 in https://github.com/dotnet/csharplang/issues. Prototyping of the compiler portion of language features is still tracked here; see, for example, https://github.com/dotnet/roslyn/tree/features/DefaultInterfaceImplementation and issue 17952.

In order to facilitate that transition, we have started closing language design discussions from the roslyn repo with a note briefly explaining why. When we are aware of an existing discussion for the feature already in the new repo, we are adding a link to that. But we're not adding new issues to the new repos for existing discussions in this repo that the language design team does not currently envision taking on. Our intent is to eventually close the language design issues in the Roslyn repo and encourage discussion in one of the new repos instead.

Our intent is not to shut down discussion on language design - you can still continue discussion on the closed issues if you want - but rather we would like to encourage people to move discussion to where we are more likely to be paying attention (the new repo), or to abandon discussions that are no longer of interest to you.

If you happen to notice that one of the closed issues has a relevant issue in the new repo, and we have not added a link to the new issue, we would appreciate you providing a link from the old to the new discussion. That way people who are still interested in the discussion can start paying attention to the new issue.

Also, we'd welcome any ideas you might have on how we could better manage the transition. Comments and discussion about closing and/or moving issues should be directed to #18002. Comments and discussion about this issue can take place here or on an issue in the relevant repo.


I am not moving this particular issue because I don't have confidence that the LDM would likely consider doing this.

@gafter gafter closed this as completed Apr 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests