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: cmd/vet: report evaluation order ambiguities #27098

Closed
go101 opened this issue Aug 20, 2018 · 14 comments

Comments

Projects
None yet
6 participants
@go101
Copy link

commented Aug 20, 2018

As some evaluation orders in multi value assignments are unspecified,
the following two programs may print either 0 123 or 123 123.

package main

import "fmt"

func f(p *int) int {
	*p = 123
	return *p
}

func main() {
	var x int
	y, z := x, f(&x)
	fmt.Println(y, z)
}
package main

import "fmt"

func f(p *int) int {
	*p = 123
	return *p
}

func g(x int) (a, b int) {
	return x, f(&x) // <=> a, b = x, f(&x); return
}

func main() {
	fmt.Println(g(123))
}

I propose that go vet should warn on such use cases to avoid unexpected behaviors.

@gopherbot gopherbot added this to the Proposal milestone Aug 20, 2018

@gopherbot gopherbot added the Proposal label Aug 20, 2018

@dgryski

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2018

Is vet sufficiently smart to handle this? Or do we need the equivalent of ubsan for Go?

@ALTree

This comment has been minimized.

Copy link
Member

commented Aug 20, 2018

One of the three vet criteria is frequency

y, z := x, f(&x)

how often do people write this kind of expression in real programs? Is this "evaluation-order confusion" actually source of a significant number of problems in the wild?

@go101

This comment has been minimized.

Copy link
Author

commented Aug 20, 2018

y, z := x, f(&x)

how often do people write this kind of expression in real programs?

There are many variants of the case in the above examples.

@go101

This comment has been minimized.

Copy link
Author

commented Aug 20, 2018

@dgryski

Is vet sufficiently smart to handle this?

Yes, I also think it is really hard for vet to detect some special such cases.
For example, it is hard for vet to determine whether or not p1 and p2 store the same address.

func f(p1, p2 *int) {
	y, z := *p1, f(p2)
	fmt.Println(y, z)
}

If this proposal is unpractical, I would make another proposal (a Go 2 one) to remove the unspecified behavior.

@go101

This comment has been minimized.

Copy link
Author

commented Aug 20, 2018

In fact, sometimes, I feel that Go compilers should report a compilation error instead of a vet warning for the case in my starting comment. Because that code is definitely bad for sure.

@ALTree

This comment has been minimized.

Copy link
Member

commented Aug 20, 2018

There are many variants of the case in the above examples.

Yes... but you wrote those examples, except for the one from #25609, which is the only report by someone being bitten by this. What I asked is if this is actually a common source of errors in Go code; that's what the 3rd vet criteria asks for:

A new check that finds only a handful of problems across all existing programs, even if the problem is significant, is not worth adding

@go101

This comment has been minimized.

Copy link
Author

commented Aug 20, 2018

I can't say it is uncommon. Please see the code modifications mentioned in this issue by tamird.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2018

Vet can't answer this perfectly. As long as it only reported true errors, it would be OK to have false negatives. The question is: how much work is it to detect these?

@rsc rsc changed the title proposal: vet should warning on uncertain results caused by unspecified expression evaluation orders proposal: cmd/vet: report evaluation order ambiguities Aug 20, 2018

@rsc

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2018

@alandonovan

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2018

The analytical firepower required to do this is far beyond vet. Every statement of the form a[i] = f(x) is a potential mistake of this kind; to prove it safe you must show that f(x) doesn't modify a or i, which means you need to prove that a and i don't escape (an escape analysis), or that they do escape but they are not mutated by f(x), which is an even more complex whole-program analysis. Even the most trivial case in which f contains nothing but an assignment to a or i requires an interprocedural analysis, and I doubt it would uncover significant bugs; it would certainly discover all kinds of infeasible scenarios.

A better approach would be a dynamic analysis along the lines of the race detector: statically transform a statement such as:

a[i] = f(x)

into:

saved_a := a
saved_i := i
tmp := f(x)   		
if a != saved_a { fail() }
if i != saved_i { fail() }
a[i] = tmp

The trick would be to eliminate as many of these checks as possible for efficiency. (This example gives just a flavor of the approach.)

@go101

This comment has been minimized.

Copy link
Author

commented Aug 21, 2018

@alandonovan's example makes me more tend to think that this unspecified behavior is common encountered in practice and this is a fundamental design issue of Go. After all, Go, as a so called C+ language, one of its main goals is to remove as many unspecified behaviors in C as possible, in particular for the unspecified behaviors which may be common encountered.

I feel this unspecified behavior can be removed for sure, there are no technique obstacles to achieve it.
The only opposing opinion I have ever heard of is removing the unspecified behavior would make some obstacles for complication optimizations. I think we should evaluate the degree of the negative impact for complication optimizations by removing the unspecified behavior. And I think if the degree of negative impact for complication optimizations is not very very large, removing the unspecified behavior is worth it.

@alandonovan

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2018

I'm not so pessimistic; I think genuine bugs will be rare. Of course that means I'm less sanguine about the value of a vet check. A competent programmer knows whether the values f and x might have something do with a and i, or whether they are definitively unrelated. This intuition, when well honed, is far stronger than any practical static analysis.

Removing unspecified behavior entirely from the language is unlikely to be worth the considerable effort and dynamic costs. Even if you specify expression evaluation order completely, including the order in which error cases are checked by the runtime, and forbid "unsafe" and concurrency, the libraries on which one builds any application are not specified to that same level of detail. Specifying the evaluation order also dramatically increases register pressure in the compiler, which is why even Scheme, with arguably the cleanest semantics of any language, resisted the urge to specify the evaluation order.

@go101

This comment has been minimized.

Copy link
Author

commented Aug 21, 2018

I think I agree on your opinion in theory. I just need to see some concrete examples to be persuaded convinced.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2018

It sounds like this is beyond vet, so closing.

@rsc rsc closed this Sep 19, 2018

quasilyte added a commit to go-critic/go-critic that referenced this issue Mar 23, 2019

checkers: add evalOrder checker
Checks for return statements that may have unwanted
dependency on the evaluation order.

See:
	golang/go#27098
	golang/go#23188

Signed-off-by: Iskander Sharipov <quasilyte@gmail.com>

quasilyte added a commit to go-critic/go-critic that referenced this issue Mar 23, 2019

checkers: add evalOrder checker (#829)
Checks for return statements that may have unwanted
dependency on the evaluation order.

See:
	golang/go#27098
	golang/go#23188

Signed-off-by: Iskander Sharipov <quasilyte@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.