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

Generate errors for getters that return no value #79363

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

garychia
Copy link
Contributor

Replaced #63541
Fixes #63393
Getters now must return a value.

@garychia garychia requested a review from a team as a code owner July 12, 2023 08:09
@dalexeev dalexeev added this to the 4.2 milestone Jul 12, 2023
Copy link
Member

@dalexeev dalexeev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds validation only for inline getters (PROP_INLINE). Perhaps we should do this for separated getters (PROP_SETGET) as well?

modules/gdscript/gdscript_analyzer.cpp Outdated Show resolved Hide resolved
@garychia garychia marked this pull request as draft July 13, 2023 09:57
@garychia garychia marked this pull request as ready for review July 13, 2023 12:20
Copy link
Member

@dalexeev dalexeev left a 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, except for a minor note.

Also I noticed that empty return doesn't raise an error/warning:

var property:
    get:
        return

But this is beyond the scope of this PR, as it happens here too:

func my_func() -> Variant:
    return

In my opinion empty return should only be used with void functions or if no return type is specified. In a type is specified or it is getter, return must be non-empty (explicit return null).

modules/gdscript/gdscript_analyzer.cpp Outdated Show resolved Hide resolved
@adamscott
Copy link
Member

Discussed (a lot) during the GDScript meeting. Currently, we would wait a bit before merging this PR to make sure that a future PR (@dalexeev ?) in that vein (the following citation) would still be compatible with/not make obsolete this PR.

But this is beyond the scope of this PR, as it happens here too:

func my_func() -> Variant:
   return

In my opinion empty return should only be used with void functions or if no return type is specified. In a type is specified or it is getter, return must be non-empty (explicit return null).

There's no rush to merge this PR yet, but thank you @garychia for that PR, we will follow with something soon.

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

Successfully merging this pull request may close these issues.

GDScript 2.0: getter of untyped variables can return nothing on some code paths
4 participants