Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: Go 2: permit goto over declaration if variable is not used after goto label #26058

Open
pjebs opened this issue Jun 26, 2018 · 22 comments

Comments

@pjebs
Copy link

commented Jun 26, 2018

Right now you get this error: goto SKIP jumps over declaration of queryArgs

if len(s.standardDays) == 0 {
goto SKIP
}

queryArgs := []interface{}{}
//Do stuff with queryArgs

SKIP:
//Do stuff here BUT queryArgs is not used ANYWHERE from SKIP onwards

Currently I have to refactor all my code like this (including placing var err error at the top):

var queryArgs []interface{}

if len(s.standardDays) == 0 {
goto SKIP
}

queryArgs = []interface{}{}
//Do stuff with queryArgs

SKIP:
//Do stuff here BUT queryArgs is not used ANYWHERE from SKIP onwards
@pjebs

This comment has been minimized.

Copy link
Author

commented Jun 26, 2018

This can be fixed in Go 1 too since it's backwards compatible.

@ianlancetaylor ianlancetaylor changed the title Go 2 proposal: fix goto proposal: Go 2: permit goto over declaration if variable is not used after goto label Jun 26, 2018

@gopherbot gopherbot added this to the Proposal milestone Jun 26, 2018

@gopherbot gopherbot added the Proposal label Jun 26, 2018

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2018

This seems like it makes code a bit fragile: if you do this, and then later add a use of the variable, you will get a compilation error about lines that you didn't touch at all.

What is the benefit of making this change? How often does this problem arise?

@pjebs

This comment has been minimized.

Copy link
Author

commented Jun 26, 2018

The benefit is that the goto does what intuitively it is meant to do.

I can't tell you how often the problem arises because the use of goto is extremely rare ..... but when I do attempt to use goto, it arrises VERY VERY often and it usually means I have to refactor my code to place the variable definitions at the top (above the goto), rather than have the variables where they are most appropriate, and also I lose the ability to use := for variable define+assign.

Re:
if you do this, and then later add a use of the variable, you will get a compilation error about lines that you didn't touch at all.

Of the best parts of Go is how amazing and helpful the compiler is. If I later add a use of the variable, I would want to get a compilation error. It usually means that goto is no longer the best way to write the code.

@mark-rushakoff

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2018

Instead of moving variable declarations to the top, you could just introduce a new block around what you're goto-ing over:

package main

import (
	"fmt"
)

func main() {
	if true {
		goto FINISH
	}

	// New block, so it's safe to skip the declaration and assignment of secret.
	{
		secret := "You'll never see this line."
		fmt.Println(secret)
	}

FINISH:
	fmt.Println("Bye!")
}

https://play.golang.org/p/kzFxyaeOmkQ

@cznic

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2018

The status quo enables optimizations that would be no more [easily] possible under the proposal.

@mark-rushakoff

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2018

Looks like this could be closed as a duplicate of #27165.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2018

This is a slightly different approach to the same problem.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2018

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2018

I believe the intent of this proposal is the same as in #27165 (with that proposal based on an idea discussed in May of this year at an internal Go Team summit, but only written down yesterday).

However, this proposal disallows a goto if it jumps over a variable declaration of a variable that is used (lexically) after the target label (but always allows the variable access). In contrast, proposal #27165 disallows access of a variable (lexically) after a label, if there is a goto that jumped over it's declaration (but always allows the goto).

It does not seem immediately clear which approach is better. I think we should leave this proposal open as a clearly viable alternative.

@pjebs

This comment has been minimized.

Copy link
Author

commented Oct 8, 2018

My proposal is much clearer to understand in my opinion. The alternative proposal will be confusing for beginners.

It is also Go 1 compatible. I don't think the alternative is.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2018

This is what I said over on #27165:

Closing in favor of #26058, which is the same idea expressed in a slightly different way. Both issues accept the same set of programs; the difference is whether the error is reported on the use of the variable or on the goto statement. The (small) advantage of #26058 is that it does not change variable scope. It just relaxes, slightly, the existing constraint on goto over a variable declaration.

(I do still think that both proposals accept the same set of programs. Might be interesting to see a program that behaves differently under the two proposals. Not very important, though.)

@pjebs

This comment has been minimized.

Copy link
Author

commented Oct 8, 2018

will it be applied to Go 1 too?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2018

We aren't making any language changes until the Go 2 process starts.

Note that this has not been accepted.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2018

To clarify, it hasn't been declined, either. It's pending a decision.

@darkfeline

This comment has been minimized.

Copy link

commented Jan 26, 2019

Isn't this proposal just syntactic sugar for @mark-rushakoff's suggestion? In which case the decision to accept this proposal boils down to whether we want such syntactic sugar. Consider:

if foo {
	goto done
}
x := 1
if bar {
	goto done
}
y := 1
if baz {
	goto done
}
z := 1
fmt.Println(x, y, z)
done:
doStuff()

Under this proposal, this is exactly equivalent to:

if foo {
	goto done
}
{
   	x := 1
	if bar {
		goto done
	}
        {
        	y := 1
		if baz {
			goto done
		}
                {
			z := 1
        		fmt.Println(x, y, z)
                }
        }
}
done:
doStuff()

https://play.golang.org/p/4W6wRO-rmel

The syntactic sugar would very desirable in such an example.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

The current goto rules balance two competing goals:
(1) don't allow valid Go programs refer to uninitialized variables, and
(2) don't require complex analysis to decide whether a Go program is valid.

