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

proposal: cmd/vet: append with a single argument should be a go vet warning #21482

Closed
natefinch opened this Issue Aug 16, 2017 · 15 comments

Comments

Projects
None yet
10 participants
@natefinch
Contributor

natefinch commented Aug 16, 2017

What version of Go are you using (go version)?

go 1.9rc2

What operating system and processor architecture are you using (go env)?

OSX 64 bit

What did you do?

a := []int{1,2,3}
b := []int{4}
a = append(b)  // <------
fmt.Println(a)
// prints [4]

https://play.golang.org/p/wJZDoNLZeN

While this is not exactly surprising, it is a line of code that can be hard to catch in code reviews, and basically never makes sense to write. It is semantically equivalent to a = b

What did you expect to see?

it would be nice if this were caught with go vet

What did you see instead?

no warning from go vet

@natefinch

This comment has been minimized.

Show comment
Hide comment
@natefinch
Contributor

natefinch commented Aug 16, 2017

@ianlancetaylor ianlancetaylor changed the title from go/vet: append with a single argument should be a go vet warning to cmd/vet: append with a single argument should be a go vet warning Aug 16, 2017

@ianlancetaylor ianlancetaylor added this to the Go1.10 milestone Aug 16, 2017

@cespare

This comment has been minimized.

Show comment
Hide comment
@cespare

cespare Aug 16, 2017

Contributor

The frequency criterion is probably the questionable one here. We probably need some analysis of how often this occurs.

Contributor

cespare commented Aug 16, 2017

The frequency criterion is probably the questionable one here. We probably need some analysis of how often this occurs.

@robpike

This comment has been minimized.

Show comment
Hide comment
@robpike

robpike Aug 16, 2017

Contributor

As hinted at by cespare@, you should demonstrate the three points of cmd/vet/README: correctness, frequency, and precision. Correctness and precision look good by construction, but frequency is likely hard to demonstrate. Until now, I have never seen this issue in the wild.

Contributor

robpike commented Aug 16, 2017

As hinted at by cespare@, you should demonstrate the three points of cmd/vet/README: correctness, frequency, and precision. Correctness and precision look good by construction, but frequency is likely hard to demonstrate. Until now, I have never seen this issue in the wild.

@rwcarlsen

This comment has been minimized.

Show comment
Hide comment
@rwcarlsen

rwcarlsen Aug 16, 2017

Because making this mistake would likely make your code run clearly wrong - I think it is likely to not show up in committed code much in the wild - it is an issue people hunt around for and figure out before committing - vet could still save them time.

rwcarlsen commented Aug 16, 2017

Because making this mistake would likely make your code run clearly wrong - I think it is likely to not show up in committed code much in the wild - it is an issue people hunt around for and figure out before committing - vet could still save them time.

@dominikh

This comment has been minimized.

Show comment
Hide comment
@dominikh

dominikh Aug 16, 2017

Member

3 matches in the Go corpus.

Member

dominikh commented Aug 16, 2017

3 matches in the Go corpus.

@robpike

This comment has been minimized.

Show comment
Hide comment
@robpike

robpike Aug 17, 2017

Contributor

I think we can close this. It doesn't seem to be frequent enough, or otherwise uncatchable enough, to merit adding a special check to vet.

But I will leave it open a little longer in case anyone has a compelling counterargument.

Contributor

robpike commented Aug 17, 2017

I think we can close this. It doesn't seem to be frequent enough, or otherwise uncatchable enough, to merit adding a special check to vet.

But I will leave it open a little longer in case anyone has a compelling counterargument.

@robpike

This comment has been minimized.

Show comment
Hide comment
@robpike

robpike Aug 17, 2017

Contributor

Another possibility, triggered by a suggestion from the mailing list: The compiler already gives an error if append is called and the result thrown away. It is conceivable to add this case as a second error. I may be imaginative enough but I haven't come up with a case where append with one argument achieves something otherwise impossible or even difficult.

Educate me.

Contributor

robpike commented Aug 17, 2017

Another possibility, triggered by a suggestion from the mailing list: The compiler already gives an error if append is called and the result thrown away. It is conceivable to add this case as a second error. I may be imaginative enough but I haven't come up with a case where append with one argument achieves something otherwise impossible or even difficult.

Educate me.

@martisch

This comment has been minimized.

Show comment
Hide comment
@martisch

martisch Aug 17, 2017

Member

I had thought about making it a compile error too:

I think not allowing to leave out the variadic argument could violate the current language spec:
"The variadic function append appends zero or more values x to s of type S"
+
"A function with such a parameter is called variadic and may be invoked with zero or more arguments for that parameter"

So i think append would become inconsistent with other variadic functions and would need a definition change which i guess is not worth catching these cases.

Member

martisch commented Aug 17, 2017

I had thought about making it a compile error too:

I think not allowing to leave out the variadic argument could violate the current language spec:
"The variadic function append appends zero or more values x to s of type S"
+
"A function with such a parameter is called variadic and may be invoked with zero or more arguments for that parameter"

So i think append would become inconsistent with other variadic functions and would need a definition change which i guess is not worth catching these cases.

@dominikh

This comment has been minimized.

Show comment
Hide comment
@dominikh

dominikh Aug 17, 2017

Member

The check failed the three criteria for addition to vet. I would think that backwards incompatible compiler changes that add more special cases to the language would at least require passing the vet criteria.

Member

dominikh commented Aug 17, 2017

The check failed the three criteria for addition to vet. I would think that backwards incompatible compiler changes that add more special cases to the language would at least require passing the vet criteria.

@natefinch

This comment has been minimized.

Show comment
Hide comment
@natefinch

natefinch Aug 17, 2017

Contributor

I don't think it's worth changing the compiler and language spec, especially since as others have said, it doesn't appear to be a common mistake.

Contributor

natefinch commented Aug 17, 2017

I don't think it's worth changing the compiler and language spec, especially since as others have said, it doesn't appear to be a common mistake.

@robpike

This comment has been minimized.

Show comment
Hide comment
@robpike

robpike Aug 18, 2017

Contributor

These recent comments appear to miss the point I was making, although not making well. Append is already special in the compiler. It could be more special and this problem would go away, at essentially zero cost.

I am not arguing that we should do it, I'm just saying it would be easy and I think safe.

Contributor

robpike commented Aug 18, 2017

These recent comments appear to miss the point I was making, although not making well. Append is already special in the compiler. It could be more special and this problem would go away, at essentially zero cost.

I am not arguing that we should do it, I'm just saying it would be easy and I think safe.

@DrGo

This comment has been minimized.

Show comment
Hide comment
@DrGo

DrGo Aug 20, 2017

Upvoted robpike's suggestion. I was recently bitten by this one, so I am a bit sensitized. But I won't be terribly disappointed if it stayed as is.
Perhaps in Go 2.0, append() will get revamped, so reassigning to the same slice is no longer needed and this and other complaints about append() will go away.

DrGo commented Aug 20, 2017

Upvoted robpike's suggestion. I was recently bitten by this one, so I am a bit sensitized. But I won't be terribly disappointed if it stayed as is.
Perhaps in Go 2.0, append() will get revamped, so reassigning to the same slice is no longer needed and this and other complaints about append() will go away.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Aug 21, 2017

Contributor

This seems low frequency because almost no one is going to write this code and not notice that it doesn't work. But if vet were on by default as part of go test, the essentially 100% accuracy might suggest doing it, to help people more directly (instead of having to debug a test failure). But it sure doesn't seem to happen much at all.

Note that x = append(x) is a no-op and maybe reportable but x = append(y) is not a no-op and probably not worth reporting. Yes it's the same as x = y but so is x = y+0 and we don't report that.

go-corpus-0.01/src/go.uber.org/zap/zbark/debark_test.go has

