Make memnew(RefCounted) return Ref, to improve ownership safety

[Ivorforce]

This PR improves RefCounted ownership semantics for newly created objects, preventing potential crashes and other footguns at build time.

Background

When memnew initializes RefCounted subclasses, they start with an (effective) allocation count of 0.

This creates two ownership related problems:

• When stored as RefCounted * initially, some code may increase and later decrease the refcount (e.g. when passing as Variant). This destructs the object, because the refcount is decreased to 0. The actual owner's RefCounted * will be a dangling pointer. The owner should have stored Ref<RefCounted> instead.
• When in the postinitialize_handler / NOTIFICATION_POSTINITIALIZE, RefCounted objects still have an allocation count of 0, because the caller of memnew didn't claim a reference yet. If the allocation count is increased and later decreased during this function, the object will unexpectedly destruct (see RefCounted releases itself if it is referenced in NOTIFICATION_POSTINITIALIZE #108395).

The ideal (only?) way to fix this is to start RefCounted with a refcount of 1. The memnew caller must take ownership of the refcount after the call. This just means we return Ref from memnew. Most callers already store the result in a Ref, so they are compatible with this change.

Overview

Implementing the change was fairly straight-forward; I add and specialize memnew_result_t such that for RefCounted types, memnew returns Ref<T>.

The rest of the changes are fixing current ownership problems. Mostly, it just involves changing T * to Ref<T>. It has some inherent risk, but I don't think it's huge.

[KoBeWi]

Generally the preferred style is

Suggested change

	Ref<ColorPalette> palette = memnew(ColorPalette);
	Ref<ColorPalette> palette;
	palette.instantiate();

Some code just wrongly uses RefCounted.

[Ivorforce]

Why is .instantiate() better than memnew?

[KoBeWi]

Well, before this PR, it was the only way to properly create RefCounted, other than Ref<Something>(memnew(Something)) (which is more verbose and unnecessary). But with this PR, there won't be difference I guess.

[Ivorforce]

Makes sense.
In the context of this change, it would also be possible to disallow memnew for RefCounted types (so callers have to use .instatiate() instead). I suppose now would be a good time to figure out which style we prefer going forward.

[KoBeWi]

This is unnecessary.

Suggested change

	Ref<MissingResource> missing_resource = nullptr;
	Ref<MissingResource> missing_resource;

[AThousandShips]

There would probably need to be some way to create these without explicit ownership as there are times when we need to not do refcounting for safety, also in some low-level construction I'd say, though usually the cases where we want to avoid refcounting are when we already have a reference and we avoid referencing it again

[KoBeWi]

To me it looks like the problem is that some people used RefCounted wrong. You should either not use it with memnew(), or wrap that in Ref (AFAIK Ref<Type>(memnew(Type)) ensures proper ownership). Using instantiate() is more or less established in the codebase, so this PR only adds another available syntax for creating RefCounted objects. Instead of doing that, we could prevent the wrong usage.

[Ivorforce]

There would probably need to be some way to create these without explicit ownership as there are times when we need to not do refcounting for safety, also in some low-level construction I'd say, though usually the cases where we want to avoid refcounting are when we already have a reference and we avoid referencing it again

Could you provide an example? Usually the only reason to avoid ref counting is performance, and you can still pass RefCounted objects around by reference.

To me it looks like the problem is that some people used RefCounted wrong.

This is part of the problem, but just using .instantiate() wouldn't solve the second problem from the OP.

You should either not use it with memnew(), or wrap that in Ref (AFAIK Ref(memnew(Type)) ensures proper ownership). Using instantiate() is more or less established in the codebase, so this PR only adds another available syntax for creating RefCounted objects.

This PR basically enforces the Ref<Type>(memnew(Type)) style for callers with memnew. It doesn't introduce new syntax.

Instead of doing that, we could prevent the wrong usage.

Yea, disallowing memnew for all RefCounted objects could be done as well, in the context of this PR. It's just a matter of stylistic preference whether we want memnew for all objects or just non-refcounted ones.

[AThousandShips]

Could you provide an example? Usually the only reason to avoid ref counting is performance, and you can still pass RefCounted objects around by reference.

I can't remember where it was currently, but and I think that was related to creating Ref where there already was ownership, but it had to do with dangling copies and extending ownership in some contexts, but it might not be directly relevant, but might be cases where the choice to use a raw pointer is not an error

