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

Core: Fix Freed Object booleanization #93885

Merged

Conversation

dalexeev
Copy link
Member

@dalexeev dalexeev commented Jul 3, 2024

  • This PR only extracts the freed objects booleanization fix from Make freed object different than null in comparison operators (reverted) #73896. The operator == behavior remains unchanged.
  • This is "Option 2. Intuitive and Beginner Friendly" from Make `null` vs `Freed Object` behavior more consistent godot-proposals#10098.
  • Also the discussion lists other comparison styles. If we go with this option, it probably makes sense to replace cases "3. By Variant.Type" with "1. Intuitive".
  • I assigned milestone 4.4 because there is no consensus on whether null == freed or null != freed. If we choose the first, then falsy freed object makes sense. If the second, then truthy freed object makes more sense. It's probably too risky to make such changes for 4.3 at this point.
  • For now, this PR can be considered as not breaking compatibility because it only fixes already broken behavior. But if we want other changes, it will be breaking.
str(value)                    <null>  <Object#null>  <Freed Object>
var_to_str(value)             null    null           null

true if value else false      false   false          true -> false
true if not value else false  true    true           true
is_instance_valid(value)      false   false          false

value == null                 true    true           true
value == object_null          true    true           true
value == freed_object         true    true           true (same)
                                                     true (other)

is_same(value, null)          true    false          false
is_same(value, object_null)   false   true           false
is_same(value, freed_object)  false   false          true (same)
                                                     false (other)

typeof(value) == TYPE_NIL     true    false          false
typeof(value) == TYPE_OBJECT  false   true           true
value is Object               false   false          Error

Test project: null-vs-freed.zip

@dalexeev dalexeev added this to the 4.4 milestone Jul 3, 2024
@dalexeev dalexeev requested a review from a team July 3, 2024 07:14
@dalexeev dalexeev requested a review from a team as a code owner July 3, 2024 07:14
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Preemptively approving of this because this is actually how most people assume it works, on Discord. Even not-so-savvy users can agree this makes sense.

Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

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

What a one-liner.

@Mickeon
Copy link
Contributor

Mickeon commented Sep 4, 2024

Note that #93896 would have to be changed if this is merged beforehand. The boolean note is still important for current/prior versions so I'm a bit baffled it wasn't merged sooner, but I digress.

Copy link
Member

@RandomShaper RandomShaper left a comment

Choose a reason for hiding this comment

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

This fix makes sense in the stated scope of just making the current behavior consistent.

For the records,true if not value else false gives the expected false on a freed object, because that goes through OperatorEvaluatorNotObject, which uses get_validated_object(). This PR fixes the positive condition that doesn't have an operator to evaluate so the GDScript VM just has to booleanize the Variant.

@Repiteo Repiteo merged commit f233d18 into godotengine:master Nov 11, 2024
18 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 11, 2024

Thanks!

@dalexeev dalexeev deleted the core-fix-freed-object-booleanization branch November 11, 2024 21:51
@Macksaur
Copy link
Contributor

Sorry to be a pest but can you include is_same and typeof in the current tables? 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.

6 participants