1. 8
  1. 13

    This article can be summarized as “run go vet”:

    $ go vet a.go
    # command-line-arguments
    ./a.go:11: call of f copies lock value: main.container
    ./a.go:14: f passes lock by value: main.container
    

    All checks in go vet are considered to be highly reliable. go test runs a subset of “checks believed to be always worth addressing”. I’m not sure why copylocks isn’t in there; looking at the implementation it looks like it considers anything with a Lock() and Unlock() method as something that shouldn’t be copied; this will trigger a similar error:

    type container struct{ locker }
    type locker struct{}
    
    func (l *locker) Lock()   {}
    func (l *locker) Unlock() {}
    

    That seems like a reasonably fool-proof implementation to me, and I could find only a single complaint about it on GitHub.

    1. 1

      Yup. I don’t think this was always part of go vet. I don’t know when it was added (I’ve been using Go nearly daily since 1.0), but it was only relatively recently that I learned go vet was essentially perfect on this. Before that, I had advocated always using *sync.Mutex (and the like) instead of sync.Mutex, unless a benchmark could motivate otherwise.

    2. 6

      This is a very trivial issue (very well known and documented in the Go space) explained in a terribly convoluted and confusing way.

      Related: https://golang.org/issue/6188

      1. 4

        This is a very tricky and nonobvious issue, well known to Go experts (a.k.a. “pitfall/wart”), thus worth publicizing from time to time for the benefit of newbie and intermediate Go users. It is explained in a confusing way because it is tricky enough that the author still doesn’t fully grasp the consequences. There’s a classic article by Dave Cheney on the topic, demonstrating subtle non-Mutex races for value receivers, but it’s not so easy to find - you basically have to know about it already to find it… Though I’m rather curious, is go vet advanced enough now to warn about this non-Mutex race in the code somehow?

        1. 1

          it’s not so easy to find

          https://golang.org/pkg/sync/#Mutex

          A Mutex must not be copied after first use.

          1. 5

            My focus was not on the “Mutex must not be copied” (albeit I admit this is the title of the article), but on the fact that a value-receiver does copy the underlying object. Which per se is also arguably “obvious”, but the interaction between the two observations is, in my experience, much less “in the face” (in part because being somewhat non-local in the code). Also, I do concede that in my mind I already subconsciously extrapolated the article into the case described by Dave (that I linked above), which is “one level more” tricky, but I agree is not really what the OP article is about. Oops, sorry!

      2. 1

        What’s neat about Rust is that it’s basically impossible to get the version of this code that does the wrong thing in Go to compile in Rust:

        use std::collections::HashMap;
        
        #[derive(Debug)]
        struct Container {
            counters: HashMap<String, u32>
        }
        
        impl Container {
            fn inc(&mut self, name: &str) {
                let value = self.counters.get_mut(name).unwrap();
                *value += 1;
            }
        }
        
        fn main() {
            let mut c = Container { counters: HashMap::new() };
            c.counters.insert("a".to_string(), 0);
            c.counters.insert("b".to_string(), 0);
        
            let mut do_increment = |name: &str, n: u32| {
                for _ in 0..n {
                    c.inc(&name);
                }
            };
        
            do_increment("a", 100000);
        
            println!("{:#?}", c);
        }
        

        That compiles, because inc takes the Container instance as a mutable pointer. Take away the & from the inc declaration, and it won’t even compile because, as the compiler tells you, the first iteration of the loop in do_increment consumes the Container instance, and it can’t be used again, either by the loop or by the println.

        Getting the second version to compile in Rust is a bit more of a headache, but I don’t think it’s possible to get it to compile in such a way that it expresses the bug that was seen in the Go version.