parallel-letter-frequency: Use &str with lifetime, not String

[petertseng]

The previous version had a comment that explained a need for copying the
&str into Strings, but this can be avoided.

The disadvantage is now that this function can only be invoked with
&'static str, which means that it can't (for example) accept arbitrary
user input as part of a command-line program. So it works for our style
of testing (all our tests do indeed use &'static str) but is
unrealistic code.

---

Reviewers should consider the disadvantage carefully before deciding whether to merge. Of course, this is only the example and doesn't affect the test code. On the other hand, it could be better to keep the example code realistic.

In the extreme case, we might even choose to add a test that creates non-'static strings, so that using &'static str is not possible (I do not advocate for this, but y'all can if you like)

[IanWhitney]

I think I'm fine with this. Declaring static lifetimes is a good thing to see, assuming anyone looks at the examples.

[ijanos]

I don't like this. Who will look at examples? I guess some people who want to simply "cheat" and those who are stuck and want to peek at a solution. It may look like a function accepting only 'static str is a good solution to these people, while in the real world it is rarely useful nor a good pattern to use.

If we merge this at least put a warning comment before the function... and by doing that we are back to the original problem with the comment explaining things.

[IanWhitney]

@ijanos Do you have more details on why it's not a good pattern? I would like to know more!

From my experience, I use the examples a lot when learning a new language. I usually know how to solve the problem, but I don't know the language's syntax or idioms. So I'll look at the example code to figure out what I need to use. I don't know if that's common, but this use case is one of the reasons we settled on the goal of having our example solutions be, well, exemplary. We want them to use reasonably idiomatic, well-written Rust.

[petertseng]

The problem is explained shortly with: It is now impossible for this function to take a &str with non-static lifetime, and this is a very harsh restriction.

I can try to come up with example code soon, but not now.

[petertseng]

No intent to merge - better close now to avoid any mistakes. I do still plan to get back to you with a reason why it's not going to get merged.

[ijanos]

@IanWhitney Accepting a static string slice basically means accepting only strings available at compile time. Imagine using a library that has a restriction like that, you cannot call it on anything you don't know during compile, like data from a file, data downloaded from somewhere or user input.

[petertseng]

Reopening to show Travis. Please do not merge.

[petertseng]

The commit 8fe6c5b should show the restrictiveness of &'static str. I couldn't even pass the result of a format! into frequency. You will see that the build fails: https://travis-ci.org/exercism/xrust/builds/189277410

Failure reproduced here for posterity:

   Compiling parallel-letter-frequency v0.0.0 (file:///tmp/exercises/parallel-letter-frequency.8GszVBeYDe)
error: `hi` does not live long enough
  --> tests/parallel-letter-frequency.rs:46:29
   |
46 |     frequency::frequency(&[&hi], 4);
   |                             ^^ does not live long enough
47 | }
   | - borrowed value only lives until here
   |
   = note: borrowed value must be valid for the static lifetime...
error: aborting due to previous error
error: Could not compile `parallel-letter-frequency`.

[IanWhitney]

Ok, I think I see what's going on here now. My understanding was that all &str were really &'static str under the hood. That was my understanding from the book.

So, based on that belief, I thought these two lines are identical

pub fn frequency(texts: &[&str], num_workers: usize) -> HashMap<char, usize> {

pub fn frequency(texts: &[&'static str], num_workers: usize) -> HashMap<char, usize> {

But the non-lifetime version is allowing Rust to do some implicit conversion of

let hi = format!("Hello, {}!", "world");
&[&hi]

into something it can think of as a str. Right?

But one you say "I only accept static str" then the compiler can't do the conversion since it can't satisfy the lifetime requirements.

🦆

[petertseng]

But the non-lifetime version is allowing Rust to do some implicit conversion of

let hi = format!("Hello, {}!", "world");
&[&hi]

into something it can think of as a str. Right?

format! creates a value of type String. If we take a reference to it, deref coercion allows it to be used wherever a &str is required, because String implements Deref<Target=str>

---

But one you say "I only accept static str" then the compiler can't do the conversion since it can't satisfy the lifetime requirements.

I believe that's a good summary. My understanding of the full story is below, if it is unclear to anyone. Please correct if I made any errors.

the non-lifetime version

The version without lifetime annotations follows the rules of lifetime elision which should mean that it is equivalent to:

pub fn frequency<'a, 'b>(texts: &'a [&'b str], num_workers: usize) -> HashMap<char, usize>

The only requirement on texts is that frequency may not outlive the slice nor the strings contained within it. Put another way, the minimum lifetime of the slice and strings is the scope of frequency. This requirement is the standard rule for borrowing. In this case, no additional constraints have been imposed by the lifetimes (or absence thereof).

At this point, let's see why we have to do parts[i].push(line.to_string()) instead of just parts[i].push(line). We have to examine the type of thread::spawn - its argument (normally a closure, but anything implementing FnOnce) must at least have 'static lifetime (as long as the program). This is because the threads can outlive their spawner.

The strings in texts are not guaranteed to have 'static lifetime. The only constraint imposed on them is that they must live as long as frequency. Since they are not guaranteed to have 'static lifetime, part (which is a Vec<&str> containing them) cannot be closed over by the closure passed to thread::spawn.

However, if part were a Vec<String>, there are no references (as long as we use a move closure), so any lifetime constraint can be satisfied (they only affect references). So that's why the example used Vec<String> before this PR.

Aside: If we wanted to have the type system prove that the threads terminate before their spawner, that seems a bit tricky! But it's been done! I have not investigated this crate to see what it is doing.

---

What happens in this PR? If I understand lifetime elision properly, what I wrote should be equivalent to:

pub fn frequency<'a>(texts: &'a [&'static str], num_workers: usize) -> HashMap<char, usize>

For the texts slice, the slice is still required to only live for as long as frequency does (again, standard borrow rule, no additional constraint imposed by lifetime). But now the strings are required to live for lifetime &'static, as long as the program. So, because frequency now imposes this requirement on its callers, frequency is now at liberty to allow closures passed to thread::spawn to close over part, which is a Vec<&'static str> containing some string slices from texts.

But imposing the requirement on the callers leads to unrealistic code, and would not make a good example for our students. As I showed, references of arbitrary Strings (such as those created by fmt! or any other function) are no longer acceptable as arguments of frequency, because those Strings do not live for &'static time.