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

Loosen lifetime bounds on accessing named captures via Index. #143

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "regex"
version = "0.1.48" #:version
version = "0.1.49" #:version
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remove the version bump please? I typically do them in separate commits with tags using a script.

authors = ["The Rust Project Developers"]
license = "MIT/Apache-2.0"
readme = "README.md"
Expand Down
35 changes: 33 additions & 2 deletions src/re.rs
Original file line number Diff line number Diff line change
Expand Up @@ -951,6 +951,15 @@ impl<'t> Captures<'t> {

/// Get a group by index.
///
/// `'t` is the lifetime of the matched text.
///
/// The text can't outlive the `Captures` object if this method is
/// used, because of how `Index` is defined (normally `a[i]` is part
/// of `a` and can't outlive it); to do that, use [`at()`][]
/// instead.
///
/// [`at()`]: #method.at
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remove the links here? I've generally avoided putting links to things in the docs because they're liable to become rotten.

///
/// # Panics
/// If there is no group at the given index.
impl<'t> Index<usize> for Captures<'t> {
Expand All @@ -965,13 +974,23 @@ impl<'t> Index<usize> for Captures<'t> {

/// Get a group by name.
///
/// `'t` is the lifetime of the matched text and `'i` is the lifetime
/// of the group name (the index).
///
/// The text can't outlive the `Captures` object if this method is
/// used, because of how `Index` is defined (normally `a[i]` is part
/// of `a` and can't outlive it); to do that, use [`name()`][]
/// instead.
///
/// [`name()`]: #method.name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here for links.

///
/// # Panics
/// If there is no group named by the given value.
impl<'t> Index<&'t str> for Captures<'t> {
impl<'t, 'i> Index<&'i str> for Captures<'t> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add a bit of documentation here that briefly describes what the 'i lifetime parameter stands for? Thanks! (The 't lifetime should also be documented, e.g., http://doc.rust-lang.org/regex/regex/struct.FindMatches.html)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point; thanks.

While I'm editing the docs: I noticed that the str can't outlive the Captures if it's obtained through either Index impl; should that be a separate PR? It's a little surprising, so a warning and advice to use methods instead might be nice. (This is part of the definition of the Index trait — normally a[i] is part of a and can't outlive it — so it can't be fixed without changing the Output type to &'t str and breaking compatibility.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jld So sorry I missed this comment! You raise a good point. Including those docs in this PR seems fine to me!


type Output = str;

fn index<'a>(&'a self, name: &str) -> &'a str {
fn index<'a>(&'a self, name: &'i str) -> &'a str {
match self.name(name) {
None => panic!("no group named '{}'", name),
Some(ref s) => s,
Expand Down Expand Up @@ -1261,4 +1280,16 @@ mod test {
let cap = re.captures("abc").unwrap();
let _ = cap["bad name"];
}

#[test]
fn test_cap_index_lifetime() {
// This is a test of whether the types on `caps["..."]` are general
// enough. If not, this will fail to typecheck.
fn inner(s: &str) -> usize {
let re = Regex::new(r"(?P<number>\d+)").unwrap();
let caps = re.captures(s).unwrap();
caps["number"].len()
}
assert_eq!(inner("123"), 3);
}
}