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

Revert "Make freed object different than null in comparison operators" #93809

Merged
merged 1 commit into from
Jul 1, 2024

Conversation

akien-mga
Copy link
Member

@akien-mga akien-mga commented Jul 1, 2024

This reverts commit 150b50c.

As discussed with the GDScript team, this has some implications which aren't fully consensual yet, and which we want to revisit.

For now we revert to the 4.2 behavior for the 4.3 release, to avoid breaking user expectations.

This reverts commit 150b50c.

As discussed with the GDScript team, this has some implications which aren't
fully consensual yet, and which we want to revisit.

For now we revert to the 4.2 behavior for the 4.3 release, to avoid breaking
user expectations.
@akien-mga akien-mga added this to the 4.3 milestone Jul 1, 2024
@akien-mga akien-mga requested a review from a team as a code owner July 1, 2024 12:14
@akien-mga akien-mga requested a review from a team July 1, 2024 12:15
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.

It's sad that we reintroduce the bug that if freed and if not freed are both true, but I think it's better than introducing freed != null since we haven't reached a consensus on it.

If we can find a safe way to fix the first bug without changing the second, that would be great. But until then, reverting the change looks reasonable to me.

@akien-mga akien-mga merged commit 8d76f0e into godotengine:master Jul 1, 2024
18 checks passed
@akien-mga akien-mga deleted the revert-73896 branch July 1, 2024 16:33
@Macksaur
Copy link
Contributor

Macksaur commented Jul 1, 2024

A shame. I don't understand the consensus? are you trying to obfuscate non-owning references to Objects? imo that would be a bad choice. It should always remain possible to discern whether or not a variable has been set with a value (or not) distinct from whether or not said value is special (is_instance_valid).

Can you document more of this issue publicly?

@adamscott
Copy link
Member

adamscott commented Jul 2, 2024

It should always remain possible to discern whether or not a variable has been set with a value (or not) distinct from whether or not said value is special (is_instance_valid).

You can always use is_same() together with null.

@Macksaur
Copy link
Contributor

Macksaur commented Jul 2, 2024

Ohh, does that work like an is operator? That would likely placate my future needs to avoid further changes here. However, it does feels strange that the 3.5 behaviour is not replicated properly by 4.

@DinoMC
Copy link

DinoMC commented Jul 3, 2024

Here's my take on it, hopefully it can help.

I just upgraded my project from 4.2 to 4.3 beta 2 (so, a version with #73896) and it was crashing because I use "!= null" and then ".has_method('queue_free')" to check if I can free a node, but now != null returned false on previously freed objects.

Problem is, while I was trying to debug this, I set a breakpoint at the start of my function. And in the editor, while I'm paused just before the first check, I look at the stack and I see that the value of target is indeed <null>. But then if I Step Over, the check is true, and it goes to the second check.
The value does NOT show as "<Freed Object>" in the stack.

This is absolutely not my area of expertise and I'm confused by the difference between null and freed, so the following might be wrong. But to me, it seems like the change was badly implemented. I don't think it makes sense for the editor to show the value of the variable as "null", while at the same time a check of "target != null" returns true.

If this is reintroduced, I think it would need to appear correctly in the debugger.

@dalexeev
Copy link
Member

dalexeev commented Jul 3, 2024

@DinoMC #73896 has been reverted. In beta 3 the behavior will be the same as in 4.2. However, please note that is_instance_valid() is the safest and most portable option across different engine versions (including 3.x).

I look at the stack and I see that the value of target is indeed .

See also:

@DinoMC
Copy link

DinoMC commented Jul 3, 2024

Yes I know about the reverting, but it seems there's some discussions about maybe reintroducing it later in a better way so I wanted to chime in.
But my exact point was indeed already described in #92632 , didn't find that one, my bad.
In my project I have switched to is_instance_valid() and everything works well now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Release Blocker
Development

Successfully merging this pull request may close these issues.

Freed nodes are no longer evaluating as truthy (as in they have a value)
5 participants