Nim: No compiler warnings when shadowing is encountered

[johnnovak]

(edits: grammar)

The following compiles just fine without any warnings about variable shadowings. I think this is a problem because it can be a source of subtle bugs that are just a waste of time to debug. A simple warning about shadowing could at least point the programmer in the right direction, or even better, why not just disallow shadowing altogether? (I understand this would break lots of existing code though, but I think it's safer still)

Example

# Global scope test
var a = 1
if true:
  a = 2
  echo a
  let a = 3
  echo a
  #a = 4   # would result in a compile error, OK

echo a

# Proc scope test
echo "----------------"

proc test() =
  var a = 1
  if true:
    a = 2
    echo a
    let a = 3
    echo a
    #a = 4   # would result in a compile error, OK

  echo a

test()

Current Output

3
2
----------------
2
3
2

Expected Output

Compiler warnings (or possibly even errors) about variable shadowing.

Possible Solution

• I would strongly prefer a compiler warning by default whenever a variable shadowing is detected (or at least the introduction of an optional compiler flag for it).
• It would be even better to ban shadowing altogether and always just thrown an error, or at least introduce a compiler flag to optionally enable strict shadowing checks.

Additional Information

Nim Compiler Version 1.0.2 [Windows: amd64]
Compiled at 2019-10-22
Copyright (c) 2006-2019 by Andreas Rumpf

git hash: 193b3c66bbeffafaebff166d24b9866f1eaaac0e
active boot switches: -d:release

[mratsim]

Note that result variable are special cased and the compiler generates a shadowing warning for them

[johnnovak]

@mratsim That's good to know, that's something at least, but it doesn't help with the general shadowing problem.

[narimiran]

From what I know, stuff like:

proc foo(a: int): int =
  var a = a
  ...

is idiomatic Nim.

[johnnovak]

@narimiran I still don't like it :)

[Araq]

It would be even better to ban shadowing altogether and always just thrown an error, or at least introduce a compiler flag to optionally enable strict shadowing checks.

Seems to ignore the fact that Nim has a macro system and sometimes a macro injects symbols.

[bluenote10]

Personally, I even prefer Rust's take on this to fully support shadowing, even in the same scope -- a feature I was sometimes missing in Nim. It is quite useful for input parsing / minor data type conversions: You don't have to come up with a slightly different variable name e.g. by adding some kind of type suffix to a variable because it is already in scope. With a proper IDE the type can always be looked up anyway.

[mratsim]

I'm surprised Rust allows changing the type of the shadowed variable. The purpose of shadowing is reuse the same name in an inner scope but for me reusing the name also means that the usage will be only slightly different, i.e. if it's only slightly different it makes sense to be the same type.

[johnnovak]

@Araq True, I haven't written many macros yet. Maybe banning shadowing would be a bit too strong, I agree. But having something like -Wshadow would be very handy, even if it's off by default.

[disruptek]

A style warning when the type changes seems to strike the right balance.

[Araq]

Shadowing is also useful to prevent bugs:

let x = x.strip # cannot accidentically use the x that still has the annoying whitespace.

And that's another reason why Nim and Rust support it.

[bluenote10]

Other benefits of local scope shadowing are:

• Mutability control

let x = 42
# immutability guaranteed here

var x = x
# make x is mutable, but with limited scope

let x = x
# immutability guaranteed again

• Working with Option/Result types: They wrap a Foo and in many cases a wrapped Foo is still best called foo. Without shadowing the code need lots of variable siblings like let foo = foo_opt.unwrap() to avoid clashes. There is no point in repeating their optional nature in variable names all the time, it's expressed by the type already.

Prohibiting shadowing enforces Hungarian notation, possibly even with mutability suffices.

[johnnovak]

@bluenote10 @araq The above use cases are fine and I can see the benefits. I haven't used shadowing this way before, so I've learned something.

I'm fine with not having warnings if the shadowing is happening _in the same scope_. My original issue was with a name in the inner scope shadowing something else in the _outer scope_. That can be a source of bugs, and in fact it was in my case.

I'd suggest the following (and I retract my previous wish to ban shadowing altogether):

• If shadowing happens in the same scope, no warnings
• If we're shadowing something that exists in the outer (or global) scope, issue a warning by default (or at least introduce a compiler flag to enable it)