Replacer impl for FnMut() -> String leads to many wasted Strings

[Arnavion]

Since Replacer is impled for FnMut(&Captures) -> String, it means a closure that wants to replace the captures with various const string literals has to allocate a new String for each invocation. It would be better if String had some sort of small-string optimization, but it doesn't.

To make it worse, the returned String is only used as a &str to be push_str()ed into the result String anyway. The optimizer is not smart enough to elid the allocation either AFAICT, such as for the code in this SO answer.

It would be nice to impl it for FnMut(&Captures) -> Cow<'static, str> instead, so that such a closure could return Cow::Borrowed("some_literal"). Doing that would be backwards-incompatible as the FnMut() -> String impl would need to be removed, since they would conflict otherwise. impl<F, I> Replacer for F where F: FnMut(&Captures) -> I, I: Into<Cow<'static, str>> { } could work as a backward-compatible impl.

[BurntSushi]

The whole point of the impl is convenience. If you're providing a closure to the replacer function, then you're probably building a string from the capture groups dynamically. The easiest thing to do in that case is to allocate a new string, usually with format!.

If performance is a problem, then you can provide your own implementation of Replacer, and in particular, the Replacer::replace_append method permits amortizing allocation.

Making the type signatures even more complicated than they already are needs a lot more motivation that theoretical. I think for me to be convinced to do this, I'd need to see evidence of this as a not infrequent footgun that impacts the performance of real programs.

It would be nice to impl it for FnMut(&Captures) -> Cow<'static, str> instead, so that such a closure could return Cow::Borrowed("some_literal").

In the vast majority of cases, you can just write replace_all(..., "some_literal") instead of replace_all(..., |_| "some_literal".to_string()), where the former is implemented as efficiently as your Cow suggestion. The only reason for a closure to return a static literal is if it does its replacement based on case analysis over the capture groups, where at least one of the replacements isn't dependent on any of the matched capture group values. I'm not sure how prevalent that is, and moreover, I'm not sure what the performance impact of such a thing is when compared to a more fastidious allocation strategy.

[Arnavion]

If you're providing a closure to the replacer function, then you're probably building a string from the capture groups dynamically.

I'm providing a closure to the replacer function because I want to replace a capture with a different replacement based on which capture it is. There's no reason to assume the replacements must be dynamic. Indeed the replacements are static string constants in the OP scenario as well as in the SO answer linked in the OP, but are forced to be dynamically allocated by the function signature.

If performance is a problem, then you can provide your own implementation of Replacer

Yes, of course I can make a newtype around FnMut(&Captures) -> &'static str and impl Replacer on it. I opened this issue to talk about making the situation better in this crate itself.

Making the type signatures even more complicated than they already are...

The current signature is this:

impl<F> Replacer for F where F: FnMut(&Captures) -> String

and I'm proposing changing it to either:

• impl<F> Replacer for F where F: FnMut(&Captures) -> Cow<'static, str> which requires users whose closures return String to change their code from |captures| some_string to |captures| some_string.into(), and new users whose closures should return &'static str to write |captures| some_static_str.into()

or:

• impl<F, I> Replacer for F where F: FnMut(&Captures) -> I, I: Into<Cow<'static, str>> which allows existing users whose closures return String to continue writing |captures| some_string, and new users whose closures should return &'static str to write |captures| some_static_str.

So there is either minimal or zero change for users depending on the proposal. Even for this crate itself, the proposals have no more "complexity" than the existing signature.

