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

Open
driusan opened this Issue Oct 9, 2018 · 10 comments

Comments

Projects
None yet
8 participants
@driusan

driusan commented Oct 9, 2018

It's a common mistake for new users of Go* to accidentally attempt to set a field on a non-pointer receiver.

For instance:

type Something struct{
     Done bool
}

func (s Something) Bar() {
     if s.Done {
         return
     }
     // Code goes here
     s.Done = true
}

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 ago vet check in Go 1.x, or a language change in Go2.

  • I don't have any numbers to support this claim, but blaming new users means I wouldn't need to admit if it were to theoretically be a mistake that I might still occasionally make.

@gopherbot gopherbot added this to the Proposal milestone Oct 9, 2018

@gopherbot gopherbot added the Proposal label Oct 9, 2018

@agnivade

This comment has been minimized.

Member

agnivade commented Oct 9, 2018

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 ?

@bradfitz

This comment has been minimized.

Member

bradfitz commented Oct 9, 2018

A vet check seems more likely than special cases in the language.

@driusan

This comment has been minimized.

driusan commented Oct 9, 2018

@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:

func (s Something) Bar() Something {
     // Code goes here
     sprime := s
     sprime.Done = true
     return sprime
}
@driusan

This comment has been minimized.

driusan commented Oct 9, 2018

@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.

@robpike

This comment has been minimized.

Contributor

robpike commented Oct 9, 2018

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.

@ianlancetaylor ianlancetaylor changed the title from Proposal: Do not allow modification of non-pointer receivers. to proposal: Go 2: do not allow modification of non-pointer receivers Oct 10, 2018

@bradfitz

This comment has been minimized.

Member

bradfitz commented Oct 10, 2018

I'm not even sure that vet should check, since vet failures block builds now.

I thought there were two sets of vet checks now: those that break go test upon matching and the full set when you run go vet explicitly? But perhaps I wasn't paying enough attention.

@bronze1man

This comment has been minimized.

bronze1man commented Oct 10, 2018

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).
And this rule should be enough to catch some of bugs of the non-pointer receivers.

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
}
@driusan

This comment has been minimized.

driusan commented Oct 10, 2018

There are a few reasons I proposed only receiver arguments:

  1. They're already special arguments (ie. for method invocation or resolving the method set of a type) so the compiler frontend/vet already has the information needed to do the check. (And since it's only a static check, the backend would continue to treat them equivalently.)
  2. I agree that modifying function arguments is a perfectly reasonable thing to do. Restricting to the receiver makes it a more minimal change that doesn't forbid that or affect normal function calls. When I wrote the proposal I couldn't think of any circumstances where it would be a reasonable thing to do for a receiver (although @agnivade has a great counter-point that I hadn't thought of)
  3. The behaviour is less surprising for other 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..)

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Oct 30, 2018

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.

@ianlancetaylor ianlancetaylor changed the title from proposal: Go 2: do not allow modification of non-pointer receivers to cmd/vet: warn about changing fields in non-escaping variables if they are not set after assignment Oct 30, 2018

@ianlancetaylor ianlancetaylor modified the milestones: Proposal, Unplanned Oct 30, 2018

@vladcomp

This comment has been minimized.

vladcomp commented Oct 31, 2018

+1 for this feature

however its implemented, you should not be able to modify a reciever passed by value unless it escapes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment