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

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
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@driusan
Copy link

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
@agnivade
Copy link
Contributor

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
Copy link
Contributor

bradfitz commented Oct 9, 2018

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

@driusan
Copy link
Author

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
Copy link
Author

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
Copy link
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 Proposal: Do not allow modification of non-pointer receivers. proposal: Go 2: do not allow modification of non-pointer receivers Oct 10, 2018
@ianlancetaylor ianlancetaylor added LanguageChange v2 A language change or incompatible library change labels Oct 10, 2018
@bradfitz
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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 proposal: Go 2: do not allow modification of non-pointer receivers cmd/vet: warn about changing fields in non-escaping variables if they are not set after assignment Oct 30, 2018
@ianlancetaylor ianlancetaylor added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed v2 A language change or incompatible library change LanguageChange Proposal labels Oct 30, 2018
@ianlancetaylor ianlancetaylor modified the milestones: Proposal, Unplanned Oct 30, 2018
@gitsakos
Copy link

gitsakos 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

@adonovan adonovan added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Apr 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

9 participants