@pjebs's proposal and @mark-rushakoff's rewrite are different in general, although perhaps not in this case, and the difference highlights exactly how we have balanced these two competing goals.

It is true that this invalid Go program would, if it were redefined to be valid, not refer to uninitialized variables:

goto L
x := 1
print(x)
L:

It is also true that it can be mechanically transformed into this valid Go program:

goto L
{
x := 1
print(x)
}
L:

(The only change is the introduction of { and } on two new lines.)
However, here is an invalid Go program that does refer to uninitialized variables:

goto L
x := 1
L2:
print(x)
goto L3
L:
goto L2
L3:

The mechanical block rewrite preserves invalidity, because the goto L2 cannot jump into a block:

goto L
{
x := 1
L2:
print(x)
goto L3
}
L:
goto L2
L3:

(Again, the only change is the introduction of { and } on two new lines.)

It is of course good that adding braces has not taken this bad program from invalid to valid. That is why the block restriction exists.

The original message on this proposal did not suggest a detailed fix (the original title was simply "fix goto"). It has been retitled "permit goto over declaration if variable is not used after goto label" but a literal reading of "after" meaning "after in the source code" would violate goal (1) by admitting this buggy program. A reading of "after" meaning "after in the control flow" would violate goal (2) by requiring more complex compiler analysis to decide whether a program is valid.


Obviously, C compilers do this kind of analysis in their "used and not set" warnings, but experience has shown that these are sensitive to how aggressively the compiler optimizes (= learns new details about) the code. Whether a particular input is a valid Go program must not depend on compiler optimization level. There needs to be a clear, simple process for deciding. That process must produce consistent results in all implementations, and it must not admit any buggy programs.

As an example of where we have solved this kind of problem in the past, functions with results originally had to end in a return statement, but that was problematic for things like

func max(x, y int) int {
    if x > y {
        return x
    } else {
        return y
    } 
    // invalid Go program: missing return - in early versions of Go
}

We fixed this by writing a precise definition of a terminating statement and requiring that functions end with one of those instead of just a return.

We could conceivably do something equally precise here. But I think the definition would end up being longer, since it would have to capture some clear algorithm for deciding control flow. The current rules avoid introducing a control flow algorithm into the spec, because of goal (2).


I do see one possible simple change, which would be to say that it's ok to jump over declarations (unconditionally), and that jumping over one is the same as executing it, meaning the name is in scope and has the zero value. Goal (1) is satisfied because the programs never see uninitialized variables, and goal (2) is satisfied because there is no complex logic about whether a program is valid. An optimizing compiler would need to initialize the value earlier than it comes into scope, by sliding any (zeroing) initialization code backward up to just before the first goto that might jump over the value. This is basically what @pjebs says he does by hand already in the top comment.

Is this worth doing? Maybe, maybe not. That's an open question.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

For what it's worth, I think another way to achieve the goal of this proposal would be to say that goto label is permitted to skip over a variable declaration if 1) label is in the outermost block of the function; 2) label is not followed by a goto statement; 3) the variable is not referenced after label. But I don't know if it's worth it.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

For what it's worth, I don't understand why the outermost block would need to be called out specially to keep the program valid. Maybe I am missing something, of course.

I realize I should have said one more thing about the "Is this worth doing? Maybe, maybe not." Ultimately that comes down to first finding the simplest viable solution we can and then deciding whether the cost of adopting that solution is paid for by the benefits it would bring. In this case the benefits seem very near zero: goto is in the language primarily for generated code, and even for hand-written code it is trivial to move the necessary declarations earlier and keeps the code clear. On the other side of the scale, rolling out any change to language semantics has real, non-zero costs in terms of implementations and documentation (not just in our repo but everything everyone has ever written about goto). It's not obvious to me that the benefit of improvements here could possibly outweigh those fixed costs inherent to any change at all.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

You're right, we don't need the outermost block restriction. I got confused about continue L. But that shouldn't be a problem because a continue statement after a goto can only ever get us back before the goto.

@josharian

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

goto is in the language primarily for generated code, and even for hand-written code it is trivial to move the necessary declarations earlier and keeps the code clear.

Anecdotally, this restriction is extremely frustrating when working by hand on non-trivial functions involving gotos. My experience is with trying to clean up and clarify the compiler after the c2go rewrite. Being able to shorten variable lifetime is an important step in making code clear. I often found that I had to refactor to remove gotos, even in cases in which they did not otherwise impede readability, before I could do any other meaningful cleanup.

@pjebs

This comment has been minimized.

Copy link
Author

commented Jun 14, 2019

Notwithstanding @josharian experience, from my experience, it has been a minor annoyance, but nothing too monumental to overcome.

The proposal provides benefits that are intangible:

  1. goto will behave as it intuitively should. This is good for beginners of Go. I feel it makes the language more internally consistent too but that's prob a Rob Pike Q.
  2. I feel that documentation will in fact be simpler. My proposal is the more natural interpretation of how it works. There is less to document in terms of the "exceptions" and "provisos" on how goto operates as is the case now.
@kortschak

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

Having spent a fair bit of time porting old numerical Fortran code to Go, I'd seen a lot of GOTO use and abuse. Of the problems that come from use of gotos, this is by far and away the least problematic, the more significant issues have been (at least in those code bases) non-nesting of loops (or at least nesting of loops that are not trivially nestable) and identifying labels. Hoisting has always been a relatively easy process to work around the restriction here (and the restriction is a good motivation to remove as many goto statements as possible).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.