I agree that false positives are a bane, and lean towards rolling back the new checks until we can reduce their frequency.

The case with cancelCtx looks bogus, since it returns a struct containing lock by value. It may be easily fixed - see this CL.

The case with types.Struct is definitely bogus, since it overwrites lock value inside t by a new lock value:

*t = *types.NewStruct(p.fieldList(parent))

This case remains bogus even if the right hand side value contains newly created zero lock.

The cancelCtx code is safe, though.

func newCancelCtx(parent Context) cancelCtx {
    return cancelCtx{
        Context: parent,
        done:    make(chan struct{}),
    }
}

This always copies the zero value, which is always OK. You may not like the style, but it's not wrong. And your fix changes a struct field from cancelCtx to *cancelCtx, which requires an additional memory allocation. It's not good if the vet tool issues a warning on correct code for which avoiding the warning makes the code less efficient.

I think the types.Struct might be ok too. The code wants to abandon the old value of t, complete with the old lock contained in t, and replace it with a totally new one, which has its lock set to the zero value. What am I missing?

Note that of the three warnings from go vet context, one has been issued ever since 1.4:

context.go:236: newCancelCtx returns Lock by value: context.cancelCtx contains sync.Mutex

The other two are new on tip.

The code wants to abandon the old value of t, complete with the old lock contained in t, and replace it with a totally new one, which has its lock set to the zero value. What am I missing?

@josharian , the old lock may be still in use. Probably other cases are possible. cc'ing @dvyukov , who may know more about such cases.

@valyala there will always be stuff that vet doesn't catch. But false positives are worse than false negatives: They cause annoyance, they can cause people to write worse code to silence the tool, and worst of all, they can cause people to stop using the tool altogether. Unless the false positive rate is extremely low for a check, it's better for the check not to go in. I'd suggest that allowing assignment where the RHS is a struct literal in which all lock fields are zero is going to be a net win, taking into account both false positives and false negatives and their relative weight.

@josharian cmd/vet does already allow an assignment where the RHS is a struct literal. The warning in go/internal/gcimporter is a different case: an assignment where the RHS is an indirection of a function call.

I'd suggest that allowing assignment where the RHS is a struct literal in which all lock fields are zero is going to be a net win

go vet already handles this case :) The problem is that the cases above don't contain RHS struct literal - it is hidden behind a function

This always copies the zero value, which is always OK. You may not like the style, but it's not wrong. And your fix changes a struct field from cancelCtx to *cancelCtx, which requires an additional memory allocation. It's not good if the vet tool issues a warning on correct code for which avoiding the warning makes the code less efficient.

There is another easy way to fix the warning without losing the efficiency - see the CL.

@valyala I agree that that works, but I also agree with @josharian 's point: false positives from a tool like vet must be as low as possible.

cmd/vet does already allow an assignment where the RHS is a struct literal. The warning in go/internal/gcimporter is a different case: an assignment where the RHS is an indirection of a function call.

I see. I've been in the compiler too long--in my brain, that was getting inlined. If this is common enough, we could have vet do a bit of (semantic) inlining. But that's definitely way out of scope for 1.7.

I changed cmd/vet to avoid these cases in https://golang.org/cl/24711 .

Now we need to decide which approach to take.

CL https://golang.org/cl/24711 mentions this issue.

CL https://golang.org/cl/24693 mentions this issue.

CL https://golang.org/cl/24694 mentions this issue.

Moving any remaining work to 1.8. It's actually not clear to me what work remains, but I will leave that for @josharian .

I think we can call this closed. We'll open a new issue if new false positives arise.