-
Notifications
You must be signed in to change notification settings - Fork 17.5k
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: spec: define return statement's result assignment order? #58233
Comments
CC @mdempsky |
I'd think that returns work like assignments (of the result values to the result variables) and thus should behave the same way with respect to evaluation order. (Haven't investigated the spec yet.) |
Note that even FWIW, cmd/compile already uses the same logic to handle assignment and return statements. |
Perhaps this should be documented as an implementation restriction? "A compiler may disallow return statements for which the order of evaluation cannot be determined due to side effects." Or something similar? |
It can determine for sure, it just doesn't now. |
I agree that if you have
then the spec does not guarantee whether the first result is evaluated before or after the call to But if you have:
then I don't believe the spec defines whether In either case, I don't think that's something we can easily change at this point. |
This proposal has been added to the active column of the proposals project |
We can use an func Eval[T any](v T) T {
return v
}
func As(err error) (_ Error, ok bool) {
var e Error
return Eval(e), errors.As(err, &e)
} [update]: similar to #36449 |
@go101 In my opinion it would be better to not write such confusing code in the first place. In my opinion we should either fully specify the evaluation order or we should have a vet check to warn about cases in which we both read and write a variable in the same statement. The vet check wouldn't be perfect but it might be able to catch cases like this. |
Based on the discussion above, this proposal seems like a likely decline. |
I've used this pattern extensively ever since I learned of it. It did seem odd at first blush. Nowadays it feels similar to an inline defer, if that makes sense. I'd venture to guess that defining it strictly as left-to-right would break a lot of code; my preference is to officially define the current semantics. I'd be -1 on a vet but mostly because I like the current pattern a lot for very boilerplate HTTP methods (e.g., It seems to be that defining the semantics would go a long way towards eliminating confusion. I remember being just as confused by defer having the ability to modify named returns after a return -- but after internalizing that, it's clear and obviously useful. |
Note that there is not a current semantics. The exact behavior depends on compiler details that can vary from release to release. |
Interesting. In that case, I do still recommend officially defining the current behavior as per cmd/compile. Looking through code I have checked out with the rather lazy Vendored in kubernetes, but also their own actual real libraries: Vendored in kubernetes and not in the library anymore: Prometheus: |
I think that you are suggesting that we define return e, errors.As(err, &e) as being implemented as
The rule would be something like "when multiple expressions appear on the right hand side of an assignment or return statement, first evaluate all the function calls, method calls, and channel operations in left-to-right order. Then evaluate all the other expressions in left-to-right order." That strikes me as a strange choice to make. Wouldn't it be simpler say "evaluate all expressions in left-to-right order"? If we're going to give a precise definition for this kind of confusing code, shouldn't we aim for the simplest precise definition? |
Another way to frame it might be, "when multiple expressions appear on the right hand side of an assignment or return statement, evaluate all operations that can modify an expression in left to right order, and then evaluate all other expressions in left to right order", though it is perhaps less explicit. It seems a bit easier to explain than deferred functions, where the wording is currently "deferred functions are executed after any result parameters are set by that return statement but before the function returns to its caller" (and then a large example to be clearer). |
Maybe I'm too close to it, but to me those seem like very different things. A deferred function is executed after the return statement is complete, and after the values have been assigned to the result parameters. That seems clear. There is no ambiguity or confusion about order of evaluation. 1) Evaluate all return expressions (in some order); 2) Assign them all to result parameters; 3) Execute deferred functions. The order you suggest, which is similar to the one I mentioned, is still more complicated than "evaluate all expressions in left to right order." And my point is really that, if we are going to specify an order of evaluation, why wouldn't we just say "evaluate all expressions in left to right order"? Why would we want to say something that is more complicated than that? |
I agree that "evaluate all expressions in left to right order" is much simpler to reason about. Potential points in favor of being more complicated are,
If I have type foo struct {
copyByVal string
copyByAddr map[string]bool
}
func modify(f *foo) bool {
f.copyByVal = "modified"
f.copyByAddr["modified"] = true
return true
}
func problem() (foo, bool) {
f := foo{copyByAddr: make(map[string]bool)}
return f, modify(&f)
} With the current behavior, However, defining a vet, or more strictly, refusing to compile this type of return would be a strong signal to developers that they can't do this anymore (and will force ecosystem library churn) |
Broadly speaking, I think this issue can be generalized to this sentence in the spec, |
return f, modify(&f) Such code seems to me to be pretty clearly buggy, in a similar way to code that relied on map iteration order (prior to the randomization update). An update to the spec might be "The order of those events ... is not specified and is unpredictable. Code which relies on any specific ordering behavior is incorrect. A future version of Go may codify this unpredictability." |
This is not true. The current cmd/compile implementation has several self-inconsistencies. See #36449 |
We can't adopt the rule "run function calls then evaluate expressions", because then if you have code like
or
and you have a tool that inlines all the calls to f() (suppose
or
Before global was evaluated before g(); now it's evaluated after g(). Of course the rewritten code is undefined today, but if we're going to define it, we should define it consistent with manual inlining, which would be a strict left-to-right order. Return and assignments also have to match, so if we're going to make a rule, it has to be for both, which is why I showed both above. All that said, I don't think we are considering redoing evaluation order in assignments today. If someone wants to implement strict left-to-right and measure the performance hit, that would be useful data to open a conversation about evaluation order more broadly. |
Am I reading this correctly,
Unfortunately it seems like this would've been valuable to specify in the 1.0 spec. Since it's too late for that -- I'd side with @ianlancetaylor's original comment of adding a vet check, to gradually migrate the existing ecosystem away from the currently assumed behavior. |
Note that |
I agree that we would have to try it out. I do think that we should get a warning on every piece of code in that comment. |
No change in consensus, so declined. |
A coworker sent out a code review with:
... and during review I said I wasn't sure whether it works. Turns out it worked but it's undefined. It happens to work in cmd/compile and doesn't work in gccgo.
The spec defines assignment order for e.g.
a, b = foo(), bar()
ora, b = b, a
but not forreturn
.Another example which has different results between gc and gccgo: https://go.dev/play/p/SDMlczFBshC
I propose the spec defines this. My preference would be left to right assignment to results.
But admittedly it might break code like above. (which we fortunately rewrote to be simpler and explicit)
/cc @ianlancetaylor @griesemer (sorry)
The text was updated successfully, but these errors were encountered: