How to avoid accidentally redeclaring a variable inside a code block?

help
1

Please forgive me if that's a newbie question, but let's imagine I'm modifying a (not so small) method that someone else wrote or that I wrote a couple of years ago and I come across the need to declare a variable. I go ahead and do:

let temp = 2;

Great! Now how can I be sure that I haven't erased a previous declared variable with the same name?

Do I have to read the entire method, keep track of all the variable names in my head to then be safe that I'm not declaring a variable that was previously declared accidentally?

That feels unsafe to me. I would expect the compiler to tell me: "Hey! This variable is already taken, please choose another name.

How do you manage this situation in a safe way?

3 Likes
2

  1. Use blocks to scope the new variable to the smallest possible region. This avoids "polluting" the rest of the surrounding code. If the block is too big to see all the pertinent variables at once, make it smaller. If you can't, consider breaking it out into a function.

  2. Don't use vague names like "temp". Name the variable for what it contains. If you have a name collision, that's a strong indication that the new variable is either redundant, or you're confusing matters and shouldn't call it that.

  3. Use clippy if you want to prevent shadowing entirely.

Also, as an aside, shadowing is in no way unsafe by any reasonable definition of safety in Rust.

10 Likes
3

At the risk of sounding preachy, I would strongly reiterate @DanielKeep's first point; the fact that rust allows shadowing is clearly no accident, and it is a feature found by many to be extremely desirable in modern languages. Whether it is a danger or a boon largely comes down to coding style, and as long as most variables are declared close to where they are used, you have a fair bit to gain and little to worry about.

The most obvious place where it is useful is in closures; the majority of closures are simple 1-liners that shouldn't have to worry about context, and there is little reason to call most closure variables anything but x. By using such a name you are making a statement: this variable is really short-lived and you should have no trouble seeing how it is used.

// option 1
test_results.filter(|x| x.iter().all(|&&x| x))
// option 1b
test_results.filter(|t| t.iter().all(|&&c| c))
// option 2
test_results.filter(|test| test.iter().all(|&&case| case))

When I look at #1 I don't "see" variables named x; I focus on the shape and method names. When I look at #2 my eyes must quickly scan ahead to see whether test is used a second time. So while #2 has its merits, in this respect, I think it is also noisy.

In such a short closure, it is very difficult to accidentally shadow something without noticing, as it is often the difference between |x| center + x and |x| x + x.

Notice that rust even permits shadowing within the same scope, i.e. rebinding.

// without shadowing
let iter = vec.into_iter();
let modified = modify(iter);
let doubled = double(modified);

for x in modified { ... }

//-----------
// with shadowing
let iter = vec.into_iter();
let iter = modify(iter);
let iter = double(iter);

for x in iter { ... }

In the second version, you can easily delete one of the function calls or insert another when the logic needs to be revised.

(edit: the for x in modified was both entirely unintentional and yet quite exemplary!)

Caveat: There is one real danger of shadowing, which is when you rename a variable in an outer scope; this is a circumstance where e.g. |x| y + x can turn into |x| x + x without your immediate knowledge. I would exercise a bit of caution using find and replace, and hope that any more sophisticated tooling for refactoring would catch this.

2 Likes
5

I hesitate to call this "rebinding" because that implies the original binding is lost, but that's not true. This is an extremely small semantic nit :smile:

6

But isn't it, at least effectively?

To clarify, when I say "binding" I mean a name-value association. Or perhaps what I rather see is a sort of name-memory association (in some model where we pretend that every local lives on the stack (no registers) and assume that separate stack memory is reserved for every let statement in a function). In these terms, I call it a "rebinding" when a new name-memory association replaces an old one in such a manner that permanently hides it; it cannot cannot be recovered at any later point in the function code

Then, under my definition, I say that:

  • Mutation (including reassignment) does not rebind: let mut x = vec![]; x = vec![];
  • Nested scopes may shadow, but they never rebind since the original bindings are recovered when you leave the scope: assert_eq!(0, { let x = 0; { let x = 1; }; x });
  • Individual loop iterations... erm... technically don't rebind under this definition. (unfortunate, since I would tend to say that for i in xs does create "new bindings" for i each iteration... but then how do I truly define a "binding?" Aaaagh, get outta my head, Socrates!)
  • Successive let bindings do rebind: let x = 0; let x = 0; (the memory slot for the first x can never be accessed again).
7

In my mind, if the original binding is lost...

let x = 5;

{
    let x = 6;
}

// does x exist here?

