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

Suggestion: abstract classes #6

Closed
RyanCavanaugh opened this issue Jul 15, 2014 · 50 comments
Closed

Suggestion: abstract classes #6

RyanCavanaugh opened this issue Jul 15, 2014 · 50 comments
Labels
Committed The team has roadmapped this issue Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Jul 15, 2014

Support an abstract keyword for classes and their methods

Examples:

abstract class Base {
    abstract getThing(): string;
    getOtherThing() { return 'hello'; }
}
var x = new Base(); // Error, 'Base' is abstract

// Error, must either be 'abstract' or implement concrete 'getThing'
class Derived1 extends Base { }

class Derived2 extends Base {
    getThing() { return 'hello'; }
    foo() { super.getThing(); } // Error: cannot invoke abstract members through 'super'
}
var x = new Derived2(); // OK
var y: Base = new Derived2(); // Also OK
y.getThing(); // OK
y.getOtherThing(); // OK

abstract class Empty { } // OK
@am11
Copy link

am11 commented Jul 27, 2014

👍

The example would be more awesome if you incorporate virtual as well. 😄

abstract class Base {
    abstract getThing(): string;
    getOtherThing() { return 'hello'; }
    virtual getVirtualThing() { return 'hello virtual'; }
}
...

@DanielRosenwasser
Copy link
Member

@am11 given that methods in JS get called by traversing the prototype chain, all methods are already "virtual" in the traditional sense that C++, C#, Java, and similar languages define it.

For instance:

class E { 
    f () { alert("E"); }
}

class F extends E {
    f () { alert("F"); }
}

var x: E = new F();
x.f();

will alert "F".

Did you have something else in mind?

@am11
Copy link

am11 commented Jul 27, 2014

@DanielRosenwasser, that is so true. Thanks!

Actually, I was thinking about the conventional OO keywords virtual and overrirde, so its easy to clearly differentiate between various inheritance behaviors. The TS compiler might just throw warning if those keywords are missing in applicable scenarios and fallback to standard JS convention.

I believe this way we can bring C#'s new modifier as well:

abstract class A {
    public void foo() { }
    protected virtual void bar() { }
}

class A1 : A {
    protected override void bar() { }
}

there may exist A2, such that:

class A2 : A {
    public new void foo() { }
}

to explicitly hide super's implementation (without new keyword there, CS compiler gives warning).

@RyanCavanaugh
Copy link
Member Author

new is infeasible given JavaScript's runtime semantics; given that we've already shipped the language this way, virtual would be the default in the absence of any other modifier. Let's take discussion of those modifiers to a different suggestion if needed.

Seems like the example above has enough information to discuss. Some open questions:

  • Can I have an abstract class with zero abstract methods?
  • Is the abstract modifier implicit in a class that doesn't implement all abstract methods?

@am11
Copy link

am11 commented Jul 29, 2014

Can I have an abstract class with zero abstract methods?

Since abstract class -- in principle -- serves a purpose of grouping commonalities together such that its objects alone don't make sense; I think we should rule no_abstract_method as a legit case and let the TypeScript linter take care of recommending best practices, code-styling et el.

Is the abstract modifier implicit in a class that doesn't implement all abstract methods?

