CODEWITHSUNDEEP
enumsrustrefactoring
Jonathan Jacobson
Jonathan Jacobson

Reputation: 1518

Logically yet illegaly uninitialized variable in Rust

I'm relatively new to Rust. This question turned out to be pretty long so I'll start with the bottom-line: Which solution do you prefer? Do you have any ideas or remarks?

My code does not compile because lines lines 6 (prev = curr) and 12 (bar(...)) use variables which the compiler suspects are possibly uninitialized. As the programmer, I know that there is no reason to worry because lines 6 and 12 do not run during the first iteration.

let mut curr: Enum;
let mut prev: Enum;

for i in 0..10 {
    if i > 0 {
        prev = curr;
    }

    curr = foo();

    if i > 0 {
        bar(&curr, &prev);
    }
}

I understand that there's a limit to what you can expect from a compiler to know. So I came up with 3 different ways to answer the language's safety restriction.

1) Initialize and stop thinking too much

I could just initialize the variables with arbitrary values. The risk is that a maintainer might mistakenly think that those initial, hopefully unused, values have some important meaning. Lines 1-2 would become:

let mut curr: Enum = Enum::RED; // Just an arbitrary value!
let mut prev: Enum = Enum::BLUE; // Just an arbitrary value!

2) Add None value to Enum

Lines 1-2 would become

let mut curr: Enum = Enum::None;
let mut prev: Enum = Enum::None;

The reason that I dislike this solution is that now I've added a new possible and unnatrural value to my enum. For my own safety, I will have to add assertion checks and match-branches to foo() and bar(), and any other function that uses Enum.

3) Option<Enum>

I think that this solution is the most "by-the-book" one, but it makes the code longer and harder to comprehend.

Lines 1-2 would become

let mut curr: Option<Enum> = None;
let mut prev: Option<Enum> = None;

This time, None belongs to Option and not to Enum. The innocent prev = curr statement would become

prev = match curr {
    Some(_) => curr,
    None => panic!("Ah!")
};

and the naive call to bar() would uglify to

match prev {
    Some(_) => bar(&curr, &prev),
    None => panic!("Oy!")
};

Questions: Which solution do you prefer? Do you have any ideas or remarks?

Upvotes: 10

Views: 1558

Answers (4)

Sven Marnach
Sven Marnach

Reputation: 601401

For what it's worth, a simple way of fixing your code is to pull the first iteration out of the loop. Because of the if statements, the only thing the first iteration does is calling foo(). Additionally, you can move the assignment to prev to the end of the loop, so you can scope curr to the inside of the loop:

let mut prev = foo();
for _ in 1..10 {
    let curr = foo();
    bar(&curr, &prev);
    prev = curr;
}

Note that I changed the range to start at 1 instead of 0, since we pulled the first iteration out of the loop.

I don't think this approach is better than the one in the accepted answer, but I do think it's quite simple and easy to understand.

Upvotes: 2

Peter Hall
Peter Hall

Reputation: 58695

Where possible, avoid using too many mutable variable bindings in Rust, especially in loops. Managing state within a loop using mutating variables can make code harder to understand and lead to bugs. It's usually a red flag to see a counter variable in a for loop, which can very often be replaced with an iterator over values instead.

In your case, you can create an iterator over the values produced by foo, and pass them in pairs to bar, using tuple_windows from the itertools crate:

use itertools::Itertools as _; // 0.9.0
use std::iter::repeat_with;

for (prev, curr) in repeat_with(foo).tuple_windows().take(9) {
    bar(&curr, &prev);
}

Note that you can't make a mistake here and forget to set prev or curr, and future maintainers can't accidentally swap two lines and mess it up either.

This style is very succinct and expressive. For example, the way I've written the snippet above emphasises that bar will be called 9 times. You could also modify it slightly if you preferred to emphasise that foo will be called 10 times:

for (prev, curr) in repeat_with(foo).take(10).tuple_windows() {
    bar(&curr, &prev);
}

Upvotes: 12

Zorf
Zorf

Reputation: 6464

Actually, the logic that you are expressing is what is called a “fold” and you could be using a fold instead of a loop.

What one could do is:

(0..9).map(|_|foo()) //an iterator that outputs foo() 9 times, not 10
   .fold(foo(), |prev, curr| {bar(&curr, &prev); curr});

The business end is of course the line with .fold in it. What a fold does is that it takes a single argument for an initial value, calls a binary function on that initial value and the current value the iterator produces, and then uses the result as the new initial value for the next iteration. I have simply used a function here that is called for the side-effect as you have, and as resulting value use the curr value, which is thus used as the initial value of the next iteration, and serves as prev.

The final value returned by this expression is the last curr, which can of curse be ignored.

https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.fold

Upvotes: 8

Jeff Garrett
Jeff Garrett

Reputation: 7383

I am also very new to Rust, but I find the third option clearest. I will add that unwrap() or expect() can be used in place of the match expressions with the same meaning ("I know this is occupied"):

let mut curr: Option<Enum> = None;
let mut prev: Option<Enum> = None;

for i in 0..10 {
    if i > 0 {
        prev = curr;
    }

    curr = Some(foo());

    if i > 0 {
        bar(curr.as_ref().unwrap(), prev.as_ref().unwrap());
    }
}

See the playground. It still has a little extra ceremony to it.

Generally I find longer real life examples of loops in this form hard to follow; the structure of the loop obscures the desired logic. If one can structure it more like an iteration, I find it clearer:

for (prev, curr) in FooGenerator::new().tuple_windows().take(10) {
    bar(&prev, &curr);
}

See the playground.

Upvotes: 3

Related Questions