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.

Sign up for GitHub

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

Jump to bottom

Implement abstract methods in GDScript #82987

Open

ryanabx wants to merge 2 commits into godotengine:master from ryanabx-contrib:features/abstract-methods

+163 −21

Contributor

ryanabx commented Oct 8, 2023 •

edited by adamscott

Loading

Depends on #67777 .

This PR allows the @abstract annotation defined in the PR above to be used on methods. Abstract methods have the following properties:

They must exist inside an abstract class:
They cannot have an implementation
They MUST be implemented by subclasses

Please test this and let me know your thoughts!

Below is a simple example project to use in testing:

test_project_abstract_methods.zip

Bugsquad edit: Closes Add GDScript support for defining virtual/prototype/must-override methods in base classes godot-proposals#1631.
Production edit: closes godotengine/godot-roadmap#66

ryanabx requested review from a team as code owners

October 8, 2023 06:25

dalexeev added enhancement topic:gdscript labels

dalexeev added this to the 4.x milestone

dalexeev requested changes

View reviewed changes

Member

dalexeev left a comment

We also need to check that super() and super.method() do not refer to an abstract method.

modules/gdscript/gdscript_analyzer.cpp Outdated Show resolved Hide resolved

modules/gdscript/gdscript_parser.cpp Show resolved Hide resolved

modules/gdscript/gdscript_analyzer.cpp Outdated Show resolved Hide resolved

modules/gdscript/gdscript_analyzer.cpp

    
              	if (!p_function->get_datatype().is_hard_type() && p_function->body->get_datatype().is_set()) {

              		// Use the suite inferred type if return isn't explicitly set.

              		p_function->set_datatype(p_function->body->get_datatype());

              	} else if (p_function->get_datatype().is_hard_type() && (p_function->get_datatype().kind != GDScriptParser::DataType::BUILTIN || p_function->get_datatype().builtin_type != Variant::NIL)) {

              		if (!p_function->body->has_return && (p_is_lambda || p_function->identifier->name != GDScriptLanguage::get_singleton()->strings._init)) {

              		if (!p_function->is_abstract && !p_function->body->has_return && (p_is_lambda || p_function->identifier->name != GDScriptLanguage::get_singleton()->strings._init)) {

Member

dalexeev Oct 8, 2023

We need to check if there is code that relies on has_return and if this could lead to negative effects. I remember exactly that this is used in the compiler and in DocGen. Alternatively, we could add a fake return for abstract non-void functions, but this is an undesirable hack.

Contributor Author

ryanabx Oct 8, 2023

How would one check for this? I'm still learning about the GDScript codebase :p

modules/gdscript/gdscript_parser.cpp Outdated Show resolved Hide resolved

modules/gdscript/gdscript_analyzer.cpp Outdated Show resolved Hide resolved

Member

dalexeev commented Oct 8, 2023

It should also be prohibited to mark constructor (_init()) and several multi-level methods (_notification(), _get_property_list(), ...) as abstract.

ryanabx force-pushed the features/abstract-methods branch 4 times, most recently from 7c06997 to 4d2d2e8 Compare

October 9, 2023 01:20

Contributor Author

ryanabx commented Oct 9, 2023

I resolved most of the changes from you @dalexeev . I will need some help with the remaining steps you mentioned:

Prohibiting @abstract annotation on _init, and multi-level function calls _notification(), _get_property_list(), etc.)
Checking if there is code that relies on has_return, per your comment
Making sure that super() and super().method don't refer to an abstract method

dalexeev reviewed

View reviewed changes

modules/gdscript/gdscript_analyzer.cpp Outdated Show resolved Hide resolved

Chubercik reviewed

View reviewed changes

modules/gdscript/gdscript_analyzer.cpp Outdated Show resolved Hide resolved

modules/gdscript/gdscript_analyzer.cpp Outdated Show resolved Hide resolved

ryanabx force-pushed the features/abstract-methods branch 4 times, most recently from 192e0fd to bd03c7a Compare

October 9, 2023 22:02

This was referenced Oct 16, 2023

Syntax highlighting changes godotengine/godot-vscode-plugin#515

Merged

Allow explicitly defining interfaces in GDScript godotengine/godot-proposals#4872

Open

aaronfranke and others added 2 commits

October 18, 2023 14:14


          Add an annotation for abstract classes in GDScript

4dfafe0

Co-authored-by: Danil Alexeev <[email protected]>


          Implement abstract methods in GDScript

c380cc2

ryanabx force-pushed the features/abstract-methods branch from bd03c7a to c380cc2 Compare

October 18, 2023 19:14

InkONat commented Feb 19, 2024

It's been a while since anything was said about this PR...

Contributor Author

ryanabx commented Feb 19, 2024

It's been a while since anything was said about this PR...

This PR depends on #67777

Once it's merged, I will probably come back to it and update the PR to accommodate any changes made since my last push.

dalexeev mentioned this pull request

Add a keyword for abstract classes in GDScript #67777

Open

dalexeev mentioned this pull request

Unused parameters show underscore prefix in documentation #89688

Open

allenwp mentioned this pull request

Improve description of annotations godotengine/godot-docs#9188

Merged

leandro-benedet-garcia commented May 26, 2024 •

edited

Loading

I think since the required pr uses keywords to declare abstract classes, you should also use keywords to declare abstract functions

Member

AThousandShips commented May 26, 2024 •

edited

Loading

This was not originally the decision, so this simply follows the original design, for more details see #67777 (comment), but see also #82987 (comment), this is waiting on another PR

leandro-benedet-garcia commented May 26, 2024

This was not originally the decision, so this simply follows the original design, for more details see #67777 (comment)

Yes, so shouldn't this PR also be changed?

Contributor Author

ryanabx commented May 26, 2024

This was not originally the decision, so this simply follows the original design, for more details see #67777 (comment)

Yes, so shouldn't this PR also be changed?

Yes, but I will work on it once the abstract classes PR gets merged 😅

Member

dalexeev commented May 26, 2024

Yes, so shouldn't this PR also be changed?

It should, but it depends on the desire and availability of the author; we cannot require volunteers to work. It also makes sense to wait until the base PR is at least reviewed and approved, so as not to ask the author of this PR to do extra work.

leandro-benedet-garcia commented May 26, 2024

Yes, so shouldn't this PR also be changed?

It should, but it depends on the desire and availability of the author; we cannot require volunteers to work. It also makes sense to wait until the base PR is at least reviewed and approved, so as not to ask the author of this PR to do extra work.

Ah, sorry for my phrasing, I was just asking if it should, not that it needs to.

dalexeev mentioned this pull request

Add Trait System to GDScript #97657

Open

12 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement topic:gdscript