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

Add support for abstract classes #4005

Closed
wants to merge 22 commits into from

Conversation

popham
Copy link
Contributor

@popham popham commented May 23, 2017

This adds support for abstract methods and abstract static methods, e.g.

class Foo {
  static k: number
  abstract m(): void;
  abstract static sm(): void;
}

Setting of class fields like k are admitted, but gets are rejected:

Foo.k = 1; //ok
let n: number = Foo.k //ng

I didn't bother with digging into class initializers, but I expect that they won't work properly with abstracts because the this that they use doesn't bother with abstractness checking.

@popham popham force-pushed the abstract_class_methods branch 2 times, most recently from 1fe99da to a7b961d Compare May 24, 2017 16:43
@nmote
Copy link
Contributor

nmote commented May 24, 2017

Thanks for putting up the PR!

We are discussing this, but it might be a while. Not only is it a pretty large change which will take a while to review, but we also need to be careful whenever we add new features to the language. One concern is that if we merge this, and then a future ES spec includes abstract classes that are slightly different, we will have to unify them somehow. This sort of thing needs to be carefully considered.

@popham
Copy link
Contributor Author

popham commented May 24, 2017

ACK

@popham
Copy link
Contributor Author

popham commented May 24, 2017

I've been push -fing rebases. If anybody starts working with this PR separately, then make a bit of noise so I can use my manners.

@popham
Copy link
Contributor Author

popham commented May 26, 2017

I just noticed that ES6 has dropped the reservation on abstract from the spec (11.6.2).

@popham
Copy link
Contributor Author

popham commented May 30, 2017

I just noticed that there's a TypeScript proposal with some relevant commentary: microsoft/TypeScript#3578.

@nmote
Copy link
Contributor

nmote commented May 30, 2017

That's good context, thanks. Some of that mirrors the things we have been discussing. Like I said, the main concern is that if someday the ES standard includes abstract classes, we will have to reconcile with that. However, the consensus seems to be that abstract classes are mainly a type system feature, so it seems unlikely that they will make it into the standard in the near future.

One possibility for such a feature would be to check, at class construction time, that if the current class is concrete and the parent class is abstract, that the current class provides implementations for all the abstract methods of the parent class (and its parents). Of course, I haven't thought this through and it may in fact be a bad idea, but it is conceivable that abstract classes could be implemented without a static type checker. However, they certainly make more sense as a type system feature.

Anyway, the consensus is that the potential risk is small enough that we are willing to add abstract classes.

Could you write a summary of abstract classes, as implemented by this PR? I first want to make sure that we are all on the same page. I noticed in your tests that there is a new AbstractClass type -- I would like to make sure that it is included in your summary, since it's not immediately clear to me what it is for.

Unfortunately I don't feel qualified to review this PR but I can certainly act as point of contact, and at least give it a once-over before handing it off to someone else.

Thanks again for your work on this!

@popham
Copy link
Contributor Author

popham commented May 30, 2017

Sweet! Give me a few days.

@popham
Copy link
Contributor Author

popham commented Jun 6, 2017

This PR introduces abstract instance methods (like Java's abstract and C++'s pure virtuals) and abstract class methods to Flow. Abstractness on class methods sounds goofy from my Java and C++ intuitions, but ES6's prototype chaining on static methods (and Flow's enforcement of subtyping on those static methods) makes abstract class methods an expected feature beside abstract instance methods. In addition to the constraint on instantiation that abstract instance methods bring to Java and C++, abstract class methods bring in more constraints: No static methods can be called on an abstract class and its static fields can only be set.

  • Class<.> annotations have been migrated to produce NonabstractClassTs that reject abstract classes.
  • To accomodate flows from abstract classes, an AbstractClass<.> annotation has been introduced. This annotation, like class declarations, produces ClassTs which could be written more accurately as PossiblyAbstractClassTs. My one annoyance with the AbstractClass<.> annotation is that it doesn't allow degrees of abstractness; there's no way to properly annotate a function that extends an abstract class by providing non-abstract implementations of a strict subset of the class's abstracts, e.g.
class A {
  abstract m(): string;
  abstract static n(): void;
}

function ext(s: string): AbstractClass<A> {
  //Class<A> is a ng return type because Ext has an abstract static `n`.
  return class Ext extends A {
    m(): string { return s; }
  };
}

let E1 = ext("asdf");
// An extension of E1 must provide an `m` and a static `n` to obtain a non-abtract class.

I was thinking a syntax like AbstractClass<A, ["m"], ["n"]> (where both tuples are optional) could be sufficient.

  • Only contravariant access of class fields on abstract classes is permitted. The covariant access of class fields on such classes was forbidden to prevent users from sneaking in a this-referencing abstract method and then calling it. In retrospect, I feel that covariant access is safe because a direct call (and therefore an abstract this) would entail a MethodT flow that is properly gated for abstractness; I suppose bind and CallT could sneak around somehow, but I haven't explored the possibility. While the covariant access constraint may be unnecessary, I'm not offended by the limitation--it feels kinda icky to be doing more than the minimum with static fields on an abstract class anyhow.
  • Within an abstract class's instance and class methods, this gets treated as a non-abstract. The abstractness gating at instantiation and at class uses excluding contravariant field access together is sufficient to guarantee a non-abstract this at runtime.
  • Within an abstract class's instance and class methods, I've neglected super. My understanding is that super(...) calls are all safe and that super.x and super.y(...) need to check for the existence of non-abstract x and y methods on the superclass.
  • Abstract instance and class methods admit multiple signatures. Should this be limited to one?
  • Once an abstract instance or class method has been implemented, it cannot be declared abstract again further down the inheritance chain.
  • Constructors cannot be marked abstract.
  • Getters and setters cannot be marked abstract.
  • I was worried about interactions between field initializers and abstract methods, but looking at https://tc39.github.io/proposal-class-public-fields/#initialize-public-static-fields, I found
    • 1.5.4a(iii).1 states that this is undefined within class property initializers, so I don't have to worry about abstract/nonabstract this.
    • For instance fields, abstractness gating at ConstructorT guarantees that this is nonabstract when InitializePublicInstanceFields gets called during [[Construct]].

I just realized that I've been saying "non-abstract" instead of "concrete." Within the code I used variations on "non-abstract" instead of "concrete" to avoid conceptual overloading on other uses of "concrete." Apologies if I sound like a bigger idiot than usual.

@popham
Copy link
Contributor Author

popham commented Jun 26, 2017

It looks like abstractness handling of super methods within instance methods is going to take some nontrivial data remodeling. This PR is therefore not ready for review. Hopefully I'll get around to it within the next month.

@nmote
Copy link
Contributor

nmote commented Sep 11, 2017

Hey, I'm just tidying up old PRs. This hasn't seen any recent activity, and would probably be a huge pain to rebase at this point. I also had trouble getting someone to sign up to review a diff of this scope. If you'd like to take another stab at this problem we are certainly open to it but before you put more time into the implementation it might be worth discussing in an issue and getting a commitment from someone to review.

I really appreciate your willingness to step up and implement something like this, even if this time it didn't make it across the finish line.

@nmote nmote closed this Sep 11, 2017
@popham popham mentioned this pull request Dec 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants