[RFC FS-1033] Discussion: Extend String module

[saul]

Discussion for RFC FS-1033, suggestion at: fsharp/fslang-suggestions#112

RFC at: https://github.com/fsharp/fslang-design/blob/master/RFCs/FS-1033-extend-string-module.md

[isaacabraham]

Just might two cents on the string split - 9/10 times I just want to split on a single char, without worrying about StringSplitOptions etc.. I wonder whether just having those two functions take in a char / string (or list of char / string, which isn't so much more fluff). If you need the "full" version, either another version could be provided, or the user goes back to the BCL call.

[saul]

Yeah maybe another function like splitSingleChar that just splits on one character with no options.

[isaacabraham]

Regarding the issue with overloads for things like invariants - we already have this issue with things like int (which AFAIK works with the Invariant Culture?). I suggest that for better or for worse, we should follow that pattern for consistency.

[isaacabraham]

I've made a provisional branch from which to start a discussion on this...

https://github.com/isaacabraham/visualfsharp/tree/string-module-extensions

I've just done the first ten methods or so. A few points: -

1. I'm using FSCheck to simply test that the behaviour is the same as the original BCL methods (rather than reinvent the wheel and write unit tests for different behaviours)
2. I've changed the outputs in some places e.g. IndexOf returns int option rather than int (where "could not find a match" is encoded as -1)
3. I've kept consistency with the length function in terms of null check but I return false rather than replacing it with an empty string - I think this is more what users would expect as behaviour.
4. I've tried to put the function arguments in the order that best allows pipelining and / or partial application.

Feedback welcome.

[rmunn]

My first thoughts:

I'm not a big fan of having the compare default to StringComparison.InvariantCultureIgnoreCase. I would expect the default comparison to compare foo and FOO as different, and only give me the case-insensitive comparison if I call a variant of compare (say, a function named comparei or compareI).

I like the handling of null strings as being basically equivalent to empty strings (e.g., let contains value str = if isNull str then false else ...). I say "basically" equivalent because the way you've currently written the contains function, if I said let s = null in s |> String.contains "", the result would be false. Which is what I would expect (null doesn't contain anything, even the empty string) but it might or might not be what other people's intuition would lead them to expect.

[isaacabraham]

Ooops, good point. So perhaps just InvariantCulture?

[isaacabraham]

Thinking about it, I actually think that the default of most of these in the BCL is CurrentCulture.

[rmunn]

I'd prefer InvariantCulture as the default, with CurrentCulture being an explicitly opt-in choice. (I've seen LOTS of Stack Overflow questions that were caused by "You assumed that your string comparison was invariant, so your code doesn't work in culture XYZ" -- for example, in the Turkish culture, the uppercase of i is not I, and the lowercase of I is not i.) But the "copy what the BCL does" argument is strong as well.

Edit: I've changed my mind and now prefer Ordinal, not InvariantCulture, as the default, for reasons stated in this comment of mine further downthread.

[rmunn]

So I see two possibilities:

1. Default is to copy what the BCL does. String.compare uses CurrentCulture, String.compareIgnoreCase (or maybe just String.compareI as a name?) uses CurrentCultureIgnoreCase, and the String.compareInvariant and String.compareInvariantIgnoreCase functions do just what their names suggest they do. :-)

2. Default is to use the invariant culture. String.compare uses InvariantCulture, String.compareIgnoreCase (or maybe just String.compareI as a name?) uses InvariantCultureIgnoreCase, and the String.compareCurrentCulture and String.compareCurrentCultureIgnoreCase functions do just what their names suggest they do. :-)

[isaacabraham]

Having started to do a few more of these, I'm not especially fond of the null checks at the start of each of the functions. IMHO people should be pushing these strings into string option before calling these (which is what you'd do when calling the BCL versions anyway).

[saul]

As in the RFC, the default should do the BCL default. An explicit Invariant function should be added, as with the BCL. Finally a way to provide a custom comparison should also be added, again as with the BCL. Consistency with the BCL is key. Also any surprising behaviour, such as accepting null etc, should be removed. You shouldn't have to think "does C# string functions accept null or is it F#?" - I strongly advocate that we keep the F# String functions and the String class as close as possible.

[matthid]