(Ideally you would just add a new impl<F> Replacer for F where F: FnMut(&Captures) -> &'static str and leave the existing one alone. But coherence thinks the two impls will conflict because it isn't smart enough to understand that they only differ in the trait's assoc type and could not both be implemented on the same type.)

... needs a lot more motivation that theoretical.

Not sure what's "theoretical" here. I have some very real code that has this very real problem. There's a very real SO answer that's linked in the OP with the same problem. There's a very real function in the regex crate that only needs a &str value since it only needs it for String::push_str, but requires its caller to give it a String anyway.

It would be nice to impl it for FnMut(&Captures) -> Cow<'static, str> instead, so that such a closure could return Cow::Borrowed("some_literal").

In the vast majority of cases, you can just write replace_all(..., "some_literal") instead of replace_all(..., |_| "some_literal".to_string()), where the former is implemented as efficiently as your Cow suggestion.

I'm not talking about replace_all(..., |_| "some_literal") and certainly not about replace_all(..., |_| "some_literal".to_string()). Again, as I said in the OP and as you can see in the SO answer linked in the OP, the choice of replacement string depends on the capture.

Furthermore, multiple chained replace_all(..., "some_literal")s would still lead to multiple wasted Strings, one for each replace_all except the last. These wasted Strings would be proportial to the size of the input rather than the replacements, which would be even worse for large inputs.

The only reason for a closure to return a static literal is if it does its replacement based on case analysis over the capture groups...

Exactly.

... where at least one of the replacements isn't dependent on any of the matched capture group values.

I'm not sure I understand where this requirement comes from.

[BurntSushi]

Even for this crate itself, the proposals have no more "complexity" than the existing signature.

We won't get anywhere if you won't acknowledge the downsides of your proposal.

I'm not sure I understand where this requirement comes from.

Because if all branches depend on a capture value to build the replacement, then none of the branches return a literal.

Not sure what's "theoretical" here.

I outlined a sketch of the sort of evidence I'd want to evaluate before considering a change like this. I don't see that evidence anywhere. Certainly, your SO link is insufficient. (Specifically, for your backwards compatible proposal. I would need even more compelling evidence for your backwards incompatible proposal, on the order of, "this is causing a significant problem for a large number of users of this crate." My belief is that sort of scale of evidence would have manifested naturally by now.)

[Arnavion]

Even for this crate itself, the proposals have no more "complexity" than the existing signature.

We won't get anywhere if you won't acknowledge the downsides of your proposal.

Both of my comments, and especially the second one that you quoted this from, contain details of exactly what the downsides of my proposals are.

I'm not sure I understand where this requirement comes from.

Because if all branches depend on a capture value to build the replacement, then none of the branches return a literal.

In the SO answer linked in the OP:

    let result = re.replace_all(string, |cap: &Captures| {
        match &cap[0] {
            "or" => "str",
            "e" => "er",
            _ => panic!("We should never get here"),
        }.to_string()
    });

you can see that all the branches depend on the capture value and also return a literal.

Not sure what's "theoretical" here.

I outlined a sketch of the sort of evidence I'd want to evaluate before considering a change like this. I don't see that evidence anywhere. Certainly, your SO link is insufficient. [...] I would need even more compelling evidence for your backwards incompatible proposal, on the order of, "this is causing a significant problem for a large number of users of this crate."

Sorry, I'm only one person. I can only tell you about the problems that I have, not other users of your crate. The best I can do is point out, as I did in the OP, that it's objectively true that rustc does not elid the allocations of the Strings constructed from static literals, and thus it's objectively worse that a closure that could just return &'static str or Cow<'static, str> has to allocate a new String instead.

I'll make a newtype around FnMut(&Captures) -> &'static str in my crate.

[BurntSushi]

I don't really believe there is much about API design that is "objective." I need to consider all facets of a change, not just one use case. That's why I outlined my concerns (type signature complexity) and what kind of evidence I'd like to consider to weigh against the concerns. Namely, examples of the footgun occurring with a measurable impact on program performance. The SO answer might meet the first half of that, but not the second half. Finally, the prevalence of said evidence also matters.

[BurntSushi]

you can see that all the branches depend on the capture value and also return a literal.

That's not what I meant. All of those cases could be converted to using Cow::Borrowed in your proposal. Again, I am thinking holistically here, beyond your singular problem. That is, I am trying to reason about the prevalence of this particular footgun in an ad hoc fashion.

[Arnavion]

This has been fixed by #509