Add sorted() method to Array and Dictionary

[Kaleb-Reid]

Similar to #112077 but for sort() instead of reverse(). This adds the method to Array, the Packed*Arrays, and Dictionary. It also adds sorted_custom() to Array

Haskell has left me feeling like these are missing every time I go to use the ones we already have and remember that they don't return anything.

Closes godotengine/godot-proposals#13509.

[AThousandShips]

Related proposals:

• Array.sort() should return a reference to the array godot-proposals#5771
• Make Array.sort() and Array.sort_custom() return the array it sorted. godot-proposals#6877

[AThousandShips]

I'd personally suggest adding Array.sorted_custom as well for completeness

[Mickeon]

I must note that the reason these methods don't exist is probably due to performance concerns, to prevent users from shooting themselves in the foot with a wasteful array copy. I just checked to confirm my doubts in the proposal, and yeah. I don't think adding these for the packed arrays is a great idea, personally.

[Kaleb-Reid]

@Mickeon Yes I agree the performance is worse than if you are not already duplicating the container. In this case I intentionally made it so that all instances of sorted() return a copy instead of a reference to the same container, similar to the difference between Dictionary.merge() and Dictionary.merged(). I guess an alternative to removing it from packed arrays and switching it to return a reference to the same container is to put a note in the docs on each method saying that the copy makes it much slower.

After reading some of the proposal you linked, I think that this is a bit different than what is proposed there. If sort() was changed to return a copy then there would be no alternative and performance would be a concern but we still have sort() with this pr. Adding sorted() which does the same thing (no copy) as sort() but returns the container feels more like a bandaid for the problem that we can't change the signatures of variant methods without breaking compatibility. I think once that is figured out sort() can be changed for array / dictionary to return a reference to the same container if necessary but sorted() should be distinct.

[Mickeon]

You're right. In the proposal it is rightfully assumed that there would be no performant alternative to sort() returning a duplicate, so the criticism is skewed towards that.

Still, let's tread carefully here. We generally avoid bloating the API, especially for core Variant types, unless there's big demand for it.
I am not sure it's appropriate to have this method for the packed arrays. Aside from the aforementioned bloat, packed arrays are designed and intended to be more efficient than Arrays. For more complex and situational methods, the Array type has always been the way to go. There's many other precedents though, so I don't know.

Regardless of what you do, a note warning of the potential performance caveats should be added (similarly to what is done in remove_at's description if I recall correctly).

[AThousandShips]

I think the benefit of saving one line needs to be weighed against the risk of abuse, especially with the potential confusion from things like String and basic types not allowing mutation, people could easily look at my_string = my_string.capitalize() and think you should always do that, as most simple types are non-mutable and the container types are a weird exception to that

Having to use:

var my_copy = my_array.duplicate()
my_copy.sort()

Isn't that big of an inconvenience I'd say, and I don't think there would be any real performance difference between that and this new method

The special case I can see is cases like immediate use, like:

for key in my_dictionary.keys().sorted():

Where you don't ever need to touch the copy "really"

Personally I think the safest option, though it's unfortunately more disruptive, would be to have Array.sort return a reference to itself, that way you can just do my_array.duplicate().sort()

Edit: I think there are three options with varying pros and cons:

• Don't do anything, which requires a little bit of extra code, but is perfectly clear and creates no confusion, and avoids misleading people to use things incorrectly
• This solution, which has the benefit of saving a line of code, but risks creating confusion and inexperienced users might use it not realizing because they see example code using it and just go with it (this is, unfortunately, common in my experience)
• Make sort (and several other mutating methods) return a reference to the container itself, beyond the breaking change, and the issue with handling compatibility for built-in types (we can't do that yet), this has the benefit of not encouraging incorrect use (you just use arr.duplicate().sort()), but the drawback is that people might think sort doesn't mutate but returns a copy, but I'd say that's less likely than people using the wrong method because they don't know better

[Ivorforce]

While similar to #112077, as well as godotengine/godot-proposals#5771 and godotengine/godot-proposals#6877, it's not quite the same.
So like suggested in #112077, please open a feature proposal for this change.

Personally, I think it is less error prone than godotengine/godot-proposals#5771 and godotengine/godot-proposals#6877, so I think it has a better chance of being accepted.

We'll monitor discussions and interest in this feature, and will consider merging it if it becomes popular enough.