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

Abstract class support #5452

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Abstract class support #5452

wants to merge 4 commits into from

Conversation

popham
Copy link
Contributor

@popham popham commented Dec 3, 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.

  • An earlier attempt (Add support for abstract classes #4005) allowed buggy use of methods on the super reference even when the method was abstract on the superclass. This has been solved.
  • Mixins don't work with abstracts. If a declared class has a mixin, then the extended class (explicit or implicit) and the mixin class(es) must all be non-abstract.
  • The async keyword is allowed on the abstract methods of classes (not declared classes). There's no function body for return value lifting, so it's just sugar.
  • Generator syntax is allowed on the abstract methods of classes (not declared classes). Again, there's no function body, so it's just sugar.
  • Only contravariant access of class fields on abstract classes is permitted.
  • An abstract prefix on classes gets eaten for compatibility with TypeScript's abstract classes. The prefix doesn't alter Flow's behavior. My implementation diverges from TypeScript's in that TypeScript doesn't allow abstract static methods. This choice allows TypeScript to admit static method calls on abstract classes, whereas my implementation forbids such calls.
  • Abstract instance methods and abstract class methods only admit a single signature. This constraint may be relaxed in the future.
  • Once an abstract instance method or abstract 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.
  • Properties cannot be marked abstract.

When I first saw the number of lines touched, I thought "holy crap." I then went through and counted the number of non-test lines touched:

  • Abstract static and non-static method parsing: 969 lines,
  • Abstract classes: 1781 lines,
  • Update extant parser tests: 0 lines, and
  • Support optional abstract prefix on classes for TypeScript compatibility: 5 lines

The PR is probably too huge in its current form for a merge, but can I negotiate a sequence of smaller pull requests working up to a final, abstract class PR? Or I could partition changes into commits with finer granularity? In particular:

  • src/parser/type_parser.ml and src/parser/object_parser.ml both parse classes and accumulate members while logging errors. I've introduced a functor to internalize accumulation and most of the error handling. See src/parser/object_members.ml, src/parser/type_parser.ml#L499-L535, and src/parser/object_parser.ml#L411-L514. This eliminates the error_unexpected_variance functions and their respective call sites in type_parser.ml and object_parser.ml.
  • Error messages for abstract method compatibility during the SuperT flow required the locs of full fields, not just those of the keys. This added another typical entry to Type.Property.t (beyond Loc.t option and TypeTerm.t). Bundling these typical entries into a tuple simplified a lot of the data processing in src/typing/class_sig.ml. Further, the introduction of Property.read: Property.t -> Property.triple and Property.write: Property.t -> Property.triple simplified a few ugly pattern matches.
  • Many of my parser changes were intended to fail better than the current version. There exist classes of bad data that the parser could fail better on. I don't have much of a vocabulary for describing these classes of bad data.

fixes #3129 and fixes #5318

@popham popham force-pushed the abstract branch 2 times, most recently from 7954fb6 to 772add4 Compare December 3, 2017 07:49
@popham popham changed the title Abstract Class Support Abstract class support Dec 4, 2017
@popham popham force-pushed the abstract branch 5 times, most recently from b337559 to a3a448a Compare December 12, 2017 01:05
@popham popham force-pushed the abstract branch 9 times, most recently from 1a00f4c to c17c5d8 Compare December 20, 2017 19:40
@taion
Copy link

taion commented Feb 9, 2018

This would be pretty nice to have. In the context of object-oriented APIs, it's pretty nice to have abstract classes where consumers have to implement specific methods to get certain behaviors. While it's possible to do this with e.g. required configuration objects, there are in fact many use cases where abstracts feel easier.

This is a pretty big PR, and it looks like it's been around for a while and now has conflicts, but it'd be quite nice to have this feature.

@TrySound
Copy link
Contributor

TrySound commented Feb 9, 2018

Missing these object-oriented features is one of the reason people choose flow over typescript.

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.

Dead parser code Can't use static as an object key
4 participants