HaxeFoundation / haxe-evolution

Repository for maintaining proposal for changes to the Haxe programming language
111 stars 58 forks source link

Abstract classes #69

Closed RealyUniqueName closed 4 years ago

RealyUniqueName commented 4 years ago

Introduce classes that provide incomplete implementation and cannot be instantiated directly.

Rendered version

Simn commented 4 years ago

It does not matter if a class has private modifier before or after abstract

This will be annoying to handle in the parser, but I suppose there's no way around that. We had the same issue with final.

Abstract class is not required to implement all the methods of interfaces that class implements. Instead missing interface implementations are implicitly treated as abstract methods

Hmm, that's interesting. I'm worried about confusing error messages here because this involves 3 different types (the abstract class, the interface and the implementation class). But if this is how it works in Java then I agree we should comply.

Aurel300 commented 4 years ago

Abstract method is not allowed to have an implementation

Why not? I think default implementations can be quite useful, especially when an abstract class provides multiple methods defined in terms of the other methods. Then only providing an implementation for some is enough.

ibilon commented 4 years ago

You can just have a normal function which you'll override if you want an implementation, if I understand correctly. Doesn't make much sense to both have a default implementation and force reimplementation.

Aurel300 commented 4 years ago

Ah indeed, the methods also have to be marked with abstract. My bad :)

Ilir-Liburn commented 4 years ago

I can live with abstract class syntax, but I would prefer abstract keyword to stay as compile time feature, while virtual keyword could better describe runtime feature, e.g. something which exists at runtime, but it is not real unless implemented.

And that leads to another interesting thing: if abstract class implements interface, access to members of inherited classes could be a lot faster through abstract class than trough interface.

And what about private constructor on abstract class? Will be allowed? Forcing super call on inherited classes?

RealyUniqueName commented 4 years ago

No special rules for constructors. That's why I didn't mention it.

grepsuzette commented 4 years ago

I like it.

There is another alternative that I sometimes use (but I’ve always missed traits or abstract classes as it is described here):

abstract access modifier cannot be used with final or inline access modifiers

Can you define static vars in an abstract class if theses vars are not marked abstract? I think this was not explained. (But I guess so, same as if you implement a function in the abstract class by not declaring that function abstract).

Lastly for me the biggest problem is that future Haxe beginners will have a hard time figuring out what all these abstracts are. We already know the language so it’s not a big problem for us.

ah one last question: Were you suggesting modifying current abstract Y (Int) to abstract type Y (Int) or just call it abstract type in the documentation? I wouldn’t mind the first one if we introduced abstract classes...

kaikoga commented 4 years ago

We already have enum abstract and I don't think we'll introduce another keyword here, so I suppose the "abstract type" notion is just document or "human wording" issue.

grepsuzette commented 4 years ago

We already have enum abstract and I don't think we'll introduce another keyword here, so I suppose the "abstract type" notion is just document or "human wording" issue.

On further thinking yes. abstract type (Y) must be wrong, since classes and enums are also types, and as you said it would force us renaming enum abstract E to enum abstract type E (and break syntax).

I feel the reason I don't feel comfortable with all the variants is partly because historically it would have been more logical getting abstract class before abstract Y (C), since the later is a bolder concept while the first one concept is widespread in OOP.

It also means, if in the documentation we introduce abstract class before abstract, and abstract before enum abstract, it will be easier for beginners. Abstract class should be a basic OOP feature for classes, while abstract can be a more advanced or Haxe-only concept, in a different section.

RealyUniqueName commented 4 years ago

Updated with clarifications regarding constructors and static fields.

RealyUniqueName commented 4 years ago

@ncannasse @andyli @hughsando @nadako @jdonaldson ?

markknol commented 4 years ago

If you don't mind, I have some questions:

  1. Can you mix implementation in an abstract class? Or does it have to be all abstract? Eg. can you do this:

    abstract class Abstr {
    public final value:String;
    public function new(value:String) {
      this.value = value;
    }
    
    public abstract function toString():Void;
    }

    or

    abstract class Abstr {
    public function sayHello() trace(toString());
    
    public abstract function toString():Void;
    }
  2. Can you define abstract properties? Can imagine it could be useful for getters/setters too.