Shadowing means that when the shadow goes away, the original binding is still there. Re-binding would mean that x would no longer happen, as it would be unbound and then re-bound.

This behavior is important in Rust, it allows for code like this:

let s: String = ...; // (i have a String from somewhere)
let s = &s[..];

Here, we know s will be valid, because the underlying binding, while not accessible, is still there. So the reference still exists.

8

As far as I can see, a binding is not a prerequisite for live data:

fn get_a_string() -> String { "I was never here!".to_string() }

fn main() {
    // this method has no bindings to Strings
    let s: &str = &get_a_string()[..];
    println!("{}", s);
}
2 Likes
9

I am... shocked that this works.

3 Likes
10

As was I! :slight_smile:

2 Likes
11

C++ allows this for const&, as discussed in GotW #88, but I had no idea Rust does anything like this. I even tried it with 1.0.0, successfully, so this is nothing new.

It doesn't have to be "const" like C++ either -- &mut get_a_string()[..] still works. So does let s: &str = &get_a_string(); without slicing.

However, get_a_string().as_str() does not work!

error: borrowed value does not live long enough
 --> <anon>:5:42
  |
5 |     let s: &str = get_a_string().as_str();
  |                   --------------         ^ temporary value dropped here while still borrowed
  |                   |
  |                   temporary value created here
6 |     println!("{}", s);
7 | }
  | - temporary value needs to live until here
  |
  = note: consider using a `let` binding to increase its lifetime
1 Like
12

@cuviper: Most curious! So indexing is the exception, not the rule...

I found this issue:

https://github.com/rust-lang/rust/issues/30127

pnkfelix has an in-depth comment on this describing the cause. I didn't read it too closely yet, but it looks like similar semantics happen for all lvalue expressions (*thing, thing.x, thing[..]), and that the difference is embedded in the grammar.

Edit: Perhaps "lvalue expression" isn't the right term (in the linked discussion, they are referred to as rvalues with "extended lifetimes")

1 Like
13

Thanks for finding that issue! From there, I guess the other one to watch is the tracker for RFC66:

https://github.com/rust-lang/rust/issues/15023

14

@rahlvarsson, I'm with you. This is an awful "feature".

15

Personally I think if one is modifying code down to the scale of a function then yes you do need to read the entire function and understand what is going on before you make any changes. Otherwise how do you know you are making a change that does not break the operation of the function.

Which leads me to think that your "not so small" is the problem here. If they are so long and contain so many temporary variables that you cannot easily keep track of things in your head then those functions are just too long. I would be looking at splitting them up into more manageable, smaller, functions, before worrying about misnaming things.

The feature you are referring to is known as "shadowing". I too was surprised that Rust allows this and thought that it would be a source of problems. However now I see it as quite useful. For example it allows me to have some data as one type, say a JSON string, and then transform it into another type, say a struct contains the data items in that JSON string, and use the same name. I don't have to dream up some new name for the same thing represented in different types. I don't have to add ugly suffixes to names to separate one from another. Eg 'my_thing_buffer' then 'my_thing_string' then finally 'my_thing'.

Meanwhile I have not had any problems caused by shadowing, Rust's type system generally complains soon enough or the compiler finds some other error.

Also personally I don't like to see things like temp in code much. Part from perhaps indicating a short lifetime they are meaningless, they say nothing about the data the hold, there are generally ways to get rid of them.

We should be careful using the words "safe" and "unsafe" in conversations about Rust. The concept of safety has a specific meaning in Rust to do with memory usage. It is not about other logical errors one might make.

1 Like
16

I think this is a edge case of using let.

If you do

let x = &String::from("hello world");

You have on the surface a value, "hello world", that lives without a owner/binding. But in reality rust inserts an owner. Something like this

let x_val = String::from("hello world");
let x = &x_val;

But this is only for convenience. It doesn't work in a lot of other cases. For example when reasigning.

fn main() {
    let mut x = &String::from("hello world");
    x = &String::from("hello shmorld");
    dbg!(x);
}

error[E0716]: temporary value dropped while borrowed
 --> src/main.rs:3:10
  |
3 |     x = &String::from("hello shmorld");
  |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- temporary value is freed at the end of this statement
  |          |
  |          creates a temporary which is freed while still in use
4 |     dbg!(x);
  |          - borrow later used here
  |
help: consider using a `let` binding to create a longer lived value
  |
3 ~     let binding = String::from("hello shmorld");
4 ~     x = &binding;
  |
17

Please don’t revive very old topics.