What E to use in stubs for Result<T, E>?

[petertseng]

In #269 we decided that exercises should have stubs. In #371 we further decided that the provided stubs should compile.

For all exercises that have function(s) expecting a Result<T, E>, this poses a question: What type should go in the stub for E?

• Use (), but have a comment explaining that the type can be changed. Currently used in https://github.com/exercism/rust/blob/master/exercises/react/src/lib.rs (rather too late, I would think).
• Use (), with no additional explanations.
• Use a nonsense placeholder such as PUT_YOUR_ERROR_TYPE_HERE. Not yet used in this track, because it would cause the stub not to compile. We are able to mark stubs as allowed to not compile (https://github.com/exercism/rust/blob/master/exercises/decimal/.meta/ALLOWED_TO_NOT_COMPILE) but it should be used sparingly.
• Use a domain-specific error type that is already defined for the student. Currently used in https://github.com/exercism/rust/blob/master/exercises/forth/src/lib.rs.
• Use a domain-specific error type, but stub out the definition. For example, perhaps we would have a BowlingError but leave BowlingError as simply pub struct BowlingError {}.
• Use &'static str with the idea that it's to be used as an error message. Currently used in https://github.com/exercism/rust/blob/master/exercises/variable-length-quantity/src/lib.rs
• Use an arbitrary choice, decided by the contributor who contributes the stub.
• Your suggestion here.

I'm starting to shy away from using str as an error type since it is difficult to programmatically inspect, but now need to think about which of the other options I prefer. Perhaps my reason is bogus and using str is okay (let me know!). A contributor once also said this as a tip, but did not provide reasoning, so I cannot evaluate the tip. #259 (comment)

Need to think about the goals: Are we trying to teach something via these stubs? Or is it precisely the minimum code needed to get it to compile?

[petertseng]

I think the goals are:

• If stubs compile, students can run cargo test without having to reconstruct expected signatures from each test file.
• Since it's better to let CI enforce the fact that stubs compile, ALLOWED_TO_NOT_COMPILE should be used sparingly as possible.
• Keep the stubs minimal so as to not overly constrain the solution. https://github.com/exercism/docs/tree/master/language-tracks/exercises#exercises tells us "Exercises should not enforce a single way to solve the problem, if possible".
• Try to avoid appearing to endorse bad habits, because students may decide to use this track to be more familiar with Rust.

I think that means this decision process:

• If it is important for tests to detect which specific error occurred (such as in forth), use a domain-specific error type. Since the tests expect it, we will have to define it in the stub so that it compiles.
• Otherwise, use (). A comment explaining that the error type can be changed is required for the first N exercises that use Result; it is optional for all others. Choose an arbitrary value for N, perhaps 2 or 3.

[petertseng]

In the future, we might consider using the ! type. Currently it is experimental.

#![feature(never_type)]

fn f() -> Result<usize, !> {
    Ok(5)
}

fn main() {
    println!("Hello, world! {:?}", f());
}

#[cfg(test)]
mod test {
    // Test doesn't run on playground, that's OK, I just need it to compile.

    #[test]
    fn t() {
        assert_eq!(f(), Ok(5));
    }
}

[coriolinus]

I think my order of preference for this would be a domain-specific error type, then!, then &'static str. Reasoning:

• The whole point of using Result instead of Option is that there are a few distinct errors which may occur, and it will be useful for calling code to be able to distinguish between them. If this is the case, having the student create the Enum themselves is just busywork; it requires very little thought.
  • We might consider looking through student exercises and compiling some statistics: what do students tend to actually use, when the tests require a Result but no stub is provided?
  • I suspect that many of them just use a &'static str and move on, which we all agree is suboptimal. If that is in fact true, then we'll be promoting good habit transfer by giving them an Enum to use.
• If we choose not to use a custom domain-specific error type, then it should be something like ! which clearly needs to be replaced with student code. This doesn't eliminate or even reduce the need for explanatory documentation, but it complements it nicely. Of course, we'd need to wait until that feature joined stable.
• We might consider defining pub type ProblemError = &'static str // REPLACE THIS TYPE and just using that. It makes it a bit more explicit that errors may, but should not, be simple strings.
• If all else fails, using an &'static str at least encourages the student to write something human-readable about what went wrong. It stands out as a placeholder type about as well as () does, which is to say, not very well. It would require some documentation.

My preference by far is the domain-specific types, though. I acknowledge that this involves moving some of the implementation burden from the student to the maintainers. I submit as response to that criticism that the point of an exercise is very rarely to introduce the Result monad and force the student to design their return types from scratch, and we can special-case the exercises for which that is in fact the point.

[petertseng]

My first priority here will be to change https://github.com/exercism/rust/tree/master/exercises/variable-length-quantity then, as its stub uses a str (not preferred by discussion participants).

We can continue to develop ideas about what to use for E here. When we think we have figured things out, we can close the issue. Possibly adding what we decide to the README beforehand.

Here is another facet to think about: In the case where multiple functions may return different kinds of errors, shall we encourage using the same E or a different E for each?

We'll use react as an example. In https://github.com/exercism/rust/blob/master/exercises/react/src/lib.rs we see that different functions on react may result in different kinds of errors.

• create_compute: Error if an input cell doesn't exist.
• set_value: Error if called on a compute cell. Error if called on a nonexistent cell.
• add_callback: Error if called on a nonexistent cell.
• remove_callback: Error if called on a nonexistent cell. Error if called on a nonexistent callback.

Each decision does have a disadvantage.

If using a single domain-specific error type and have all four functions use it as E, callers may feel the need to deal with branches that won't happen. For example, I wouldn't want to force a caller of create_compute to deal with the NonexistentCallback case, since that won't happen.

If using one error type for each of those four functions, then maybe users of the API feel it is too many error types to deal with.

If it makes sense for the track's stubs to favour one approach over the other, which one and why? Or, we may remain silent on the matter, depending on how soon ! becomes stable.

[coriolinus]

React is something of a special case, because it does have so many error types; that's unusual for exercism. That does make it useful as a test of principles.

My inclination in this case is to ask what a well-engineered piece of software would look like if I were developing this for a client. The answer in this case is to have a custom error type for set_value and remove_callback, as each of them can fail in more than one distinct way, and replace the Results with Options in create_compute and add_callback, as each of those can only fail in a single distinct way.

Given that we only really need two distinct error types, are those too many to deal with? I don't think so, but there's reasonable scope to disagree here. In that case, it may be time to introduce error-coalescing libraries such as the failure crate.

[shingtaklam1324]

If a custom error type is to be used, it would make sense to impl std::error::Error(docs) for it, which allows for better formatting of error messages.

[coriolinus]

std::error::Error takes some amount of busywork to implement. Should the track maintainers implement it? If so, why? The students aren't expected to catch these errors or send descriptions to end users; they're expected simply to raise the appropriate errors in the appropriate conditions. Should the students implement it? If so, why? The test framework that Exercism provides doesn't care about the legibility of the error messages. I could see the benefit to someone introducing a new exercise whose purpose was to introduce the Error trait for its coalescing properties, because that is something useful to know about. (It could also be a fun design challenge to write an exercise which might be solved either by using the std Error trait or by using the one from the Failure crate; the latter may be of interest to students because trait implementations can be derived.) However, I don't believe that we should expect anyone to implement this trait for their error messages as a matter of course throughout this track.

…

On Sat, Mar 10, 2018 at 4:09 AM, Shing Tak Lam ***@***.***> wrote: If a custom error type is to be used, it would make sense to impl std::error::Error for it, which allows for better formatting of error messages. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#444 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHdeTrcb9v7vKU_37rHyok88KUCSHY78ks5tc0P2gaJpZM4SbBcW> .

[petertseng]

The stubs with Result are:

$ git grep -l 'Result' exercises/*/src/lib.rs
exercises/all-your-base/src/lib.rs
exercises/forth/src/lib.rs
exercises/ocr-numbers/src/lib.rs
exercises/react/src/lib.rs
exercises/variable-length-quantity/src/lib.rs

Of these: variable-length-quantity has already been changed to a domain-specific error, forth has always had a domain-specific error, I sent out PRs for react and ocr-numbers. all-your-base coming soon.

After that, we'll look at stubs as they get contributed, and perhaps think of any other exercises whose tests may want to look for specific errors, rather than the generic is_err

[petertseng]

git grep Result exercises/*/src/lib.rs shows me a satisfactory result for all E in stubs, which is my intended scope of this issue. I'd thus like to close this issue.

Please feel free to refer back to this issue when deciding what should happen in any stubs that get added in the future.

There are still some tests containing is_err that could instead be changed to Option (if there is only one failure mode) or a domain-specific error (if there are multiple). IMO these are relatively low priority so I'll be focusing my attention elsewhere for the time being, but I invite interested parties to look at that and see if they disagree.