Repository navigation
Make Data.Text.replace total #425
Description
Data.Text.replace errors if the needle is the empty string. Every other implementation I tried matches the empty string against every position in the string. I suggest that for consistency with other languages and to avoiding partiality we adopt that behavior.
Expected behavior:
ghci> replace "" "x" "foo"
"xfxoxox"Actual behavior:
ghci> replace "" "x" "foo"
"*** Exception: Data.Text.replace: empty input
CallStack (from HasCallStack):
error, called at libraries/text/src/Data/Text.hs:1862:18 in text-1.2.5.0:Data.TextRust:
fn main() {
println!("{}", "foo".replace("", "x")); // prints "xfxoxox"
}$ rustc main.rs && ./main
xfxoxoxPython:
>>> 'foo'.replace('', 'x')
'xfxoxox'JavaScript:
> 'foo'.replaceAll('', 'x')
'xfxoxox'Reacted by Wismill and Sergey Vinokurov
Activity
Bodigrim commentedon Apr 18, 2022
I'm sympathetic to the idea, but technically this is a breaking change.
Bodigrim commentedon Apr 20, 2022
@sol could you please gather community feedback on this change?
sol commentedon Apr 24, 2022
@Bodigrim I would love to help, however, even if I try hard, I'll not be able to free up time to mediate this issue.
If somebody else wants to pick it up, this might be useful:
The reason why I think this change is a net positive is that relying on the semantics of pure exceptions is inherently risky:
https://gitlab.haskell.org/ghc/ghc/-/issues/1171
https://gitlab.haskell.org/ghc/ghc/-/issues/2273
https://gitlab.haskell.org/ghc/ghc/-/issues/5561
https://gitlab.haskell.org/ghc/ghc/-/issues/5129
Read: If your program relies on the type of pure exceptions, it may produce different results depending on GHC version and optimization level. For that reason some people will advice that you should only ever use a partial function if you make sure that your input will not trigger an exception.
Observations:
Lysxia commentedon May 20, 2022
This is a very benign breaking change. People push back against changes that cause unnecessary churn, but nobody depends on
replace "" _ _being undefined. I will try to collect a few more such changes so we can ask/notify about them all in one post (there is a similar "nobody should care, but do you" conundrum in improvingfoldl).I've confirmed that every occurrence of
Text.replaceon Hackage (~50 packages) is ostensibly applied or meant to be applied to a non-empty string. Almost all of them are a literal or some obvious construction likeText.singleton _or"foo" <> _ <> "bar", so they're quick to go through. The few other uses are a little less obvious, but it is still clear that the empty string is not expected; listed below for exhaustiveness:replaceBy = replace, which I couldn't find any use of on Hackage https://github.com/andreymulik/sdp4text/blob/12990eaa7b39ff1cad7bd52be26986fa0bd0a30d/src/SDP/Text.hs#L172sergv commentedon May 25, 2022
I've noticed that
breakOnandbreakOnEndalso throw exceptions when their pattern is an empty string. Surprisingly this is not mentioned in the documentation and can only be inferred from the presence ofHasCallStackconstraint.I think it might be reasonable to make
breakOnandbreakOnEndtotal. After all, breaking on empty string once can be treated as not doing anything - there's an empty string at the start and end of any string. So something likebreakOn "" str = ("", str)andbreakOnEnd "" str = (str, "")would make those functions total and probably shouldn't be too surprising.There's also
breakOnAllwhich is also partial, but at least this one mentions that the pattern to break on may not be empty in the docs. However I think it could be usefully defined on empty pattern as well - just explode original text into individual characters,breakOnAll "" str = map singleton (unpack str). At first glance this even seems to be consistent to what's written in the docs ofbreakOnAll, which isFind all non-overlapping instances of needle in haystack- all non-overlapping occurrences of an empty string in 'gaps' between characters got broken on.tbidne commentedon Jun 21, 2022
For comparison,
splitOn(also partial) has this to say:So I would suggest either making all of these total (my preference) or at least copying
splitOn's documentation.