Given an abstract class would be able to inherit another abstract class, implicit inference like this would be confusing? I guess (at least C#/Java) developers would expect compiler to throw error and editor to help them correct it, if the first non-abstract / concrete implementer is missing any expected override. Well as a Ruby guy, I'd probably appreciate this kind of crypto-magic :`)

@RamIdeas
Copy link

Even before abstract is implemented, I'm still hoping you'd allow intellisense kick in (in Visual Studio) when overriding base class functions:

class BaseClass {
  myFunction() { /* TO BE OVERRIDDEN */ }
}
class MyClass extends BaseClass {
  myF // intellisense should have kicked in -- but it doesn't
}

My only option is to open BaseClass.ts to check I'm spelling it correctly.

@RamIdeas
Copy link

I'd still prefer to see more descriptive keywords, as I mentioned on https://typescript.codeplex.com/discussions/449920

C# VB (on classes) VB (on methods)
abstract MustInherit MustOverride
virtual Overridable Overridable
sealed NotInheritable NotOverridable

From these more descriptive keywords, it can be seen immediately that the overridable keyword is unnecessary as methods in JavaScript are overridable by default as opposed to in C# and is less likely to lead to confusion demonstrated by @am11. I believe the same gain in clarity is achieved with the other keywords listed.

@RyanCavanaugh
Copy link
Member Author

We're trying to clear out our design backlog and I think this should come up soon once ES6 features are wrapped up. We discussed abstract in a design meeting a few months ago and there didn't seem to be any big open questions.

@metaweta
Copy link

The standard design pattern in JS is to throw an exception in the abstract class:

function Base() { throw new Error("Not implemented."); }
Base.prototype.f = function () { throw new Error("Not implemented."); };
Base.prototype.g = function () { /* default impl */ };

function Derived() {}
Derived.prototype.f = function () { /* impl */ };
Derived.prototype.g = function () { /* override impl */ };

The benefit TypeScript could provide is reducing boilerplate and a compile-time error.

@RyanCavanaugh
Copy link
Member Author

Discussed today in the design meeting.

The open question here is how we prevent you from trying to instantiate an abstract class. Consider some code:

abstract class Abs {
  constructor() { /* some general init here */ }
}

var x = new Abs(); // Desired: error

This case is easy -- when you invoke new on a symbol, we see if the symbol is a class with abstract, and error if so.

Next:

var a = Abs;
var x = new a(); // Not a good thing to do

This is less obvious. You probably want an error? But then consider this case:

class B extends Abs { /* ... */ }
class C extends Abs { /* ... */ }

var t: typeof Abs;
if(/*... */) {
  t = B;
} else {
  t = C;
}
var j = new t(); // This is fine

There's nothing distinguishing this case from the previous one (in terms of the type system), but the latter is perfectly valid code to write (and would probably even be common code to write in a factory method). We will probably have to not error on the new a(); case, both because it mimics a valid pattern and because attempting to prevent it would require a lot of extra type system mechanics.

@Vadorequest
Copy link

What about an hidden field?

Let's say we have a class

abstract class Abs {
  constructor() { /* some general init here */ }
}

Because the class is abstract, we cannot instanciate it, you said you wanted to check if the class was abstract or not to do so. What if having a abstract class would generate somehow hidden field ___abstract___ = true. If the class isn't abstract like with class B extends Abs { /* ... */ } then we would have the ___abstract___ = false, so to check if a class is abstract or not we could just check that hidden field.

I don't know if it's a proper solution, or even if it's possible, but the point is to check if the closest class is abstract or not, whatever if it extends and abstract class or not.

@mihhail-lapushkin
Copy link

I think, you don't have to go crazy with this.
I believe, the main thing that people want from this is to be able to get a compile-time error on non-implemented abstract methods and also to omit implementation of interface methods in abstract classes.
The rest is great, but not that crucial. Start small and extend if needed.

@mwisnicki
Copy link

Abstract type shouldn't have visible constructor. i.e. new t() in above example shall fail.

If user wants constructible type he should explicitly allow it which is possible thanks to type unions:

var t: typeof Abs | new() => Abs;

@iislucas
Copy link

I would have thought that a type being abstract would be a bit in the type system. That's how people code it up in other functional languages. Then the compiler maintains knowledge of which types are abstract and which are concrete. Make sense?

@pspi
Copy link

pspi commented Dec 13, 2014

I wouldn't mind if the typeof corner-case doesn't catch abstract.

I feel the common use case is to simply define abstract methods that are left for child classes to implement.

@Griffork
Copy link

I like two proposed solutions personally,

  1. Abstract classes are type only, they have no code and what would be their code is added to child classes code.
  2. don't handle the typeof case.
    Here's something else too: abstract classes could have an init method instead of a constructor, and their constructor could error. The init method must be called by child classes (like super ()) and must be bound to a child instance. Init methods on abstract class must call their parent's constructor or init.
    This leaves the constructor free to contain a throw new error and still have child classes be able to work correctly.
    The method doesn't need to be called init, and can be marked with an underscore or something if deemed necessary.

@RamIdeas
Copy link

@Griffork, your first proposition wouldn't allow for someChild instanceof MyAbstractClass at runtime

@RyanCavanaugh, instances like your typeof example is when I really hate static typing. With static typing this shouldn't be allowed because there is a chance that variable holds an abstract class. In an ideal world, the compiler would "run" the code and only error in the case I've assigned non-abstract classes to the variable (not the case in your example).

Also, are you definitely going with the abstract keyword?

@RyanCavanaugh
Copy link
Member Author

I don't think any alternatives have even been mentioned.

@RamIdeas
Copy link

I think mustoverride would be a more descriptive keyword for abstract methods

For alternatives in other scenarios that you may use abstract, please see my comment above


To spell it out, I think mustoverride, mustinherit, notoverridable and notinheritable are more descriptive keywords

@Griffork
Copy link

@RamIdeas Good point, didn't think of that.

Alternatives to mustoverride and mustinherit that are more descriptive of the current behaviour than the intended use case would include notcallable and notnewable.

After all, you can't garentee how something is going to be used, but you can garentee it's state, and that's what the abstract and virtual keywords were designed to do.

@enoshixi
Copy link

I would say prevent directly instantiating abstract classes at compile time. Otherwise let it be a run time error. It works the same way in other statically typed languages.

// e.g. C#
object value = new Stream(); // compile time error

Type type = typeof(Stream);
value = Activator.CreateInstance(type); // this compiles fine but will fail at runtime

What about something like this to prevent runtime instantiation?

// TypeScript
abstract class GreeterBase {
    greeting: string;
    constructor(message: string) {
        this.greeting = message;
    }
    abstract greet(): void;
}

class Greeter extends GreeterBase {
    constructor() {
        super("World");
    }

    greet() {
        alert("Hello, " + this.greeting);
    }
}
// JavaScript
var GreeterBase = (function () {
    function GreeterBase(message) {
        throw new Error("Cannot instantiate abstract class");
    }
    GreeterBase.prototype.__ctor = function (message) {
        this.greeting = message;
    }
    return GreeterBase;
})();
var Greeter = (function (_super) {
    __extends(Greeter, _super);
    function Greeter() {
        _super.prototype.__ctor.call(this, "World");
    }
    Greeter.prototype.greet = function () {
        alert("Hello, " + this.greeting);
    };
    return Greeter;
})(GreeterBase);

@nathggns
Copy link

That sounds like the best solution.

Sent from my iPhone

On 19 Dec 2014, at 19:20, enoshixi notifications@github.com wrote:

I would say prevent directly instantiating abstract classes at compile time. Otherwise let it be a run time error. It works the same way in other statically typed languages.

// e.g. C#
object value = new Stream(); // compile time error

Type type = typeof(Stream);
value = Activator.CreateInstance(type); // this compiles fine but will fail at runtime
What about something like this to prevent runtime instantiation?

// TypeScript
abstract class GreeterBase {
greeting: string;
constructor(message: string) {
this.greeting = message;
}
abstract greet();
}

class Greeter extends GreeterBase {
constructor() {
super("World");
}

greet() {
    "Hello, " + this.greeting;
}

}
// JavaScript
var GreeterBase = (function () {
function GreeterBase(message) {
throw new Error("Cannot instantiate abstract class");
}
GreeterBase.prototype.__ctor = function (message) {
this.greeting = message;
}
return GreeterBase;
})();
var Greeter = (function (_super) {
__extends(Greeter, _super);
function Greeter() {
_super.prototype.__ctor.call(this, "World");
}
Greeter.prototype.greet = function () {
"Hello, " + this.greeting;
};
return Greeter;
})(GreeterBase);

Reply to this email directly or view it on GitHub.

@Griffork
Copy link

@enoshixi
Exactly what I was trying to suggest, but much better presented!

@panost
Copy link

panost commented Dec 21, 2014

Just an idea of a more JScript oriented implementation of abstract methods, without abstract classes

class GreetBase {
    greeting: string;
    constructor(message: string) {
        this.greeting = message;
    }
    greet:()=>void;
}

class Greeter extends GreeterBase {
    greet() {
        console.log("Hello, " + this.greeting);
    }
}

and used as

function logGreet() {
    console.log( "Log: " + this.greeting );
}

var gr =  new Greeter("World");
gr.greet = logGreet; // supply implementation on the fly
gr.greet(); // "Log: World"
delete gr.greet; // back to prototyped
gr.greet(); // "Hello, World"

var gBase = new GreetBase("Abstract");
gBase.greet = logGreet; // supply implementation on the fly
gBase.greet(); // "Log: Abstract"
delete gBase.greet; // back to default
gBase.greet(); // error

If you wanted to prevent public construction of GreetBase then perhaps the constructor could be decorated with "protected"

mhegazy pushed a commit that referenced this issue Apr 16, 2015
@RyanCavanaugh RyanCavanaugh added Help Wanted You can do this and removed In Discussion Not yet reached consensus labels Apr 27, 2015
@RyanCavanaugh RyanCavanaugh added this to the Community milestone Apr 27, 2015
@RyanCavanaugh
Copy link
Member Author

Going to delete a bunch of 👍 comments to make this thread shorter (upvote feature please GitHub!).

List of fine people who have 👍'd:
basarat, ddotlic, nathggns, ca0v, oswdr, Mogglewump, jeffmay, Lenne231, Griffork, filipstachura, Vadorequest, mihhail-lapushkin, cy, koloboid, Elephant-Vessel, pspi, am11, FedericoZanin, Kagiyama, vnenkpet, rolyp, bvaughn, bryceg, Nimbus89, gotenxds

@RyanCavanaugh
Copy link
Member Author

Accepting PRs.

Design notes for potential implementors:

  • No runtime checking, per our usual design goals
  • It is an error to invoke new on an identifier that binds to an abstract class declaration. Indirect invocations, e.g. var x = MyAbstractClass; var y = new x(); are allowed, and the static side of abstract classes have the same construct signatures they would have if they weren't abstract
  • No abstract methods/properties/fields at this time

@DickvdBrink
Copy link
Contributor

For the record, I'm currently (trying) to implement this.

edit What is the best way for this? First some basic stuff and create a PR for review? Or implement it with full test-suite and stuff? Because when I'm moving in total wrong direction I might waste a lot of my time and maybe yours too.

@mhegazy
Copy link
Contributor

mhegazy commented Apr 28, 2015

We are open to looking at incremental changes. Just make sure the first iteration is substantial enough to warrant feedback.

I would recommend sharing your approach and design on this issue before jumping into implementation. this way we can help save you time early on.

@jeffreymorlan
Copy link
Contributor

Indirect invocations, e.g. var x = MyAbstractClass; var y = new x(); are allowed, and the static side of abstract classes have the same construct signatures they would have if they weren't abstract

I think this should be reconsidered. Special-casing only a direct "new MyAbstractClass" (rather than removing abstract classes' new() signatures) is hacky, and makes it easy to accidentally pass an abstract class to a function that instantiates it (function constructAndDoOtherStuff(clazz: new() => Foo) { ... })

Earlier, this code was given as an example in favor of abstract classes having a new() signature:

class B extends Abs { /* ... */ }
class C extends Abs { /* ... */ }

var t: typeof Abs;
if(/*... */) {
  t = B;
} else {
  t = C;
}
var j = new t(); // error here if `typeof Abs` is not instantiable

But t could be declared as new() => Abs instead, which better describes what is expected of a valid t value anyway. (There is no backward compatibility to worry about, since there are no abstract classes in existing TypeScript code.)

In fact, using typeof Abs here may already be an error even without abstract! Suppose that Abs has an extra constructor parameter, which B and C supply in their super(...) calls (This is a pattern I use a lot - currently, it's the only way for a class to have an abstract-like "hole" that must be supplied by subclasses):

class Abs { constructor(public typeName: string) {} }
class B extends Abs { constructor() { super("Class B"); } }
class C extends Abs { constructor() { super("Class C"); } }

var t: typeof Abs;
t = B;
new t(); // Error here - wrong number of parameters

Because Abs has the wrong constructor signature, we are already required to use new() => Abs instead of typeof Abs in this case. It doesn't seem an undue burden to require the same if Abs's "holes" take the form of abstract methods rather than constructor parameters.

@Griffork
Copy link

@jeffreymorlan there was a suggestion that users can program their abstract classes to error in the constructor if they wanted it to (it's above) like this:

class classa {
    constructor () {
        if(Object.getPrototypeOf && (Object.getPrototypeOf(this) === classa.prototype) {
            throw new error("classa is abstract and should not be directly instantiated");
        } 
    } 
} 

This specifically checks of classa is 'bottom' of the prototype chain.

@aozgaa aozgaa closed this as completed Jul 1, 2015
@RyanCavanaugh RyanCavanaugh added Committed The team has roadmapped this issue Fixed A PR has been merged for this issue and removed Help Wanted You can do this labels Jul 1, 2015
@RyanCavanaugh
Copy link
Member Author

Merged into master and will be available in our 1.6 release. Thanks @aozgaa !

@ghost
Copy link

ghost commented Jul 2, 2015

In case you are wondering, it all happened with #3579 (with no back reference to this original issue 😢)

@ddotlic
Copy link

ddotlic commented Jul 2, 2015

Yeah, thanks @aozgaa! Lately, I've been adding many, many throw new Error('this should not have been called, it's abstract') across my codebase with the intention of replacing it all with proper abstract support.

Beta/Alpha 1.6 cannot come soon enough 😉

@aozgaa
Copy link
Contributor

aozgaa commented Jul 2, 2015

My apologies about not offering the back-reference. Thanks @jasonwilliams200OK for linking above!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Committed The team has roadmapped this issue Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests