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 classes #69

Merged
merged 3 commits into from
Jun 19, 2020
Merged

Conversation

RealyUniqueName
Copy link
Member

@RealyUniqueName RealyUniqueName commented Dec 6, 2019

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

Rendered version

@Simn
Copy link
Member

Simn commented Dec 6, 2019

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
Copy link
Member

Aurel300 commented Dec 6, 2019

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
Copy link
Member

ibilon commented Dec 6, 2019

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
Copy link
Member

Aurel300 commented Dec 6, 2019

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

@skial skial mentioned this pull request Dec 9, 2019
1 task
@Ilir-Liburn
Copy link

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
Copy link
Member Author

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

@grepsuzette
Copy link

grepsuzette commented Dec 10, 2019

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):

  • have an interface X,
  • have a class XCommon not implementing X (but still declare some compatible functions) then
  • have all other classes implementing X AND extending XCommon.
    It works relatively well if you have a lot of classes, otherwise it’s not worth the trouble.

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
Copy link

kaikoga commented Dec 10, 2019

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
Copy link

grepsuzette commented Dec 11, 2019

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
Copy link
Member Author

Updated with clarifications regarding constructors and static fields.

@markknol
Copy link
Member

markknol commented Jan 3, 2020

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;
}
  1. Can you define abstract properties? Can imagine it could be useful for getters/setters too.
@jdonaldson
Copy link
Member

jdonaldson commented Jan 3, 2020

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
Copy link
Member

Simn commented Jan 3, 2020

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
Copy link

kaikoga commented Jan 3, 2020

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
Copy link

grepsuzette commented Jan 4, 2020

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
Copy link
Member Author

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

@nadako
Copy link
Member

nadako commented Jan 12, 2020

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
Copy link

grepsuzette commented Jan 12, 2020

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
Copy link
Member

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
Copy link
Member

hughsando commented Jan 13, 2020 via email

@kaikoga
Copy link

kaikoga commented Jan 13, 2020

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
Copy link
Member

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
Copy link

grepsuzette commented Jan 13, 2020

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
Copy link

kaikoga commented Jan 13, 2020

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

RealyUniqueName commented Jan 13, 2020

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
Copy link
Member

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
Copy link
Member Author

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

@Simn
Copy link
Member

Simn commented Jan 13, 2020

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
Copy link
Member

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
Copy link
Member

Simn commented Jan 13, 2020

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
Copy link

kaikoga commented Jan 13, 2020

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
Copy link

grepsuzette commented Jan 14, 2020

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
Copy link

grepsuzette commented Mar 15, 2020

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
Copy link
Member

nadako commented Mar 30, 2020

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
Copy link
Member

Simn commented Jun 19, 2020

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.

@Simn Simn merged commit cba0c19 into HaxeFoundation:master Jun 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment