Name Shadowing

[llogiq]

Some people (myself included) think that name shadowing makes code harder to reason about.

Now of course there is a continuum of shadowing operations:

• shadow_same: The name is re-bound to the same data (e.g. let mut x = &mut x; for mutable re-bind)
• shadow_reuse_change: The name is re-used in the bound expression (e.g. let x = x + 1;)
• shadow_foreign: The name is re-bound without even using the original value

We could create lints for all three groups, plus a shadow_reuse lint group that combines shadow_same and shadow_reuse_change and a shadow lint group that combines all three.

Now for those lints, we want to make the macro check more strict in that we do not check within macro expansions at all.

I think (but am open to discussion) that the shadow_foreign lint should be Warn by default, while the others should be Allow. This won't disturb people too much but still makes for fairly readable code. Organizations or projects can then opt to Deny all shadowing, or whatever suits them.

[Manishearth]

I think shadowing is quite liberally used in Rust, so we should tread lightly here.

I think your proposed scheme should work well.

I think especially in nested libsyntax code it's quite common to shadow and re-shadow stuff like ty and expr because they end up being used a lot. But those users can just disable the lint.

[llogiq]

I know that many don't see shadowing as a problem. But I maintain that it does make code harder to visually parse than strictly necessary (where does that foo come from again?). And often, reusing a binding stems from lazyness to think up a more specific name.

Btw. scopes are a good antidote to shadowing – if a binding is out of scope, it cannot shadow anything. Thus, reducing scopes also reduces shadowing.

I'd really like to see how many warnings libsyntax would generate with shadow_foreign turned on. 😄

[Manishearth]

Sounds good to me.

[llogiq]

I see you added [T-middle]. Is there something that would ease building this?

I figure it would be possible to implement this by walking the AST of any fn and keeping a chain of sets of local bindings (one for each scope), but perhaps there is an easier solution?

[Manishearth]

I guess an AST walk would work. I was thinking along the lines of ExprUseVisitor but that's not exactly what we want anyway.

[llogiq]

@Manishearth Now I have a very strange compiler error:

error: duplicate specification of lint shadow_same
error: duplicate specification of lint shadow_reuse
error: duplicate specification of lint shadow_foreign

I'm quite sure I have only one definition of each lint. Perhaps a rustup --channel=nightly will fix it.

[birkenfeld]

This comes from get_lints() returning the same lint name(s) for multiple passes.

[llogiq]

Ah, I had registered the lint twice. Thanks, @birkenfeld.