for _, l := range append(levels) {

which looks weird but then elsewhere in the file it says:

for _, l := range append(levels, zap.PanicLevel, zap.FatalLevel) {

so it looks like maybe a placeholder.

go-corpus-0.01/src/github.com/openshift/origin/pkg/auth/ldaputil/query_test.go has:

testSearchRequest := &ldap.SearchRequest{
	BaseDN:       "dc=example,dc=com",
	Scope:        ldap.ScopeWholeSubtree,
	DerefAliases: int(DefaultDerefAliases),
	SizeLimit:    DefaultSizeLimit,
	TimeLimit:    DefaultTimeLimit,
	TypesOnly:    DefaultTypesOnly,
	Filter:       "(objectClass=*)",
	Attributes:   append(DefaultAttributes),
	Controls:     DefaultControls,
}

and

expectedRequest: &ldap.SearchRequest{
	BaseDN:       DefaultBaseDN,
	Scope:        int(DefaultScope),
	DerefAliases: int(DefaultDerefAliases),
	SizeLimit:    DefaultSizeLimit,
	TimeLimit:    DefaultTimeLimit,
	TypesOnly:    DefaultTypesOnly,
	Filter:       fmt.Sprintf("(&(%s)(%s=%s))", DefaultFilter, DefaultQueryAttribute, "bar"),
	Attributes:   append(DefaultAttributes, []string{"email", "phone"}...),
	Controls:     DefaultControls,
},

but again it looks like a kind of placeholder for things that might be appended later (or were appended before). People already get mad about taking unused imports out. Do we really want to start complaining about empty appends too?

At first I thought other code looked like maybe the author thinks append unconditionally copies its arguments, but each of the ones I looked at turned out to be like the above. (And in any case, that mistake can be made about both x = append(y) and x = append(y, y1), so this is not the right place to complain.)

Contributor

rsc commented Aug 21, 2017

This seems low frequency because almost no one is going to write this code and not notice that it doesn't work. But if vet were on by default as part of go test, the essentially 100% accuracy might suggest doing it, to help people more directly (instead of having to debug a test failure). But it sure doesn't seem to happen much at all.

Note that x = append(x) is a no-op and maybe reportable but x = append(y) is not a no-op and probably not worth reporting. Yes it's the same as x = y but so is x = y+0 and we don't report that.

go-corpus-0.01/src/go.uber.org/zap/zbark/debark_test.go has

for _, l := range append(levels) {

which looks weird but then elsewhere in the file it says:

for _, l := range append(levels, zap.PanicLevel, zap.FatalLevel) {

so it looks like maybe a placeholder.

go-corpus-0.01/src/github.com/openshift/origin/pkg/auth/ldaputil/query_test.go has:

testSearchRequest := &ldap.SearchRequest{
	BaseDN:       "dc=example,dc=com",
	Scope:        ldap.ScopeWholeSubtree,
	DerefAliases: int(DefaultDerefAliases),
	SizeLimit:    DefaultSizeLimit,
	TimeLimit:    DefaultTimeLimit,
	TypesOnly:    DefaultTypesOnly,
	Filter:       "(objectClass=*)",
	Attributes:   append(DefaultAttributes),
	Controls:     DefaultControls,
}

and

expectedRequest: &ldap.SearchRequest{
	BaseDN:       DefaultBaseDN,
	Scope:        int(DefaultScope),
	DerefAliases: int(DefaultDerefAliases),
	SizeLimit:    DefaultSizeLimit,
	TimeLimit:    DefaultTimeLimit,
	TypesOnly:    DefaultTypesOnly,
	Filter:       fmt.Sprintf("(&(%s)(%s=%s))", DefaultFilter, DefaultQueryAttribute, "bar"),
	Attributes:   append(DefaultAttributes, []string{"email", "phone"}...),
	Controls:     DefaultControls,
},

but again it looks like a kind of placeholder for things that might be appended later (or were appended before). People already get mad about taking unused imports out. Do we really want to start complaining about empty appends too?

At first I thought other code looked like maybe the author thinks append unconditionally copies its arguments, but each of the ones I looked at turned out to be like the above. (And in any case, that mistake can be made about both x = append(y) and x = append(y, y1), so this is not the right place to complain.)

@rsc rsc added the Proposal label Aug 21, 2017

@rsc rsc changed the title from cmd/vet: append with a single argument should be a go vet warning to proposal: cmd/vet: append with a single argument should be a go vet warning Aug 21, 2017

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Oct 9, 2017

Contributor

No updates since last comment, but the examples in the previous comment seem to suggest that this (append(x)) comes up in reasonable code and should not be rejected out of hand.

Closing per discussion with proposal review and comments above.

Contributor

rsc commented Oct 9, 2017

No updates since last comment, but the examples in the previous comment seem to suggest that this (append(x)) comes up in reasonable code and should not be rejected out of hand.

Closing per discussion with proposal review and comments above.

@rsc rsc closed this Oct 9, 2017

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