jdonaldson commented 4 years ago

I vote to accept.

There's several places in the Haxe std lib where we rely on hacks to get around the lack of a proper abstract class. Input comes to mind: https://github.com/HaxeFoundation/haxe/blob/development/std/haxe/io/Input.hx (check where we throw "not found", etc).

I think where we do this, the code feels awkward and fragile. Haxe contributors don't feel confident making changes since they don't understand the impact across all targets, and as a result the code becomes neglected. We really should fix it.

I realize that we will have this weird semantics problem with "abstract" as a descriptor... it could be "abstract type", "abstract enum", or "abstract class", all of which are fundamentally different in potentially surprising ways. I think the notion of"abstract" should get a special section in the manual at the very least. Understanding the notion of "abstract" is key to exploiting the power of Haxe (probably more so even than macros), but I doubt many folks in the community clearly understand these concepts.

I'll give more specific thoughts inline in other places, just wanted to give my top level thoughts here.

Simn commented 4 years ago

Yes from me too.

I'm very much looking forward to a cleaner standard library that doesn't have these throw "Not implemented" things.

kaikoga commented 4 years ago

For having common implementation mixed in abstract classes I think it's the core strength of abstract classes so it should be allowed, I wonder if any of our targets with abstract classes actually disallowed it.

Abstract properties

abstract function get_foo() and abstract function set_foo() looks to be legal already, and they are what actually makes sense.

One thing I've remembered: Is the override keyword required when we implement the abstract method, as in C# ?

grepsuzette commented 4 years ago

One thing I've remembered: Is the override keyword required when we implement the abstract method, as in C# ?

