Lint dangerous uses of shadowing

[oli-obk]

We have three shadowing lints

• shadow reuse
  • detect let x = consuming_operation(x);
• shadow same
  • detect let x = &x;
• shadow unrelated
  • detect let x = ...; let x = y;

which all try to address shadowing issues. Unfortunately enabling either of these lints will also lint idiomatic and readable code. We should find a way to write a lint that only detects actually problematic cases of shadowing.

All of the cases below are even more prominent if you include mutable bindings, as it becomes very hard to distinguish which variable is changed by a x = ... statement.

Shadowing in a small scope

let x = foo;
....
{
    let x = bar;
    ...
    use(x);
    use1(x);
}
use2(x); // at this point we see the `use1(x)` above and
// may assume these two `x` are related

Shadowing in match arm patterns

#2890

let x = foo;
...
match bar {
    Some(x) => use(x),
    None => use(x),
}

Shadowing in conditional early aborts

probably not as problematic as the first case mentioned, but would get caught by a naive implementation of the first case.

let x = foo;
....
if meh {
    let x = bar;
    ...
    use(x);
    use1(x);
    return;
}
use2(x); // at this point we see the `use1(x)` above and
// may assume these two `x` are related

[ghost]

'Shadowing in a small scope' and 'Shadowing in conditional early aborts' are only an issue if the foo and bar are unrelated. So those lints would have to check that foo and bar are unrelated first, right?

I don't get why 'Shadowing in match arm patterns' is a problem. The variable and the one it shadows are always related. If that's OK in other places why is it a problem in a match arm?

[llogiq]

Look at the example again: The x from the outer scope used in the None arm needs not be related to the x from the Some arm.

[dongleiw]

Why do we need shadowing? it could destroy code readability.

[jayaddison]

From the perspective of writing code that is as easy as possible for both humans and machines to understand, I'd agree with @dongleiw that shadowing may be intrinsically risky.

I understand the desire to use shadowing for temporary variables, where the programmer does not want to create and keep track of additional names.

However those situations seem like they should be relatively rare, and shadowing introduces a possible source of subtle bugs (think: hours trying to determine why the value of x is 10 and not 11, in a case where the type has not been changed), so the ability to flag them as lint warnings could be useful.

(this might be a radical opinion; I'm new to rust, so it's possible there are more reasons that justify shadowing that I'm yet to learn)

[kvark]

I have similar thoughts about Naga/wgpu codebases. We want to disable most shadowing as dangerous, but leave a class of cases that are fine. More concretely, I think shadowing is fine when it's both related and having a different type. I.e. this one is not good:

let x = x +1;

But this one is totally fine:

if let Some(x) = x {
  // do something
}

Unfortunately, the current set of lints does not allow us to have that behavior. Please consider adding a check for "same type" when shadowing.

[KutnerUri]

yea, this seems weird - shadowing seems like a way to get around immutability.
(I get that it's subtly different because of the limitations on mut)

Yes, it is useful when processing / parsing a variable.
But no, I don't think it should be allowed because the chance of accidentally shadowing a variable I didn't notice existing.

[llogiq]

I personally think that with suitable highlighting, IDEs can greatly reduce the trouble that shadowing brings. On the other hand, re-using a suitable name can be the best option where other names are clunky.

[ojeda]

The Linux kernel hit a shadowing-related bug: https://lore.kernel.org/rust-for-linux/[email protected]/

This case is currently linted by shadow_reuse. However, as mentioned above and elsewhere, shadow_reuse lints for too many things.

The linked #2890 above would have caught the bug. That issue was closed because the shadow_* lints catch it now, but they catch many other cases than just match arms.

If we enable the shadow_reuse lint in our kernel crate, then we would get the following cases. First, a bunch of let ... = ones:

let old = unsafe { old.unwrap_unchecked() };
let val = unsafe { Pin::new_unchecked(val) };
let slot = slot.cast::<T>();
let mut item = unsafe { ListLinks::fields(T::view_links(item)) };
let ptr = ptr.cast::<Self>();
let page = NonNull::new(page).ok_or(AllocError)?;
let node = RBTreeNode { node };
let neighbor = neighbor.as_ptr();
let node = KBox::into_raw(node.node);
let ptr = ptr as *const ArcInner<T>;
let ptr = unsafe { ptr.byte_sub(val_offset) };
let ptr = unsafe { ArcInner::container_of(ptr) };
let jiffies = jiffies.try_into().unwrap_or(MAX_SCHEDULE_TIMEOUT);
let out = unsafe { &mut *(out as *mut [u8] as *mut [MaybeUninit<u8>]) };
let ptr = ptr as *mut Work<T, ID>;
let ptr = unsafe { T::work_container_of(ptr) };
let work_ptr = unsafe { Work::raw_get(work_ptr) };
let init = unsafe {
    pin_init_from_closure(|slot| init.__pinned_init(slot).map_err(|e| Error::from(e)))
};
let tag_set = core::mem::size_of::<RequestDataWrapper>()
    .try_into()
    .map(|cmd_size| {
        bindings::blk_mq_tag_set { ...
            nr_maps: num_maps,
            ..tag_set
        }
    });

These seem reasonably OK, and the original binding becomes inaccessible "vertically". Of course, one can still make mistakes moving code up and down within the function.

Then others:

match Self::try_from_arc(arc) {
    Ok(list_arc) => Some(list_arc),
    Err(arc) => Arc::into_unique_or_drop(arc).map(Self::from),
}

let current = match (prev, next) {
    (_, Some(next)) => next,
    (Some(prev), None) => prev,
    (None, None) => {
        return (None, node);
    }
};

let ptr = match ptr {
    Some(ptr) => {
        if old_layout.size() == 0 {
            ptr::null()
        } else {
            ptr.as_ptr()
        }
    }
    None => ptr::null(),
};

// The buggy one.
match len.checked_mul(core::mem::size_of::<T>()) {
    Some(len) if len <= ISIZE_MAX => {
        Ok(Self {
            len,
            _phantom: PhantomData,
        })
    }
    _ => Err(LayoutError),
}

For these, I think on our side we could easily enable a lint that would trigger in match cases (or even in all non-let ... = cases), since there are only a few cases that could easily be renamed. I think that would be an improvement already.

Moreover, we could likely also enable it in the let ... = nested cases mentioned in OP as well. That is, we could just allow "top-level let ... =" cases.

The idea mentioned in a previous comment about linting only when the type does not change sounds interesting (alone or on top of other filters). In our case, it would have still caught the bug, and would not have linted in most of the other cases.

Another way could be avoiding linting in cases that would not have compiled anyway, e.g. if the shadowed value has been consumed, like in our first match case above, thus no ambiguity.

[llogiq]

@ojeda what kind of distinction would you suggest we make between the cases you want linted and the cases that are OK in your book?

[ojeda]

@llogiq Sorry, I am not sure what you mean -- there are several approaches mentioned above that could be useful to filter what a lint like shadow_reuse currently does:

• Linting only for a subset of expressions/statements (e.g. only matches, everything except (possibly top-level) let ... =s...).
• Linting only when it is the same type.
• Linting only when it would still compile.
• A combination of the above.

But I am not sure which one is best -- it would be a good idea to try with some popular crates out there to figure out a good threshold.

Allowing existing projects to configure the lint would help them enable it (or implementing it as new finer-grained lints).

From what I saw in the kernel from that quick look, I think something like "same type" (or "would still compile") plus ignoring top-level let ... =s would already restrict the lint a lot.