godot
godot copied to clipboard
godotengine
Add a keyword for abstract classes in GDScript
Implements and closes https://github.com/godotengine/godot-proposals/issues/5641
This PR adds a keyword for marking a script class as abstract in GDScript.
I tested multiple combinations of abstract and non-abstract inheritance and they all work. As an example, in this image MyAbstract is abstract, while the other is not. Note that if ExtendsMyAbstract was made abstract then both would be hidden.
abstract class_name MyAbstract extends Node
class_name ExtendsMyAbstract extends MyAbstract
And here's what happens if you try to instance with .new():
A previous version of the PR used @virtual, but the feedback has been overwhelming that abstract is better. After that, another previous version of this PR used an annotation @abstract, but it was discussed that a keyword is preferred.
Production edit: closes godotengine/godot-roadmap#66
- I think the
@abstractannotation should be possible to apply not only to the top-level script, but also to inner classes. Applying the annotation to a script should not make inner classes abstract. - The check should be performed in release builds too.
- Perhaps this should be keyword
abstract(likestaticand potentialpublic/private) rather than annotation@abstract, especially if we add the ability to declare methods abstract (must be implemented in a child class so that it can be non-abstract).
Talked about in the current GDScript meeting. The PR should implement the elements 1 and 2 stated by @dalexeev. For number 3, we're still discussing this in the meeting, so no decision has been made.
Yeah, my thoughts on 3. are whether it makes sense to (for this PR or eventually) have the "granularity" of abstractness exist at the class or at the method level. For example, with it at the method level:
class AbstractBecauseOfAbstractMethods:
func this_method_is_implemented():
pass
@abstract
func this_method_is_virtual_slash_abstract()
func this_is_also_implemented():
pass
# ----------------------
class StillAbstract extends AbstractBecauseOfAbstractMethods:
func some_new_implemented_func():
pass
# ----------------------
class FinallyNotAbstractClass extends StillAbstract:
func this_method_is_virtual_slash_abstract():
pass
We could force the users to annotate a class to be marked as abstract if any of its functions are still abstract.
This would give people more granularity in what precisely makes the class abstract. It doesn't make GDScript any more expressive, since you can always 1) tag a class as abstract, then 2) have methods implemented with assert(false) or pass, which would be functionally the same.
Folks agreed during the meeting that this wouldn't be a blocking thing for this PR, and that it could be implemented later :)
I will get to work on 1. For 2, this is trivial because it's just removing #ifdef DEBUG_ENABLED. To be clear I don't mind it in release builds, I just wanted to exclude it for improved performance, but if it's desired then we can do that. For 3, I think an annotation makes more sense than a keyword.
that would be nice to have in 4.2 ;)
4.2 beta 1 means feature freeze, so that won't happen.
But you can help test and review the implementation so that it's ready to merge early on for 4.3.
@MikeSchulze To clarify further, a PR can't be considered for merging until it's reviewed. You can help by reviewing it. The more reviews, the better.
You may worry about not being senior enough to review, don't be, anyone can review. Reviews from non-members will show up as a gray checkmark, which doesn't unblock the actual merge button, but it's still valuable as information that it's tested and working. Just say what you did in the review (did you compile it? test it? look at the code? etc).
IMHO I think the same reason made static a keyword applies for abstract/virtual as well.
Also more consistent with C# syntax.
When I see decorators I think of additional functionality rather than core design.
@salloom-domani But it is additional functionality, it sets a flag that is read by the editor to determine if the script is abstract or not, so it knows to gray out the type in the Create New Node dialog. The @tool annotation is very similar, it sets a flag that is read by the editor to determine if the script should be run in the editor.
@aaronfranke For me, this is more of a language feature, since it prohibits instantiation of the class, and not only makes the class unselectable in the editor dialog. Especially if we add abstract methods later (#82987). That is, abstract is a common modifier for languages, the same as static and private. If we add this, then it makes sense for all three to be either keywords or annotations. I think we should look into the annotation vs keywords issue. See also godotengine/godot-proposals#5165 and godotengine/godot-proposals#6192.
@dalexeev I would say static is far more fundamental than abstract, as it drastically changes behavior, while @abstract and a potential @private only restrict behavior, preventing you from doing something.
This PR initially did not have @abstract used in release mode since it is mostly a tool for code organization, and you can just drop it to get the same behavior for code that ran correctly in the editor. I changed this due to feedback since then. But anyway, if you drop or add static, the behavior vastly changes and is mutually incompatible in both directions.
It does not make sense to me for static to be an annotation.
To support @aaronfranke opinion, global abstract annotation indeed makes more sense than a keyword since it's kinda like @tool and more style consistent.
However, it will become an issue when abstract methods are implemented, because it has to be annotated for consistency reasons, even though a keyword better suits this case.
Yeah that's true, if the behavior is significantly different with abstract methods a keyword might make more sense.
I'm confused on what we're waiting on for this to be merged. Are we just waiting for people on the gdscript team to review this? Is there something I can do to help?
I'm confused on what we're waiting on for this to be merged. Are we just waiting for people on the gdscript team to review this? Is there something I can do to help?
You can help get this PR merged by testing it locally and making it sure it works correctly (make sure to report results).
We can't guarantee this PR will be merged for 4.3 though, since it has the 4.x milestone.
I'm confused on what we're waiting on for this to be merged. Are we just waiting for people on the gdscript team to review this? Is there something I can do to help?
There's ongoing discussion among GDScript contributors on whether this should be an annotation or a keyword. We'll update when a consensus is reached.
At today's GDScript team meeting, we reached a consensus that abstract should be a keyword instead of an annotation.
@aaronfranke Please let us know if you would like to continue working on this and if you need any help. You can discuss implementation details on the #gdscript channel. Thanks for your patience!
I don't think the name abstract is good, because it creates a confusion with native abstract classes which behave differently (e.g. can't be inherited), this will create confusion when talking about it or documenting stuff about it.
Could someone link me to the previous version were virtual was discarded?
