cognitive-engineering-lab / rust-book

The Rust Programming Language: Experimental Edition
https://rust-book.cs.brown.edu
Other
503 stars 82 forks source link

Ownership inventory #1 question 3 has a misleading (wrong?) answer #155

Open aleksanderkrauze opened 5 months ago

aleksanderkrauze commented 5 months ago

URL to the section(s) of the book with this problem:

Description of the problem:

Question 3 in Ownership inventory #1 asks:

/// Makes a string to separate lines of text, 
/// returning a default if the provided string is blank
fn make_separator(user_str: &str) -> &str {
    if user_str == "" {
        let default = "=".repeat(10);
        &default
    } else {
        user_str
    }
}

Of the following fixes (highlighted in yellow), which fix best satisfies these three criteria:

  1. The fixed function passes the Rust compiler,
  2. The fixed function preserves the intention of the original code, and
  3. The fixed function does not introduce unnecessary inefficiencies

And gives as an answer:

fn make_separator(user_str: &str) -> String {
    if user_str == "" {
        let default = "=".repeat(10);
        default
    } else {
        user_str.to_string()        
    }
}

With an explanation:

There is no valid way to return a pointer to a stack-allocated variable. The simple solution is therefore to change the return type to String and copy the input user_str into an owned string. However, requiring user_str to be a String would reduce the flexibility of the API, e.g. a caller could not call make_separator on a substring of a bigger string. It would also require callers to heap-allocate strings, e.g. they could not use a string literal like make_separator("Rust").

The most idiomatic solution to this problem uses a construct you haven't seen yet: Cow. The clone-on-write smart pointer would enable this function to return either an owned string or a string reference without a type error.

However, I believe that this is not correct and in fact one of the "distractors" should be preferred instead:

fn make_separator(user_str: String) -> String {
    if user_str == "" {
        let default = "=".repeat(10);
        default
    } else {
        user_str
    }
}

Rationale:

Since make_separator must return an owned String this function must be able to produce owned String from the input as well. In the proposed solution however function hides this fact from the caller, as it is not obvious that it can allocate memory. This contradicts Rust API Guidelines. C-CALLER-CONTROL guideline says that the caller should decide how to provide an owned version of an argument instead of callee. If all that caller has is a reference to the string, it still needs to be cloned. And if caller has an owned string and doesn't need it anymore it can just give an ownership of it to the callee.

The argument about being less flexible could be addressed by applying C-GENERIC guideline and accepting S: Into<String> as an argument (although it probably shouldn't be a proposed answer since this questions comes several chapters before a chapter on generics).

I am not necessarily arguing that this distractor is the best answer, but it certainly is a better one than the one that is expected. If this distractor weren't in the list I would probably say that the expected version is indeed the best solution. But seeing them side by side is really misleading.

Suggested fix:

Either change expected answer, or remove talked above distractor. If the expected answer is not changed, at least mention taking owned string (or even S: Into<String>) as an alternative in the explanations (similarly like you mention Cow).