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 constant conversions #71844

Merged
merged 1 commit into from Jan 29, 2023

Conversation

vonagam
Copy link
Contributor

@vonagam vonagam commented Jan 22, 2023

A constant value can be converted to a specified type during analysis, not at runtime every time.

var x: StringName = 'a' # initializer

x = 'b' # assignment

func print_name(y: StringName): print(y)
print_name('c') # call argument

func make_name() -> StringName: return 'd' # return

var z := 'c' as StringName # cast

Out of those places only variable initializer was handled at compile time before.

Initializer for constants was not converted at all which was leading to many crash reports.

While at it, allowed a cast to be considered as constant expression under certain circumstances (which will be expanded with array PR from which code for this PR partially comes from).

Fixed minor things with casts and arguments validations.

Fixes #70861.
Fixes #71255.
Fixes #71483.
Fixes #71635.
Fixes #72209.

Related to #70872 (supersedes).
Related to #70873 (supersedes).

@vonagam vonagam requested a review from a team as a code owner January 22, 2023 08:50
@Chaosus Chaosus added this to the 4.0 milestone Jan 22, 2023
@dalexeev
Copy link
Member

dalexeev commented Jan 22, 2023

This PR does not fully supersede #70873, as there are more changes to resolve_assignable(), not just the part that can be replaced by update_const_expression_builtin_type() call. (I will rebase #70873 after this PR is merged.)

@vonagam
Copy link
Contributor Author

vonagam commented Jan 22, 2023

I looked now and assume that the thing that wasn't included here was a correction for wrong check of compatibility when it comes to asymmetric conversions - String can be converted to Color, but other way around will never work.

It can be fixed by disallowing implicit conversions in reverse check (reverse check is done only to allow cases where variable of class type is used where subclass is expected, since those are objects no implicit conversions should take place). Updated the PR and fixed the same mistake in usual assignments too.

Is there anything else that is missing?

@dalexeev
Copy link
Member

Is there anything else that is missing?

  1. Some tests are missing.
  2. I think in my PR the code has been reorganized to be more readable: first the status is determined (SAFE, UNSAFE, ERROR) and then actions are performed depending on this. This can help avoid mistakes in the future.

@vonagam vonagam force-pushed the fix-constant-conversions branch 4 times, most recently from 18df701 to 9710bc0 Compare January 24, 2023 00:06
@vonagam vonagam requested a review from a team as a code owner January 24, 2023 00:06
@akien-mga
Copy link
Member

Needs rebase.

@akien-mga
Copy link
Member

Needs another rebase to pass CI, I fixed the GDScript test error.

@akien-mga akien-mga merged commit 4011a09 into godotengine:master Jan 29, 2023
@akien-mga
Copy link
Member

Thanks!

@vonagam vonagam deleted the fix-constant-conversions branch January 29, 2023 04:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment