-
-
Notifications
You must be signed in to change notification settings - Fork 22.8k
GDScript: Replace abstract
keyword with @abstract
annotation
#107717
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
Conversation
be946d4
to
7767d3f
Compare
Hey! This PR feels more complete than #107713 because it has an explanation. I understand that it's "too late" to be trying to stop the moving train but still I would like to express my thoughts. I'm all for UX, consistency, minimalism, readability and legibility but I must say these reasons don't really justify the change IMO. I agree with the fact that From readability standpoint (keyword vs annotation) I think it really depends on the user, on how they perceive code and how they write it. What I really want to say is this. This is my opinion and a view of how I perceive annotations in GDScript:
That was what I wanted to say. If annotations are moving towards becoming a more generic system (allowing users to create their own like decorators in JS, attributes in C#, annotations in Java, etc.) then I'm all for it. |
I have always assumed annotations are used for anything that affect the editor in some way and keywords were for things that affect the code. So this change seems confusing to me. I shall give some quick (vague) examples:
I think having abstract as an annotation deviates from this and is a little confusing. |
I somewhat disagree with the above comments, I don't think it's fair to say that annotations are exclusive to the editor.
My only gripe with the reasoning outlined here is that, following the "Reasons For", doesn't this also apply to the I do think this reason
is quite valuable, simplicity of the underlying code is immensely valuable, especially in a project where open contributions are encouraged and things like this can improve coprehensibility. |
Honestly, to me I already accepted that the difference between keyword and annotations in GDScript currently is too arbitrary anyway, so I'm like... whatever. I'd normally prefer keyword, but if virtual and override will be annotations, I prefer consistency more. However, if there's ever a GDScript 3.0 (5.0?) on Godot 5, please make the annotation system more meaningful, with annotations only for things that need it, have a more clearly defined reason for annotations to exist. I think everything in the language where its presence/absence check is mandatory (i.e. that breaks forward-compatibility) should be a keyword, even if it has Then make annotations some extensible system allowing custom classes and some hook system to modify the compilation generically like a preprocessor (but syntax-based, not text-based like C). This would allow some GDScript features be introduced by addons, reducing some implementation weight on core devs. Core features could be introduced as inbuilt annotations when they can be forward-compatible, and as keyword otherwise. It would also facilitate testing these features by many users without recompiling the engine. |
virtual and override have not yet been introduced, and according to the link you sent me in a previous discussion here, they will be decided upon on a case by case basis. It seems that contributors could not decide upon a consensus, so we all have to suffer the consequences of a mish mash design. Although it already seems like there's a pattern for what an annotation is or is not, so why not continue with that. I guess that's unfortunately one of the issues with open source development. |
7767d3f
to
bdeb83d
Compare
@dalexeev Dang, we worked on the same thing. I said in the GDScript meeting that I would work on this. Un-un-bumped, and I updated my PR to match the more detailed documentation in your PR. You and I took different approaches, though. I initially dismissed the approach in your PR because it's pretty much only a syntax change, and still keeps the "special case" code in the language parser. The approach in this PR is to make it behave like other annotations, which for functions, does involve moving the body-or-not check to the analyzer stage. @romgerman @romgerman @jordedwards The plan is also to replace |
abstract
keyword with @abstract
annotation
@aaronfranke gotcha, sorry for not clarifying before assuming static was treated differently |
I think Both are modifiers, both apply to both classes and methods, and both are language-level features, that don't interact with other parts of the engine (like most annotations do, as @produno said). I understand the team's reluctance to add more keywords to the language, 3.X went too far into that direction and the ones that were turned into annotations in 4.0 are a good change. But I think this is going too far into the other direction. |
Here's how I've always interpreted the responsibilities of keywords and annotations:
As such, I fundamentally disagree with abstract, static, virtual, or override being annotations. I know that GDScript is developed specifically for Godot. But, I believe that the past language and implementation division of keywords and annotations was wise, even if it was unintentional. |
Based on vnen and my thought, the most reason why we treat some modifiers as annotations is because of the frustrating system of adding new keywords. Every time you add a keyword, you have to change a lot of files, even the core files of gdscript module. Don't believe it? Check the source file and see how these keywords work. Meanwhile, the handle of keywords is much more difficult than annotations, which just require registration and implementation method during their definitions. Adding more keywords means higher unreadability of the source code, increasing the difficulty of maintaining the engine module. Maybe we can dig deeper on this issue in GDScript 3.0 in the future and overhaul the whole keyword system to make it much more simpler to handle. As for I admit that it is still ambiguous for most of us when a syntax element is a keyword and when it is presented as an annotation. But for modifiers in the middle of the both, we have to discuss more and reach a clear and determined consensus on this question. |
All keywords modify some behavior in some way. Otherwise, they would just be white space. So, the very idea of modifiers as a distinct category is vague. I much prefer the distinction between language features (provided by keywords) and engine features (exposed to the language through annotations). Annotations act as an extension system for the language, enabling context-specific features (i.e., those from an engine or game) to be injected into the language. Naturally, adding an extension is easier than modifying core files. If keyword implementation could be streamlined, that's a separate matter. And, indeed, if keyword implementation is difficult, and a conversion is to be made, it makes far more sense to convert from annotation to keyword rather than from keyword to annotation. One could implement a language feature as a prototype annotation and then replace it with a keyword when it's ready. |
In fear of flogging a dead horse, I shall make one last comment. As an end user, and selfishly so, I do not care how complex something is to implement and maintain. All I care about is the end user experience being as good as it possibly can be. I don't think usability should be sacrificed for less complexity, or at least, if such an integral part of the experience is difficult to maintain, then I guess that's an issue within itself. (Of course I understand this is not the case for everything.) That said, I will respect the decision of the maintainers. I will just have nightmares of annotations littering my codebase. |
I'll note something that I think has not been said yet. To me, the main motivation to make
Making it a keyword forces parsers to resolve it immediately when building the syntax tree. This can bring problems, such as missing syntax highlighting, broken autocomplete, or unhelpful error messages while editing the code. To provide the best user experience, the language grammar needs to be agnostic to contextual details, such as whether a function needs a body. That type of question is better answered during type and variable resolution. I think some people feel strongly about |
Beyond a few early adopters using 4.5.dev5 I don't see any usability issues with changing this from a keyword to an annotation, so there isn't really a trade off between code complexity etc. and user experience I can see (and while we should reduce friction as much as possible the development versions are unstable and that should be well known, so some friction is to be expected) |
If the parser and variable resolution can't elegantly handle an The usability issues, or friction, of an |
Yes, that's the point I was making :) Language grammar is the important bit, not the internal implementation. Whether or not it has a leading |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This took me quite a moment
An abstract class is a class that cannot be instantiated directly. Instead, it is meant to be subclassed by other classes. Attempting to instantiate an abstract class will result in an error. | ||
An abstract method is a non-static member function that has no implementation. An abstract function has no body; instead, a newline or semicolon is expected after the function header. An abstract method defines an interface that subclasses must conform to, because the method signature must be compatible when overriding a base class method. | ||
If a class has at least one unimplemented abstract method (either its own or inherited), then it must also be declared abstract. However, the reverse is not true; an abstract class may have no abstract methods. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Attempting some consistent language. The entire class reference is guilty of this, and I could explain myself further, but I don't know if I should in one relatively random reply.
An abstract class is a class that cannot be instantiated directly. Instead, it is meant to be subclassed by other classes. Attempting to instantiate an abstract class will result in an error. | |
An abstract method is a non-static member function that has no implementation. An abstract function has no body; instead, a newline or semicolon is expected after the function header. An abstract method defines an interface that subclasses must conform to, because the method signature must be compatible when overriding a base class method. | |
If a class has at least one unimplemented abstract method (either its own or inherited), then it must also be declared abstract. However, the reverse is not true; an abstract class may have no abstract methods. | |
An abstract class is a class that cannot be instantiated directly. Instead, it is meant to be inherited by other classes. Attempting to instantiate an abstract class will result in an error. | |
An abstract method has no implementation. Therefore, a newline or semicolon is expected after the function header. This defines an interface that inheriting classes must conform to, because the method signature must be compatible when overriding a method from the inherited class. | |
If a class has at least one unimplemented abstract method (either its own or inherited), then it must also be marked as abstract. However, the reverse is not true; an abstract class may have no abstract methods. |
Or... I am struggling to keep the same information, but at the same time it's not the class reference's business to explain why abstract
classes are so useful. I think I would prefer:
An abstract class is a class that cannot be instantiated directly. Instead, it is meant to be subclassed by other classes. Attempting to instantiate an abstract class will result in an error. | |
An abstract method is a non-static member function that has no implementation. An abstract function has no body; instead, a newline or semicolon is expected after the function header. An abstract method defines an interface that subclasses must conform to, because the method signature must be compatible when overriding a base class method. | |
If a class has at least one unimplemented abstract method (either its own or inherited), then it must also be declared abstract. However, the reverse is not true; an abstract class may have no abstract methods. | |
An abstract class is a class that cannot be instantiated directly. Instead, it is meant to be inherited by other scripts. Attempting to instantiate an abstract class will result in an error. | |
An abstract method must have no implementation, and a newline or semicolon after the method's declaration. When defining or inheriting a class, its abstract methods must be overridden and compatibile. If this isn't done, the class must be marked as abstract. | |
[b]Note:[/b] An abstract class may have no abstract methods. |
Or something similar. You know this better.
What became a note in the above suggestion should ideally be placed below the [codeblock]
example.
Alternatively, it's worth considering that oftentimes there's a distinction between "classes" and "scripts". Even if essentially script inheritance works just the same way as classes do, descriptions like these further muddy the water...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dalexeev Thoughts? I just copied this from your PR's docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess inherited
instead of subclassed
sounds better. But I would like to use the term subclass
more often (inherited class
is fine but more verbose and child class
should never happen).
Now, I would prefer class
here instead of script
. While every class in GDScript is also a script, users may not realize it also applies to nested classes. Since this is GDScript docs, it's obviously talking about GDScript classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts? I just copied this from your PR's docs.
I just copied this from another PR. :)
I don't have a strong opinion about documentation. Personally, I would like to point out that static methods cannot be abstract (because GDScript does not have late static binding, unlike PHP). I agree about script vs class. Also, maybe the example in codeblock should be improved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess
inherited
instead ofsubclassed
sounds better. But I would like to use the termsubclass
more often
I must insist against "subclass" right now, if only to keep the terminology consistent. The term is uncommon compared to referring to the overall concept of inheritance.
child class
should never happen
You'd be surprised. Unfortunately "child/parent class" is roughly as common as "subclass"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite everything from #107717 (comment) has been addressed.
Inheriting classes must either provide implementations for all abstract methods, or the inheriting class must be marked as abstract. If a class has at least one abstract method (either its own or an unimplemented inherited one), then it must also be marked as abstract. However, the reverse is not true: an abstract class is allowed to have no abstract methods.
To make it brief, this final line is unusually verbose. keeping this in mind for later.
For those who still have confusion on whether
The documentation has clearly stated the usage of annotation, so in this case I don't think there is critical issue on converting |
That description could also apply to keywords. Modified:
So, it still leaves the issue of distinction. |
modules/gdscript/tests/scripts/parser/errors/static_abstract_func.gd
Outdated
Show resolved
Hide resolved
modules/gdscript/tests/scripts/parser/errors/static_abstract_func.gd
Outdated
Show resolved
Hide resolved
bdeb83d
to
00d4748
Compare
modules/gdscript/tests/scripts/analyzer/errors/abstract_func_with_body.gd
Outdated
Show resolved
Hide resolved
00d4748
to
de77ce2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Co-authored-by: Danil Alexeev <[email protected]>
de77ce2
to
1085200
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks for working on this!
Thanks! |
This PR has sparked so much controversy: 29 👍, 15 👎, yet why was it merged so quickly? Do 👍 and 👎 actually have any influence? Is the standard for merging a PR simply that the maintainer feels it's ready to merge? Another PR I'm following, #76462, has 101 👍 and zero 👎. What's holding it back from being merged? Some mentioned it was due to the lack of a physics maintainer, but since then, several other physics-related PRs have been merged 😕. All of this just feels odd to me. |
@scgm0 Reactions are taken into account, but are not the only or most important criterion when making decisions. The main criterion is the consensus of the community and maintainers on whether we want to see a feature in the engine or not, as well as on how feasible, maintainable and consistent with the engine design and other features it is. Sometimes, proposals that are highly supported by the community are difficult to implement or significantly conflict with the existing architecture and design of the engine. In the case of PRs, the reason may also be the lack of response from the maintainers, their temporary unavailability, etc. Something that prevents the production team from concluding that there is a consensus and the code is of good quality. Also, putting a thumb up or a thumb down is a matter of 5 seconds. We are all subject to emotional impulses, but not everyone spends at least 10 minutes to read the discussion and weigh the pros and cons. In this case, there is a consensus among the maintainers that this change is good. Please note that this does not affect existing functionality, the Finally, there is no solution that is without flaws and satisfies absolutely everyone. In my opinion, at least this criterion for dividing keywords vs annotations is the clearest and will allow us to avoid unproductive debates every time we want to add another modifier. See also my comment above. Thank you for your question, but please note for future reference that PRs are not intended for meta-discussions about Godot merge policy and priority. It is fine to ask something about a specific PR, but nothing more. Closed PRs on the one hand attract less attention from contributors, but on the other hand can cause unnecessary notification noise for many participants. Alternatively, you can ask the question in the Contributors Chat in the general or specialized channel (in this case |
From the response, the information I gathered is:
Godot is a community-driven project. Even if the above two points weren't the maintainers' original intent, these actions have left me feeling disheartened. That's all. Regardless, I hope everyone has a wonderful day. |
Yes, maintainers have the final say on what gets merged in the engine, if they have consensus among themselves. Input from the broader community is taken into account, and it was even discussed here, and all maintainers believe that the arguments made do not justify going against the direction chosen by the maintainers. We appreciate the feedback nonetheless. You should see this as a minor adjustment of the PRs adding support for declaring abstract classes and methods in GDScript. If maintainers had agreed beforehand to use annotations for those, most likely the feedback you'd see on the PR would be a hundred something thumbsup like #67777 gathered, and maybe a handful of people arguing for it to be made a keyword. It actually even started with an annotation and there wasn't significant pushback for a whole 2 years. The "popularity contest" ratio would be quite different to what is now seen on a PR that's just tweaking the implementation details of an unreleased feature. In that hypothetical scenario, you might have then had someone propose a change from annotation to keyword after merge, and receive maybe 17 thumbs up and 29 thumbs down. What would we then do? So we can't just operate by popularity contest, and popularity also doesn't translate to maintainability and people taking initiative to fix issues. So at the end of the day, the people who do take on responsibility to make the engine what it is over a long period of time (the maintainers) are the ones to make decisions on implementations. |
From the beginning (starting from #107713 which was made without stating any reasoning from the maintainers side except "we decided so") the reasoning from the maintainers has often felt inconsistent and underexplained. It felt like the maintainers weren't entirely sure why they decision was made themselves. And it comes up again and again throughout this discussion.
But for some reason maintainers decided to make it a keyword and there were almost no pushback (see #67777 (comment)). There were still a discussion about whether it needs to be an annotation or a keyword and even that maybe it should be called What felt especially discouraging was the general tone toward users. At times it felt like we were going in circles, as if no matter what arguments were presented the outcome had already been decided. In the end it just seems pointless. |
Well yes, as you point it out the maintainers had 3 years of discussion to come to a consensus. That consensus evolved over time and it finally reached a point which every maintainer is satisfied with. Nobody is saying this is something objective. We discussed the subjective arguments in this very thread and GDScript maintainers presented their vision (which is consensual among other maintainers) for why this feature is better represented by an annotation than a keyword. You can see it in @dalexeev's comment here: #107717 (comment) What should be an annotation versus a keyword has been a source of internal discussion since annotations were introduced to Godot. There wasn't a clear design for it at the time, mainly the shared consensus that GDScript had too many keywords and that many of those would be better served by the concept of annotations. That change happened in Godot 4.0 (you can see that many of the annotations in Godot 4 were keywords in Godot 3). After years of bikeshedding, the GDScript maintainers have come to a consensus for what annotations should be used for, and that's the one dalexeev described like this:
That's it. It's a subjective consensus but we feel it's the best definition we have for what annotations should be, and to put an end to the endless bikeshedding about annotations versus keywords. We considered the feedback given here, discussed it, and that didn't change that resolution, so we proceed with the change. Now we are going in circles with this discussion, I agree. You need to understand that we're in the development phase for Godot 4.5, and anything that was merged so far for this milestone is still subject to change during the beta phase. It's totally normal for maintainers to change their mind about some implementation detail and correct it before a feature gets released in a stable release (after which it can't be changed anymore without breaking compatibility). We could also outright revert the support for abstract classes if its syntax is such an issue, but I doubt the 100+ upvotes and dozens of ecstatic commenters on social media would see it as a positive change either. At the end of the day, Godot being community-driven doesn't mean that it's a tyranny of the majority (and here it's not even a majority pushing back on this change but a handful of users). The community provides feedback, bug reports, contributions, code reviews, etc. and helps define the direction of the project, e.g. by supporting the proposal to have abstract classes in GDScript. The actual decision on how do we implement features in a way that's maintainable for us in the long time belongs to maintainers. They're the one who take on the responsibility to make Godot what it is, and with great responsibility comes great power something something. So the decisions of maintainers are naturally weighed more heavily than that of commenters who have "less skin in the game", as maintainers are the ones who have to deal with the maintenance burden of anything that's added to the engine. I think everything has been said here and we don't intend to continue painting that shed, so I'll lock the thread. |
This was requested by @vnen and discussed in the GDScript meeting on 2025-06-17. Meeting notes:
This effectively restores the code to an earlier version of PR #67777, except that it includes abstract functions as added by PR #106409.
Why?
Disclaimer: I am in favor of this PR, so this list is probably biased.
Reasons For
@abstract
is a modifier that modifies aclass
,class_name
, orfunc
. It can't be used by itself.@override
and@virtual
which go well with@abstract
.Reasons Against
abstract
from other languages, thenabstract
would be less friction. (possibly? only slightly?)static
.static
is planned to be replaced with@static
. Since removingstatic
would break compatibility, we might add@static
before removingstatic
so both work for awhile.master
branch and already usingabstract
.Neutral
: pass
except that abstract functions explicitly cannot have a body and so: pass
is forbidden there and non-abstract functions must have a body.Godot isn't a democracy, but you can react with 👍 and 👎 to show your support for or against this change.