5 ms·

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 cod

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 https://doc.rust-lang.org/std/borrow/enum.Cow.html and https://deterministic.space/secret-life-of-cows.html 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 6y ago
Sure! So I'm trying to implement this feature: https://github.com/tree-sitter/tree-sitter/issues/982#issuecomment-797812264 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-separator 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 6y ago
  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 6y ago
    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 6y ago
  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&edition=2018&gist=d1744a6e660e39aaff47bc6aec1dae4f https://play.rust-lang.org/?version=stable&mode=debug&editio...
loa_in_ 6y ago
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

ahelwer 6y ago

Sure! So I'm trying to implement this feature: https://github.com/tree-sitter/tree-sitter/issues/982#issuecomment-797812264 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-separator 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 6y ago
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 6y ago
  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 6y ago
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&edition=2018&gist=d1744a6e660e39aaff47bc6aec1dae4f https://play.rust-lang.org/?version=stable&mode=debug&editio...

loa_in_ 6y ago

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