This PR basically enforces the Ref<Type>(memnew(Type)) style for callers with memnew.

But that's establishing a different style than what is established

[KoBeWi]

This PR basically enforces the Ref(memnew(Type)) style for callers with memnew. It doesn't introduce new syntax.

I meant "new syntax" for RefCounted objects. After this PR you can do Ref<Type> var = memnew(Type), which wasn't possible before.

This is part of the problem, but just using .instantiate() wouldn't solve the second problem from the OP.

Right, so I guess that would justify this change.

[AThousandShips]

After this PR you can do Ref<Type> var = memnew(Type), which wasn't possible before.

That is fully code wise possible currently? You don't have to do Ref<Type> var = Ref<Type>(memnew(Type))

[Ivorforce]

I can't remember where it was currently, but and I think that was related to creating Ref where there already was ownership, but it had to do with dangling copies and extending ownership in some contexts, but it might not be directly relevant, but might be cases where the choice to use a raw pointer is not an error

I think using any refcounted object without a primary owner is an error. Passing it around via pointer is fine, but owning one without it is always unsafe, because anyone you pass it to can destroy your object (even by accident).

This PR basically enforces the Ref<Type>(memnew(Type)) style for callers with memnew.

But that's establishing a different style than what is established

That's true. I have some problems with .instantiate(), but not enough to warrant churning the whole codebase to change it. It's a good argument for going with the "forbid" route instead.

This PR basically enforces the Ref(memnew(Type)) style for callers with memnew. It doesn't introduce new syntax.

I meant "new syntax" for RefCounted objects. After this PR you can do Ref<Type> var = memnew(Type), which wasn't possible before.

This is already possible in master. Ref has an implicit conversion from T *.

[AThousandShips]

One potential issue I would see is that this could hide incorrect code, it should be the case that:

Ref<T1> t1 = memnew(T2);

Fails when T2 is not derived from T1, but with this change it would be a silent empty Ref, though I haven't tested if the former actually does fail but it should

Though ultimately we don't want Ref<T> t = memnew(T); except where it's actually necessary, like when constructing a derived type, though those would usually be cases where it's not a declaration but later, like:

Ref<Texture2D> tex;
...
tex = memnew(ImageTexture(...));

[Ivorforce]

One potential issue I would see is that this could hide incorrect code, it should be the case that:

Ref<T1> t1 = memnew(T2);

Fails when T2 is not derived from T1, but with this change it would be a silent empty Ref, though I haven't tested if the former actually does fail but it should

That's a good point. This was also previously a problem (when using the memnew syntax). But it can be easily resolved to fail at compile time. I'll make a note to add a pull request for that.
Edit: #111967

[AThousandShips]

It shouldn't have been a problem as it should fail to compiletime right now, I can't test it right now but it should as T2 * can't be cast to T1 * unless T2 is T1 or a derived type

So this would be a change in this PR, so I'd say a check for this at compile time should be in this PR

I'd test just with:

Ref<Texture3D> r = memnew(ImageTexture2D(...));

Edit: There isn't really any way I can see that this can be guarded against, not sure how we can resolve that at compile time, as it's just Ref<T> t = Ref<T2>(memnew(T2)); so the problem isn't solvable as far as I can tell

To clarify the problem:

This fails (and should fail to make it clear to the user they're doing something wrong):

Ref<Texture2D> t = memnew(Texture3D);

But this doesn't (as conversion is fully permitted between Ref):

Ref<Texture3D> t3 = memnew(Texture3D);
Ref<Texture2D> t = t3;

The result will be that t is empty though

With this change the first case will not fail at compile time, it will just yield an empty result, making this very hard to diagnose if you, for example, accidentally picked the wrong type when doing something

[dsnopek]

If we want to continue to allow code like this (from ATS' comment above):

Ref<Texture2D> tex;
...
tex = memnew(ImageTexture(...));

Then I think we need the changes in this PR. Because without this PR, in code like that it's still possible for a class to accidentally delete itself during NOTIFICATION_POSTINITIALIZE, which is what this is aiming to fix.

Maybe some of the push back here can be addressed by changing the memnew() conversions that @Ivorforce did into .instantiate() where possible?