Skip to content

Make NodePath copy-on-write, to avoid it changing accidentally (through copies)#113204

Open
Ivorforce wants to merge 1 commit into
godotengine:masterfrom
Ivorforce:nodepath-cow
Open

Make NodePath copy-on-write, to avoid it changing accidentally (through copies)#113204
Ivorforce wants to merge 1 commit into
godotengine:masterfrom
Ivorforce:nodepath-cow

Conversation

@Ivorforce
Copy link
Copy Markdown
Member

@Ivorforce Ivorforce commented Nov 26, 2025

This changes NodePath to copy-on-write on mutation, protecting owners of NodePath that assume it's const / pass-by-value (which is likely all owners).
Effectively, this might fix a few obscure NodePath related bugs (explained later), at no cost to reading, and little cost to writing.

Explanation

NodePath has no exposed mutating function. By all accounts, it could be assumed that NodePath is completely const. It is also passed by reference internally (NodePath is a pointer to a payload).

However, NodePath is not fully const, because there are two internal functions (effectively 3, foreshadowing...) that mutate it in-place: simplify() and prepend_period. Coupled with the pass-by-reference semantics, this means that any NodePath you own could change under you without you knowing, just because you accepted (or passed out) a copy of it, which was actually a reference. For example, Node::path_cache could be accidentally mutated, which would leave it with an incorrect path cache, leading to obscure bugs.

simplify() is called in only two places on newly created NodePath instances, so it should be safe. prepend_period() is not called at all. (called once now). This means we're safe, right?
Nope, because simplify() is actually used in the implementation of simplified(). This means that every call to simplified() actually modifies the callee NodePath in-place, even though it's marked as const. The function is called in 7 places in resource_format_text, all of which might be affected by this (rather obscure) bug. On the bright side, the NodePath would at worst only be simplified, not changed, but it still is unexpected.

We discussed the situation in a core meeting, and decided to make NodePath fully const to fix this issue.
However, looking at the implementation, i have decided CoW is actually more appropriate after all. The reason is that this approach was both simpler to implement and more efficient to execute.
Note that const and copy-on-write are the same thing in implementation, except that copy-on-write has mutating methods (that only affect the held instance).

Note

I removed prepend_period because it has no callers. If there's desire to keep it (like for modules?) we could add a _copy_on_write call to it instead and keep it.

@Ivorforce Ivorforce added this to the 4.6 milestone Nov 26, 2025
@Ivorforce Ivorforce requested a review from a team as a code owner November 26, 2025 19:34
@Mickeon
Copy link
Copy Markdown
Member

Mickeon commented Nov 26, 2025

For reference, prepend_period was added in 0988970 which leads me to believe it was just past Juan adding things that became superfluous and outdated over time, without the discussions we have today. I don't know, classic "GODOT IS OPEN SOURCE" moment.

@Ivorforce Ivorforce modified the milestones: 4.6, 4.x Nov 26, 2025
@lawnjelly
Copy link
Copy Markdown
Member

As said in RC my thoughts are, that in a way it would be nicer to make it fully const and make simplify() return a new NodePath so that modification is explicit. There only seem to be very limited cases where COW is needed, so it seems a little overkill and perhaps limiting in the future?

But if that is difficult then COW should work.
It looks fine to me (although I might have missed some threading issue, would be good to test this with sanitizers before going forward).

Copy link
Copy Markdown
Member

@lawnjelly lawnjelly left a comment

Choose a reason for hiding this comment

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

Subject to testing with sanitizers.

Would also be good to get a secondary core look if possible (e.g. @hpvb ) as NodePath is quite central.

@hpvb
Copy link
Copy Markdown
Member

hpvb commented May 6, 2026

This looks good to me, but there does appear to be a caller to prepend_period here: https://github.com/godotengine/godot/blob/master/scene/resources/resource_format_text.cpp#L208

@Ivorforce
Copy link
Copy Markdown
Member Author

Ivorforce commented May 7, 2026

This looks good to me, but there does appear to be a caller to prepend_period here: https://github.com/godotengine/godot/blob/master/scene/resources/resource_format_text.cpp#L208

Funny, that was added after the PR was created. I added it back.

Comment thread core/string/node_path.cpp
Copy link
Copy Markdown
Member

@hpvb hpvb left a comment

Choose a reason for hiding this comment

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

LGTM, seems to fix a footgun!

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