I thought we (F#) settled on InvariantCulture as being the default. Don't make it like C# where we have to introduce warnings everywhere to make people use the invariant version...

In general I agree with @dsyme here: fsharp/fslang-suggestions#112 (comment)

[isaacabraham]

@saul I agree broadly with you. The only thing I wouldn't want to do is restrict the F# module to not take advantage of things like option where possible. So indexOf returns int option rather than int. It really comes down to - do we want behaviour of these to be the same as the equivalent method in the BCL? Probably - this lowers barrier to entry and falls into pit of least surprise.

On the other hand, we also have consistency with the rest of F# to consider (for example, int uses InvariantCulture).

For someone coming from .NET already, they'll much prefer consistency with BCL methods. Someone who is an "F#-first" user might not.

Discuss.

[isaacabraham]

@saul I'm also concerned at the possible of massive amount of pseudo-overloads for these functions e.g.

let indexOf // indexOf, default culture (whatever we decide upon)
let indexOfComparison // indexOf, custom culture

etc. etc. etc.

An alternative is that (just like int) is that we drop the Comparison versions and just use a default StringComparison type. If you want anything more, you fall back to the BCL.

[rmunn]

As @gusty pointed out in this comment, if the String module functions default to CurrentCulture then they won't be referentially transparent, as they will have an invisible dependency on an "outside" parameter (the system's current culture) which could change at any time.

I've also found https://stackoverflow.com/a/20085219/2314532 which strongly suggests (to me) that we want to default to Ordinal, not InvariantCulture, for the string comparison. This also fits with the Best Practices for Using Strings in the .NET Framework guidelines, which state in part:

Use StringComparison.Ordinal or StringComparison.OrdinalIgnoreCase for comparisons as your safe default for culture-agnostic string matching.

So for both consistency and safety reasons, I recommend using Ordinal as the default for compare, with two pseudo-overloads: one that uses OrdinalIgnoreCase, and one that allows the user to pass in a StringComparison enum value to select a specific comparison.

[gusty]

Regarding the contains function, note that in order to make it consistent with the other collections it should check whether a char is present in the string.

In order to get the desired functionality in a consistent way we should name that function isSubstring which will be consistent with Set.isSubset and we can propose to add related functions to other collections (Array, list, seq), the only particularity is that the name is not the same although it follows the rule isSub[CollectionName].

Note that this is slightly related to the naming discussion on fsharp/fslang-suggestions#102 when considering names like Array.replaceSubsequence or Array.replaceSegment (Array.replaceSubArray ???)

[dsyme]

... in order to make it consistent with the other collections it should check whether a char is present in the string.

This is not quite the case: the String module functions are already inconsistent on this point, e.g. String.replicate takes a string, while String.iter provides chars.

[gusty]

I know, and I find it a bit unfortunate. But it's already there, and we'll have to live with it.

[isaacabraham]

In order to (maybe) give this issue a bit of a boost to get something reasonable in, I've created a single file which contains a version of the branch I created but as a standalone file. This can easily be added to your existing F# projects through Paket's github dependency feature:

github CompositionalIT/fshelpers src/FsHelpers/FSharpCore/String.fs

Perhaps we can test this out and see if it "feels right" in this low-risk manner, and then makes changes to it as needed with PRs etc. to that file. Once we feel it's good to go, we can port it here with confidence that it'll be in a good position to move into the real Core library.

[isaacabraham]

I've tried today incorporating this into an existing application of ours, replacing any instance methods with ones from the module. Some thoughts:

• Obviously, replacing instance methods with these static ones isn't appropriate all of the time - for simple ones like ToUpper() etc. it's not worthwhile IMHO (unless we add behaviour to do eg null checks first).
• Returning None rather than -1 for the "failure" case of IndexOf worked quite nicely. Perhaps we can explore doing this more in some of the other functions.
• For lambdas over e.g. collections, the String module works nicely e.g. Array.map String.toUpper
• However, not being able to open the String module means that you have to continually type String. - so, for composed functions it can become a little cumbersome e.g.
  String.toUpper >> String.substring 3 >> String.indexOf "a" instead of
  toUpper >> substring 3 >> indexOf "a"
• It's become apparent that we need to be very careful with making clear whether these functions are like-for-like replacements of the member methods on strings - changing things like "Foo".Compare to "Foo" |> String.compare (with different StringComparison defaults) could cause some really awful bugs. Changing the defaults are fine, as long as we make it damn clear in intellisense etc.

Just my two cents.

[jbtule]

Just came across this today from StructOptions discussion.

I have a library which I generate from script pointed at various BCL classes with static functions and or non side effect methods, and just produces many inline functions with argument reordering.

Most of the signatures match except for the ones you've hand tuned for better default behaviors as my script I only give heuristics for argument order.

Thought I might mention as an alternative, or as independent confirmation.

