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: copylocks false positives #16227

Closed
josharian opened this issue Jun 30, 2016 · 18 comments

Comments

Projects
None yet
6 participants
@josharian
Copy link
Contributor

commented Jun 30, 2016

Examples from the standard library that I believe were introduced between Go 1.6 and Go 1.7.

context/context.go:230: assignment copies lock value to c: context.cancelCtx contains sync.Mutex
context/context.go:236: newCancelCtx returns lock by value: context.cancelCtx contains sync.Mutex
context/context.go:375: literal copies lock value from newCancelCtx(parent): context.cancelCtx contains sync.Mutex

func WithCancel(parent Context) (ctx Context, cancel CancelFunc) {
    c := newCancelCtx(parent) // LINE 230
    propagateCancel(parent, &c)
    return &c, func() { c.cancel(true, Canceled) }
}

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

    c := &timerCtx{
        cancelCtx: newCancelCtx(parent), // LINE 375
        deadline:  deadline,
    }

cancelCtx is a struct type containing a sync.Mutex. But all of these are just fine--in newCancelCtx (line 236), the sync.Mutex is (implicitly) being set to its zero value, which is fine. And since it is the zero value, and these are used as initializations of new values (lines 230, 375), they are also ok.

go/internal/gcimporter/bimport.go:347: assignment copies lock value to *t: go/types.Struct contains sync.Once contains sync.Mutex

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

Types.NewStruct constructs a new struct and omits (implicitly zeros) the lock fields.

I think we should consider rolling back the new checks for 1.7 and restoring them in 1.8 without these false positives. At least in the case of struct initialization, ignoring fields being initialized to the zero value will help. I'm not sure how to detect copying of values that are known to contain zero-valued locks. Marking Go 1.7; Rob can adjust milestone from there.

cc @valyala @robpike

@josharian josharian added this to the Go1.7 milestone Jun 30, 2016

@robpike

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2016

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

@valyala

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2016

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.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2016

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.

@josharian

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2016

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?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2016

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.

@valyala

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2016

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.

@josharian

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2016

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

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2016

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

@valyala

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2016

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

@valyala

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2016

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.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2016

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

@josharian

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2016

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.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2016

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

Now we need to decide which approach to take.

@gopherbot

This comment has been minimized.

Copy link

commented Jul 1, 2016

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

@gopherbot

This comment has been minimized.

Copy link

commented Jul 1, 2016

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

@gopherbot

This comment has been minimized.

Copy link

commented Jul 1, 2016

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

gopherbot pushed a commit that referenced this issue Jul 4, 2016

cmd/vet: remove copylock warning about result types and calls
Don't issue a copylock warning about a result type; the function may
return a composite literal with a zero value, which is OK.

Don't issue a copylock warning about a function call on the RHS, or an
indirection of a function call; the function may return a composite
literal with a zero value, which is OK.

Updates #16227.

Change-Id: I94f0e066bbfbca5d4f8ba96106210083e36694a2
Reviewed-on: https://go-review.googlesource.com/24711
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2016

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

@ianlancetaylor ianlancetaylor modified the milestones: Go1.8, Go1.7 Jul 6, 2016

@quentinmit quentinmit added the NeedsFix label Oct 6, 2016

@josharian

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2016

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.