[ahelwer]

Sure! So I'm trying to implement this feature: https://github.com/tree-sitter/tree-sitter/issues/982#issuec...

And here's my branch (you can see the latest commits to see the file I'm modifying): https://github.com/ahelwer/tree-sitter/tree/testfile-separat...

I haven't had a chance to go through and add clones everywhere, and will be away at a PT appointment for the next hour or so, but would appreciate any pointers you can give.

[devit]

It passes cargo check with this change.

  diff --git a/cli/src/test.rs b/cli/src/test.rs
  index ac4807bf..8b9882ab 100644
  --- a/cli/src/test.rs
  +++ b/cli/src/test.rs
  @@ -410,16 +410,17 @@ fn parse_test_content(name: String, content: String, file_path: Option<PathBuf>)
           .map(|b| String::from_utf8_lossy(b).to_string())
           .map(|s| escape_reserved_regex_chars(&s));
       
  -    let suffixHeaderPattern : Option<String> = suffix
  +    let suffixHeaderPattern : Option<String> = suffix.as_ref()
           .map(|s| String::from(r"^===+") + &s + r"\r?\n([^=]*)\r?\n===+" + &s + r"\r?\n");
       
  -    let suffixDividerPattern: Option<String> = suffix
  +    let suffixDividerPattern: Option<String> = suffix.as_ref()
           .map(|s| String::from(r"^---+") + &s + r"\r?\n");
   
  -    let headerRegex = suffixHeaderPattern
  -        .and_then(|s| ByteRegexBuilder::new(&s[..]).multi_line(true).build().ok())
  -        .as_ref()
  -        .unwrap_or(&HEADER_REGEX);
  +    let headerRegexFromSuffixHeaderPattern = suffixHeaderPattern.as_ref()
  +        .and_then(|s| ByteRegexBuilder::new(&s[..]).multi_line(true).build().ok());
  +
  +    let headerRegex = headerRegexFromSuffixHeaderPattern
  +        .as_ref().unwrap_or(&HEADER_REGEX);
   
       // Identify all of the test descriptions using the `======` headers.
       for (header_start, header_end) in headerRegex

[ahelwer]

Thank you for this! the .as_ref() seems to solve a lot of the problems, since I guess just doing a straight .map() takes ownership of the contained string.

[estebank]

The error output for your code is

    error[E0382]: use of moved value: `suffix`
       --> cli/src/test.rs:416:48
        |
    406 |     let suffix = FIRST_HEADER_REGEX
        |         ------ move occurs because `suffix` has type `std::option::Option<std::string::String>`, which does not implement the `Copy` trait
    ...
    414 |         .map(|s| String::from(r"^===+") + &s + r"\r?\n([^=]*)\r?\n===+" + &s + r"\r?\n");
        |          ------------------------------------------------------------------------------- `suffix` moved due to this method call
    415 |
    416 |     let suffixDividerPattern: Option<String> = suffix
        |                                                ^^^^^^ value used here after move
        |
    note: this function consumes the receiver `self` by taking ownership of it, which moves `suffix`
       --> /Users/ekuber/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/src/rust/library/core/src/option.rs:451:38
        |
    451 |     pub fn map<U, F: FnOnce(T) -> U>(self, f: F) -> Option<U> {
        |                                      ^^^^
    
    error[E0716]: temporary value dropped while borrowed
       --> cli/src/test.rs:419:23
        |
    419 |       let headerRegex = suffixHeaderPattern
        |  _______________________^
    420 | |         .and_then(|s| ByteRegexBuilder::new(&s[..]).multi_line(true).build().ok())
        | |__________________________________________________________________________________^ creates a temporary which is freed while still in use
    421 |           .as_ref()
    422 |           .unwrap_or(&HEADER_REGEX);
        |                                    - temporary value is freed at the end of this statement
    ...
    425 |       for (header_start, header_end) in headerRegex
        |                                         ----------- borrow later used here
        |
        = note: consider using a `let` binding to create a longer lived value

You need to change line 413 to turn `suffix` into an `Option<&str>` to avoid taking ownership of it:

    let suffixHeaderPattern: Option<String> = suffix
        .as_ref()
        .map(...);

and you need to change the `headerRegex` extraction to turn it also into an `Option<&str>` from an `Option<String>` by using `.as_ref()` before the `.and_then` call, which lets you avoid the `&s[..]` reborrow, that only lives until the end of that closure:

    let headerRegex = suffixHeaderPattern
        .as_ref()
        .and_then(|s| ByteRegexBuilder::new(s).multi_line(true).build().ok())
        .unwrap_or(HEADER_REGEX.clone());

Edit: I would also consider this to be a diagnostics bug, for at least the first case rustc should have suggested .as_ref(). For the second part, the compiler would ideally have pointed at the `&s[..]` as part of the problem, and the sibling comment has the change you likely want.

Edit 2: to further drive the point home that this should be a bug, this is the current output for a similar case while I was trying to minimize this:

    error[E0308]: mismatched types
      --> src/main.rs:10:22
       |
    10 |         .map(|s| bar(s));
       |          ---         ^ expected `&Struct`, found struct `Struct`
       |          |
       |          help: consider using `as_ref` instead: `as_ref().map`

https://play.rust-lang.org/?version=stable&mode=debug&editio...

[devit]

The latter change introduces an unnecessary clone(). The correct approach is to instead introduce a variable so that headerRegex can keep being a reference to either that variable or to HEADER_REGEX.

[ahelwer]

Can you expand on how this stops a clone from happening?

[devit]

If you don't call clone() then a clone doesn't happen...

[ahelwer]

Ah, I didn't see the extra clone. I thought it was something to do with that additional variable you defined. I guess you need to do that or else you have to do a clone.

[shepmaster]

> into an `Option<&str>` from an `Option<String>`

`Option::deref` is a good choice here.