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

GDScript: Properly validate return type #47569

Merged
merged 1 commit into from
Apr 5, 2021

Conversation

vnen
Copy link
Member

@vnen vnen commented Apr 2, 2021

When the type cannot be validated at compile time, the runtime must do a check to ensure type safety is kept, as the code might be assuming the return type is correct in another place, leading to crashes if the contract is broken.

@vnen vnen added this to the 4.0 milestone Apr 2, 2021
@vnen vnen requested a review from a team as a code owner April 2, 2021 13:43
@vnen vnen force-pushed the gdscript-typed-return branch 2 times, most recently from ea54338 to 933ba3b Compare April 2, 2021 14:19
@qarmin
Copy link
Contributor

qarmin commented Apr 2, 2021

When running https://github.com/qarmin/RegressionTestProject/archive/refs/heads/4.0.zip I got this errors:

SCRIPT ERROR: Trying to return a previously freed instance.
          at: get_object (res://AutomaticBugs/ValueCreator.gd:289)

In master I don't have similar

When the type cannot be validated at compile time, the runtime must do a
check to ensure type safety is kept, as the code might be assuming the
return type is correct in another place, leading to crashes if the
contract is broken.
@vnen
Copy link
Member Author

vnen commented Apr 5, 2021

@qarmin thanks, I've mistakenly inverted the logic in the get_validated_object_with_check() call (the boolean flag is for "freed" but I wrongly assumed it was for "valid").

@akien-mga akien-mga merged commit 5b2c4ad into godotengine:master Apr 5, 2021
@akien-mga
Copy link
Member

Thanks!

@vnen vnen deleted the gdscript-typed-return branch May 6, 2021 15:34
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.

3 participants