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
cmd/vet: warn about changing fields in non-escaping variables if they are not set after assignment #28099
Comments
What if the code is like this - func (s Something) Bar() Something {
// Code goes here
s.Done = true
return s
} Are you proposing code like this should be illegal ? |
A vet check seems more likely than special cases in the language. |
@agnivade Yes, that's the intentional case that I couldn't think of. Writing code like that would still be possible, but require the copy to be explicit rather than implicit:
|
@bradfitz Sure, that's why I said it could be either a vet check or a change. A vet check would catch regressions when refactoring large codebases or when something changes inside a of type (ie. adding a cache) and the method definition and other call sites (direct or indirect) need to be updated to pointers. A compiler error would be more likely to alert new Go users who are most likely to make the mistake and have trouble finding the source of their bugs. |
To the language, the receiver is really just a parameter. It should not forbid modifying the arguments; that's a perfectly reasonable thing to do. Making this an error for the receiver but not other arguments feels wrong, which means I'm not convinced this is a great idea, although I do understand the motivation. I'm not even sure that vet should check, since vet failures block builds now. Maybe maybe vet with an optional flag. |
I thought there were two sets of vet checks now: those that break go test upon matching and the full set when you run |
I think it should be ok to disallow write to a field to a non-pointer struct without any possible read following.(the possible read like pass the struct to another place or read that field). Following should disallow: type Something struct{
Done bool
}
func (s Something) Bar() {
if s.Done {
return
}
// Code goes here
s.Done = true
// no any possible read after the field write to this non-pointer struct. The programer write a line of useless code, may be a bug.
} Following should allow: func (s Something) Bar() Something {
// Code goes here
s.Done = true
return s // read the struct.
} func (s Something) Bar() int {
// Code goes here
s.Done = true
if s.Done{ // read the field
return 1
}
return 2
} |
There are a few reasons I proposed only receiver arguments:
Since receivers are described in the spec (and even moreso colloquially) using terminology borrowed from object oriented programming (ie. "methods" vs "functions") and the dot notation for calling methods is also borrowed from there, it lends itself to a mental model where using a receiver is seen as equivalent to "defining a method on an object" in other languages (even if it's not the same thing) rather than "passing the first argument to a function" (which is more accurate). In this model, the behaviour is surprising for the receiver (I'm not aware of any OOP languages that have a concept of a method which can modify the object but only for the lifetime of the method call) but not for the other arguments (function/method arguments being passed-by-value is fairly mundane, even in OOP languages.) (I was also under the impression that vet only gets implicitly run by go test, not go build..) |
Changing this issue to suggest a vet check. We should consider adding a vet check to warn about assigning to fields in non-escaping variables (including parameters) if the fields are never referenced after they are set. |
+1 for this feature however its implemented, you should not be able to modify a reciever passed by value unless it escapes |
It's a common mistake for new users of Go* to accidentally attempt to set a field on a non-pointer receiver.
For instance:
The above is legal in Go 1.x because s is passed by value to Bar, but the change only lasts for the duration of the function call since the local version on the stack was modified, not the value which the method was called on. This is almost always a bug and I can't think of any situations where that would be the intention of the programmer when attempting to set a property inside of a method.
This proposal is to make it illegal to modify a property on a receiver when the receiver is passed-by value. Arguments other than the receiver would unaffected.
This could be done either as a
go vet
check in Go 1.x, or a language change in Go2.The text was updated successfully, but these errors were encountered: