proposal: Go 2: require explicit variable shadowing

[networkimprov]

Branched from #377.

Problem

Unintended shadows are easy to create with := and cause nearly invisible bugs.

Go takes one of eight possible paths for a, b := f(), as each var is defined, assigned, or shadowed. One cannot tell which without searching prior code, so it's easy to write something that behaves incorrectly.

Proposal

Benefits Provided

a) eliminate bugs due to unintended := shadows
b) reduce the complexity of := statements (a, b := f() would have three paths)
c) no learning curve for the go vet changes
d) backwards compatibility

Changes to go vet and var:

1. Let go vet flag implicit shadows, s := t.

2. Let var s T or var s = t in the new scope silence go vet for intended shadows.

   x, err := Fa()
   if err != nil {
      var err error     // explicit shadow; not flagged by go vet
      y, z, err := Fb()
      ...
      x := 1            // implicit shadow: flagged by vet
   }
   

3. Let var name allow redeclaration of the name within its scope, without creating a shadow. Python also permits this. (Not essential to (1) & (2), so could be omitted.)

   x, err := Fa()
   if err == nil {      // OR: if var err; err == nil 
      var err           // override shadowing in if-scope
      y, z, err := Fb() // no need for separate declarations of y & z
      ...
   }
   if err != nil { ... }
   

[beoran]

So, If I understand correctly, what you propose is only a change to go vet, then?

[deanveloper]

The title and contents of this proposal seem to be conflicting. The title says to require declaration using var name in order to take advantage of variable shadowing. The LanguageChange label also seems to suggest this. But the contents seem to suggest that this is a backwards-compatible change that is only enforced by go vet.

Either way, this is a good change and has gotten a +1 from me.

[networkimprov]

@beoran @deanveloper: see item 3.

[IngCr3at1on]

Call me crazy but neither of those examples seem idiomatic to me.

The following is readable even if you aren't insanely familiar with Go; while your implementation requires either always adding those comments or implying an extra familiarity of (in my opinion counter-intuitive) functionality in the var built-in.

var (
    y <type>
    z <type>
)
x, err := Fa()
if err == nil {
    y, z, err = Fb()
}
if err != nil { ... }

It reminds me of ruby variable scoping, which I personally have never been fond of.
You could complain that my version is more verbose but it's that verbosity that makes it more obvious what it does; simplicity is good 😄

[networkimprov]

@IngCr3at1on, your example doesn't reproduce mine. It should be:

x, err := Fa()
if err == nil {
   var (
      y <type>
      z <type>
   )
    y, z, err = Fb()
}
if err != nil { ... }

[IngCr3at1on]

@networkimprov funny enough, that's what I had but then I changed it because your example doesn't use y and z and therefore will result in a compile error (assuming y and z were to be handled after the second err check); my mistake in reading your code (but I feel this also reinforces my point does it not?).

[IngCr3at1on]

Using the example from the OP to expand on the point I was trying to make.

x, err := Fa()
if err == nil {      // OR: if var err; err == nil 
   var err           // override shadowing in if-scope
   y, z, err := Fb() // no need for separate declarations of y & z
   if y && z { ... } // Some random operations on y and z so that we're using these values...
}
// Now we check the error reassigned when Fb ran _after_ performing operations on y and z.
if err != nil { ... }

I'd like to understand what instances you would write something like that myself because again it seems particularly unsafe, allowing the error to be shadowed without at least a warning from vet seems very dangerous here.

I'd almost argue the better solution would be to explicitly disallow shadowing at the compiler, resulting in a build error.

[networkimprov]

allowing the error to be shadowed without at least a warning from vet seems very dangerous here

Per the first sentence of the text, "let var name prevent shadows of the name within its scope."

EDIT: there is no shadowing in the above example.

[IngCr3at1on]

@networkimprov I fail to see how quoting a sentence from your OP addresses my concerns... I'd like to understand (as a Go user and a software programmer) why such a change is actually wanted because so far as I can tell it does not improve on anything?

For my perspective it only makes the functionality of the var built in less obvious and therefore the language less idiomatic.

[DylanMeeus]

Whilst I am not convinced the proposed solution is good, because it's not 'unambigious behaviour' of the var keyword. Or in any case, it makes it less obvious as @IngCr3at1on has mentioned.

However, I do think that requiring variable shadowing to be an explicit operation is a good idea. But I'd propose a change that won't break existing codebases that use shadowing consciously by introducing a new keyword.

i := 10
for {
     shadow i := 9
     i -= 1
     fmt.Printf("%v\n", i)
     break
}
fmt.Printf("%v\n",i)

// output:
8
10

This is not actually a real proposal, but I just want to raise the idea of making shadowing more explicit. Perhaps I could turn that into a more thought out proposal :)

[networkimprov]

FYI, a new keyword breaks any existing code using it as an identifier. That wouldn't rule it out, but it's a harder sell.

[IngCr3at1on]

It's the "breaks existing code" that is the biggest argument against removing shadowing entirely as well (though personally I'm still in favor of it vs the other options I've seen)...

The main thing in either case would be coming up with a way to keep with the Go promise of having language support for 2 versions back; we don't have to necessarily have to come up with something that avoids breaking all older code but rather something that can be used to migrate towards the 'ideal future' (whatever it's decided that is; be it 'no shadowing', 'ruby-esque shadowing' (sorry just not sure what else to call it 😄), 'shadow shadowing' (😂) or something else all together).

Edit:
To that end I wonder if a possible way to implement either 'shadow shadowing' or 'no shadowing' would be an opt-in env similar to the GO111MODULE option added in 1.11 as an early beta and a way to get people to start moving towards that method (might allow for facilitating the backwards compat promise?)

Note @networkimprov I didn't mention 'ruby esque shadowing' as needing an env option because though I don't necessarily favor the method it is the least invasive and wouldn't require such an option (so far as I can think).

[ianlancetaylor]

It seems to me that shadowing is only potentially confusing when using :=. We already have a keyword that fairly clearly indicates that the variable may be shadowing another variable: var. I don't see a reason to introduce a new keyword for the same purpose.

[beoran]

How about a new operator, say :== that works exactly like :=, but does not allow any shadowing. That is, all variables on the left hand side of :== must be new. That seems to be a backwards compatible solution for the shadowing that := allows.

[networkimprov]

That wouldn't flag existing unintended shadows; nor enable explicit ones.

[mibk]

I had a similar idea as @beoran, but reversed: add a new operator (I was thinking of ::=, but it really doesn't matter) that would allow variable shadowing, and change the meaning of := to ban variable shadowing.

[DylanMeeus]

Yeah, silly me, I didn't think about the fact that the reserved keyword would already exist in the codebase as a name somewhere else. :)

Considering that, a new operator might make more sense. I do wonder though, are people using shadowing consciously? Maybe it's because of the background of languages that I come from that it just seems like an odd choice to me. In terms of readabilty of the code, shadowing seems to be "bad choice" to me.

[networkimprov]

A new meaning for := would break existing code. This proposal only vet-flags existing implicit shadows.

[DylanMeeus]

@networkimprov unless you allow for := to mean "shadow-able" and :== to mean "not-shadow-able". But I admit, since most of the time you wouldn't really think about the shadowing, you'd end up almost never using :==.

Is there a consensus that changes can't be 'break existing code' for Go 2?

[ianlancetaylor]

Breaking existing code is a very heavy cost, and it requires a corresponding benefit. It can be done, but it requires a very very good reason.

[IngCr3at1on]

@ianlancetaylor correct me if I'm wrong but I believe an acceptable solution for a breaking change would be something like what I was talking about above; feature flag the breaking change to be disabled by default in one release, assuming at least some acceptance this would later be switched to enabled in a subsequent release before eventually being removed (with the behavior left in tact)... I believe this would allow for keeping the '2 version back compat promise' regardless of whether the final change was made in 2.0 or some other release.

Assuming that's the case I feel like the "this breaks existing code" argument might be moot?

[ianlancetaylor]

@IngCr3at1on For our plan for language changes see https://go.googlesource.com/proposal/+/refs/heads/master/design/28221-go2-transitions.md

[beoran]

Yes, but that stipulates "I think the only feasible safe approach is to not permit language redefinitions."

I agree with that. Redefining := to become non-shadowing would be exactly such a redefinition which cannot be safely permitted. The := "operator" is about the most widely used one in Go language programs everywhere, so we can't redefine it's meaning now. There already is go vet -shadow, that could be enhanced to detect more cases of shadowing. And then a new operator ::= or :== or whatever could be added to make non-shadowing assignments, and we can all gradually move to using that exclusively.

[deanveloper]

I think having multiple, similar, non orthogonal assignment operators is a bad idea. It reminds me of JavaScript's multiple comparing operators and it just feels wrong.

[beoran]

True, but I think the reason javascript has those is because the first equality operator turned out to have problems, so a second one was needed. When I think about it now, I would say that allowing := to shadow existing variables is a historic misfeature of Go. If we want to fix this in a backwards compatible way, then I think something like an extra operator will be needed, unless if someone else has a better idea?

[networkimprov]

Erm, the better idea is the original text of this proposal :-p

If you want to discuss new declaration operators, please start a new proposal!

[beoran]

Well this is what the discussion here lead up to. The general topic seems to be how to solve the shadowing problem in Go. Maybe a new operator isn't a good solution for this problem, but I'm not convinced the original proposal is either, sorry.

[IngCr3at1on]

Erm, the better idea is the original text of this proposal :-p

whether it's the "better" idea is still absolutely a subject for debate; however I agree that this thread has exploded a little bit.

please start a new proposal!

again, agreed (skipping my opinion about the OP); except that I feel like this "proposal" is incomplete as well (at least in regards to https://github.com/golang/proposal); perhaps another possible idea would be to move this conversation somewhere else (the gopher's slack or forums might be a good place for an informal 'round table') and shelve the proposals until some more formal versions are written (including complete reasoning around said proposals)?

edit: to be clear I understand that this is an initial issue and therefore does meet the minimum requirements for an initial proposal; I'm just wondering if at this point a design-doc wouldn't be a bad idea to try to address (perhaps quell?) the concerns of myself and others.

[ianlancetaylor]

Thanks for the suggestion. This addresses a problem with variable redeclaration in :=. It addresses it by adding an additional variable declaration (var err in the original example). But we can already address the problem by adding additional variable declarations (of y and z in the original example, as discussed above), and using a regular assignment with = rather than a declaration with :=. So the benefit of this proposal seems too small to be worth the cost of an unusual and new form of variable declaration.

[networkimprov]

Hm, did you consider the suggested method to prevent accidental shadowing? That is really the soul of the matter.

EDIT: should I file a separate proposal for that?

[ianlancetaylor]

@networkimprov I'm sorry, I don't know what "method to prevent accidental shadowing" you are referring to.

[networkimprov]

I suggested that vet flag implicit shadows.

For intended shadows, make them explicit with var name T or var name = t in the new scope.

x, err := Fa()
if err == nil {
   x := 1            // implicit shadowing; flagged by vet
   var err error     // explicit shadowing; not flagged by vet
   y, err := Fb()

The point of this proposal is to fix accidental shadowing, a common source of complaints.

[ianlancetaylor]

@networkimprov Thanks. That seems entirely different from the proposal at the top of this issue.

I'm not sure how I feel about that suggestion. It's fairly routine to write code along the lines of

    f, err := os.Create(...)
    if err != nil {
        if err := os.Mkdir(...); err != nil {
            return err
        }
    }

It seems awkward to have to remember to use = in the inner block, or to have to explicitly declare var err error.

[networkimprov]

Creating shadows should be "awkward", because you should almost never do that!

Implicit shadows may be routine, but they're often disastrously wrong, hence the constant complaints about shadowing.

Go philosophy has long held that an extra line for the sake of clarity is beneficial ;-)

That seems entirely different from the proposal at the top of this issue.

My previous code example is from the proposal text, with one line added for contrast.

[ianlancetaylor]

The advantage of shadowing in a block structured language is that blocks can be moved around independently without having to worry about whether shadowing is being introduced or removed. That is, shadowing was an intentional choice made when the language was designed. In this, Go of course follows many other languages, notably C.

It may have been the wrong choice, and we can consider changing it, but in my opinion we shouldn't approach from the basis that shadowing should be available but awkward. Either the language does shadowing or it does not.

[networkimprov]

You'd have to vet-flag shadows if you conclude they're a misfeature.

Then you'd need a way to denote intentional shadows. I suggested explicit declaration. Others suggested a new operator like ::=. You rejected both; do you have something else in mind?

[ianlancetaylor]

I don't think you are framing the question in the most useful way. Either shadowing is fine or it is not. If it is fine then we don't need to do anything. If it is not fine then we don't need any special mechanism to denote intentional shadows.

[beoran]

@ianlancetaylor Sorry to butt in here, but it's not so simple. Go has many features that are useful and "fine", and shadowing is one of those fine features. However, as Go is today, with :=, it is too easy to shadow accidentally. I myself have inadvertently shadowed variables on several occasions and it causes subtle, hard to detect bugs. I think that is what this issue was about in the first place.

Now, there seems to be no consensus over how to solve this problem, and it's true that for my projects, I could adopt a code standard where the use of := is simply banned. However, it would be nicer if there was support either in the language or in the supporting tools to detect and warn me of such such accidental shadowing. Of course, the question is then by what heuristic this could be detected...

[ianlancetaylor]

I can see a plausible argument for permitting shadowing, as we do today. I can also see a plausible argument for banning shadowing, though I would worry about how much existing code would break. But I can't yet see a plausible argument for banning shadowing and then adding a special mechanism to permit shadowing. Why bother? It's trivial to just rename the variable.

[networkimprov]

I don't think you're understanding the pain that implicit shadowing is causing.

Go takes one of eight paths for a, b := f(), as each var is defined, assigned, or shadowed. You cannot tell which without searching prior code, so it's easy to write something that works differently than intended.

Either shadowing is fine or it is not

Shades of gray. Shadows are beneficial in some cases, harmful in others; @beoran said the same. Can we not accommodate both?

[ianlancetaylor]

Every language decision is shades of gray. Every language decision is a cost/benefit decision.

Accommodating two different approaches to shadowing means additional language complexity that every Go programmer needs to learn. What benefit do we get that is worth that cost?

[networkimprov]

@ianlancetaylor, I have rewritten the proposal in light of the discussion since the issue was closed.

Could we give other members of the Go team a chance to consider this?

Could you re-open the issue pending the outcome of that review?

[ianlancetaylor]

@networkimprov The original proposal has a lot of comments that seem no longer applicable. Would you be OK with simply opening a new issue instead? Thanks.

[networkimprov]

Surely, I've filed #31064. Thanks for your feedback!