godot
godot copied to clipboard
godotengine
StringExtensions.CompareTo leads to IndexOutOfRangeException on non-empty strings
Tested versions
It happens in 4.2.2-stable and 4.2.2-rc1, but likely all others too because they use the same implementation.
System information
Godot v4.2.2.stable.mono - Garuda Linux #1 ZEN SMP PREEMPT_DYNAMIC Wed, 17 Apr 2024 15:20:00 +0000 - X11 - Vulkan (Mobile) - integrated Intel(R) Xe Graphics (TGL GT2) () - 11th Gen Intel(R) Core(TM) i5-1135G7 @ 2.40GHz (8 Threads)
Issue description
Calling StringExtensions.CompareTo() with non-empty strings crashes the script with IndexOutOfRangeException. You falsely assume, the strings are nul-terminated. Maybe Godot.Strings are (is this just a copy of the code from your implementation for GDScript?) but normal C# strings aren't which you are using in the parameters.
Just replace stringVariable[index] == 0 with index >= stringVariable.Length and you are good. This might be a good time to check your other C# functions.
Here it is: StringExtensions.cs:452…489
PS: copy-pasting that code is dirty and should be cleaned up. The performance benefit is almost non-existent. Not recommended.
Steps to reproduce
Add a C# script and write if ("a".CasecmpTo("a") == 0); into it. When it is run, it will crash script execution.
Minimal reproduction project (MRP)
To be honest, I don't know what we're not simply calling CompareInfo.Compare(string, string, CompareOptions) here.
My thought was similar. It looks like it fell under the Radar because C# provides these functions already and calling string.CompareTo(string) does not call the StringExtensions method of Godot but the standard .NET method in which everything works fine. I only actually noticed it because I added the boolean argument.
For someone who wants to exercise using Git pull requests, it could look like this:
public static int CompareTo(this string instance, string to, bool caseSensitive = true)
{
return String.Compare(instance, to, caseSensitive? StringComparison.Ordinal : StringComparison.OrdinalIgnoreCase);
}
I ran a quick test to check if String.Compare would work and found out that:
Mathf.Clamp(String.Compare(instance, to, caseSensitive? StringComparison.Ordinal : StringComparison.OrdinalIgnoreCase), -1, 1)
outputs a different result to your originally suggested fix (index >= stringVariable.Length) ~50% of the time.
I don't know if this will cause any issues. Maybe it's risky to have inconsistent results between c# and GDScript? At the same time it might be more risky for the StringExtensions method to not match the expected C# behaviour, in which case we should also drop the [-1,1] range...
Using invariant instead of ordinal should match the implementation more closely IMHO.
You are right, I re-ran the test using Invariant and got a 100% match for both String.Compare and InvariantCulture.CompareInfo.Compare.
Ah, I didn't know, GDScript uses an Invariant Culture like comparison. The original C# implementation (that I linked above) looks perfectly ordinal to me (instance[instanceIndex] < to[toIndex]) and using InvariantCulture comparison could be a breaking change, or I am missing something. But people get Invariant Culture comparison when they don't provide the boolean 2nd argument because Invariant Culture comparison is the comparison used by C#'s own string.CompareTo(). Maybe this means, InvariantCulture comparison indeed reduces the surprises (between passing a 2nd argument true and no second argument). The behaviour just needs to be documented properly.
Ah, I didn't know, GDScript uses an Invariant Culture like comparison.
It does not, it is ordinal. But I'd still be inclined to match default BCL behaviour here, as I think that's what most people would expect.
About the change being breaking, yes, but the current behaviour we are breaking is an NRE so I'm not sure how much of a consideration that really is.
Cc @raulsntos @Repiteo since we talked quite a lot about cultures and comparisons a while ago.
While I'm generally in favor of matching BCL behavior over GDScript behavior, the purpose of StringExtensions is to match the Godot string methods. In this particular case, it may be better for C# users to just use string.CompareTo with a StringComparison parameter directly which allows you to get the exact behavior you want.
Personally, I'd just deprecate this method since we already have string.CompareTo in the BCL. But last time I proposed this I was told that the boolean parameter was more convenient over the StringComparison parameter so it was worth keeping it.
Regarding changing behavior, @paulloz is right. Since the current behavior is a bug (resulting in an exception), it's fine to change behavior here.
It's not technically a behaviour bug (the behaviour is ordinal comparison, the bug is the nul-termination assumption) but never mind, just make sure to document the choice, e.g. here.
I would fully support deprecating this Godot method because there is a BCL-type method String.Compare(String, String, Boolean) https://learn.microsoft.com/en-us/dotnet/api/system.string.compare?view=net-8.0#system-string-compare(system-string-system-string-system-boolean) which already does string comparison with boolean ignoreCase parameter. Typing just an additional String class name in front is not a valid argument about less convenience as I think.
This wouldn't be the first time a StringExtensions method was deprecated in favor of a native alternative; I say we just go for that.
I didn't realize there was also an overload with a boolean, I must have confused this with another method. In that case I agree that we should just deprecate it.
TBH, even deprecated, the method probably should not NRE. It doesn't change the fact that that needs to be addressed.