Remove warning about shadowed value names?

[garyb]

This seems like much more of an opinionated lint-y thing than the other warnings we throw out. There are definite valid reasons for wanting to shadow names - for example, often I want to hide an old version of a value in scope, or if #3245 is rejected, it enables an alternate workaround for nested do with differing bind.

It's hard to say for sure, but I think it has caused me more trouble than it has saved. Interested to hear other people's thoughts... pretty sure I know what @natefaubion thinks. 😉

[natefaubion]

I've never understood why this is a warning we want in the compiler vs a linter. Why do shadowed names indicate likely broken code or code that will break in the future (which is generally what our bar is for warnings)?

[milesfrain]

I'm definitely in the enable-all-warnings camp, and would prefer to rename a shadowed variable rather than risk a bug.

Giving users a choice about this via a compiler flag or linter option seems fair too. Do we have a linter that detects shadowing?

[garyb]

You can end up with bugs because of disallowing shadowed warnings too - sometimes you reuse an a now where actually you meant to use a1 or whatever, because you were forced to give the "current version" of a a new name. Having unused warning won't necessarily help you there either as you might end up using a mixture of a and a1 past the point that a1 is introduced.

Not having compiler options for toggling warnings is principle I'd like to stick with. Filtering them in your project's build is fine if you think you know better, but libraries should be required to resolve all of these warnings. Having them suppressable as part of the compiler interface will make it seem like ignoring them is okay.

But that's also why this is a questionable warning: it's an opinion, rather than "you're definitely making a mistake, albeit a non-fatal one" like the other warnings are.

[hdgarrood]

I'm actually starting to be less in favour of this.

Why do shadowed names indicate likely broken code or code that will break in the future (which is generally what our bar is for warnings)?

They don't necessarily, but I'm not sure that generally is our bar for warnings. For instance, I don't think any of these indicate likely broken code or code that will break in the future:

• top-level values missing type declarations, or having type declarations involving type wildcards
• data types missing kind declarations (if they have polymorphic kinds)
• unused variables, which we don't have yet but we would like to add (admittedly they do sometimes indicate broken code, but I find that most of the time they don't)

I feel like some warnings are more "things we are pretty confident ought to be addressed, for the sake of others reading this code later". To me, it doesn't quite feel consistent to reject a warning for shadowed names, but include one for unused variables.

Filtering them in your project's build is fine if you think you know better, but libraries should be required to resolve all of these warnings. Having them suppressable as part of the compiler interface will make it seem like ignoring them is okay.

At this point, psa (which provides warning filtering, amongst other things) is pretty much the standard; lots of people use it, because it makes the compiler's interface better, and it has no competitors as far as I'm aware. Clearly warning filtering is something that's desired, and I am not actually sure if it's our place to say that libraries should be required to resolve them. I'm not sure not putting these features in the compiler is really accomplishing anything other than making it slightly more awkward to set up a fully functional and comfortable development environment.

[hdgarrood]

Actually, come to think of it, if the basis of warnings is "things we are pretty confident ought to be addressed", maybe shadowed variable names don't make the cut, given that there are cases where we do in fact want to shadow variables (in order to prevent ourselves from referring to them again).

[joneshf]

I'm actually starting to be less in favour of this.

I'm glad someone else said it. I didn't want to be the only one to dissent, but I'm against removing this warning at the moment. My reasoning is anecdotal: fewer issues exist if you cannot shadow a name than if you can. It's anecdotal because I haven't looked for actual research.

If we're asking for vetoes, I'd like to veto removing this warning for now.

[garyb]

Fair enough! I had been intending to come back today and restate my dislike of this warning, since all the times I've had to work around it recently came back to me when I was trying to sleep last night 😄

Shall I close it then?

[maxdeviant]

often I want to hide an old version of a value in scope

This and shadowing irrelevant values from the outer scope in a lambda cover ~95% of the time that I would want to use shadowing. In these cases adding a variable number of 's to the name is just useless noise.

[milesfrain]

I'm still a fan of the shadowing warnings, but wanted to note for completeness that fixing shadowing puts a damper on using record pun shorthand:

-- Concise with shadowing
foo1 :: Int -> Int
foo1 x = go {x, n: 7}
  where
    go {x, n: 0} = x
    go {x, n} = go {x: x*x, n: n-1}

-- Noisy to fix shadowing
foo2 :: Int -> Int
foo2 x' = go {x:x', n: 7}
  where
    go {x, n: 0} = x
    go {x, n} = go {x: x*x, n: n-1}

Of course, there are lots of better ways to rewrite this arbitrary example.

[Quelklef]

@milesfrain FWIW, and for anyone who may find it useful, the way I typically fix cases like that is to rewrite it as

foo = \x -> ...
  where
  go { x, n } = ...

[Quelklef]

A possible compromise is to allow variable shadowing in let expressions and do blocks, but not in e.g. nested functions. Thus:

-- allowed
let
  state = getState
  state = doAThing state
  state = doAnotherThing state
in
  state

-- disallowed
strs # filter \str -> strs # some \str -> str /= str && isPrefix str str

[purefunctor]

A possible compromise is to allow variable shadowing in let expressions and do blocks, but not in e.g. nested functions. Thus:

I feel like allowing shadowing in let like declarations like this shouldn't be the case, as the implementation will definitely run into trouble with mutually recursive bindings. In particular, allowing shadowing in nested let expressions like this is much more safer.

let state = getState in
let state = doAThing state in
let state = doAnotherThing state in
state

As for do notation, I don't feel strongly for/against shadowing in <--bound names, since they'd desugar into lambdas anyways. I do agree that adding ticks ' to resolve shadowing is a bit annoying in certain cases, like with clashing auxiliary functions. Moreover, I'd say that we probably want more powerful IDE capabilities, most importantly type/reference/definition information for locally-bound values, before we commit to allowing shadowing in value names.

[rhendric]

If there's one place in PureScript syntax where we carve out an exception for this warning, it should be do notation. do notation is explicitly sequential. When reading a do block, the reader already needs to take care to notice what is defined before what else. If a name is defined twice in a do block, it could still be an accident, but it's a different order of accident than if a name is redefined deep in an inner scope where you might only have noticed the outer binding. I'm comfortable thinking of redefinition of a binding as an effect that can be implicitly composed with any other monadic effect. It's a very imperative thing to want, and do notation is where we indulge our imperative desires.

IMO we should absolutely not allow double-definition of a binding in a let or where block! The ordering of those bindings is otherwise meaningless, so we would need to add a new ad-hoc rule that says to resolve such redefinitions from top to bottom in order to know which is the first state, which is the second, etc. Feels very unnatural, so much so that I want to say that, with no disrespect meant to anyone, if it feels natural to you, you should consider whether you have a profound misunderstanding of how let and where blocks behave.

I'm fully neutral on continuing to warn on outer-to-inner shadowing, as with nested lets or lambdas, which is why I haven't chimed in before. 😄