"Matrix" doesn't test that index zero is handled correctly

Programming Rust
1

The Matrix exercise expects row and column functions that accept a 1-based index, and return the corresponding piece of a matrix, or None if the index is out of range. One simple solution for the row function, assuming the matrix data is stored as a Vec<Vec<u32>>, is as follows:

pub fn row(&self, row_no: usize) -> Option<Vec<u32>> {
    self.data.get(row_no - 1).cloned()
}

This causes unexpected behaviour when 0 is passed as an index:

  • In debug mode, where tests are usually run, 0 - 1 panics.
  • In release mode everything works, but for the wrong reason. 0 - 1 wraps to usize::MAX, which is then out of range and results in None.

This is not the only way to solve this, but other approaches are just as easy to get wrong if index 0 is not considered.

I propose adding two tests to the exercise to check that solutions handle the this case correctly - row(0) == None and column(0) == None. I’ve created a PR for this, but it will cause many past solutions to fail, so let me know if it’s worth making the change anyway.

2

Why is None the correct result for invalid inputs?
We’ve explicitly opted to remove handling of invalid inputs from exercises as that changes the focus from the actual exercise to making them into error handling exercises.

3

This was a decision made when initially bringing the exercise to the Rust track, as seen on GitHub.
The key thing seems to be that because of Rust’s focus on explicit errors as values, making the return type anything other than Option<Vec<u32>> would be unidiomatic Rust. Since this is now the expected behavior in the Rust version of the exercise, it seems strange to allow solutions that handle most invalid inputs but not all. It’s also pretty simple to handle this case - see my updated example solution.

4

Let’s leave it the way it is. This is not worth the churn.

5

Ugh, this is still bugging me. I wrote a little patch with one possible approach. It changes the function signature to just Vec<u32> without Option, making it more clear that the function cannot fail. (because input is assumed to be valid)

I even managed to write some code to keep this backwards compatible with existing solutions that return Option.

I see several options for how to proceed:

  • do nothing
    • pros: existing solutions stay valid
    • cons: confusing, inconsistent, unidiomatic
  • add the proposed tests
    • pros: consistency, idiomatic Rust
    • cons: existing solutions break, further deviation from original exercise design
  • remove Option from the function signature
    • pros: consistency, stay close to original exercise design
    • cons: unidiomatic Rust (? see below)
    • can be done with or without backwards compatibility. (tradeoff with added complexity)

I don’t weigh the argument of “idiomatic Rust” very heavily anymore. It’s idiomatic in general to use Option because in general, input cannot be assumed to be valid. But in cases where it can, the Option makes no sense.

@ErikSchierboom what do you think about this? You always have good inputs about exercise design.

6

I really like that it’s possible to remove Option but stay backwards compatible, and I feel like that option is more appealing than leaving it as is - the exercise should be consistent at least, whether it includes invalid indexes or not.

I still think that for this specific exercise, it does make sense to have the Option return value. This pattern seems to be pretty much universal across Rust data structures, including third-party crates. It’s how the Vec that almost everyone solves this exercise with works. Finally, this exercise is marked as medium difficulty and appears towards the end of the track, so I don’t think the added solution complexity is unreasonable.