Runtime: Proposal: Add an overload of string.Replace that accepts CultureInfo and CompareOption (and additionally Span-based overloads)

[Gnbrkm41]

Rationale and Usage

Related issue: https://github.com/dotnet/corefx/issues/36251, https://github.com/dotnet/vblang/issues/390

Currently there is no way to treat Japanese characters that only differ in its width (e.g. アイウエオ, ｱｲｳｴｵ) when replacing characters in a string. The closest existing API out-of-the-box is string.Replace(String, String, Boolean, CultureInfo) method, however ignoring Kana type / Width cannot be done through CultureInfo and is usually specified through CompareOption enum. (e.g. string.Compare(String, String, CultureInfo, CompareOptions)). From what I can see, this is somewhat tedious to implement on one's own as there is no overload of IndexOf or similar method that enables one to ignore the width/kana type.

Therefore, I suggest that we add an overload of string.Replace that accepts string, string, CultureInfo, CompareOptions to allow people to have fine-grained control over the replacement.

Proposed API

namespace System
{
    public class String
    {
        // On top of the existing methods...
        public string Replace(string oldValue, string? newValue, CultureInfo? culture, CompareOptions options);
    }
}

(The order of the CultureInfo/CompareOptions is the same as string.Compare(String, String, Boolean, CultureInfo), just in case if someone's wondering why it's not the other way around)

Details

Currently, Replace(String, String, Boolean, CultureInfo) method is implemented as follows, calling the internal method ReplaceCore that accepts a CompareOptions:

public string Replace(string oldValue, string? newValue, bool ignoreCase, CultureInfo? culture)
{
    return ReplaceCore(oldValue, newValue, culture, ignoreCase ? CompareOptions.IgnoreCase : CompareOptions.None);
}

Instead of using ternary to pass only the IgnoreCase option, we can validate the option passed from the caller (e.g. if any flags not defined in the enum is set, Ordinal is set with others...) then call the ReplaceCore method as usual to perform the operation.

Open Questions

• Upon a quick glance over the code, it seems that the ReplaceCore calls some methods (e.g. CompareInfo.IndexOf) which call Windows native functions to perform certain operations. Is the method available in other platforms as well?
• Should this be provided for StringBuilder as well? it appears that the type has some similar methods, albeit with much less overloads. Maybe we could open a separate issue regarding this.

Span-based overloads are discussed below.

[Gnbrkm41]

Note: half-width and full-width characters seem to be considered different under CultureInfo.InvariantCulture/InvariantCultureIgnoreCase and StringComparison Equivalent of it.

[bartonjs]

@tarekgh Thoughts?

[tarekgh]

I think we had similar request opened before. let me dig to find it.

[tarekgh]

Linking the related issues:

https://github.com/dotnet/corefx/issues/1271
https://github.com/dotnet/coreclr/pull/9316
https://github.com/dotnet/corefx/issues/28001
https://github.com/dotnet/corefx/issues/33543

@Gnbrkm41 did you run into issues you needed this API or you just suggesting this will be a useful API?

side question: why we have the new value and string parameters are nullable? do you expect the api can take a null values?

@bartonjs the proposed API can make sense if anyone really need it. there could be some work around for people to do too if needed when we expose the linked proposal. Also, there is other work around by setting current culture and then use the current APIs to do the job and then revert the current culture. I know this ugly but just wanted to point at it.

[Gnbrkm41]

I didn't have a use for it myself, but @Nukepayload2 who have opened the issues mentioned in the beginning (#36251, dotnet/vblang#390) needed to do so due to customer request,(https://github.com/dotnet/vblang/issues/390#issuecomment-471180816) and was using legacy Visual Basic's Strings.Replace method to do such replacements because there's no API that lets you do so out-of-the-box (Which also is buggy). Thought it'd be nice to have.

newValue and culture are nullable because the closest overload public string Replace(string oldValue, string? newValue, bool ignoreCase, CultureInfo? culture) had these as nullable. 😀 Maybe you can ask @safern for the answer to that. Related: https://github.com/dotnet/coreclr/pull/23450

[tarekgh]

If we are going to proceed with this proposal, I would say we should add it to CompareInfo too.

C# namespace System.Globalization { public class CompareInfo { public string Replace(string oldValue, string newValue, CompareOptions options); } }

For completeness and also will have some little perf benefits.

Upon a quick glance over the code, it seems that the ReplaceCore calls some methods (e.g. CompareInfo.IndexOf) which call Windows native functions to perform certain operations. Is the method available in other platforms as well?

Yes we should have the same support on Linux/OSX as we do on Windows.

Should this be provided for StringBuilder as well? it appears that the type has some similar methods, albeit with much less overloads. Maybe we could open a separate issue regarding this.

No, I don't think we should add any such methods on StringBuilder. I doubt if anyone will use it there.

[tarekgh]

forgot to mention we should also have the same APIs that takes and return Spans.

[Gnbrkm41]

How would we return spans? I thought ReadOnlySpans were only for copying parts of existing string for free (which is okay because they are immutable). Wouldn't Replace require modification to the content of the original string, meaning the result string would end up being allocated somewhere to ensure immutability of strings?

Or have you meant the non-readonly Span<T>?

[tarekgh]

We can have ref parameter for the returned span. I don't think in the case of using spans, the API should do any allocations. I was just mentioning we should have the API that work with spans for the perf scenarios.

[bartonjs]

@tarekgh I don't think a Span-returning Replace makes sense.

One that writes to a span, sure; but it can't just make up new memory and return a Span (if it's making up new memory it should return Memory, or (better) string/array/whatever-it-actually-is)

[Gnbrkm41]

The problem with span-manipulating-replace is also that the result string can result in longer length if new value is longer than old value (e.g. Replace("a", "aaa")).

I was thinking about StringBuilder, as it can be mutable and can be appended / manipulated quite easily. The consumer can also chain Replace calls without creating a new string every call as well.

[Gnbrkm41]

Regarding the span(or whatever) accepting/returning overload, shall I modify the issue a bit or just open a new issue?

[safern]

Maybe you can ask @safern for the answer to that

if newValue is null, we replace it for string.Empty. If culture is null, we use CurrentCulture. That is why those values are nullable. Since we're already annotating corelib with nullables, new APIs should think of nullable as well. As part of the API review, we should decide which parameters can be null and if the return type can be nullable as well. So I think this new proposed overloads should accept nulls for newValue and culture, following what our current APIs do.

[bartonjs]

The problem with span-manipulating-replace is also that the result string can result in longer length if new value is longer than old value

Yeah, it'd probably need to be OperationStatus-based. Something like
C# partial class NotSureWhere { public static OperationStatus StringReplace( ReadOnlySpan<char> source, ReadOnlySpan<char> oldValue, ReadOnlySpan<char> newValue, CultureInfo? cultureInfo, CompareOption compareOption, Span<char> destination, out int charsWritten); }

Looks lovely, everyone will want to call it... or... well... not.

[GrabYourPitchforks]

@tarekgh Would it make sense to put this only on CompareInfo and _not_ on string? That way we keep the existing string APIs simple. I don't think we need a span-based version right now since we don't even have MemoryExtensions.Replace yet.

[tarekgh]

@GrabYourPitchforks I think adding it in CompareInfo only would address the current need for it. I already proposed to have it in CompareInfo here https://github.com/dotnet/runtime/issues/29111#issuecomment-480332085. I believe it was proposed in String for simplicity/convenience and easy to discover.