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

Add a lifetime to Replacer #776

Closed
wants to merge 1 commit into from

Conversation

sgrif
Copy link

@sgrif sgrif commented May 5, 2021

The current implementation requires any implementations of Replacer be
valid for all possible lifetimes. However, this is neither required nor
is it always desirable. For example, a user who wishes to use the
lifetime of the input text as some part of the return type when
interacting with these APIs would end up with a closure that is valid
for one specific lifetime.

A concrete example of where this was problematic was described in
#775

Any future code that does require a Replacer which is valid for all
lifetimes can do so with HRTB:

where
    for<'a> R: Replacer<'a>

or R: Replacer<'_> in some contexts

The current implementation requires any implementations of `Replacer` be
valid for all possible lifetimes. However, this is neither required nor
is it always desirable. For example, a user who wishes to use the
lifetime of the input text as some part of the return type when
interacting with these APIs would end up with a closure that is valid
for one specific lifetime.

A concrete example of where this was problematic was described in
rust-lang#775

Any future code that does require a `Replacer` which is valid for all
lifetimes can do so with HRTB:

```
where
    for<'a> R: Replacer<'a>
```

or `R: Replacer<'_>` in some contexts
@sgrif
Copy link
Author

sgrif commented May 5, 2021

It looks like this might be a breaking change, since Replacer appears to be part of the public API (at least it shows up on docs.rs). Unfortunately I don't think there's any way we can work around this. The scope of the breakage would be anybody who is either implementing or using this trait, and the fix would be similar to this PR, which is what the compiler will suggest (typically adding <'_>)

@BurntSushi
Copy link
Member

Thanks so much! Yeah, the breaking changes means this can't land until regex 2.0.0. I filed #777 to track this. It links to this PR, so I can bring this patch in whenever that happens. But I'll close this for now. :-) Thanks again!

@BurntSushi BurntSushi closed this May 5, 2021
@sgrif
Copy link
Author

sgrif commented May 5, 2021

No worries. Glad I could help <3

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.

None yet

2 participants