Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generalize Replacer impls for FnMut types #509

Merged
merged 1 commit into from
Sep 6, 2018

Conversation

elidupree
Copy link
Contributor

Currently, Replacer is implemented for any F where F: FnMut(&Captures) -> String. I encountered a case where it would be more efficient for the FnMut closure to return &str instead of String.

It was a trivial change to implement Replacer for any F where F: FnMut(&Captures) -> T, T: AsRef<str>. This covers the String and &str cases, as well as supporting other possible types such as Cow<str>.

I have added tests for both String and &str cases (where there were no tests previously), and I have made the corresponding changes for bytes::Replacer.

@BurntSushi
Copy link
Member

@elidupree Thanks! I think this seems sensible to me. The impl for this is already kind of hidden, so I don't really have much concern over this making things more opaque.

The only other concern I have here is whether this could break extant programs due to an inference failure. Firstly, I'm not sure it even applies in this case since it's making an existing impl more generic, and secondly, even if it does apply, I don't think that will be common, but it could happen with e.g., "foo".into().

cc @rust-lang/libs Do you see any pitfalls with inference failures here? I realize it's technically within our purview to make this sort of change in a semver compatible release, but it would be good to get more eyes on this. Do we have any crater-like functionality that could help test this in the ecosystem before a release? (I guess something would need to download each extant dependent, then do a [patch.crates-io] before building.)

@alexcrichton
Copy link
Member

I think @pietroalbini has helped out in the past with non-rustc crater runs which I think could help assess the amount of breakage? Otherwise having a libstd-like policy where "little breaks" are allowed so long as you can write code for all versions makes sense I think

@BurntSushi
Copy link
Member

@alexcrichton Yeah I believe that's the case here, since the impl makes everything strictly more generic. I'll go ahead and merge this and see if there's any fall out. We can always revert & yank if necessary.

@BurntSushi BurntSushi merged commit 53385d7 into rust-lang:master Sep 6, 2018
@BurntSushi
Copy link
Member

This PR is in regex 1.0.5 on crates.io.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants