get_slice behavior is inconsistent and error prone when the string contains no matching delimiter

[FeldrinH]

Tested versions

• Reproducible in: 4.4.stable.official, 4.3.stable.official

System information

N/A

Issue description

String.get_slice returns the entire original string when delimiter does not occur in the string. This behavior is inconsistent and can cause a number of subtle and confusing bugs in user code.

I propose that this special case when the delimiter is not found is effectively a bug and should be removed. Instead, get_slice should follow the general rule when the delimiter is not found (i.e. return the entire string if slice == 0, otherwise return the empty string).

Steps to reproduce

Consider the following examples that illustrate issues that this behavior can cause:

A simple function that returns the third element from a comma separated list:

func third_element(values):
    return values.get_slice(",", 2)

This produces the following output:

third_element("0,1,2,3") == "2" # correct
third_element("0,1,2") == "2" # correct
third_element("0,1") == "" # correct
third_element("0") == "0" # incorrect, the first element shows up as the third element

A simple function that extracts and concatenates the first and third element:

func extract_pair(values):
    values.get_slice(",", 0) + "," + values.get_slice(",", 2)

This produces the following output:

extract_pair("0,1,2,3") == "0,2" # correct
extract_pair("0,1,2") == "0,2" # correct
extract_pair("0,1") == "0," # correct
extract_pair("0") == "0,0" # incorrect, the first element unexpectedly appears to be the third element as well

In addition, the current behavior breaks the intuitive expectation that get_slice is equivalent to split followed by getting the respective element or empty string if there is no such element:

func get_slice(value, delimiter, slice):
    var parts = value.split(delimiter)
    return parts[slice] if slice < len(parts) else ""

This produces the following output:

"0,1,2,3".get_slice(",", 2) == get_slice("0,1,2,3", ",", 2) == "2" # matches
"0,1,2".get_slice(",", 2) == get_slice("0,1,2", ",", 2) == "2" # matches
"0,1".get_slice(",", 2) == get_slice("0,1", ",", 2) == "" # matches
"0".get_slice(",", 2) != get_slice("0", ",", 2) # does not match (built-in get_slice returns "0", while the split-based implementation returns "")

Minimal reproduction project (MRP)

N/A

[AThousandShips]

This is intentional, and documented:

Returns the original string if delimiter does not occur in the string.

Changing this would break compatibility, and would be unhelpful, a lot of cases depend on this fact

[FeldrinH]

Changing this would break compatibility, and would be unhelpful, a lot of cases depend on this fact

Do you have an example of code that depends on this? In theory I can believe that there is some code out there relying on the current behavior, but I can't think of an example where the current behavior would be expected or helpful.

[AThousandShips]

Any case where you want for example just foo when checking foo or foo.bar, two examples of this being used in practice are:

godot/editor/project_settings_editor.cpp

Lines 232 to 235 in 9a39760

	int slices = setting.get_slice_count(".");
	const String name = setting.get_slicec('.', 0);
	const String feature = setting.get_slicec('.', 1);
	bool feature_invalid = slices > 2 || (slices == 2 && feature.is_empty());

godot/modules/text_server_adv/text_server_adv.cpp

Lines 518 to 519 in 9a39760

	String l = p_locale.get_slicec('_', 0);
	if ((l == "ar") || (l == "dv") || (l == "he") || (l == "fa") || (l == "ff") || (l == "ku") || (l == "ur")) {

These were ones I found just quickly without further digging, I'm sure there are far more

But regardless changing this (as it is documented) would mean a major compatibility breakage as people might be depending on it, even

[FeldrinH]

These examples use get_slicec, not get_slice. As far as I can tell get_slicec doesn't have the edge case behavior that this issue is about.

However, if you consider changing documented edge case behavior to be a major breaking change then my proposal is ultimately doomed regardless. Feel free to close the issue if this decision is final.

[AThousandShips]

It's the exact same behavior, the two methods are the same, or should be, if they aren't that's a bug, that would be good to test

[FeldrinH]

Well, I get the following:

"0".get_slice(",", 2) == "0"
"0".get_slicec(",".unicode_at(0), 2) == ""

So they are in fact different.

I guess now the question is which one is the bug, because both are following their documented behavior.

[AThousandShips]

That's a good point! And I think I missed a detail and missed this wasn't for position 0 but non-zero, that would be more of a bug IMO but it does match what the documentation says it will do, and as you say it does technically for get_slicec too as it does not mention that part (though it does work the same way for the 0 slice)

So less clear cut indeed, I'd say that it's hard to say what is fully correct though, as except for mentioning that it's faster than split there's no promise that it is indeed equivalent to split in that sense

Personally for the general case I don't know that I think it's more or less reasonable that non-zero slice index returns the full string, but I do think it might make more sense that get_slice and get_slicec behave the same, and in this case I'd say to make get_slicec work the same way

[FeldrinH]

Personally for the general case I don't know that I think it's more or less reasonable that non-zero slice index returns the full string, but I do think it might make more sense that get_slice and get_slicec behave the same, and in this case I'd say to make get_slicec work the same way

At least to for me the get_slicec behavior makes more sense, because it has fewer special cases and matches the behavior of split. This is in my opinion a reasonable expectation that get_slice should also fulfill. I can think of quite a lot of examples where the current get_slicec behavior is useful (see first post of this issue) and no examples where the current get_slice behavior is useful.

Also worth noting that I believe the first Godot internal code example you shared depends on the current behavior of get_slicec to work and would break if you used the get_slice behavior instead.

[AThousandShips]

That's a good point