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

Virtual functions with "pass" keyword and optional returns #2656

Closed
oysuta opened this issue Apr 26, 2021 · 10 comments
Closed

Virtual functions with "pass" keyword and optional returns #2656

oysuta opened this issue Apr 26, 2021 · 10 comments

Comments

@oysuta
Copy link

oysuta commented Apr 26, 2021

Describe the project you are working on

I'm making a visual novel framework for Godot that involves virtual functions.

Describe the problem or limitation you are having in your project

Creating a virtual funciton is possible, but annoying and not as clear as it can be. We don't even need to add anything to GDScript. Just change a bit about how it works. (To be honest, I'm not sure if this is actually a missing feature or just a bug, but I thought it would be easier to discuss as a feature proposal, so here we are.)

The sensible thing to type something like this:

func execute(command : Dictionary) -> GDScriptFunctionState:
    pass

But that produces the following error:

A non-void function must return a value in all possible paths.

There's two options to get rid of the error:

  • I can return garbage and include a comment about there being garbage in the return value, OR
  • I can get rid of the optional return value. But I think that's backwards. If there's one place where it makes sense to have an explicit return value, it's with a virtual function to give people more information about how and why it's supposed to be used.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Allow functions with a "pass" body to have a defined return value without throwing an error.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

See above.

If this enhancement will not be used often, can it be worked around with a few lines of script?

The workaround is to write code that's less clear just to avoid an error that arguably shouldn't appear. (Again, this might be a bug? Dunno)

Is there a reason why this should be core and not an add-on in the asset library?

This can't be done as an add-on since it's about GDScript

@Calinou
Copy link
Member

Calinou commented Apr 27, 2021

cc @vnen

@YuriSizov
Copy link
Contributor

Problem here is that pass is not a special keyword for empty functions. It's just a non-operational line that can be put in any place of your functions. So it would be weird to have it do something special here.

@PranavSK
Copy link

The type system in GDScript is optional and this means that it exists to make writing code a bit easier and to avoid bloated code checks and eliminate some errors at the type of coding. When you put a return type on a function it represents that it must have a return type and that you are not checking for its validity when using it. If you want the given function to be something to implemented in a child class one option is to raise an error if it is not done. eg:

func test() -> int: # in parent
    assert(false, "This function needs to implemented in the child class.")
    return 0

@oysuta
Copy link
Author

oysuta commented Apr 27, 2021

I didn't mean to close this, how did it even? anyway...

@pycbouh good point, but my understanding of "pass" is that the whole reason it exists is that you can provide it in place of something GDScript syntactically requires, so I don't think it's too much of a stretch at all to expect this to work

@PranavSK thanks for the info, this is certainly the way to do it as GDScript works right now. And while you listed some great technical reasons to use optional return types, another reason to use them is to support a certain coding style: making it more explicit what the function is supposed to do, reducing the need for comments and documentation, etc

@oysuta oysuta reopened this Apr 27, 2021
@YuriSizov
Copy link
Contributor

good point, but my understanding of "pass" is that the whole reason it exists is that you can provide it in place of something GDScript syntactically requires, so I don't think it's too much of a stretch at all to expect this to work

It's basically a way to create an empty block in an indent-based language, yes. What I mean is that it doesn't actually relay any special behavior to the parent scope, so syntactically you are fully expected to provide the rest of your method to make it valid.

I think the assert way is how developers should handle cases like yours. You should be able to return null in place of your object. And if you can't, it falls under other proposals such as #162 and #2241.

@KoBeWi
Copy link
Member

KoBeWi commented Apr 27, 2021

Somewhat related to #393

We could have @virtual annotation and methods marked as such could allow having only pass even if they should return something. You wouldn't be able to call such method if it's not overriden.

@YuriSizov
Copy link
Contributor

YuriSizov commented Apr 27, 2021

We could have @virtual annotation and methods marked as such could allow having only pass even if they should return something. You wouldn't be able to call such method if it's not overriden.

To that end see #1631.

@vnen
Copy link
Member

vnen commented Apr 27, 2021

To that end see #1631.

I believe we can close this one as a duplicate of that one.

There's also a distinction between "virtual" (it can be overridden) and "pure virtual" (it must be overridden). I don't intend to add an implicit pure virtual syntax like asked here because I can only see it leading to confusion, given that someone might do it by accident without noticing. If it is pure virtual, it doesn't even need a body.

A return type is a contract, so you always have to return that type, otherwise you break expectations of the callers. This contract is used for optimizations as well, so a wrong type could even lead to a crash.

@oysuta
Copy link
Author

oysuta commented Apr 28, 2021

Thank you, everyone!

The reason I posted this is because I thought it would be a good solution that would help with the engine's goal of keeping GDScript simple. But if it's a bad idea, then so be it; lots of other great ideas here!

For now I believe I'll use assertions as @pycbouh suggests, but I don't know if it's a perfect solution because in Godot assertions only work on debug builds and/or in the editor. That's not exactly an ironclad guarantee, if say I was creating a framework or add-on to share that many people would be using. People use Godot in all kinds of interesting and unexpected ways.

Also special thanks to @pycbouh for explaining what pass actually does, I understand it a lot better now than from the documentation and/or examples I've been able to find.

@oysuta oysuta closed this as completed Apr 28, 2021
@Calinou
Copy link
Member

Calinou commented Apr 28, 2021

For now I believe I'll use assertions as @pycbouh suggests, but I don't know if it's a perfect solution because in Godot assertions only work on debug builds and/or in the editor. That's not exactly an ironclad guarantee, if say I was creating a framework or add-on to share that many people would be using. People use Godot in all kinds of interesting and unexpected ways.

Even if you distribute an add-on, people will be using it in debug builds while they work on their project, so it should be fine. See also #502.

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

No branches or pull requests

6 participants