https://github.com/jbtule/FSharp.Interop.Compose/blob/8c916b4f1bc919142740c9e3cd61e443d5909118/src/NETSTD_2_0/FSharp.Interop.Compose.System/String.fs#L3-L33

But I cover a lot more than just string methods this way. http://jay.tuley.name/FSharp.Interop.Compose/reference/index.html

[Happypig375]

For context, dotnet/designs#207 suggests that having CurrentCulture by default for string APIs is a pit of failure. F# must not just copy .NET APIs, rather we should have Ordinal as default as well.

[krauthaufen]

Hey, how about a canonical String.splitLines : string -> string[] (maybe seq<string> or list<string> would feel better in F#) which is agnostic to \r\n vs \n things?
I need that quite often and I think it would fit nicely here.

[Happypig375]

It should include all Unicode line endings like U+2028.

[Happypig375]

apisof.net no longer lists usage statistics - Does anyone have a copy of the data?

[charlesroddie]

Should add to drawbacks:

• This adds alternative ways to do something that can already be done.
• No improvements are made to the faithfulness or naturalness of type signatures - the input and output information is identical - so these are not the best candidates for creating special F# methods.

[charlesroddie]

In the sense of adding extra alternative ways (static methods) of doing what you could do before (with instance methods). These alternatives are close to equivalent, unlike using a Map vs implementing your own Map, which involve very different levels of programmer effort.

[krauthaufen]

Okay maybe I was a little polemic there, sorry. But what about things like Option.isSome, isNull, Array.length, etc?

[jwosty]

Should add to drawbacks:

* This adds alternative ways to do something that can already be done.

* No improvements are made to the faithfulness or naturalness of type signatures - the input and output information is identical - so these are not the best candidates for creating special F# methods.

I hold the opinion that at least some of these are definitely worth it. Two huge pro's that outweigh those cons for me are:

1. Type inference. Having these be static module functions rather than instance methods lets me write (str: string) fewer times
2. Fluent API. Chaining with right-pipes is more idiomatic F# than dot-chaining, especially in this circumstance, as I do find myself chaining string operations straight into seq operations. It's awkward to switch back and forth between right-pipe-chaining and dot-chaining.

Piggybacking off point 2, I would absolutely love a curried String module version of String.Join. I very often find myself building sequences of things which end in String.Join and have to have a nice pipe chain interrupted as a result:

// dumb example - today we have to write this:
let results =
    things
    |> Seq.map (fun x -> x + 1)
    |> Seq.filter (fun x -> x % 2 = 0)
    |> Seq.map string
String.Join (",", results)

// when it would be way better as:
things
|> Seq.map (fun x -> x + 1)
|> Seq.filter (fun x -> x % 2 = 0)
|> Seq.map string
|> String.join ","

[NicoVIII]

Without wanting to adress any of the main points in this discussion:
Isn't String.concat doing exactly that today?
https://github.com/dotnet/fsharp/blob/cbe88a8acb16e60979ab67d073d3c0527d089376/src/fsharp/FSharp.Core/string.fs#L22

[jwosty]

@NicoVIII you're right. Honestly didn't know that existed. I always assumed String.concat didn't take a separator and so have never looked at the signature!

[jwosty]

How about instead of "changing" the default for Compare (and the likes), we instead implement String.compareOrd without adding a String.compare? That way, it would be clear that it is using different behavior than System.String.Compare, but we still encourage its usage by excluding an alternative choice. If someone really wants to use CurrentCulture, they would have to use either the .NET method, or perhaps a String.compareWith.

[Happypig375]

I would instead bank on System.String.Comapre being deprecated as in dotnet/designs#207. We should keep the succinct name of String.compare.

[Happypig375]

We should also be consistent with F#'s own compare function: https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AbEAzAzmgExAGoAfSAWwAcBDWAAgCIBGRpgJkaA===

[jwosty]

Also, what is the int? intending to convey in the following signature?

substring : (length: int?) -> (startIndex: int) -> string -> string

[jwosty]

Why not simply use slicing-syntax str.[3..] which already has more or less the desired signature?

When adding a substring function I'd make it like Array.sub for consistency reasons (and maybe even just call it sub?)

Honestly, I think it's unfortunate these functions aren't called slice. But I think it would be sensible to call it sub.

I would actually personally expect the signature to be like so, however:

val substring : startIndex:int -> endIndex:int -> string -> string

This would make the following two equivalent:

str.[5..10]
str |> String.substring 0 10 // longer to type, but better with type inference and piping

Perhaps we can call it slice instead, to make room for adding a similar method to array -- i.e. why does Array.sub have the array argument first, and take a count instead of an ending index (unlike .Item)?

[krauthaufen]

You just reminded me why I never use Array.sub (the non-pipeable argument order), so I'm totally in favor of putting the string argument last 👍

[heronbpv]

Right, but curried functions in modules can’t have optional arguments, which is why I ask. I wonder if it’s indicating an unresolved question?

Was this found in a signature file? AFAICS, trying to declare such a function in a module would result in a compilation error.

[jwosty]

You just reminded me why I never use Array.sub (the non-pipeable argument order), so I'm totally in favor of putting the string argument last 👍

The question is, then, should it be called substring or slice? Substring would be nice, because it clearly parallels the terminology of System.String, but slice is nice, because it would leave the door open to do the same in the Array module.

Was this found in a signature file? AFAICS, trying to declare such a function in a module would result in a compilation error.

I'm referring to the RFC

[heronbpv]

I'm referring to the RFC, which currently proposes the following signature: substring : (length: int?) -> (startIndex: int) -> string -> string

Interesting. The system version has no such optional argument, so maybe in this case, since it doesn't even compile as is, just treat it as a typo.

[jwosty]

Should String.split and friends really return an array, or an immutable sequence? Returning an array would be good because it would more directly wrap the BCL (and avoid copying to an immutable type), but returning an immutable seq would be more idiomatic F#.

[dsyme]

A reminder to everyone that the only thing "approved" so far is really based on this comment:

fsharp/fslang-suggestions#112 (comment)

Where I said:

I'm OK with the specific list above, especially since it is determined by data rather than adhoc design.

However it's a very slippery slope. At some early point F# developers just need to learn how to call .NET APIs. The more you delay it, the more you build up the expectation that you can do more and more without doing that, and the more APIs you end up re-creating.

Strings also have the whole huge issue with language culture. Historically we've only ever put culture-invariant operations in FSharp.Core, and nothing that relies on CurrentCulture. All of the above look invariant, correct?

Is the RFC up-to-date with the proposed list? Some comments on that list

1. It's much longer than anything that was mooted in the suggestion. This indicates a slippery slope, the destination of that is "hey, let's wrap every .NET API in an F# pipelining style!" and even worse "dot notation bad, objects bad!". That's not F#'s destination.

2. From a teaching perspective, on first glance the list of functions looks a bit like the kind of thing we don't want to add to FSharp.Core. When I look at it I simply think "the .NET APIs are better"... As a result these would not be on the list of functions that every F# programmer should know (arguably, some existing String module functions are already in that).

Anyway, I added some content to the RFC here: #622, and I'd recommend we go back and clarify the methodology.

[dsyme]

OK, well, I've been over the proposed list and to be honest I don't think we should proceed with this RFC. I was skeptical about it at the start and I don't find the current RFC compelling.

Looking through the list of functions:

1. All the "Comparison" and "CultureInfo" functions need things from the .NET libraries that the F# programmer will need to learn about. At that point they may as well learn how to use the .NET libraries and overloads.

2. Things like toLower are listed as taking CultureInfo which is no better or worse than the .NET APIs

3. trim, endsWith, contains, trimChars - none of these are compelling - why not just use the .NET APIs? Pipelining alone is not a valid rationale

4. substring is just slicing and that would always be used instead

To be honest I just don't think there's a sound methodology here that substantially improves on the status quo. The .NET APIs are not bad, and they are canonical and documented. There is a very strong argument that List, Seq, Array bring excellent uniformity and sanity to immutable data programming (pretending Array is immutable). But these additions just aren't in that category.

[jwosty]

Replying to @dsyme saying:

BTW another thing that's often in libraries is splitting strings into lines.

Should I take that as a go-ahead to put that in the RFC as well? Would be straightforward, and should split on \r, \r\n (I'm not convinced it should try to handle the various Unicode line endings -- though if File.ReadAllLines splits on these, then our function should do that too, for consistency)

splitLines : string -> string[]

[dsyme]

Should I take that as a go-ahead to put that in the RFC as well?

Personally I think I would use it. But we should canvas a little wider than just me :)

[dsyme]

though if File.ReadAllLines splits on these, then our function should do that too, for consistency

agreed

[ErikSchierboom]

I would really like splitLines!

[tomcl]

On this topic, remembering when I was learning F#, String.concat is very confusing because of the difference from List.concat. In fact I still find it confusing. We cannot change it, but we could still add an alias:

/// Returns a new string made by concatenating the given strings with separator sep
let String.join (sep: string) (strings:seq<string>) = String.concat sep strings

It would reduce friction for many people.