[estebank]

If you don't mind sharing snippets for the diagnostics you couldn't figure out, I would love to see them in case we can mechanically suggest the appropriate code, but in general a well placed .clone(), .cloned() or .collect() will be what you want. Likely what's happening is you're iterating over a sequence (you can think of options as a sequence of only one iteration as well), but that iterator is over borrows of the sequence's data, instead of owned values. If you try to return that outside of its enclosing function, you will see that error.

As for a "borrow cow", you probably want to read https://doc.rust-lang.org/std/borrow/enum.Cow.html and https://deterministic.space/secret-life-of-cows.html. For what you are doing, it is an optimization to avoid unnecessary clones that is absolutely not required: make your code work with String and worry about Cow<'_, str> later.

[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.

[loa_in_]

I have to agree that at the beginning those unfortunate names (Arc, Cow) confused me more than they should, given that I know perfectly well what Copy on Write and reference count means.

If only it was spelled CoW and ARC I would very quickly grasp it (probably).

But it's an extremely minor inconvenience and arguably makes the whole thing very memorable