We would not be overriding any implementation, so as a Haxe user it would bring confusion and inconvenience if we had to use override (I suspect in C# they are tied to their virtual/non-virtual function system).

In C# I think an abstract class implementing an interface also needs to provide implementation for that interface, contrarily to Java or the present proposal IIRC.

RealyUniqueName commented 4 years ago

Added notes about abstract properties, override and that abstract classes can contain implementations.

nadako commented 4 years ago

Requiring override is an interesting question. Not having it is consistent with interfaces and convenient. However one point for having override is that without it, removing an abstract method from the base class won't trigger errors, potentially leaving unused methods in the children. While with override we would get the "no super method to override" error.

Also it's not clear to me how this plays with visibility "inheritance". E.g. currently you don't have to specify public when overrideing a method since it'll be inherited. It would be nice to keep this behaviour, but it looks weird without override.

That said, In general I'm in favor of proper abstract classes, good job!

grepsuzette commented 4 years ago

I see what you mean, but one also have to think about implemented methods in the abstract class, which can be overriden too:

abstract class WhatAndWhere {
  abstract public function where() : String;
  public function what() return "it's what it is";
}

// style 1, no override for implementation
class SomeAnswers extends WhatAndWhere {
  function where() return "In the universe";
  override function what() return "42";
}

// style 2: with override 
class TrueAnswers extends WhatAndWhere {
  override function where() return "here";
  override function what() return "what it is";
}

All in all I prefer style 1. If it matters you can always specify visibility (and get an error if you attempt to e.g. change a public to private). But at least when you see override you know you trully did have a change of default behaviour, unlike style 2 where that information is lost.

In my view that information is more important for a programmer (we spend a lot of time reading declarations) than knowing when a mandatory method was removed from an abstract class (it would just become a useless method, and it happens unfrequently).


In fact --and this is a separate question-- Why do we need the abstract prefix for methods too? I read the doc again, and I fail to see what makes it impossible to drop it. Can't we just have abstract in front of the class and nowhere else?

Not that I don't like it, in fact with complex return types and optional {} it may be necessary to help programmer to quickly discriminate abstract methods from the rest, but we'd better figure out now whether they're just decorative and we deliberately opt to enforce them, or if we simply have no choice. Like: are we talking about abstract methods right now, or simply about unimplemented fields of an abstract class? If we enforce the keyword, they will be mostly the same thing.

hughsando commented 4 years ago

I am in favour of requiring override - it reduces the chance of typos/misunderstandings from going unnoticed, which is really what I like about override in "normal" classes in the first place. As for "abstract" - I like it on the functions and not on the class. On the class seems redundant ("at least one of the functions is abstract") , and confusing with other uses of "abstract class".

hughsando commented 4 years ago

You are right. I was thinking: "override function wot()" - "wot does not override anything", but "function wot()" will generate "missing abstract what", so no real problem there. I still have a slight preference for 'override', mainly for consistency and to allow a default implementation in the future without breaking anything.

On Mon, Jan 13, 2020 at 10:13 AM Emugel notifications@github.com wrote:

@hughsando https://github.com/hughsando If you make a typo in the case of abstract classes, it gets you an error because you have not implemented something. Therefore what is the added benefit, could you provide an example?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/HaxeFoundation/haxe-evolution/pull/69?email_source=notifications&email_token=AAMWTVXA6EIYY6XGA43JKO3Q5PE6DA5CNFSM4JWUHDG2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIXLLAY#issuecomment-573486467, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMWTVSTRJPBTGE35O6KTNLQ5PE6DANCNFSM4JWUHDGQ .

kaikoga commented 4 years ago

Requiring override when implementing abstract methods is at least consistent with throw "Not implemented" and therefore should have less impact when existing code would adopt abstract classes, which would be a pretty minor pro.


For abstract class notation, one can define an abstract class with every function implemented, yet it should not be directly instantiated, as the proposal says:

Abstract classes may or may not contain abstract methods.

So abstract class notation is actually not redundant, unless we imply abstract class when we encounter one or more abstract function, which doesn't look like a good idea. Marking abstract methods abstract, on the other way, is also not redundant, when we look at this example:

extern abstract class AbstractClassFromJava {
  function thisMethodHasImplementationInJava():String; 
  abstract function butThisMethodDoesNot():String;
}

class Implementation extends AbstractClassFromJava {
  // override function thisMethodHasImplementationInJava():String {
  //   return "We are just overriding Java implementation";
  // }
  override function butThisMethodDoesNot():String {
    return "Abstract method in Java is implemented with Haxe! Cool!";
  }
}
hughsando commented 4 years ago

Ok, so I misunderstood the "abstract class" thing. As it stands, it is not the sort of thing I would have ever wanted to use. The "abstract function" thing I really like, but really dislike the need to declare the class abstract. Seems like double-entry bookkeeping, that could simply be stated as "any class with unimplemented abstract functions can't be instantiated". No need to annotate the class with something the compiler can deduce. There is not much difference between "Class X can't be created because function Y in not implemented" vs "Class X can't be created because it was declared abstract" or "Class X can't be created because it is abstract due to missing functions Y,Z" I guess a macro to "auto-annotate classes as abstract" would do the trick, but ultimately would be too slow. Also, have you considered re-declaring methods as abstract (ie, require re-implentation)? c++ allows this, although I'm not sure how widely it is used, and I'm not necessarily advocating it.

grepsuzette commented 4 years ago

Also, have you considered re-declaring methods as abstract (ie, require re-implentation)? c++ allows this,

Maybe it's simply a bare-metal consequence of their vtable of functions (e.g. you set the pointer to 0 and it requires 'implementation' again) rather than a real feature? In fact it's been a long time since I don't do c++ so IDK, it might be a decision left to the implementer.

kaikoga commented 4 years ago

Maybe what abstract class competes against is private constructor, which also disallows direct instantiation of the class but still allow from inside itself or subclasses. With that restriction we might live without abstract class notations, taking "allowing instantiation" and "having some methods unimplemented (causing the class unable to directly instantiate)" nuances sort of completely apart.

Yes we have the whole @:allow @:access @:privateAccess things, but you should know what you're doing :)

hughsando commented 4 years ago

I think of the "=0" as a vtable thing conceptually, but it is actually a compile-time thing. I have only ever seen it used once in real life.

The combination of private constructor and not creating classes with dangling abstract classes covers the cases I would use, but an alternative might be "abstract function new" which would require a derived class to have a "new" (and call super I guess)

RealyUniqueName commented 4 years ago

Missing abstract keyword on an abstract class would be confusing when reading code. You'll have to look through all the methods or compile the project to find out if the class is abstract.

RealyUniqueName commented 4 years ago

As for override - it helps to avoid situations when you accidentally override some implementation. In this case override helps to state that the implementation is overridden intentionally. But for abstract methods (in my understanding) removing or adding parent abstract methods with the same name/signature should not lead to a compilation error. That's why I think override should not be required in this case. And that's how it works for interface implementations too.

hughsando commented 4 years ago

I disagree that missing abstract on class is sufficiently confusing to warrant the extra busy-work or requiring making synchronized changes (eg, add abstract function to base class requires all intermediate classes to change). Especially since the keyword is "abstract" which is super-confusing on a class. This is like the case with a private "new" - if you are looking through source-code, you have to do some searching. But compiling, documentation or comment can tell you. I believe this is the haxe philosophy, and why I like haxe. It does not make you annotate the types when it can infer them - the compiler does the work, and can fully check the logic. I don't see why it should be different the the class "abstractness".

RealyUniqueName commented 4 years ago

What if there are no abstract methods, but I still want the class to be abstract?

Simn commented 4 years ago

I can see why you would want to infer the abstractness of a class. However, I'm worried about the confusion potential when it's not clear that/why a class is abstract. On the other hand, we'll have the same problem for required abstract modifiers because we'll need some sort of "This class has to be declared abstract because ..." error.

hughsando commented 4 years ago

I'm not sure why you would want an abstract class with no abstract methods - perhaps a use case would help me understand. But a private constructor would have a similar effect. And you could always work around the intention of (fully resolved) abstract classes with class A extends some.abstract.Class { }; new A();. So abstract would mean "you just have to go though some pain to use this class" - a construct I'm not a fan of.

Simn commented 4 years ago

To be clear, we should definitely allow abstract class. The question for me is if we want to make it optional with clear rules for inference.

kaikoga commented 4 years ago

One possible use case for an abstract class with no abstract methods would be the Active Record pattern, where the default implementation to resolve the table name in database may use reflection, and you definitely want to override the class name.

grepsuzette commented 4 years ago

One possible use case for an abstract class with no abstract methods would be the Active Record pattern, where the default implementation to resolve the table name in database may use reflection, and you definitely want to override the class name.

Yes you mean some class fields generated using reflection directly from a database table, like so class BestScore extends ActiveRecord {} ?

But easy workaround:

class ActiveRecord {
   // many implemented methods and...
   abstract public function tableName() : String;
}
class BestScore extends ActiveRecord { 
    function tableName() return "BestScore";
}

My knowledge of Haxe maybe lacking, but I have to ask what follows regarding the presence of abstract keyword before class.

The possibility to have non-static methods in abstract types has been removed recently IIRC (Edit: this is just for @:op fields, I remember to have had to convert some @:op non-static functions in Haxe 3 to static in Haxe 4). But since this worked already, can we guarantee at some point in 2032 we will not want to have this syntax:

abstract Foo (String) {
    // ...
    abstract function bar();
}

That is abstract methods in abstract types? If so, should we not want to drop the abstract in front of the class as well, and talk about abstract methods instead of abstract classes? I like the proposal as it is, but I just want to make sure.

grepsuzette commented 4 years ago

We seem at a deadlock. I fear I contributed to it.

I want to withdraw my idea of having abstractness depend merely only unto the presence of an unimplemented abstract function and support the original proposal, with mandatory declaration of abstract class. It was bad thinking from me (the moment you don't have it mandatory means any new class you stumble upon you can not possibly know whether it is abstract or not, that kind of doubt can be pretty toxic).

nadako commented 4 years ago

Another thing that might be worth mentioning is that super.abstractMethod() (if it is actually abstract and not an implementation in the middle of hierarchy) should be forbidden.

This might be implied by

If a class provides an implementation for an abstract method it does not need override keyword as no actual implementation is overridden.

But still, depending on the implementation we might miss that check, so better to mention it and add unit-test later.

Simn commented 4 years ago

We have decided to ~reject~ accept this proposal in our haxe-evolution meeting yesterday.

There was consensus that this feature is useful. An open debate is whether or not the abstract-ness of a class should be inferred by the compiler. We decided to implement abstract classes first and then discuss the inference as a followup issue.