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] Fix type mismatch in optimized single arg range #68125

Conversation

kleonc
Copy link
Member

@kleonc kleonc commented Nov 1, 2022

Fixes #68093.

When range is used in a for loop and when all of its 1/2/3 arguments are constants (int or float each) then it's being optimized to generate a loop directly without generating an actual array. In such case list->reduced_value of the relevant GDScriptParser::ForNode should be set to int/Vector2i/Vector3i according to the argument count. The problem was in case of a single argument the argument was not casted to int, meaning in case the original argument was a float the reduced_value would end up being a float as well.

This PR solves it by adding the missing cast. I'm casting to int32_t to make it consistent with 2/3 arguments cases as Vector2i/Vector3i used in there are capable of storing only 32-bit signed integers. Making the range work properly with 64-bit integers (including the optimized for case) is for another PR (I've already done it partially in #67025, without the optimized for part).

Note that if range is optimized then for's variable is already annotated to be an int (including the bugged single argument case):

if (list_resolved) {
variable_type.type_source = GDScriptParser::DataType::ANNOTATED_INFERRED;
variable_type.kind = GDScriptParser::DataType::BUILTIN;
variable_type.builtin_type = Variant::INT; // Can this ever be a float or something else?
p_for->variable->set_datatype(variable_type);

so when the actual assigment during looping is happening the conversion should happen even from a float. Meaning the inconsistency fixed in this PR was not the solely reason of the bug in #68093. But the no-conversion bug is likely to be the cause of e.g. #62650 which might be fixed by #62674/#62688 so it's not like it will be left unfixed.

@kleonc kleonc added this to the 4.0 milestone Nov 1, 2022
@kleonc kleonc requested a review from a team as a code owner November 1, 2022 16:38
@akien-mga
Copy link
Member

Can we add some test cases to validate this? (that would have caught the bug)

@kleonc kleonc force-pushed the range-fix-single-arg-optimized-type-mismatch branch from 073ecaa to a48ca04 Compare November 2, 2022 12:16
@kleonc
Copy link
Member Author

kleonc commented Nov 2, 2022

Can we add some test cases to validate this? (that would have caught the bug)

Added tests ensuring range produces only integers. Not sure if I categorized them properly but I think it's runtime.

@kleonc kleonc force-pushed the range-fix-single-arg-optimized-type-mismatch branch from a48ca04 to c268e3a Compare November 2, 2022 12:21
@akien-mga akien-mga merged commit 7eb44fa into godotengine:master Nov 2, 2022
@akien-mga
Copy link
Member

Thanks!

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.

range value over any builtin type property is broken
2 participants