Skip to content

feat: Regex::new takes Into<String> #811

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

Closed
wants to merge 1 commit into from
Closed

feat: Regex::new takes Into<String> #811

wants to merge 1 commit into from

Conversation

DeveloperC286
Copy link

Makes Regex::new more flexible and allowing the type Into to be used to create a regex.

Makes Regex::new more flexible and allowing the type Into<String> to be used to create a regex.
@BurntSushi
Copy link
Member

I'm sorry, but I'm opposed to this for a few reasons:

  1. Generics add complexity. I use them when necessary or compelling, but I don't think either is the case here.
  2. I suspect this will break people. I'm on mobile, so I can't work through an example, but the main thing I would be concerned about is whether the generics prevent deref to &str from occurring in certain instances.

@BurntSushi BurntSushi closed this Oct 31, 2021
@BurntSushi
Copy link
Member

I had a chance to play around with things, and this does indeed look like a bad enough breaking change to be a non-starter. For example, this compiles:

fn test1(_s: &str) {}

fn main() {
    let x: Vec<&str> = vec!["foo"];
    x.iter().map(|s| test1(s)).collect::<Vec<()>>();
}

but this doesn't:

fn test1(_s: impl Into<String>) {}

fn main() {
    let x: Vec<&str> = vec!["foo"];
    x.iter().map(|s| test1(s)).collect::<Vec<()>>();
}

and AIUI, this is exactly because of deref working in the former example due to the concrete type, where as in the latter example, the lack of concrete type makes the deref inference fail. I don't consider this a weird corner case, and although this is technically under the umbrella of "allowable breakage," I think this would be too severe in practice.

So if this change were made, it would have to wait until a regex 2.0 release (of which there are no plans). And even then, my reason (1) in my previous comment would still apply.

@DeveloperC286
Copy link
Author

Hi, @BurntSushi yes you are right this was a breaking change. I tested my patch using my tool called did_i_break_it to compile all crates on crates.io that depend on regex and there were a number that failed to compile.

Regarding the unnecessary complexity that generics add, I was actually adding the generic to make it more ergonomic.In my usecase I had a Vec<String> I wanted to create regexes from, but as Regex::new only accept &str I was having to add a & into my code to get a reference.

I then read regex's source code and seen you actually convert it back to a String, so thought you may aswell use Into<String> as it may offer a minimal performance improvement for certain cases when people give you ownership of the String, by removing another allocation(did not test, just a guess).

So if Into<String> is a breaking change what about new() taking a AsRef<str>? So new can take either a &str or String?

@BurntSushi
Copy link
Member

I think AsRef has the same problems.

as it may offer a minimal performance improvement for certain cases

Very unlikely. The copying of a single string is a pittance compared to the work of regex compilation.

Regarding the unnecessary complexity that generics add, I was actually adding the generic to make it more ergonomic.In my usecase I had a Vec<String> I wanted to create regexes from, but as Regex::new only accept &str I was having to add a & into my code to get a reference.

Yes, I understand how generics can occasionally make things more ergonomic. But it's a balancing act, as the generics will also cause less ergonomics in some cases. And generics tend to increase complexity by making the API harder to read.

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.

2 participants