Warnings on variable shadowing

[evancz]

Follow up to #829 so we can warn folks when they are shadowing variables in confusing ways.

Takes over from #834 which was more generally about adding warnings.

[basti1302]

I just had a case where shadowing caused a runtime error (see https://gist.github.com/basti1302/94b1ffde9d1e3414a8e6). Since a stated goal is to reduce runtime errors as much as possible, shouldn't something like this even be a compile error (instead of just a warning, as the issue suggests)?

[evancz]

@basti1302, your issue is the same as #873. The problem is that definitions are recursive by default. The current in the body is actually the same as the current on the lefthand side. We have a fix for that in the works. If you have further questions, please ask on elm-discuss, not on this issue. Also, thanks for sharing a nice example like this!

[savuori]

I would like variable shadowing to be a warning or error in pretty much all cases because it is never strictly needed but has resulted in a number of bugs. However, I understand that some people would like to keep that feature. We could have both with compiler command-line parameters like:

elm-make --warn shadowing,annotations --output ...

Then we could also have an equivalent to -Werror which turns all warnings into errors.

Another possible way of opting-out of these warnings could be to prefix variables with underscore like:

update _model =
    let _model =
        something_else
    in
        ...

This would tell the compiler that "I don't want to see warnings concerning these variables". However, this is a bit of unobvious syntactic sugar.

[kurtharriger]

I don't find shadowing in other languages to be all that problematic. What makes it problematic in elm is that the following is interpreted as a recursion rather instead of name shadowing.

update model =
    let model = clean model
    in
        ...

My proposal would be to opt in to recursion explicitly as is required in F# by prefixing the value with rec.

The following would be a recursive definition and model in case would refer to the recursive value.

update model =
   -- the following would be equivalent to current behavior.  model use in definition refers to model defined in by let and shadow the model passed to update
    let rec model = case model of ...  
    in
        ...

When value is not defined as recursive then we would have behavior similar to what developers in other languages would expect:

update model =
   -- here model on rhs is model provided to update and the result of which shadows the model argument
    let model = clean model
    in
        ...

Shadowing could be super convenient for chaining multiple transformations in a way that facilitates debugging and such.

For example:

update model =
    let model = Debug.log "model" model,
         model = { model | someValue = "override this for debugging" },
         --  model = { model | someValue = "other thing I tried" }
    in
       ...

[jvoigtlaender]

The piece

update model =
    let model = clean model
    in
        ...

can be written as

update model =
    case clean model of 
      model -> ...

and then has the "non-recursive" interpretation. It still depends on the availability of name shadowing as an option.

For the sake and scope of this discussion here, I suggest to stick to the assumption that the syntax of Elm does not change. So no "let vs. let rec". (We can have a discussion about that potential change elsewhere.)

@kurtharriger, assuming you don't get let rec. So instead of choosing between let and let rec in your code, you can only choose between let and case (in the form as in my example above), would you still want to be allowed to use name shadowing in your code? Or does the non-restriction of let to only non-recursive cases render name shadowing useless to you?

[kurtharriger]

I still think shadowing is useful. Perhaps you found a function online and its implementation uses a value in its implementation that conflicts with a top level value. Being forced to unnecessarily change the implementation its just asking to introduce errors, perhaps you missed one.

I would guess that the only time shadowing is really problematic and confusing is in the let binding and recursive values. Perhpas it would be sufficient to warn only when a recursive definition shadows an existing value.

[savuori]

I know this is not just as convenient but writing the debug use case as

update model2 =
    let model = Debug.log "model" model2,
         model = { model | someValue = "override this for debugging" },
         --  model = { model | someValue = "other thing I tried" }
    in
       ...

shouldn't be too taxing either. Also, if you copy&paste code somewhere and get shadowing warnings you can select the area you pasted and run search&replace so you won't miss an instance. To me bugs that fall silently through is always a bigger concern.

[kurtharriger]

I like the idea of renaming upward, but if you'll notice there was 3 models here not 2 which I think demonstrates my point. Compiler error would have prevented the shadowing, but it wont help if you with renaming if both names in scope. Also the idea with the commented out line is that you might add it temporarily to test a specific state then remove it and maybe adding and removing it several time and each add or removal may require updating names up or down the scope each time. Find and replace the first time is easy but when you switch back and forth several times one is more likely to forget. Im sure there are probably other techniques that people use when name shadowing is not permitted, but I personally find it a useful debugging technique I use in clojure all the time.

I could be wrong, but my assumption is that shadowing would not be nearly as big of an issue if definitions were not recursive by default. I think that if recursion were explicit or even just warning was issued when a recursive function shadows an existing value that bugs caused by shadowing would almost entirely go way.

I don't have any objections to name shadowing being considered a bad practice and maybe a rename values before publishing code is in general a good idea, but I think this is the job of a linter and a compiler error would introduce unwanted friction during the development cycle.

[savuori]

I agree that it shouldn't be an error because you might be mucking around and trying stuff. However, I'd really like to have the option to tell the compiler to warn me about everything that might be going wrong to speed up debugging and linting/finishing up before pushing.

[abradley2]

This burned me pretty hard today

Would definitely love to see this throw a warning :)

[zwilias]

This will be fixed by making (local) variable shadowing in 0.19 illegal. Code simply won't compile if there is any shadowing going on.